From 5fc9c32c24349507e6737ecdf69bcf63cf004963 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Wed, 8 Jul 2026 10:33:31 +0200 Subject: [PATCH] [treeplayer] Don't mistake formulas for script files in TTree::Draw MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `TTree::Draw` can take the name of a C++ script file instead of a TTreeFormula expression for both the variable expression and the selection. To decide between the two, `TTreePlayer::DrawSelect` checked whether a matching file exists via `TSystem::IsFileInIncludePath()`, which strips a trailing parenthesized part as ACLiC-style macro arguments. A formula like `"abs(Bs_TRUEID)"` was thus reduced to `"abs"`, and if an unrelated file with that name happened to exist in the current directory, the Draw call failed: ``` root [0] tree->Draw("Bs_MCORR", "abs(Bs_TRUEID)") Error in : Drawing using a C++ file currently requires that both the expression and the selection are files: "Bs_MCORR" is not a file ``` Two inconsistencies made the trap wider: the selection branch did not require a dot in the candidate string at all, and the varexp branch checked for a dot before stripping the arguments, so a decimal constant inside the parentheses (e.g. `"abs(x+0.5)"`) satisfied it. Factor the decision into a single helper used for both arguments, which requires the candidate - after stripping the ACLiC mode and arguments - to have an extension. This is not a new restriction: `TTreeProxyGenerator::WriteProxy()` already refuses a script without an extension (the function name is derived from the basename minus the extension, for the cut script too), so such a file could never be processed anyway. It now evaluates as a formula instead of failing with a misleading error. Add a regression test that exercises the previously failing Draw calls in the presence of a shadowing file named "abs". Fixes https://its.cern.ch/jira/browse/ROOT-8000 🤖 Done with the help of AI --- tree/treeplayer/src/TTreePlayer.cxx | 79 +++++++++++++++++----------- tree/treeplayer/test/regressions.cxx | 33 ++++++++++++ 2 files changed, 82 insertions(+), 30 deletions(-) diff --git a/tree/treeplayer/src/TTreePlayer.cxx b/tree/treeplayer/src/TTreePlayer.cxx index 4065337e4d956..c00372a029981 100644 --- a/tree/treeplayer/src/TTreePlayer.cxx +++ b/tree/treeplayer/src/TTreePlayer.cxx @@ -274,6 +274,46 @@ void TTreePlayer::DeleteSelectorFromFile() fSelectorClass = nullptr; } +namespace { + +//////////////////////////////////////////////////////////////////////////////// +/// TTree::Draw can take the name of a C++ script file (optionally with an +/// ACLiC mode suffix and arguments, e.g. "myscript.C+(2)") instead of a +/// TTreeFormula expression for both the variable expression and the +/// selection; see "Drawing a user function accessing the TTree data directly" +/// in the TTree::Draw documentation. Determine whether 'expression' names +/// such a script file. +/// +/// Besides checking that the file exists, require that it has an extension: +/// TTreeProxyGenerator::WriteProxy() needs one to derive the name of the +/// function to call. This avoids misinterpreting an expression like "abs(x)" +/// as the script "abs" called with argument "(x)" whenever an unrelated file +/// with that name happens to exist in the current directory (JIRA ROOT-8000). + +bool IsScriptFile(const char *expression) +{ + if (!expression || !expression[0]) + return false; + + const TString candidate = expression; + if (candidate.Index("Alt$") >= 0 || candidate.Index("Entries$") >= 0 || candidate.Index("LocalEntries$") >= 0 || + candidate.Index("Length$") >= 0 || candidate.Index("Entry$") >= 0 || candidate.Index("LocalEntry$") >= 0 || + candidate.Index("Min$") >= 0 || candidate.Index("Max$") >= 0 || candidate.Index("MinIf$") >= 0 || + candidate.Index("MaxIf$") >= 0 || candidate.Index("Iteration$") >= 0 || candidate.Index("Sum$") >= 0 || + candidate.Index(">") >= 0 || candidate.Index("<") >= 0) + return false; + + TString aclicMode, arguments, io; + const TString realname = gSystem->SplitAclicMode(candidate, aclicMode, arguments, io); + const Ssiz_t dot_pos = realname.Last('.'); + if (dot_pos == kNPOS || dot_pos < realname.Last('/')) + return false; + + return gSystem->IsFileInIncludePath(candidate); +} + +} // anonymous namespace + //////////////////////////////////////////////////////////////////////////////// /// Draw the result of a C++ script. /// @@ -357,20 +397,9 @@ Long64_t TTreePlayer::DrawSelect(const char *varexp0, const char *selection, Opt // Let's see if we have a filename as arguments instead of // a TTreeFormula expression. - TString possibleFilename = varexp0; - Ssiz_t dot_pos = possibleFilename.Last('.'); - if ( dot_pos != kNPOS - && possibleFilename.Index("Alt$")<0 && possibleFilename.Index("Entries$")<0 - && possibleFilename.Index("LocalEntries$")<0 - && possibleFilename.Index("Length$")<0 && possibleFilename.Index("Entry$")<0 - && possibleFilename.Index("LocalEntry$")<0 - && possibleFilename.Index("Min$")<0 && possibleFilename.Index("Max$")<0 - && possibleFilename.Index("MinIf$")<0 && possibleFilename.Index("MaxIf$")<0 - && possibleFilename.Index("Iteration$")<0 && possibleFilename.Index("Sum$")<0 - && possibleFilename.Index(">")<0 && possibleFilename.Index("<")<0 - && gSystem->IsFileInIncludePath(possibleFilename.Data())) { - - if (selection && strlen(selection) && !gSystem->IsFileInIncludePath(selection)) { + if (IsScriptFile(varexp0)) { + + if (selection && strlen(selection) && !IsScriptFile(selection)) { Error("DrawSelect", "Drawing using a C++ file currently requires that both the expression and the selection are files\n\t\"%s\" is not a file", selection); @@ -378,23 +407,13 @@ Long64_t TTreePlayer::DrawSelect(const char *varexp0, const char *selection, Opt } return DrawScript("generatedSel",varexp0,selection,option,nentries,firstentry); - } else { - possibleFilename = selection; - if (possibleFilename.Index("Alt$")<0 && possibleFilename.Index("Entries$")<0 - && possibleFilename.Index("LocalEntries$")<0 - && possibleFilename.Index("Length$")<0 && possibleFilename.Index("Entry$")<0 - && possibleFilename.Index("LocalEntry$")<0 - && possibleFilename.Index("Min$")<0 && possibleFilename.Index("Max$")<0 - && possibleFilename.Index("MinIf$")<0 && possibleFilename.Index("MaxIf$")<0 - && possibleFilename.Index("Iteration$")<0 && possibleFilename.Index("Sum$")<0 - && possibleFilename.Index(">")<0 && possibleFilename.Index("<")<0 - && gSystem->IsFileInIncludePath(possibleFilename.Data())) { + } else if (IsScriptFile(selection)) { - Error("DrawSelect", - "Drawing using a C++ file currently requires that both the expression and the selection are files\n\t\"%s\" is not a file", - varexp0); - return 0; - } + Error("DrawSelect", + "Drawing using a C++ file currently requires that both the expression and the selection are " + "files\n\t\"%s\" is not a file", + varexp0); + return 0; } Long64_t oldEstimate = fTree->GetEstimate(); diff --git a/tree/treeplayer/test/regressions.cxx b/tree/treeplayer/test/regressions.cxx index 37d978031385a..54d8128561f39 100644 --- a/tree/treeplayer/test/regressions.cxx +++ b/tree/treeplayer/test/regressions.cxx @@ -359,6 +359,39 @@ TEST(TTreeReaderRegressions, ValueFastTuple) EXPECT_NEAR(total, 866026269930.1345215, 100); } +// ROOT-8000 https://its.cern.ch/jira/browse/ROOT-8000 +// An unrelated file lying around in the current directory whose name matches +// the leading part of a formula (e.g. a file named "abs") must not make +// TTree::Draw interpret the formula as the name of a C++ script called with +// arguments: only files with an extension qualify as scripts. +TEST(TTreeDrawRegressions, ExpressionNotShadowedByFile) +{ + ROOT::TestSupport::FileRaii shadowingFile{"abs"}; + { + std::ofstream f(shadowingFile.GetPath()); + f << "This is a text file, not a C++ script.\n"; + } + + TTree t("t", "t"); + double x; + int id; + t.Branch("x", &x); + t.Branch("id", &id); + for (int i = 0; i < 100; ++i) { + x = i - 50; + id = (i % 2) ? 531 : -531; + t.Fill(); + } + + // These formulas end with a parenthesized part, which used to be stripped + // as ACLiC-style arguments, leaving "abs" to be found as a file. + EXPECT_EQ(t.Draw("abs(x)", "", "goff"), 100); + EXPECT_EQ(t.Draw("x", "abs(id)", "goff"), 100); + // The dot in "0.5" used to make the varexp pass the extension check. + EXPECT_EQ(t.Draw("abs(x + 0.5)", "abs(id) == 531", "goff"), 100); + EXPECT_EQ(t.Draw("x", "abs(id) == id", "goff"), 50); +} + // https://github.com/root-project/root/issues/20226 TEST(TTreeScan, IntOverflow) {