Merge pull request #343 from JeffersonLab/nbrei_issue333

Bugfix: Use-after-free when same plugin is loaded twice

.github/workflows/eicshell.yml

@@ -24,6 +24,7 @@ jobs:
           cd $GITHUB_WORKSPACE/build
           cmake .. \
             -DCMAKE_INSTALL_PREFIX=$GITHUB_WORKSPACE \
+            -DCMAKE_BUILD_TYPE=Debug \
             -DCMAKE_CXX_STANDARD=20 \
             -DUSE_PYTHON=ON \
             -DUSE_ROOT=ON \
@@ -40,23 +41,26 @@ jobs:
           cd $GITHUB_WORKSPACE/build
           make -j4 install
     
-    - name: Examine dynamic linking
-      run: |
-        export JANA_HOME=$GITHUB_WORKSPACE
-        export JANA_PLUGIN_PATH=$JANA_HOME/plugins
-        export LD_LIBRARY_PATH=$JANA_HOME/lib:$JANA_HOME/lib/JANA/plugins:$LD_LIBRARY_PATH
-        echo "ldd bin/jana"
-        ldd bin/jana
-        echo "ldd bin/libJANA.so"
-        ldd lib/libJANA.so
-        echo "bin/jana rpath"
-        objdump -x bin/jana | grep PATH
-        echo "lib/libJANA.so rpath"
-        objdump -x lib/libJANA.so | grep PATH
-        echo "lib/JANA/plugins/TimesliceExample.so"
-        objdump -x lib/JANA/plugins/TimesliceExample.so | grep PATH
-        echo "bin/PodioExample"
-        objdump -x bin/PodioExample | grep PATH
+    - name: Examine JANA2 dynamic linking
+      uses: eic/run-cvmfs-osg-eic-shell@main
+      with:
+        platform-release: "jug_xl:nightly"
+        run: |
+          export JANA_HOME=$GITHUB_WORKSPACE
+          export JANA_PLUGIN_PATH=$JANA_HOME/plugins
+          export LD_LIBRARY_PATH=$JANA_HOME/lib:$JANA_HOME/lib/JANA/plugins:$LD_LIBRARY_PATH
+          echo "ldd bin/jana"
+          ldd bin/jana
+          echo "ldd bin/libJANA.so"
+          ldd lib/libJANA.so
+          echo "bin/jana rpath"
+          objdump -x bin/jana | grep PATH
+          echo "lib/libJANA.so rpath"
+          objdump -x lib/libJANA.so | grep PATH
+          echo "lib/JANA/plugins/TimesliceExample.so"
+          objdump -x lib/JANA/plugins/TimesliceExample.so | grep PATH
+          echo "bin/PodioExample"
+          objdump -x bin/PodioExample | grep PATH
 
     - name: Run JTest plugin with 100 events
       uses: eic/run-cvmfs-osg-eic-shell@main
@@ -115,8 +119,24 @@ jobs:
           cd ..
           git clone https://github.com/eic/EICrecon
           cd EICrecon
-          cmake -S . -B build -DJANA_DIR=$JANA_HOME/lib/cmake/JANA
+          cmake -S . -B build -DJANA_DIR=$JANA_HOME/lib/JANA/cmake -DCMAKE_BUILD_TYPE=Debug
           cmake --build build --target install -- -j8
+    
+    - name: Examine EICrecon dynamic linking
+      uses: eic/run-cvmfs-osg-eic-shell@main
+      with:
+        platform-release: "jug_xl:nightly"
+        run: |
+          export JANA_HOME=$GITHUB_WORKSPACE
+          export JANA_PLUGIN_PATH=$JANA_HOME/plugins
+          export LD_LIBRARY_PATH=${GITHUB_WORKSPACE}/../EICrecon/lib:${GITHUB_WORKSPACE}/../EICrecon/lib/EICrecon/plugins:${JANA_HOME}/lib:${JANA_HOME}/lib/JANA/plugins:$LD_LIBRARY_PATH
+          cd ../EICrecon
+          echo "ldd bin/eicrecon"
+          ldd bin/eicrecon
+          echo "ldd lib/libreco.so"
+          ldd lib/libreco.so
+          echo "ldd lib/EICrecon/plugins/tracking.so"
+          ldd lib/EICrecon/plugins/tracking.so
 
     - name: Generate EICrecon input data
       uses: eic/run-cvmfs-osg-eic-shell@main
@@ -136,9 +156,8 @@ jobs:
         platform-release: "jug_xl:nightly"
         setup: "/opt/detector/epic-main/bin/thisepic.sh"
         run: |
-          echo "--- Running EICrecon ---"
           export JANA_HOME=$GITHUB_WORKSPACE
           export JANA_PLUGIN_PATH=$GITHUB_WORKSPACE/../EICrecon/lib/EICrecon/plugins
-          export LD_LIBRARY_PATH=$GITHUB_WORKSPACE/../EICrecon/lib:$JANA_HOME/lib:$JANA_HOME/lib/JANA/plugins:$LD_LIBRARY_PATH
+          export LD_LIBRARY_PATH=${GITHUB_WORKSPACE}/../EICrecon/lib:${GITHUB_WORKSPACE}/../EICrecon/lib/EICrecon/plugins:${JANA_HOME}/lib:${JANA_HOME}/lib/JANA/plugins:$LD_LIBRARY_PATH
           ../EICrecon/bin/eicrecon sim_e_1GeV_20GeV_craterlake.edm4hep.root
 

src/libraries/JANA/JEventProcessor.h

@@ -112,9 +112,9 @@ class JEventProcessor : public jana::omni::JComponent,
     }
 
 
-    void Summarize(JComponentSummary& summary) {
+    void Summarize(JComponentSummary& summary) const override {
         auto* result = new JComponentSummary::Component(
-                JComponentSummary::ComponentType::Processor, GetPrefix(), GetTypeName(), GetLevel(), GetPluginName());
+            "Processor", GetPrefix(), GetTypeName(), GetLevel(), GetPluginName());
 
         for (const auto* input : m_inputs) {
             size_t subinput_count = input->names.size();

src/libraries/JANA/JEventSource.h

@@ -373,10 +373,10 @@ class JEventSource : public jana::omni::JComponent, public jana::omni::JHasOutpu
         }
     }
 
-    void Summarize(JComponentSummary& summary) {
+    void Summarize(JComponentSummary& summary) const override {
 
         auto* result = new JComponentSummary::Component(
-                JComponentSummary::ComponentType::Source, GetPrefix(), GetTypeName(), GetLevel(), GetPluginName());
+            "Source", GetPrefix(), GetTypeName(), GetLevel(), GetPluginName());
 
         for (const auto* output : m_outputs) {
             size_t suboutput_count = output->collection_names.size();

src/libraries/JANA/JEventUnfolder.h

@@ -169,9 +169,9 @@ class JEventUnfolder : public jana::omni::JComponent,
         }
     }
 
-    void Summarize(JComponentSummary& summary) override {
+    void Summarize(JComponentSummary& summary) const override {
         auto* us = new JComponentSummary::Component( 
-                JComponentSummary::ComponentType::Unfolder, GetPrefix(), GetTypeName(), GetLevel(), GetPluginName());
+            "Unfolder", GetPrefix(), GetTypeName(), GetLevel(), GetPluginName());
 
         for (const auto* input : m_inputs) {
             size_t subinput_count = input->names.size();

src/libraries/JANA/JFactory.cc

@@ -47,10 +47,10 @@ void JFactory::DoInit() {
     }
 }
 
-void JFactory::Summarize(JComponentSummary& summary) {
+void JFactory::Summarize(JComponentSummary& summary) const {
 
     auto fs = new JComponentSummary::Component(
-            JComponentSummary::ComponentType::Factory,
+            "Factory",
             GetPrefix(),
             GetTypeName(),
             GetLevel(),

src/libraries/JANA/JFactory.h

@@ -174,7 +174,7 @@ class JFactory : public jana::omni::JComponent {
     /// type of object contained. In order to access these objects when all you have is a JFactory*, use JFactory::GetAs().
     virtual void Create(const std::shared_ptr<const JEvent>& event);
     void DoInit();
-    void Summarize(JComponentSummary& summary);
+    void Summarize(JComponentSummary& summary) const override;
 
 
     virtual void Set(const std::vector<JObject *> &data) = 0;

src/libraries/JANA/JFactoryGenerator.h

@@ -57,7 +57,7 @@ class JFactoryGeneratorT : public JFactoryGenerator {
             // Otherwise, use whatever tag the factory may have set for itself.
             factory->SetTag(m_tag);
         }
-        factory->SetFactoryName(JTypeInfo::demangle<T>());
+        factory->SetTypeName(JTypeInfo::demangle<T>());
         factory->SetPluginName(GetPluginName());
         factory->SetApplication(GetApplication());
         factory->SetLogger(GetApplication()->template GetService<JLoggingService>()->get_logger(factory->GetPrefix()));

src/libraries/JANA/JFactorySet.cc

@@ -5,7 +5,6 @@
 #include <iterator>
 #include <iostream>
 
-#include "JApplication.h"
 #include "JFactorySet.h"
 #include "JFactory.h"
 #include "JMultifactory.h"
@@ -22,32 +21,11 @@ JFactorySet::JFactorySet(void)
 //---------------------------------
 // JFactorySet    (Constructor)
 //---------------------------------
-JFactorySet::JFactorySet(const std::vector<JFactoryGenerator*>& aFactoryGenerators)
+JFactorySet::JFactorySet(const std::vector<JFactoryGenerator*>& generators)
 {
-    //Add all factories from all factory generators
-    for(auto sGenerator : aFactoryGenerators){
-
-        // Generate the factories into a temporary JFactorySet.
-        JFactorySet myset;
-        sGenerator->GenerateFactories( &myset );
-
-        // Merge factories from temporary JFactorySet into this one. Any that
-        // already exist here will leave the duplicates in the temporary set
-        // where they will be destroyed by its destructor as it falls out of scope.
-        Merge( myset );
-    }
-}
-
-JFactorySet::JFactorySet(JFactoryGenerator* source_gen, const std::vector<JFactoryGenerator*>& default_gens) {
-
-    if (source_gen != nullptr) source_gen->GenerateFactories(this);
-    for (auto gen : default_gens) {
-        JFactorySet temp_set;
-        gen->GenerateFactories(&temp_set);
-        Merge(temp_set); // Factories which are shadowed stay in temp_set; others are removed
-        for (auto straggler : temp_set.GetAllFactories()) {
-            LOG << "Factory '" << straggler->GetFactoryName() << "' overriden, will be excluded from event." << LOG_END;
-        }
+    // Add all factories from all factory generators
+    for(auto generator : generators){
+        generator->GenerateFactories(this);
     }
 }
 
@@ -64,8 +42,6 @@ JFactorySet::~JFactorySet()
     }
     // Now that the factories are deleted, nothing can call the multifactories so it is safe to delete them as well
     for (auto* mf : mMultifactories) { delete mf; }
-
-
 }
 
 //---------------------------------
@@ -88,8 +64,20 @@ bool JFactorySet::Add(JFactory* aFactory)
     if (typed_result != std::end(mFactories) || untyped_result != std::end(mFactoriesFromString)) {
         // Factory is duplicate. Since this almost certainly indicates a user error, and
         // the caller will not be able to do anything about it anyway, throw an exception.
-        throw JException("JFactorySet::Add failed because factory is duplicate");
-        // return false;
+        // We show the user which factory is causing this problem, including both plugin names
+        std::string other_plugin_name;
+        if (typed_result != std::end(mFactories)) {
+            other_plugin_name = typed_result->second->GetPluginName();
+        }
+        else {
+            other_plugin_name = untyped_result->second->GetPluginName();
+        }
+        auto ex = JException("Attempted to add duplicate factories");
+        ex.function_name = "JFactorySet::Add";
+        ex.instance_name = aFactory->GetPrefix();
+        ex.type_name = aFactory->GetTypeName();
+        ex.plugin_name = aFactory->GetPluginName() + ", " + other_plugin_name;
+        throw ex;
     }
 
     mFactories[typed_key] = aFactory;
@@ -153,51 +141,6 @@ std::vector<JMultifactory*> JFactorySet::GetAllMultifactories() const {
     return results;
 }
 
-//---------------------------------
-// Merge
-//---------------------------------
-void JFactorySet::Merge(JFactorySet &aFactorySet)
-{
-    /// Merge any factories in the specified JFactorySet into this
-    /// one. Any factories which don't have the same type and tag as one
-    /// already in this set will be transferred and this JFactorySet
-    /// will take ownership of them. Ones that have a type and tag
-    /// that matches one already in this set will be left in the
-    /// original JFactorySet. Thus, all factories left in the JFactorySet
-    /// passed into this method upon return from it can be considered
-    /// duplicates. It will be left to the caller to delete those.
-
-    JFactorySet tmpSet; // keep track of duplicates to copy back into aFactorySet
-    for( auto pair : aFactorySet.mFactories ){
-        auto factory = pair.second;
-
-        auto typed_key = std::make_pair(factory->GetObjectType(), factory->GetTag());
-        auto untyped_key = std::make_pair(factory->GetObjectName(), factory->GetTag());
-
-        auto typed_result = mFactories.find(typed_key);
-        auto untyped_result = mFactoriesFromString.find(untyped_key);
-
-        if (typed_result != std::end(mFactories) || untyped_result != std::end(mFactoriesFromString)) {
-            // Factory is duplicate. Return to caller just in case
-            tmpSet.mFactories[pair.first] = factory;
-        }
-        else {
-            mFactories[typed_key] = factory;
-            mFactoriesFromString[untyped_key] = factory;
-        }
-    }
-
-    // Copy duplicates back to aFactorySet
-    aFactorySet.mFactories.swap( tmpSet.mFactories );
-    tmpSet.mFactories.clear(); // prevent ~JFactorySet from deleting any factories
-
-    // Move ownership of multifactory pointers over.
-    for (auto* mf : aFactorySet.mMultifactories) {
-        mMultifactories.push_back(mf);
-    }
-    aFactorySet.mMultifactories.clear();
-}
-
 //---------------------------------
 // Print
 //---------------------------------

src/libraries/JANA/JFactorySet.h

@@ -21,14 +21,12 @@ class JMultifactory;
 class JFactorySet : public JResettable
 {
     public:
-        JFactorySet(void);
+        JFactorySet();
         JFactorySet(const std::vector<JFactoryGenerator*>& aFactoryGenerators);
-        JFactorySet(JFactoryGenerator* source_gen, const std::vector<JFactoryGenerator*>& default_gens);
         virtual ~JFactorySet();
 
         bool Add(JFactory* aFactory);
         bool Add(JMultifactory* multifactory);
-        void Merge(JFactorySet &aFactorySet);
         void Print(void) const;
         void Release(void);
 

src/libraries/JANA/JMultifactory.cc

@@ -74,15 +74,15 @@ void JMultifactory::DoInit() {
     }
 }
 
-void JMultifactory::Summarize(JComponentSummary& summary) {
+void JMultifactory::Summarize(JComponentSummary& summary) const {
     auto mfs = new JComponentSummary::Component(
-            JComponentSummary::ComponentType::Factory,
+            "Multifactory",
             GetPrefix(),
             GetTypeName(),
             GetLevel(),
             GetPluginName());
 
-    for (auto* helper : GetHelpers()->GetAllFactories()) {
+    for (auto* helper : mHelpers.GetAllFactories()) {
         auto coll = new JComponentSummary::Collection(
                 helper->GetTag(), 
                 helper->GetFactoryName(),