From 056769cdbc60569b01c2f1ff67507f64ad83771f Mon Sep 17 00:00:00 2001 From: Douglas Yung Date: Sat, 4 Feb 2023 10:18:58 -0800 Subject: [PATCH] Revert "[Sanitizers] Fix read buffer overrun in scanning loader commands" This reverts commit abbd4da2043856f443e3d1c8d2c7627cac93a6ac. This change is breaking many bots including: - http://45.33.8.238/linux/98629/step_10.txt - https://buildkite.com/llvm-project/llvm-main/builds/6461#01861c4f-9d9c-4781-88f7-d6ccddcb4b06/919-8848 - https://lab.llvm.org/buildbot/#/builders/94/builds/13196 - https://lab.llvm.org/buildbot/#/builders/45/builds/10633 - https://lab.llvm.org/buildbot/#/builders/247/builds/1238 - https://lab.llvm.org/buildbot/#/builders/70/builds/33424 - https://lab.llvm.org/buildbot/#/builders/168/builds/11693 - https://lab.llvm.org/buildbot/#/builders/74/builds/17006 - https://lab.llvm.org/buildbot/#/builders/85/builds/14120 --- .../lib/sanitizer_common/sanitizer_procmaps.h | 14 +-- .../sanitizer_common/sanitizer_procmaps_mac.cpp | 16 +-- .../lib/sanitizer_common/tests/CMakeLists.txt | 1 - .../tests/sanitizer_procmaps_mac_test.cpp | 137 --------------------- 4 files changed, 8 insertions(+), 160 deletions(-) delete mode 100644 compiler-rt/lib/sanitizer_common/tests/sanitizer_procmaps_mac_test.cpp diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_procmaps.h b/compiler-rt/lib/sanitizer_common/sanitizer_procmaps.h index a3887ea..19bad15 100644 --- a/compiler-rt/lib/sanitizer_common/sanitizer_procmaps.h +++ b/compiler-rt/lib/sanitizer_common/sanitizer_procmaps.h @@ -65,8 +65,6 @@ class MemoryMappedSegment { MemoryMappedSegmentData *data_; }; -struct ImageHeader; - class MemoryMappingLayoutBase { public: virtual bool Next(MemoryMappedSegment *segment) { UNIMPLEMENTED(); } @@ -77,10 +75,10 @@ class MemoryMappingLayoutBase { ~MemoryMappingLayoutBase() {} }; -class MemoryMappingLayout : public MemoryMappingLayoutBase { +class MemoryMappingLayout final : public MemoryMappingLayoutBase { public: explicit MemoryMappingLayout(bool cache_enabled); - virtual ~MemoryMappingLayout(); + ~MemoryMappingLayout(); virtual bool Next(MemoryMappedSegment *segment) override; virtual bool Error() const override; virtual void Reset() override; @@ -92,14 +90,10 @@ class MemoryMappingLayout : public MemoryMappingLayoutBase { // Adds all mapped objects into a vector. void DumpListOfModules(InternalMmapVectorNoCtor *modules); - protected: -#if SANITIZER_APPLE - virtual const ImageHeader *CurrentImageHeader(); -#endif - MemoryMappingLayoutData data_; - private: void LoadFromCache(); + + MemoryMappingLayoutData data_; }; // Returns code range for the specified module. diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_procmaps_mac.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_procmaps_mac.cpp index b44e016a..4b0e678 100644 --- a/compiler-rt/lib/sanitizer_common/sanitizer_procmaps_mac.cpp +++ b/compiler-rt/lib/sanitizer_common/sanitizer_procmaps_mac.cpp @@ -250,9 +250,7 @@ static bool NextSegmentLoad(MemoryMappedSegment *segment, MemoryMappedSegmentData *seg_data, MemoryMappingLayoutData *layout_data) { const char *lc = layout_data->current_load_cmd_addr; - layout_data->current_load_cmd_addr += ((const load_command *)lc)->cmdsize; - layout_data->current_load_cmd_count--; if (((const load_command *)lc)->cmd == kLCSegment) { const SegmentCommand* sc = (const SegmentCommand *)lc; uptr base_virt_addr, addr_mask; @@ -360,16 +358,11 @@ static bool IsModuleInstrumented(const load_command *first_lc) { return false; } -const ImageHeader *MemoryMappingLayout::CurrentImageHeader() { - const mach_header *hdr = (data_.current_image == kDyldImageIdx) - ? get_dyld_hdr() - : _dyld_get_image_header(data_.current_image); - return (const ImageHeader *)hdr; -} - bool MemoryMappingLayout::Next(MemoryMappedSegment *segment) { for (; data_.current_image >= kDyldImageIdx; data_.current_image--) { - const mach_header *hdr = (const mach_header *)CurrentImageHeader(); + const mach_header *hdr = (data_.current_image == kDyldImageIdx) + ? get_dyld_hdr() + : _dyld_get_image_header(data_.current_image); if (!hdr) continue; if (data_.current_load_cmd_count < 0) { // Set up for this image; @@ -399,7 +392,7 @@ bool MemoryMappingLayout::Next(MemoryMappedSegment *segment) { (const load_command *)data_.current_load_cmd_addr); } - while (data_.current_load_cmd_count > 0) { + for (; data_.current_load_cmd_count >= 0; data_.current_load_cmd_count--) { switch (data_.current_magic) { // data_.current_magic may be only one of MH_MAGIC, MH_MAGIC_64. #ifdef MH_MAGIC_64 @@ -420,7 +413,6 @@ bool MemoryMappingLayout::Next(MemoryMappedSegment *segment) { } // If we get here, no more load_cmd's in this image talk about // segments. Go on to the next image. - data_.current_load_cmd_count = -1; // This will trigger loading next image } return false; } diff --git a/compiler-rt/lib/sanitizer_common/tests/CMakeLists.txt b/compiler-rt/lib/sanitizer_common/tests/CMakeLists.txt index 2ff6d46..0c729ec 100644 --- a/compiler-rt/lib/sanitizer_common/tests/CMakeLists.txt +++ b/compiler-rt/lib/sanitizer_common/tests/CMakeLists.txt @@ -35,7 +35,6 @@ set(SANITIZER_UNITTESTS sanitizer_posix_test.cpp sanitizer_printf_test.cpp sanitizer_procmaps_test.cpp - sanitizer_procmaps_mac_test.cpp sanitizer_ring_buffer_test.cpp sanitizer_quarantine_test.cpp sanitizer_stack_store_test.cpp 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 deleted file mode 100644 index f622bba..0000000 --- a/compiler-rt/lib/sanitizer_common/tests/sanitizer_procmaps_mac_test.cpp +++ /dev/null @@ -1,137 +0,0 @@ -//===-- sanitizer_procmaps_mac_test.cpp ---------------------------------------===// -// -// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. -// See https://llvm.org/LICENSE.txt for license information. -// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception -// -//===----------------------------------------------------------------------===// -// -// This file is a part of ThreadSanitizer/AddressSanitizer runtime. -// -//===----------------------------------------------------------------------===// - -# include "sanitizer_common/sanitizer_platform.h" - -# if SANITIZER_APPLE - -# include -# include -# include -# include - -# include -# include -# include - -# include "gtest/gtest.h" - -# include "sanitizer_common/sanitizer_procmaps.h" - -namespace __sanitizer { - -class MemoryMappingLayoutMock final : public MemoryMappingLayout { -private: - static constexpr uuid_command mock_uuid_command = { - .cmd = LC_UUID, - .cmdsize = sizeof(uuid_command), - .uuid = {} - }; - - static constexpr char dylib_name[] = "libclang_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) - } - } - }; - - static constexpr uuid_command mock_trap_command = { - .cmd = LC_UUID, - .cmdsize = 0x10000, - .uuid = {} - }; - - const char *start_load_cmd_addr; - size_t sizeofcmds; - std::vector 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 - } - - size_t SizeOfLoadCommands() { - return sizeofcmds; - } - - size_t CurrentLoadCommandOffset() { - size_t offset = data_.current_load_cmd_addr - start_load_cmd_addr; - return offset; - } - -protected: - virtual ImageHeader *CurrentImageHeader() override { - return (ImageHeader *)mock_header.data(); - } -}; - -TEST(MemoryMappingLayout, Next) { - __sanitizer::MemoryMappingLayoutMock memory_mapping; - __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 -} - -} // namespace __sanitizer - -# endif // SANITIZER_APPLE -- 2.7.4