-
Notifications
You must be signed in to change notification settings - Fork 158
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Avoid overflow when header.n_namesz is 0 #256
Conversation
Note the overflow only crashes on debug builds. |
9163d15
to
6f54fca
Compare
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 |
So like, iiuc, your binary has a note section with sh_offset, but the notes have empty names? lol |
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). |
*offset += 1; | ||
let name = bytes.gread_with::<&'a str>(offset, ctx::StrCtx::Length(header.n_namesz.saturating_sub(1)))?; | ||
if (header.n_namesz > 0) { | ||
*offset += 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup i think this is the right thing to do, the offset is used in subsequent notes, via NoteIterator
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 |
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. |
well i published 0.3.2 anyway with this change, so it's there now; thanks for the patch! |
No description provided.