fix: Match the complete BcSymbolMap header (NATIVE-281)

[Swatinem]

Previously, the match was truncated to the input buffer length, which meant that
empty files always passed the check, which they obviously should not.

[flub]

I can't request changes anymore... but I fear this breaks things

[flub]

IIRC it is important that this does pass the test, I suspect if sentry-cli adopts this you won't be able to upload any bcsymbolmaps anymore.

[flub]

likely this 20 bytes was therefore also not an accidental number

[Swatinem]

Looking at this in more detail, I see two code paths calling BcSymbolMap::test/open in sentry-cli, both get a ByteView, which is either mmaped via .open() or read from a zip stream via .read() which according to its docs exhausts the whole read stream.

[Swatinem]

Either way, we can also back out "half" of the change, meaning: Allow truncating down to ~20bytes. Archive::peek makes a cut at 16 bytes btw.

[flub]

Archive::peek makes a cut at 16 bytes btw.

Is this what I recall then? If sentry-cli looks inside an archive this test only gets called with 16 bytes? That would still mean we want it to consider it a match if there are fewer bytes matching.

IIRC this test method is supposed to be best effort, so if you only provide a slice of say 4 bytes and they match the first 4 bytes of our prefix it should still say "this could be a bcsymbolmap". though obviously doing this at 0 bytes is taking it too far :)

[Swatinem]

Oh what I meant to refer to was this:

symbolic/symbolic-debuginfo/src/object.rs

Lines 127 to 129 in 10ccd26

	if data.len() < 16 {
	return FileFormat::Unknown;
	}

Symbolic needs at least 16 bytes to actually detect anything. It does not truncate, only checks for that.

Other longer file headers are also checked for precisely:

symbolic/symbolic-debuginfo/src/pdb.rs

Line 127 in 10ccd26

data.starts_with(MAGIC_BIG)

[flub]

hum, did some spelunking in the sentry-cli codebase as well and I can't find a reason for the requirement to be able to do partial matches, though I'm like 100% sure this was a requirement and that this was done explicitly like this before. I think these API's are a bit meh as they are, they require a "large enough" slice but you're left not knowing what "large enough" means is as a caller. So whatever, I guess it's fine, thanks for having a look as well.

[Swatinem]

Hm, maybe we should have a look at the sentry codebase itself, as it pulls in parts of the symbolic python API.

[flub]

good point, though it seems the test methods aren't even exposed on the python API and sentry re-implements this: https://github.com/getsentry/sentry/blob/master/src/sentry/models/debugfile.py#L421-L433

[Swatinem]

👍🏻 thanks for looking into this, in which case I think we are good.