PE: fix base relocation parser panic

[kkent030315]

• Fixes base relocation parser panicking on malformed binaries.

Found the issue on the same binary in PR below.

• fix: Prevent panic in TLS parser when bounds are invalid #458 (comment)

Malformed entity: tls raw data offset (0xae400) and size (0x10) greater than byte slice len (0x415eb)

The base relocation directory 0xB1000..size(0xADAC) does not exists in the binary.

1, .nsp0, 0x400, 0x400, 0, 0x1000, 0xc7000, 792 kB, Code, Initialized data, Executable, Readable, Writeable (0xe0000060), , , , 
2, .nsp1, 0x400, 0x4155a, 260.34 kB, 0xc7000, 0x109000, 264 kB, Code, Initialized data, Executable, Readable, Writeable (0xe0000060), 6e162fbb8f934b424d0f8c91d9a7d78b, 7.98, 6144:3J4KYvxupv2mlkmJhJP7JC9AnruTag4lsFItEnYFupq4gI0:54K+xusn9ArCag9FItEnmupq4gI0, T1E84423B7321D5F00E44C74FBFADD2733B6E7C9F3158AA34A1A612488BE759801DA1167
3, .nsp2, 0x400, 0x400, 0, 0x109000, 0x109562, 1.35 kB, Code, Initialized data, Executable, Readable, Writeable (0xe0000060), , , , 

N.B.
This is the super "special" case where only supposed to happen in this sort of badly "packed" binaries. Proprietary linkers would usually not produce such a completely broken binaries.

[m4b]

One worry j have is proliferation of these flags so that it overwhelms the user or has weird hard to debug affects if multiple settings are changed and they affect parsing in various hard to understand ways, but I don’t think we’re there yet (or ever will be?)

I would like to understand why the te has this set to false otherwise seems reasonable !

[m4b]

It would be maybe more idiomatic to write something like:

relocation_data = get_base_relocation_data().map(|dir|
RelocationData::parse(bytes, dir, etc))

Sorry for not complete example, on phone. But I think you get the gist of using map here and assigning to relocation data so the Some wraps disappear.

[kkent030315]

One worry j have is proliferation of these flags so that it overwhelms the user or has weird hard to debug affects if multiple settings are changed and they affect parsing in various hard to understand ways, but I don’t think we’re there yet (or ever will be?)

I would like to understand why the te has this set to false otherwise seems reasonable !

IMO returning an empty basereloc table in the case of the malformation would be an alternate option, but frankly, I don't like such a fail-silence design that makes debugging harder. However while I don't think basereloc malformation error is ever "useful."

Should we stand by parse_basereloc flag?

[kkent030315]

i.e., most PE parsers ignore basereloc malformation and are returning empty info

[chf0x]

How about we just get .reloc size from the SectionTable when reported size in DIRECTORY is invalid?
Something like this, works well for me: chf0x@3b46c38

[kkent030315]

How about we just get .reloc size from the SectionTable when reported size in DIRECTORY is invalid? Something like this, works well for me: chf0x@3b46c38

Hi there. Thank you for your proposal. If I get your solution correct, you're ignoring the invalid range of bytes while parsing the other. That would be an option too but my concern still stands by. The concern is that whether ignoring malformations in such a way isn't a traditional way in goblin. As I mentioned previously, yet it is totally doable particularly this case however.

And specifically for the binary in question, the base relocation directory is completely wrong even if some partial bytes point to the correct range of bytes. This is also my concern if this is confusing to consumers.

[kkent030315]

@m4b I reverted the change that introduces parse_basereloc: bool option in ParseOptions. Instead, it should store an empty bytes that implies no-op (so that consumers can parse the rest of binary). Is this acceptable solution?

[kkent030315]

The CI failure is not relevant to this change it seems (it is in ELF parser)

[m4b]

can this be rebased, the CI failure was fixed

[kkent030315]

@m4b Done!

[m4b]

LGTM, thank you!

[m4b]

NB: non-breaking