Merge pull request #1580 from assimp/scene_combiner_crash

check for nullptr dereferencing before copying scene data

code/ColladaExporter.cpp

@@ -1280,7 +1280,7 @@ void ColladaExporter::WriteAnimationLibrary(size_t pIndex)
 
 			std::vector<ai_real> frames;
 			for( size_t i = 0; i < nodeAnim->mNumPositionKeys; ++i) {
-				frames.push_back(nodeAnim->mPositionKeys[i].mTime);
+				frames.push_back(static_cast<ai_real>(nodeAnim->mPositionKeys[i].mTime));
 			}
 			
 			WriteFloatArray( node_idstr , FloatType_Time, (const ai_real*) frames.data(), frames.size());

code/SceneCombiner.cpp

@@ -66,8 +66,8 @@ namespace Assimp {
 
 // ------------------------------------------------------------------------------------------------
 // Add a prefix to a string
-inline void PrefixString(aiString& string,const char* prefix, unsigned int len)
+inline
-{
+void PrefixString(aiString& string,const char* prefix, unsigned int len) {
     // If the string is already prefixed, we won't prefix it a second time
     if (string.length >= 1 && string.data[0] == '$')
         return;
@@ -88,8 +88,7 @@ inline void PrefixString(aiString& string,const char* prefix, unsigned int len)
 
 // ------------------------------------------------------------------------------------------------
 // Add node identifiers to a hashing set
-void SceneCombiner::AddNodeHashes(aiNode* node, std::set<unsigned int>& hashes)
+void SceneCombiner::AddNodeHashes(aiNode* node, std::set<unsigned int>& hashes) {
-{
     // Add node name to hashing set if it is non-empty - empty nodes are allowed
     // and they can't have any anims assigned so its absolutely safe to duplicate them.
     if (node->mName.length) {
@@ -103,25 +102,23 @@ void SceneCombiner::AddNodeHashes(aiNode* node, std::set<unsigned int>& hashes)
 
 // ------------------------------------------------------------------------------------------------
 // Add a name prefix to all nodes in a hierarchy
-void SceneCombiner::AddNodePrefixes(aiNode* node, const char* prefix, unsigned int len)
+void SceneCombiner::AddNodePrefixes(aiNode* node, const char* prefix, unsigned int len) {
-{
     ai_assert(NULL != prefix);
     PrefixString(node->mName,prefix,len);
 
     // Process all children recursively
-    for (unsigned int i = 0; i < node->mNumChildren;++i)
+    for ( unsigned int i = 0; i < node->mNumChildren; ++i ) {
-        AddNodePrefixes(node->mChildren[i],prefix,len);
+        AddNodePrefixes( node->mChildren[ i ], prefix, len );
+    }
 }
 
 // ------------------------------------------------------------------------------------------------
 // Search for matching names
-bool SceneCombiner::FindNameMatch(const aiString& name, std::vector<SceneHelper>& input, unsigned int cur)
+bool SceneCombiner::FindNameMatch(const aiString& name, std::vector<SceneHelper>& input, unsigned int cur) {
-{
     const unsigned int hash = SuperFastHash(name.data, static_cast<uint32_t>(name.length));
 
     // Check whether we find a positive match in one of the given sets
     for (unsigned int i = 0; i < input.size(); ++i) {
-
         if (cur != i && input[i].hashes.find(hash) != input[i].hashes.end()) {
             return true;
         }
@@ -132,14 +129,12 @@ bool SceneCombiner::FindNameMatch(const aiString& name, std::vector<SceneHelper>
 // ------------------------------------------------------------------------------------------------
 // Add a name prefix to all nodes in a hierarchy if a hash match is found
 void SceneCombiner::AddNodePrefixesChecked(aiNode* node, const char* prefix, unsigned int len,
-    std::vector<SceneHelper>& input, unsigned int cur)
+        std::vector<SceneHelper>& input, unsigned int cur) {
-{
     ai_assert(NULL != prefix);
     const unsigned int hash = SuperFastHash(node->mName.data, static_cast<uint32_t>(node->mName.length));
 
     // Check whether we find a positive match in one of the given sets
     for (unsigned int i = 0; i < input.size(); ++i) {
-
         if (cur != i && input[i].hashes.find(hash) != input[i].hashes.end()) {
             PrefixString(node->mName,prefix,len);
             break;
@@ -153,27 +148,25 @@ void SceneCombiner::AddNodePrefixesChecked(aiNode* node, const char* prefix, uns
 
 // ------------------------------------------------------------------------------------------------
 // Add an offset to all mesh indices in a node graph
-void SceneCombiner::OffsetNodeMeshIndices (aiNode* node, unsigned int offset)
+void SceneCombiner::OffsetNodeMeshIndices (aiNode* node, unsigned int offset) {
-{
     for (unsigned int i = 0; i < node->mNumMeshes;++i)
         node->mMeshes[i] += offset;
 
-    for (unsigned int i = 0; i < node->mNumChildren;++i)
+    for ( unsigned int i = 0; i < node->mNumChildren; ++i ) {
-        OffsetNodeMeshIndices(node->mChildren[i],offset);
+        OffsetNodeMeshIndices( node->mChildren[ i ], offset );
+    }
 }
 
 // ------------------------------------------------------------------------------------------------
 // Merges two scenes. Currently only used by the LWS loader.
-void SceneCombiner::MergeScenes(aiScene** _dest,std::vector<aiScene*>& src,
+void SceneCombiner::MergeScenes(aiScene** _dest,std::vector<aiScene*>& src, unsigned int flags) {
-    unsigned int flags)
+    if ( nullptr == _dest ) {
-{
+        return;
-    ai_assert(NULL != _dest);
+    }
 
     // if _dest points to NULL allocate a new scene. Otherwise clear the old and reuse it
-    if (src.empty())
+    if (src.empty()) {
-    {
+        if (*_dest) {
-        if (*_dest)
-        {
             (*_dest)->~aiScene();
             SceneCombiner::CopySceneFlat(_dest,src[0]);
         }
@@ -198,8 +191,7 @@ void SceneCombiner::MergeScenes(aiScene** _dest,std::vector<aiScene*>& src,
 }
 
 // ------------------------------------------------------------------------------------------------
-void SceneCombiner::AttachToGraph (aiNode* attach, std::vector<NodeAttachmentInfo>& srcList)
+void SceneCombiner::AttachToGraph (aiNode* attach, std::vector<NodeAttachmentInfo>& srcList) {
-{
     unsigned int cnt;
     for ( cnt = 0; cnt < attach->mNumChildren; ++cnt ) {
         AttachToGraph( attach->mChildren[ cnt ], srcList );
@@ -239,19 +231,16 @@ void SceneCombiner::AttachToGraph (aiNode* attach, std::vector<NodeAttachmentInf
 }
 
 // ------------------------------------------------------------------------------------------------
-void SceneCombiner::AttachToGraph ( aiScene* master,
+void SceneCombiner::AttachToGraph ( aiScene* master, std::vector<NodeAttachmentInfo>& src) {
-    std::vector<NodeAttachmentInfo>& src)
-{
     ai_assert(NULL != master);
     AttachToGraph(master->mRootNode,src);
 }
 
 // ------------------------------------------------------------------------------------------------
-void SceneCombiner::MergeScenes(aiScene** _dest, aiScene* master,
+void SceneCombiner::MergeScenes(aiScene** _dest, aiScene* master, std::vector<AttachmentInfo>& srcList, unsigned int flags) {
-    std::vector<AttachmentInfo>& srcList,
+    if ( nullptr == _dest ) {
-    unsigned int flags)
+        return;
-{
+    }
-    ai_assert(NULL != _dest);
 
     // if _dest points to NULL allocate a new scene. Otherwise clear the old and reuse it
     if (srcList.empty())    {
@@ -708,7 +697,9 @@ void SceneCombiner::BuildUniqueBoneList(std::list<BoneWithHash>& asBones,
 void SceneCombiner::MergeBones(aiMesh* out,std::vector<aiMesh*>::const_iterator it,
     std::vector<aiMesh*>::const_iterator end)
 {
-    ai_assert(NULL != out && !out->mNumBones);
+    if ( nullptr == out || out->mNumBones == 0 ) {
+        return;
+    }
 
     // find we need to build an unique list of all bones.
     // we work with hashes to make the comparisons MUCH faster,
@@ -762,7 +753,9 @@ void SceneCombiner::MergeMeshes(aiMesh** _out, unsigned int /*flags*/,
     std::vector<aiMesh*>::const_iterator begin,
     std::vector<aiMesh*>::const_iterator end)
 {
-    ai_assert(NULL != _out);
+    if ( nullptr == _out ) {
+        return;
+    }
 
     if (begin == end)   {
         *_out = NULL; // no meshes ...
@@ -903,7 +896,9 @@ void SceneCombiner::MergeMaterials(aiMaterial** dest,
         std::vector<aiMaterial*>::const_iterator begin,
         std::vector<aiMaterial*>::const_iterator end)
 {
-    ai_assert(NULL != dest);
+    if ( nullptr == dest ) {
+        return;
+    }
 
     if (begin == end)   {
         *dest = NULL; // no materials ...
@@ -953,10 +948,9 @@ void SceneCombiner::MergeMaterials(aiMaterial** dest,
 
 // ------------------------------------------------------------------------------------------------
 template <typename Type>
-inline void CopyPtrArray (Type**& dest, const Type* const * src, ai_uint num)
+inline
-{
+void CopyPtrArray (Type**& dest, const Type* const * src, ai_uint num) {
-    if (!num)
+    if (!num) {
-    {
         dest = NULL;
         return;
     }
@@ -968,9 +962,11 @@ inline void CopyPtrArray (Type**& dest, const Type* const * src, ai_uint num)
 
 // ------------------------------------------------------------------------------------------------
 template <typename Type>
-inline void GetArrayCopy (Type*& dest, ai_uint num )
+inline
-{
+void GetArrayCopy(Type*& dest, ai_uint num ) {
-    if (!dest)return;
+    if ( !dest ) {
+        return;
+    }
     Type* old = dest;
 
     dest = new Type[num];
@@ -978,22 +974,27 @@ inline void GetArrayCopy (Type*& dest, ai_uint num )
 }
 
 // ------------------------------------------------------------------------------------------------
-void SceneCombiner::CopySceneFlat(aiScene** _dest,const aiScene* src)
+void SceneCombiner::CopySceneFlat(aiScene** _dest,const aiScene* src) {
-{
+    if ( nullptr == _dest || nullptr == src ) {
+        return;
+    }
+
     // reuse the old scene or allocate a new?
     if (*_dest) {
         (*_dest)->~aiScene();
         new (*_dest) aiScene();
+    } else {
+        *_dest = new aiScene();
     }
-    else *_dest = new aiScene();
 
     ::memcpy(*_dest,src,sizeof(aiScene));
 }
 
 // ------------------------------------------------------------------------------------------------
-void SceneCombiner::CopyScene(aiScene** _dest,const aiScene* src,bool allocate)
+void SceneCombiner::CopyScene(aiScene** _dest,const aiScene* src,bool allocate) {
-{
+    if ( nullptr == _dest || nullptr == src ) {
-    ai_assert(NULL != _dest && NULL != src);
+        return;
+    }
 
     if (allocate) {
         *_dest = new aiScene();
@@ -1044,9 +1045,10 @@ void SceneCombiner::CopyScene(aiScene** _dest,const aiScene* src,bool allocate)
 }
 
 // ------------------------------------------------------------------------------------------------
-void SceneCombiner::Copy     (aiMesh** _dest, const aiMesh* src)
+void SceneCombiner::Copy( aiMesh** _dest, const aiMesh* src ) {
-{
+    if ( nullptr == _dest || nullptr == src ) {
-    ai_assert(NULL != _dest && NULL != src);
+        return;
+    }
 
     aiMesh* dest = *_dest = new aiMesh();
 
@@ -1072,17 +1074,17 @@ void SceneCombiner::Copy     (aiMesh** _dest, const aiMesh* src)
 
     // make a deep copy of all faces
     GetArrayCopy(dest->mFaces,dest->mNumFaces);
-    for (unsigned int i = 0; i < dest->mNumFaces;++i)
+    for (unsigned int i = 0; i < dest->mNumFaces;++i) {
-    {
         aiFace& f = dest->mFaces[i];
         GetArrayCopy(f.mIndices,f.mNumIndices);
     }
 }
 
 // ------------------------------------------------------------------------------------------------
-void SceneCombiner::Copy (aiMaterial** _dest, const aiMaterial* src)
+void SceneCombiner::Copy (aiMaterial** _dest, const aiMaterial* src) {
-{
+    if ( nullptr == _dest || nullptr == src ) {
-    ai_assert(NULL != _dest && NULL != src);
+        return;
+    }
 
     aiMaterial* dest = (aiMaterial*) ( *_dest = new aiMaterial() );
 
@@ -1110,9 +1112,10 @@ void SceneCombiner::Copy (aiMaterial** _dest, const aiMaterial* src)
 }
 
 // ------------------------------------------------------------------------------------------------
-void SceneCombiner::Copy  (aiTexture** _dest, const aiTexture* src)
+void SceneCombiner::Copy(aiTexture** _dest, const aiTexture* src) {
-{
+    if ( nullptr == _dest || nullptr == src ) {
-    ai_assert(NULL != _dest && NULL != src);
+        return;
+    }
 
     aiTexture* dest = *_dest = new aiTexture();
 
@@ -1139,10 +1142,10 @@ void SceneCombiner::Copy  (aiTexture** _dest, const aiTexture* src)
 }
 
 // ------------------------------------------------------------------------------------------------
-void SceneCombiner::Copy( aiAnimation** _dest, const aiAnimation* src )
+void SceneCombiner::Copy( aiAnimation** _dest, const aiAnimation* src ) {
-{
+    if ( nullptr == _dest || nullptr == src ) {
-    ai_assert( NULL != _dest );
+        return;
-    ai_assert( NULL != src );
+    }
 
     aiAnimation* dest = *_dest = new aiAnimation();
 
@@ -1154,9 +1157,10 @@ void SceneCombiner::Copy( aiAnimation** _dest, const aiAnimation* src )
 }
 
 // ------------------------------------------------------------------------------------------------
-void SceneCombiner::Copy     (aiNodeAnim** _dest, const aiNodeAnim* src)
+void SceneCombiner::Copy(aiNodeAnim** _dest, const aiNodeAnim* src) {
-{
+    if ( nullptr == _dest || nullptr == src ) {
-    ai_assert(NULL != _dest && NULL != src);
+        return;
+    }
 
     aiNodeAnim* dest = *_dest = new aiNodeAnim();
 
@@ -1170,9 +1174,10 @@ void SceneCombiner::Copy     (aiNodeAnim** _dest, const aiNodeAnim* src)
 }
 
 // ------------------------------------------------------------------------------------------------
-void SceneCombiner::Copy   (aiCamera** _dest,const  aiCamera* src)
+void SceneCombiner::Copy( aiCamera** _dest,const  aiCamera* src) {
-{
+    if ( nullptr == _dest || nullptr == src ) {
-    ai_assert(NULL != _dest && NULL != src);
+        return;
+    }
 
     aiCamera* dest = *_dest = new aiCamera();
 
@@ -1181,9 +1186,10 @@ void SceneCombiner::Copy   (aiCamera** _dest,const  aiCamera* src)
 }
 
 // ------------------------------------------------------------------------------------------------
-void SceneCombiner::Copy   (aiLight** _dest, const aiLight* src)
+void SceneCombiner::Copy(aiLight** _dest, const aiLight* src) {
-{
+    if ( nullptr == _dest || nullptr == src ) {
-    ai_assert(NULL != _dest && NULL != src);
+        return;
+    }
 
     aiLight* dest = *_dest = new aiLight();
 
@@ -1192,9 +1198,10 @@ void SceneCombiner::Copy   (aiLight** _dest, const aiLight* src)
 }
 
 // ------------------------------------------------------------------------------------------------
-void SceneCombiner::Copy     (aiBone** _dest, const aiBone* src)
+void SceneCombiner::Copy(aiBone** _dest, const aiBone* src) {
-{
+    if ( nullptr == _dest || nullptr == src ) {
-    ai_assert(NULL != _dest && NULL != src);
+        return;
+    }
 
     aiBone* dest = *_dest = new aiBone();
 
@@ -1230,10 +1237,14 @@ void SceneCombiner::Copy     (aiNode** _dest, const aiNode* src)
 }
 
 // ------------------------------------------------------------------------------------------------
-void SceneCombiner::Copy(aiMetadata** _dest, const aiMetadata* src)
+void SceneCombiner::Copy(aiMetadata** _dest, const aiMetadata* src) {
-{
+    if ( nullptr == _dest || nullptr == src ) {
-    ai_assert( NULL != _dest );
+        return;
-    ai_assert( NULL != src);
+    }
+
+    if ( 0 == src->mNumProperties ) {
+        return;
+    }
 
     aiMetadata* dest = *_dest = aiMetadata::Alloc( src->mNumProperties );
     std::copy(src->mKeys, src->mKeys + src->mNumProperties, dest->mKeys);
@@ -1271,4 +1282,5 @@ void SceneCombiner::Copy(aiMetadata** _dest, const aiMetadata* src)
     }
 }
 
-}
+} // Namespace Assimp
+

include/assimp/SceneCombiner.h

@@ -197,13 +197,17 @@ struct SceneHelper
  * The class is currently being used by various postprocessing steps
  * and loaders (ie. LWS).
  */
-class ASSIMP_API SceneCombiner
+class ASSIMP_API SceneCombiner {
-{
     // class cannot be instanced
-    SceneCombiner() {}
+    SceneCombiner() {
+        // empty
+    }
+
+    ~SceneCombiner() {
+        // empty
+    }
 
 public:
-
     // -------------------------------------------------------------------
     /** Merges two or more scenes.
      *

include/assimp/metadata.h

@@ -187,7 +187,7 @@ struct aiMetadata {
     static inline
     aiMetadata *Alloc( unsigned int numProperties ) {
         if ( 0 == numProperties ) {
-            return NULL;
+            return nullptr;
         }
 
         aiMetadata *data = new aiMetadata;

test/unit/utSceneCombiner.cpp

@@ -71,3 +71,8 @@ TEST_F( utSceneCombiner, MergeMeshes_ValidNames_Test ) {
     std::string outName = out->mName.C_Str();
     EXPECT_EQ( "mesh_1.mesh_2.mesh_3", outName );
 }
+
+TEST_F( utSceneCombiner, CopySceneWithNullptr_NoException ) {
+    EXPECT_NO_THROW( SceneCombiner::CopyScene( nullptr, nullptr ) );
+    EXPECT_NO_THROW( SceneCombiner::CopySceneFlat( nullptr, nullptr ) );
+}