From 495ae70cc5423b1cda7c5999c4b44cf9fb9de0cd Mon Sep 17 00:00:00 2001 From: Kim Kulling Date: Tue, 6 Feb 2018 19:21:56 +0100 Subject: [PATCH] XFileParser: release x-file-based scene when throwing an exception. --- code/3DSLoader.cpp | 16 +- code/PlyLoader.cpp | 352 +++++++++++++++----------------- code/PlyLoader.h | 1 - code/PlyParser.h | 43 ++-- code/XFileParser.cpp | 42 ++-- test/unit/utPLYImportExport.cpp | 25 +++ 6 files changed, 243 insertions(+), 236 deletions(-) diff --git a/code/3DSLoader.cpp b/code/3DSLoader.cpp index 7a5e2b6e5..9d61e125c 100644 --- a/code/3DSLoader.cpp +++ b/code/3DSLoader.cpp @@ -113,22 +113,24 @@ Discreet3DSImporter::Discreet3DSImporter() , mScene() , mMasterScale() , bHasBG() -, bIsPrj() -{} +, bIsPrj() { + // empty +} // ------------------------------------------------------------------------------------------------ // Destructor, private as well -Discreet3DSImporter::~Discreet3DSImporter() -{} +Discreet3DSImporter::~Discreet3DSImporter() { + // empty +} // ------------------------------------------------------------------------------------------------ // Returns whether the class can handle the format of the given file. -bool Discreet3DSImporter::CanRead( const std::string& pFile, IOSystem* pIOHandler, bool checkSig) const -{ +bool Discreet3DSImporter::CanRead( const std::string& pFile, IOSystem* pIOHandler, bool checkSig) const { std::string extension = GetExtension(pFile); if(extension == "3ds" || extension == "prj" ) { return true; } + if (!extension.length() || checkSig) { uint16_t token[3]; token[0] = 0x4d4d; @@ -210,7 +212,7 @@ void Discreet3DSImporter::InternReadFile( const std::string& pFile, ConvertScene(pScene); // Generate the node graph for the scene. This is a little bit - // tricky since we'll need to split some meshes into submeshes + // tricky since we'll need to split some meshes into sub-meshes GenerateNodeGraph(pScene); // Now apply the master scaling factor to the scene diff --git a/code/PlyLoader.cpp b/code/PlyLoader.cpp index b6ae368df..5f4e556c1 100644 --- a/code/PlyLoader.cpp +++ b/code/PlyLoader.cpp @@ -58,16 +58,16 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. using namespace Assimp; static const aiImporterDesc desc = { - "Stanford Polygon Library (PLY) Importer", - "", - "", - "", - aiImporterFlags_SupportBinaryFlavour | aiImporterFlags_SupportTextFlavour, - 0, - 0, - 0, - 0, - "ply" + "Stanford Polygon Library (PLY) Importer", + "", + "", + "", + aiImporterFlags_SupportBinaryFlavour | aiImporterFlags_SupportTextFlavour, + 0, + 0, + 0, + 0, + "ply" }; @@ -92,229 +92,209 @@ namespace // ------------------------------------------------------------------------------------------------ // Constructor to be privately used by Importer PLYImporter::PLYImporter() - : mBuffer(nullptr) - , pcDOM(nullptr) - , mGeneratedMesh(nullptr){ - // empty +: mBuffer(nullptr) +, pcDOM(nullptr) +, mGeneratedMesh(nullptr) { + // empty } // ------------------------------------------------------------------------------------------------ // Destructor, private as well PLYImporter::~PLYImporter() { - // empty + // empty } // ------------------------------------------------------------------------------------------------ // Returns whether the class can handle the format of the given file. -bool PLYImporter::CanRead(const std::string& pFile, IOSystem* pIOHandler, bool checkSig) const -{ - const std::string extension = GetExtension(pFile); +bool PLYImporter::CanRead(const std::string& pFile, IOSystem* pIOHandler, bool checkSig) const { + const std::string extension = GetExtension(pFile); - if (extension == "ply") - return true; - else if (!extension.length() || checkSig) - { - if (!pIOHandler)return true; - const char* tokens[] = { "ply" }; - return SearchFileHeaderForToken(pIOHandler, pFile, tokens, 1); - } - return false; + if ( extension == "ply" ) { + return true; + } else if (!extension.length() || checkSig) { + if ( !pIOHandler ) { + return true; + } + static const char* tokens[] = { "ply" }; + return SearchFileHeaderForToken(pIOHandler, pFile, tokens, 1); + } + + return false; } // ------------------------------------------------------------------------------------------------ -const aiImporterDesc* PLYImporter::GetInfo() const -{ - return &desc; +const aiImporterDesc* PLYImporter::GetInfo() const { + return &desc; } // ------------------------------------------------------------------------------------------------ static bool isBigEndian(const char* szMe) { - ai_assert(NULL != szMe); + ai_assert(NULL != szMe); - // binary_little_endian - // binary_big_endian - bool isBigEndian(false); + // binary_little_endian + // binary_big_endian + bool isBigEndian(false); #if (defined AI_BUILD_BIG_ENDIAN) - if ( 'l' == *szMe || 'L' == *szMe ) { - isBigEndian = true; - } + if ( 'l' == *szMe || 'L' == *szMe ) { + isBigEndian = true; + } #else - if ('b' == *szMe || 'B' == *szMe) { - isBigEndian = true; - } + if ('b' == *szMe || 'B' == *szMe) { + isBigEndian = true; + } #endif // ! AI_BUILD_BIG_ENDIAN - return isBigEndian; + return isBigEndian; } // ------------------------------------------------------------------------------------------------ // Imports the given file into the given scene structure. -void PLYImporter::InternReadFile(const std::string& pFile, - aiScene* pScene, IOSystem* pIOHandler) -{ - static const std::string mode = "rb"; - std::unique_ptr fileStream(pIOHandler->Open(pFile, mode)); - if (!fileStream.get()) { - throw DeadlyImportError("Failed to open file " + pFile + "."); - } +void PLYImporter::InternReadFile(const std::string& pFile, aiScene* pScene, IOSystem* pIOHandler) { + static const std::string mode = "rb"; + std::unique_ptr fileStream(pIOHandler->Open(pFile, mode)); + if (!fileStream.get()) { + throw DeadlyImportError("Failed to open file " + pFile + "."); + } - // Get the file-size - size_t fileSize = fileStream->FileSize(); - if ( 0 == fileSize ) { - throw DeadlyImportError("File " + pFile + " is empty."); - } + // Get the file-size + const size_t fileSize( fileStream->FileSize() ); + if ( 0 == fileSize ) { + throw DeadlyImportError("File " + pFile + " is empty."); + } - IOStreamBuffer streamedBuffer(1024 * 1024); - streamedBuffer.open(fileStream.get()); + IOStreamBuffer streamedBuffer(1024 * 1024); + streamedBuffer.open(fileStream.get()); - // the beginning of the file must be PLY - magic, magic - std::vector headerCheck; - streamedBuffer.getNextLine(headerCheck); + // the beginning of the file must be PLY - magic, magic + std::vector headerCheck; + streamedBuffer.getNextLine(headerCheck); - if ((headerCheck.size() < 3) || - (headerCheck[0] != 'P' && headerCheck[0] != 'p') || - (headerCheck[1] != 'L' && headerCheck[1] != 'l') || - (headerCheck[2] != 'Y' && headerCheck[2] != 'y') ) - { - streamedBuffer.close(); - throw DeadlyImportError("Invalid .ply file: Magic number \'ply\' is no there"); - } + if ((headerCheck.size() < 3) || + (headerCheck[0] != 'P' && headerCheck[0] != 'p') || + (headerCheck[1] != 'L' && headerCheck[1] != 'l') || + (headerCheck[2] != 'Y' && headerCheck[2] != 'y') ) { + streamedBuffer.close(); + throw DeadlyImportError("Invalid .ply file: Magic number \'ply\' is no there"); + } - std::vector mBuffer2; - streamedBuffer.getNextLine(mBuffer2); - mBuffer = (unsigned char*)&mBuffer2[0]; + std::vector mBuffer2; + streamedBuffer.getNextLine(mBuffer2); + mBuffer = (unsigned char*)&mBuffer2[0]; - char* szMe = (char*)&this->mBuffer[0]; - SkipSpacesAndLineEnd(szMe, (const char**)&szMe); + char* szMe = (char*)&this->mBuffer[0]; + SkipSpacesAndLineEnd(szMe, (const char**)&szMe); - // determine the format of the file data and construct the aimesh - PLY::DOM sPlyDom; - this->pcDOM = &sPlyDom; + // determine the format of the file data and construct the aimesh + PLY::DOM sPlyDom; + this->pcDOM = &sPlyDom; - if (TokenMatch(szMe, "format", 6)) { - if (TokenMatch(szMe, "ascii", 5)) { - SkipLine(szMe, (const char**)&szMe); - if (!PLY::DOM::ParseInstance(streamedBuffer, &sPlyDom, this)) - { - if (mGeneratedMesh != NULL) - { - delete(mGeneratedMesh); - mGeneratedMesh = nullptr; + if (TokenMatch(szMe, "format", 6)) { + if (TokenMatch(szMe, "ascii", 5)) { + SkipLine(szMe, (const char**)&szMe); + if (!PLY::DOM::ParseInstance(streamedBuffer, &sPlyDom, this)) { + if (mGeneratedMesh != NULL) { + delete(mGeneratedMesh); + mGeneratedMesh = nullptr; + } + + streamedBuffer.close(); + throw DeadlyImportError("Invalid .ply file: Unable to build DOM (#1)"); + } + } else if (!::strncmp(szMe, "binary_", 7)) { + szMe += 7; + const bool bIsBE(isBigEndian(szMe)); + + // skip the line, parse the rest of the header and build the DOM + if (!PLY::DOM::ParseInstanceBinary(streamedBuffer, &sPlyDom, this, bIsBE)) { + if (mGeneratedMesh != NULL) { + delete(mGeneratedMesh); + mGeneratedMesh = nullptr; + } + + streamedBuffer.close(); + throw DeadlyImportError("Invalid .ply file: Unable to build DOM (#2)"); + } + } else { + if (mGeneratedMesh != NULL) { + delete(mGeneratedMesh); + mGeneratedMesh = nullptr; + } + + streamedBuffer.close(); + throw DeadlyImportError("Invalid .ply file: Unknown file format"); + } + } else { + AI_DEBUG_INVALIDATE_PTR(this->mBuffer); + if (mGeneratedMesh != NULL) { + delete(mGeneratedMesh); + mGeneratedMesh = nullptr; } streamedBuffer.close(); - throw DeadlyImportError("Invalid .ply file: Unable to build DOM (#1)"); - } + throw DeadlyImportError("Invalid .ply file: Missing format specification"); } - else if (!::strncmp(szMe, "binary_", 7)) - { - szMe += 7; - const bool bIsBE(isBigEndian(szMe)); - // skip the line, parse the rest of the header and build the DOM - if (!PLY::DOM::ParseInstanceBinary(streamedBuffer, &sPlyDom, this, bIsBE)) - { - if (mGeneratedMesh != NULL) - { - delete(mGeneratedMesh); - mGeneratedMesh = nullptr; + //free the file buffer + streamedBuffer.close(); + + if (mGeneratedMesh == NULL) { + throw DeadlyImportError("Invalid .ply file: Unable to extract mesh data "); + } + + // if no face list is existing we assume that the vertex + // list is containing a list of points + bool pointsOnly = mGeneratedMesh->mFaces == NULL ? true : false; + if (pointsOnly) { + if (mGeneratedMesh->mNumVertices < 3) { + if (mGeneratedMesh != NULL) { + delete(mGeneratedMesh); + mGeneratedMesh = nullptr; + } + + streamedBuffer.close(); + throw DeadlyImportError("Invalid .ply file: Not enough " + "vertices to build a proper face list. "); } - streamedBuffer.close(); - throw DeadlyImportError("Invalid .ply file: Unable to build DOM (#2)"); - } - } - else - { - if (mGeneratedMesh != NULL) - { - delete(mGeneratedMesh); - mGeneratedMesh = nullptr; - } + const unsigned int iNum = (unsigned int)mGeneratedMesh->mNumVertices / 3; + mGeneratedMesh->mNumFaces = iNum; + mGeneratedMesh->mFaces = new aiFace[mGeneratedMesh->mNumFaces]; - streamedBuffer.close(); - throw DeadlyImportError("Invalid .ply file: Unknown file format"); - } - } - else - { - AI_DEBUG_INVALIDATE_PTR(this->mBuffer); - if (mGeneratedMesh != NULL) - { - delete(mGeneratedMesh); - mGeneratedMesh = nullptr; + for (unsigned int i = 0; i < iNum; ++i) { + mGeneratedMesh->mFaces[i].mNumIndices = 3; + mGeneratedMesh->mFaces[i].mIndices = new unsigned int[3]; + mGeneratedMesh->mFaces[i].mIndices[0] = (i * 3); + mGeneratedMesh->mFaces[i].mIndices[1] = (i * 3) + 1; + mGeneratedMesh->mFaces[i].mIndices[2] = (i * 3) + 2; + } } - streamedBuffer.close(); - throw DeadlyImportError("Invalid .ply file: Missing format specification"); - } + // now load a list of all materials + std::vector avMaterials; + std::string defaultTexture; + LoadMaterial(&avMaterials, defaultTexture, pointsOnly); - //free the file buffer - streamedBuffer.close(); - - if (mGeneratedMesh == NULL) - { - throw DeadlyImportError("Invalid .ply file: Unable to extract mesh data "); - } - - // if no face list is existing we assume that the vertex - // list is containing a list of points - bool pointsOnly = mGeneratedMesh->mFaces == NULL ? true : false; - if (pointsOnly) - { - if (mGeneratedMesh->mNumVertices < 3) - { - if (mGeneratedMesh != NULL) - { - delete(mGeneratedMesh); - mGeneratedMesh = nullptr; - } - - streamedBuffer.close(); - throw DeadlyImportError("Invalid .ply file: Not enough " - "vertices to build a proper face list. "); + // now generate the output scene object. Fill the material list + pScene->mNumMaterials = (unsigned int)avMaterials.size(); + pScene->mMaterials = new aiMaterial*[pScene->mNumMaterials]; + for (unsigned int i = 0; i < pScene->mNumMaterials; ++i) { + pScene->mMaterials[i] = avMaterials[i]; } - const unsigned int iNum = (unsigned int)mGeneratedMesh->mNumVertices / 3; - mGeneratedMesh->mNumFaces = iNum; - mGeneratedMesh->mFaces = new aiFace[mGeneratedMesh->mNumFaces]; + // fill the mesh list + pScene->mNumMeshes = 1; + pScene->mMeshes = new aiMesh*[pScene->mNumMeshes]; + pScene->mMeshes[0] = mGeneratedMesh; + mGeneratedMesh = nullptr; - for (unsigned int i = 0; i < iNum; ++i) - { - mGeneratedMesh->mFaces[i].mNumIndices = 3; - mGeneratedMesh->mFaces[i].mIndices = new unsigned int[3]; - mGeneratedMesh->mFaces[i].mIndices[0] = (i * 3); - mGeneratedMesh->mFaces[i].mIndices[1] = (i * 3) + 1; - mGeneratedMesh->mFaces[i].mIndices[2] = (i * 3) + 2; + // generate a simple node structure + pScene->mRootNode = new aiNode(); + pScene->mRootNode->mNumMeshes = pScene->mNumMeshes; + pScene->mRootNode->mMeshes = new unsigned int[pScene->mNumMeshes]; + + for (unsigned int i = 0; i < pScene->mRootNode->mNumMeshes; ++i) { + pScene->mRootNode->mMeshes[i] = i; } - } - - // now load a list of all materials - std::vector avMaterials; - std::string defaultTexture; - LoadMaterial(&avMaterials, defaultTexture, pointsOnly); - - // now generate the output scene object. Fill the material list - pScene->mNumMaterials = (unsigned int)avMaterials.size(); - pScene->mMaterials = new aiMaterial*[pScene->mNumMaterials]; - for (unsigned int i = 0; i < pScene->mNumMaterials; ++i) { - pScene->mMaterials[i] = avMaterials[i]; - } - - // fill the mesh list - pScene->mNumMeshes = 1; - pScene->mMeshes = new aiMesh*[pScene->mNumMeshes]; - pScene->mMeshes[0] = mGeneratedMesh; - mGeneratedMesh = nullptr; - - // generate a simple node structure - pScene->mRootNode = new aiNode(); - pScene->mRootNode->mNumMeshes = pScene->mNumMeshes; - pScene->mRootNode->mMeshes = new unsigned int[pScene->mNumMeshes]; - - for (unsigned int i = 0; i < pScene->mRootNode->mNumMeshes; ++i) { - pScene->mRootNode->mMeshes[i] = i; - } } void PLYImporter::LoadVertex(const PLY::Element* pcElement, const PLY::ElementInstance* instElement, unsigned int pos) { @@ -521,9 +501,7 @@ void PLYImporter::LoadVertex(const PLY::Element* pcElement, const PLY::ElementIn // ------------------------------------------------------------------------------------------------ // Convert a color component to [0...1] -ai_real PLYImporter::NormalizeColorValue(PLY::PropertyInstance::ValueUnion val, - PLY::EDataType eType) -{ +ai_real PLYImporter::NormalizeColorValue(PLY::PropertyInstance::ValueUnion val, PLY::EDataType eType) { switch (eType) { case EDT_Float: diff --git a/code/PlyLoader.h b/code/PlyLoader.h index beada5f23..9199e17d7 100644 --- a/code/PlyLoader.h +++ b/code/PlyLoader.h @@ -57,7 +57,6 @@ struct aiMesh; namespace Assimp { - using namespace PLY; // --------------------------------------------------------------------------- diff --git a/code/PlyParser.h b/code/PlyParser.h index cba48a4b6..261ac7b82 100644 --- a/code/PlyParser.h +++ b/code/PlyParser.h @@ -39,12 +39,11 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. ---------------------------------------------------------------------- */ - /** @file Defines the helper data structures for importing PLY files */ +#pragma once #ifndef AI_PLYFILEHELPER_H_INC #define AI_PLYFILEHELPER_H_INC - #include #include #include @@ -58,8 +57,7 @@ class PLYImporter; // http://local.wasp.uwa.edu.au/~pbourke/dataformats/ply/ // http://w3.impa.br/~lvelho/outgoing/sossai/old/ViHAP_D4.4.2_PLY_format_v1.1.pdf // http://www.okino.com/conv/exp_ply.htm -namespace PLY -{ +namespace PLY { // --------------------------------------------------------------------------------- /* @@ -78,8 +76,7 @@ int8 int16 uint8 ... forms are also used */ -enum EDataType -{ +enum EDataType { EDT_Char = 0x0u, EDT_UChar, EDT_Short, @@ -98,8 +95,7 @@ enum EDataType * * Semantics define the usage of a property, e.g. x coordinate */ -enum ESemantic -{ +enum ESemantic { //! vertex position x coordinate EST_XCoord = 0x0u, //! vertex position x coordinate @@ -182,15 +178,14 @@ enum ESemantic * * Semantics define the usage of an element, e.g. vertex or material */ -enum EElementSemantic -{ +enum EElementSemantic { //! The element is a vertex EEST_Vertex = 0x0u, //! The element is a face description (index table) EEST_Face, - //! The element is a tristrip description (index table) + //! The element is a triangle-strip description (index table) EEST_TriStrip, //! The element is an edge description (ignored) @@ -211,17 +206,16 @@ enum EElementSemantic * * This can e.g. be a part of the vertex declaration */ -class Property -{ +class Property { public: - //! Default constructor Property() - : eType (EDT_Int), - Semantic(), - bIsList(false), - eFirstType(EDT_UChar) - {} + : eType (EDT_Int) + , Semantic() + , bIsList(false) + , eFirstType(EDT_UChar) { + // empty + } //! Data type of the property EDataType eType; @@ -260,15 +254,14 @@ public: * This can e.g. be the vertex declaration. Elements contain a * well-defined number of properties. */ -class Element -{ +class Element { public: - //! Default constructor Element() - : eSemantic (EEST_INVALID) - , NumOccur(0) - {} + : eSemantic (EEST_INVALID) + , NumOccur(0) { + // empty + } //! List of properties assigned to the element //! std::vector to support operator[] diff --git a/code/XFileParser.cpp b/code/XFileParser.cpp index cf3529653..4017ed596 100644 --- a/code/XFileParser.cpp +++ b/code/XFileParser.cpp @@ -1047,8 +1047,10 @@ void XFileParser::readHeadOfDataObject( std::string* poName) if( poName) *poName = nameOrBrace; - if( GetNextToken() != "{") - ThrowException( "Opening brace expected."); + if ( GetNextToken() != "{" ) { + delete mScene; + ThrowException( "Opening brace expected." ); + } } } @@ -1224,21 +1226,29 @@ void XFileParser::GetNextTokenAsString( std::string& poString) } FindNextNoneWhiteSpace(); - if( mP >= mEnd) - ThrowException( "Unexpected end of file while parsing string"); + if ( mP >= mEnd ) { + delete mScene; + ThrowException( "Unexpected end of file while parsing string" ); + } - if( *mP != '"') - ThrowException( "Expected quotation mark."); + if ( *mP != '"' ) { + delete mScene; + ThrowException( "Expected quotation mark." ); + } ++mP; while( mP < mEnd && *mP != '"') poString.append( mP++, 1); - if( mP >= mEnd-1) - ThrowException( "Unexpected end of file while parsing string"); + if ( mP >= mEnd - 1 ) { + delete mScene; + ThrowException( "Unexpected end of file while parsing string" ); + } - if( mP[1] != ';' || mP[0] != '"') - ThrowException( "Expected quotation mark and semicolon at the end of a string."); + if ( mP[ 1 ] != ';' || mP[ 0 ] != '"' ) { + delete mScene; + ThrowException( "Expected quotation mark and semicolon at the end of a string." ); + } mP+=2; } @@ -1449,15 +1459,15 @@ aiColor3D XFileParser::ReadRGB() // ------------------------------------------------------------------------------------------------ // Throws an exception with a line number and the given text. -AI_WONT_RETURN void XFileParser::ThrowException( const std::string& pText) -{ - if( mIsBinaryFormat) - throw DeadlyImportError( pText); - else +AI_WONT_RETURN void XFileParser::ThrowException( const std::string& pText) { + delete mScene; + if ( mIsBinaryFormat ) { + throw DeadlyImportError( pText ); + } else { throw DeadlyImportError( format() << "Line " << mLineNumber << ": " << pText ); + } } - // ------------------------------------------------------------------------------------------------ // Filters the imported hierarchy for some degenerated cases that some exporters produce. void XFileParser::FilterHierarchy( XFile::Node* pNode) diff --git a/test/unit/utPLYImportExport.cpp b/test/unit/utPLYImportExport.cpp index fdd20008d..633beda5f 100644 --- a/test/unit/utPLYImportExport.cpp +++ b/test/unit/utPLYImportExport.cpp @@ -103,3 +103,28 @@ TEST_F( utPLYImportExport, vertexColorTest ) { const aiScene *scene = importer.ReadFile( ASSIMP_TEST_MODELS_DIR "/PLY/float-color.ply", 0 ); EXPECT_NE( nullptr, scene ); } + +static const char *test_file = + "ply\n" + "format ascii 1.0\n" + "element vertex 4\n" + "property float x\n" + "property float y\n" + "property float z\n" + "property uchar red\n" + "property uchar green\n" + "property uchar blue\n" + "property float nx\n" + "property float ny\n" + "property float nz\n" + "end_header\n" + "0.0 0.0 0.0 255 255 255 0.0 1.0 0.0\n" + "0.0 0.0 1.0 255 0 255 0.0 0.0 1.0\n" + "0.0 1.0 0.0 255 255 0 1.0 0.0 0.0\n" + "0.0 1.0 1.0 0 255 255 1.0 1.0 0.0\n"; + +TEST_F( utPLYImportExport, parseErrorTest ) { + Assimp::Importer importer; + const aiScene *scene = importer.ReadFileFromMemory( test_file, strlen( test_file ), 0 ); + EXPECT_NE( nullptr, scene ); +}