[PATCH] D69265: [scudo][standalone] Consolidate lists

[Mitch Phillips via Phabricator via llvm-commits]

hctim added inline comments.


================
Comment at: lib/scudo/standalone/list.h:17
+// Intrusive POD singly and doubly linked list.
 // An object with all zero fields should represent a valid empty list. clear()
 // should be called on all non-zero-initialized objects before using.
----------------
cryptoad wrote:
> hctim wrote:
> > Why does the requirements to call `clear()` exist for non-zero-inited objects? For globally zero-init'ed instances of this object, we don't encounter a perf overhead of having `Size`, `First`, and `Last` be initialised to zero. For non-global instances, we have to call `clear()`, which seems easy to forget, and moving this initialisation into the c'tor doesn't cause any additional overhead (and I'd assume the call is elided, making it faster).
> > 
> > It also feels easy to forget to call `clear()`, and if any global objects are moved into static scopage, it may be incredibly hard to patch any instances where `clear()` isn't called.
> Part of the issue, even with `constexpr explicit` constructors is that `__thread` can't seem to deal with them (as opposed to `thread_local`) and complains about it needing "dynamic initialization". It circles back to the choices made with `init` & `initLinkerInitialized`.
> While your point makes sense, I don't think I can do it, unless I am missing something which is also very possible.
So I think I remember the same problem from `gwp_asan`, but it was solved for us by marking the c'tor as `constexpr`.

https://godbolt.org/z/j1q8uR
^ works with constexpr c'tor with `__thread` and `thread_local`, fails for `__thread` if not `constexpr` c'tor. Sounds like this might be the same problem?


================
Comment at: lib/scudo/standalone/list.h:206
+    if (Prev) {
+      CHECK_EQ(Prev->Next, X);
+      Prev->Next = Next;
----------------
cryptoad wrote:
> hctim wrote:
> > DCHECK? (and throughout this function)
> On this one I don't want to change the CHECKs to DCHECKs because doubly-lists unlinking is often the source of a mirrored write-4 primitive for exploitation.
> Some examples can be found in http://xcon.xfocus.org/XCon2004/archives/14_Reliable%20Windows%20Heap%20Exploits_BY_SHOK.pdf.
> Since it's being used in a large block header, I can't rule out that this could be targeted.
> I can probably relax the nullptr CHECKs as they are probably less relevant to prevent further exploitation.
Sounds good to me, and the rationale sounds very reasonable. Is it possible to add a comment here to clarify this deliberate intent to future readers?


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D69265