[scudo] Call __scudo_deallocate_hook on reallocations.

[chelfi]

Summary

Scudo is expected to call scudo_allocate_hook on allocations, and
scudo_deallocate_hook on deallocations, but it's behavior is not
clear on reallocations. Currently, non-trivial reallocations call
scudo_allocate_hook but never scudo_deallocate_hook. We should
prefer either calling both, none, or a dedicated
hook (__scudo_reallocate_hook, for instance).

This patch implements the former, and adds a unit test to enforce
those expectations.

[cryptoad]

compiler-rt/lib/scudo/standalone/tests/combined_test.cpp

780 ↗

(On Diff #487847)

Can this test be put in a separate unit so that the hooks don't impact the rest of combined_test?

786 ↗

(On Diff #487847)

Maybe __attribute__((visibility("default")))?

813 ↗

(On Diff #487847)

This likely will depend on the chosen SizeClass for the platform this is running on.
Ideally you could check that the returned pointer is indeed different, and then verified the callbacks were called.
Same for next test case, check that they are the same.

[Chia-hungDuan]

compiler-rt/lib/scudo/standalone/combined.h

687

I'm thinking that we can move this to quarantineOrDeallocateChunk() then we have single location for both allocation and deallocation. Then for the tests, we only need to ensure that calling allocate/deallocate have the callback invoked, i.e., we don't need to depend on the implementation (like the size-class in the test case)

The only case we may miss is when nullptr deallocation but I think that may be fine.

@cryptoad, what do you think?

[cryptoad]

compiler-rt/lib/scudo/standalone/combined.h

687

I think we would miss more than the nullptr.
Any report... in deallocate as well a the GWP-ASan path wouldn't have the hook called, and those cases are arguably valuable.
So I think it makes sense to have the hook in a "top" functions.

[Chia-hungDuan]

compiler-rt/lib/scudo/standalone/tests/combined_test.cpp

813 ↗

(On Diff #487847)

You can also try to do realloc according to the sizes in SizeClassMap so that you can expect its returning different pointer, then you can always check thing like,

ptr = malloc(...);
for (size : size_in_size_class_map) {
  new_ptr = realloc(ptr, size);
  if (same_pointer) 
    // expect no allocate
  else
    // expect deallocation and allocation
}

[Chia-hungDuan]

compiler-rt/lib/scudo/standalone/combined.h

687

I see. I thought this is only used to report the valid pointers. May I know what's the goal of using these hooks?

[cryptoad]

compiler-rt/lib/scudo/standalone/combined.h

687

Generally, yes, as it is mostly used for memory tracing (like https://cs.github.com/vsrinivas/fuchsia/blob/30435a9d0f0b67c94e3c70760b522c9f7fbbd6be/src/performance/memory/profile/memory_trace.cc#L120).
But ideally, it should report *all* allocations and deallocations, including the "bogus" ones (eg: allocation of 0-sized chunks, free of null pointers, non-fatal inconsistencies, etc), mostly because we initially did not want to make a choice for the user as to what is reported and what isn't.

[chelfi]

compiler-rt/lib/scudo/standalone/combined.h

687

What would you prefer we do, in the context of this patch? I see two obvious options:

1. Keep it here, with maybe a comment to revisit exposing more info in a future patch.
2. Call the hook everywhere relevant. We can maybe move those calls to the top of the allocate/deallocate/reallocate, so that everything is reported regardless of what is actually happening in the allocator. We might even introduce a __scudo_reallocate_hook so that we don't have to translate reallocate calls to non-obvious combinations of allocate/deallocate calls (though this might just push the complexity downwards to profiler implementations.)

Any preference?

[chelfi]

Apply reviewer comments.

• Move tests to a separate compile unit.
• Programmatically iterate through the size class map to ensure meaningful reallocations.

This update copies a non-trivial amount of helper macros from combined_test.cpp, in order to reuse the nice support for iterating tests on allocator configs.

[cryptoad]

compiler-rt/lib/scudo/standalone/combined.h

687

I personally prefer it this way in reallocate

[Chia-hungDuan]

compiler-rt/lib/scudo/standalone/combined.h

687

@cryptoad , thanks for the background and Yes, I agree with you. Let's stick on the way here

compiler-rt/lib/scudo/standalone/tests/scudo_hooks_test.cpp

47–56

I think we don't need to distinguish the platform and a DefaultConfig may be enough. The goal is to ensure the hooks are invoked properly and iterate the size-class-map is to ensure the hooks will be called.

In addition, we may not need the SCUDO_TYPED_TEST, a simple TEST with the use of DefaultConfig should be enough

133–134

I may suggest just to add something more concrete rather than just a counter. For example,

if (NewPtr != Ptr) {
  // We expect Ptr to be reported to deallocate-hook and NewPtr to be reported allocate-hook
}

So that we won't report wrong information (e.g., report the block address rather than the aligned address)

[chelfi]

Improve unit tests.

• Remove the SCUDO_TYPED_TEST machinery.
• Further verify the data reported by the hooks (i.e. check content rather than just calls).

[chelfi]

Fix whitespace issues.

[chelfi]

Fix type issue.

I've manually tested the change in a Fuchsia checkout, because I was worried about the binary stripping policy in that toolchain.
My worries were unwarranted, so I removed part of the warnings in the test comments.

[Chia-hungDuan]

compiler-rt/lib/scudo/standalone/tests/scudo_hooks_test.cpp

23

compilation

26–28

Do we need them to be in the scope of extern "C"? I may suggest moving them out and mark as static

55

lint: I suggest spelling it out, void

75

Let's still catch the return value and you may want to expect the same pointer.

98

Same as above

102

I may suggest LastRequestSize

[chelfi]

Applying reviewer comments:

• Use explicit type names instead of auto.
• Make unnecessarily exposed symbols static.
• Ensure no-op reallocations return the same pointer.

[Chia-hungDuan]

compiler-rt/lib/scudo/standalone/tests/CMakeLists.txt

134

lit: align with s like others

compiler-rt/lib/scudo/standalone/tests/scudo_hooks_test.cpp

47

scudo::Allocator has larger alignment requirement so that we can't use make_unique here. Can you change this to local variable instead? Like

scudo::Allocator<scudo::DefaultConfig> Allocator;
...

I'll fix the other tests which are luckily got aligned.

[chelfi]

• Fix alignment issues.
• Rebase.

[stefanp]

@chelfi
It looks like this changeset has broken one of the bots on PowerPC.
https://lab.llvm.org/buildbot/#/builders/57

Could you please take a look at this?
If you have any questions or would like help reproducing the issue please let me know.

[cryptoad]

Seems to be EXPECT_EQ(0, LastRequestSize); related. Potential fix would be 0U for both instances.

[Chia-hungDuan]

In D141407#4129696, @cryptoad wrote:

Seems to be EXPECT_EQ(0, LastRequestSize); related. Potential fix would be 0U for both instances.

Yep, I'm on it. D144121

[chelfi]

Sorry for the breakage; thanks for the fix!

compiler-rt/lib/scudo/standalone/allocator_config.h

 
 namespace scudo {
 
 // The combined allocator uses a structure as a template argument that
 // specifies the configuration options for the various subcomponents of the
 // allocator.
 //
 // struct ExampleConfig {
-//   // SizeClasMmap to use with the Primary.
+//   // SizeClassMap to use with the Primary.
 //   using SizeClassMap = DefaultSizeClassMap;
 //   // Indicates possible support for Memory Tagging.
 //   static const bool MaySupportMemoryTagging = false;
 //   // Defines the Primary allocator to use.
 //   typedef SizeClassAllocator64<ExampleConfig> Primary;
 //   // Log2 of the size of a size class region, as used by the Primary.
 //   static const uptr PrimaryRegionSizeLog = 30U;
 //   // Log2 of the size of block group, as used by the Primary. Each group

compiler-rt/lib/scudo/standalone/combined.h

#endif // GWP_ASAN_HOOKS
 
     // Otherwise we allocate a new one, and deallocate the old one. Some
     // allocators will allocate an even larger chunk (by a fixed factor) to
     // allow for potential further in-place realloc. The gains of such a trick
     // are currently unclear.
     void *NewPtr = allocate(NewSize, Chunk::Origin::Malloc, Alignment);
     if (LIKELY(NewPtr)) {
       memcpy(NewPtr, OldTaggedPtr, Min(NewSize, OldSize));
+      if (UNLIKELY(&__scudo_deallocate_hook))
+        __scudo_deallocate_hook(OldTaggedPtr);
       quarantineOrDeallocateChunk(Options, OldTaggedPtr, &OldHeader, OldSize);
     }
     return NewPtr;
   }
 
   // TODO(kostyak): disable() is currently best-effort. There are some small
   //                windows of time when an allocation could still succeed after
   //                this function finishes. We will revisit that later.

compiler-rt/lib/scudo/standalone/tests/CMakeLists.txt

 set(SCUDO_CXX_UNIT_TEST_SOURCES
   wrappers_cpp_test.cpp
   scudo_unit_test_main.cpp
   )
 
 add_scudo_unittest(ScudoCxxUnitTest
   SOURCES ${SCUDO_CXX_UNIT_TEST_SOURCES}
   ADDITIONAL_RTOBJECTS RTScudoStandaloneCWrappers RTScudoStandaloneCxxWrappers)
+
+set(SCUDO_HOOKS_UNIT_TEST_SOURCES
+  scudo_hooks_test.cpp
+  scudo_unit_test_main.cpp
+)
+
+add_scudo_unittest(ScudoHooksUnitTest
+  SOURCES ${SCUDO_HOOKS_UNIT_TEST_SOURCES})

compiler-rt/lib/scudo/standalone/tests/scudo_hooks_test.cpp

+//===-- scudo_hooks_test.cpp ------------------------------------*- C++ -*-===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#include "tests/scudo_unit_test.h"
+
+#include "allocator_config.h"
+#include "combined.h"
+
+// Scudo defines weak symbols that can be defined by a client binary
+// to register callbacks at key points in the allocation timeline.  In
+// order to enforce those invariants, we provide definitions that
+// update some global state every time they are called, so that tests
+// can inspect their effects.  An unfortunate side effect of this
+// setup is that because those symbols are part of the binary, they
+// can't be selectively enabled; that means that they will get called
+// on unrelated tests in the same compile unit. To mitigate this
+// issue, we insulate those tests in a separate compile unit. This
+// also has implications on platforms that strip symbols from binaries
+// that are not shared libraries.
+extern "C" {
+
+void *LastAllocatedPtr = nullptr;
+int LastAllocatedSize = 0;
+void* LastDeallocatedPtr = nullptr;
+
+__attribute__((visibility("default"))) void __scudo_allocate_hook(void *Ptr,
+                                                                  size_t Size) {
+  LastAllocatedPtr = Ptr;
+  LastAllocatedSize = Size;
+}
+__attribute__((visibility("default"))) void __scudo_deallocate_hook(void *Ptr) {
+  LastDeallocatedPtr = Ptr;
+}
+}
+
+// Simple check that allocation callbacks, when registered, are called:
+//   1) __scudo_allocate_hook is called when allocating.
+//   2) __scudo_deallocate_hook is called when deallocating.
+//   3) Both hooks are called when reallocating.
+//   4) Neither are called for a no-op reallocation.
+TEST(ScudoHooksTest, AllocateHooks) {
+  auto Allocator = std::make_unique<scudo::Allocator<scudo::DefaultConfig>>();
+  constexpr scudo::uptr DefaultSize = 16U;
+  constexpr scudo::Chunk::Origin Origin = scudo::Chunk::Origin::Malloc;
+
+  // Simple allocation and deallocation.
+  {
+    LastAllocatedPtr = nullptr;
+    LastAllocatedSize = 0;
+
+    auto *Ptr = Allocator->allocate(DefaultSize, Origin);
+
+    EXPECT_EQ(Ptr, LastAllocatedPtr);
+    EXPECT_EQ(DefaultSize, LastAllocatedSize);
+
+    LastDeallocatedPtr = nullptr;
+
+    Allocator->deallocate(Ptr, Origin);
+
+    EXPECT_EQ(Ptr, LastDeallocatedPtr);
+  }
+
+  // Simple no-op, same size reallocation.
+  {
+    auto *Ptr = Allocator->allocate(DefaultSize, Origin);
+
+    LastAllocatedPtr = nullptr;
+    LastAllocatedSize = 0;
+    LastDeallocatedPtr = nullptr;
+
+    Allocator->reallocate(Ptr, DefaultSize);
+
+    EXPECT_EQ(nullptr, LastAllocatedPtr);
+    EXPECT_EQ(0, LastAllocatedSize);
+    EXPECT_EQ(nullptr, LastDeallocatedPtr);
+  }
+
+  // Reallocation in increasing size classes. This ensures that at
+  // least one of the reallocations will be meaningful.
+  {
+    auto *Ptr = Allocator->allocate(0, Origin);
+
+    for (scudo::uptr ClassId = 1U;
+         ClassId <= scudo::DefaultConfig::Primary::SizeClassMap::LargestClassId;
+         ++ClassId) {
+      const scudo::uptr Size =
+      scudo::DefaultConfig::Primary::SizeClassMap::getSizeByClassId(ClassId);
+
+      LastAllocatedPtr = nullptr;
+      LastAllocatedSize = 0;
+      LastDeallocatedPtr = nullptr;
+
+      auto *NewPtr = Allocator->reallocate(Ptr, Size);
+
+      if (NewPtr != Ptr) {
+        EXPECT_EQ(NewPtr, LastAllocatedPtr);
+        EXPECT_EQ(Size, LastAllocatedSize);
+        EXPECT_EQ(Ptr, LastDeallocatedPtr);
+      } else {
+        EXPECT_EQ(nullptr, LastAllocatedPtr);
+        EXPECT_EQ(0, LastAllocatedSize);
+        EXPECT_EQ(nullptr, LastDeallocatedPtr);
+      }
+
+      Ptr = NewPtr;
+    }
+  }
+}