[PATCH] D72364: [scudo][standalone] Modify malloc_{enable,disable} wrt fork

[Kostya Kortchinsky via Phabricator via llvm-commits]

cryptoad added inline comments.


================
Comment at: compiler-rt/lib/scudo/standalone/tests/wrappers_c_test.cpp:314
+
+// If a fork occurs while the allocator was disabled, we want to ensure that the
+// child can use allocation primitives without enabling the allocator first.
----------------
hctim wrote:
> Maybe add a test with a fork() where malloc isn't disabled?
Ack


================
Comment at: compiler-rt/lib/scudo/standalone/wrappers_c.inc:158
+static void SCUDO_PREFIX(setupAtFork)() {
+  pthread_atfork(nullptr, nullptr, SCUDO_PREFIX(malloc_enable));
+}
----------------
hctim wrote:
> instead of unconditionally calling `malloc_enable` on fork, can you guard it behind an `if Disabled`? Otherwise you call `unlock()` on an already-unlocked mutex in the allocators.
I am guarding `Disabled` with the mutex, so it makes it easier to call `malloc_enable` as it does check for Disabled within the mutex.
Unless I am misunderstanding the comment.


================
Comment at: compiler-rt/lib/scudo/standalone/wrappers_c.inc:163
+  scudo::ScopedLock L(SCUDO_PREFIX(Mutex));
+  if (SCUDO_PREFIX(Disabled))
+    return;
----------------
hctim wrote:
> Just to clarify, this is non-racy because malloc_enable and malloc_disable are always called in a single-threaded context? libmemunreachable I know `PTRACE_INTERRUPT`'s the other threads, but unsure about other uses of malloc_enable.
I don't think this can be racey since we are protecting the blocks with the mutex. Concurrent call to enable & disable should be resolved appropriately, as well as concurrent calls to the same API since we check for `Disabled` at the beginning of the block.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72364