Fix nullptr dereference bug in TList::RecursiveRemove()

amadio · amadio · commit 71b778c4d3ce · 2017-09-06T22:21:41.000+02:00
When multiple threads are touching the list of cleanups, another thread
can delete the object retrieved via TObjLink::GetObject(), and then when
it is dereferenced in ob-&gt;TestBit(...) it causes a crash in ROOT.

Stack trace:
in TObject::TestBit (this=0x0, f=33554432) at TObject.h:159
                          ^^^
in TList::RecursiveRemove (this=0xb3c3e0, obj=0x7ff3547da6b0)
  at root/core/cont/src/TList.cxx:717
                                  ^^^
in THashList::RecursiveRemove (this=0xb504b0, obj=0x7ff3547da6b0)
  at root/core/cont/src/THashList.cxx:286
in TObject::~TObject (this=0x7ff3547da6b0, __in_chrg=&lt;optimized out&gt;)
  at root/core/base/src/TObject.cxx:88
diff --git a/core/cont/src/TList.cxx b/core/cont/src/TList.cxx
@@ -703,45 +703,45 @@ void TList::RecursiveRemove(TObject *obj)
 
    if (!obj) return;
 
-   // When fCache is set and has no previous and next node, it represents
-   // the node being cleared and/or deleted.
-   if (fCache && fCache->fNext == 0 && fCache->fPrev == 0) {
-      TObject *ob = fCache->GetObject();
-      if (ob && ob->TestBit(kNotDeleted)) {
+   // When fCache is set and has no previous and next node,
+   // it represents the node being cleared and/or deleted.
+   if (fCache && fCache->fNext == 0 && fCache->fPrev == 0)
+      if (TObject *ob = fCache->GetObject())
+         if (ob->TestBit(kNotDeleted))
+            ob->RecursiveRemove(obj);
+
+   for (TObjLink *lnk = fFirst; lnk; lnk = lnk->Next()) {
+      TObject *ob = lnk->GetObject();
+
+      // skip if deleted or being deleted
+      if (!ob || !ob->TestBit(kNotDeleted))
+         continue;
+
+      // not this object, but may contain this object
+      if (!ob->IsEqual(obj)) {
          ob->RecursiveRemove(obj);
+         continue;
       }
-   }
 
-   TObjLink *lnk  = fFirst;
-   TObjLink *next = 0;
-   while (lnk) {
-      next = lnk->Next();
-      TObject *ob = lnk->GetObject();
-      if (ob->TestBit(kNotDeleted)) {
-         if (ob->IsEqual(obj)) {
-            if (lnk == fFirst) {
-               fFirst = next;
-               if (lnk == fLast)
-                  fLast = fFirst;
-               else
-                  fFirst->fPrev = 0;
-               DeleteLink(lnk);
-            } else if (lnk == fLast) {
-               fLast = lnk->Prev();
-               fLast->fNext = 0;
-               DeleteLink(lnk);
-            } else {
-               lnk->Prev()->fNext = next;
-               lnk->Next()->fPrev = lnk->Prev();
-               DeleteLink(lnk);
-            }
-            fSize--;
-            fCache = 0;
-            Changed();
-         } else
-            ob->RecursiveRemove(obj);
+      // object found, remove it from the list
+      if (lnk == fFirst) {
+         fFirst = fFirst->Next();
+         if (lnk == fLast)
+            fLast = fFirst;
+         else
+            fFirst->fPrev = 0;
+      } else if (lnk == fLast) {
+         fLast = lnk->Prev();
+         fLast->fNext = nullptr;
+      } else {
+         lnk->Prev()->fNext = lnk->Next();
+         lnk->Next()->fPrev = lnk->Prev();
       }
-      lnk = next;
+
+      fSize--;
+      DeleteLink(lnk);
+      fCache = nullptr;
+      Changed();
    }
 }
 
