fixes a crash (#3183)

This PR fixes a crash caused by the improperly ensured lifetime of RigGeometry::mSourceGeometry. mSourceGeometry was not adequate to ensure mSourceGeometry would outlive mGeometry because we extend mGeometrys lifetime beyond this lifetime by passing mGeometry to the draw traversal instead of this.
In addition,

We add important comments.
We detect and prevent generally unsafe operations in high level code.
We add a sprinkling of const to help clarify intentions.

apps/opencs/view/render/actor.cpp

@@ -111,7 +111,7 @@ namespace CSVRender
         if (!mesh.empty() && node != mNodeMap.end())
         {
             auto instance = sceneMgr->getInstance(mesh);
-            SceneUtil::attach(instance, mSkeleton, boneName, node->second);
+            SceneUtil::attach(instance, mSkeleton, boneName, node->second, sceneMgr);
         }
     }
 

apps/openmw/mwrender/animation.cpp

@@ -1319,10 +1319,10 @@ namespace MWRender
 
                 cache.insert(std::make_pair(model, created));
 
-                return sceneMgr->createInstance(created);
+                return sceneMgr->getInstance(created);
             }
             else
-                return sceneMgr->createInstance(found->second);
+                return sceneMgr->getInstance(found->second);
         }
         else
         {

apps/openmw/mwrender/creatureanimation.cpp

@@ -164,7 +164,7 @@ void CreatureWeaponAnimation::updatePart(PartHolderPtr& scene, int slot)
         NodeMap::const_iterator found = nodeMap.find(Misc::StringUtils::lowerCase(bonename));
         if (found == nodeMap.end())
             throw std::runtime_error("Can't find attachment node " + bonename);
-        osg::ref_ptr<osg::Node> attached = SceneUtil::attach(node, mObjectRoot, bonename, found->second.get());
+        osg::ref_ptr<osg::Node> attached = SceneUtil::attach(node, mObjectRoot, bonename, found->second.get(), mResourceSystem->getSceneManager());
 
         scene.reset(new PartHolder(attached));
 

apps/openmw/mwrender/npcanimation.cpp

@@ -717,7 +717,7 @@ PartHolderPtr NpcAnimation::insertBoundedPart(const std::string& model, const st
     if (found == nodeMap.end())
         throw std::runtime_error("Can't find attachment node " + bonename);
 
-    osg::ref_ptr<osg::Node> attached = SceneUtil::attach(templateNode, mObjectRoot, bonefilter, found->second);
+    osg::ref_ptr<osg::Node> attached = SceneUtil::attach(templateNode, mObjectRoot, bonefilter, found->second, mResourceSystem->getSceneManager());
     if (enchantedGlow)
         mGlowUpdater = SceneUtil::addEnchantedGlow(attached, mResourceSystem, *glowColor);
 

apps/openmw/mwrender/objectpaging.cpp

@@ -617,6 +617,10 @@ namespace MWRender
                     pat->setAttitude(nodeAttitude);
                 }
 
+                // DO NOT COPY AND PASTE THIS CODE. Cloning osg::Geometry without also cloning its contained Arrays is generally unsafe.
+                // In this specific case the operation is safe under the following two assumptions:
+                // - When Arrays are removed or replaced in the cloned geometry, the original Arrays in their place must outlive the cloned geometry regardless. (ensured by TemplateMultiRef)
+                // - Arrays that we add or replace in the cloned geometry must be explicitely forbidden from reusing BufferObjects of the original geometry. (ensured by needvbo() in optimizer.cpp)
                 copyop.setCopyFlags(merge ? osg::CopyOp::DEEP_COPY_NODES|osg::CopyOp::DEEP_COPY_DRAWABLES : osg::CopyOp::DEEP_COPY_NODES);
                 copyop.mOptimizeBillboards = (size > 1/4.f);
                 copyop.mNodePath.push_back(trans);
@@ -645,7 +649,8 @@ namespace MWRender
             }
             if (numinstances > 0)
             {
-                // add a ref to the original template, to hint to the cache that it's still being used and should be kept in cache
+                // add a ref to the original template to help verify the safety of shallow cloning operations
+                // in addition, we hint to the cache that it's still being used and should be kept in cache
                 templateRefs->addRef(cnode);
 
                 if (pair.second.mNeedCompile)

components/resource/scenemanager.cpp

@@ -693,19 +693,33 @@ namespace Resource
         }
     }
 
-    osg::ref_ptr<osg::Node> SceneManager::createInstance(const std::string& name)
+    osg::ref_ptr<osg::Node> SceneManager::getInstance(const std::string& name)
     {
         osg::ref_ptr<const osg::Node> scene = getTemplate(name);
-        return createInstance(scene);
+        return getInstance(scene);
     }
 
-    osg::ref_ptr<osg::Node> SceneManager::createInstance(const osg::Node *base)
+    osg::ref_ptr<osg::Node> SceneManager::cloneNode(const osg::Node* base)
     {
-        osg::ref_ptr<osg::Node> cloned = static_cast<osg::Node*>(base->clone(SceneUtil::CopyOp()));
+        SceneUtil::CopyOp copyop;
-
+        if (const osg::Drawable* drawable = base->asDrawable())
-        // add a ref to the original template, to hint to the cache that it's still being used and should be kept in cache
+        {
+            if (drawable->asGeometry())
+            {
+                Log(Debug::Warning) << "SceneManager::cloneNode: attempting to clone osg::Geometry. For safety reasons this will be expensive. Consider avoiding this call.";
+                copyop.setCopyFlags(copyop.getCopyFlags()|osg::CopyOp::DEEP_COPY_ARRAYS|osg::CopyOp::DEEP_COPY_PRIMITIVES);
+            }
+        }
+        osg::ref_ptr<osg::Node> cloned = static_cast<osg::Node*>(base->clone(copyop));
+        // add a ref to the original template to help verify the safety of shallow cloning operations
+        // in addition, if this node is managed by a cache, we hint to the cache that it's still being used and should be kept in cache
         cloned->getOrCreateUserDataContainer()->addUserObject(new TemplateRef(base));
+        return cloned;
+    }
 
+    osg::ref_ptr<osg::Node> SceneManager::getInstance(const osg::Node *base)
+    {
+        osg::ref_ptr<osg::Node> cloned = cloneNode(base);
         // we can skip any scene graphs without update callbacks since we know that particle emitters will have an update callback set
         if (cloned->getNumChildrenRequiringUpdateTraversal() > 0)
         {
@@ -716,11 +730,6 @@ namespace Resource
         return cloned;
     }
 
-    osg::ref_ptr<osg::Node> SceneManager::getInstance(const std::string &name)
-    {
-        return createInstance(name);
-    }
-
     osg::ref_ptr<osg::Node> SceneManager::getInstance(const std::string &name, osg::Group* parentNode)
     {
         osg::ref_ptr<osg::Node> cloned = getInstance(name);

components/resource/scenemanager.hpp

@@ -132,16 +132,22 @@ namespace Resource
         /// @note Thread safe.
         osg::ref_ptr<const osg::Node> getTemplate(const std::string& name, bool compile=true);
 
-        osg::ref_ptr<osg::Node> createInstance(const std::string& name);
+        /// Clone osg::Node safely.
+        /// @note Thread safe.
+        static osg::ref_ptr<osg::Node> cloneNode(const osg::Node* base);
 
-        osg::ref_ptr<osg::Node> createInstance(const osg::Node* base);
         void shareState(osg::ref_ptr<osg::Node> node);
-        /// Get an instance of the given scene template
+
+        /// Clone osg::Node and adjust it according to SceneManager's settings.
+        /// @note Thread safe.
+        osg::ref_ptr<osg::Node> getInstance(const osg::Node* base);
+
+        /// Instance the given scene template.
         /// @see getTemplate
         /// @note Thread safe.
         osg::ref_ptr<osg::Node> getInstance(const std::string& name);
 
-        /// Get an instance of the given scene template and immediately attach it to a parent node
+        /// Instance the given scene template and immediately attach it to a parent node
         /// @see getTemplate
         /// @note Not thread safe, unless parentNode is not part of the main scene graph yet.
         osg::ref_ptr<osg::Node> getInstance(const std::string& name, osg::Group* parentNode);

components/sceneutil/attach.cpp

@@ -13,9 +13,9 @@
 #include <components/misc/stringops.hpp>
 
 #include <components/sceneutil/skeleton.hpp>
+#include <components/resource/scenemanager.hpp>
 
 #include "visitor.hpp"
-#include "clone.hpp"
 
 namespace SceneUtil
 {
@@ -49,10 +49,10 @@ namespace SceneUtil
             if (!filterMatches(drawable.getName()))
                 return;
 
-            osg::Node* node = &drawable;
+            const osg::Node* node = &drawable;
             for (auto it = getNodePath().rbegin()+1; it != getNodePath().rend(); ++it)
             {
-                osg::Node* parent = *it;
+                const osg::Node* parent = *it;
                 if (!filterMatches(parent->getName()))
                     break;
                 node = parent;
@@ -60,11 +60,11 @@ namespace SceneUtil
             mToCopy.emplace(node);
         }
 
-        void doCopy()
+        void doCopy(Resource::SceneManager* sceneManager)
         {
-            for (const osg::ref_ptr<osg::Node>& node : mToCopy)
+            for (const osg::ref_ptr<const osg::Node>& node : mToCopy)
             {
-                mParent->addChild(static_cast<osg::Node*>(node->clone(SceneUtil::CopyOp())));
+                mParent->addChild(sceneManager->getInstance(node));
             }
             mToCopy.clear();
         }
@@ -78,7 +78,7 @@ namespace SceneUtil
                 || (lowerName.size() >= mFilter2.size() && lowerName.compare(0, mFilter2.size(), mFilter2) == 0);
         }
 
-        using NodeSet = std::set<osg::ref_ptr<osg::Node>>;
+        using NodeSet = std::set<osg::ref_ptr<const osg::Node>>;
         NodeSet mToCopy;
 
         osg::ref_ptr<osg::Group> mParent;
@@ -100,7 +100,7 @@ namespace SceneUtil
         }
     }
 
-    osg::ref_ptr<osg::Node> attach(osg::ref_ptr<const osg::Node> toAttach, osg::Node *master, const std::string &filter, osg::Group* attachNode)
+    osg::ref_ptr<osg::Node> attach(osg::ref_ptr<const osg::Node> toAttach, osg::Node *master, const std::string &filter, osg::Group* attachNode, Resource::SceneManager* sceneManager)
     {
         if (dynamic_cast<const SceneUtil::Skeleton*>(toAttach.get()))
         {
@@ -108,7 +108,9 @@ namespace SceneUtil
 
             CopyRigVisitor copyVisitor(handle, filter);
             const_cast<osg::Node*>(toAttach.get())->accept(copyVisitor);
-            copyVisitor.doCopy();
+            copyVisitor.doCopy(sceneManager);
+            // add a ref to the original template to hint to the cache that it is still being used and should be kept in cache.
+            handle->getOrCreateUserDataContainer()->addUserObject(new Resource::TemplateRef(toAttach));
 
             if (handle->getNumChildren() == 1)
             {
@@ -127,7 +129,7 @@ namespace SceneUtil
         }
         else
         {
-            osg::ref_ptr<osg::Node> clonedToAttach = static_cast<osg::Node*>(toAttach->clone(SceneUtil::CopyOp()));
+            osg::ref_ptr<osg::Node> clonedToAttach = sceneManager->getInstance(toAttach);
 
             FindByNameVisitor findBoneOffset("BoneOffset");
             clonedToAttach->accept(findBoneOffset);

components/sceneutil/attach.hpp

@@ -10,6 +10,10 @@ namespace osg
     class Node;
     class Group;
 }
+namespace Resource
+{
+    class SceneManager;
+}
 
 namespace SceneUtil
 {
@@ -19,7 +23,7 @@ namespace SceneUtil
     /// Otherwise, just attach all of the toAttach scenegraph to the attachment node on the master scenegraph, with no filtering.
     /// @note The master scene graph is expected to include a skeleton.
     /// @return A newly created node that is directly attached to the master scene graph
-    osg::ref_ptr<osg::Node> attach(osg::ref_ptr<const osg::Node> toAttach, osg::Node* master, const std::string& filter, osg::Group* attachNode);
+    osg::ref_ptr<osg::Node> attach(osg::ref_ptr<const osg::Node> toAttach, osg::Node* master, const std::string& filter, osg::Group* attachNode, Resource::SceneManager *sceneManager);
 
 }
 

components/sceneutil/clone.hpp

@@ -18,6 +18,7 @@ namespace SceneUtil
     /// @par Defines the cloning behaviour we need:
     /// * Assigns updated ParticleSystem pointers on cloned emitters and programs.
     /// * Deep copies RigGeometry and MorphGeometry so they can animate without affecting clones.
+    /// @warning Avoid using this class directly. The safety of cloning operations depends on the copy flags and the objects involved. Consider using SceneManager::cloneNode for additional safety.
     /// @warning Do not use an object of this class for more than one copy operation.
     class CopyOp : public osg::CopyOp
     {

components/sceneutil/morphgeometry.cpp

@@ -1,6 +1,7 @@
 #include "morphgeometry.hpp"
 
 #include <cassert>
+#include <components/resource/scenemanager.hpp>
 
 #include <osg/Version>
 
@@ -27,11 +28,19 @@ MorphGeometry::MorphGeometry(const MorphGeometry &copy, const osg::CopyOp &copyo
 
 void MorphGeometry::setSourceGeometry(osg::ref_ptr<osg::Geometry> sourceGeom)
 {
+    for (unsigned int i=0; i<2; ++i)
+        mGeometry[i] = nullptr;
+
     mSourceGeometry = sourceGeom;
 
     for (unsigned int i=0; i<2; ++i)
     {
+        // DO NOT COPY AND PASTE THIS CODE. Cloning osg::Geometry without also cloning its contained Arrays is generally unsafe.
+        // In this specific case the operation is safe under the following two assumptions:
+        // - When Arrays are removed or replaced in the cloned geometry, the original Arrays in their place must outlive the cloned geometry regardless. (ensured by TemplateRef)
+        // - Arrays that we add or replace in the cloned geometry must be explicitely forbidden from reusing BufferObjects of the original geometry. (ensured by vbo below)
         mGeometry[i] = new osg::Geometry(*mSourceGeometry, osg::CopyOp::SHALLOW_COPY);
+        mGeometry[i]->getOrCreateUserDataContainer()->addUserObject(new Resource::TemplateRef(mSourceGeometry));
 
         const osg::Geometry& from = *mSourceGeometry;
         osg::Geometry& to = *mGeometry[i];

components/sceneutil/riggeometry.cpp

@@ -3,6 +3,7 @@
 #include <osg/Version>
 
 #include <components/debug/debuglog.hpp>
+#include <components/resource/scenemanager.hpp>
 #include <osg/MatrixTransform>
 
 #include "skeleton.hpp"
@@ -60,12 +61,22 @@ RigGeometry::RigGeometry(const RigGeometry &copy, const osg::CopyOp &copyop)
 
 void RigGeometry::setSourceGeometry(osg::ref_ptr<osg::Geometry> sourceGeometry)
 {
+    for (unsigned int i=0; i<2; ++i)
+        mGeometry[i] = nullptr;
+
     mSourceGeometry = sourceGeometry;
 
     for (unsigned int i=0; i<2; ++i)
     {
         const osg::Geometry& from = *sourceGeometry;
+
+        // DO NOT COPY AND PASTE THIS CODE. Cloning osg::Geometry without also cloning its contained Arrays is generally unsafe.
+        // In this specific case the operation is safe under the following two assumptions:
+        // - When Arrays are removed or replaced in the cloned geometry, the original Arrays in their place must outlive the cloned geometry regardless. (ensured by mSourceGeometry)
+        // - Arrays that we add or replace in the cloned geometry must be explicitely forbidden from reusing BufferObjects of the original geometry. (ensured by vbo below)
         mGeometry[i] = new osg::Geometry(from, osg::CopyOp::SHALLOW_COPY);
+        mGeometry[i]->getOrCreateUserDataContainer()->addUserObject(new Resource::TemplateRef(mSourceGeometry));
+
         osg::Geometry& to = *mGeometry[i];
         to.setSupportsDisplayList(false);
         to.setUseVertexBufferObjects(true);

components/sceneutil/riggeometry.hpp

@@ -9,6 +9,13 @@ namespace SceneUtil
     class Skeleton;
     class Bone;
 
+    // TODO: This class has a lot of issues.
+    // - We require too many workarounds to ensure safety.
+    // - mSourceGeometry should be const, but can not be const because of a use case in shadervisitor.cpp.
+    // - We create useless mGeometry clones in template RigGeometries.
+    // - We do not support compileGLObjects.
+    // - We duplicate some code in MorphGeometry. 
+
     /// @brief Mesh skinning implementation.
     /// @note A RigGeometry may be attached directly to a Skeleton, or somewhere below a Skeleton.
     /// Note though that the RigGeometry ignores any transforms below the Skeleton, so the attachment point is not that important.