[6.42] [RF] Don't touch deleted nodes in RooFit::Evaluator destructor

roofit/roofit/test/testRooMomentMorphFuncND.cxx

@@ -3,6 +3,7 @@
 
 #include <RooBinning.h>
 #include <RooDataHist.h>
+#include <RooDataSet.h>
 #include <RooFit/Detail/NormalizationHelpers.h>
 #include <RooFit/Evaluator.h>
 #include <RooGaussian.h>
@@ -15,27 +16,17 @@
 
 #include <gtest/gtest.h>
 
+#include <cmath>
 #include <memory>
 
 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<RooWorkspace> 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>();
+   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<RooWorkspace> 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<RooWorkspace> ws = makeMorphWorkspace(/*pdfMode=*/true);
+   RooAbsPdf &morphPdf = *ws->pdf("morph");
+   std::unique_ptr<RooDataSet> data{ws->pdf("hp0")->generate(*ws->var("x"), 100)};
+   std::unique_ptr<RooAbsReal> nll{morphPdf.createNLL(*data)};
+   EXPECT_TRUE(std::isfinite(nll->getVal()));
+
+   ws.reset();
+   nll.reset();
+}

roofit/roofitcore/inc/RooAbsArg.h

@@ -52,6 +52,7 @@ using RooListProxy = RooCollectionProxy<RooArgList>;
 class RooExpensiveObjectCache;
 class RooWorkspace;
 namespace RooFit {
+class Evaluator;
 namespace Experimental {
 class CodegenContext;
 }
@@ -612,8 +613,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<std::size_t>::max(); }
-   void setDataToken(std::size_t index);
-   void resetDataToken() { _dataToken = std::numeric_limits<std::size_t>::max(); }
+   void setDataToken(std::size_t index, RooFit::Evaluator *evaluator = nullptr);
+   void resetDataToken()
+   {
+      _dataToken = std::numeric_limits<std::size_t>::max();
+      _evaluator = nullptr;
+   }
 
 protected:
    mutable bool _valueDirty = true; // Flag set if value needs recalculating because input values modified
@@ -638,6 +643,7 @@ class RooAbsArg : public TNamed, public RooPrintable {
 
    std::size_t _dataToken = std::numeric_limits<std::size_t>::max(); ///<! Set by the RooFitDriver for this arg to
                                                                      ///<! retrieve its result in the run context
+   RooFit::Evaluator *_evaluator = nullptr; ///<! The Evaluator that set the data token and resets it in its destructor
 
    /// \cond ROOFIT_INTERNAL
    // Legacy streamers need the following statics:

roofit/roofitcore/inc/RooFit/Evaluator.h

@@ -51,6 +51,8 @@ class Evaluator {
 
    std::unique_ptr<ChangeOperModeRAII> setOperModes(RooAbsArg::OperMode opMode);
 
+   void unregisterNode(RooAbsArg const &arg);
+
 private:
    void processVariable(NodeInfo &nodeInfo);
    void processCategory(NodeInfo &nodeInfo);

roofit/roofitcore/src/RooAbsArg.cxx

@@ -74,6 +74,7 @@ for single nodes.
 #include <RooArgSet.h>
 #include <RooConstVar.h>
 #include <RooExpensiveObjectCache.h>
+#include <RooFit/Evaluator.h>
 #include <RooHelpers.h>
 #include "RooFitImplHelpers.h"
 #include <RooListProxy.h>
@@ -168,6 +169,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);
@@ -2164,17 +2171,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<std::size_t>::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;
 }

roofit/roofitcore/src/RooFit/Evaluator.cxx

@@ -190,7 +190,7 @@ Evaluator::Evaluator(const RooAbsReal &absReal, bool useGPU)
       if (dynamic_cast<RooRealVar const *>(arg)) {
          nodeInfo.isVariable = true;
       } else {
-         arg->setDataToken(iNode);
+         arg->setDataToken(iNode, this);
       }
       if (dynamic_cast<RooAbsCategory const *>(arg)) {
          nodeInfo.isCategory = true;
@@ -351,7 +351,8 @@ 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();
       }
    }
@@ -360,6 +361,18 @@ Evaluator::~Evaluator()
    }
 }
 
+/// 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;

roofit/roofitcore/test/CMakeLists.txt

@@ -84,6 +84,7 @@ endif()
 ROOT_ADD_GTEST(testNaNPacker testNaNPacker.cxx LIBRARIES RooFitCore)
 ROOT_ADD_GTEST(testRooBatchCompute testRooBatchCompute.cxx LIBRARIES RooBatchCompute)
 ROOT_ADD_GTEST(testRooEfficiency testRooEfficiency.cxx LIBRARIES RooFitCore RooFit)
+ROOT_ADD_GTEST(testRooEvaluator testRooEvaluator.cxx LIBRARIES RooFitCore RooFit)
 ROOT_ADD_GTEST(testRooExtendedBinding testRooExtendedBinding.cxx LIBRARIES RooFitCore RooFit)
 ROOT_ADD_GTEST(testRooMCStudy testRooMCStudy.cxx LIBRARIES RooFitCore RooFit)
 ROOT_ADD_GTEST(testRooMinimizer testRooMinimizer.cxx LIBRARIES RooFitCore RooFit)

roofit/roofitcore/test/testRooEvaluator.cxx

@@ -0,0 +1,43 @@
+// Tests for RooFit::Evaluator
+// Author: Jonas Rembser, CERN 10/2026
+
+#include <RooFit/Evaluator.h>
+#include <RooGaussian.h>
+#include <RooRealVar.h>
+
+#include <gtest/gtest.h>
+
+#include <memory>
+
+/// 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<RooRealVar>("x", "x", 0.0, -5.0, 5.0);
+   auto mu = std::make_unique<RooRealVar>("mu", "mu", 0.0, -1.0, 1.0);
+   auto sigma = std::make_unique<RooRealVar>("sigma", "sigma", 1.0, 0.1, 10.0);
+   auto gauss = std::make_unique<RooGaussian>("gauss", "gauss", *x, *mu, *sigma);
+
+   auto evaluator = std::make_unique<RooFit::Evaluator>(*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<RooGaussian>("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());
+}

tutorials/roofit/roofit/rf619_discrete_profiling.py

@@ -166,11 +166,6 @@
     minim.minimize("Minuit2", "Migrad")
     profileGraph.SetPoint(i, meanVal, nll.getVal())
 
-# The fit objects are not needed anymore. Delete them already before the
-# plotting phase, so that the NLL evaluation machinery is not torn down at
-# interpreter shutdown, where the order of cleanups is less controlled.
-del minim
-del nll
 
 c = ROOT.TCanvas("c_rf619", "NLL vs Mean for Different Discrete Combinations", 1200, 400)
 c.Divide(3, 1)

tutorials/roofit/roostats/rs401d_FeldmanCousins.py

@@ -164,12 +164,6 @@
     dataCanvas.Draw()
     dataCanvas.SaveAs("3.png")
 
-    # The profile likelihood and the NLL are not needed anymore. Delete them
-    # already here, so that the NLL evaluation machinery is not torn down at
-    # interpreter shutdown, where the order of cleanups is less controlled.
-    del pll
-    del nll
-
     # --------------------------------------------------------------
     # show use of Feldman-Cousins utility in RooStats
     # set the distribution creator, which encodes the test statistic
@@ -236,7 +230,7 @@
         forContour = hist.Clone()
 
         # now loop through the points and put a marker if it's in the interval
         tmpPoint = RooArgSet()
         # loop over points to test
         for i in range(parameterScan.numEntries):
             # get a parameter point from the list of points to test.
@@ -263,7 +257,7 @@
 
     mcPlot = ROOT.nullptr
     if mcInt:
         print(f"MCMC actual confidence level: ", mcInt.GetActualConfidenceLevel())
         mcPlot = ROOT.RooStats.MCMCIntervalPlot(mcInt)
         mcPlot.SetLineColor(ROOT.kMagenta)
         mcPlot.Draw()