From 69810a2a07db62606596c73eea969bf7c3e1db6f Mon Sep 17 00:00:00 2001 From: Jared Duke Date: Sun, 17 Aug 2014 08:41:45 -0700 Subject: [PATCH] Avoid raw reinterpret_casts in the FBX parser As reinterpret_cast can break strict aliasing rules, causing runtime failure on Android, replace such usage in FBXParser with memcpy. Also provide a utility routine for both performing the copy and asserting the validity of the buffer length relative to the copied region. --- code/FBXParser.cpp | 52 +++++++++++++++++++--------------------------- 1 file changed, 21 insertions(+), 31 deletions(-) diff --git a/code/FBXParser.cpp b/code/FBXParser.cpp index f6dc2e7e2..bc0458ff6 100644 --- a/code/FBXParser.cpp +++ b/code/FBXParser.cpp @@ -113,6 +113,18 @@ namespace { ParseError(message); } + // Initially, we did reinterpret_cast, breaking strict aliasing rules. + // This actually caused trouble on Android, so let's be safe this time. + // https://github.com/assimp/assimp/issues/24 + template + T SafeParse(const char* data, const char* end) { + // Actual size validation happens during Tokenization so + // this is valid as an assertion. + ai_assert(static_cast(end - data) >= sizeof(T)); + T result = static_cast(0); + ::memcpy(&result, data, sizeof(T)); + return result; + } } namespace Assimp { @@ -275,9 +287,7 @@ uint64_t ParseTokenAsID(const Token& t, const char*& err_out) return 0L; } - ai_assert(t.end() - data == 9); - - BE_NCONST uint64_t id = *reinterpret_cast(data+1); + BE_NCONST uint64_t id = SafeParse(data+1, t.end()); AI_SWAP8(id); return id; } @@ -316,8 +326,7 @@ size_t ParseTokenAsDim(const Token& t, const char*& err_out) return 0; } - ai_assert(t.end() - data == 9); - BE_NCONST uint64_t id = *reinterpret_cast(data+1); + BE_NCONST uint64_t id = SafeParse(data+1, t.end()); AI_SWAP8(id); return static_cast(id); } @@ -364,24 +373,10 @@ float ParseTokenAsFloat(const Token& t, const char*& err_out) } if (data[0] == 'F') { - // Actual size validation happens during Tokenization so - // this is valid as an assertion. - ai_assert(t.end() - data == sizeof(float) + 1); - // Initially, we did reinterpret_cast, breaking strict aliasing rules. - // This actually caused trouble on Android, so let's be safe this time. - // https://github.com/assimp/assimp/issues/24 - - float out_float; - ::memcpy(&out_float, data+1, sizeof(float)); - return out_float; + return SafeParse(data+1, t.end()); } else { - ai_assert(t.end() - data == sizeof(double) + 1); - - // Same - double out_double; - ::memcpy(&out_double, data+1, sizeof(double)); - return static_cast(out_double); + return SafeParse(data+1, t.end()); } } @@ -416,8 +411,7 @@ int ParseTokenAsInt(const Token& t, const char*& err_out) return 0; } - ai_assert(t.end() - data == 5); - BE_NCONST int32_t ival = *reinterpret_cast(data+1); + BE_NCONST int32_t ival = SafeParse(data+1, t.end()); AI_SWAP4(ival); return static_cast(ival); } @@ -453,10 +447,8 @@ std::string ParseTokenAsString(const Token& t, const char*& err_out) return ""; } - ai_assert(t.end() - data >= 5); - // read string length - BE_NCONST int32_t len = *reinterpret_cast(data+1); + BE_NCONST int32_t len = SafeParse(data+1, t.end()); AI_SWAP4(len); ai_assert(t.end() - data == 5 + len); @@ -494,7 +486,7 @@ void ReadBinaryDataArrayHead(const char*& data, const char* end, char& type, uin type = *data; // read number of elements - BE_NCONST uint32_t len = *reinterpret_cast(data+1); + BE_NCONST uint32_t len = SafeParse(data+1, end); AI_SWAP4(len); count = len; @@ -508,14 +500,12 @@ void ReadBinaryDataArray(char type, uint32_t count, const char*& data, const cha std::vector& buff, const Element& el) { - ai_assert(static_cast(end-data) >= 4); // runtime check for this happens at tokenization stage - - BE_NCONST uint32_t encmode = *reinterpret_cast(data); + BE_NCONST uint32_t encmode = SafeParse(data, end); AI_SWAP4(encmode); data += 4; // next comes the compressed length - BE_NCONST uint32_t comp_len = *reinterpret_cast(data); + BE_NCONST uint32_t comp_len = SafeParse(data, end); AI_SWAP4(comp_len); data += 4;