m4b / goblin

An impish, cross-platform binary parsing crate, written in Rust
MIT License
1.2k stars 160 forks source link

Avoid overflow when header.n_namesz is 0 #256

Closed glandium closed 3 years ago

glandium commented 3 years ago

Note the overflow only crashes on debug builds.

m4b commented 3 years ago

the only thing that worries me slightly is the offset is returned:

            Ok((Note {
                name,
                desc,
                n_type: header.n_type,
            }, *offset))

so we did technically increment it by 1 even if we got empty strings and empty desc, so their len is all 0, but we manually bump by +1 for the null terminator. now i'm kind of thinking we shouldn't bump by +1?

I can't remember offhand what's after notes, i think they're just byte streams in the file and the offset isn't used in gread/pread chaining; iirc, each note has a particular offset and goblin preads it out from there?

if not, and the notes are read by gread chaining with offsets, then the +1 could be trouble, i'm not sure. this is definitely a weird corner case hah

m4b commented 3 years ago

So like, iiuc, your binary has a note section with sh_offset, but the notes have empty names? lol

glandium commented 3 years ago

The new version of the patch doesn't make it increase the offset. The binary is the in-memory vdso (we use goblin to generate crash reports in Firefox, that's part of what we dump).

m4b commented 3 years ago

do you need a new version immediately? there's a minor breaking change (#253) that will be merged for returning option for file size ranges, i can bump patch version and push this before merging that

glandium commented 3 years ago

We're actually on 0.1 currently for some reason, so a bump in version number is not affecting us more than upgrading to 0.3 would already do.

m4b commented 3 years ago

well i published 0.3.2 anyway with this change, so it's there now; thanks for the patch!