From bcdc79ba73e325510c451d343bec10230cf552f8 Mon Sep 17 00:00:00 2001 From: Kim Kulling Date: Sun, 4 Sep 2016 20:22:04 +0200 Subject: [PATCH 1/6] Fix invalid release of mat + mesh. --- code/IRRMeshLoader.cpp | 49 ++++++++++++++++++++++++------------------ 1 file changed, 28 insertions(+), 21 deletions(-) diff --git a/code/IRRMeshLoader.cpp b/code/IRRMeshLoader.cpp index 0a2a54e84..567c98188 100644 --- a/code/IRRMeshLoader.cpp +++ b/code/IRRMeshLoader.cpp @@ -116,6 +116,16 @@ const aiImporterDesc* IRRMeshImporter::GetInfo () const return &desc; } +static void releaseMaterial( aiMaterial **mat ) { + delete mat; + *mat = nullptr; +} + +static void releaseMesh( aiMesh **mesh ) { + delete mesh; + *mesh = nullptr; +} + // ------------------------------------------------------------------------------------------------ // Imports the given file into the given scene structure. void IRRMeshImporter::InternReadFile( const std::string& pFile, @@ -135,7 +145,7 @@ void IRRMeshImporter::InternReadFile( const std::string& pFile, std::vector materials; std::vector meshes; materials.reserve (5); - meshes.reserve (5); + meshes.reserve(5); // temporary data - current mesh buffer aiMaterial* curMat = NULL; @@ -159,11 +169,10 @@ void IRRMeshImporter::InternReadFile( const std::string& pFile, if (!ASSIMP_stricmp(reader->getNodeName(),"buffer") && (curMat || curMesh)) { // end of previous buffer. A material and a mesh should be there if ( !curMat || !curMesh) { - DefaultLogger::get()->error("IRRMESH: A buffer must contain a mesh and a material"); - delete curMat; - delete curMesh; - } - else { + DefaultLogger::get()->error("IRRMESH: A buffer must contain a mesh and a material"); + releaseMaterial( &curMat ); + releaseMesh( &curMesh ); + } else { materials.push_back(curMat); meshes.push_back(curMesh); } @@ -183,7 +192,7 @@ void IRRMeshImporter::InternReadFile( const std::string& pFile, if (!ASSIMP_stricmp(reader->getNodeName(),"material")) { if (curMat) { DefaultLogger::get()->warn("IRRMESH: Only one material description per buffer, please"); - delete curMat;curMat = NULL; + releaseMaterial( &curMat ); } curMat = ParseMaterial(curMatFlags); } @@ -195,17 +204,16 @@ void IRRMeshImporter::InternReadFile( const std::string& pFile, // This is possible ... remove the mesh from the list and skip further reading DefaultLogger::get()->warn("IRRMESH: Found mesh with zero vertices"); - delete curMat;curMat = NULL; - - curMesh = NULL; + releaseMaterial( &curMat ); + releaseMesh( &curMesh ); textMeaning = 0; continue; } - curVertices.reserve (num); - curNormals.reserve (num); - curColors.reserve (num); - curUVs.reserve (num); + curVertices.reserve(num); + curNormals.reserve(num); + curColors.reserve(num); + curUVs.reserve(num); // Determine the file format const char* t = reader->getAttributeValueSafe("type"); @@ -240,7 +248,7 @@ void IRRMeshImporter::InternReadFile( const std::string& pFile, vertexFormat = 2; } else if (ASSIMP_stricmp("standard", t)) { - delete curMat; + releaseMaterial( &curMat ); DefaultLogger::get()->warn("IRRMESH: Unknown vertex format"); } else vertexFormat = 0; @@ -248,7 +256,7 @@ void IRRMeshImporter::InternReadFile( const std::string& pFile, } else if (!ASSIMP_stricmp(reader->getNodeName(),"indices")) { if (curVertices.empty() && curMat) { - delete curMat; + releaseMaterial( &curMat ); throw DeadlyImportError("IRRMESH: indices must come after vertices"); } @@ -264,10 +272,10 @@ void IRRMeshImporter::InternReadFile( const std::string& pFile, DefaultLogger::get()->warn("IRRMESH: Found mesh with zero indices"); // mesh - away - delete curMesh; curMesh = NULL; + releaseMesh( &curMesh ); // material - away - delete curMat;curMat = NULL; + releaseMaterial( &curMat ); textMeaning = 0; continue; @@ -469,7 +477,6 @@ void IRRMeshImporter::InternReadFile( const std::string& pFile, break; default: - // GCC complains here ... break; @@ -480,8 +487,8 @@ void IRRMeshImporter::InternReadFile( const std::string& pFile, if (curMat || curMesh) { if ( !curMat || !curMesh) { DefaultLogger::get()->error("IRRMESH: A buffer must contain a mesh and a material"); - delete curMat; - delete curMesh; + releaseMaterial( &curMat ); + releaseMesh( &curMesh ); } else { materials.push_back(curMat); From 9d4d2b2a1c549d4e958c8e0637d483a7f4e948d4 Mon Sep 17 00:00:00 2001 From: Kim Kulling Date: Sun, 4 Sep 2016 20:35:07 +0200 Subject: [PATCH 2/6] ComputeUVMappingprocess: add missing initialization for scalar value. --- code/ComputeUVMappingProcess.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/code/ComputeUVMappingProcess.cpp b/code/ComputeUVMappingProcess.cpp index 33a6f4edb..ff5142fc0 100644 --- a/code/ComputeUVMappingProcess.cpp +++ b/code/ComputeUVMappingProcess.cpp @@ -437,7 +437,7 @@ void ComputeUVMappingProcess::Execute( aiScene* pScene) } } - unsigned int idx; + unsigned int idx( 99999999 ); // Check whether we have this mapping mode already std::list::iterator it = std::find (mappingStack.begin(),mappingStack.end(), info); From cc860ede66422c84d4a2211d52ad971011c1b0c5 Mon Sep 17 00:00:00 2001 From: Kim Kulling Date: Sun, 4 Sep 2016 20:40:34 +0200 Subject: [PATCH 3/6] Fix coverity findings: fix usage after free. --- code/SMDLoader.cpp | 31 ++++++++++++++++--------------- code/SMDLoader.h | 2 +- 2 files changed, 17 insertions(+), 16 deletions(-) diff --git a/code/SMDLoader.cpp b/code/SMDLoader.cpp index 7aef7bc56..2db9f15e7 100644 --- a/code/SMDLoader.cpp +++ b/code/SMDLoader.cpp @@ -74,20 +74,22 @@ static const aiImporterDesc desc = { // ------------------------------------------------------------------------------------------------ // Constructor to be privately used by Importer SMDImporter::SMDImporter() - : configFrameID(), - mBuffer(), - pScene(), - iFileSize(), - iSmallestFrame(), - dLengthOfAnim(), - bHasUVs(), - iLineNumber() -{} +: configFrameID(), +mBuffer(), +pScene( nullptr ), +iFileSize( 0 ), +iSmallestFrame( -1 ), +dLengthOfAnim( 0.0 ), +bHasUVs(false ), +iLineNumber(-1) { + // empty +} // ------------------------------------------------------------------------------------------------ // Destructor, private as well -SMDImporter::~SMDImporter() -{} +SMDImporter::~SMDImporter() { + // empty +} // ------------------------------------------------------------------------------------------------ // Returns whether the class can handle the format of the given file. @@ -133,9 +135,8 @@ void SMDImporter::InternReadFile( const std::string& pFile, aiScene* pScene, IOS // Allocate storage and copy the contents of the file to a memory buffer this->pScene = pScene; - std::vector buff(iFileSize+1); - TextFileToBuffer(file.get(),buff); - mBuffer = &buff[0]; + mBuffer.resize( iFileSize + 1 ); + TextFileToBuffer(file.get(), mBuffer ); iSmallestFrame = (1 << 31); bHasUVs = true; @@ -694,7 +695,7 @@ void SMDImporter::CreateOutputMaterials() // Parse the file void SMDImporter::ParseFile() { - const char* szCurrent = mBuffer; + const char* szCurrent = &mBuffer[0]; // read line per line ... for ( ;; ) diff --git a/code/SMDLoader.h b/code/SMDLoader.h index 0b069cc25..96ea1d195 100644 --- a/code/SMDLoader.h +++ b/code/SMDLoader.h @@ -372,7 +372,7 @@ private: unsigned int configFrameID; /** Buffer to hold the loaded file */ - const char* mBuffer; + std::vector mBuffer; /** Output scene to be filled */ From 8f808f0e2e2a49a50ef0e31645853364133784ce Mon Sep 17 00:00:00 2001 From: Kim Kulling Date: Mon, 5 Sep 2016 00:27:59 +0200 Subject: [PATCH 4/6] Doc: fix redundant doc. --- doc/dox_cmd.h | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/doc/dox_cmd.h b/doc/dox_cmd.h index e740886a1..78e755d56 100644 --- a/doc/dox_cmd.h +++ b/doc/dox_cmd.h @@ -494,21 +494,11 @@ more information can be found in the aiPostProcess.h header. --transform-uv-coords Will transform uv-coordinates if possible. - - -guv - --gen-uvcoords - Will generate uv-coordinates for textures if possible. - -fid --find-invalid-data Will look for invalid data in the imported model structure. - - -fixn - --fix normals - Imported normal vector will be fixed. - -db --debone From 9e19b5103cf1666c48b11c1d5080bc220f856d35 Mon Sep 17 00:00:00 2001 From: Kim Kulling Date: Mon, 5 Sep 2016 10:48:30 +0200 Subject: [PATCH 5/6] IrrImporter: Fix release functions. --- code/IRRMeshLoader.cpp | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/code/IRRMeshLoader.cpp b/code/IRRMeshLoader.cpp index 567c98188..a06449dca 100644 --- a/code/IRRMeshLoader.cpp +++ b/code/IRRMeshLoader.cpp @@ -116,14 +116,14 @@ const aiImporterDesc* IRRMeshImporter::GetInfo () const return &desc; } -static void releaseMaterial( aiMaterial **mat ) { +static void releaseMaterial( aiMaterial *mat ) { delete mat; - *mat = nullptr; + mat = nullptr; } -static void releaseMesh( aiMesh **mesh ) { +static void releaseMesh( aiMesh *mesh ) { delete mesh; - *mesh = nullptr; + mesh = nullptr; } // ------------------------------------------------------------------------------------------------ @@ -170,8 +170,8 @@ void IRRMeshImporter::InternReadFile( const std::string& pFile, // end of previous buffer. A material and a mesh should be there if ( !curMat || !curMesh) { DefaultLogger::get()->error("IRRMESH: A buffer must contain a mesh and a material"); - releaseMaterial( &curMat ); - releaseMesh( &curMesh ); + releaseMaterial( curMat ); + releaseMesh( curMesh ); } else { materials.push_back(curMat); meshes.push_back(curMesh); @@ -192,7 +192,7 @@ void IRRMeshImporter::InternReadFile( const std::string& pFile, if (!ASSIMP_stricmp(reader->getNodeName(),"material")) { if (curMat) { DefaultLogger::get()->warn("IRRMESH: Only one material description per buffer, please"); - releaseMaterial( &curMat ); + releaseMaterial( curMat ); } curMat = ParseMaterial(curMatFlags); } @@ -204,8 +204,8 @@ void IRRMeshImporter::InternReadFile( const std::string& pFile, // This is possible ... remove the mesh from the list and skip further reading DefaultLogger::get()->warn("IRRMESH: Found mesh with zero vertices"); - releaseMaterial( &curMat ); - releaseMesh( &curMesh ); + releaseMaterial( curMat ); + releaseMesh( curMesh ); textMeaning = 0; continue; } @@ -248,7 +248,7 @@ void IRRMeshImporter::InternReadFile( const std::string& pFile, vertexFormat = 2; } else if (ASSIMP_stricmp("standard", t)) { - releaseMaterial( &curMat ); + releaseMaterial( curMat ); DefaultLogger::get()->warn("IRRMESH: Unknown vertex format"); } else vertexFormat = 0; @@ -256,7 +256,7 @@ void IRRMeshImporter::InternReadFile( const std::string& pFile, } else if (!ASSIMP_stricmp(reader->getNodeName(),"indices")) { if (curVertices.empty() && curMat) { - releaseMaterial( &curMat ); + releaseMaterial( curMat ); throw DeadlyImportError("IRRMESH: indices must come after vertices"); } @@ -272,10 +272,10 @@ void IRRMeshImporter::InternReadFile( const std::string& pFile, DefaultLogger::get()->warn("IRRMESH: Found mesh with zero indices"); // mesh - away - releaseMesh( &curMesh ); + releaseMesh( curMesh ); // material - away - releaseMaterial( &curMat ); + releaseMaterial( curMat ); textMeaning = 0; continue; @@ -487,8 +487,8 @@ void IRRMeshImporter::InternReadFile( const std::string& pFile, if (curMat || curMesh) { if ( !curMat || !curMesh) { DefaultLogger::get()->error("IRRMESH: A buffer must contain a mesh and a material"); - releaseMaterial( &curMat ); - releaseMesh( &curMesh ); + releaseMaterial( curMat ); + releaseMesh( curMesh ); } else { materials.push_back(curMat); From 0f2cea7ba6d046a02bf3bf8813bb1dfbe73fc253 Mon Sep 17 00:00:00 2001 From: Kim Kulling Date: Tue, 6 Sep 2016 10:42:02 +0200 Subject: [PATCH 6/6] Blender: revert fix for suspicious crash in blender on windows. --- code/BlenderIntermediate.h | 4 ++-- code/ObjFileImporter.cpp | 43 +++++++++++++++++++------------------- code/ObjFileImporter.h | 12 +++-------- 3 files changed, 26 insertions(+), 33 deletions(-) diff --git a/code/BlenderIntermediate.h b/code/BlenderIntermediate.h index 83b10e870..0290185de 100644 --- a/code/BlenderIntermediate.h +++ b/code/BlenderIntermediate.h @@ -123,7 +123,7 @@ namespace Blender { struct ObjectCompare { bool operator() (const Object* left, const Object* right) const { - return ::strncmp(left->id.name, right->id.name, strlen( left->id.name ) ) == -1; + return ::strncmp(left->id.name, right->id.name, strlen( left->id.name ) ) == 0; } }; @@ -144,7 +144,7 @@ namespace Blender { struct ObjectCompare { bool operator() (const Object* left, const Object* right) const { - return ::strncmp( left->id.name, right->id.name, strlen( left->id.name ) ) == -1; + return ::strncmp( left->id.name, right->id.name, strlen( left->id.name ) ) == 0; } }; diff --git a/code/ObjFileImporter.cpp b/code/ObjFileImporter.cpp index 2d6c859bf..a70d69b4a 100644 --- a/code/ObjFileImporter.cpp +++ b/code/ObjFileImporter.cpp @@ -387,8 +387,7 @@ void ObjFileImporter::createVertexArray(const ObjFile::Model* pModel, const ObjFile::Object* pCurrentObject, unsigned int uiMeshIndex, aiMesh* pMesh, - unsigned int numIndices) -{ + unsigned int numIndices) { // Checking preconditions ai_assert( NULL != pCurrentObject ); @@ -398,8 +397,9 @@ void ObjFileImporter::createVertexArray(const ObjFile::Model* pModel, // Get current mesh ObjFile::Mesh *pObjMesh = pModel->m_Meshes[ uiMeshIndex ]; - if ( NULL == pObjMesh || pObjMesh->m_uiNumIndices < 1) + if ( NULL == pObjMesh || pObjMesh->m_uiNumIndices < 1 ) { return; + } // Copy vertices of this mesh instance pMesh->mNumVertices = numIndices; @@ -427,27 +427,25 @@ void ObjFileImporter::createVertexArray(const ObjFile::Model* pModel, // Copy vertices, normals and textures into aiMesh instance unsigned int newIndex = 0, outIndex = 0; - for ( size_t index=0; index < pObjMesh->m_Faces.size(); index++ ) - { + for ( size_t index=0; index < pObjMesh->m_Faces.size(); index++ ) { // Get source face ObjFile::Face *pSourceFace = pObjMesh->m_Faces[ index ]; // Copy all index arrays - for ( size_t vertexIndex = 0, outVertexIndex = 0; vertexIndex < pSourceFace->m_pVertices->size(); vertexIndex++ ) - { + for ( size_t vertexIndex = 0, outVertexIndex = 0; vertexIndex < pSourceFace->m_pVertices->size(); vertexIndex++ ) { const unsigned int vertex = pSourceFace->m_pVertices->at( vertexIndex ); - if ( vertex >= pModel->m_Vertices.size() ) + if ( vertex >= pModel->m_Vertices.size() ) { throw DeadlyImportError( "OBJ: vertex index out of range" ); + } pMesh->mVertices[ newIndex ] = pModel->m_Vertices[ vertex ]; // Copy all normals - if ( !pModel->m_Normals.empty() && vertexIndex < pSourceFace->m_pNormals->size()) - { + if ( !pModel->m_Normals.empty() && vertexIndex < pSourceFace->m_pNormals->size()) { const unsigned int normal = pSourceFace->m_pNormals->at( vertexIndex ); - if ( normal >= pModel->m_Normals.size() ) - throw DeadlyImportError("OBJ: vertex normal index out of range"); - + if ( normal >= pModel->m_Normals.size() ) { + throw DeadlyImportError( "OBJ: vertex normal index out of range" ); + } pMesh->mNormals[ newIndex ] = pModel->m_Normals[ normal ]; } @@ -544,20 +542,21 @@ void ObjFileImporter::countObjects(const std::vector &rObjects // ------------------------------------------------------------------------------------------------ // Add clamp mode property to material if necessary -void ObjFileImporter::addTextureMappingModeProperty(aiMaterial* mat, aiTextureType type, int clampMode) -{ - ai_assert( NULL != mat); - mat->AddProperty(&clampMode, 1, AI_MATKEY_MAPPINGMODE_U(type, 0)); - mat->AddProperty(&clampMode, 1, AI_MATKEY_MAPPINGMODE_V(type, 0)); +void ObjFileImporter::addTextureMappingModeProperty( aiMaterial* mat, aiTextureType type, int clampMode) { + if ( nullptr == mat ) { + return; + } + + mat->AddProperty( &clampMode, 1, AI_MATKEY_MAPPINGMODE_U( type, 0 ) ); + mat->AddProperty( &clampMode, 1, AI_MATKEY_MAPPINGMODE_V( type, 0 ) ); } // ------------------------------------------------------------------------------------------------ // Creates the material -void ObjFileImporter::createMaterials(const ObjFile::Model* pModel, aiScene* pScene ) -{ - ai_assert( NULL != pScene ); - if ( NULL == pScene ) +void ObjFileImporter::createMaterials(const ObjFile::Model* pModel, aiScene* pScene ) { + if ( NULL == pScene ) { return; + } const unsigned int numMaterials = (unsigned int) pModel->m_MaterialLib.size(); pScene->mNumMaterials = 0; diff --git a/code/ObjFileImporter.h b/code/ObjFileImporter.h index b03d00b9b..8c482017b 100644 --- a/code/ObjFileImporter.h +++ b/code/ObjFileImporter.h @@ -37,8 +37,6 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. ---------------------------------------------------------------------- */ - - #ifndef OBJ_FILE_IMPORTER_H_INC #define OBJ_FILE_IMPORTER_H_INC @@ -49,11 +47,9 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. struct aiMesh; struct aiNode; -namespace Assimp -{ +namespace Assimp { -namespace ObjFile -{ +namespace ObjFile { struct Object; struct Model; } @@ -62,8 +58,7 @@ struct Model; /// \class ObjFileImporter /// \brief Imports a waveform obj file // ------------------------------------------------------------------------------------------------ -class ObjFileImporter : public BaseImporter -{ +class ObjFileImporter : public BaseImporter { public: /// \brief Default constructor ObjFileImporter(); @@ -77,7 +72,6 @@ public: bool CanRead( const std::string& pFile, IOSystem* pIOHandler, bool checkSig) const; private: - //! \brief Appends the supported extension. const aiImporterDesc* GetInfo () const;