Fix out-of-bounds read in objdump reloc section handling

[sumleo]

Summary

• BinaryReaderObjdump::OnRelocCount ignored the error returned by BinaryReaderObjdumpBase::OnRelocCount when the section_index was invalid, causing it to proceed to GetSectionName → GetSectionStart with BinarySection::Invalid (~0), resulting in an out-of-bounds read on the stack-allocated section_starts_ array of size kBinarySectionCount (14).
• Propagate the error via CHECK_RESULT so the out-of-bounds access is never reached.

Details

In binary-reader-objdump.cc, BinaryReaderObjdump::OnRelocCount calls the base class method but discards its return value:

Result BinaryReaderObjdump::OnRelocCount(Index count, Index section_index) {
  BinaryReaderObjdumpBase::OnRelocCount(count, section_index); // return value ignored
  PrintDetails("  - relocations for section: %d (%s) [%d]\n",
               section_index,
               GetSectionName(section_index),  // proceeds despite error
               count);
  return Result::Ok;
}

The base class returns Result::Error for out-of-range indices and sets reloc_section_ = BinarySection::Invalid. Later, OnReloc calls GetSectionStart(reloc_section_) which indexes section_starts_[~0] — far past the 14-element array.

Test plan

• Existing test/binary/bad-relocs.txt covers the invalid section index case — updated expected output to reflect that the relocation details line is no longer printed
• All existing tests pass

[zherczeg]

This patch looks valid.

Binary tests usually uses this file format:
https://github.com/WebAssembly/wabt/blob/main/test/binary/bad-alignment.txt
As far as I remember, not everything can be described though.

[sbc100]

It seems like this existing test should already have triggered the OOB access since there is no section 99 right?

Maybe a better fix would be to have GetSectionName check its argument and return the string <INVALID_SECTION>?

In any case, I would hope that it would be enough to modify this existing test which seems like it already covers the invalid section index case

[sumleo]

You're right — the existing bad-relocs.txt test already covers the invalid section index case. I've rebased onto main, dropped the C++ unit test, and simplified the fix to just the one-line CHECK_RESULT change plus updating the existing test expectation. The relocation details line is now suppressed for invalid indices since the error is properly propagated.

[sbc100]

Would changing the 99 to 0xff here not make this test basically the same as the new one added above?