Add with overflow in breakpad_symbols with fuzzed symbol file

[5225225]

fn main() {
    let data = b"FUNC 0 0 0 x\nffffffffffffffff 1 0 0\n";
    breakpad_symbols::SymbolFile::from_bytes(data);
}

Good shout on the adding a fuzzer for this. I'll fix this tomorrow, with any luck. Also will add the fuzzer.

[5225225]

In release mode, this is a backwards range call to Range::new

That footgun has got us twice now (at least that I've seen, probably happened before in the past a lot more).

[luser]

Thanks! @Gankra has done a ton of great work making these crates production-ready but even that can't fix the fact that they started out as a hobby project and there were plenty of questionable implementation choices made along the way.

The RangeMap stuff is a bit of a sore spot. I remember being very frustrated at the time that I couldn't find a useful crate that provided the data structure I wanted, so the one I wound up using was a compromise. At this point, given the utility of these crates, it's worth exploring whether there's a better alternative. We could also explore forking the one we're currently using to make it more amenable to our use cases.

[Gankra]

Just quickly running through my bugmail and just wanting to confirm without digging into this immediately: the panic you're seeing isn't because the Range code got the lhs and rhs mixed up again, but rather that it's blindly feeding in values from the symbol file and tripping an inner assertion because the symbol file itself is backwards?

Definitely good to fix if so, great catch! This code should already have one or two places where we identify "corrupt" lines and discard them, so it should hopefully be pretty easy to add this case.

[5225225]

Ah, my bad, I should have explained more. The issue is the line here:

https://github.com/luser/rust-minidump/blob/4c73bbafb3be526b6ce989d48cc000b590b465d7/breakpad-symbols/src/sym_file/parser.rs#L511-L525

Specifically the addition on line 518. If l.address + l.size overflows, then you can be constructing a backwards range. If address is u64::MAX, and size is 2, then you'll be making a Range::new(u64::MAX, 0). (2 instead of 1 because we subtract 1).

Fix here is to check for l.size > 0 and l.address + (l.size - 1) not overflowing. Should be reasonably simple, in any case.

[Gankra]

Ohh sorry I completely glossed over the issue title! Yeah I think discarding any line like this as corrupt is probably the more practical solution (alternatively we could use saturating_add to try to make it vaguely work, but you still have size==0 issues).

[luser]

alternatively we could use saturating_add to try to make it vaguely work

I'd argue that this should probably just use checked_add and drop entries that would overflow. (IRL this should never ever happen anyway since these are RVAs, offsets from the base address of the module.)