Crash when exiting python if a writable TFile is still open (appears to be modification of freed object?)

[will-cern]

Check duplicate issues.

• Checked for duplicates

Description

I have been trying to isolate a nasty crash that occurs when python exits. It appears to be triggered when there are writable TFiles still open at the end of the script. The reproducer looks a bit strange but is reflecting the sort of file my code was building. I do not know if the problem is related to the specific writing of those objects (a FitConfig and a RooLinkedList) but the reproducer does at least reproduce the crash 9 times out of 10.

If the files are closed before the end of the script, there is no crash.

The crash seems to occur because of the call to the WriteStreamerInfo method of the writable files when they are being closed. It appears as if something that the StreamerInfo needs has perhaps already been destroyed?

When the crash occurs on my mac, it reports that there is a modification of an already freed piece of memory:

python(80807,0x7ff8572ddfc0) malloc: Heap corruption detected, free list is damaged at 0x600006d00240

Here's the stack trace I get from attaching a debugger:

__pthread_kill 0x00007ff813d6414a
pthread_kill 0x00007ff813d9cebd
abort 0x00007ff813cc2a79
malloc_vreport 0x00007ff813bc73a9
malloc_zone_error 0x00007ff813beac5b
nanov2_guard_corruption_detected 0x00007ff813bdc389
nanov2_allocate_outlined 0x00007ff813bdb692
nanov2_malloc 0x00007ff813bba3c7
operator new(unsigned long) 0x00007ff813d5734a
std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>* std::__1::vector<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, std::__1::allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>>>::__push_back_slow_path<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&>(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&) 0x0000000105af6c4f
TClassEdit::GetSplit(char const*, std::__1::vector<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, std::__1::allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>>>&, int&, TClassEdit::EModType) 0x0000000105b48f57
TClassEdit::GetNormalizedName(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>&, std::__1::basic_string_view<char, std::__1::char_traits<char>>) 0x0000000105b4bb0a
TEnum::GetEnum(char const*, TEnum::ESearchAction) 0x0000000105b7b653
TDataMember::Init(bool) 0x0000000105b75a01
TDataMember::TDataMember(DataMemberInfo_t*, TClass*) 0x0000000105b7562b
TListOfDataMembers::Get(DataMemberInfo_t*, bool) 0x0000000105b8187b
TListOfDataMembers::Load() 0x0000000105b81dbe
TClass::CreateListOfDataMembers(std::__1::atomic<TListOfDataMembers*>&, TDictionary::EMemberSelection, bool) 0x0000000105b69913
TClass::GetDataMember(char const*) const 0x0000000105b5e6cf
TBuildRealData::Inspect(TClass*, char const*, char const*, void const*, bool) 0x0000000105b5e840
TCling::InspectMembers(TMemberInspector&, void const*, TClass const*, bool) 0x000000010c517b24
TCling::InspectMembers(TMemberInspector&, void const*, TClass const*, bool) 0x000000010c518034
TClass::CallShowMembers(void const*, TMemberInspector&, bool) const 0x0000000105b63f5a
TClass::BuildRealData(void*, bool) 0x0000000105b5f17d
TStreamerInfo::Build(bool) 0x00000001064ab007
TClass::GetStreamerInfoImpl(int, bool) const 0x0000000105b64db7
TClass::GetStreamerInfo(int, bool) const 0x0000000105b5e78d
TBufferFile::WriteClassBuffer(TClass const*, void*) 0x000000010640d8eb
TBufferFile::WriteObjectClass(void const*, TClass const*, bool) 0x000000010640ba29
TBufferIO::WriteObjectAny(void const*, TClass const*, bool) 0x0000000106411cff
TObjArray::Streamer(TBuffer&) 0x0000000105b2e94a
TBufferFile::WriteObjectClass(void const*, TClass const*, bool) 0x000000010640ba29
TBufferIO::WriteObjectAny(void const*, TClass const*, bool) 0x0000000106411d17
TStreamerInfo::Streamer(TBuffer&) 0x00000001064bed3b
TBufferFile::WriteObjectClass(void const*, TClass const*, bool) 0x000000010640ba29
TBufferIO::WriteObjectAny(void const*, TClass const*, bool) 0x0000000106411cff
TList::Streamer(TBuffer&) 0x0000000105b2af5a
TKey::TKey(TObject const*, char const*, int, TDirectory*) 0x000000010649f0a4
TFile::WriteStreamerInfo() 0x000000010647602d
TFile::Close(char const*) 0x000000010646c3a6
TROOT::CloseFiles() 0x0000000105ac8415
__cxa_finalize_ranges 0x00007ff813c6f238
exit 0x00007ff813c6f052
dyld4::LibSystemHelpers::exit(int) const 0x00007ff813db4de3
start 0x00007ff813a11397

Reproducer

Run the following as a python script:

import ROOT

ROOT.gInterpreter.ProcessLine("""                                                                                                                                         
   auto f = new TFile("/tmp/testFile.root","RECREATE");                                                                                                                   
   auto fr = new ROOT::Fit::FitConfig;                                                                                                                                    
   RooLinkedList l;                                                                                                                                                       
   l.Add(RooFit::Offset().Clone());                                                                                                                                       
   f->WriteObject(fr,"blah");                                                                                                                                             
   f->WriteObject(&l,"myList");                                                                                                                                           
""")

# ROOT.gROOT.CloseFiles() # this fixes it     

ROOT version

tested with 6.36.00 and 6.34.08

Installation method

build from source

Operating system

macos and linux

Additional context

No response

[ferdymercury]

Maybe related: #19076 and https://its.cern.ch/jira/browse/ROOT-4855

[vepadulano]

Hi @will-cern ,

Thanks for the report! While I'm no RooFit expert, I would argue that your code is not written using modern C++ standards. The following snippet works on my machine and on lxplus, produces no segfaults, and takes care of the memory management of the TFile using modern C++ guidelines.

import ROOT

ROOT.gInterpreter.ProcessLine("""                                                                                                                           
{
   auto f = std::make_unique<TFile>("test_file_19077.root","RECREATE");                                                                                                                   
   auto fr = std::make_unique<ROOT::Fit::FitConfig>();                                                                                                                                    
   RooLinkedList l;                                                                                                                                                       
   l.Add(RooFit::Offset().Clone());                                                                                                                                       
   f->WriteObject(fr.get(),"blah");                                                                                                                                             
   f->WriteObject(&l,"myList");                                                                                                                             
}
""")

It appears as if something that the StreamerInfo needs has perhaps already been destroyed?

Without following this type of pattern, your description from above makes sense to me and I wouldn't expect any other behaviour from the application.

[will-cern]

Hello @vepadulano ,

Ok I see your concern, but you've worked around the problem by putting the unique_ptr to the TFile inside a c++ code block, meaning that the TFile will be closed and destroyed at the end of the block, rather than at the end of the script.

I still get the crash if I remove the code block, so that the TFile persists in memory at the time the script terminates (you can see this with ROOT.gROOT.GetListOfFiles().Print()

import ROOT

ROOT.gInterpreter.ProcessLine("""                                                                                                                           
   auto f = std::make_unique<TFile>("test_file_19077.root","RECREATE");                                                                                                                   
   auto fr = std::make_unique<ROOT::Fit::FitConfig>();                                                                                                                                    
   RooLinkedList l;                                                                                                                                                       
   l.Add(RooFit::Offset().Clone());                                                                                                                                       
   f->WriteObject(fr.get(),"blah");                                                                                                                                             
   f->WriteObject(&l,"myList");                                                                                                                             
""")

Seeing as ROOT has the apparent capability of having some files still open at the close out, It would be nice if ROOT could gracefully handle the closing of open writable files at teardown time, rather than just crashing out. I'd tolerate a warning about open files being closed if you still really believe nobody should ever have left a writable file open.

[vepadulano]

gracefully handle the closing of open writable files at teardown time,

This is interesting, as I'm pretty sure something is already done about this by default. I believe this is the part that should be investigated, indeed.

[ferdymercury]

A bit related: #18403