Revert "[Sanitizers] Fix read buffer overrun in scanning loader commands"
authorDouglas Yung <douglas.yung@sony.com>
Sat, 4 Feb 2023 18:18:58 +0000 (10:18 -0800)
committerDouglas Yung <douglas.yung@sony.com>
Sat, 4 Feb 2023 18:19:57 +0000 (10:19 -0800)
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

compiler-rt/lib/sanitizer_common/sanitizer_procmaps.h
compiler-rt/lib/sanitizer_common/sanitizer_procmaps_mac.cpp
compiler-rt/lib/sanitizer_common/tests/CMakeLists.txt
compiler-rt/lib/sanitizer_common/tests/sanitizer_procmaps_mac_test.cpp [deleted file]

index a3887ea..19bad15 100644 (file)
@@ -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<LoadedModule> *modules);
 
- protected:
-#if SANITIZER_APPLE
-  virtual const ImageHeader *CurrentImageHeader();
-#endif
-  MemoryMappingLayoutData data_;
-
  private:
   void LoadFromCache();
+
+  MemoryMappingLayoutData data_;
 };
 
 // Returns code range for the specified module.
index b44e016..4b0e678 100644 (file)
@@ -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;
 }
index 2ff6d46..0c729ec 100644 (file)
@@ -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 (file)
index f622bba..0000000
+++ /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 <stdlib.h>
-#  include <string.h>
-#  include <stdint.h>
-#  include <stdio.h>
-
-#  include <vector>
-#  include <mach-o/dyld.h>
-#  include <mach-o/loader.h>
-
-#  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<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
-  }
-
-  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