fix for empty string table parsing in COFF files. fixes issue #502. #503
Conversation
glslang
force-pushed
the
issue_502
branch
2 times, most recently
from
3556e32 to
f0ba6de
Compare
kkent030315
left a comment
kkent030315
left a comment
There was a problem hiding this comment.
Perfect PR. LGTM, Thank you!
|
|
||
| // Handle completely invalid offset | ||
| if offset >= bytes.len() { | ||
| if offset > bytes.len() { |
There was a problem hiding this comment.
sorry doesn't this mean offset is pointing to bytes.len() in some cases, which is an OOB if we did bytes[offset] ?
There was a problem hiding this comment.
The code at https://github.com/m4b/goblin/blob/master/src/pe/header.rs#L870 adds the length to the offset which makes an empty string table offset to be equal to bytes.len().
The test succeeds and then an empty slice is created at https://github.com/m4b/goblin/blob/master/src/strtab.rs#L62 which should be safe.
I think offset can only be equal to bytes.len() when len is zero, as per https://github.com/m4b/goblin/blob/master/src/pe/header.rs#L862
Alternatively, in header.rs it could be parsed_with_opts and make it permissive.
There was a problem hiding this comment.
Could you add a version (maybe shorter/more condensed) of this reply as a comment above that check? I feel like in the future someone may suspect this was a typical off by one error but apparently it’s a quirk to parsing
3 participants
No description provided.