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

[Mitch Phillips via Phabricator via llvm-commits]

hctim added inline comments.


================
Comment at: compiler-rt/lib/scudo/standalone/wrappers_c.inc:158
+static void SCUDO_PREFIX(setupAtFork)() {
+  pthread_atfork(nullptr, nullptr, SCUDO_PREFIX(malloc_enable));
+}
----------------
cryptoad wrote:
> 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.
Yep, missed this too...


================
Comment at: compiler-rt/lib/scudo/standalone/wrappers_c.inc:163
+  scudo::ScopedLock L(SCUDO_PREFIX(Mutex));
+  if (SCUDO_PREFIX(Disabled))
+    return;
----------------
cryptoad wrote:
> 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.
Oops, early morning code reviews are a bad idea. You're right.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72364