Skip to content
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Partial revert of "Fix nullptr dereference bug in TList::RecursiveRem…
…ove()"

This partially revert commit 71b778c

We can not use (directly) a for loop in RecursiveRemove as its deletes
the object/iterator inside the loop:

for (TObjLink *lnk = fFirst; lnk; lnk = lnk->Next()) {
  ...
  DeleteLink(lnk);
}

Resulting in :

Processing tutorials/graphs/multipalette.C...
(TCanvas *) 0x12fd08e0
==7117== Invalid read of size 8
==7117==    at 0x5111612: TObjLink::Next() (TList.h:142)
==7117==    by 0x51B74F4: TList::RecursiveRemove(TObject*) (TList.cxx:713)
==7117==    by 0x4164444: TPad::RecursiveRemove(TObject*) (TPad.cxx:5143)
==7117==    by 0x51B73B7: TList::RecursiveRemove(TObject*) (TList.cxx:722)
==7117==    by 0x4164444: TPad::RecursiveRemove(TObject*) (TPad.cxx:5143)
==7117==    by 0x51B73B7: TList::RecursiveRemove(TObject*) (TList.cxx:722)
==7117==    by 0x51B96CB: THashList::RecursiveRemove(TObject*) (THashList.cxx:286)
==7117==    by 0x514245A: TObject::~TObject() (TObject.cxx:88)
==7117==    by 0x50EDC1D: TNamed::~TNamed() (TNamed.h:41)
==7117==    by 0x14535B0F: TF1::~TF1() (TF1.cxx:827)
==7117==    by 0x1454CA07: TF2::~TF2() (TF2.cxx:153)
==7117==    by 0x1454CA3D: TF2::~TF2() (TF2.cxx:155)
==7117==    by 0x51B09D4: TCollection::GarbageCollect(TObject*) (TCollection.cxx:748)
==7117==    by 0x51B6BE8: TList::Delete(char const*) (TList.cxx:501)
==7117==    by 0x518E05D: TROOT::EndOfProcessCleanups() (TROOT.cxx:1171)
==7117==    by 0x524F659: TUnixSystem::Exit(int, bool) (TUnixSystem.cxx:2153)
==7117==    by 0x5169BE1: TApplication::Terminate(int) (TApplication.cxx:1279)
==7117==    by 0x4049925: TRint::Terminate(int) (TRint.cxx:686)
==7117==    by 0x40486D5: TRint::Run(bool) (TRint.cxx:437)
==7117==    by 0x4012AA: main (rmain.cxx:30)
==7117==  Address 0x6e72ff8 is 8 bytes inside a block of size 56 free'd
==7117==    at 0x4C2918D: operator delete(void*) (vg_replace_malloc.c:576)
==7117==    by 0x51B8B11: TObjOptLink::~TObjOptLink() (TList.h:163)
==7117==    by 0x51B6DAF: TList::DeleteLink(TObjLink*) (TList.cxx:530)
==7117==    by 0x51B74C0: TList::RecursiveRemove(TObject*) (TList.cxx:742)
==7117==    by 0x4164444: TPad::RecursiveRemove(TObject*) (TPad.cxx:5143)
==7117==    by 0x51B73B7: TList::RecursiveRemove(TObject*) (TList.cxx:722)
==7117==    by 0x4164444: TPad::RecursiveRemove(TObject*) (TPad.cxx:5143)
==7117==    by 0x51B73B7: TList::RecursiveRemove(TObject*) (TList.cxx:722)
==7117==    by 0x51B96CB: THashList::RecursiveRemove(TObject*) (THashList.cxx:286)
==7117==    by 0x514245A: TObject::~TObject() (TObject.cxx:88)
==7117==    by 0x50EDC1D: TNamed::~TNamed() (TNamed.h:41)
==7117==    by 0x14535B0F: TF1::~TF1() (TF1.cxx:827)
==7117==    by 0x1454CA07: TF2::~TF2() (TF2.cxx:153)
==7117==    by 0x1454CA3D: TF2::~TF2() (TF2.cxx:155)
  • Loading branch information
pcanal committed Sep 7, 2017
commit b7b348d7d302bbaafc6a25115febcc9443e67ac2
46 changes: 27 additions & 19 deletions core/cont/src/TList.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -710,7 +710,11 @@ void TList::RecursiveRemove(TObject *obj)
if (ob->TestBit(kNotDeleted))
ob->RecursiveRemove(obj);

for (TObjLink *lnk = fFirst; lnk; lnk = lnk->Next()) {
TObjLink *lnk = fFirst;
TObjLink *next = 0;
while (lnk) {
next = lnk->Next();

TObject *ob = lnk->GetObject();

// skip if deleted or being deleted
Expand All @@ -719,29 +723,33 @@ void TList::RecursiveRemove(TObject *obj)

// not this object, but may contain this object
if (!ob->IsEqual(obj)) {

ob->RecursiveRemove(obj);
continue;
}

// 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();

// 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();
}

fSize--;
DeleteLink(lnk);
fCache = nullptr;
Changed();
}

fSize--;
DeleteLink(lnk);
fCache = nullptr;
Changed();
lnk = next;
}
}

Expand Down