From 4b90a87d1faa1f233eebeea10f7d72a436d2a1d7 Mon Sep 17 00:00:00 2001 From: Vincenzo Eduardo Padulano Date: Fri, 12 Sep 2025 10:44:31 +0200 Subject: [PATCH 1/3] Revert "[treeplayer] prevent nullptr access in Restart (#19817)" This reverts commit d95cff9980db555459b6c775cefe85a89e6b18d7. --- tree/treeplayer/src/TTreeReader.cxx | 11 +++-------- tree/treeplayer/test/regressions.cxx | 28 ---------------------------- 2 files changed, 3 insertions(+), 36 deletions(-) diff --git a/tree/treeplayer/src/TTreeReader.cxx b/tree/treeplayer/src/TTreeReader.cxx index d3ea3bc4879b7..f338fbac1ec6c 100644 --- a/tree/treeplayer/src/TTreeReader.cxx +++ b/tree/treeplayer/src/TTreeReader.cxx @@ -567,14 +567,9 @@ void TTreeReader::Restart() fProxiesSet = false; // we might get more value readers, meaning new proxies. fEntry = -1; if (const auto curFile = fTree->GetCurrentFile()) { - if (!fTree->GetTree()) { - fTree->LoadTree(0); - } - if (fTree->GetTree()) { - if (auto tc = fTree->GetTree()->GetReadCache(curFile, true)) { - tc->DropBranch("*", true); - tc->ResetCache(); - } + if (auto tc = fTree->GetTree()->GetReadCache(curFile, true)) { + tc->DropBranch("*", true); + tc->ResetCache(); } } } diff --git a/tree/treeplayer/test/regressions.cxx b/tree/treeplayer/test/regressions.cxx index db6658f4b7b71..f263d57197373 100644 --- a/tree/treeplayer/test/regressions.cxx +++ b/tree/treeplayer/test/regressions.cxx @@ -263,31 +263,3 @@ TEST(TTreeFormulaRegressions, WrongName) EXPECT_EQ(t.Draw("s.Eta()", ""), -1); } } - -// https://github.com/root-project/root/issues/19814 -TEST(TTreeReaderRegressions, UninitializedChain) -{ - auto filename = "eve19814.root"; - auto treename = "events"; - auto brname = "x"; - const int refval = 19814; - { - TFile f(filename, "RECREATE"); - TTree t(treename, ""); - int x = refval; - t.Branch(brname, &x); - t.Fill(); - f.Write(); - } - { - TChain ch(treename); - ch.Add(filename); - TTreeReader reader(&ch); - reader.SetEntriesRange(0, ch.GetEntries()); - EXPECT_EQ(reader.GetEntries(), 1); - TTreeReaderValue x(reader, brname); - EXPECT_TRUE(reader.Next()); - EXPECT_EQ(*x, refval); - } - gSystem->Unlink(filename); -} From a5526789a82dc549a31ede9eb83985c1462ab6f5 Mon Sep 17 00:00:00 2001 From: Vincenzo Eduardo Padulano Date: Fri, 12 Sep 2025 12:21:07 +0200 Subject: [PATCH 2/3] [tree] Cleanup also current file in TChain::GetEntries The previous code was invalidating the tree after the full search through the list of files to gather the number of entries, but it was not taking care of the last file that had been opened. --- tree/tree/inc/TChain.h | 1 + tree/tree/src/TChain.cxx | 109 +++++++++++++++++++++------------------ 2 files changed, 59 insertions(+), 51 deletions(-) diff --git a/tree/tree/inc/TChain.h b/tree/tree/inc/TChain.h index 31e9b2a02516e..d6db02c3c80e8 100644 --- a/tree/tree/inc/TChain.h +++ b/tree/tree/inc/TChain.h @@ -50,6 +50,7 @@ class TChain : public TTree { void ParseTreeFilename(const char *name, TString &filename, TString &treename, TString &query, TString &suffix) const; Long64_t RefreshFriendAddresses(); + TTreeCache *CleanupCurrentFileAndTree(); protected: void InvalidateCurrentTree(); diff --git a/tree/tree/src/TChain.cxx b/tree/tree/src/TChain.cxx index 12f526c8314ba..f8b28ae94bb8a 100644 --- a/tree/tree/src/TChain.cxx +++ b/tree/tree/src/TChain.cxx @@ -893,6 +893,62 @@ Long64_t TChain::GetChainEntryNumber(Long64_t entry) const return entry + fTreeOffset[fTreeNumber]; } +TTreeCache *TChain::CleanupCurrentFileAndTree() +{ + // Delete file unless the file owns this chain! + // FIXME: The "unless" case here causes us to leak memory. + TTreeCache *tpf = nullptr; + if (fFile) { + if (!fDirectory->GetList()->FindObject(this)) { + if (fTree) { + // (fFile != 0 && fTree == 0) can happen when + // InvalidateCurrentTree is called (for example from + // AddFriend). Having fTree === 0 is necessary in that + // case because in some cases GetTree is used as a check + // to see if a TTree is already loaded. + // However, this prevent using the following to reuse + // the TTreeCache object. + tpf = fTree->GetReadCache(fFile); + if (tpf) { + tpf->ResetCache(); + } + + fFile->SetCacheRead(nullptr, fTree); + // If the tree has clones, copy them into the chain + // clone list so we can change their branch addresses + // when necessary. + // + // This is to support the syntax: + // + // TTree* clone = chain->GetTree()->CloneTree(0); + // + // We need to call the invalidate exactly here, since + // we no longer need the value of fTree and it is + // about to be deleted. + InvalidateCurrentTree(); + } + + if (fCanDeleteRefs) { + fFile->Close("R"); + } + delete fFile; + fFile = nullptr; + } else { + // If the tree has clones, copy them into the chain + // clone list so we can change their branch addresses + // when necessary. + // + // This is to support the syntax: + // + // TTree* clone = chain->GetTree()->CloneTree(0); + // + if (fTree) + InvalidateCurrentTree(); + } + } + return tpf; +} + //////////////////////////////////////////////////////////////////////////////// /// Return the total number of entries in the chain. /// In case the number of entries in each tree is not yet known, @@ -908,7 +964,7 @@ Long64_t TChain::GetEntries() const const auto readEntry = fReadEntry; auto *thisChain = const_cast(this); thisChain->LoadTree(TTree::kMaxEntries - 1); - thisChain->InvalidateCurrentTree(); + thisChain->CleanupCurrentFileAndTree(); if (readEntry >= 0) thisChain->LoadTree(readEntry); else @@ -1410,56 +1466,7 @@ Long64_t TChain::LoadTree(Long64_t entry) } // Delete the current tree and open the new tree. - TTreeCache* tpf = nullptr; - // Delete file unless the file owns this chain! - // FIXME: The "unless" case here causes us to leak memory. - if (fFile) { - if (!fDirectory->GetList()->FindObject(this)) { - if (fTree) { - // (fFile != 0 && fTree == 0) can happen when - // InvalidateCurrentTree is called (for example from - // AddFriend). Having fTree === 0 is necessary in that - // case because in some cases GetTree is used as a check - // to see if a TTree is already loaded. - // However, this prevent using the following to reuse - // the TTreeCache object. - tpf = fTree->GetReadCache(fFile); - if (tpf) { - tpf->ResetCache(); - } - - fFile->SetCacheRead(nullptr, fTree); - // If the tree has clones, copy them into the chain - // clone list so we can change their branch addresses - // when necessary. - // - // This is to support the syntax: - // - // TTree* clone = chain->GetTree()->CloneTree(0); - // - // We need to call the invalidate exactly here, since - // we no longer need the value of fTree and it is - // about to be deleted. - InvalidateCurrentTree(); - } - - if (fCanDeleteRefs) { - fFile->Close("R"); - } - delete fFile; - fFile = nullptr; - } else { - // If the tree has clones, copy them into the chain - // clone list so we can change their branch addresses - // when necessary. - // - // This is to support the syntax: - // - // TTree* clone = chain->GetTree()->CloneTree(0); - // - if (fTree) InvalidateCurrentTree(); - } - } + auto *tpf = CleanupCurrentFileAndTree(); TChainElement* element = (TChainElement*) fFiles->At(treenum); if (!element) { From afb31f5feb294381a48b6b43122cff77d3eca2ab Mon Sep 17 00:00:00 2001 From: Vincenzo Eduardo Padulano Date: Fri, 12 Sep 2025 12:24:58 +0200 Subject: [PATCH 3/3] [treeplayer] Add back regression test for #19814 Co-authored-by: ferdymercury --- tree/treeplayer/test/regressions.cxx | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/tree/treeplayer/test/regressions.cxx b/tree/treeplayer/test/regressions.cxx index f263d57197373..db6658f4b7b71 100644 --- a/tree/treeplayer/test/regressions.cxx +++ b/tree/treeplayer/test/regressions.cxx @@ -263,3 +263,31 @@ TEST(TTreeFormulaRegressions, WrongName) EXPECT_EQ(t.Draw("s.Eta()", ""), -1); } } + +// https://github.com/root-project/root/issues/19814 +TEST(TTreeReaderRegressions, UninitializedChain) +{ + auto filename = "eve19814.root"; + auto treename = "events"; + auto brname = "x"; + const int refval = 19814; + { + TFile f(filename, "RECREATE"); + TTree t(treename, ""); + int x = refval; + t.Branch(brname, &x); + t.Fill(); + f.Write(); + } + { + TChain ch(treename); + ch.Add(filename); + TTreeReader reader(&ch); + reader.SetEntriesRange(0, ch.GetEntries()); + EXPECT_EQ(reader.GetEntries(), 1); + TTreeReaderValue x(reader, brname); + EXPECT_TRUE(reader.Next()); + EXPECT_EQ(*x, refval); + } + gSystem->Unlink(filename); +}