[RF] Memory issue in destructor of RooStats::HypoTestInverterResult

[TomasDado]

Check duplicate issues.

• Checked for duplicates

Description

The attached code causes random crashes but it can be reliably identified as an issue with valgrind.
The example stack trace is:

===========================================================
There was a crash.
This is the entire stack trace of all threads:
===========================================================
#0  __syscall_cancel (a1=<optimized out>, a2=a2
entry=140730861103080, a3=a3
entry=0, a4=a4
entry=0, a5=a5
entry=0, a6=a6
entry=0, nr=61) at ./nptl/cancellation.c:77
#1  0x00007291be31b79f in __GI___wait4 (pid=<optimized out>, stat_loc=stat_loc
entry=0x7ffe74fc27e8, options=options
entry=0, usage=usage
entry=0x0) at ../sysdeps/unix/sysv/linux/wait4.c:30
#2  0x00007291be31b7eb in __GI___waitpid (pid=<optimized out>, stat_loc=stat_loc
entry=0x7ffe74fc27e8, options=options
entry=0) at ./posix/waitpid.c:38
#3  0x00007291be25bebb in do_system (line=<optimized out>) at ../sysdeps/posix/system.c:172
#4  0x00007291bff1a6fc in TUnixSystem::Exec (this=0x5d47e70e21c0, shellcmd=<optimized out>) at /home/tomas/ROOT_DEBUG/root_src/core/unix/src/TUnixSystem.cxx:2160
#5  TUnixSystem::StackTrace (this=0x5d47e70e21c0) at /home/tomas/ROOT_DEBUG/root_src/core/unix/src/TUnixSystem.cxx:2451
#6  0x00007291bff1a004 in TUnixSystem::DispatchSignals (this=0x5d47e70e21c0, sig=kSigSegmentationViolation) at /home/tomas/ROOT_DEBUG/root_src/core/unix/src/TUnixSystem.cxx:3671
#7  <signal handler called>
#8  TObjArray::Expand (this=this
entry=0x5d47eb599048, newSize=0) at /home/tomas/ROOT_DEBUG/root_src/core/cont/src/TObjArray.cxx:400
#9  0x00007291beeca9b9 in RooAbsArg::ioStreamerPass2Finalize () at /home/tomas/ROOT_DEBUG/root_src/roofit/roofitcore/src/RooAbsArg.cxx:2433
#10 0x00007291bf16d58a in RooWorkspace::Streamer (this=0x5d47eb2725f0, R__b=...) at /home/tomas/ROOT_DEBUG/root_src/roofit/roofitcore/src/RooWorkspace.cxx:2455
#11 0x00007291bf7adc06 in TClass::Streamer (this=0x5d47e9555e90, obj=0x5d47eb2725f0, b=..., onfile_class=0x0) at /home/tomas/ROOT_DEBUG/build/include/TClass.h:623
#12 TKey::ReadObjectAny (this=0x5d47eb7d64b0, expectedClass=<optimized out>) at /home/tomas/ROOT_DEBUG/root_src/io/io/src/TKey.cxx:1118
#13 0x00007291bf76acf1 in TDirectoryFile::GetObjectChecked (this=this
entry=0x5d47eb587460, namecycle=namecycle
entry=0x5d47eb7d64c9 "combined", expectedClass=0x5d47e9555e90) at /home/tomas/ROOT_DEBUG/root_src/io/io/src/TDirectoryFile.cxx:1128
#14 0x00007291beb399ff in TDirectory::Get<RooWorkspace> (this=<optimized out>, namecycle=0x5d47eb7d64c9 "combined") at /home/tomas/ROOT_DEBUG/build/include/TDirectory.h:207
#15 TDirectoryFile::Get<RooWorkspace> (this=<optimized out>, namecycle=0x5d47eb7d64c9 "combined") at /home/tomas/ROOT_DEBUG/build/include/TDirectoryFile.h:84
#16 ROOT::Experimental::XRooFit::xRooNode::xRooNode (this=0x7ffe74fc6c20, name=<optimized out>, comp=..., parent=std::shared_ptr<ROOT::Experimental::XRooFit::xRooNode> (empty) = {...}) at /home/tomas/ROOT_DEBUG/root_src/roofit/xroofit/src/xRooNode.cxx:297
#17 0x00005d47ca91d5df in draw () at /home/tomas/Reproducers/RooFit/double_free.cxx:13
#18 0x00005d47ca91d9c8 in main () at /home/tomas/Reproducers/RooFit/double_free.cxx:35

Valgrind gives:

==32996== 1 errors in context 4 of 64:
==32996== Invalid read of size 4
==32996==    at 0x4ADD8B2: TestBit (TObject.h:205)
==32996==    by 0x4ADD8B2: IsUsingRWLock (TCollection.h:212)
==32996==    by 0x4ADD8B2: TObjArray::GetAbsLast() const (TObjArray.cxx:545)
==32996==    by 0x55178D0: GetEntriesFast (TObjArray.h:59)
==32996==    by 0x55178D0: RooAbsArg::ioStreamerPass2Finalize() (RooAbsArg.cxx:2432)
==32996==    by 0x57BA589: RooWorkspace::Streamer(TBuffer&) (RooWorkspace.cxx:2455)
==32996==    by 0x4F11C05: Streamer (TClass.h:623)
==32996==    by 0x4F11C05: TKey::ReadObjectAny(TClass const*) (TKey.cxx:1118)
==32996==    by 0x4ECECF0: TDirectoryFile::GetObjectChecked(char const*, TClass const*) (TDirectoryFile.cxx:1128)
==32996==    by 0x5B9F9FE: Get<RooWorkspace> (TDirectory.h:207)
==32996==    by 0x5B9F9FE: Get<RooWorkspace> (TDirectoryFile.h:84)
==32996==    by 0x5B9F9FE: ROOT::Experimental::XRooFit::xRooNode::xRooNode(char const*, std::shared_ptr<TObject> const&, std::shared_ptr<ROOT::Experimental::XRooFit::xRooNode> const&) (xRooNode.cxx:297)
==32996==    by 0x40025BE: draw() (double_free.cxx:13)
==32996==    by 0x4002985: main (double_free.cxx:35)
==32996==  Address 0x27452084 is 436 bytes inside a block of size 1,000 free'd
==32996==    at 0x485766F: operator delete(void*) (vg_replace_malloc.c:1131)
==32996==    by 0x553AA1D: RooAbsCollection::deleteList() (RooAbsCollection.cxx:202)
==32996==    by 0x5542234: RooAbsCollection::~RooAbsCollection() (RooAbsCollection.cxx:178)
==32996==    by 0x7716FC1: ~SimpleInterval (SimpleInterval.h:20)
==32996==    by 0x7716FC1: RooStats::HypoTestInverterResult::~HypoTestInverterResult() (HypoTestInverterResult.cxx:185)
==32996==    by 0x7716FFC: RooStats::HypoTestInverterResult::~HypoTestInverterResult() (HypoTestInverterResult.cxx:185)
==32996==    by 0x56C1005: RooLinkedList::Delete(char const*) (RooLinkedList.cxx:580)
==32996==    by 0x57C434E: RooWorkspace::~RooWorkspace() (RooWorkspace.cxx:254)
==32996==    by 0x57C450C: RooWorkspace::~RooWorkspace() (RooWorkspace.cxx:260)
==32996==    by 0x4003B87: std::default_delete<RooWorkspace>::operator()(RooWorkspace*) const (unique_ptr.h:93)
==32996==    by 0x4003405: std::unique_ptr<RooWorkspace, std::default_delete<RooWorkspace> >::~unique_ptr() (unique_ptr.h:399)
==32996==    by 0x400287D: significance() (double_free.cxx:29)
==32996==    by 0x4002980: main (double_free.cxx:32)
==32996==  Block was alloc'd at
==32996==    at 0x4853F95: operator new(unsigned long) (vg_replace_malloc.c:487)
==32996==    by 0x4A7D9AC: TStorage::ObjectAlloc(unsigned long) (TStorage.cxx:293)
==32996==    by 0x580C1C9: operator new (TObject.h:187)
==32996==    by 0x580C1C9: ROOT::new_RooRealVar(void*) (G__RooFitCore.cxx:15778)
==32996==    by 0x4B19DF8: TClass::NewObject(TClass::ENewType, bool) const (TClass.cxx:5143)
==32996==    by 0x4B1D4F3: TClass::New(TClass::ENewType, bool) const (TClass.cxx:5120)
==32996==    by 0x4E81BB4: TBufferFile::ReadObjectAny(TClass const*) (TBufferFile.cxx:2625)
==32996==    by 0x4EFDE31: TGenCollectionStreamer::ReadObjects(int, TBuffer&, TClass const*) (TGenCollectionStreamer.cxx:395)
==32996==    by 0x4EFE666: TGenCollectionStreamer::ReadBufferGeneric(TBuffer&, void*, TClass const*) (TGenCollectionStreamer.cxx:1368)
==32996==    by 0x4E7B1B7: Streamer (TClass.h:623)
==32996==    by 0x4E7B1B7: TBufferFile::ReadFastArray(void*, TClass const*, int, TMemberStreamer*, TClass const*) (TBufferFile.cxx:1649)
==32996==    by 0x505D43E: ReadSTLObjectWiseFastArray (TStreamerInfoActions.cxx:1014)
==32996==    by 0x505D43E: int TStreamerInfoActions::ReadSTL<&TStreamerInfoActions::ReadSTLMemberWiseSameClass, &TStreamerInfoActions::ReadSTLObjectWiseFastArray>(TBuffer&, void*, TStreamerInfoActions::TConfiguration const*) (TStreamerInfoActions.cxx:4524)
==32996==    by 0x4E78C04: operator() (TStreamerInfoActions.h:123)
==32996==    by 0x4E78C04: TBufferFile::ApplySequence(TStreamerInfoActions::TActionSequence const&, void*) (TBufferFile.cxx:3747)
==32996==    by 0x4E80F2D: TBufferFile::ReadClassBuffer(TClass const*, void*, TClass const*) (TBufferFile.cxx:3666)
==32996==    by 0x4F421B5: TStreamerInfoActions::ReadViaClassBuffer(TBuffer&, void*, TStreamerInfoActions::TConfiguration const*) (TStreamerInfoActions.cxx:339)
==32996==    by 0x4E78C04: operator() (TStreamerInfoActions.h:123)
==32996==    by 0x4E78C04: TBufferFile::ApplySequence(TStreamerInfoActions::TActionSequence const&, void*) (TBufferFile.cxx:3747)
==32996==    by 0x4E80F2D: TBufferFile::ReadClassBuffer(TClass const*, void*, TClass const*) (TBufferFile.cxx:3666)
==32996==    by 0x5176F18: Streamer (TClass.h:623)
==32996==    by 0x5176F18: int TStreamerInfo::ReadBuffer<char**>(TBuffer&, char** const&, TStreamerInfo::TCompInfo* const*, int, int, int, int, int) (TStreamerInfoReadBuffer.cxx:1376)
==32996==    by 0x4F3F5A3: TStreamerInfoActions::GenericReadAction(TBuffer&, void*, TStreamerInfoActions::TConfiguration const*) (TStreamerInfoActions.cxx:254)
==32996==    by 0x4E78C04: operator() (TStreamerInfoActions.h:123)
==32996==    by 0x4E78C04: TBufferFile::ApplySequence(TStreamerInfoActions::TActionSequence const&, void*) (TBufferFile.cxx:3747)
==32996==    by 0x4E80F2D: TBufferFile::ReadClassBuffer(TClass const*, void*, TClass const*) (TBufferFile.cxx:3666)
==32996==    by 0x4F421B5: TStreamerInfoActions::ReadViaClassBuffer(TBuffer&, void*, TStreamerInfoActions::TConfiguration const*) (TStreamerInfoActions.cxx:339)
==32996==    by 0x4E78C04: operator() (TStreamerInfoActions.h:123)
==32996==    by 0x4E78C04: TBufferFile::ApplySequence(TStreamerInfoActions::TActionSequence const&, void*) (TBufferFile.cxx:3747)
==32996==    by 0x4E80F2D: TBufferFile::ReadClassBuffer(TClass const*, void*, TClass const*) (TBufferFile.cxx:3666)
==32996==    by 0x4ED166D: TDirectoryFile::CloneObject(TObject const*, bool) (TDirectoryFile.cxx:427)
==32996==    by 0x4A558C0: TNamed::Clone(char const*) const (TNamed.cxx:76)
==32996==    by 0x57C6BA1: RooWorkspace::import(TObject const&, bool) (RooWorkspace.cxx:1918)
==32996==    by 0x4002816: significance() (double_free.cxx:26)
==32996==    by 0x4002980: main (double_free.cxx:32)

Reproducer

#include "TFile.h"
#include "RooWorkspace.h"
#include "RooFit/xRooFit/xRooNode.h"
#include "RooStats/HypoTestInverterResult.h"

#include <memory>

using ROOT::Experimental::XRooFit::xRooNode;

std::string path = "workspace.root";

void draw() {
  xRooNode node(path.c_str());
}

void significance() {
  std::unique_ptr<TFile> file(TFile::Open(path.c_str(), "read"));
  std::unique_ptr<RooWorkspace> ws(file->Get<RooWorkspace>("combined"));

  xRooNode model(*ws);
  auto hs = model.nll("obsData").hypoSpace("mu_WH", ROOT::Experimental::XRooFit::xRooFit::Asymptotics::Uncapped, 1);

  auto& hp = hs.AddPoint(0.);
  std::unique_ptr<RooStats::HypoTestInverterResult> result(hs.result());
  model.ws()->import(*result);

  file->Close();
}

int main() {
  significance();

  // commenting out the following line "fixes" the memory issue
  draw();
}

With CMake:

cmake_minimum_required(VERSION 3.27)
project(mwe)

set( CMAKE_CXX_FLAGS "-g" )

find_package( xRooFit QUIET )
if(NOT xRooFit_FOUND)
   set( _xRooFitComp "RooFitXRooFit" )
   message(STATUS "Using xRooFit From ROOT")
else()
    message(STATUS "Using xRooFit standalone")
    add_definitions( -DSA_XROOFIT ) #used in c++, see example below
endif()

find_package( ROOT COMPONENTS RooFitCore RooFit ${_xRooFitComp} )

add_executable(double_free double_free.cxx)
target_include_directories(double_free PUBLIC ${ROOT_INCLUDE_DIRS} ${XROOFIT_INCLUDE_DIRS} )
target_link_libraries(double_free PUBLIC ${ROOT_LIBRARIES} ${XROOFIT_LIBRARIES} )

The input file can be found at: /afs/cern.ch/user/t/tdado/public/reproducers/workspace.root

ROOT version

ROOT Version: 6.37.01
Built for linuxx8664gcc on May 23 2025, 06:46:45
From heads/master@v6-37-01-6741-g9ff60071f40

Installation method

Built from source

Operating system

Ubuntu 25.04

Additional context

No response

[guitargeek]

Hi @pcanal, why was this reopened? It's true that the schema evolution of RooAbsArg still has some flaws, but this particular issue was solved. The HypoTestInverterResult only contains RooRealVars, which are RooAbsArg-derived classes with no dependencies on other RooAbsArgs. As of my PR, the evolution of the proxy objects that manage these dependencies is skipped for RooAbsArgs with no dependencies, so this issue is gone.

[guitargeek]

To clarify even more: as you can see from the stack trace posted by @TomasDado, the double free happens because the RooAbsArgs are already deleted by the HypoTestInverter destructor before the RooWorkspace can finish its second pass for the schema evolution. That's because it's base class, SimpleInterval, indeed owns some RooRealVar parameters in this RooArgSet:

https://github.com/root-project/root/blob/master/roofit/roostats/inc/RooStats/SimpleInterval.h#L62

[pcanal]

I am too fast in re-opening this PR based on discovering that the underlying code was not thread safe but not realizing that this fact was pre-existing.