[PATCH] D71719: [scudo][standalone] Implement TSD registry disabling

[Evgenii Stepanov via Phabricator via llvm-commits]

eugenis accepted this revision.
eugenis added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: compiler-rt/lib/scudo/standalone/tsd_shared.h:83
+      TSDs[I].unlock();
+  }
+
----------------
cryptoad wrote:
> cryptoad wrote:
> > eugenis wrote:
> > > I'm also concerned about the shared tsd code path. Is locking the tsd enough to ensure that no allocations will happen in the future? It looks like threads hold the tsd lock for as short a time as possible, and do the rest of the work such as updating the chuck state later without synchronization. This work may not be finished by the time malloc_disable returns. Could malloc_iterate be confused if it sees the allocator in that state?
> > You are correct, a thread could still be updating a block by the time `disable` returns.
> > 
> > Hopefully the window is small enough for the amount of potential confusion to be low. Given that this is mostly used in the context of libmemunreachable, which is mostly for debugging purposes, I think a best-effort approach as opposed to a catch-all that might have stronger performance implication is acceptable.
> > 
> > To be fair there might be something better that I haven't thought about as well.
> PS: I can add a TODO stating so.
I think this can be fixed properly with one or several strategically placed atomic stores in the block update code such that malloc_iterate can always recognize the state the block is in. We've made changes like this in asan & lsan some time ago, mostly in response to bad reports of racy use-after-frees.

This stuff is really hard to reason about. I think this change is OK as a first step, but please leave a TODO.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71719