Skip to content
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

Check for invalid record count in mac stream #406

Merged
merged 3 commits into from
Jan 31, 2022

Conversation

5225225
Copy link
Contributor

@5225225 5225225 commented Jan 26, 2022

Found with #405 (But I rebased this to not include that commit so it doesn't matter which one of the two is merged first).

@5225225
Copy link
Contributor Author

5225225 commented Jan 27, 2022

Hmm... Do we want to be calling dumps with an incorrect number of records invalid? Or should we do header.records.iter().take(header.record_count), perhaps?

Both is better than crashing, but up to whoever looks at this what I should do. Both work.

@luser
Copy link
Collaborator

luser commented Jan 27, 2022

Hmm... Do we want to be calling dumps with an incorrect number of records invalid? Or should we do header.records.iter().take(header.record_count), perhaps?

It's tricky, for sure. This crate has historically erred on the side of "get as much data as possible out of malformed minidumps", because our experience dealing with Firefox minidumps from end-users showed that we'd get dumps that were broken in all sorts of fascinating ways and getting something out of them was better than nothing. (Especially since Firefox exposes crash reports to enthusiastic end-users via about:crashes. There's nothing more demoralizing than having your browser crash, going to look up your crash report, and finding that it has no useful information because the crash broke things in a very specific way.)

@5225225
Copy link
Contributor Author

5225225 commented Jan 27, 2022

Okay, I'll take the first header.record_count

Actually, why is there a record count anyways? Surely we already parsed some number of records and put them in header.records?

@5225225
Copy link
Contributor Author

5225225 commented Jan 27, 2022

Ahhh, records is a fixed length array of records, and the ones past the record count is uninit (Well, presumably Default::default() because it's not wrapped in MaybeUninit, but semantically uninit).

Makes sense, avoids allocation. So if the record count is over 20, everything gets returned.

minidump/src/minidump.rs Show resolved Hide resolved
@5225225
Copy link
Contributor Author

5225225 commented Jan 31, 2022

Rebased ontop of master to resolve the merge conflict. Other than that, left it untouched.

@Gankra Gankra merged commit d34bcc0 into rust-minidump:master Jan 31, 2022
@Gankra
Copy link
Collaborator

Gankra commented Jan 31, 2022

thank you so much!! https://twitter.com/Gankra_/status/1488254127633227780

@5225225
Copy link
Contributor Author

5225225 commented Jan 31, 2022

awww that's made my day <3

Yep, I've not found any security vulns (other than DoS, i mean actual UB) in rust code through fuzzing

But I find a absolute ton of logic bugs and unexpected panics

The spiciest issues I tend to see are infinite loops in parsers and stack overflows (which kill a whole process, and catch_unwind will not save you >:))

@5225225 5225225 deleted the invalid-record-count branch January 31, 2022 21:08
@Gankra
Copy link
Collaborator

Gankra commented Jan 31, 2022

I like to think I've been reasonably good at enumerating and testing corner cases (at least you're hitting assertions!) but yeah it's extremely hard to think of them all when you are literally writing "the thing that runs when a program has encountered an unrecoverable corner case" and at some point you get tired of coming up with contrived inputs that "surely will never happen in practice anyway, why am I even bothering".

Nice to have a machine look for them instead! 😸

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants