From e886b5a76cd073b40e14cf534d2ea652009417f6 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Thu, 29 Oct 2026 12:49:02 +0000 Subject: [PATCH] [RF] Don't touch deleted nodes in RooFit::Evaluator destructor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Evaluator resets the data tokens of all nodes of its computation graph in the destructor, through raw pointers and without knowing whether the nodes still exist. Nodes that outlive the Evaluator need this reset, because a node can only be registered with one Evaluator at a time. But nodes can also be deleted before the Evaluator: the compiled NLL graph of a RooMomentMorphFuncND references the original morph function and its RooHistPdf templates owned by the RooWorkspace, and at the exit of a Python script the garbage collector can delete the workspace before the NLL object. The destructor then writes into freed memory, which showed up as a `free(): invalid pointer` abort of tutorial-roofit-roofit-rf616_morphing-py in the v6-42-00-rc1 release build on ubuntu2610 (valgrind: "Invalid write of size 8" in RooFit::Evaluator::~Evaluator()). Track the ownership of the data token: RooAbsArg remembers the Evaluator that set its token, tells it in the destructor that the node is gone (Evaluator::unregisterNode), and the Evaluator skips such nodes when resetting the tokens. Add a unit test for the Evaluator that deletes a node before the Evaluator and checks that tokens can be reused in both orders, and a regression test for the workspace-deleted-before-NLL scenario with the morph pdf. This also means we can remove the workarounds in the tutorials introduced by commits 49fb353caf7 and 06879112f32. 🤖 Done with the help of AI --- .../roofit/test/testRooMomentMorphFuncND.cxx | 68 ++++++++++++++----- roofit/roofitcore/inc/RooAbsArg.h | 10 ++- roofit/roofitcore/inc/RooFit/Evaluator.h | 2 + roofit/roofitcore/src/RooAbsArg.cxx | 34 +++++++--- roofit/roofitcore/src/RooFit/Evaluator.cxx | 17 ++++- roofit/roofitcore/test/CMakeLists.txt | 1 + roofit/roofitcore/test/testRooEvaluator.cxx | 43 ++++++++++++ 7 files changed, 143 insertions(+), 32 deletions(-) create mode 100644 roofit/roofitcore/test/testRooEvaluator.cxx diff --git a/roofit/roofit/test/testRooMomentMorphFuncND.cxx b/roofit/roofit/test/testRooMomentMorphFuncND.cxx index 8033df2170dd9..963e1e84e943f 100644 --- a/roofit/roofit/test/testRooMomentMorphFuncND.cxx +++ b/roofit/roofit/test/testRooMomentMorphFuncND.cxx @@ -3,6 +3,7 @@ #include #include +#include #include #include #include @@ -15,27 +16,17 @@ #include +#include #include namespace { -// For each morph configuration we expect: a normal `getVal(normSet)` on the -// uncompiled pdf and an Evaluator run on the same pdf compiled for that -// normSet must agree. Whether the morph is in pdf mode (sum is a RooAddPdf -// whose components must be normalized for the value to be a proper density) -// or in func mode (sum is a RooRealSumFunc, the value is the raw weighted -// sum of bin contents) is internal to compileForNormSet -- both paths must -// produce the same final value. -void checkBackendConsistency(bool pdfMode) +/// Workspace with the observable "x", the morphing parameter "mu", and the +/// morph pdf "morph" with two RooHistPdf templates. +std::unique_ptr makeMorphWorkspace(bool pdfMode) { - // FATAL silences a transient logEvalError that the morph emits from its - // slope/offset RooFormulaVars at cache-construction time (in func mode the - // initial rms-fractions are zero, so slope = inf for one self-evaluation - // before calculateFractions runs). It does not affect correctness. - // TODO: its not elegant that this happens and should be fixed. - RooHelpers::LocalChangeMsgLevel changeMsgLevel{RooFit::FATAL}; - - RooWorkspace ws; + auto wsPtr = std::make_unique(); + RooWorkspace &ws = *wsPtr; ws.factory("x[-5,15]"); ws.factory("mu[0.5,0.0,1.0]"); RooRealVar &x = *ws.var("x"); @@ -63,7 +54,30 @@ void checkBackendConsistency(bool pdfMode) RooWrapperPdf morph("morph", "morph", morph_func, /*selfNormalized=*/true); ws.import(morph, RooFit::Silence(true)); - RooAbsPdf &morphPdf = *ws.pdf("morph"); + return wsPtr; +} + +// For each morph configuration we expect: a normal `getVal(normSet)` on the +// uncompiled pdf and an Evaluator run on the same pdf compiled for that +// normSet must agree. Whether the morph is in pdf mode (sum is a RooAddPdf +// whose components must be normalized for the value to be a proper density) +// or in func mode (sum is a RooRealSumFunc, the value is the raw weighted +// sum of bin contents) is internal to compileForNormSet -- both paths must +// produce the same final value. +void checkBackendConsistency(bool pdfMode) +{ + // FATAL silences a transient logEvalError that the morph emits from its + // slope/offset RooFormulaVars at cache-construction time (in func mode the + // initial rms-fractions are zero, so slope = inf for one self-evaluation + // before calculateFractions runs). It does not affect correctness. + // TODO: its not elegant that this happens and should be fixed. + RooHelpers::LocalChangeMsgLevel changeMsgLevel{RooFit::FATAL}; + + std::unique_ptr ws = makeMorphWorkspace(pdfMode); + RooRealVar &x = *ws->var("x"); + RooRealVar &mu = *ws->var("mu"); + + RooAbsPdf &morphPdf = *ws->pdf("morph"); RooArgSet normSet(x); mu.setVal(0.5); @@ -89,3 +103,23 @@ TEST(RooMomentMorphFuncND, BackendConsistencyFuncMode) { checkBackendConsistency(/*pdfMode=*/false); } + +/// The compiled computation graph of the morph pdf references the original +/// morph function and the RooHistPdf templates owned by the workspace. If the +/// workspace is deleted before an NLL object, the destructor of the NLL must +/// not touch the deleted nodes anymore. This happened at the exit of the +/// rf616_morphing.py tutorial, where the Python garbage collector deletes the +/// workspace first, and was observed as a `free(): invalid pointer` abort. +TEST(RooMomentMorphFuncND, WorkspaceDeletedBeforeNLL) +{ + RooHelpers::LocalChangeMsgLevel changeMsgLevel{RooFit::FATAL}; + + std::unique_ptr ws = makeMorphWorkspace(/*pdfMode=*/true); + RooAbsPdf &morphPdf = *ws->pdf("morph"); + std::unique_ptr data{ws->pdf("hp0")->generate(*ws->var("x"), 100)}; + std::unique_ptr nll{morphPdf.createNLL(*data)}; + EXPECT_TRUE(std::isfinite(nll->getVal())); + + ws.reset(); + nll.reset(); +} diff --git a/roofit/roofitcore/inc/RooAbsArg.h b/roofit/roofitcore/inc/RooAbsArg.h index ab6a659165f71..52ed6596af55c 100644 --- a/roofit/roofitcore/inc/RooAbsArg.h +++ b/roofit/roofitcore/inc/RooAbsArg.h @@ -52,6 +52,7 @@ using RooListProxy = RooCollectionProxy; class RooExpensiveObjectCache; class RooWorkspace; namespace RooFit { +class Evaluator; namespace Experimental { class CodegenContext; } @@ -634,8 +635,12 @@ class RooAbsArg : public TNamed, public RooPrintable { /// Returns the token for retrieving results in the BatchMode. For internal use only. std::size_t dataToken() const { return _dataToken; } bool hasDataToken() const { return _dataToken != std::numeric_limits::max(); } - void setDataToken(std::size_t index); - void resetDataToken() { _dataToken = std::numeric_limits::max(); } + void setDataToken(std::size_t index, RooFit::Evaluator *evaluator = nullptr); + void resetDataToken() + { + _dataToken = std::numeric_limits::max(); + _evaluator = nullptr; + } protected: mutable bool _valueDirty = true; // Flag set if value needs recalculating because input values modified @@ -660,6 +665,7 @@ class RooAbsArg : public TNamed, public RooPrintable { std::size_t _dataToken = std::numeric_limits::max(); /// setOperModes(RooAbsArg::OperMode opMode); + void unregisterNode(RooAbsArg const &arg); + private: void processVariable(NodeInfo &nodeInfo); void processCategory(NodeInfo &nodeInfo); diff --git a/roofit/roofitcore/src/RooAbsArg.cxx b/roofit/roofitcore/src/RooAbsArg.cxx index ab4297ad9cad2..0d7ecbb6dc0c8 100644 --- a/roofit/roofitcore/src/RooAbsArg.cxx +++ b/roofit/roofitcore/src/RooAbsArg.cxx @@ -74,6 +74,7 @@ for single nodes. #include #include #include +#include #include #include "RooFitImplHelpers.h" #include @@ -169,6 +170,12 @@ RooAbsArg::RooAbsArg(const RooAbsArg &other, const char *name) RooAbsArg::~RooAbsArg() { + // If a RooFit::Evaluator still holds a data token for this arg, tell it that + // the arg is gone so that it doesn't reset the token in its destructor. + if (_evaluator) { + _evaluator->unregisterNode(*this); + } + // Notify all servers that they no longer need to serve us while (!_serverList.empty()) { removeServer(*_serverList.containedObjects().back(), true); @@ -2261,17 +2268,22 @@ RooAbsArg::compileForNormSet(RooArgSet const &normSet, RooFit::Detail::CompileCo } /// Sets the token for retrieving results in the BatchMode. For internal use only. -void RooAbsArg::setDataToken(std::size_t index) -{ - if (_dataToken == index) { - return; +/// If an `evaluator` is passed, it is the RooFit::Evaluator that resets the +/// token in its destructor, and this arg will notify it if it gets deleted +/// before the Evaluator. +void RooAbsArg::setDataToken(std::size_t index, RooFit::Evaluator *evaluator) +{ + if (_dataToken != index) { + if (hasDataToken()) { + std::stringstream errMsg; + errMsg << "The data token for \"" << GetName() << "\" is already set!" + << " Are you trying to evaluate the same object by multiple RooFit::Evaluator instances?" + << " This is not allowed."; + throw std::runtime_error(errMsg.str()); + } + _dataToken = index; } - if (_dataToken != std::numeric_limits::max()) { - std::stringstream errMsg; - errMsg << "The data token for \"" << GetName() << "\" is already set!" - << " Are you trying to evaluate the same object by multiple RooFit::Evaluator instances?" - << " This is not allowed."; - throw std::runtime_error(errMsg.str()); + if (evaluator) { + _evaluator = evaluator; } - _dataToken = index; } diff --git a/roofit/roofitcore/src/RooFit/Evaluator.cxx b/roofit/roofitcore/src/RooFit/Evaluator.cxx index 06ced37a724a4..d8cefc7db1a00 100644 --- a/roofit/roofitcore/src/RooFit/Evaluator.cxx +++ b/roofit/roofitcore/src/RooFit/Evaluator.cxx @@ -189,7 +189,7 @@ Evaluator::Evaluator(const RooAbsReal &absReal, bool useGPU) if (dynamic_cast(arg)) { nodeInfo.isVariable = true; } else { - arg->setDataToken(iNode); + arg->setDataToken(iNode, this); } if (dynamic_cast(arg)) { nodeInfo.isCategory = true; @@ -338,12 +338,25 @@ void Evaluator::updateOutputSizes() Evaluator::~Evaluator() { for (auto &info : _nodes) { - if (!info.isVariable) { + // Nodes that were deleted before the Evaluator were unregistered + if (!info.isVariable && info.absArg) { info.absArg->resetDataToken(); } } } +/// Called from RooAbsArg::~RooAbsArg() when a node of the computation graph is +/// deleted while this Evaluator is still alive. The Evaluator can't be used +/// anymore after that, but its destructor must not touch the deleted node. +/// For internal use only. +void Evaluator::unregisterNode(RooAbsArg const &arg) +{ + auto found = _nodesMap.find(arg.namePtr()); + if (found != _nodesMap.end() && found->second->absArg == &arg) { + found->second->absArg = nullptr; + } +} + void Evaluator::computeCPUNode(const RooAbsArg *node, NodeInfo &info) { using namespace Detail; diff --git a/roofit/roofitcore/test/CMakeLists.txt b/roofit/roofitcore/test/CMakeLists.txt index def1fa324c837..c81bfb6fe11a5 100644 --- a/roofit/roofitcore/test/CMakeLists.txt +++ b/roofit/roofitcore/test/CMakeLists.txt @@ -81,6 +81,7 @@ if(clad) ROOT_ADD_GTEST(testRooFuncWrapper testRooFuncWrapper.cxx LIBRARIES RooFitCore RooFit HistFactory) endif() ROOT_ADD_GTEST(testNaNPacker testNaNPacker.cxx LIBRARIES RooFitCore) +ROOT_ADD_GTEST(testRooEvaluator testRooEvaluator.cxx LIBRARIES RooFitCore RooFit) ROOT_ADD_GTEST(testRooExtendedBinding testRooExtendedBinding.cxx LIBRARIES RooFitCore RooFit) ROOT_ADD_GTEST(testRooMinimizer testRooMinimizer.cxx LIBRARIES RooFitCore RooFit) ROOT_ADD_GTEST(testRooMulti testRooMulti.cxx LIBRARIES RooFitCore RooFit) diff --git a/roofit/roofitcore/test/testRooEvaluator.cxx b/roofit/roofitcore/test/testRooEvaluator.cxx new file mode 100644 index 0000000000000..84fa3ff904209 --- /dev/null +++ b/roofit/roofitcore/test/testRooEvaluator.cxx @@ -0,0 +1,43 @@ +// Tests for RooFit::Evaluator +// Author: Jonas Rembser, CERN 10/2026 + +#include +#include +#include + +#include + +#include + +/// Deleting nodes of the computation graph before the Evaluator must not +/// crash: the Evaluator used to reset the data tokens of its nodes in the +/// destructor without checking whether the nodes still exist (use after free +/// that was observed as a `free(): invalid pointer` abort at the exit of +/// Python scripts, where the garbage collector can delete the RooWorkspace +/// owning the model before the NLL object). +TEST(RooEvaluator, NodesDeletedBeforeEvaluator) +{ + auto x = std::make_unique("x", "x", 0.0, -5.0, 5.0); + auto mu = std::make_unique("mu", "mu", 0.0, -1.0, 1.0); + auto sigma = std::make_unique("sigma", "sigma", 1.0, 0.1, 10.0); + auto gauss = std::make_unique("gauss", "gauss", *x, *mu, *sigma); + + auto evaluator = std::make_unique(*gauss); + EXPECT_FLOAT_EQ(evaluator->run()[0], gauss->getVal()); + + // Delete the pdf before the Evaluator that holds a data token for it. + gauss.reset(); + evaluator.reset(); + + // A new model with the same node name must get a fresh data token. + auto gauss2 = std::make_unique("gauss", "gauss", *x, *mu, *sigma); + { + RooFit::Evaluator evaluator2{*gauss2}; + EXPECT_FLOAT_EQ(evaluator2.run()[0], gauss2->getVal()); + } + + // The other order: the node outlives the Evaluator. Its data token must + // have been reset such that it can be used by a new Evaluator. + RooFit::Evaluator evaluator3{*gauss2}; + EXPECT_FLOAT_EQ(evaluator3.run()[0], gauss2->getVal()); +}