From c9c8d02332ff334eddf20eeaf1670dbf0b15c861 Mon Sep 17 00:00:00 2001 From: Bo Svensson <90132211+bosvensson1@users.noreply.github.com> Date: Sat, 23 Oct 2021 08:31:46 +0000 Subject: [PATCH] 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 | 2 +- apps/openmw/mwrender/animation.cpp | 4 +-- apps/openmw/mwrender/creatureanimation.cpp | 2 +- apps/openmw/mwrender/npcanimation.cpp | 2 +- apps/openmw/mwrender/objectpaging.cpp | 7 ++++- components/resource/scenemanager.cpp | 31 ++++++++++++++-------- components/resource/scenemanager.hpp | 14 +++++++--- components/sceneutil/attach.cpp | 22 ++++++++------- components/sceneutil/attach.hpp | 6 ++++- components/sceneutil/clone.hpp | 1 + components/sceneutil/morphgeometry.cpp | 9 +++++++ components/sceneutil/riggeometry.cpp | 11 ++++++++ components/sceneutil/riggeometry.hpp | 7 +++++ 13 files changed, 86 insertions(+), 32 deletions(-) diff --git a/apps/opencs/view/render/actor.cpp b/apps/opencs/view/render/actor.cpp index 271ca2365a..94b82c96cd 100644 --- a/apps/opencs/view/render/actor.cpp +++ b/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); } } diff --git a/apps/openmw/mwrender/animation.cpp b/apps/openmw/mwrender/animation.cpp index 10624c8bca..ca76856d48 100644 --- a/apps/openmw/mwrender/animation.cpp +++ b/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 { diff --git a/apps/openmw/mwrender/creatureanimation.cpp b/apps/openmw/mwrender/creatureanimation.cpp index 502340d4b9..b64b205961 100644 --- a/apps/openmw/mwrender/creatureanimation.cpp +++ b/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 attached = SceneUtil::attach(node, mObjectRoot, bonename, found->second.get()); + osg::ref_ptr attached = SceneUtil::attach(node, mObjectRoot, bonename, found->second.get(), mResourceSystem->getSceneManager()); scene.reset(new PartHolder(attached)); diff --git a/apps/openmw/mwrender/npcanimation.cpp b/apps/openmw/mwrender/npcanimation.cpp index 0e100326dd..42215c00bc 100644 --- a/apps/openmw/mwrender/npcanimation.cpp +++ b/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 attached = SceneUtil::attach(templateNode, mObjectRoot, bonefilter, found->second); + osg::ref_ptr attached = SceneUtil::attach(templateNode, mObjectRoot, bonefilter, found->second, mResourceSystem->getSceneManager()); if (enchantedGlow) mGlowUpdater = SceneUtil::addEnchantedGlow(attached, mResourceSystem, *glowColor); diff --git a/apps/openmw/mwrender/objectpaging.cpp b/apps/openmw/mwrender/objectpaging.cpp index 5c22c3fc8d..947f23f781 100644 --- a/apps/openmw/mwrender/objectpaging.cpp +++ b/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) diff --git a/components/resource/scenemanager.cpp b/components/resource/scenemanager.cpp index c5ef957c3e..4184a77c54 100644 --- a/components/resource/scenemanager.cpp +++ b/components/resource/scenemanager.cpp @@ -693,19 +693,33 @@ namespace Resource } } - osg::ref_ptr SceneManager::createInstance(const std::string& name) + osg::ref_ptr SceneManager::getInstance(const std::string& name) { osg::ref_ptr scene = getTemplate(name); - return createInstance(scene); + return getInstance(scene); } - osg::ref_ptr SceneManager::createInstance(const osg::Node *base) + osg::ref_ptr SceneManager::cloneNode(const osg::Node* base) { - osg::ref_ptr cloned = static_cast(base->clone(SceneUtil::CopyOp())); - - // add a ref to the original template, to hint to the cache that it's still being used and should be kept in cache + SceneUtil::CopyOp copyop; + if (const osg::Drawable* drawable = base->asDrawable()) + { + 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 cloned = static_cast(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 SceneManager::getInstance(const osg::Node *base) + { + osg::ref_ptr 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 SceneManager::getInstance(const std::string &name) - { - return createInstance(name); - } - osg::ref_ptr SceneManager::getInstance(const std::string &name, osg::Group* parentNode) { osg::ref_ptr cloned = getInstance(name); diff --git a/components/resource/scenemanager.hpp b/components/resource/scenemanager.hpp index b5d3e453a0..85e012071d 100644 --- a/components/resource/scenemanager.hpp +++ b/components/resource/scenemanager.hpp @@ -132,16 +132,22 @@ namespace Resource /// @note Thread safe. osg::ref_ptr getTemplate(const std::string& name, bool compile=true); - osg::ref_ptr createInstance(const std::string& name); + /// Clone osg::Node safely. + /// @note Thread safe. + static osg::ref_ptr cloneNode(const osg::Node* base); - osg::ref_ptr createInstance(const osg::Node* base); void shareState(osg::ref_ptr 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 getInstance(const osg::Node* base); + + /// Instance the given scene template. /// @see getTemplate /// @note Thread safe. osg::ref_ptr 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 getInstance(const std::string& name, osg::Group* parentNode); diff --git a/components/sceneutil/attach.cpp b/components/sceneutil/attach.cpp index 6690148c74..9dabb282b0 100644 --- a/components/sceneutil/attach.cpp +++ b/components/sceneutil/attach.cpp @@ -13,9 +13,9 @@ #include #include +#include #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& node : mToCopy) + for (const osg::ref_ptr& node : mToCopy) { - mParent->addChild(static_cast(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>; + using NodeSet = std::set>; NodeSet mToCopy; osg::ref_ptr mParent; @@ -100,7 +100,7 @@ namespace SceneUtil } } - osg::ref_ptr attach(osg::ref_ptr toAttach, osg::Node *master, const std::string &filter, osg::Group* attachNode) + osg::ref_ptr attach(osg::ref_ptr toAttach, osg::Node *master, const std::string &filter, osg::Group* attachNode, Resource::SceneManager* sceneManager) { if (dynamic_cast(toAttach.get())) { @@ -108,7 +108,9 @@ namespace SceneUtil CopyRigVisitor copyVisitor(handle, filter); const_cast(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 clonedToAttach = static_cast(toAttach->clone(SceneUtil::CopyOp())); + osg::ref_ptr clonedToAttach = sceneManager->getInstance(toAttach); FindByNameVisitor findBoneOffset("BoneOffset"); clonedToAttach->accept(findBoneOffset); diff --git a/components/sceneutil/attach.hpp b/components/sceneutil/attach.hpp index 806fc53488..f5fc693724 100644 --- a/components/sceneutil/attach.hpp +++ b/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 attach(osg::ref_ptr toAttach, osg::Node* master, const std::string& filter, osg::Group* attachNode); + osg::ref_ptr attach(osg::ref_ptr toAttach, osg::Node* master, const std::string& filter, osg::Group* attachNode, Resource::SceneManager *sceneManager); } diff --git a/components/sceneutil/clone.hpp b/components/sceneutil/clone.hpp index 1cf00c9e58..35240cbfba 100644 --- a/components/sceneutil/clone.hpp +++ b/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 { diff --git a/components/sceneutil/morphgeometry.cpp b/components/sceneutil/morphgeometry.cpp index 78be559989..59adbffffe 100644 --- a/components/sceneutil/morphgeometry.cpp +++ b/components/sceneutil/morphgeometry.cpp @@ -1,6 +1,7 @@ #include "morphgeometry.hpp" #include +#include #include @@ -27,11 +28,19 @@ MorphGeometry::MorphGeometry(const MorphGeometry ©, const osg::CopyOp ©o void MorphGeometry::setSourceGeometry(osg::ref_ptr 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]; diff --git a/components/sceneutil/riggeometry.cpp b/components/sceneutil/riggeometry.cpp index ec00efa535..84b31f4afc 100644 --- a/components/sceneutil/riggeometry.cpp +++ b/components/sceneutil/riggeometry.cpp @@ -3,6 +3,7 @@ #include #include +#include #include #include "skeleton.hpp" @@ -60,12 +61,22 @@ RigGeometry::RigGeometry(const RigGeometry ©, const osg::CopyOp ©op) void RigGeometry::setSourceGeometry(osg::ref_ptr 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); diff --git a/components/sceneutil/riggeometry.hpp b/components/sceneutil/riggeometry.hpp index e01583399e..25ae5a3243 100644 --- a/components/sceneutil/riggeometry.hpp +++ b/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.