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

[Kostya Kortchinsky via Phabricator via llvm-commits]

cryptoad marked 16 inline comments as done.
cryptoad added a comment.

Mitch, thanks for the review! You made excellent points.



================
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.
----------------
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.


================
Comment at: lib/scudo/standalone/list.h:121
     DCHECK_NE(X, nullptr);
-    DCHECK_EQ(Prev->Next, X);
+    CHECK_EQ(Prev->Next, X);
     Prev->Next = X->Next;
----------------
hctim wrote:
> Why `/s/DCHECK/CHECK` here? This shouldn't fail in normal program execution, right?
You are right. I initially thought that it would be important to ensure consistency, but extract is only called with X being Prev->Next, and while someone could attempt to TOCTOU it, I don't see that happening with a meaningful success chance.


================
Comment at: lib/scudo/standalone/list.h:151
+    if (this->empty()) {
+      X->Next = nullptr;
+      First = Last = X;
----------------
hctim wrote:
> Isn't there an invariant held that if `empty() == true`, then `First == nullptr`? In which case, can't we hoist the `X->Next = First;` out from this conditional?
Good point thank you!


================
Comment at: lib/scudo/standalone/list.h:163
+  void insert(T *X, T *Y) {
+    if (Y == First) {
+      CHECK_EQ(Y->Prev, nullptr);
----------------
hctim wrote:
> Would the specialisation:
> 
> ```
> if (Y == First)
>   return push_front(X);
> ```
> be more performant?
> 
> You should then be able to remove the `if (Prev)` check below, as assumedly there's an invariant that if `Y` isn't the first element, it has a `Prev`.
Good point again, thanks!


================
Comment at: lib/scudo/standalone/list.h:164
+    if (Y == First) {
+      CHECK_EQ(Y->Prev, nullptr);
+      First = X;
----------------
hctim wrote:
> DCHECK?
Removed as part of the previously suggested change.


================
Comment at: lib/scudo/standalone/list.h:206
+    if (Prev) {
+      CHECK_EQ(Prev->Next, X);
+      Prev->Next = Next;
----------------
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.


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D69265