core: Disable check for already deleted object.

bindings/jupyroot/python/JupyROOT/helpers/cppcompleter.py

@@ -75,6 +75,7 @@ class CppCompleter(object):
     ...     print(suggestion)
     TROOT::IsA
     TROOT::IsBatch
+    TROOT::IsDestructed
     TROOT::IsEqual
     TROOT::IsEscaped
     TROOT::IsExecutingMacro

bindings/pyroot_legacy/JupyROOT/helpers/cppcompleter.py

@@ -87,6 +87,7 @@ class CppCompleter(object):
     ...     print(suggestion)
     TROOT::IsA
     TROOT::IsBatch
+    TROOT::IsDestructed
     TROOT::IsEqual
     TROOT::IsEscaped
     TROOT::IsExecutingMacro

core/base/inc/TObject.h

@@ -33,6 +33,10 @@ class TObjArray;
 class TMethod;
 class TTimer;
 
+namespace ROOT {
+namespace Internal {
+   bool DeleteChangesMemoryImpl();
+}}
 
 class TObject {
 
@@ -163,6 +167,16 @@ class TObject {
    virtual Int_t       Write(const char *name = nullptr, Int_t option = 0, Int_t bufsize = 0);
    virtual Int_t       Write(const char *name = nullptr, Int_t option = 0, Int_t bufsize = 0) const;
 
+   /// IsDestructed
+   ///
+   /// \note This function must be non-virtual as it can be used on destructed (but
+   /// not yet modified) memory.  This is used for example in TClonesArray to record
+   /// the element that have been destructed but not deleted and thus are ready for
+   /// re-use (by operator new with placement).
+   ///
+   /// \return true if this object's destructor has been run.
+   Bool_t IsDestructed() const { return !TestBit(kNotDeleted); }
+
    //----- operators
    void    *operator new(size_t sz) { return TStorage::ObjectAlloc(sz); }
    void    *operator new[](size_t sz) { return TStorage::ObjectAllocArray(sz); }
@@ -226,6 +240,7 @@ class TObject {
    static void      SetObjectStat(Bool_t stat);
 
    friend class TClonesArray; // needs to reset kNotDeleted in fBits
+   friend bool ROOT::Internal::DeleteChangesMemoryImpl();
 
    ClassDef(TObject,1)  //Basic ROOT object
 };
@@ -365,4 +380,31 @@ namespace cling {
    std::string printValue(TObject *val);
 }
 
+namespace ROOT {
+
+namespace Internal {
+   bool DeleteChangesMemory();
+} // Internal
+
+namespace Detail {
+
+
+/// @brief Check if the TObject's memory has been deleted.
+/// @warning This should be only used for error mitigation as the answer is only
+///    sometimes correct.  It actually just checks whether the object has been
+///    deleted, so this will falsely return true for an object that has
+///    been destructed but its memory has not been deleted.  This will return an
+///    undefined value if the memory is re-used between the deletion and the check.
+///    i.e.  This is useful to prevent a segmentation fault in case where the problem
+///    can be detected when the deletion and the usage are 'close-by'
+/// @warning In enviroment where delete taints (changes) the memory, this function
+///    always returns false as the marker left by ~TObject will be overwritten.
+/// @param obj The memory to check
+/// @return true if the object has been destructed and it can be inferred that it has been deleted
+R__ALWAYS_INLINE bool HasBeenDeleted(const TObject *obj) {
+   return !ROOT::Internal::DeleteChangesMemory() && obj->IsDestructed();
+}
+
+}} // ROOT::Details
+
 #endif

core/base/src/TObject.cxx

@@ -56,6 +56,76 @@ Bool_t TObject::fgObjectStat = kTRUE;
 
 ClassImp(TObject);
 
+namespace ROOT {
+namespace Internal {
+
+// Return true if delete changes/poisons/taints the memory.
+//
+// Detect whether operator delete taints the memory. If it does, we can not rely
+// on TestBit(kNotDeleted) to check if the memory has been deleted (but in case,
+// like TClonesArray, where we know the destructor will be called but not operator
+// delete, we can still use it to detect the cases where the destructor was called.
+
+bool DeleteChangesMemoryImpl()
+{
+   static constexpr UInt_t kGoldenUUID = 0x00000021;
+   static constexpr UInt_t kGoldenbits = 0x03000000;
+
+   TObject *o = new TObject;
+   o->SetUniqueID(kGoldenUUID);
+   UInt_t *o_fuid = &(o->fUniqueID);
+   UInt_t *o_fbits = &(o->fBits);
+
+   if (*o_fuid != kGoldenUUID) {
+      Error("CheckingDeleteSideEffects",
+            "fUniqueID is not as expected, we got 0x%.8x instead of 0x%.8x",
+            *o_fuid, kGoldenUUID);
+   }
+   if (*o_fbits != kGoldenbits) {
+      Error("CheckingDeleteSideEffects",
+            "fBits is not as expected, we got 0x%.8x instead of 0x%.8x",
+            *o_fbits, kGoldenbits);
+   }
+   if (gDebug >= 9) {
+      unsigned char *oc = reinterpret_cast<unsigned char *>(o); // for address calculations
+      unsigned char references[sizeof(TObject)];
+      memcpy(references, oc, sizeof(TObject));
+
+      // The effective part of this code (the else statement is just that without
+      // any of the debug statement)
+      delete o;
+
+      // Not using the error logger, as there routine is meant to be called
+      // during library initialization/loading.
+      fprintf(stderr,
+              "DEBUG: Checking before and after delete the content of a TObject with uniqueID 0x21\n");
+      for(size_t i = 0; i < sizeof(TObject); i += 4) {
+        fprintf(stderr, "DEBUG: 0x%.8x vs 0x%.8x\n", *(int*)(references +i), *(int*)(oc + i));
+      }
+   } else
+      delete o;  // the 'if' part is that surrounded by the debug code.
+
+   // Intentionally accessing the deleted memory to check whether it has been changed as
+   // a consequence (side effect) of executing operator delete.  If there no change, we
+   // can guess this is always the case and we can rely on the changes to fBits made
+   // by ~TObject to detect use-after-delete error (and print a message rather than
+   // stop the program with a segmentation fault)
+   if ( *o_fbits != 0x01000000 ) {
+      // operator delete tainted the memory, we can not rely on TestBit(kNotDeleted)
+      return true;
+   }
+   return false;
+}
+
+bool DeleteChangesMemory()
+{
+   static const bool value = DeleteChangesMemoryImpl();
+   if (gDebug >= 9)
+      DeleteChangesMemoryImpl(); // To allow for printing the debug info
+   return value;
+}
+
+}} // ROOT::Detail
 
 ////////////////////////////////////////////////////////////////////////////////
 /// Copy this to obj.

core/base/src/TQConnection.cxx

@@ -284,7 +284,7 @@ CallFunc_t *TQSlot::StartExecuting() {
 
 void TQSlot::EndExecuting() {
    fExecuting--;
-   if (!TestBit(kNotDeleted) && !fExecuting)
+   if (ROOT::Detail::HasBeenDeleted(this) && !fExecuting)
       gCling->CallFunc_Delete(fFunc);
 }
 
@@ -344,7 +344,7 @@ inline void TQSlot::ExecuteMethod(void *object, Longptr_t *paramArr, Int_t npara
    fExecuting++;
    gCling->CallFunc_Exec(fFunc, address);
    fExecuting--;
-   if (!TestBit(kNotDeleted) && !fExecuting)
+   if (ROOT::Detail::HasBeenDeleted(this) && !fExecuting)
       gCling->CallFunc_Delete(fFunc);
 }
 

core/base/src/TROOT.cxx

@@ -1371,7 +1371,7 @@ TObject *TROOT::FindSpecialObject(const char *name, void *&where)
       }
    }
    if (!temp) return nullptr;
-   if (temp->TestBit(kNotDeleted)) return temp;
+   if (!ROOT::Detail::HasBeenDeleted(temp)) return temp;
    return nullptr;
 }
 

core/cont/src/TClonesArray.cxx

@@ -156,7 +156,7 @@ bool R__SetClonesArrayTFormulaUpdater(Updater_t func) {
 /// Internal Utility routine to correctly release the memory for an object
 static inline void R__ReleaseMemory(TClass *cl, TObject *obj)
 {
-   if (obj && obj->TestBit(TObject::kNotDeleted)) {
+   if (obj && ! obj->IsDestructed()) {
       // -- The TObject destructor has not been called.
       cl->Destructor(obj);
    } else {
@@ -366,7 +366,7 @@ void TClonesArray::Compress()
 TObject *TClonesArray::ConstructedAt(Int_t idx)
 {
    TObject *obj = (*this)[idx];
-   if ( obj && obj->TestBit(TObject::kNotDeleted) ) {
+   if ( obj && ! obj->IsDestructed() ) {
       return obj;
    }
    return (fClass) ? static_cast<TObject*>(fClass->New(obj)) : nullptr;
@@ -388,7 +388,7 @@ TObject *TClonesArray::ConstructedAt(Int_t idx)
 TObject *TClonesArray::ConstructedAt(Int_t idx, Option_t *clear_options)
 {
    TObject *obj = (*this)[idx];
-   if ( obj && obj->TestBit(TObject::kNotDeleted) ) {
+   if ( obj && !obj->IsDestructed() ) {
       obj->Clear(clear_options);
       return obj;
    }
@@ -444,13 +444,13 @@ void TClonesArray::Delete(Option_t *)
       // In case of emulated class, we can not use the delete operator
       // directly, it would use the wrong destructor.
       for (Int_t i = 0; i < fSize; i++) {
-         if (fCont[i] && fCont[i]->TestBit(kNotDeleted)) {
+         if (fCont[i] && ! fCont[i]->IsDestructed()) {
             fClass->Destructor(fCont[i],kTRUE);
          }
       }
    } else {
       for (Int_t i = 0; i < fSize; i++) {
-         if (fCont[i] && fCont[i]->TestBit(kNotDeleted)) {
+         if (fCont[i] && ! fCont[i]->IsDestructed()) {
             fCont[i]->~TObject();
          }
       }
@@ -516,7 +516,7 @@ void TClonesArray::ExpandCreate(Int_t n)
    for (i = 0; i < n; i++) {
       if (!fKeep->fCont[i]) {
          fKeep->fCont[i] = (TObject*)fClass->New();
-      } else if (!fKeep->fCont[i]->TestBit(kNotDeleted)) {
+      } else if (fKeep->fCont[i]->IsDestructed()) {
          // The object has been deleted (or never initialized)
          fClass->New(fKeep->fCont[i]);
       }
@@ -553,7 +553,7 @@ void TClonesArray::ExpandCreateFast(Int_t n)
    for (i = 0; i < n; i++) {
       if (i >= oldSize || !fKeep->fCont[i]) {
          fKeep->fCont[i] = (TObject*)fClass->New();
-      } else if (!fKeep->fCont[i]->TestBit(kNotDeleted)) {
+      } else if (fKeep->fCont[i]->IsDestructed()) {
          // The object has been deleted (or never initialized)
          fClass->New(fKeep->fCont[i]);
       }
@@ -575,7 +575,7 @@ TObject *TClonesArray::RemoveAt(Int_t idx)
 
    int i = idx-fLowerBound;
 
-   if (fCont[i] && fCont[i]->TestBit(kNotDeleted)) {
+   if (fCont[i] && ! fCont[i]->IsDestructed()) {
       fCont[i]->~TObject();
    }
 
@@ -601,7 +601,7 @@ TObject *TClonesArray::Remove(TObject *obj)
 
    if (i == -1) return nullptr;
 
-   if (fCont[i] && fCont[i]->TestBit(kNotDeleted)) {
+   if (fCont[i] && ! fCont[i]->IsDestructed()) {
       fCont[i]->~TObject();
    }
 
@@ -627,7 +627,7 @@ void TClonesArray::RemoveRange(Int_t idx1, Int_t idx2)
    Bool_t change = kFALSE;
    for (TObject **obj = fCont+idx1; obj <= fCont+idx2; obj++) {
       if (!*obj) continue;
-      if ((*obj)->TestBit(kNotDeleted)) {
+      if (!(*obj)->IsDestructed()) {
          (*obj)->~TObject();
       }
       *obj = nullptr;
@@ -801,7 +801,7 @@ void TClonesArray::Streamer(TBuffer &b)
          for (Int_t i = 0; i < nobjects; i++) {
             if (!fKeep->fCont[i]) {
                fKeep->fCont[i] = (TObject*)fClass->New();
-            } else if (!fKeep->fCont[i]->TestBit(kNotDeleted)) {
+            } else if (fKeep->fCont[i]->IsDestructed()) {
                // The object has been deleted (or never initialized)
                fClass->New(fKeep->fCont[i]);
             }
@@ -836,7 +836,7 @@ void TClonesArray::Streamer(TBuffer &b)
             if (nch) {
                if (!fKeep->fCont[i])
                   fKeep->fCont[i] = (TObject*)fClass->New();
-               else if (!fKeep->fCont[i]->TestBit(kNotDeleted)) {
+               else if (fKeep->fCont[i]->IsDestructed()) {
                   // The object has been deleted (or never initialized)
                   fClass->New(fKeep->fCont[i]);
                }
@@ -922,7 +922,7 @@ TObject *&TClonesArray::operator[](Int_t idx)
       fKeep->fCont[idx] = (TObject*) TStorage::ObjectAlloc(fClass->Size());
       // Reset the bit so that:
       //    obj = myClonesArray[i];
-      //    obj->TestBit(TObject::kNotDeleted)
+      //    ! obj->IsDestructed()
       // will behave correctly.
       // TObject::kNotDeleted is one of the higher bit that is not settable via the public
       // interface. But luckily we are its friend.

core/cont/src/TCollection.cxx

@@ -584,7 +584,7 @@ void TCollection::RecursiveRemove(TObject *obj)
    TObject *object;
 
    while ((object = next())) {
-      if (object->TestBit(kNotDeleted)) object->RecursiveRemove(obj);
+      if (!ROOT::Detail::HasBeenDeleted(object)) object->RecursiveRemove(obj);
    }
 }
 

core/cont/src/THashList.cxx

@@ -227,7 +227,7 @@ void THashList::Delete(Option_t *option)
          auto obj = tlk->GetObject();
          // In case somebody else access it.
          tlk->SetObject(nullptr);
-         if (obj && !obj->TestBit(kNotDeleted))
+         if (obj && ROOT::Detail::HasBeenDeleted(obj))
             Error("Delete", "A list is accessing an object (%p) already deleted (list name = %s)",
                   obj, GetName());
          else if (obj && obj->IsOnHeap())
@@ -350,7 +350,7 @@ void THashList::RecursiveRemove(TObject *obj)
    while (lnk.get()) {
       next = lnk->NextSP();
       TObject *ob = lnk->GetObject();
-      if (ob && ob->TestBit(kNotDeleted)) {
+      if (ob && !ROOT::Detail::HasBeenDeleted(ob)) {
          ob->RecursiveRemove(obj);
       }
       lnk = next;

core/cont/src/TList.cxx

@@ -440,12 +440,12 @@ void TList::Clear(Option_t *option)
       // delete only heap objects marked OK to clear
       auto obj = tlk->GetObject();
       if (!nodel && obj) {
-         if (!obj->TestBit(kNotDeleted)) {
+         if (ROOT::Detail::HasBeenDeleted(obj)) {
             Error("Clear", "A list is accessing an object (%p) already deleted (list name = %s)",
                   obj, GetName());
          } else if (obj->IsOnHeap()) {
             if (obj->TestBit(kCanDelete)) {
-               if (obj->TestBit(kNotDeleted)) {
+               if (!ROOT::Detail::HasBeenDeleted(obj)) {
                   TCollection::GarbageCollect(obj);
                }
             }
@@ -501,7 +501,7 @@ void TList::Delete(Option_t *option)
 
          // delete only heap objects
          auto obj = tlk->GetObject();
-         if (obj && !obj->TestBit(kNotDeleted))
+         if (obj && ROOT::Detail::HasBeenDeleted(obj))
             Error("Delete", "A list is accessing an object (%p) already deleted (list name = %s)",
                   obj, GetName());
          else if (obj && obj->IsOnHeap())
@@ -530,7 +530,7 @@ void TList::Delete(Option_t *option)
          // delete only heap objects
          auto obj = tlk->GetObject();
          tlk->SetObject(nullptr);
-         if (obj && !obj->TestBit(kNotDeleted))
+         if (obj && ROOT::Detail::HasBeenDeleted(obj))
             Error("Delete", "A list is accessing an object (%p) already deleted (list name = %s)",
                   obj, GetName());
          else if (obj && obj->IsOnHeap())
@@ -643,7 +643,7 @@ TObjLink *TList::FindLink(const TObject *obj, Int_t &idx) const
    while (lnk) {
       object = lnk->GetObject();
       if (object) {
-         if (object->TestBit(kNotDeleted)) {
+         if (!ROOT::Detail::HasBeenDeleted(object)) {
             if (object->IsEqual(obj)) return lnk;
          }
       }
@@ -773,7 +773,7 @@ void TList::RecursiveRemove(TObject *obj)
       auto cached = fCache.lock();
       if (cached && cached->fNext.get() == nullptr && cached->fPrev.lock().get() == nullptr) {
          TObject *ob = cached->GetObject();
-         if (ob && ob->TestBit(kNotDeleted)) {
+         if (ob && !ROOT::Detail::HasBeenDeleted(ob)) {
             ob->RecursiveRemove(obj);
          }
       }
@@ -787,7 +787,7 @@ void TList::RecursiveRemove(TObject *obj)
    while (lnk.get()) {
       next = lnk->fNext;
       TObject *ob = lnk->GetObject();
-      if (ob && ob->TestBit(kNotDeleted)) {
+      if (ob && !ROOT::Detail::HasBeenDeleted(ob)) {
          if (ob->IsEqual(obj)) {
             lnk->SetObject(nullptr);
             if (lnk == fFirst) {