[PATCH] D73557: [GWP-ASan] Crash Handler API.

[Evgenii Stepanov via Phabricator via llvm-commits]

eugenis added inline comments.


================
Comment at: compiler-rt/lib/gwp_asan/crash_handler.cpp:79
+
+bool __gwp_asan_has_metadata(const gwp_asan::AllocatorState *State,
+                             const gwp_asan::AllocationMetadata *Metadata,
----------------
hctim wrote:
> eugenis wrote:
> > Why not return the metadata pointer and null if it could not be found, and then pass it to all other query functions?
> The crash handler isn't responsible for finding the metadata, as the original process might be dead. We actually need the metadata here to check that the allocation provided has actually been allocated once upon a time.
I actually meant instead of 
  if (Meta->Addr == 0)
      return false;
do
  if (Meta->Addr != 0)
    return Meta;

And then pass that pointer to the rest of the API, so they do not need to repeat the look up.





================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:149
   }
-  uninstallSignalHandlers();
+  crash_handler::uninstallSignalHandlers();
+}
----------------
if init() does not set the signal handlers, maybe uninit should not remove them.
Also, weak declarations are not very portable.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:201
+  // be able to catch our error.
+  raise(SIGSEGV);
+  __builtin_unreachable();
----------------
Touch memory in a guard page instead.
This will produce the same type of failure the fault handler is catching, which could be different from raise(SIGSEGV) (which is also posix-only).



================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:12
 
 #include "gwp_asan/definitions.h"
 #include "gwp_asan/mutex.h"
----------------
clang-format plz


================
Comment at: compiler-rt/lib/gwp_asan/tests/crash_handler_api.cpp:158
+TEST_F(CrashHandlerAPITest, InvalidFree) {
+  uintptr_t FailureAddress = 0x7001;
+
----------------
It's usually better to be a bit more explicit in tests and setup the program state in each test case. In this case, I'd need to look ~2 pages up to understand what 0x7001 refers to. The test class could provide metadata setup helpers, and test case could look something like

metadata(0, 0x7000, 0x20, /*allocated*/false);
State.FailureAddress = 0x7001;
State.FailureType = Error::INVALID_FREE;
EXPECT_TRUE(__gwp_asan_error_is_mine(&State));



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73557/new/

https://reviews.llvm.org/D73557