Skip to content

Commit 62a6d63

Browse files
wrotkiMariusz Borsa
wrotki
and
Mariusz Borsa
authored
[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 <[email protected]>
1 parent df570da commit 62a6d63

File tree

2 files changed

+101
-64
lines changed

2 files changed

+101
-64
lines changed

‎compiler-rt/lib/sanitizer_common/sanitizer_procmaps_mac.cpp‎

Lines changed: 28 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -334,9 +334,22 @@ static const load_command *NextCommand(const load_command *lc) {
334334
return (const load_command *)((const char *)lc + lc->cmdsize);
335335
}
336336

337-
static void FindUUID(const load_command *first_lc, u8 *uuid_output) {
338-
for (const load_command *lc = first_lc; lc->cmd != 0; lc = NextCommand(lc)) {
339-
if (lc->cmd != LC_UUID) continue;
337+
# ifdef MH_MAGIC_64
338+
static constexpr size_t header_size = sizeof(mach_header_64);
339+
# else
340+
static constexpr size_t header_size = sizeof(mach_header);
341+
# endif
342+
343+
static void FindUUID(const load_command *first_lc, const mach_header *hdr,
344+
u8 *uuid_output) {
345+
uint32_t curcmd = 0;
346+
for (const load_command *lc = first_lc; curcmd < hdr->ncmds;
347+
curcmd++, lc = NextCommand(lc)) {
348+
CHECK_LT((const char *)lc,
349+
(const char *)hdr + header_size + hdr->sizeofcmds);
350+
351+
if (lc->cmd != LC_UUID)
352+
continue;
340353

341354
const uuid_command *uuid_lc = (const uuid_command *)lc;
342355
const uint8_t *uuid = &uuid_lc->uuid[0];
@@ -345,9 +358,16 @@ static void FindUUID(const load_command *first_lc, u8 *uuid_output) {
345358
}
346359
}
347360

348-
static bool IsModuleInstrumented(const load_command *first_lc) {
349-
for (const load_command *lc = first_lc; lc->cmd != 0; lc = NextCommand(lc)) {
350-
if (lc->cmd != LC_LOAD_DYLIB) continue;
361+
static bool IsModuleInstrumented(const load_command *first_lc,
362+
const mach_header *hdr) {
363+
uint32_t curcmd = 0;
364+
for (const load_command *lc = first_lc; curcmd < hdr->ncmds;
365+
curcmd++, lc = NextCommand(lc)) {
366+
CHECK_LT((const char *)lc,
367+
(const char *)hdr + header_size + hdr->sizeofcmds);
368+
369+
if (lc->cmd != LC_LOAD_DYLIB)
370+
continue;
351371

352372
const dylib_command *dylib_lc = (const dylib_command *)lc;
353373
uint32_t dylib_name_offset = dylib_lc->dylib.name.offset;
@@ -393,10 +413,10 @@ bool MemoryMappingLayout::Next(MemoryMappedSegment *segment) {
393413
continue;
394414
}
395415
}
396-
FindUUID((const load_command *)data_.current_load_cmd_addr,
416+
FindUUID((const load_command *)data_.current_load_cmd_addr, hdr,
397417
data_.current_uuid);
398418
data_.current_instrumented = IsModuleInstrumented(
399-
(const load_command *)data_.current_load_cmd_addr);
419+
(const load_command *)data_.current_load_cmd_addr, hdr);
400420
}
401421

402422
while (data_.current_load_cmd_count > 0) {

‎compiler-rt/lib/sanitizer_common/tests/sanitizer_procmaps_mac_test.cpp‎

Lines changed: 73 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -37,16 +37,15 @@ class MemoryMappingLayoutMock final : public MemoryMappingLayout {
3737
.uuid = {}
3838
};
3939

40-
static constexpr char dylib_name[] = "libclang_rt.\0\0\0"; // 8 bytes aligned, padded with zeros per loader.h
40+
static constexpr char libclang_rt_dylib_name[] =
41+
"libclang_rt.\0\0\0"; // 8 bytes aligned, padded with zeros per loader.h
42+
static constexpr char uninstrumented_dylib_name[] =
43+
"uninst___rt.\0\0\0"; // 8 bytes aligned, padded with zeros per loader.h
44+
4145
static constexpr dylib_command mock_dylib_command = {
42-
.cmd = LC_LOAD_DYLIB,
43-
.cmdsize = sizeof(dylib_command) + sizeof(dylib_name),
44-
.dylib = {
45-
.name = {
46-
.offset = sizeof(dylib_command)
47-
}
48-
}
49-
};
46+
.cmd = LC_LOAD_DYLIB,
47+
.cmdsize = sizeof(dylib_command) + sizeof(libclang_rt_dylib_name),
48+
.dylib = {.name = {.offset = sizeof(dylib_command)}}};
5049

5150
static constexpr uuid_command mock_trap_command = {
5251
.cmd = LC_UUID,
@@ -59,51 +58,57 @@ class MemoryMappingLayoutMock final : public MemoryMappingLayout {
5958
std::vector<unsigned char> mock_header;
6059

6160
public:
62-
MemoryMappingLayoutMock(): MemoryMappingLayout(false) {
63-
EXPECT_EQ(mock_uuid_command.cmdsize % 8, 0u);
64-
EXPECT_EQ(mock_dylib_command.cmdsize % 8, 0u);
65-
66-
Reset();
67-
68-
#ifdef MH_MAGIC_64
69-
const struct mach_header_64 *header = (mach_header_64 *)_dyld_get_image_header(0); // Any header will do
70-
const size_t header_size = sizeof(mach_header_64);
71-
#else
72-
const struct mach_header *header = _dyld_get_image_header(0);
73-
const size_t header_size = sizeof(mach_header);
74-
#endif
75-
const size_t mock_header_size_with_extras = header_size + header->sizeofcmds +
76-
mock_uuid_command.cmdsize + mock_dylib_command.cmdsize + sizeof(uuid_command);
77-
78-
mock_header.reserve(mock_header_size_with_extras);
79-
// Copy the original header
80-
copy((unsigned char *)header,
81-
(unsigned char *)header + header_size + header->sizeofcmds,
82-
back_inserter(mock_header));
83-
// The following commands are not supposed to be processed
84-
// by the (correct) ::Next method at all, since they're not
85-
// accounted for in header->ncmds .
86-
copy((unsigned char *)&mock_uuid_command,
87-
((unsigned char *)&mock_uuid_command) + mock_uuid_command.cmdsize,
88-
back_inserter(mock_header));
89-
copy((unsigned char *)&mock_dylib_command,
90-
((unsigned char *)&mock_dylib_command) + sizeof(dylib_command), // as mock_dylib_command.cmdsize contains the following string
91-
back_inserter(mock_header));
92-
copy((unsigned char *)dylib_name,
93-
((unsigned char *)dylib_name) + sizeof(dylib_name),
94-
back_inserter(mock_header));
95-
96-
// Append a command w. huge size to have the test detect the read overrun
97-
copy((unsigned char *)&mock_trap_command,
98-
((unsigned char *)&mock_trap_command) + sizeof(uuid_command),
99-
back_inserter(mock_header));
100-
101-
start_load_cmd_addr = (const char *)(mock_header.data() + header_size);
102-
sizeofcmds = header->sizeofcmds;
103-
104-
const char *last_byte_load_cmd_addr = (start_load_cmd_addr+sizeofcmds-1);
105-
data_.current_image = -1; // So the loop in ::Next runs just once
106-
}
61+
MemoryMappingLayoutMock(bool instrumented) : MemoryMappingLayout(false) {
62+
EXPECT_EQ(mock_uuid_command.cmdsize % 8, 0u);
63+
EXPECT_EQ(mock_dylib_command.cmdsize % 8, 0u);
64+
65+
Reset();
66+
67+
# ifdef MH_MAGIC_64
68+
const struct mach_header_64 *header =
69+
(mach_header_64 *)_dyld_get_image_header(0); // Any header will do
70+
const size_t header_size = sizeof(mach_header_64);
71+
# else
72+
const struct mach_header *header = _dyld_get_image_header(0);
73+
const size_t header_size = sizeof(mach_header);
74+
# endif
75+
const size_t mock_header_size_with_extras =
76+
header_size + header->sizeofcmds + mock_uuid_command.cmdsize +
77+
mock_dylib_command.cmdsize + sizeof(uuid_command);
78+
79+
mock_header.reserve(mock_header_size_with_extras);
80+
// Copy the original header
81+
copy((unsigned char *)header,
82+
(unsigned char *)header + header_size + header->sizeofcmds,
83+
back_inserter(mock_header));
84+
// The following commands are not supposed to be processed
85+
// by the (correct) ::Next method at all, since they're not
86+
// accounted for in header->ncmds .
87+
copy((unsigned char *)&mock_uuid_command,
88+
((unsigned char *)&mock_uuid_command) + mock_uuid_command.cmdsize,
89+
back_inserter(mock_header));
90+
copy((unsigned char *)&mock_dylib_command,
91+
((unsigned char *)&mock_dylib_command) +
92+
sizeof(dylib_command), // as mock_dylib_command.cmdsize contains
93+
// the following string
94+
back_inserter(mock_header));
95+
const char(&dylib_name)[16] =
96+
instrumented ? libclang_rt_dylib_name : uninstrumented_dylib_name;
97+
copy((unsigned char *)dylib_name,
98+
((unsigned char *)dylib_name) + sizeof(dylib_name),
99+
back_inserter(mock_header));
100+
101+
// Append a command w. huge size to have the test detect the read overrun
102+
copy((unsigned char *)&mock_trap_command,
103+
((unsigned char *)&mock_trap_command) + sizeof(uuid_command),
104+
back_inserter(mock_header));
105+
106+
start_load_cmd_addr = (const char *)(mock_header.data() + header_size);
107+
sizeofcmds = header->sizeofcmds;
108+
109+
const char *last_byte_load_cmd_addr = (start_load_cmd_addr + sizeofcmds - 1);
110+
data_.current_image = -1; // So the loop in ::Next runs just once
111+
}
107112

108113
size_t SizeOfLoadCommands() {
109114
return sizeofcmds;
@@ -120,8 +125,20 @@ class MemoryMappingLayoutMock final : public MemoryMappingLayout {
120125
}
121126
};
122127

123-
TEST(MemoryMappingLayout, Next) {
124-
__sanitizer::MemoryMappingLayoutMock memory_mapping;
128+
TEST(MemoryMappingLayout, NextInstrumented) {
129+
__sanitizer::MemoryMappingLayoutMock memory_mapping(true);
130+
__sanitizer::MemoryMappedSegment segment;
131+
size_t size = memory_mapping.SizeOfLoadCommands();
132+
while (memory_mapping.Next(&segment)) {
133+
size_t offset = memory_mapping.CurrentLoadCommandOffset();
134+
EXPECT_LE(offset, size);
135+
}
136+
size_t final_offset = memory_mapping.CurrentLoadCommandOffset();
137+
EXPECT_EQ(final_offset, size); // All commands processed, no more, no less
138+
}
139+
140+
TEST(MemoryMappingLayout, NextUnInstrumented) {
141+
__sanitizer::MemoryMappingLayoutMock memory_mapping(false);
125142
__sanitizer::MemoryMappedSegment segment;
126143
size_t size = memory_mapping.SizeOfLoadCommands();
127144
while (memory_mapping.Next(&segment)) {

0 commit comments

Comments
 (0)