From 05792433a9d9ee9e2d35e88029319252bae8c4de Mon Sep 17 00:00:00 2001 From: ferdymercury Date: Mon, 2 Feb 2026 16:16:54 +0100 Subject: [PATCH 01/12] [hist] prevent TFormula out of bounds access Fixes https://github.com/root-project/root/issues/21104 --- hist/hist/src/TFormula.cxx | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/hist/hist/src/TFormula.cxx b/hist/hist/src/TFormula.cxx index 3a1df9b26d918..b6e2c20af1356 100644 --- a/hist/hist/src/TFormula.cxx +++ b/hist/hist/src/TFormula.cxx @@ -1428,15 +1428,15 @@ void TFormula::HandleFunctionArguments(TString &formula) // ignore things that start with square brackets if (formula[i] == '[') { - while (formula[i] != ']') + while (i < formula.Length() && formula[i] != ']') i++; continue; } // ignore strings - if (formula[i] == '\"') { + if (i < formula.Length() && formula[i] == '\"') { do i++; - while (formula[i] != '\"'); + while (i < formula.Length() && formula[i] != '\"'); continue; } // ignore numbers (scientific notation) @@ -1444,13 +1444,13 @@ void TFormula::HandleFunctionArguments(TString &formula) continue; // ignore x in hexadecimal number if (IsHexadecimal(formula, i)) { - while (!IsOperator(formula[i]) && i < formula.Length()) + while (i < formula.Length() && !IsOperator(formula[i])) i++; continue; } // investigate possible start of function name - if (isalpha(formula[i]) && !IsOperator(formula[i])) { + if (i < formula.Length() && isalpha(formula[i]) && !IsOperator(formula[i])) { // std::cout << "character : " << i << " " << formula[i] << " is not an operator and is alpha" << std::endl; int j; // index to end of name From f004d15b8e7eea80c4e6c9b130404b344d2e7577 Mon Sep 17 00:00:00 2001 From: ferdymercury Date: Mon, 2 Feb 2026 16:29:49 +0100 Subject: [PATCH 02/12] [test] verify that TFormula open bracket does not go into infinite loop --- hist/hist/test/test_TFormula.cxx | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/hist/hist/test/test_TFormula.cxx b/hist/hist/test/test_TFormula.cxx index 8d0650905d766..a390580314f93 100644 --- a/hist/hist/test/test_TFormula.cxx +++ b/hist/hist/test/test_TFormula.cxx @@ -98,3 +98,10 @@ TEST(TFormulaPolTest, CompoundExpressions) TFormula f1("f1", "pol1(x,0) + pol1(y,2)"); EXPECT_EQ(f1.GetExpFormula(), TString("([p0]+[p1]*x)+([p2]+[p3]*y)")); } + +// https://github.com/root-project/root/issues/21104 +TEST(TFormula, SingleOpeningBracket) +{ + TFormula f1("f1", "["); + EXPECT_EQ(f1.GetNdim(), 0); +} From 2146db78c92384891454a1aad8900fe7a9e230c3 Mon Sep 17 00:00:00 2001 From: ferdymercury Date: Mon, 2 Feb 2026 16:44:09 +0100 Subject: [PATCH 03/12] [hist] fix access when opening bracket pos exactly at length --- hist/hist/src/TFormula.cxx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hist/hist/src/TFormula.cxx b/hist/hist/src/TFormula.cxx index b6e2c20af1356..8af347e40b4fc 100644 --- a/hist/hist/src/TFormula.cxx +++ b/hist/hist/src/TFormula.cxx @@ -1250,7 +1250,7 @@ void TFormula::HandleParametrizedFunctions(TString &formula) // check if function has specified the [...] e.g. gaus[x,y] Int_t openingBracketPos = funPos + funName.Length() + (isNormalized ? 1 : 0); Int_t closingBracketPos = kNPOS; - if (openingBracketPos > formula.Length() || formula[openingBracketPos] != '[') { + if (openingBracketPos >= formula.Length() || formula[openingBracketPos] != '[') { dim = funDim; variables.resize(dim); for (Int_t idim = 0; idim < dim; ++idim) From ed3d8e01fc65f13c3777b7d3c27dc63037ecf2cf Mon Sep 17 00:00:00 2001 From: ferdymercury Date: Tue, 3 Feb 2026 09:44:02 +0100 Subject: [PATCH 04/12] [hist] additional out of bounds access safety guards --- hist/hist/src/TFormula.cxx | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/hist/hist/src/TFormula.cxx b/hist/hist/src/TFormula.cxx index 8af347e40b4fc..b27ca65b0b9dd 100644 --- a/hist/hist/src/TFormula.cxx +++ b/hist/hist/src/TFormula.cxx @@ -1956,7 +1956,7 @@ void TFormula::ExtractFunctors(TString &formula) // look for next instance of "\" do { i++; - } while (formula[i] != '\"'); + } while (i < formula.Length() && formula[i] != '\"'); } // case of e or E for numbers in exponential notation (e.g. 2.2e-3) if (IsScientificNotation(formula, i)) @@ -1965,7 +1965,7 @@ void TFormula::ExtractFunctors(TString &formula) if (IsHexadecimal(formula, i)) { // find position of operator // do not check cases if character is not only a to f, but accept anything - while (!IsOperator(formula[i]) && i < formula.Length()) { + while (i < formula.Length() && !IsOperator(formula[i])) { i++; } continue; @@ -1997,7 +1997,7 @@ void TFormula::ExtractFunctors(TString &formula) // printf(" build a name %s \n",name.Data() ); if (formula[i] == '(') { i++; - if (formula[i] == ')') { + if (i < formula.Length() && formula[i] == ')') { fFuncs.push_back(TFormulaFunction(name, body, 0)); name = body = ""; continue; @@ -3622,8 +3622,8 @@ TString TFormula::GetExpFormula(Option_t *option) const // look for p[number if (clingFormula[i] == 'p' && clingFormula[i+1] == '[' && isdigit(clingFormula[i+2]) ) { int j = i+3; - while ( isdigit(clingFormula[j]) ) { j++;} - if (clingFormula[j] != ']') { + while ( j < clingFormula.Length() && isdigit(clingFormula[j]) ) { j++;} + if ( j >= clingFormula.Length() || clingFormula[j] != ']') { Error("GetExpFormula","Parameters not found - invalid expression - return default cling formula"); return clingFormula; } @@ -3647,8 +3647,8 @@ TString TFormula::GetExpFormula(Option_t *option) const // look for [parName] if (expFormula[i] == '[') { int j = i+1; - while ( expFormula[j] != ']' ) { j++;} - if (expFormula[j] != ']') { + while ( j < expFormula.Length() && expFormula[j] != ']' ) { j++;} + if ( j >= expFormula.Length() || expFormula[j] != ']') { Error("GetExpFormula","Parameter names not found - invalid expression - return default formula"); return expFormula; } From 2c4a4a0ca996d336f46ebda749bc6d8ede6bf7b7 Mon Sep 17 00:00:00 2001 From: ferdymercury Date: Tue, 3 Feb 2026 15:04:49 +0100 Subject: [PATCH 05/12] [test] test also opening parenthesis --- hist/hist/test/test_TFormula.cxx | 2 ++ 1 file changed, 2 insertions(+) diff --git a/hist/hist/test/test_TFormula.cxx b/hist/hist/test/test_TFormula.cxx index a390580314f93..bbf6027c145d3 100644 --- a/hist/hist/test/test_TFormula.cxx +++ b/hist/hist/test/test_TFormula.cxx @@ -104,4 +104,6 @@ TEST(TFormula, SingleOpeningBracket) { TFormula f1("f1", "["); EXPECT_EQ(f1.GetNdim(), 0); + TFormula f2("f2", "("); + EXPECT_EQ(f2.GetNdim(), 0); } From 6983ed3e03ba9f98bf63cbdcbdb5a595bd8b0910 Mon Sep 17 00:00:00 2001 From: ferdymercury Date: Wed, 4 Feb 2026 17:37:57 +0100 Subject: [PATCH 06/12] [hist] check for balanced parentheses, brackets, braces or quotes set IsValid to false otherwise while constructing TFormula --- hist/hist/src/TFormula.cxx | 45 +++++++++++++++++++++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-) diff --git a/hist/hist/src/TFormula.cxx b/hist/hist/src/TFormula.cxx index b27ca65b0b9dd..9b1a5c60309cd 100644 --- a/hist/hist/src/TFormula.cxx +++ b/hist/hist/src/TFormula.cxx @@ -22,13 +22,13 @@ #include "ROOT/StringUtils.hxx" -#include #include #include #include #include #include #include +#include #include #include @@ -49,6 +49,43 @@ std::string doubleToString(double val) return ss.str(); } +bool areMatching(char opening, char closing) +{ + return (opening == '(' && closing == ')') || (opening == '{' && closing == '}') || + (opening == '[' && closing == ']') || (opening == '\"' && closing == '\"'); +} + +bool isBalanced(const TString &s) +{ + std::stack i; + auto sLength = s.Length(); + for (Ssiz_t c = 0; c < sLength; ++c) { + if (s[c] == '[' || s[c] == '{' || s[c] == '(') { + i.push(s[c]); + } else if (s[c] == ']' || s[c] == '}' || s[c] == ')') { + if (i.empty() || !areMatching(i.top(), s[c])) { + Error("TFormula", "Found unbalanced char %c (expected closing of %c) at index %d of %s", s[c], i.top(), c, + s.Data()); + return false; + } else + i.pop(); + } else if (s[c] == '\"') { + if (i.empty()) + i.push(s[c]); + else if (areMatching(i.top(), s[c])) + i.pop(); + else { + i.push(s[c]); + } + } + } + if (!i.empty()) { + Error("TFormula", "String %s with %zu unbalanced chars.", s.Data(), i.size()); + return false; + } + return true; +} + // In the interpreter, we must match the SIMD width used by compiled ROOT. // ROOT::Double_v aliases the best available native SIMD type, which may differ // between compiled and interpreted contexts (e.g. with -march=native). @@ -1860,6 +1897,12 @@ Bool_t TFormula::PrepareFormula(TString &formula) { fFuncs.clear(); fReadyToExecute = false; + + // Check for balanced parentheses + if (!isBalanced(formula)) { + return false; + } + ExtractFunctors(formula); // update the expression with the new formula From a587c6db6fa16b5bb6f625314d400be8580ac6f7 Mon Sep 17 00:00:00 2001 From: ferdymercury Date: Wed, 4 Feb 2026 17:42:42 +0100 Subject: [PATCH 07/12] [test] TFormula unbalanced: check error detection and IsValid value --- hist/hist/test/test_TFormula.cxx | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/hist/hist/test/test_TFormula.cxx b/hist/hist/test/test_TFormula.cxx index bbf6027c145d3..a1f926d2c5945 100644 --- a/hist/hist/test/test_TFormula.cxx +++ b/hist/hist/test/test_TFormula.cxx @@ -102,8 +102,13 @@ TEST(TFormulaPolTest, CompoundExpressions) // https://github.com/root-project/root/issues/21104 TEST(TFormula, SingleOpeningBracket) { + ROOT::TestSupport::CheckDiagsRAII diags; + diags.requiredDiag(kError, "TFormula", "String [ with 1 unbalanced chars."); TFormula f1("f1", "["); + EXPECT_FALSE(f1.IsValid()); EXPECT_EQ(f1.GetNdim(), 0); + diags.requiredDiag(kError, "TFormula", "String ( with 1 unbalanced chars."); TFormula f2("f2", "("); + EXPECT_FALSE(f2.IsValid()); EXPECT_EQ(f2.GetNdim(), 0); } From d572c47288b435f4019efe5623e1018020cd59b8 Mon Sep 17 00:00:00 2001 From: ferdymercury Date: Thu, 5 Feb 2026 09:58:10 +0100 Subject: [PATCH 08/12] [nfc] improve TFormula docu --- hist/hist/src/TFormula.cxx | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/hist/hist/src/TFormula.cxx b/hist/hist/src/TFormula.cxx index 9b1a5c60309cd..eed90579b706a 100644 --- a/hist/hist/src/TFormula.cxx +++ b/hist/hist/src/TFormula.cxx @@ -411,9 +411,9 @@ Bool_t TFormula::IsHexadecimal(const TString & formula, int i) // } return false; } + //////////////////////////////////////////////////////////////////////////// -// check is given position is in a parameter name i.e. within "[ ]" -//// +/// check if given position is in a parameter name i.e. within "[ ]" Bool_t TFormula::IsAParameterName(const TString & formula, int pos) { Bool_t foundOpenParenthesis = false; @@ -1833,7 +1833,7 @@ void TFormula::HandleExponentiation(TString &formula) //////////////////////////////////////////////////////////////////////////////// -/// Handle linear functions defined with the operator ++. +/// Handle linear functions defined with the operator ++ (@ is the shorthand). void TFormula::HandleLinear(TString &formula) { @@ -1866,7 +1866,7 @@ void TFormula::HandleLinear(TString &formula) //////////////////////////////////////////////////////////////////////////////// /// Preprocessing of formula -/// Replace all ** by ^, and removes spaces. +/// Replace all ** by ^, all ++ by @, and removes spaces. /// Handle also parametrized functions like polN,gaus,expo,landau /// and exponentiation. /// Similar functionality should be added here. @@ -1881,7 +1881,7 @@ void TFormula::PreProcessFormula(TString &formula) HandleParamRanges(formula); HandleFunctionArguments(formula); HandleExponentiation(formula); - // "++" wil be dealt with Handle Linear + // "++" (now @) wil be dealt with Handle Linear HandleLinear(formula); // special case for "--" and "++" // ("++" needs to be written with whitespace that is removed before but then we re-add it again From d370a81092fe198c0c385eb657e6a42d2ea1108f Mon Sep 17 00:00:00 2001 From: ferdymercury Date: Fri, 6 Feb 2026 08:38:48 +0100 Subject: [PATCH 09/12] [test] singleQuote printed string Co-authored-by: Vincenzo Eduardo Padulano --- hist/hist/src/TFormula.cxx | 4 ++-- hist/hist/test/test_TFormula.cxx | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/hist/hist/src/TFormula.cxx b/hist/hist/src/TFormula.cxx index eed90579b706a..3ea4bfebebe21 100644 --- a/hist/hist/src/TFormula.cxx +++ b/hist/hist/src/TFormula.cxx @@ -64,7 +64,7 @@ bool isBalanced(const TString &s) i.push(s[c]); } else if (s[c] == ']' || s[c] == '}' || s[c] == ')') { if (i.empty() || !areMatching(i.top(), s[c])) { - Error("TFormula", "Found unbalanced char %c (expected closing of %c) at index %d of %s", s[c], i.top(), c, + Error("TFormula", "Found unbalanced char '%c' (expected closing of '%c') at index %d of '%s'", s[c], i.top(), c, s.Data()); return false; } else @@ -80,7 +80,7 @@ bool isBalanced(const TString &s) } } if (!i.empty()) { - Error("TFormula", "String %s with %zu unbalanced chars.", s.Data(), i.size()); + Error("TFormula", "String '%s' with %zu unbalanced chars.", s.Data(), i.size()); return false; } return true; diff --git a/hist/hist/test/test_TFormula.cxx b/hist/hist/test/test_TFormula.cxx index a1f926d2c5945..288af5f8cb936 100644 --- a/hist/hist/test/test_TFormula.cxx +++ b/hist/hist/test/test_TFormula.cxx @@ -103,11 +103,11 @@ TEST(TFormulaPolTest, CompoundExpressions) TEST(TFormula, SingleOpeningBracket) { ROOT::TestSupport::CheckDiagsRAII diags; - diags.requiredDiag(kError, "TFormula", "String [ with 1 unbalanced chars."); + diags.requiredDiag(kError, "TFormula", "String '[' with 1 unbalanced chars."); TFormula f1("f1", "["); EXPECT_FALSE(f1.IsValid()); EXPECT_EQ(f1.GetNdim(), 0); - diags.requiredDiag(kError, "TFormula", "String ( with 1 unbalanced chars."); + diags.requiredDiag(kError, "TFormula", "String '(' with 1 unbalanced chars."); TFormula f2("f2", "("); EXPECT_FALSE(f2.IsValid()); EXPECT_EQ(f2.GetNdim(), 0); From f4e615e5bbd1a785c5d778218f69ffdd2cf1966e Mon Sep 17 00:00:00 2001 From: ferdymercury Date: Fri, 6 Feb 2026 08:42:59 +0100 Subject: [PATCH 10/12] [nfc][hist] clarify docu --- hist/hist/src/TFormula.cxx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hist/hist/src/TFormula.cxx b/hist/hist/src/TFormula.cxx index 3ea4bfebebe21..c7fc26019e0e0 100644 --- a/hist/hist/src/TFormula.cxx +++ b/hist/hist/src/TFormula.cxx @@ -1866,7 +1866,7 @@ void TFormula::HandleLinear(TString &formula) //////////////////////////////////////////////////////////////////////////////// /// Preprocessing of formula -/// Replace all ** by ^, all ++ by @, and removes spaces. +/// Replace all ** by ^, all ++ by @, and removes spaces. /// Handle also parametrized functions like polN,gaus,expo,landau /// and exponentiation. /// Similar functionality should be added here. @@ -1881,7 +1881,7 @@ void TFormula::PreProcessFormula(TString &formula) HandleParamRanges(formula); HandleFunctionArguments(formula); HandleExponentiation(formula); - // "++" (now @) wil be dealt with Handle Linear + // "++" (now @ since they we just called ReplaceAll a few lines above) wil be dealt with Handle Linear HandleLinear(formula); // special case for "--" and "++" // ("++" needs to be written with whitespace that is removed before but then we re-add it again From 10c702a3fd33c128e1b1b2f663bd420ee318e0fb Mon Sep 17 00:00:00 2001 From: ferdymercury Date: Mon, 9 Feb 2026 10:59:25 +0100 Subject: [PATCH 11/12] [hist] fix restitution of parameter name containing @ Amends https://github.com/root-project/root/commit/dcf920904564bbdca08a6acfa70019bbc24b3897#r176468371 see also https://github.com/ferdymercury/root/commit/1f1cb91033243b6182fe34cff2fc9d449ba52071 Should make TFormulaTest39 pass again --- hist/hist/src/TFormula.cxx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hist/hist/src/TFormula.cxx b/hist/hist/src/TFormula.cxx index c7fc26019e0e0..cfe0f79b92c26 100644 --- a/hist/hist/src/TFormula.cxx +++ b/hist/hist/src/TFormula.cxx @@ -1846,11 +1846,11 @@ void TFormula::HandleLinear(TString &formula) int delimeterPos = 0; for(std::size_t iTerm = 0; iTerm < terms.size(); ++iTerm) { // determine the position of the "@" operator in the formula - delimeterPos += terms[iTerm].size() + (iTerm == 0); + delimeterPos += terms[iTerm].size() + iTerm; if(IsAParameterName(formula, delimeterPos)) { // append the current term and the remaining formula unchanged to the expanded formula expandedFormula += terms[iTerm]; - expandedFormula += formula(delimeterPos, formula.Length() - (delimeterPos + 1)); + expandedFormula += formula(delimeterPos, formula.Length() - delimeterPos); break; } SetBit(kLinear, true); From d07b47b79cf846ab32da4759dfcdb8ba88c208ff Mon Sep 17 00:00:00 2001 From: ferdymercury Date: Mon, 9 Feb 2026 11:41:46 +0100 Subject: [PATCH 12/12] [hist] fix out of bounds access at N-th term --- hist/hist/src/TFormula.cxx | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/hist/hist/src/TFormula.cxx b/hist/hist/src/TFormula.cxx index cfe0f79b92c26..b248fc1bc9878 100644 --- a/hist/hist/src/TFormula.cxx +++ b/hist/hist/src/TFormula.cxx @@ -1845,13 +1845,15 @@ void TFormula::HandleLinear(TString &formula) TString expandedFormula = ""; int delimeterPos = 0; for(std::size_t iTerm = 0; iTerm < terms.size(); ++iTerm) { - // determine the position of the "@" operator in the formula - delimeterPos += terms[iTerm].size() + iTerm; - if(IsAParameterName(formula, delimeterPos)) { - // append the current term and the remaining formula unchanged to the expanded formula - expandedFormula += terms[iTerm]; - expandedFormula += formula(delimeterPos, formula.Length() - delimeterPos); - break; + if (iTerm < terms.size() - 1) { // N terms, N - 1 @ + // determine the position of the "@" operator in the formula + delimeterPos += terms[iTerm].size() + iTerm; + if(IsAParameterName(formula, delimeterPos)) { + // append the current term and the remaining formula unchanged to the expanded formula + expandedFormula += terms[iTerm]; + expandedFormula += formula(delimeterPos, formula.Length() - delimeterPos); + break; + } } SetBit(kLinear, true); auto termName = std::string("__linear") + std::to_string(iTerm+1);