Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -712,6 +712,7 @@ if (BUILD_TESTS)

# wabt-unittests
set(UNITTESTS_SRCS
src/binary-reader-objdump.cc
src/test-binary-reader.cc
src/test-interp.cc
src/test-intrusive-list.cc
Expand Down
2 changes: 1 addition & 1 deletion src/binary-reader-objdump.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2161,7 +2161,7 @@ Result BinaryReaderObjdump::OnDylinkNeeded(std::string_view so_name) {
}

Result BinaryReaderObjdump::OnRelocCount(Index count, Index section_index) {
BinaryReaderObjdumpBase::OnRelocCount(count, section_index);
CHECK_RESULT(BinaryReaderObjdumpBase::OnRelocCount(count, section_index));
PrintDetails(" - relocations for section: %d (" PRIstringview ") [%d]\n",
section_index,
WABT_PRINTF_STRING_VIEW_ARG(GetSectionName(section_index)),
Expand Down
37 changes: 37 additions & 0 deletions src/test-binary-reader.cc
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include "gtest/gtest.h"

#include "wabt/binary-reader-nop.h"
#include "wabt/binary-reader-objdump.h"
#include "wabt/binary-reader.h"
#include "wabt/leb128.h"
#include "wabt/opcode.h"
Expand Down Expand Up @@ -73,3 +74,39 @@ TEST(BinaryReader, DisabledOpcodes) {
<< "Got error message: " << message;
}
}

TEST(BinaryReaderObjdump, RelocInvalidSectionIndex) {
// Minimal wasm with a reloc section referencing a section_index that exceeds
// the actual number of sections. Before the fix, the derived-class
// OnRelocCount ignored the error returned by the base class and proceeded
// to call GetSectionName with BinarySection::Invalid, causing an
// out-of-bounds read on the section_starts_ array.
//
// The fix propagates the base class error via CHECK_RESULT so that the
// out-of-bounds GetSectionName call is never reached. The overall result
// is still Ok because custom section errors are not fatal by default.

uint8_t data[] = {
0x00, 0x61, 0x73, 0x6d, 0x01, 0x00, 0x00, 0x00, // magic + version

// Custom section pretending to be "reloc." (section id 0)
0x00, // section code: custom
0x0a, // section size: 10 bytes
// section name "reloc." (length-prefixed)
0x06, // name length
'r', 'e', 'l', 'o', 'c', '.', 0xff,
0x01, // section_index = 255 (invalid, LEB128)
0x00, // relocation count = 0
};

ObjdumpOptions options;
memset(&options, 0, sizeof(options));
options.mode = ObjdumpMode::Details;
options.details = true;
ObjdumpState state;

// Should not crash. Custom section errors are suppressed, so the overall
// result is Ok even though the reloc section itself fails.
Result result = ReadBinaryObjdump(data, sizeof(data), &options, &state);
EXPECT_EQ(Result::Ok, result);
}
1 change: 0 additions & 1 deletion test/binary/bad-relocs.txt
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ Section Details:

Custom:
- name: "reloc.BAD"
- relocations for section: 99 () [0]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


Code Disassembly:

Expand Down
Loading