Non-deterministic behaviour in deleting object in nested `TCollections` about root

[ferdymercury]

If you run it with:

valgrind --leak-check=full --suppressions=$ROOTSYS/etc/valgrind-root.supp root.exe -l -b -q test.cpp+

you'll get:

==721499== Invalid read of size 4
==721499==    at 0x4AF2199: TList::Clear(char const*) (in /opt/root/lib/libCore.so.6.30.04)
==721499==    by 0x4AF26D4: TList::~TList() (in /opt/root/lib/libCore.so.6.30.04)
==721499==    by 0x4AF287C: TList::~TList() (in /opt/root/lib/libCore.so.6.30.04)
==721499==    by 0x2729A8D9: deleteTCollections(TObject*) (in /tmp/tmp/test_cpp.so)
==721499==    by 0x2729F039: ???
==721499==    by 0x6AA94F7: cling::IncrementalExecutor::executeWrapper(llvm::StringRef, cling::Value*) const (in /opt/root/lib/libCling.so.6.30.04)
==721499==    by 0x6A31E4B: cling::Interpreter::RunFunction(clang::FunctionDecl const*, cling::Value*) (in /opt/root/lib/libCling.so.6.30.04)
==721499==    by 0x6A32596: cling::Interpreter::EvaluateInternal(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, cling::CompilationOptions, cling::Value*, cling::Transaction**, unsigned long) (in /opt/root/lib/libCling.so.6.30.04)
==721499==    by 0x6A327B7: cling::Interpreter::process(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, cling::Value*, cling::Transaction**, bool) (in /opt/root/lib/libCling.so.6.30.04)
==721499==    by 0x6B09BC6: cling::MetaProcessor::process(llvm::StringRef, cling::Interpreter::CompilationResult&, cling::Value*, bool) (in /opt/root/lib/libCling.so.6.30.04)
==721499==    by 0x693813B: HandleInterpreterException(cling::MetaProcessor*, char const*, cling::Interpreter::CompilationResult&, cling::Value*) (in /opt/root/lib/libCling.so.6.30.04)
==721499==    by 0x694C6E7: TCling::ProcessLine(char const*, TInterpreter::EErrorCode*) (in /opt/root/lib/libCling.so.6.30.04)
==721499==  Address 0xf0ff6dc is 12 bytes inside a block of size 1,000 free'd
==721499==    at 0x484B8AF: operator delete(void*) (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==721499==    by 0x2729A8D9: deleteTCollections(TObject*) (in /tmp/tmp/test_cpp.so)
==721499==    by 0x2729A8D9: deleteTCollections(TObject*) (in /tmp/tmp/test_cpp.so)
==721499==    by 0x2729F039: ???
==721499==    by 0x6AA94F7: cling::IncrementalExecutor::executeWrapper(llvm::StringRef, cling::Value*) const (in /opt/root/lib/libCling.so.6.30.04)
==721499==    by 0x6A31E4B: cling::Interpreter::RunFunction(clang::FunctionDecl const*, cling::Value*) (in /opt/root/lib/libCling.so.6.30.04)
==721499==    by 0x6A32596: cling::Interpreter::EvaluateInternal(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, cling::CompilationOptions, cling::Value*, cling::Transaction**, unsigned long) (in /opt/root/lib/libCling.so.6.30.04)
==721499==    by 0x6A327B7: cling::Interpreter::process(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, cling::Value*, cling::Transaction**, bool) (in /opt/root/lib/libCling.so.6.30.04)
==721499==    by 0x6B09BC6: cling::MetaProcessor::process(llvm::StringRef, cling::Interpreter::CompilationResult&, cling::Value*, bool) (in /opt/root/lib/libCling.so.6.30.04)
==721499==    by 0x693813B: HandleInterpreterException(cling::MetaProcessor*, char const*, cling::Interpreter::CompilationResult&, cling::Value*) (in /opt/root/lib/libCling.so.6.30.04)
==721499==    by 0x694C6E7: TCling::ProcessLine(char const*, TInterpreter::EErrorCode*) (in /opt/root/lib/libCling.so.6.30.04)
==721499==    by 0x694CB7A: TCling::ProcessLineSynch(char const*, TInterpreter::EErrorCode*) (in /opt/root/lib/libCling.so.6.30.04)
==721499==  Block was alloc'd at
==721499==    at 0x4849013: operator new(unsigned long) (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==721499==    by 0x4A95EDD: TStorage::ObjectAlloc(unsigned long) (in /opt/root/lib/libCore.so.6.30.04)
==721499==    by 0x2729AAAE: test() (in /tmp/tmp/test_cpp.so)
==721499==    by 0x2729F039: ???
==721499==    by 0x6AA94F7: cling::IncrementalExecutor::executeWrapper(llvm::StringRef, cling::Value*) const (in /opt/root/lib/libCling.so.6.30.04)
==721499==    by 0x6A31E4B: cling::Interpreter::RunFunction(clang::FunctionDecl const*, cling::Value*) (in /opt/root/lib/libCling.so.6.30.04)
==721499==    by 0x6A32596: cling::Interpreter::EvaluateInternal(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, cling::CompilationOptions, cling::Value*, cling::Transaction**, unsigned long) (in /opt/root/lib/libCling.so.6.30.04)
==721499==    by 0x6A327B7: cling::Interpreter::process(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, cling::Value*, cling::Transaction**, bool) (in /opt/root/lib/libCling.so.6.30.04)
==721499==    by 0x6B09BC6: cling::MetaProcessor::process(llvm::StringRef, cling::Interpreter::CompilationResult&, cling::Value*, bool) (in /opt/root/lib/libCling.so.6.30.04)
==721499==    by 0x693813B: HandleInterpreterException(cling::MetaProcessor*, char const*, cling::Interpreter::CompilationResult&, cling::Value*) (in /opt/root/lib/libCling.so.6.30.04)
==721499==    by 0x694C6E7: TCling::ProcessLine(char const*, TInterpreter::EErrorCode*) (in /opt/root/lib/libCling.so.6.30.04)
==721499==    by 0x694CB7A: TCling::ProcessLineSynch(char const*, TInterpreter::EErrorCode*) (in /opt/root/lib/libCling.so.6.30.04)
==721499== 
Error in <TList::Clear>: A list is accessing an object (0xf0ff6d0) already deleted (list name = TList)

[mconcas]

I conclude that manually deleting the objects is not ok, even if the list is not the owner of the objects.
Am I missing any good practice here?

(edit: I misread the nodelete flag in the TList::Clear)

[pcanal]

If you are deleting an object that is contained in a TCollection, you must remove it from that collection, otherwise the collection has a dangling pointer (that it will use during certain operations, including Clear but could be any other operation, etc ls() ).

[ferdymercury]

Maybe setting the content of the dangling TList pointer to NULL just after delete helps ?

[mconcas]

If you are deleting an object that is contained in a TCollection, you must remove it from that collection, otherwise the collection has a dangling pointer (that it will use during certain operations, including Clear but could be any other operation, etc ls() ).

Ok, that makes sense, and indeed, it is impossible to do the other way around (i.e. delete obj before and Remove(obj) from the list after) as there is no obvious way to know if the object has been deleted. So, there is little you can do from the ROOT side.

Maybe setting the content of the dangling TList pointer to NULL just after delete helps ?

It would break the recursive structure of the deleteTCollections function, which we'll need to rethink.

Thanks for the clarifications and for your time.

[github-actions]

Hi @mconcas,

It appears this issue is closed, but wasn't yet added to a project. Please add upcoming versions that will include the fix, or 'not applicable' otherwise.

Sincerely,
🤖

[wdconinc]

We are seeing similar issues as a regression when writing ROOT files with 6.30 vs 6.28 (using AIDASoft/podio), and then opening them in older ROOT versions. We're still narrowing this into a reproducer but it seems like something changed in this context to make this issue appear now, with 6.30...

File written by 6.30, read by 6.24

File written by 6.28, read by 6.24

[wdconinc]

(I'll file a new issue later today and refer to this one. Consider this a heads-up :-) )

[chrisburr]

@wdconinc did you open this issue anywhere? I've also ran what appears to be the same issue.

[wdconinc]

No, unfortunately I haven't found the time yet for that one :-) It's been hard to write a reproducer that isn't "take our entire software stack and run a full simulation".