From 3685791e0d05623c68d990ba3878f17d99888211 Mon Sep 17 00:00:00 2001 From: Kim Kulling Date: Sun, 4 Feb 2018 22:15:18 +0100 Subject: [PATCH 01/20] closes https://github.com/assimp/assimp/issues/1729: check for bit flip when unsigned int overflow happens in x-file parsing. --- code/XFileParser.cpp | 241 ++++++++++++++++++++++--------------------- code/XFileParser.h | 59 +++++------ 2 files changed, 151 insertions(+), 149 deletions(-) diff --git a/code/XFileParser.cpp b/code/XFileParser.cpp index c7efb83c4..34d8c89f9 100644 --- a/code/XFileParser.cpp +++ b/code/XFileParser.cpp @@ -87,59 +87,60 @@ static void dummy_free (void* /*opaque*/, void* address) { // ------------------------------------------------------------------------------------------------ // Constructor. Creates a data structure out of the XFile given in the memory block. XFileParser::XFileParser( const std::vector& pBuffer) -{ - mMajorVersion = mMinorVersion = 0; - mIsBinaryFormat = false; - mBinaryNumCount = 0; - P = End = NULL; - mLineNumber = 0; - mScene = NULL; - +: mMajorVersion( 0 ) +, mMinorVersion( 0 ) +, mIsBinaryFormat( false ) +, mBinaryNumCount( 0 ) +, mP( nullptr ) +, mEnd( nullptr ) +, mLineNumber( 0 ) +, mScene( nullptr ) { // vector to store uncompressed file for INFLATE'd X files std::vector uncompressed; // set up memory pointers - P = &pBuffer.front(); - End = P + pBuffer.size() - 1; + mP = &pBuffer.front(); + mEnd = mP + pBuffer.size() - 1; // check header - if( strncmp( P, "xof ", 4) != 0) - throw DeadlyImportError( "Header mismatch, file is not an XFile."); + if ( 0 != strncmp( mP, "xof ", 4 ) ) { + throw DeadlyImportError( "Header mismatch, file is not an XFile." ); + } // read version. It comes in a four byte format such as "0302" - mMajorVersion = (unsigned int)(P[4] - 48) * 10 + (unsigned int)(P[5] - 48); - mMinorVersion = (unsigned int)(P[6] - 48) * 10 + (unsigned int)(P[7] - 48); + mMajorVersion = (unsigned int)(mP[4] - 48) * 10 + (unsigned int)(mP[5] - 48); + mMinorVersion = (unsigned int)(mP[6] - 48) * 10 + (unsigned int)(mP[7] - 48); bool compressed = false; // txt - pure ASCII text format - if( strncmp( P + 8, "txt ", 4) == 0) + if( strncmp( mP + 8, "txt ", 4) == 0) mIsBinaryFormat = false; // bin - Binary format - else if( strncmp( P + 8, "bin ", 4) == 0) + else if( strncmp( mP + 8, "bin ", 4) == 0) mIsBinaryFormat = true; // tzip - Inflate compressed text format - else if( strncmp( P + 8, "tzip", 4) == 0) + else if( strncmp( mP + 8, "tzip", 4) == 0) { mIsBinaryFormat = false; compressed = true; } // bzip - Inflate compressed binary format - else if( strncmp( P + 8, "bzip", 4) == 0) + else if( strncmp( mP + 8, "bzip", 4) == 0) { mIsBinaryFormat = true; compressed = true; } else ThrowException( format() << "Unsupported xfile format '" << - P[8] << P[9] << P[10] << P[11] << "'"); + mP[8] << mP[9] << mP[10] << mP[11] << "'"); // float size - mBinaryFloatSize = (unsigned int)(P[12] - 48) * 1000 - + (unsigned int)(P[13] - 48) * 100 - + (unsigned int)(P[14] - 48) * 10 - + (unsigned int)(P[15] - 48); + mBinaryFloatSize = (unsigned int)(mP[12] - 48) * 1000 + + (unsigned int)(mP[13] - 48) * 100 + + (unsigned int)(mP[14] - 48) * 10 + + (unsigned int)(mP[15] - 48); if( mBinaryFloatSize != 32 && mBinaryFloatSize != 64) ThrowException( format() << "Unknown float size " << mBinaryFloatSize << " specified in xfile header." ); @@ -147,7 +148,7 @@ XFileParser::XFileParser( const std::vector& pBuffer) // The x format specifies size in bits, but we work in bytes mBinaryFloatSize /= 8; - P += 16; + mP += 16; // If this is a compressed X file, apply the inflate algorithm to it if (compressed) @@ -186,13 +187,13 @@ XFileParser::XFileParser( const std::vector& pBuffer) ::inflateInit2(&stream, -MAX_WBITS); // skip unknown data (checksum, flags?) - P += 6; + mP += 6; // First find out how much storage we'll need. Count sections. - const char* P1 = P; + const char* P1 = mP; unsigned int est_out = 0; - while (P1 + 3 < End) + while (P1 + 3 < mEnd) { // read next offset uint16_t ofs = *((uint16_t*)P1); @@ -216,18 +217,18 @@ XFileParser::XFileParser( const std::vector& pBuffer) // Allocate storage and terminating zero and do the actual uncompressing uncompressed.resize(est_out + 1); char* out = &uncompressed.front(); - while (P + 3 < End) + while (mP + 3 < mEnd) { - uint16_t ofs = *((uint16_t*)P); + uint16_t ofs = *((uint16_t*)mP); AI_SWAP2(ofs); - P += 4; + mP += 4; - if (P + ofs > End + 2) { + if (mP + ofs > mEnd + 2) { throw DeadlyImportError("X: Unexpected EOF in compressed chunk"); } // push data to the stream - stream.next_in = (Bytef*)P; + stream.next_in = (Bytef*)mP; stream.avail_in = ofs; stream.next_out = (Bytef*)out; stream.avail_out = MSZIP_BLOCK; @@ -242,15 +243,15 @@ XFileParser::XFileParser( const std::vector& pBuffer) // and advance to the next offset out += MSZIP_BLOCK - stream.avail_out; - P += ofs; + mP += ofs; } // terminate zlib ::inflateEnd(&stream); // ok, update pointers to point to the uncompressed file data - P = &uncompressed[0]; - End = out; + mP = &uncompressed[0]; + mEnd = out; // FIXME: we don't need the compressed data anymore, could release // it already for better memory usage. Consider breaking const-co. @@ -647,8 +648,8 @@ void XFileParser::ParseDataObjectMeshVertexColors( Mesh* pMesh) if( !mIsBinaryFormat) { FindNextNoneWhiteSpace(); - if( *P == ';' || *P == ',') - P++; + if( *mP == ';' || *mP == ',') + mP++; } } @@ -678,8 +679,8 @@ void XFileParser::ParseDataObjectMeshMaterialList( Mesh* pMesh) // commented out version check, as version 03.03 exported from blender also has 2 semicolons if( !mIsBinaryFormat) // && MajorVersion == 3 && MinorVersion <= 2) { - if(P < End && *P == ';') - ++P; + if(mP < mEnd && *mP == ';') + ++mP; } // if there was only a single material index, replicate it on all faces @@ -1029,12 +1030,12 @@ void XFileParser::TestForSeparator() return; FindNextNoneWhiteSpace(); - if( P >= End) + if( mP >= mEnd) return; // test and skip - if( *P == ';' || *P == ',') - P++; + if( *mP == ';' || *mP == ',') + mP++; } // ------------------------------------------------------------------------------------------------ @@ -1062,46 +1063,54 @@ std::string XFileParser::GetNextToken() // in binary mode it will only return NAME and STRING token // and (correctly) skip over other tokens. - if( End - P < 2) return s; + if( mEnd - mP < 2) return s; unsigned int tok = ReadBinWord(); unsigned int len; // standalone tokens switch( tok) { - case 1: - // name token - if( End - P < 4) return s; - len = ReadBinDWord(); - if( End - P < int(len)) return s; - s = std::string(P, len); - P += len; + case 1: { + // name token + if ( mEnd - mP < 4 ) return s; + len = ReadBinDWord(); + const int bounds( mEnd - mP ); + const int iLen( len ); + if ( iLen < 0 ) { + return s; + } + if ( bounds < iLen ) { + return s; + } + s = std::string( mP, len ); + mP += len; + } return s; case 2: // string token - if( End - P < 4) return s; + if( mEnd - mP < 4) return s; len = ReadBinDWord(); - if( End - P < int(len)) return s; - s = std::string(P, len); - P += (len + 2); + if( mEnd - mP < int(len)) return s; + s = std::string(mP, len); + mP += (len + 2); return s; case 3: // integer token - P += 4; + mP += 4; return ""; case 5: // GUID token - P += 16; + mP += 16; return ""; case 6: - if( End - P < 4) return s; + if( mEnd - mP < 4) return s; len = ReadBinDWord(); - P += (len * 4); + mP += (len * 4); return ""; case 7: - if( End - P < 4) return s; + if( mEnd - mP < 4) return s; len = ReadBinDWord(); - P += (len * mBinaryFloatSize); + mP += (len * mBinaryFloatSize); return ""; case 0x0a: return "{"; @@ -1159,19 +1168,19 @@ std::string XFileParser::GetNextToken() else { FindNextNoneWhiteSpace(); - if( P >= End) + if( mP >= mEnd) return s; - while( (P < End) && !isspace( (unsigned char) *P)) + while( (mP < mEnd) && !isspace( (unsigned char) *mP)) { // either keep token delimiters when already holding a token, or return if first valid char - if( *P == ';' || *P == '}' || *P == '{' || *P == ',') + if( *mP == ';' || *mP == '}' || *mP == '{' || *mP == ',') { if( !s.size()) - s.append( P++, 1); + s.append( mP++, 1); break; // stop for delimiter } - s.append( P++, 1); + s.append( mP++, 1); } } return s; @@ -1186,18 +1195,18 @@ void XFileParser::FindNextNoneWhiteSpace() bool running = true; while( running ) { - while( P < End && isspace( (unsigned char) *P)) + while( mP < mEnd && isspace( (unsigned char) *mP)) { - if( *P == '\n') + if( *mP == '\n') mLineNumber++; - ++P; + ++mP; } - if( P >= End) + if( mP >= mEnd) return; // check if this is a comment - if( (P[0] == '/' && P[1] == '/') || P[0] == '#') + if( (mP[0] == '/' && mP[1] == '/') || mP[0] == '#') ReadUntilEndOfLine(); else break; @@ -1214,22 +1223,22 @@ void XFileParser::GetNextTokenAsString( std::string& poString) } FindNextNoneWhiteSpace(); - if( P >= End) + if( mP >= mEnd) ThrowException( "Unexpected end of file while parsing string"); - if( *P != '"') + if( *mP != '"') ThrowException( "Expected quotation mark."); - ++P; + ++mP; - while( P < End && *P != '"') - poString.append( P++, 1); + while( mP < mEnd && *mP != '"') + poString.append( mP++, 1); - if( P >= End-1) + if( mP >= mEnd-1) ThrowException( "Unexpected end of file while parsing string"); - if( P[1] != ';' || P[0] != '"') + if( mP[1] != ';' || mP[0] != '"') ThrowException( "Expected quotation mark and semicolon at the end of a string."); - P+=2; + mP+=2; } // ------------------------------------------------------------------------------------------------ @@ -1238,35 +1247,35 @@ void XFileParser::ReadUntilEndOfLine() if( mIsBinaryFormat) return; - while( P < End) + while( mP < mEnd) { - if( *P == '\n' || *P == '\r') + if( *mP == '\n' || *mP == '\r') { - ++P; mLineNumber++; + ++mP; mLineNumber++; return; } - ++P; + ++mP; } } // ------------------------------------------------------------------------------------------------ unsigned short XFileParser::ReadBinWord() { - ai_assert(End - P >= 2); - const unsigned char* q = (const unsigned char*) P; + ai_assert(mEnd - mP >= 2); + const unsigned char* q = (const unsigned char*) mP; unsigned short tmp = q[0] | (q[1] << 8); - P += 2; + mP += 2; return tmp; } // ------------------------------------------------------------------------------------------------ -unsigned int XFileParser::ReadBinDWord() -{ - ai_assert(End - P >= 4); - const unsigned char* q = (const unsigned char*) P; +unsigned int XFileParser::ReadBinDWord() { + ai_assert(mEnd - mP >= 4); + + const unsigned char* q = (const unsigned char*) mP; unsigned int tmp = q[0] | (q[1] << 8) | (q[2] << 16) | (q[3] << 24); - P += 4; + mP += 4; return tmp; } @@ -1275,20 +1284,20 @@ unsigned int XFileParser::ReadInt() { if( mIsBinaryFormat) { - if( mBinaryNumCount == 0 && End - P >= 2) + if( mBinaryNumCount == 0 && mEnd - mP >= 2) { unsigned short tmp = ReadBinWord(); // 0x06 or 0x03 - if( tmp == 0x06 && End - P >= 4) // array of ints follows + if( tmp == 0x06 && mEnd - mP >= 4) // array of ints follows mBinaryNumCount = ReadBinDWord(); else // single int follows mBinaryNumCount = 1; } --mBinaryNumCount; - if ( End - P >= 4) { + if ( mEnd - mP >= 4) { return ReadBinDWord(); } else { - P = End; + mP = mEnd; return 0; } } else @@ -1299,24 +1308,24 @@ unsigned int XFileParser::ReadInt() // check preceding minus sign bool isNegative = false; - if( *P == '-') + if( *mP == '-') { isNegative = true; - P++; + mP++; } // at least one digit expected - if( !isdigit( *P)) + if( !isdigit( *mP)) ThrowException( "Number expected."); // read digits unsigned int number = 0; - while( P < End) + while( mP < mEnd) { - if( !isdigit( *P)) + if( !isdigit( *mP)) break; - number = number * 10 + (*P - 48); - P++; + number = number * 10 + (*mP - 48); + mP++; } CheckForSeparator(); @@ -1329,10 +1338,10 @@ ai_real XFileParser::ReadFloat() { if( mIsBinaryFormat) { - if( mBinaryNumCount == 0 && End - P >= 2) + if( mBinaryNumCount == 0 && mEnd - mP >= 2) { unsigned short tmp = ReadBinWord(); // 0x07 or 0x42 - if( tmp == 0x07 && End - P >= 4) // array of floats following + if( tmp == 0x07 && mEnd - mP >= 4) // array of floats following mBinaryNumCount = ReadBinDWord(); else // single float following mBinaryNumCount = 1; @@ -1341,22 +1350,22 @@ ai_real XFileParser::ReadFloat() --mBinaryNumCount; if( mBinaryFloatSize == 8) { - if( End - P >= 8) { - ai_real result = (ai_real) (*(double*) P); - P += 8; + if( mEnd - mP >= 8) { + ai_real result = (ai_real) (*(double*) mP); + mP += 8; return result; } else { - P = End; + mP = mEnd; return 0; } } else { - if( End - P >= 4) { - ai_real result = *(ai_real*) P; - P += 4; + if( mEnd - mP >= 4) { + ai_real result = *(ai_real*) mP; + mP += 4; return result; } else { - P = End; + mP = mEnd; return 0; } } @@ -1367,21 +1376,21 @@ ai_real XFileParser::ReadFloat() // check for various special strings to allow reading files from faulty exporters // I mean you, Blender! // Reading is safe because of the terminating zero - if( strncmp( P, "-1.#IND00", 9) == 0 || strncmp( P, "1.#IND00", 8) == 0) + if( strncmp( mP, "-1.#IND00", 9) == 0 || strncmp( mP, "1.#IND00", 8) == 0) { - P += 9; + mP += 9; CheckForSeparator(); return 0.0; } else - if( strncmp( P, "1.#QNAN0", 8) == 0) + if( strncmp( mP, "1.#QNAN0", 8) == 0) { - P += 8; + mP += 8; CheckForSeparator(); return 0.0; } ai_real result = 0.0; - P = fast_atoreal_move( P, result); + mP = fast_atoreal_move( mP, result); CheckForSeparator(); diff --git a/code/XFileParser.h b/code/XFileParser.h index 050ecb9ca..24eb6104d 100644 --- a/code/XFileParser.h +++ b/code/XFileParser.h @@ -49,10 +49,8 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. #include -namespace Assimp -{ - namespace XFile - { +namespace Assimp { + namespace XFile { struct Node; struct Mesh; struct Scene; @@ -61,21 +59,20 @@ namespace Assimp struct AnimBone; } -/** The XFileParser reads a XFile either in text or binary form and builds a temporary - * data structure out of it. - */ -class XFileParser -{ +/** + * @brief The XFileParser reads a XFile either in text or binary form and builds a temporary + * data structure out of it. + */ +class XFileParser { public: - /** Constructor. Creates a data structure out of the XFile given in the memory block. - * @param pBuffer Null-terminated memory buffer containing the XFile - */ + /// Constructor. Creates a data structure out of the XFile given in the memory block. + /// @param pBuffer Null-terminated memory buffer containing the XFile explicit XFileParser( const std::vector& pBuffer); - /** Destructor. Destroys all imported data along with it */ + /// Destructor. Destroys all imported data along with it ~XFileParser(); - /** Returns the temporary representation of the imported data */ + /// Returns the temporary representation of the imported data. XFile::Scene* GetImportedData() const { return mScene; } protected: @@ -101,10 +98,10 @@ protected: //! places pointer to next begin of a token, and ignores comments void FindNextNoneWhiteSpace(); - //! returns next parseable token. Returns empty string if no token there + //! returns next valid token. Returns empty string if no token there std::string GetNextToken(); - //! reads header of dataobject including the opening brace. + //! reads header of data object including the opening brace. //! returns false if error happened, and writes name of object //! if there is one void readHeadOfDataObject( std::string* poName = NULL); @@ -118,8 +115,8 @@ protected: //! checks for a separator char, either a ',' or a ';' void CheckForSeparator(); - /// tests and possibly consumes a separator char, but does nothing if there was no separator - void TestForSeparator(); + /// tests and possibly consumes a separator char, but does nothing if there was no separator + void TestForSeparator(); //! reads a x file style string void GetNextTokenAsString( std::string& poString); @@ -138,27 +135,23 @@ protected: /** Throws an exception with a line number and the given text. */ AI_WONT_RETURN void ThrowException( const std::string& pText) AI_WONT_RETURN_SUFFIX; - /** Filters the imported hierarchy for some degenerated cases that some exporters produce. - * @param pData The sub-hierarchy to filter - */ + /** + * @brief Filters the imported hierarchy for some degenerated cases that some exporters produce. + * @param pData The sub-hierarchy to filter + */ void FilterHierarchy( XFile::Node* pNode); protected: unsigned int mMajorVersion, mMinorVersion; ///< version numbers bool mIsBinaryFormat; ///< true if the file is in binary, false if it's in text form unsigned int mBinaryFloatSize; ///< float size in bytes, either 4 or 8 - // counter for number arrays in binary format - unsigned int mBinaryNumCount; - - const char* P; - const char* End; - - /// Line number when reading in text format - unsigned int mLineNumber; - - /// Imported data - XFile::Scene* mScene; + unsigned int mBinaryNumCount; /// < counter for number arrays in binary format + const char* mP; + const char* mEnd; + unsigned int mLineNumber; ///< Line number when reading in text format + XFile::Scene* mScene; ///< Imported data }; -} +} //! ns Assimp + #endif // AI_XFILEPARSER_H_INC From 862a145ea50088585319512f49aaaca2397034a2 Mon Sep 17 00:00:00 2001 From: Kim Kulling Date: Sun, 4 Feb 2018 22:18:54 +0100 Subject: [PATCH 02/20] X: add unittest to ensure no exception will be thrown. --- test/unit/utXImporterExporter.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/unit/utXImporterExporter.cpp b/test/unit/utXImporterExporter.cpp index 770e2be47..d4c742e6f 100644 --- a/test/unit/utXImporterExporter.cpp +++ b/test/unit/utXImporterExporter.cpp @@ -62,3 +62,8 @@ public: TEST_F( utXImporterExporter, importXFromFileTest ) { EXPECT_TRUE( importerTest() ); } + +TEST_F( utXImporterExporter, heap_overflow_in_tokenizer ) { + Assimp::Importer importer; + EXPECT_NO_THROW( importer.ReadFile( ASSIMP_TEST_MODELS_DIR "/X/OV_GetNextToken", 0 ) ); +} From a8f940dd8365ac8076889ee7751949dc879862bc Mon Sep 17 00:00:00 2001 From: Kim Kulling Date: Mon, 5 Feb 2018 00:05:08 +0100 Subject: [PATCH 03/20] add missing test file. --- test/models/X/OV_GetNextToken | Bin 0 -> 775 bytes 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 test/models/X/OV_GetNextToken diff --git a/test/models/X/OV_GetNextToken b/test/models/X/OV_GetNextToken new file mode 100644 index 0000000000000000000000000000000000000000..8b2a0d0ca49350f0a8dcdda8967ccd910da116bd GIT binary patch literal 775 zcmV+i1Ni)SZ)PAcGcYq^dTDSlFf%d$3jhG{3hx3#OWjsYXcIvceu*Y+vQ7G9tkp|F z5mHYEa`R_NQ)(lo5gUK-woRt#&}3J3w>6+(4uW{{AYMgKMDgNH(L;On;@?53OVlh9XK{uYRbs~>0ywsAoD^s< zE_`|(rj{?&X~l4gF`q7KPqyRO;s43aB zMopuR?!2WeU+iOx)I%qI)`KAP(3n=q8bco$Nm%b+)N=RUw)U3}{&<-Yfn3`#odwHo z%vx14YczpL2^f{9@%CxeC#h)V;%dFyl@)vV=A6{`z-pD^YcH+zRm5Hus^wr%{l&t}8 zoF)=wX%U+m_1~p9+Y^@c!TkfeB!HHlPOnWu`{(fvDGz##u>WbB)ine=8GFXE+&ELG zq`EBY?^OfC8gb*64uOxV=X@&&6A(JTJWsYCdh0q2jq&gZs*!(Hx({YN?5ZGL&}sll zz5ca*jpO=*?bQDc)*l1)K0khYPF-THcF8knP2T{kZ(_r9j6#Oh2v5ciQQI)1kRnSS z3G0k6q=+I%c&$AdXF1L&rpPfqqn?a$l8h3HjQpUGaWe1>4J$J8yM&BWg)hwAij4do zo~I_}VU|ssQFnrdK@LPuyuNk5SKqzn)JrZKp65XR{}~2NTs$}qWzuw)ZIZVvyXqT? z!|?Av+NPfbQP?MTJf^fU>}y+8v`)4uE?Onqk`%3xZ5R=)kYyiLNbuU1%8V~`Nl#DX+hsB;m*C0VkUe4QhoZ^XPrEHf13EU?ITs+9fQ@y F-#UtLa=-up literal 0 HcmV?d00001 From ff556027ef4424ba2f1ed64908be54bdcf98038e Mon Sep 17 00:00:00 2001 From: Kim Kulling Date: Mon, 5 Feb 2018 00:33:41 +0100 Subject: [PATCH 04/20] X: fix some review findings. --- code/XFileParser.cpp | 61 ++++++++++++++++++++++---------------------- 1 file changed, 31 insertions(+), 30 deletions(-) diff --git a/code/XFileParser.cpp b/code/XFileParser.cpp index 34d8c89f9..92d8a579b 100644 --- a/code/XFileParser.cpp +++ b/code/XFileParser.cpp @@ -466,12 +466,11 @@ void XFileParser::ParseDataObjectMesh( Mesh* pMesh) // read position faces unsigned int numPosFaces = ReadInt(); pMesh->mPosFaces.resize( numPosFaces); - for( unsigned int a = 0; a < numPosFaces; a++) - { + for( unsigned int a = 0; a < numPosFaces; ++a) { // read indices unsigned int numIndices = ReadInt(); Face& face = pMesh->mPosFaces[a]; - for (unsigned int b = 0; b < numIndices; b++) { + for (unsigned int b = 0; b < numIndices; ++b) { face.mIndices.push_back( ReadInt() ); } TestForSeparator(); @@ -479,11 +478,10 @@ void XFileParser::ParseDataObjectMesh( Mesh* pMesh) // here, other data objects may follow bool running = true; - while ( running ) - { + while ( running ) { std::string objectName = GetNextToken(); - if( objectName.size() == 0) + if( objectName.empty() ) ThrowException( "Unexpected end of file while parsing mesh structure"); else if( objectName == "}") @@ -518,8 +516,10 @@ void XFileParser::ParseDataObjectMesh( Mesh* pMesh) } // ------------------------------------------------------------------------------------------------ -void XFileParser::ParseDataObjectSkinWeights( Mesh *pMesh) -{ +void XFileParser::ParseDataObjectSkinWeights( Mesh *pMesh) { + if ( nullptr == pMesh ) { + return; + } readHeadOfDataObject(); std::string transformNodeName; @@ -1053,39 +1053,40 @@ void XFileParser::readHeadOfDataObject( std::string* poName) } // ------------------------------------------------------------------------------------------------ -std::string XFileParser::GetNextToken() -{ +std::string XFileParser::GetNextToken() { std::string s; // process binary-formatted file - if( mIsBinaryFormat) - { + if( mIsBinaryFormat) { // in binary mode it will only return NAME and STRING token // and (correctly) skip over other tokens. - - if( mEnd - mP < 2) return s; + if ( mEnd - mP < 2 ) { + return s; + } unsigned int tok = ReadBinWord(); unsigned int len; // standalone tokens - switch( tok) - { + switch( tok ) { case 1: { - // name token - if ( mEnd - mP < 4 ) return s; - len = ReadBinDWord(); - const int bounds( mEnd - mP ); - const int iLen( len ); - if ( iLen < 0 ) { - return s; - } - if ( bounds < iLen ) { - return s; - } - s = std::string( mP, len ); - mP += len; + // name token + if ( mEnd - mP < 4 ) { + return s; } - return s; + len = ReadBinDWord(); + const int bounds( mEnd - mP ); + const int iLen( len ); + if ( iLen < 0 ) { + return s; + } + if ( bounds < iLen ) { + return s; + } + s = std::string( mP, len ); + mP += len; + } + return s; + case 2: // string token if( mEnd - mP < 4) return s; From eb23946fe74955eca8978e168e5a6fb00eba6e36 Mon Sep 17 00:00:00 2001 From: Kim Kulling Date: Mon, 5 Feb 2018 13:44:19 +0100 Subject: [PATCH 05/20] Update XFileParser.cpp Fix alignment for float and double. --- code/XFileParser.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/code/XFileParser.cpp b/code/XFileParser.cpp index 92d8a579b..76ea71e21 100644 --- a/code/XFileParser.cpp +++ b/code/XFileParser.cpp @@ -1349,20 +1349,20 @@ ai_real XFileParser::ReadFloat() } --mBinaryNumCount; - if( mBinaryFloatSize == 8) - { + if( mBinaryFloatSize == 8) { if( mEnd - mP >= 8) { - ai_real result = (ai_real) (*(double*) mP); + double res; + ::memcpy( &res, mP, 8 ); mP += 8; return result; } else { mP = mEnd; return 0; } - } else - { + } else { if( mEnd - mP >= 4) { - ai_real result = *(ai_real*) mP; + ai_real result; + ::memcpy( &result, mP, 4 ); mP += 4; return result; } else { From dceb7257ddf888908581380fd8bf1a7ec1a98d66 Mon Sep 17 00:00:00 2001 From: Kim Kulling Date: Mon, 5 Feb 2018 13:53:06 +0100 Subject: [PATCH 06/20] Update XFileParser.cpp Fix the build. --- code/XFileParser.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/code/XFileParser.cpp b/code/XFileParser.cpp index 76ea71e21..cf3529653 100644 --- a/code/XFileParser.cpp +++ b/code/XFileParser.cpp @@ -1354,6 +1354,7 @@ ai_real XFileParser::ReadFloat() double res; ::memcpy( &res, mP, 8 ); mP += 8; + const ai_real result( static_cast( res ) ); return result; } else { mP = mEnd; From d284d107e73cd3ae80733e3351c213025365d74c Mon Sep 17 00:00:00 2001 From: Turo Lamminen Date: Tue, 6 Feb 2018 18:43:51 +0200 Subject: [PATCH 07/20] XGLLoader: Fix a memory leak --- code/XGLLoader.cpp | 21 +++++---------------- 1 file changed, 5 insertions(+), 16 deletions(-) diff --git a/code/XGLLoader.cpp b/code/XGLLoader.cpp index 2d7a40362..a9fd6a5ab 100644 --- a/code/XGLLoader.cpp +++ b/code/XGLLoader.cpp @@ -72,17 +72,6 @@ using namespace irr::io; #endif -// scopeguard for a malloc'ed buffer -struct free_it -{ - free_it(void* free) : free(free) {} - ~free_it() { - ::free(this->free); - } - - void* free; -}; - namespace Assimp { // this has to be in here because LogFunctions is in ::Assimp template<> const char* LogFunctions::Prefix() { @@ -155,8 +144,7 @@ void XGLImporter::InternReadFile( const std::string& pFile, aiScene* pScene, IOSystem* pIOHandler) { #ifndef ASSIMP_BUILD_NO_COMPRESSED_XGL - Bytef* dest = NULL; - free_it free_it_really(dest); + std::vector uncompressed; #endif m_scene = pScene; @@ -192,6 +180,7 @@ void XGLImporter::InternReadFile( const std::string& pFile, size_t total = 0l; + // TODO: be smarter about this, decompress directly into heap buffer // and decompress the data .... do 1k chunks in the hope that we won't kill the stack #define MYBLOCK 1024 Bytef block[MYBLOCK]; @@ -206,8 +195,8 @@ void XGLImporter::InternReadFile( const std::string& pFile, } const size_t have = MYBLOCK - zstream.avail_out; total += have; - dest = reinterpret_cast( realloc(dest,total) ); - memcpy(dest + total - have,block,have); + uncompressed.resize(total); + memcpy(uncompressed.data() + total - have,block,have); } while (ret != Z_STREAM_END); @@ -215,7 +204,7 @@ void XGLImporter::InternReadFile( const std::string& pFile, inflateEnd(&zstream); // replace the input stream with a memory stream - stream.reset(new MemoryIOStream(reinterpret_cast(dest),total)); + stream.reset(new MemoryIOStream(reinterpret_cast(uncompressed.data()),total)); #endif } From c42dd9104cb7a4faea288a634cd2f0c170366308 Mon Sep 17 00:00:00 2001 From: Turo Lamminen Date: Tue, 6 Feb 2018 18:52:23 +0200 Subject: [PATCH 08/20] BlenderLoader: Fix memory leak --- code/BlenderLoader.cpp | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/code/BlenderLoader.cpp b/code/BlenderLoader.cpp index 482f9ae5c..d4d6473c5 100644 --- a/code/BlenderLoader.cpp +++ b/code/BlenderLoader.cpp @@ -154,14 +154,6 @@ void BlenderImporter::SetupProperties(const Importer* /*pImp*/) // nothing to be done for the moment } -struct free_it { - free_it(void* free) : free(free) {} - ~free_it() { - ::free(this->free); - } - - void* free; -}; // ------------------------------------------------------------------------------------------------ // Imports the given file into the given scene structure. @@ -169,8 +161,7 @@ void BlenderImporter::InternReadFile( const std::string& pFile, aiScene* pScene, IOSystem* pIOHandler) { #ifndef ASSIMP_BUILD_NO_COMPRESSED_BLEND - Bytef* dest = NULL; - free_it free_it_really(dest); + std::vector uncompressed; #endif @@ -218,6 +209,7 @@ void BlenderImporter::InternReadFile( const std::string& pFile, size_t total = 0l; + // TODO: be smarter about this, decompress directly into heap buffer // and decompress the data .... do 1k chunks in the hope that we won't kill the stack #define MYBLOCK 1024 Bytef block[MYBLOCK]; @@ -232,8 +224,8 @@ void BlenderImporter::InternReadFile( const std::string& pFile, } const size_t have = MYBLOCK - zstream.avail_out; total += have; - dest = reinterpret_cast( realloc(dest,total) ); - memcpy(dest + total - have,block,have); + uncompressed.resize(total); + memcpy(uncompressed.data() + total - have,block,have); } while (ret != Z_STREAM_END); @@ -241,7 +233,7 @@ void BlenderImporter::InternReadFile( const std::string& pFile, inflateEnd(&zstream); // replace the input stream with a memory stream - stream.reset(new MemoryIOStream(reinterpret_cast(dest),total)); + stream.reset(new MemoryIOStream(reinterpret_cast(uncompressed.data()),total)); // .. and retry stream->Read(magic,7,1); From 880be5403f466541335cce1b6fcef26b18cc9f15 Mon Sep 17 00:00:00 2001 From: Turo Lamminen Date: Tue, 6 Feb 2018 19:03:47 +0200 Subject: [PATCH 09/20] OpenGEX: Replace raw pointer with vector to fix a memory leak --- code/OpenGEXImporter.cpp | 12 ++++-------- code/OpenGEXImporter.h | 3 +-- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/code/OpenGEXImporter.cpp b/code/OpenGEXImporter.cpp index 37c765f4c..a2844ccdf 100644 --- a/code/OpenGEXImporter.cpp +++ b/code/OpenGEXImporter.cpp @@ -221,9 +221,7 @@ static void propId2StdString( Property *prop, std::string &name, std::string &ke //------------------------------------------------------------------------------------------------ OpenGEXImporter::VertexContainer::VertexContainer() -: m_numVerts( 0 ) -, m_vertices( nullptr ) -, m_numColors( 0 ) +: m_numColors( 0 ) , m_colors( nullptr ) , m_numNormals( 0 ) , m_normals( nullptr ) @@ -234,7 +232,6 @@ OpenGEXImporter::VertexContainer::VertexContainer() //------------------------------------------------------------------------------------------------ OpenGEXImporter::VertexContainer::~VertexContainer() { - delete[] m_vertices; delete[] m_colors; delete[] m_normals; @@ -857,9 +854,8 @@ void OpenGEXImporter::handleVertexArrayNode( ODDLParser::DDLNode *node, aiScene const size_t numItems( countDataArrayListItems( vaList ) ); if( Position == attribType ) { - m_currentVertices.m_numVerts = numItems; - m_currentVertices.m_vertices = new aiVector3D[ numItems ]; - copyVectorArray( numItems, vaList, m_currentVertices.m_vertices ); + m_currentVertices.m_vertices.resize( numItems ); + copyVectorArray( numItems, vaList, m_currentVertices.m_vertices.data() ); } else if ( Color == attribType ) { m_currentVertices.m_numColors = numItems; m_currentVertices.m_colors = new aiColor4D[ numItems ]; @@ -922,7 +918,7 @@ void OpenGEXImporter::handleIndexArrayNode( ODDLParser::DDLNode *node, aiScene * Value *next( vaList->m_dataList ); for( size_t indices = 0; indices < current.mNumIndices; indices++ ) { const int idx( next->getUnsignedInt32() ); - ai_assert( static_cast( idx ) <= m_currentVertices.m_numVerts ); + ai_assert( static_cast( idx ) <= m_currentVertices.m_vertices.size() ); ai_assert( index < m_currentMesh->mNumVertices ); aiVector3D &pos = ( m_currentVertices.m_vertices[ idx ] ); m_currentMesh->mVertices[ index ].Set( pos.x, pos.y, pos.z ); diff --git a/code/OpenGEXImporter.h b/code/OpenGEXImporter.h index bbbe3678d..eedb84375 100644 --- a/code/OpenGEXImporter.h +++ b/code/OpenGEXImporter.h @@ -144,8 +144,7 @@ protected: private: struct VertexContainer { - size_t m_numVerts; - aiVector3D *m_vertices; + std::vector m_vertices; size_t m_numColors; aiColor4D *m_colors; size_t m_numNormals; From 1aed63afb73a83fbfd2bb246f54f2110471aa724 Mon Sep 17 00:00:00 2001 From: Turo Lamminen Date: Tue, 6 Feb 2018 19:13:54 +0200 Subject: [PATCH 10/20] OpenGEX: Replace another raw pointer with vector to fix a memory leak --- code/OpenGEXImporter.cpp | 10 +++------- code/OpenGEXImporter.h | 3 +-- 2 files changed, 4 insertions(+), 9 deletions(-) diff --git a/code/OpenGEXImporter.cpp b/code/OpenGEXImporter.cpp index a2844ccdf..dac8299c5 100644 --- a/code/OpenGEXImporter.cpp +++ b/code/OpenGEXImporter.cpp @@ -223,8 +223,6 @@ static void propId2StdString( Property *prop, std::string &name, std::string &ke OpenGEXImporter::VertexContainer::VertexContainer() : m_numColors( 0 ) , m_colors( nullptr ) -, m_numNormals( 0 ) -, m_normals( nullptr ) , m_numUVComps() , m_textureCoords() { // empty @@ -233,7 +231,6 @@ OpenGEXImporter::VertexContainer::VertexContainer() //------------------------------------------------------------------------------------------------ OpenGEXImporter::VertexContainer::~VertexContainer() { delete[] m_colors; - delete[] m_normals; for(auto &texcoords : m_textureCoords) { delete [] texcoords; @@ -861,9 +858,8 @@ void OpenGEXImporter::handleVertexArrayNode( ODDLParser::DDLNode *node, aiScene m_currentVertices.m_colors = new aiColor4D[ numItems ]; copyColor4DArray( numItems, vaList, m_currentVertices.m_colors ); } else if( Normal == attribType ) { - m_currentVertices.m_numNormals = numItems; - m_currentVertices.m_normals = new aiVector3D[ numItems ]; - copyVectorArray( numItems, vaList, m_currentVertices.m_normals ); + m_currentVertices.m_normals.resize( numItems ); + copyVectorArray( numItems, vaList, m_currentVertices.m_normals.data() ); } else if( TexCoord == attribType ) { m_currentVertices.m_numUVComps[ 0 ] = numItems; m_currentVertices.m_textureCoords[ 0 ] = new aiVector3D[ numItems ]; @@ -900,7 +896,7 @@ void OpenGEXImporter::handleIndexArrayNode( ODDLParser::DDLNode *node, aiScene * hasColors = true; } bool hasNormalCoords( false ); - if ( m_currentVertices.m_numNormals > 0 ) { + if ( !m_currentVertices.m_normals.empty() ) { m_currentMesh->mNormals = new aiVector3D[ m_currentMesh->mNumVertices ]; hasNormalCoords = true; } diff --git a/code/OpenGEXImporter.h b/code/OpenGEXImporter.h index eedb84375..ad331d8e6 100644 --- a/code/OpenGEXImporter.h +++ b/code/OpenGEXImporter.h @@ -147,8 +147,7 @@ private: std::vector m_vertices; size_t m_numColors; aiColor4D *m_colors; - size_t m_numNormals; - aiVector3D *m_normals; + std::vector m_normals; size_t m_numUVComps[ AI_MAX_NUMBER_OF_TEXTURECOORDS ]; aiVector3D *m_textureCoords[ AI_MAX_NUMBER_OF_TEXTURECOORDS ]; From 9344074a04357fb0e8fd2537557f148dbc15ce8f Mon Sep 17 00:00:00 2001 From: Turo Lamminen Date: Tue, 6 Feb 2018 19:22:32 +0200 Subject: [PATCH 11/20] MDLLoader: Replace raw pointer with vector to fix a memory leak --- code/MDLFileData.h | 4 ++-- code/MDLLoader.cpp | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/code/MDLFileData.h b/code/MDLFileData.h index 56f686280..ba732add0 100644 --- a/code/MDLFileData.h +++ b/code/MDLFileData.h @@ -844,11 +844,11 @@ struct IntGroupInfo_MDL7 struct IntGroupData_MDL7 { IntGroupData_MDL7() - : pcFaces(NULL), bNeed2UV(false) + : bNeed2UV(false) {} //! Array of faces that belong to the group - MDL::IntFace_MDL7* pcFaces; + std::vector pcFaces; //! Array of vertex positions std::vector vPositions; diff --git a/code/MDLLoader.cpp b/code/MDLLoader.cpp index 5dd471cf5..3f2bb084b 100644 --- a/code/MDLLoader.cpp +++ b/code/MDLLoader.cpp @@ -1502,7 +1502,7 @@ void MDLImporter::InternReadFile_3DGS_MDL7( ) groupData.bNeed2UV = true; } } - groupData.pcFaces = new MDL::IntFace_MDL7[groupInfo.pcGroup->numtris]; + groupData.pcFaces.resize(groupInfo.pcGroup->numtris); // read all faces into the preallocated arrays ReadFaces_3DGS_MDL7(groupInfo, groupData); From 3b68ffe363401cafd8da701a4a757bc88845e164 Mon Sep 17 00:00:00 2001 From: Turo Lamminen Date: Tue, 6 Feb 2018 19:32:34 +0200 Subject: [PATCH 12/20] LWO: Use C++11 auto for easier refactoring --- code/LWOLoader.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/code/LWOLoader.cpp b/code/LWOLoader.cpp index 38e330b8f..10cfd67bd 100644 --- a/code/LWOLoader.cpp +++ b/code/LWOLoader.cpp @@ -584,7 +584,7 @@ void LWOImporter::GenerateNodeGraph(std::map& apcNodes) //Set parent of all children, inserting pivots //std::cout << "Set parent of all children" << std::endl; std::map mapPivot; - for (std::map::iterator itapcNodes = apcNodes.begin(); itapcNodes != apcNodes.end(); ++itapcNodes) { + for (auto itapcNodes = apcNodes.begin(); itapcNodes != apcNodes.end(); ++itapcNodes) { //Get the parent index LWO::Layer* nodeLayer = (LWO::Layer*)(itapcNodes->second->mParent); @@ -615,14 +615,14 @@ void LWOImporter::GenerateNodeGraph(std::map& apcNodes) //Merge pivot map into node map //std::cout << "Merge pivot map into node map" << std::endl; - for (std::map::iterator itMapPivot = mapPivot.begin(); itMapPivot != mapPivot.end(); ++itMapPivot) { + for (auto itMapPivot = mapPivot.begin(); itMapPivot != mapPivot.end(); ++itMapPivot) { apcNodes[itMapPivot->first] = itMapPivot->second; } //Set children of all parents apcNodes[-1] = root; - for (std::map::iterator itMapParentNodes = apcNodes.begin(); itMapParentNodes != apcNodes.end(); ++itMapParentNodes) { - for (std::map::iterator itMapChildNodes = apcNodes.begin(); itMapChildNodes != apcNodes.end(); ++itMapChildNodes) { + for (auto itMapParentNodes = apcNodes.begin(); itMapParentNodes != apcNodes.end(); ++itMapParentNodes) { + for (auto itMapChildNodes = apcNodes.begin(); itMapChildNodes != apcNodes.end(); ++itMapChildNodes) { if ((itMapParentNodes->first != itMapChildNodes->first) && (itMapParentNodes->second == itMapChildNodes->second->mParent)) { ++(itMapParentNodes->second->mNumChildren); } @@ -630,7 +630,7 @@ void LWOImporter::GenerateNodeGraph(std::map& apcNodes) if (itMapParentNodes->second->mNumChildren) { itMapParentNodes->second->mChildren = new aiNode* [ itMapParentNodes->second->mNumChildren ]; uint16_t p = 0; - for (std::map::iterator itMapChildNodes = apcNodes.begin(); itMapChildNodes != apcNodes.end(); ++itMapChildNodes) { + for (auto itMapChildNodes = apcNodes.begin(); itMapChildNodes != apcNodes.end(); ++itMapChildNodes) { if ((itMapParentNodes->first != itMapChildNodes->first) && (itMapParentNodes->second == itMapChildNodes->second->mParent)) { itMapParentNodes->second->mChildren[p++] = itMapChildNodes->second; } From ef891fb850af5edb86869a1e175ce0e07e13ac02 Mon Sep 17 00:00:00 2001 From: Turo Lamminen Date: Tue, 6 Feb 2018 19:36:27 +0200 Subject: [PATCH 13/20] LWO: Move some assignments to make it clearer when the thing should be moved --- code/LWOLoader.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/code/LWOLoader.cpp b/code/LWOLoader.cpp index 10cfd67bd..e908ea178 100644 --- a/code/LWOLoader.cpp +++ b/code/LWOLoader.cpp @@ -432,7 +432,6 @@ void LWOImporter::InternReadFile( const std::string& pFile, unsigned int num = static_cast(apcMeshes.size() - meshStart); if (layer.mName != "" || num > 0) { aiNode* pcNode = new aiNode(); - apcNodes[layer.mIndex] = pcNode; pcNode->mName.Set(layer.mName); pcNode->mParent = (aiNode*)&layer; pcNode->mNumMeshes = num; @@ -442,6 +441,7 @@ void LWOImporter::InternReadFile( const std::string& pFile, for (unsigned int p = 0; p < pcNode->mNumMeshes;++p) pcNode->mMeshes[p] = p + meshStart; } + apcNodes[layer.mIndex] = pcNode; } } @@ -593,7 +593,6 @@ void LWOImporter::GenerateNodeGraph(std::map& apcNodes) //Create pivot node, store it into the pivot map, and set the parent as the pivot aiNode* pivotNode = new aiNode(); pivotNode->mName.Set("Pivot-"+std::string(itapcNodes->second->mName.data)); - mapPivot[-(itapcNodes->first+2)] = pivotNode; itapcNodes->second->mParent = pivotNode; //Look for the parent node to attach the pivot to @@ -611,6 +610,7 @@ void LWOImporter::GenerateNodeGraph(std::map& apcNodes) pivotNode->mTransformation.a4 = nodeLayer->mPivot.x; pivotNode->mTransformation.b4 = nodeLayer->mPivot.y; pivotNode->mTransformation.c4 = nodeLayer->mPivot.z; + mapPivot[-(itapcNodes->first+2)] = pivotNode; } //Merge pivot map into node map From aa434b956648af35917c9b18933b066eb16171dd Mon Sep 17 00:00:00 2001 From: Turo Lamminen Date: Tue, 6 Feb 2018 20:05:02 +0200 Subject: [PATCH 14/20] OpenGEX: Add comment about pointer ownership --- code/OpenGEXImporter.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/code/OpenGEXImporter.h b/code/OpenGEXImporter.h index ad331d8e6..5b87d5ce5 100644 --- a/code/OpenGEXImporter.h +++ b/code/OpenGEXImporter.h @@ -192,7 +192,7 @@ private: MetricInfo m_metrics[ MetricInfo::Max ]; aiNode *m_currentNode; VertexContainer m_currentVertices; - aiMesh *m_currentMesh; + aiMesh *m_currentMesh; // not owned, target is owned by m_meshCache aiMaterial *m_currentMaterial; aiLight *m_currentLight; aiCamera *m_currentCamera; From 5ce9ece0ccc110bf858bd21a19b10986236bf63e Mon Sep 17 00:00:00 2001 From: Turo Lamminen Date: Tue, 6 Feb 2018 20:08:49 +0200 Subject: [PATCH 15/20] OpenGEX: Replace std::copy with explicit loop --- code/OpenGEXImporter.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/code/OpenGEXImporter.cpp b/code/OpenGEXImporter.cpp index dac8299c5..a0bef75a9 100644 --- a/code/OpenGEXImporter.cpp +++ b/code/OpenGEXImporter.cpp @@ -1137,7 +1137,9 @@ void OpenGEXImporter::copyMeshes( aiScene *pScene ) { pScene->mNumMeshes = static_cast(m_meshCache.size()); pScene->mMeshes = new aiMesh*[ pScene->mNumMeshes ]; - std::copy( m_meshCache.begin(), m_meshCache.end(), pScene->mMeshes ); + for (unsigned int i = 0; i < pScene->mNumMeshes; i++) { + pScene->mMeshes[i] = m_meshCache[i]; + } } //------------------------------------------------------------------------------------------------ From 17b26c91e254a17091f96ab861569f10b408a9c8 Mon Sep 17 00:00:00 2001 From: Turo Lamminen Date: Tue, 6 Feb 2018 20:20:16 +0200 Subject: [PATCH 16/20] OpenGEX: Use std::unique_ptr to fix some memory leaks --- code/OpenGEXImporter.cpp | 5 +++-- code/OpenGEXImporter.h | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/code/OpenGEXImporter.cpp b/code/OpenGEXImporter.cpp index a0bef75a9..43c92c4f5 100644 --- a/code/OpenGEXImporter.cpp +++ b/code/OpenGEXImporter.cpp @@ -691,7 +691,8 @@ void OpenGEXImporter::handleTransformNode( ODDLParser::DDLNode *node, aiScene * void OpenGEXImporter::handleMeshNode( ODDLParser::DDLNode *node, aiScene *pScene ) { m_currentMesh = new aiMesh; const size_t meshidx( m_meshCache.size() ); - m_meshCache.push_back( m_currentMesh ); + // ownership is transfered but a reference remains in m_currentMesh + m_meshCache.emplace_back( m_currentMesh ); Property *prop = node->getProperties(); if( nullptr != prop ) { @@ -1138,7 +1139,7 @@ void OpenGEXImporter::copyMeshes( aiScene *pScene ) { pScene->mNumMeshes = static_cast(m_meshCache.size()); pScene->mMeshes = new aiMesh*[ pScene->mNumMeshes ]; for (unsigned int i = 0; i < pScene->mNumMeshes; i++) { - pScene->mMeshes[i] = m_meshCache[i]; + pScene->mMeshes[i] = m_meshCache[i].release(); } } diff --git a/code/OpenGEXImporter.h b/code/OpenGEXImporter.h index 5b87d5ce5..8e86a4aa8 100644 --- a/code/OpenGEXImporter.h +++ b/code/OpenGEXImporter.h @@ -183,7 +183,7 @@ private: typedef std::map > NodeChildMap; NodeChildMap m_nodeChildMap; - std::vector m_meshCache; + std::vector > m_meshCache; typedef std::map ReferenceMap; std::map m_mesh2refMap; std::map m_material2refMap; From 495ae70cc5423b1cda7c5999c4b44cf9fb9de0cd Mon Sep 17 00:00:00 2001 From: Kim Kulling Date: Tue, 6 Feb 2018 19:21:56 +0100 Subject: [PATCH 17/20] 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 ); +} From 57c1fe5954aa44107e86a7c17b0df2ff9a2cd846 Mon Sep 17 00:00:00 2001 From: Kim Kulling Date: Tue, 6 Feb 2018 23:59:46 +0100 Subject: [PATCH 18/20] x-parser: fix the crash. --- code/XFileImporter.cpp | 341 ++++++++++++++++++----------------------- code/XFileParser.cpp | 1 - 2 files changed, 153 insertions(+), 189 deletions(-) diff --git a/code/XFileImporter.cpp b/code/XFileImporter.cpp index 93ae6173b..fd28fbb79 100644 --- a/code/XFileImporter.cpp +++ b/code/XFileImporter.cpp @@ -5,8 +5,6 @@ Open Asset Import Library (assimp) Copyright (c) 2006-2018, assimp team - - All rights reserved. Redistribution and use of this software in source and binary forms, @@ -44,7 +42,6 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. * @brief Implementation of the XFile importer class */ - #ifndef ASSIMP_BUILD_NO_X_IMPORTER #include "XFileImporter.h" @@ -79,17 +76,19 @@ static const aiImporterDesc desc = { // ------------------------------------------------------------------------------------------------ // Constructor to be privately used by Importer XFileImporter::XFileImporter() -{} +: mBuffer() { + // empty +} // ------------------------------------------------------------------------------------------------ // Destructor, private as well -XFileImporter::~XFileImporter() -{} +XFileImporter::~XFileImporter() { + // empty +} // ------------------------------------------------------------------------------------------------ // Returns whether the class can handle the format of the given file. -bool XFileImporter::CanRead( const std::string& pFile, IOSystem* pIOHandler, bool checkSig) const -{ +bool XFileImporter::CanRead( const std::string& pFile, IOSystem* pIOHandler, bool checkSig) const { std::string extension = GetExtension(pFile); if(extension == "x") { return true; @@ -104,23 +103,24 @@ bool XFileImporter::CanRead( const std::string& pFile, IOSystem* pIOHandler, boo // ------------------------------------------------------------------------------------------------ // Get file extension list -const aiImporterDesc* XFileImporter::GetInfo () const -{ +const aiImporterDesc* XFileImporter::GetInfo () const { return &desc; } // ------------------------------------------------------------------------------------------------ // Imports the given file into the given scene structure. -void XFileImporter::InternReadFile( const std::string& pFile, aiScene* pScene, IOSystem* pIOHandler) -{ +void XFileImporter::InternReadFile( const std::string& pFile, aiScene* pScene, IOSystem* pIOHandler) { // read file into memory std::unique_ptr file( pIOHandler->Open( pFile)); - if( file.get() == NULL) - throw DeadlyImportError( "Failed to open file " + pFile + "."); + if ( file.get() == NULL ) { + throw DeadlyImportError( "Failed to open file " + pFile + "." ); + } + static const size_t MinSize = 16; size_t fileSize = file->FileSize(); - if( fileSize < 16) - throw DeadlyImportError( "XFile is too small."); + if ( fileSize < MinSize ) { + throw DeadlyImportError( "XFile is too small." ); + } // in the hope that binary files will never start with a BOM ... mBuffer.resize( fileSize + 1); @@ -134,8 +134,9 @@ void XFileImporter::InternReadFile( const std::string& pFile, aiScene* pScene, I CreateDataRepresentationFromImport( pScene, parser.GetImportedData()); // if nothing came from it, report it as error - if( !pScene->mRootNode) - throw DeadlyImportError( "XFile is ill-formatted - no content imported."); + if ( !pScene->mRootNode ) { + throw DeadlyImportError( "XFile is ill-formatted - no content imported." ); + } } // ------------------------------------------------------------------------------------------------ @@ -146,17 +147,15 @@ void XFileImporter::CreateDataRepresentationFromImport( aiScene* pScene, XFile:: ConvertMaterials( pScene, pData->mGlobalMaterials); // copy nodes, extracting meshes and materials on the way - pScene->mRootNode = CreateNodes( pScene, NULL, pData->mRootNode); + pScene->mRootNode = CreateNodes( pScene, nullptr, pData->mRootNode); // extract animations CreateAnimations( pScene, pData); // read the global meshes that were stored outside of any node - if( pData->mGlobalMeshes.size() > 0) - { + if( !pData->mGlobalMeshes.empty() ) { // create a root node to hold them if there isn't any, yet - if( pScene->mRootNode == NULL) - { + if( pScene->mRootNode == nullptr ) { pScene->mRootNode = new aiNode; pScene->mRootNode->mName.Set( "$dummy_node"); } @@ -180,8 +179,7 @@ void XFileImporter::CreateDataRepresentationFromImport( aiScene* pScene, XFile:: flipper.Execute(pScene); // finally: create a dummy material if not material was imported - if( pScene->mNumMaterials == 0) - { + if( pScene->mNumMaterials == 0) { pScene->mNumMaterials = 1; // create the Material aiMaterial* mat = new aiMaterial; @@ -205,10 +203,10 @@ void XFileImporter::CreateDataRepresentationFromImport( aiScene* pScene, XFile:: // ------------------------------------------------------------------------------------------------ // Recursively creates scene nodes from the imported hierarchy. -aiNode* XFileImporter::CreateNodes( aiScene* pScene, aiNode* pParent, const XFile::Node* pNode) -{ - if( !pNode) - return NULL; +aiNode* XFileImporter::CreateNodes( aiScene* pScene, aiNode* pParent, const XFile::Node* pNode) { + if ( !pNode ) { + return nullptr; + } // create node aiNode* node = new aiNode; @@ -222,13 +220,13 @@ aiNode* XFileImporter::CreateNodes( aiScene* pScene, aiNode* pParent, const XFil CreateMeshes( pScene, node, pNode->mMeshes); // handle childs - if( pNode->mChildren.size() > 0) - { + if( !pNode->mChildren.empty() ) { node->mNumChildren = (unsigned int)pNode->mChildren.size(); node->mChildren = new aiNode* [node->mNumChildren]; - for( unsigned int a = 0; a < pNode->mChildren.size(); a++) - node->mChildren[a] = CreateNodes( pScene, node, pNode->mChildren[a]); + for ( unsigned int a = 0; a < pNode->mChildren.size(); ++a ) { + node->mChildren[ a ] = CreateNodes( pScene, node, pNode->mChildren[ a ] ); + } } return node; @@ -236,16 +234,14 @@ aiNode* XFileImporter::CreateNodes( aiScene* pScene, aiNode* pParent, const XFil // ------------------------------------------------------------------------------------------------ // Creates the meshes for the given node. -void XFileImporter::CreateMeshes( aiScene* pScene, aiNode* pNode, const std::vector& pMeshes) -{ +void XFileImporter::CreateMeshes( aiScene* pScene, aiNode* pNode, const std::vector& pMeshes) { if (pMeshes.empty()) { return; } // create a mesh for each mesh-material combination in the source node std::vector meshes; - for( unsigned int a = 0; a < pMeshes.size(); a++) - { + for( unsigned int a = 0; a < pMeshes.size(); ++a ) { XFile::Mesh* sourceMesh = pMeshes[a]; if ( nullptr == sourceMesh ) { continue; @@ -255,35 +251,30 @@ void XFileImporter::CreateMeshes( aiScene* pScene, aiNode* pNode, const std::vec ConvertMaterials( pScene, sourceMesh->mMaterials); unsigned int numMaterials = std::max( (unsigned int)sourceMesh->mMaterials.size(), 1u); - for( unsigned int b = 0; b < numMaterials; b++) - { + for( unsigned int b = 0; b < numMaterials; ++b ) { // collect the faces belonging to this material std::vector faces; unsigned int numVertices = 0; - if( sourceMesh->mFaceMaterials.size() > 0) - { + if( !sourceMesh->mFaceMaterials.empty() ) { // if there is a per-face material defined, select the faces with the corresponding material - for( unsigned int c = 0; c < sourceMesh->mFaceMaterials.size(); c++) - { - if( sourceMesh->mFaceMaterials[c] == b) - { + for( unsigned int c = 0; c < sourceMesh->mFaceMaterials.size(); ++c ) { + if( sourceMesh->mFaceMaterials[c] == b) { faces.push_back( c); numVertices += (unsigned int)sourceMesh->mPosFaces[c].mIndices.size(); } } - } else - { + } else { // if there is no per-face material, place everything into one mesh - for( unsigned int c = 0; c < sourceMesh->mPosFaces.size(); c++) - { + for( unsigned int c = 0; c < sourceMesh->mPosFaces.size(); ++c ) { faces.push_back( c); numVertices += (unsigned int)sourceMesh->mPosFaces[c].mIndices.size(); } } // no faces/vertices using this material? strange... - if( numVertices == 0) + if ( numVertices == 0 ) { continue; + } // create a submesh using this material aiMesh* mesh = new aiMesh; @@ -291,11 +282,9 @@ void XFileImporter::CreateMeshes( aiScene* pScene, aiNode* pNode, const std::vec // find the material in the scene's material list. Either own material // or referenced material, it should already have a valid index - if( sourceMesh->mFaceMaterials.size() > 0) - { + if( !sourceMesh->mFaceMaterials.empty() ) { mesh->mMaterialIndex = static_cast(sourceMesh->mMaterials[b].sceneIndex); - } else - { + } else { mesh->mMaterialIndex = 0; } @@ -310,28 +299,28 @@ void XFileImporter::CreateMeshes( aiScene* pScene, aiNode* pNode, const std::vec mesh->mName.Set(sourceMesh->mName); // normals? - if( sourceMesh->mNormals.size() > 0) - mesh->mNormals = new aiVector3D[numVertices]; + if ( sourceMesh->mNormals.size() > 0 ) { + mesh->mNormals = new aiVector3D[ numVertices ]; + } // texture coords - for( unsigned int c = 0; c < AI_MAX_NUMBER_OF_TEXTURECOORDS; c++) - { - if( sourceMesh->mTexCoords[c].size() > 0) - mesh->mTextureCoords[c] = new aiVector3D[numVertices]; + for( unsigned int c = 0; c < AI_MAX_NUMBER_OF_TEXTURECOORDS; ++c ) { + if ( !sourceMesh->mTexCoords[ c ].empty() ) { + mesh->mTextureCoords[ c ] = new aiVector3D[ numVertices ]; + } } // vertex colors - for( unsigned int c = 0; c < AI_MAX_NUMBER_OF_COLOR_SETS; c++) - { - if( sourceMesh->mColors[c].size() > 0) - mesh->mColors[c] = new aiColor4D[numVertices]; + for( unsigned int c = 0; c < AI_MAX_NUMBER_OF_COLOR_SETS; ++c ) { + if ( !sourceMesh->mColors[ c ].empty() ) { + mesh->mColors[ c ] = new aiColor4D[ numVertices ]; + } } // now collect the vertex data of all data streams present in the imported mesh - unsigned int newIndex = 0; + unsigned int newIndex( 0 ); std::vector orgPoints; // from which original point each new vertex stems orgPoints.resize( numVertices, 0); - for( unsigned int c = 0; c < faces.size(); c++) - { + for( unsigned int c = 0; c < faces.size(); ++c ) { unsigned int f = faces[c]; // index of the source face const XFile::Face& pf = sourceMesh->mPosFaces[f]; // position source face @@ -341,30 +330,30 @@ void XFileImporter::CreateMeshes( aiScene* pScene, aiNode* pNode, const std::vec df.mIndices = new unsigned int[ df.mNumIndices]; // collect vertex data for indices of this face - for( unsigned int d = 0; d < df.mNumIndices; d++) - { + for( unsigned int d = 0; d < df.mNumIndices; ++d ) { df.mIndices[d] = newIndex; orgPoints[newIndex] = pf.mIndices[d]; // Position mesh->mVertices[newIndex] = sourceMesh->mPositions[pf.mIndices[d]]; // Normal, if present - if( mesh->HasNormals()) - mesh->mNormals[newIndex] = sourceMesh->mNormals[sourceMesh->mNormFaces[f].mIndices[d]]; + if ( mesh->HasNormals() ) { + mesh->mNormals[ newIndex ] = sourceMesh->mNormals[ sourceMesh->mNormFaces[ f ].mIndices[ d ] ]; + } // texture coord sets - for( unsigned int e = 0; e < AI_MAX_NUMBER_OF_TEXTURECOORDS; e++) - { - if( mesh->HasTextureCoords( e)) - { + for( unsigned int e = 0; e < AI_MAX_NUMBER_OF_TEXTURECOORDS; ++e ) { + if( mesh->HasTextureCoords( e)) { aiVector2D tex = sourceMesh->mTexCoords[e][pf.mIndices[d]]; mesh->mTextureCoords[e][newIndex] = aiVector3D( tex.x, 1.0f - tex.y, 0.0f); } } // vertex color sets - for( unsigned int e = 0; e < AI_MAX_NUMBER_OF_COLOR_SETS; e++) - if( mesh->HasVertexColors( e)) - mesh->mColors[e][newIndex] = sourceMesh->mColors[e][pf.mIndices[d]]; + for ( unsigned int e = 0; e < AI_MAX_NUMBER_OF_COLOR_SETS; ++e ) { + if ( mesh->HasVertexColors( e ) ) { + mesh->mColors[ e ][ newIndex ] = sourceMesh->mColors[ e ][ pf.mIndices[ d ] ]; + } + } newIndex++; } @@ -376,28 +365,29 @@ void XFileImporter::CreateMeshes( aiScene* pScene, aiNode* pNode, const std::vec // convert all bones of the source mesh which influence vertices in this newly created mesh const std::vector& bones = sourceMesh->mBones; std::vector newBones; - for( unsigned int c = 0; c < bones.size(); c++) - { + for( unsigned int c = 0; c < bones.size(); ++c ) { const XFile::Bone& obone = bones[c]; // set up a vertex-linear array of the weights for quick searching if a bone influences a vertex std::vector oldWeights( sourceMesh->mPositions.size(), 0.0); - for( unsigned int d = 0; d < obone.mWeights.size(); d++) - oldWeights[obone.mWeights[d].mVertex] = obone.mWeights[d].mWeight; + for ( unsigned int d = 0; d < obone.mWeights.size(); ++d ) { + oldWeights[ obone.mWeights[ d ].mVertex ] = obone.mWeights[ d ].mWeight; + } // collect all vertex weights that influence a vertex in the new mesh std::vector newWeights; newWeights.reserve( numVertices); - for( unsigned int d = 0; d < orgPoints.size(); d++) - { + for( unsigned int d = 0; d < orgPoints.size(); ++d ) { // does the new vertex stem from an old vertex which was influenced by this bone? ai_real w = oldWeights[orgPoints[d]]; - if( w > 0.0) - newWeights.push_back( aiVertexWeight( d, w)); + if ( w > 0.0 ) { + newWeights.push_back( aiVertexWeight( d, w ) ); + } } // if the bone has no weights in the newly created mesh, ignore it - if( newWeights.size() == 0) + if ( newWeights.empty() ) { continue; + } // create aiBone* nbone = new aiBone; @@ -407,14 +397,14 @@ void XFileImporter::CreateMeshes( aiScene* pScene, aiNode* pNode, const std::vec nbone->mOffsetMatrix = obone.mOffsetMatrix; nbone->mNumWeights = (unsigned int)newWeights.size(); nbone->mWeights = new aiVertexWeight[nbone->mNumWeights]; - for( unsigned int d = 0; d < newWeights.size(); d++) - nbone->mWeights[d] = newWeights[d]; + for ( unsigned int d = 0; d < newWeights.size(); ++d ) { + nbone->mWeights[ d ] = newWeights[ d ]; + } } // store the bones in the mesh mesh->mNumBones = (unsigned int)newBones.size(); - if( newBones.size() > 0) - { + if( !newBones.empty()) { mesh->mBones = new aiBone*[mesh->mNumBones]; std::copy( newBones.begin(), newBones.end(), mesh->mBones); } @@ -424,8 +414,7 @@ void XFileImporter::CreateMeshes( aiScene* pScene, aiNode* pNode, const std::vec // reallocate scene mesh array to be large enough aiMesh** prevArray = pScene->mMeshes; pScene->mMeshes = new aiMesh*[pScene->mNumMeshes + meshes.size()]; - if( prevArray) - { + if( prevArray) { memcpy( pScene->mMeshes, prevArray, pScene->mNumMeshes * sizeof( aiMesh*)); delete [] prevArray; } @@ -435,8 +424,7 @@ void XFileImporter::CreateMeshes( aiScene* pScene, aiNode* pNode, const std::vec pNode->mMeshes = new unsigned int[pNode->mNumMeshes]; // store all meshes in the mesh library of the scene and store their indices in the node - for( unsigned int a = 0; a < meshes.size(); a++) - { + for( unsigned int a = 0; a < meshes.size(); a++) { pScene->mMeshes[pScene->mNumMeshes] = meshes[a]; pNode->mMeshes[a] = pScene->mNumMeshes; pScene->mNumMeshes++; @@ -445,16 +433,15 @@ void XFileImporter::CreateMeshes( aiScene* pScene, aiNode* pNode, const std::vec // ------------------------------------------------------------------------------------------------ // Converts the animations from the given imported data and creates them in the scene. -void XFileImporter::CreateAnimations( aiScene* pScene, const XFile::Scene* pData) -{ +void XFileImporter::CreateAnimations( aiScene* pScene, const XFile::Scene* pData) { std::vector newAnims; - for( unsigned int a = 0; a < pData->mAnims.size(); a++) - { + for( unsigned int a = 0; a < pData->mAnims.size(); ++a ) { const XFile::Animation* anim = pData->mAnims[a]; // some exporters mock me with empty animation tags. - if( anim->mAnims.size() == 0) + if ( anim->mAnims.empty() ) { continue; + } // create a new animation to hold the data aiAnimation* nanim = new aiAnimation; @@ -466,15 +453,14 @@ void XFileImporter::CreateAnimations( aiScene* pScene, const XFile::Scene* pData nanim->mNumChannels = (unsigned int)anim->mAnims.size(); nanim->mChannels = new aiNodeAnim*[nanim->mNumChannels]; - for( unsigned int b = 0; b < anim->mAnims.size(); b++) - { + for( unsigned int b = 0; b < anim->mAnims.size(); ++b ) { const XFile::AnimBone* bone = anim->mAnims[b]; aiNodeAnim* nbone = new aiNodeAnim; nbone->mNodeName.Set( bone->mBoneName); nanim->mChannels[b] = nbone; // keyframes are given as combined transformation matrix keys - if( bone->mTrafoKeys.size() > 0) + if( !bone->mTrafoKeys.empty() ) { nbone->mNumPositionKeys = (unsigned int)bone->mTrafoKeys.size(); nbone->mPositionKeys = new aiVectorKey[nbone->mNumPositionKeys]; @@ -483,8 +469,7 @@ void XFileImporter::CreateAnimations( aiScene* pScene, const XFile::Scene* pData nbone->mNumScalingKeys = (unsigned int)bone->mTrafoKeys.size(); nbone->mScalingKeys = new aiVectorKey[nbone->mNumScalingKeys]; - for( unsigned int c = 0; c < bone->mTrafoKeys.size(); c++) - { + for( unsigned int c = 0; c < bone->mTrafoKeys.size(); ++c) { // deconstruct each matrix into separate position, rotation and scaling double time = bone->mTrafoKeys[c].mTime; aiMatrix4x4 trafo = bone->mTrafoKeys[c].mMatrix; @@ -516,13 +501,11 @@ void XFileImporter::CreateAnimations( aiScene* pScene, const XFile::Scene* pData // longest lasting key sequence determines duration nanim->mDuration = std::max( nanim->mDuration, bone->mTrafoKeys.back().mTime); - } else - { + } else { // separate key sequences for position, rotation, scaling nbone->mNumPositionKeys = (unsigned int)bone->mPosKeys.size(); nbone->mPositionKeys = new aiVectorKey[nbone->mNumPositionKeys]; - for( unsigned int c = 0; c < nbone->mNumPositionKeys; c++) - { + for( unsigned int c = 0; c < nbone->mNumPositionKeys; ++c ) { aiVector3D pos = bone->mPosKeys[c].mValue; nbone->mPositionKeys[c].mTime = bone->mPosKeys[c].mTime; @@ -532,8 +515,7 @@ void XFileImporter::CreateAnimations( aiScene* pScene, const XFile::Scene* pData // rotation nbone->mNumRotationKeys = (unsigned int)bone->mRotKeys.size(); nbone->mRotationKeys = new aiQuatKey[nbone->mNumRotationKeys]; - for( unsigned int c = 0; c < nbone->mNumRotationKeys; c++) - { + for( unsigned int c = 0; c < nbone->mNumRotationKeys; ++c ) { aiMatrix3x3 rotmat = bone->mRotKeys[c].mValue.GetMatrix(); nbone->mRotationKeys[c].mTime = bone->mRotKeys[c].mTime; @@ -573,56 +555,51 @@ void XFileImporter::CreateAnimations( aiScene* pScene, const XFile::Scene* pData void XFileImporter::ConvertMaterials( aiScene* pScene, std::vector& pMaterials) { // count the non-referrer materials in the array - unsigned int numNewMaterials = 0; - for( unsigned int a = 0; a < pMaterials.size(); a++) - if( !pMaterials[a].mIsReference) - numNewMaterials++; + unsigned int numNewMaterials( 0 ); + for ( unsigned int a = 0; a < pMaterials.size(); ++a ) { + if ( !pMaterials[ a ].mIsReference ) { + ++numNewMaterials; + } + } // resize the scene's material list to offer enough space for the new materials - if( numNewMaterials > 0 ) - { - aiMaterial** prevMats = pScene->mMaterials; - pScene->mMaterials = new aiMaterial*[pScene->mNumMaterials + numNewMaterials]; - if( prevMats) - { - memcpy( pScene->mMaterials, prevMats, pScene->mNumMaterials * sizeof( aiMaterial*)); - delete [] prevMats; - } - } + if( numNewMaterials > 0 ) { + aiMaterial** prevMats = pScene->mMaterials; + pScene->mMaterials = new aiMaterial*[pScene->mNumMaterials + numNewMaterials]; + if( nullptr != prevMats) { + ::memcpy( pScene->mMaterials, prevMats, pScene->mNumMaterials * sizeof( aiMaterial*)); + delete [] prevMats; + } + } // convert all the materials given in the array - for( unsigned int a = 0; a < pMaterials.size(); a++) - { + for( unsigned int a = 0; a < pMaterials.size(); ++a ) { XFile::Material& oldMat = pMaterials[a]; - if( oldMat.mIsReference) - { - // find the material it refers to by name, and store its index - for( size_t a = 0; a < pScene->mNumMaterials; ++a ) - { - aiString name; - pScene->mMaterials[a]->Get( AI_MATKEY_NAME, name); - if( strcmp( name.C_Str(), oldMat.mName.data()) == 0 ) - { - oldMat.sceneIndex = a; - break; + if( oldMat.mIsReference) { + // find the material it refers to by name, and store its index + for( size_t a = 0; a < pScene->mNumMaterials; ++a ) { + aiString name; + pScene->mMaterials[a]->Get( AI_MATKEY_NAME, name); + if( strcmp( name.C_Str(), oldMat.mName.data()) == 0 ) { + oldMat.sceneIndex = a; + break; + } + } + + if( oldMat.sceneIndex == SIZE_MAX ) { + DefaultLogger::get()->warn( format() << "Could not resolve global material reference \"" << oldMat.mName << "\"" ); + oldMat.sceneIndex = 0; + } + + continue; } - } - - if( oldMat.sceneIndex == SIZE_MAX ) - { - DefaultLogger::get()->warn( format() << "Could not resolve global material reference \"" << oldMat.mName << "\"" ); - oldMat.sceneIndex = 0; - } - - continue; - } aiMaterial* mat = new aiMaterial; aiString name; name.Set( oldMat.mName); mat->AddProperty( &name, AI_MATKEY_NAME); - // Shading model: hardcoded to PHONG, there is no such information in an XFile + // Shading model: hard-coded to PHONG, there is no such information in an XFile // FIX (aramis): If the specular exponent is 0, use gouraud shading. This is a bugfix // for some models in the SDK (e.g. good old tiny.x) int shadeMode = (int)oldMat.mSpecularExponent == 0.0f @@ -630,8 +607,8 @@ void XFileImporter::ConvertMaterials( aiScene* pScene, std::vectorAddProperty( &shadeMode, 1, AI_MATKEY_SHADING_MODEL); // material colours - // Unclear: there's no ambient colour, but emissive. What to put for ambient? - // Probably nothing at all, let the user select a suitable default. + // Unclear: there's no ambient colour, but emissive. What to put for ambient? + // Probably nothing at all, let the user select a suitable default. mat->AddProperty( &oldMat.mEmissive, 1, AI_MATKEY_COLOR_EMISSIVE); mat->AddProperty( &oldMat.mDiffuse, 1, AI_MATKEY_COLOR_DIFFUSE); mat->AddProperty( &oldMat.mSpecular, 1, AI_MATKEY_COLOR_SPECULAR); @@ -639,36 +616,33 @@ void XFileImporter::ConvertMaterials( aiScene* pScene, std::vectorAddProperty( &tex, AI_MATKEY_TEXTURE_NORMALS(0)); - else - mat->AddProperty( &tex, AI_MATKEY_TEXTURE_DIFFUSE(0)); + if ( otex.mIsNormalMap ) { + mat->AddProperty( &tex, AI_MATKEY_TEXTURE_NORMALS( 0 ) ); + } else { + mat->AddProperty( &tex, AI_MATKEY_TEXTURE_DIFFUSE( 0 ) ); + } } - } - else - { + } else { // Otherwise ... try to search for typical strings in the // texture's file name like 'bump' or 'diffuse' unsigned int iHM = 0,iNM = 0,iDM = 0,iSM = 0,iAM = 0,iEM = 0; - for( unsigned int b = 0; b < oldMat.mTextures.size(); b++) - { + for( unsigned int b = 0; b < oldMat.mTextures.size(); ++b ) { const XFile::TexEntry& otex = oldMat.mTextures[b]; std::string sz = otex.mName; - if (!sz.length())continue; - + if ( !sz.length() ) { + continue; + } // find the file name - //const size_t iLen = sz.length(); std::string::size_type s = sz.find_last_of("\\/"); - if (std::string::npos == s) + if ( std::string::npos == s ) { s = 0; + } // cut off the file extension std::string::size_type sExt = sz.find_last_of('.'); @@ -677,36 +651,27 @@ void XFileImporter::ConvertMaterials( aiScene* pScene, std::vectorAddProperty( &tex, AI_MATKEY_TEXTURE_HEIGHT(iHM++)); - } else - if (otex.mIsNormalMap || std::string::npos != sz.find( "normal", s) || std::string::npos != sz.find("nm", s)) - { + } else if (otex.mIsNormalMap || std::string::npos != sz.find( "normal", s) || std::string::npos != sz.find("nm", s)) { mat->AddProperty( &tex, AI_MATKEY_TEXTURE_NORMALS(iNM++)); - } else - if (std::string::npos != sz.find( "spec", s) || std::string::npos != sz.find( "glanz", s)) - { + } else if (std::string::npos != sz.find( "spec", s) || std::string::npos != sz.find( "glanz", s)) { mat->AddProperty( &tex, AI_MATKEY_TEXTURE_SPECULAR(iSM++)); - } else - if (std::string::npos != sz.find( "ambi", s) || std::string::npos != sz.find( "env", s)) - { + } else if (std::string::npos != sz.find( "ambi", s) || std::string::npos != sz.find( "env", s)) { mat->AddProperty( &tex, AI_MATKEY_TEXTURE_AMBIENT(iAM++)); - } else - if (std::string::npos != sz.find( "emissive", s) || std::string::npos != sz.find( "self", s)) - { + } else if (std::string::npos != sz.find( "emissive", s) || std::string::npos != sz.find( "self", s)) { mat->AddProperty( &tex, AI_MATKEY_TEXTURE_EMISSIVE(iEM++)); - } else - { + } else { // Assume it is a diffuse texture mat->AddProperty( &tex, AI_MATKEY_TEXTURE_DIFFUSE(iDM++)); } diff --git a/code/XFileParser.cpp b/code/XFileParser.cpp index 4017ed596..bda59ba59 100644 --- a/code/XFileParser.cpp +++ b/code/XFileParser.cpp @@ -1460,7 +1460,6 @@ aiColor3D XFileParser::ReadRGB() // ------------------------------------------------------------------------------------------------ // Throws an exception with a line number and the given text. AI_WONT_RETURN void XFileParser::ThrowException( const std::string& pText) { - delete mScene; if ( mIsBinaryFormat ) { throw DeadlyImportError( pText ); } else { From 990dc983ed2634bc764e6c4c1e6d63499eff6d58 Mon Sep 17 00:00:00 2001 From: Max Vollmer Date: Wed, 7 Feb 2018 10:48:39 +0100 Subject: [PATCH 19/20] Issue #1776 Fixed potential crash bug in ObjectCompare, because it didn't follow strict weak ordering. As counter-intuitive as it seems, a comparator must return false for equal values. The C++ standard defines and expects this behavior: true if lhs < rhs, false otherwise. --- code/BlenderIntermediate.h | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/code/BlenderIntermediate.h b/code/BlenderIntermediate.h index 74184a1f1..f3d34d1b2 100644 --- a/code/BlenderIntermediate.h +++ b/code/BlenderIntermediate.h @@ -122,9 +122,11 @@ namespace Blender { # pragma warning(disable:4351) #endif + // As counter-intuitive as it may seem, a comparator must return false for equal values. + // The C++ standard defines and expects this behavior: true if lhs < rhs, false otherwise. struct ObjectCompare { bool operator() (const Object* left, const Object* right) const { - return ::strncmp(left->id.name, right->id.name, strlen( left->id.name ) ) == 0; + return ::strncmp(left->id.name, right->id.name, strlen( left->id.name ) ) < 0; } }; @@ -143,9 +145,11 @@ namespace Blender { , db(db) {} + // As counter-intuitive as it may seem, a comparator must return false for equal values. + // The C++ standard defines and expects this behavior: true if lhs < rhs, false otherwise. struct ObjectCompare { bool operator() (const Object* left, const Object* right) const { - return ::strncmp( left->id.name, right->id.name, strlen( left->id.name ) ) == 0; + return ::strncmp( left->id.name, right->id.name, strlen( left->id.name ) ) < 0; } }; From b3d48d0e9a5e815c2a4acc7662736bb902a362cd Mon Sep 17 00:00:00 2001 From: Max Vollmer Date: Wed, 7 Feb 2018 11:02:08 +0100 Subject: [PATCH 20/20] Issue #1776: Updated and fixed test in utBlenderIntermediate.cpp for Blender::ObjectCompare --- test/unit/utBlenderIntermediate.cpp | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/test/unit/utBlenderIntermediate.cpp b/test/unit/utBlenderIntermediate.cpp index 9146d6d1e..55490e613 100644 --- a/test/unit/utBlenderIntermediate.cpp +++ b/test/unit/utBlenderIntermediate.cpp @@ -57,17 +57,26 @@ class BlenderIntermediateTest : public ::testing::Test { #define NAME_1 "name1" #define NAME_2 "name2" +// Updated this test after fixing #1776: +// A comparator in C++ is used for ordering and must implement strict weak ordering, +// which means it must return false for equal values. +// The C++ standard defines and expects this behavior: true if lhs < rhs, false otherwise. TEST_F( BlenderIntermediateTest,ConversionData_ObjectCompareTest ) { Object obj1, obj2; strncpy( obj1.id.name, NAME_1, sizeof(NAME_1) ); strncpy( obj2.id.name, NAME_2, sizeof(NAME_2) ); - Blender::ObjectCompare cmp_false; - bool res( cmp_false( &obj1, &obj2 ) ); + + Blender::ObjectCompare cmp_true_because_first_is_smaller_than_second; + bool res( cmp_true_because_first_is_smaller_than_second( &obj1, &obj2 ) ); + EXPECT_TRUE( res ); + + Blender::ObjectCompare cmp_false_because_equal; + res = cmp_false_because_equal( &obj1, &obj1 ); EXPECT_FALSE( res ); - Blender::ObjectCompare cmp_true; - res = cmp_true( &obj1, &obj1 ); - EXPECT_TRUE( res ); + Blender::ObjectCompare cmp_false_because_first_is_greater_than_second; + res = cmp_false_because_first_is_greater_than_second( &obj2, &obj1 ); + EXPECT_FALSE( res ); }