[compiler-rt] 62a6d63 - [Sanitizers][Darwin] Correct iterating of MachO load commands (#130161)
via llvm-commits
llvm-commits at lists.llvm.org
Sun Mar 9 10:58:49 PDT 2025
Author: Mariusz Borsa
Date: 2025-03-09T10:58:46-07:00
New Revision: 62a6d63d634d816a7d73ab7735176ad05b118c79
URL: https://github.com/llvm/llvm-project/commit/62a6d63d634d816a7d73ab7735176ad05b118c79
DIFF: https://github.com/llvm/llvm-project/commit/62a6d63d634d816a7d73ab7735176ad05b118c79.diff
LOG: [Sanitizers][Darwin] Correct iterating of MachO load commands (#130161)
The condition to stop iterating so far was to look for load command cmd
field == 0. The iteration would continue past the commands area, and
would finally find lc->cmd ==0, if lucky. Or crash with bus error, if
out of luck.
Correcting this by limiting the number of iterations to the count
specified in mach_header(_64) ncmds field.
rdar://143903403
---------
Co-authored-by: Mariusz Borsa <m_borsa at apple.com>
Added:
Modified:
compiler-rt/lib/sanitizer_common/sanitizer_procmaps_mac.cpp
compiler-rt/lib/sanitizer_common/tests/sanitizer_procmaps_mac_test.cpp
Removed:
################################################################################
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_procmaps_mac.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_procmaps_mac.cpp
index 5ff8d1832556f..a9533d6fc04ca 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_procmaps_mac.cpp
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_procmaps_mac.cpp
@@ -334,9 +334,22 @@ static const load_command *NextCommand(const load_command *lc) {
return (const load_command *)((const char *)lc + lc->cmdsize);
}
-static void FindUUID(const load_command *first_lc, u8 *uuid_output) {
- for (const load_command *lc = first_lc; lc->cmd != 0; lc = NextCommand(lc)) {
- if (lc->cmd != LC_UUID) continue;
+# ifdef MH_MAGIC_64
+static constexpr size_t header_size = sizeof(mach_header_64);
+# else
+static constexpr size_t header_size = sizeof(mach_header);
+# endif
+
+static void FindUUID(const load_command *first_lc, const mach_header *hdr,
+ u8 *uuid_output) {
+ uint32_t curcmd = 0;
+ for (const load_command *lc = first_lc; curcmd < hdr->ncmds;
+ curcmd++, lc = NextCommand(lc)) {
+ CHECK_LT((const char *)lc,
+ (const char *)hdr + header_size + hdr->sizeofcmds);
+
+ if (lc->cmd != LC_UUID)
+ continue;
const uuid_command *uuid_lc = (const uuid_command *)lc;
const uint8_t *uuid = &uuid_lc->uuid[0];
@@ -345,9 +358,16 @@ static void FindUUID(const load_command *first_lc, u8 *uuid_output) {
}
}
-static bool IsModuleInstrumented(const load_command *first_lc) {
- for (const load_command *lc = first_lc; lc->cmd != 0; lc = NextCommand(lc)) {
- if (lc->cmd != LC_LOAD_DYLIB) continue;
+static bool IsModuleInstrumented(const load_command *first_lc,
+ const mach_header *hdr) {
+ uint32_t curcmd = 0;
+ for (const load_command *lc = first_lc; curcmd < hdr->ncmds;
+ curcmd++, lc = NextCommand(lc)) {
+ CHECK_LT((const char *)lc,
+ (const char *)hdr + header_size + hdr->sizeofcmds);
+
+ if (lc->cmd != LC_LOAD_DYLIB)
+ continue;
const dylib_command *dylib_lc = (const dylib_command *)lc;
uint32_t dylib_name_offset = dylib_lc->dylib.name.offset;
@@ -393,10 +413,10 @@ bool MemoryMappingLayout::Next(MemoryMappedSegment *segment) {
continue;
}
}
- FindUUID((const load_command *)data_.current_load_cmd_addr,
+ FindUUID((const load_command *)data_.current_load_cmd_addr, hdr,
data_.current_uuid);
data_.current_instrumented = IsModuleInstrumented(
- (const load_command *)data_.current_load_cmd_addr);
+ (const load_command *)data_.current_load_cmd_addr, hdr);
}
while (data_.current_load_cmd_count > 0) {
diff --git a/compiler-rt/lib/sanitizer_common/tests/sanitizer_procmaps_mac_test.cpp b/compiler-rt/lib/sanitizer_common/tests/sanitizer_procmaps_mac_test.cpp
index f622bba246309..7547528e2adb8 100644
--- a/compiler-rt/lib/sanitizer_common/tests/sanitizer_procmaps_mac_test.cpp
+++ b/compiler-rt/lib/sanitizer_common/tests/sanitizer_procmaps_mac_test.cpp
@@ -37,16 +37,15 @@ class MemoryMappingLayoutMock final : public MemoryMappingLayout {
.uuid = {}
};
- static constexpr char dylib_name[] = "libclang_rt.\0\0\0"; // 8 bytes aligned, padded with zeros per loader.h
+ static constexpr char libclang_rt_dylib_name[] =
+ "libclang_rt.\0\0\0"; // 8 bytes aligned, padded with zeros per loader.h
+ static constexpr char uninstrumented_dylib_name[] =
+ "uninst___rt.\0\0\0"; // 8 bytes aligned, padded with zeros per loader.h
+
static constexpr dylib_command mock_dylib_command = {
- .cmd = LC_LOAD_DYLIB,
- .cmdsize = sizeof(dylib_command) + sizeof(dylib_name),
- .dylib = {
- .name = {
- .offset = sizeof(dylib_command)
- }
- }
- };
+ .cmd = LC_LOAD_DYLIB,
+ .cmdsize = sizeof(dylib_command) + sizeof(libclang_rt_dylib_name),
+ .dylib = {.name = {.offset = sizeof(dylib_command)}}};
static constexpr uuid_command mock_trap_command = {
.cmd = LC_UUID,
@@ -59,51 +58,57 @@ class MemoryMappingLayoutMock final : public MemoryMappingLayout {
std::vector<unsigned char> mock_header;
public:
- MemoryMappingLayoutMock(): MemoryMappingLayout(false) {
- EXPECT_EQ(mock_uuid_command.cmdsize % 8, 0u);
- EXPECT_EQ(mock_dylib_command.cmdsize % 8, 0u);
-
- Reset();
-
-#ifdef MH_MAGIC_64
- const struct mach_header_64 *header = (mach_header_64 *)_dyld_get_image_header(0); // Any header will do
- const size_t header_size = sizeof(mach_header_64);
-#else
- const struct mach_header *header = _dyld_get_image_header(0);
- const size_t header_size = sizeof(mach_header);
-#endif
- const size_t mock_header_size_with_extras = header_size + header->sizeofcmds +
- mock_uuid_command.cmdsize + mock_dylib_command.cmdsize + sizeof(uuid_command);
-
- mock_header.reserve(mock_header_size_with_extras);
- // Copy the original header
- copy((unsigned char *)header,
- (unsigned char *)header + header_size + header->sizeofcmds,
- back_inserter(mock_header));
- // The following commands are not supposed to be processed
- // by the (correct) ::Next method at all, since they're not
- // accounted for in header->ncmds .
- copy((unsigned char *)&mock_uuid_command,
- ((unsigned char *)&mock_uuid_command) + mock_uuid_command.cmdsize,
- back_inserter(mock_header));
- copy((unsigned char *)&mock_dylib_command,
- ((unsigned char *)&mock_dylib_command) + sizeof(dylib_command), // as mock_dylib_command.cmdsize contains the following string
- back_inserter(mock_header));
- copy((unsigned char *)dylib_name,
- ((unsigned char *)dylib_name) + sizeof(dylib_name),
- back_inserter(mock_header));
-
- // Append a command w. huge size to have the test detect the read overrun
- copy((unsigned char *)&mock_trap_command,
- ((unsigned char *)&mock_trap_command) + sizeof(uuid_command),
- back_inserter(mock_header));
-
- start_load_cmd_addr = (const char *)(mock_header.data() + header_size);
- sizeofcmds = header->sizeofcmds;
-
- const char *last_byte_load_cmd_addr = (start_load_cmd_addr+sizeofcmds-1);
- data_.current_image = -1; // So the loop in ::Next runs just once
- }
+ MemoryMappingLayoutMock(bool instrumented) : MemoryMappingLayout(false) {
+ EXPECT_EQ(mock_uuid_command.cmdsize % 8, 0u);
+ EXPECT_EQ(mock_dylib_command.cmdsize % 8, 0u);
+
+ Reset();
+
+# ifdef MH_MAGIC_64
+ const struct mach_header_64 *header =
+ (mach_header_64 *)_dyld_get_image_header(0); // Any header will do
+ const size_t header_size = sizeof(mach_header_64);
+# else
+ const struct mach_header *header = _dyld_get_image_header(0);
+ const size_t header_size = sizeof(mach_header);
+# endif
+ const size_t mock_header_size_with_extras =
+ header_size + header->sizeofcmds + mock_uuid_command.cmdsize +
+ mock_dylib_command.cmdsize + sizeof(uuid_command);
+
+ mock_header.reserve(mock_header_size_with_extras);
+ // Copy the original header
+ copy((unsigned char *)header,
+ (unsigned char *)header + header_size + header->sizeofcmds,
+ back_inserter(mock_header));
+ // The following commands are not supposed to be processed
+ // by the (correct) ::Next method at all, since they're not
+ // accounted for in header->ncmds .
+ copy((unsigned char *)&mock_uuid_command,
+ ((unsigned char *)&mock_uuid_command) + mock_uuid_command.cmdsize,
+ back_inserter(mock_header));
+ copy((unsigned char *)&mock_dylib_command,
+ ((unsigned char *)&mock_dylib_command) +
+ sizeof(dylib_command), // as mock_dylib_command.cmdsize contains
+ // the following string
+ back_inserter(mock_header));
+ const char(&dylib_name)[16] =
+ instrumented ? libclang_rt_dylib_name : uninstrumented_dylib_name;
+ copy((unsigned char *)dylib_name,
+ ((unsigned char *)dylib_name) + sizeof(dylib_name),
+ back_inserter(mock_header));
+
+ // Append a command w. huge size to have the test detect the read overrun
+ copy((unsigned char *)&mock_trap_command,
+ ((unsigned char *)&mock_trap_command) + sizeof(uuid_command),
+ back_inserter(mock_header));
+
+ start_load_cmd_addr = (const char *)(mock_header.data() + header_size);
+ sizeofcmds = header->sizeofcmds;
+
+ const char *last_byte_load_cmd_addr = (start_load_cmd_addr + sizeofcmds - 1);
+ data_.current_image = -1; // So the loop in ::Next runs just once
+ }
size_t SizeOfLoadCommands() {
return sizeofcmds;
@@ -120,8 +125,20 @@ class MemoryMappingLayoutMock final : public MemoryMappingLayout {
}
};
-TEST(MemoryMappingLayout, Next) {
- __sanitizer::MemoryMappingLayoutMock memory_mapping;
+TEST(MemoryMappingLayout, NextInstrumented) {
+ __sanitizer::MemoryMappingLayoutMock memory_mapping(true);
+ __sanitizer::MemoryMappedSegment segment;
+ size_t size = memory_mapping.SizeOfLoadCommands();
+ while (memory_mapping.Next(&segment)) {
+ size_t offset = memory_mapping.CurrentLoadCommandOffset();
+ EXPECT_LE(offset, size);
+ }
+ size_t final_offset = memory_mapping.CurrentLoadCommandOffset();
+ EXPECT_EQ(final_offset, size); // All commands processed, no more, no less
+}
+
+TEST(MemoryMappingLayout, NextUnInstrumented) {
+ __sanitizer::MemoryMappingLayoutMock memory_mapping(false);
__sanitizer::MemoryMappedSegment segment;
size_t size = memory_mapping.SizeOfLoadCommands();
while (memory_mapping.Next(&segment)) {
More information about the llvm-commits
mailing list