diff --git a/tree/treeplayer/src/TTreePlayer.cxx b/tree/treeplayer/src/TTreePlayer.cxx index 6decc11d535fb..1131639d8ac10 100644 --- a/tree/treeplayer/src/TTreePlayer.cxx +++ b/tree/treeplayer/src/TTreePlayer.cxx @@ -31,6 +31,7 @@ extra libraries (Histogram, display, etc). #include #include #include +#include #include "TROOT.h" #include "TApplication.h" @@ -274,6 +275,48 @@ 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, we require that the candidate 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; + + // These tokens only appear in TTreeFormula expressions, never in the name of + // a script file, so their presence rules out a script file. + static const std::vector formulaTokens = { + "Alt$", "Entries$", "LocalEntries$", "Length$", "Entry$", "LocalEntry$", "Min$", + "Max$", "MinIf$", "MaxIf$", "Iteration$", "Sum$", ">", "<"}; + for (const TString &token : formulaTokens) + if (candidate.Index(token) >= 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,44 +400,24 @@ 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", + "Drawing using a C++ macro currently requires that both the expression and the selection are " + "files\n\t\"%s\" is not a file", selection); return 0; } 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++ macro 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..3f53fa62b304b 100644 --- a/tree/treeplayer/test/regressions.cxx +++ b/tree/treeplayer/test/regressions.cxx @@ -359,6 +359,71 @@ 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); +} + +// ROOT-8000 https://its.cern.ch/jira/browse/ROOT-8000 +// The flip side of the test above: a *real* macro whose name matches the +// leading part of a formula must still be recognized as a script, precisely +// because it has an extension. We check this without paying for the ACLiC +// compilation of the macro: when the variable expression is a script file but +// the selection is a plain formula (not a file), DrawSelect must reject the +// combination with its "both must be files" error. That error is only reached +// if "abs.C" was taken as a script - a bare "abs" would be treated as a formula +// and the error above ("... is not a file" for the selection) would never fire. +TEST(TTreeDrawRegressions, MacroWithExtensionRecognizedAsScript) +{ + ROOT::TestSupport::FileRaii macroFile{"abs.C"}; + { + std::ofstream f(macroFile.GetPath()); + f << "double abs()\n{\n return x;\n}\n"; + } + + TTree t("t", "t"); + double x; + t.Branch("x", &x); + for (int i = 0; i < 100; ++i) { + x = i - 50; + t.Fill(); + } + + ROOT::TestSupport::CheckDiagsRAII diags{kError, "TTreePlayer::DrawSelect", + "Drawing using a C++ macro currently requires that both the expression and " + "the selection are files", + /*matchFullMessage=*/false}; + EXPECT_EQ(t.Draw("abs.C", "x > 0", "goff"), 0); +} + // https://github.com/root-project/root/issues/20226 TEST(TTreeScan, IntOverflow) {