From 385481e2983dc11e3aed79dbcd4fc5ef3b8d377c Mon Sep 17 00:00:00 2001 From: Juan Miguel Carceller Date: Thu, 22 Oct 2026 13:47:49 +0200 Subject: [PATCH 1/3] [treeplayer] Fix leak of the collection proxy in TBranchProxy --- tree/treeplayer/inc/TBranchProxy.h | 16 ++++++++++++++-- tree/treeplayer/src/TBranchProxy.cxx | 26 +++++++++++--------------- 2 files changed, 25 insertions(+), 17 deletions(-) diff --git a/tree/treeplayer/inc/TBranchProxy.h b/tree/treeplayer/inc/TBranchProxy.h index a0943f422d860..4d9925e15d61a 100644 --- a/tree/treeplayer/inc/TBranchProxy.h +++ b/tree/treeplayer/inc/TBranchProxy.h @@ -25,6 +25,7 @@ #include #include #include +#include class TBranch; class TStreamerElement; @@ -106,7 +107,18 @@ namespace Detail { Long64_t fRead; // Last entry read void *fWhere; // memory location of the data - TVirtualCollectionProxy *fCollection; // Handle to the collection containing the data chunk. + + // Detach the collection proxy from the data before deleting it: an emulated + // collection proxy clears the collection it is attached to on destruction, + // which, e.g. after a TChain switched files, may no longer exist. + struct CollectionDeleter { + void operator()(TVirtualCollectionProxy *collection) const + { + collection->PopProxy(); + delete collection; + } + }; + std::unique_ptr fCollection; //! Handle to the collection containing the data chunk. std::size_t fValueSize{}; // Size of the data type of the proxied branch. @@ -442,7 +454,7 @@ namespace Detail { return nullptr; } - TVirtualCollectionProxy *GetCollection() { return fCollection; } + TVirtualCollectionProxy *GetCollection() { return fCollection.get(); } // protected: virtual void *GetStart(UInt_t /*i*/=0) { diff --git a/tree/treeplayer/src/TBranchProxy.cxx b/tree/treeplayer/src/TBranchProxy.cxx index cff3b3537a939..4b5acfcadfab6 100644 --- a/tree/treeplayer/src/TBranchProxy.cxx +++ b/tree/treeplayer/src/TBranchProxy.cxx @@ -60,7 +60,7 @@ ROOT::Detail::TBranchProxy::TBranchProxy() : fClassName(""), fClass(nullptr), fElement(nullptr), fMemberOffset(0), fOffset(0), fArrayLength(1), fBranch(nullptr), fBranchCount(nullptr), fNotify(this), - fRead(-1), fWhere(nullptr),fCollection(nullptr) + fRead(-1), fWhere(nullptr) { }; @@ -74,7 +74,7 @@ ROOT::Detail::TBranchProxy::TBranchProxy(TBranchProxyDirector* boss, const char* fClassName(""), fClass(nullptr), fElement(nullptr), fMemberOffset(0), fOffset(0), fArrayLength(1), fBranch(nullptr), fBranchCount(nullptr), fNotify(this), - fRead(-1), fWhere(nullptr),fCollection(nullptr) + fRead(-1), fWhere(nullptr) { if (fBranchName.Length() && fBranchName[fBranchName.Length()-1]!='.' && name) { ((TString&)fBranchName).Append("."); @@ -92,7 +92,7 @@ ROOT::Detail::TBranchProxy::TBranchProxy(TBranchProxyDirector* boss, const char fClassName(""), fClass(nullptr), fElement(nullptr), fMemberOffset(0), fOffset(0), fArrayLength(1), fBranch(nullptr), fBranchCount(nullptr), fNotify(this), - fRead(-1), fWhere(nullptr),fCollection(nullptr) + fRead(-1), fWhere(nullptr) { if (name && strlen(name)) { if (fBranchName.Length() && fBranchName[fBranchName.Length()-1]!='.') { @@ -113,7 +113,7 @@ ROOT::Detail::TBranchProxy::TBranchProxy(TBranchProxyDirector* boss, Detail::TBr fClassName(""), fClass(nullptr), fElement(nullptr), fMemberOffset(0), fOffset(0), fArrayLength(1), fBranch(nullptr), fBranchCount(nullptr), fNotify(this), - fRead(-1), fWhere(nullptr),fCollection(nullptr) + fRead(-1), fWhere(nullptr) { if (name && strlen(name)) { if (fBranchName.Length() && fBranchName[fBranchName.Length()-1]!='.') { @@ -133,7 +133,7 @@ ROOT::Detail::TBranchProxy::TBranchProxy(TBranchProxyDirector* boss, TBranch* br fClassName(""), fClass(nullptr), fElement(nullptr), fMemberOffset(0), fOffset(0), fArrayLength(1), fBranch(nullptr), fBranchCount(nullptr), fNotify(this), - fRead(-1), fWhere(nullptr),fCollection(nullptr) + fRead(-1), fWhere(nullptr) { boss->Attach(this); } @@ -186,8 +186,7 @@ ROOT::Detail::TBranchProxy::TBranchProxy(TBranchProxyDirector *boss, const char fBranchCount(nullptr), fNotify(this), fRead(-1), - fWhere(nullptr), - fCollection(nullptr) + fWhere(nullptr) { // Constructor. @@ -220,8 +219,7 @@ void ROOT::Detail::TBranchProxy::Reset() fIsClone = false; fInitialized = false; fHasLeafCount = false; - delete fCollection; - fCollection = nullptr; + fCollection.reset(); } //////////////////////////////////////////////////////////////////////////////// @@ -277,8 +275,7 @@ bool ROOT::Detail::TBranchProxy::Setup() } else if (pcl->GetCollectionProxy()) { // We always skip the collections. - if (fCollection) delete fCollection; - fCollection = pcl->GetCollectionProxy()->Generate(); + fCollection.reset(pcl->GetCollectionProxy()->Generate()); pcl = fCollection->GetValueClass(); if (pcl == nullptr) { // coverity[dereference] fparent is checked jus a bit earlier and can not be null here @@ -414,8 +411,7 @@ bool ROOT::Detail::TBranchProxy::Setup() if (!fIsMember) fIsClone = true; fClass = clones->GetClass(); } else if (fClass && fClass->GetCollectionProxy()) { - delete fCollection; - fCollection = fClass->GetCollectionProxy()->Generate(); + fCollection.reset(fClass->GetCollectionProxy()->Generate()); fClass = fCollection->GetValueClass(); } @@ -437,7 +433,7 @@ bool ROOT::Detail::TBranchProxy::Setup() } else if (be->GetType()==4) { // top level TClonesArray - fCollection = be->GetCollectionProxy()->Generate(); + fCollection.reset(be->GetCollectionProxy()->Generate()); fIsaPointer = false; fWhere = be->GetObject(); @@ -449,7 +445,7 @@ bool ROOT::Detail::TBranchProxy::Setup() } else if (be->GetType()==41) { - fCollection = be->GetCollectionProxy()->Generate(); + fCollection.reset(be->GetCollectionProxy()->Generate()); fWhere = be->GetObject(); fOffset += be->GetOffset(); From 1f625a9ddb25e6eb88b3d7c328dd2dde52a0fab5 Mon Sep 17 00:00:00 2001 From: Juan Miguel Carceller Date: Thu, 22 Oct 2026 15:09:08 +0200 Subject: [PATCH 2/3] [treeplayer] Add a test reading a collection without a dictionary over a TChain --- roottest/root/tree/reader/CMakeLists.txt | 8 ++++ roottest/root/tree/reader/ChainNoDict.C | 38 +++++++++++++++++ .../root/tree/reader/CreateChainNoDictFiles.C | 41 +++++++++++++++++++ 3 files changed, 87 insertions(+) create mode 100644 roottest/root/tree/reader/ChainNoDict.C create mode 100644 roottest/root/tree/reader/CreateChainNoDictFiles.C diff --git a/roottest/root/tree/reader/CMakeLists.txt b/roottest/root/tree/reader/CMakeLists.txt index 04c821f2a07f1..dbeaa060ee23e 100644 --- a/roottest/root/tree/reader/CMakeLists.txt +++ b/roottest/root/tree/reader/CMakeLists.txt @@ -8,6 +8,14 @@ ROOTTEST_ADD_TEST(WarnNoDict ERRREF WarnNoDict.eref FIXTURES_REQUIRED root-tree-reader-WarnNoDict-fixture) +ROOTTEST_ADD_TEST(CreateChainNoDictFiles + MACRO CreateChainNoDictFiles.C+ + FIXTURES_SETUP root-tree-reader-ChainNoDict-fixture) + +ROOTTEST_ADD_TEST(ChainNoDict + MACRO ChainNoDict.C + FIXTURES_REQUIRED root-tree-reader-ChainNoDict-fixture) + # ROOT-10046 ROOTTEST_ADD_TEST(oddName COPY_TO_BUILDDIR root-10046.root diff --git a/roottest/root/tree/reader/ChainNoDict.C b/roottest/root/tree/reader/ChainNoDict.C new file mode 100644 index 0000000000000..85c058e6fdc21 --- /dev/null +++ b/roottest/root/tree/reader/ChainNoDict.C @@ -0,0 +1,38 @@ +// Read a data member of a split collection of a class without a dictionary +// over a TChain, which re-runs TBranchProxy::Setup() when switching files. +int ChainNoDict() +{ + auto c = TClass::GetClass("ChainNoDictHit"); + if (c && c->HasDictionary()) { + std::cerr << "Class ChainNoDictHit has a dictionary and it should not." << std::endl; + return 1; + } + + TChain chain("t"); + chain.Add("chainNoDict0.root"); + chain.Add("chainNoDict1.root"); + TTreeReader r(&chain); + TTreeReaderArray x(r, "hits.fX"); + int nValues = 0; + while (r.Next()) { + const auto entry = r.GetCurrentEntry(); + const int file = entry / 10; + if (x.GetSize() != static_cast(entry % 10)) { + std::cerr << "Wrong size " << x.GetSize() << " in entry " << entry << std::endl; + return 1; + } + for (std::size_t i = 0; i < x.GetSize(); ++i) { + const double expected = 100. * file + entry % 10 + 0.1 * i; + if (std::abs(x[i] - expected) > 1e-9) { + std::cerr << "Wrong value " << x[i] << " in entry " << entry << ", expected " << expected << std::endl; + return 1; + } + ++nValues; + } + } + if (nValues != 90) { + std::cerr << "Read " << nValues << " values, expected 90" << std::endl; + return 1; + } + return 0; +} diff --git a/roottest/root/tree/reader/CreateChainNoDictFiles.C b/roottest/root/tree/reader/CreateChainNoDictFiles.C new file mode 100644 index 0000000000000..43c3c19cd7a92 --- /dev/null +++ b/roottest/root/tree/reader/CreateChainNoDictFiles.C @@ -0,0 +1,41 @@ +#ifndef __CreateChainNoDictFiles__ +#define __CreateChainNoDictFiles__ + +#include "TFile.h" +#include "TTree.h" + +#include +#include +#include + +struct ChainNoDictHit { + double fX = 0; + int fId = 0; +}; + +#ifdef __ROOTCLING__ +#pragma link C++ class ChainNoDictHit+; +#pragma link C++ class std::vector+; +#endif + +// Write two files with a split std::vector of a struct. They are read in +// ChainNoDict.C without this dictionary, so that the collection proxy is emulated. +int CreateChainNoDictFiles() +{ + for (int file = 0; file < 2; ++file) { + auto f = std::make_unique(("chainNoDict" + std::to_string(file) + ".root").c_str(), "RECREATE"); + auto t = std::make_unique("t", "test tree"); + std::vector hits; + t->Branch("hits", &hits, 32000, 99); + for (int entry = 0; entry < 10; ++entry) { + hits.clear(); + for (int i = 0; i < entry; ++i) + hits.push_back({100. * file + entry + 0.1 * i, i}); + t->Fill(); + } + t->Write(); + } + return 0; +} + +#endif From ca56ea206c504ab9454cf2b6c1e9a3fee1cb79eb Mon Sep 17 00:00:00 2001 From: Juan Miguel Carceller Date: Fri, 23 Oct 2026 08:09:55 +0200 Subject: [PATCH 3/3] [treeplayer] Keep TBranchProxy copyable by sharing the collection proxy --- tree/treeplayer/inc/TBranchProxy.h | 5 ++++- tree/treeplayer/src/TBranchProxy.cxx | 8 ++++---- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/tree/treeplayer/inc/TBranchProxy.h b/tree/treeplayer/inc/TBranchProxy.h index 4d9925e15d61a..6a723a9acffff 100644 --- a/tree/treeplayer/inc/TBranchProxy.h +++ b/tree/treeplayer/inc/TBranchProxy.h @@ -114,11 +114,14 @@ namespace Detail { struct CollectionDeleter { void operator()(TVirtualCollectionProxy *collection) const { + if (!collection) + return; collection->PopProxy(); delete collection; } }; - std::unique_ptr fCollection; //! Handle to the collection containing the data chunk. + // Shared so that the proxies remain copyable, as the code generated by TTree::MakeProxy copies them + std::shared_ptr fCollection; //! Handle to the collection containing the data chunk. std::size_t fValueSize{}; // Size of the data type of the proxied branch. diff --git a/tree/treeplayer/src/TBranchProxy.cxx b/tree/treeplayer/src/TBranchProxy.cxx index 4b5acfcadfab6..4fd217e514b51 100644 --- a/tree/treeplayer/src/TBranchProxy.cxx +++ b/tree/treeplayer/src/TBranchProxy.cxx @@ -275,7 +275,7 @@ bool ROOT::Detail::TBranchProxy::Setup() } else if (pcl->GetCollectionProxy()) { // We always skip the collections. - fCollection.reset(pcl->GetCollectionProxy()->Generate()); + fCollection.reset(pcl->GetCollectionProxy()->Generate(), CollectionDeleter{}); pcl = fCollection->GetValueClass(); if (pcl == nullptr) { // coverity[dereference] fparent is checked jus a bit earlier and can not be null here @@ -411,7 +411,7 @@ bool ROOT::Detail::TBranchProxy::Setup() if (!fIsMember) fIsClone = true; fClass = clones->GetClass(); } else if (fClass && fClass->GetCollectionProxy()) { - fCollection.reset(fClass->GetCollectionProxy()->Generate()); + fCollection.reset(fClass->GetCollectionProxy()->Generate(), CollectionDeleter{}); fClass = fCollection->GetValueClass(); } @@ -433,7 +433,7 @@ bool ROOT::Detail::TBranchProxy::Setup() } else if (be->GetType()==4) { // top level TClonesArray - fCollection.reset(be->GetCollectionProxy()->Generate()); + fCollection.reset(be->GetCollectionProxy()->Generate(), CollectionDeleter{}); fIsaPointer = false; fWhere = be->GetObject(); @@ -445,7 +445,7 @@ bool ROOT::Detail::TBranchProxy::Setup() } else if (be->GetType()==41) { - fCollection.reset(be->GetCollectionProxy()->Generate()); + fCollection.reset(be->GetCollectionProxy()->Generate(), CollectionDeleter{}); fWhere = be->GetObject(); fOffset += be->GetOffset();