From 053c0f5e0a54b167dd77ab4509b47430a3422ab1 Mon Sep 17 00:00:00 2001 From: Malcolm Tyrrell Date: Mon, 15 Mar 2021 13:06:11 +0000 Subject: [PATCH 1/5] Catch type errors in gltf and try to print out useful context. --- code/AssetLib/glTF2/glTF2Asset.h | 12 ++ code/AssetLib/glTF2/glTF2Asset.inl | 155 ++++++++++++++++++----- code/AssetLib/glTF2/glTF2AssetWriter.inl | 10 +- 3 files changed, 142 insertions(+), 35 deletions(-) diff --git a/code/AssetLib/glTF2/glTF2Asset.h b/code/AssetLib/glTF2/glTF2Asset.h index d75251cb2..0e4ba6eda 100644 --- a/code/AssetLib/glTF2/glTF2Asset.h +++ b/code/AssetLib/glTF2/glTF2Asset.h @@ -370,6 +370,13 @@ struct Object { //! Maps special IDs to another ID, where needed. Subclasses may override it (statically) static const char *TranslateId(Asset & /*r*/, const char *id) { return id; } + + inline Value *FindString(Value &val, const char *id); + inline Value *FindNumber(Value &val, const char *id); + inline Value *FindUInt(Value &val, const char *id); + inline Value *FindArray(Value &val, const char *id); + inline Value *FindObject(Value &val, const char *id); + inline Value *FindExtension(Value &val, const char *extensionId); }; // @@ -780,6 +787,11 @@ struct Material : public Object { Material() { SetDefaults(); } void Read(Value &obj, Asset &r); void SetDefaults(); + + inline void SetTextureProperties(Asset &r, Value *prop, TextureInfo &out); + inline void ReadTextureProperty(Asset &r, Value &vals, const char *propName, TextureInfo &out); + inline void ReadTextureProperty(Asset &r, Value &vals, const char *propName, NormalTextureInfo &out); + inline void ReadTextureProperty(Asset &r, Value &vals, const char *propName, OcclusionTextureInfo &out); }; //! A set of primitives to be rendered. A node can contain one or more meshes. A node's transform places the mesh in the scene. diff --git a/code/AssetLib/glTF2/glTF2Asset.inl b/code/AssetLib/glTF2/glTF2Asset.inl index 389a05199..66b39ee57 100644 --- a/code/AssetLib/glTF2/glTF2Asset.inl +++ b/code/AssetLib/glTF2/glTF2Asset.inl @@ -180,41 +180,133 @@ inline Value *FindMember(Value &val, const char *id) { return (it != val.MemberEnd()) ? &it->value : nullptr; } -inline Value *FindString(Value &val, const char *id) { - Value::MemberIterator it = val.FindMember(id); - return (it != val.MemberEnd() && it->value.IsString()) ? &it->value : nullptr; +template +inline void throwUnexpectedTypeError(const char (&expectedTypeName)[N], const char* memberId, const char* context, const char* extraContext) { + std::string fullContext = context; + if (extraContext && (strlen(extraContext) > 0)) + { + fullContext += " (", extraContext, ")"; + } + throw DeadlyImportError("Member \"", memberId, "\" was not of type \"", expectedTypeName, "\" when reading ", fullContext); } -inline Value *FindNumber(Value &val, const char *id) { - Value::MemberIterator it = val.FindMember(id); - return (it != val.MemberEnd() && it->value.IsNumber()) ? &it->value : nullptr; +// Look-up functions with type checks. Context and extra context help the user identify the problem if there's an error. + +inline Value *FindStringInContext(Value &val, const char *memberId, const char* context, const char* extraContext = nullptr) { + Value::MemberIterator it = val.FindMember(memberId); + if (it == val.MemberEnd()) { + return nullptr; + } + if (!it->value.IsString()) { + throwUnexpectedTypeError("string", memberId, context, extraContext); + } + return &it->value; } -inline Value *FindUInt(Value &val, const char *id) { - Value::MemberIterator it = val.FindMember(id); - return (it != val.MemberEnd() && it->value.IsUint()) ? &it->value : nullptr; +inline Value *FindNumberInContext(Value &val, const char *memberId, const char* context, const char* extraContext = nullptr) { + Value::MemberIterator it = val.FindMember(memberId); + if (it == val.MemberEnd()) { + return nullptr; + } + if (!it->value.IsNumber()) { + throwUnexpectedTypeError("number", memberId, context, extraContext); + } + return &it->value; } -inline Value *FindArray(Value &val, const char *id) { - Value::MemberIterator it = val.FindMember(id); - return (it != val.MemberEnd() && it->value.IsArray()) ? &it->value : nullptr; +inline Value *FindUIntInContext(Value &val, const char *memberId, const char* context, const char* extraContext = nullptr) { + Value::MemberIterator it = val.FindMember(memberId); + if (it == val.MemberEnd()) { + return nullptr; + } + if (!it->value.IsUint()) { + throwUnexpectedTypeError("uint", memberId, context, extraContext); + } + return &it->value; } -inline Value *FindObject(Value &val, const char *id) { - Value::MemberIterator it = val.FindMember(id); - return (it != val.MemberEnd() && it->value.IsObject()) ? &it->value : nullptr; +inline Value *FindArrayInContext(Value &val, const char *memberId, const char* context, const char* extraContext = nullptr) { + Value::MemberIterator it = val.FindMember(memberId); + if (it == val.MemberEnd()) { + return nullptr; + } + if (!it->value.IsArray()) { + throwUnexpectedTypeError("array", memberId, context, extraContext); + } + return &it->value; } -inline Value *FindExtension(Value &val, const char *extensionId) { - if (Value *extensionList = FindObject(val, "extensions")) { - if (Value *extension = FindObject(*extensionList, extensionId)) { +inline Value *FindObjectInContext(Value &val, const char *memberId, const char* context, const char* extraContext = nullptr) { + Value::MemberIterator it = val.FindMember(memberId); + if (it == val.MemberEnd()) { + return nullptr; + } + if (!it->value.IsObject()) { + throwUnexpectedTypeError("object", memberId, context, extraContext); + } + return &it->value; +} + +inline Value *FindExtensionInContext(Value &val, const char *extensionId, const char* context, const char* extraContext = nullptr) { + if (Value *extensionList = FindObjectInContext(val, "extensions", context, extraContext)) { + if (Value *extension = FindObjectInContext(*extensionList, extensionId, "extensions")) { return extension; } } return nullptr; } + +// Overloads when the value is the document. + +inline Value *FindString(Document &doc, const char *memberId) { + return FindStringInContext(doc, memberId, "the document"); +} + +inline Value *FindNumber(Document &doc, const char *memberId) { + return FindNumberInContext(doc, memberId, "the document"); +} + +inline Value *FindUInt(Document &doc, const char *memberId) { + return FindUIntInContext(doc, memberId, "the document"); +} + +inline Value *FindArray(Document &val, const char *memberId) { + return FindArrayInContext(val, memberId, "the document"); +} + +inline Value *FindObject(Document &doc, const char *memberId) { + return FindObjectInContext(doc, memberId, "the document"); +} + +inline Value *FindExtension(Value &val, const char *extensionId) { + return FindExtensionInContext(val, extensionId, "the document"); +} } // namespace +inline Value *Object::FindString(Value &val, const char *memberId) { + return FindStringInContext(val, memberId, id.c_str(), name.c_str()); +} + +inline Value *Object::FindNumber(Value &val, const char *memberId) { + return FindNumberInContext(val, memberId, id.c_str(), name.c_str()); +} + +inline Value *Object::FindUInt(Value &val, const char *memberId) { + return FindUIntInContext(val, memberId, id.c_str(), name.c_str()); +} + +inline Value *Object::FindArray(Value &val, const char *memberId) { + return FindArrayInContext(val, memberId, id.c_str(), name.c_str()); +} + +inline Value *Object::FindObject(Value &val, const char *memberId) { + return FindObjectInContext(val, memberId, id.c_str(), name.c_str()); +} + +inline Value *Object::FindExtension(Value &val, const char *extensionId) { + return FindExtensionInContext(val, extensionId, id.c_str(), name.c_str()); +} + #ifdef ASSIMP_ENABLE_DRACO template @@ -349,17 +441,20 @@ inline LazyDict::~LazyDict() { template inline void LazyDict::AttachToDocument(Document &doc) { Value *container = nullptr; + const char* context = nullptr; if (mExtId) { if (Value *exts = FindObject(doc, "extensions")) { - container = FindObject(*exts, mExtId); + container = FindObjectInContext(*exts, mExtId, "extensions"); + context = mExtId; } } else { container = &doc; + context = "the document"; } if (container) { - mDict = FindArray(*container, mDictId); + mDict = FindArrayInContext(*container, mDictId, context); } } @@ -1107,8 +1202,7 @@ inline void Texture::Read(Value &obj, Asset &r) { } } -namespace { -inline void SetTextureProperties(Asset &r, Value *prop, TextureInfo &out) { +void Material::SetTextureProperties(Asset &r, Value *prop, TextureInfo &out) { if (r.extensionsUsed.KHR_texture_transform) { if (Value *pKHR_texture_transform = FindExtension(*prop, "KHR_texture_transform")) { out.textureTransformSupported = true; @@ -1134,8 +1228,8 @@ inline void SetTextureProperties(Asset &r, Value *prop, TextureInfo &out) { } } - if (Value *index = FindUInt(*prop, "index")) { - out.texture = r.textures.Retrieve(index->GetUint()); + if (Value *indexProp = FindUInt(*prop, "index")) { + out.texture = r.textures.Retrieve(indexProp->GetUint()); } if (Value *texcoord = FindUInt(*prop, "texCoord")) { @@ -1143,13 +1237,13 @@ inline void SetTextureProperties(Asset &r, Value *prop, TextureInfo &out) { } } -inline void ReadTextureProperty(Asset &r, Value &vals, const char *propName, TextureInfo &out) { +inline void Material::ReadTextureProperty(Asset &r, Value &vals, const char *propName, TextureInfo &out) { if (Value *prop = FindMember(vals, propName)) { SetTextureProperties(r, prop, out); } } -inline void ReadTextureProperty(Asset &r, Value &vals, const char *propName, NormalTextureInfo &out) { +inline void Material::ReadTextureProperty(Asset &r, Value &vals, const char *propName, NormalTextureInfo &out) { if (Value *prop = FindMember(vals, propName)) { SetTextureProperties(r, prop, out); @@ -1159,7 +1253,7 @@ inline void ReadTextureProperty(Asset &r, Value &vals, const char *propName, Nor } } -inline void ReadTextureProperty(Asset &r, Value &vals, const char *propName, OcclusionTextureInfo &out) { +inline void Material::ReadTextureProperty(Asset &r, Value &vals, const char *propName, OcclusionTextureInfo &out) { if (Value *prop = FindMember(vals, propName)) { SetTextureProperties(r, prop, out); @@ -1168,7 +1262,6 @@ inline void ReadTextureProperty(Asset &r, Value &vals, const char *propName, Occ } } } -} // namespace inline void Material::Read(Value &material, Asset &r) { SetDefaults(); @@ -1762,9 +1855,9 @@ inline void AssetMetadata::Read(Document &doc) { ReadMember(*obj, "copyright", copyright); ReadMember(*obj, "generator", generator); - if (Value *versionString = FindString(*obj, "version")) { + if (Value *versionString = FindStringInContext(*obj, "version", "\"asset\"")) { version = versionString->GetString(); - } else if (Value *versionNumber = FindNumber(*obj, "version")) { + } else if (Value *versionNumber = FindNumberInContext(*obj, "version", "\"asset\"")) { char buf[4]; ai_snprintf(buf, 4, "%.1f", versionNumber->GetDouble()); @@ -1772,7 +1865,7 @@ inline void AssetMetadata::Read(Document &doc) { version = buf; } - Value *curProfile = FindObject(*obj, "profile"); + Value *curProfile = FindObjectInContext(*obj, "profile", "\"asset\""); if (nullptr != curProfile) { ReadMember(*curProfile, "api", this->profile.api); ReadMember(*curProfile, "version", this->profile.version); diff --git a/code/AssetLib/glTF2/glTF2AssetWriter.inl b/code/AssetLib/glTF2/glTF2AssetWriter.inl index 8bcc89e29..166eada0f 100644 --- a/code/AssetLib/glTF2/glTF2AssetWriter.inl +++ b/code/AssetLib/glTF2/glTF2AssetWriter.inl @@ -886,6 +886,7 @@ namespace glTF2 { if (d.mObjs.empty()) return; Value* container = &mDoc; + const char* context = "Document"; if (d.mExtId) { Value* exts = FindObject(mDoc, "extensions"); @@ -894,17 +895,18 @@ namespace glTF2 { exts = FindObject(mDoc, "extensions"); } - container = FindObject(*exts, d.mExtId); + container = FindObjectInContext(*exts, d.mExtId, "extensions"); if (nullptr != container) { exts->AddMember(StringRef(d.mExtId), Value().SetObject().Move(), mDoc.GetAllocator()); - container = FindObject(*exts, d.mExtId); + container = FindObjectInContext(*exts, d.mExtId, "extensions"); + context = d.mExtId; } } - Value *dict = FindArray(*container, d.mDictId); + Value *dict = FindArrayInContext(*container, d.mDictId, context); if (nullptr == dict) { container->AddMember(StringRef(d.mDictId), Value().SetArray().Move(), mDoc.GetAllocator()); - dict = FindArray(*container, d.mDictId); + dict = FindArrayInContext(*container, d.mDictId, context); if (nullptr == dict) { return; } From 57652a9084bcd0896a414c1a73bfc39396f5826e Mon Sep 17 00:00:00 2001 From: Malcolm Tyrrell Date: Mon, 15 Mar 2021 16:27:50 +0000 Subject: [PATCH 2/5] Version is strictly a string --- code/AssetLib/glTF2/glTF2Asset.inl | 9 +- test/models/glTF2/wrongTypes/BoxTextured0.bin | Bin 0 -> 840 bytes .../glTF2/wrongTypes/CesiumLogoFlat.png | Bin 0 -> 2433 bytes test/models/glTF2/wrongTypes/badArray.gltf | 179 +++++++++++++++++ .../models/glTF2/wrongTypes/badExtension.gltf | 185 ++++++++++++++++++ test/models/glTF2/wrongTypes/badNumber.gltf | 184 +++++++++++++++++ test/models/glTF2/wrongTypes/badObject.gltf | 176 +++++++++++++++++ test/models/glTF2/wrongTypes/badString.gltf | 182 +++++++++++++++++ test/models/glTF2/wrongTypes/badUint.gltf | 182 +++++++++++++++++ test/unit/utglTF2ImportExport.cpp | 24 +++ 10 files changed, 1113 insertions(+), 8 deletions(-) create mode 100644 test/models/glTF2/wrongTypes/BoxTextured0.bin create mode 100644 test/models/glTF2/wrongTypes/CesiumLogoFlat.png create mode 100644 test/models/glTF2/wrongTypes/badArray.gltf create mode 100644 test/models/glTF2/wrongTypes/badExtension.gltf create mode 100644 test/models/glTF2/wrongTypes/badNumber.gltf create mode 100644 test/models/glTF2/wrongTypes/badObject.gltf create mode 100644 test/models/glTF2/wrongTypes/badString.gltf create mode 100644 test/models/glTF2/wrongTypes/badUint.gltf diff --git a/code/AssetLib/glTF2/glTF2Asset.inl b/code/AssetLib/glTF2/glTF2Asset.inl index 66b39ee57..8a88255c4 100644 --- a/code/AssetLib/glTF2/glTF2Asset.inl +++ b/code/AssetLib/glTF2/glTF2Asset.inl @@ -249,7 +249,7 @@ inline Value *FindObjectInContext(Value &val, const char *memberId, const char* inline Value *FindExtensionInContext(Value &val, const char *extensionId, const char* context, const char* extraContext = nullptr) { if (Value *extensionList = FindObjectInContext(val, "extensions", context, extraContext)) { - if (Value *extension = FindObjectInContext(*extensionList, extensionId, "extensions")) { + if (Value *extension = FindObjectInContext(*extensionList, extensionId, context, extraContext)) { return extension; } } @@ -1857,14 +1857,7 @@ inline void AssetMetadata::Read(Document &doc) { if (Value *versionString = FindStringInContext(*obj, "version", "\"asset\"")) { version = versionString->GetString(); - } else if (Value *versionNumber = FindNumberInContext(*obj, "version", "\"asset\"")) { - char buf[4]; - - ai_snprintf(buf, 4, "%.1f", versionNumber->GetDouble()); - - version = buf; } - Value *curProfile = FindObjectInContext(*obj, "profile", "\"asset\""); if (nullptr != curProfile) { ReadMember(*curProfile, "api", this->profile.api); diff --git a/test/models/glTF2/wrongTypes/BoxTextured0.bin b/test/models/glTF2/wrongTypes/BoxTextured0.bin new file mode 100644 index 0000000000000000000000000000000000000000..d2a73551f9456732c7ca85b9d70895f5d9ab8171 GIT binary patch literal 840 zcma)4TMmLS5FAugR8)K(r@fgRg`3F9)Sc2X4N+i|$-K6U9|D@%NdZH8sMCdXhgs?; z>8CE)+Yvq1hwmphbb0bSz9n3Qv{?B+?(fNy1()2HW=AbfwKB4_dS!i9PnJ%1er4>H zi~S!ZIHJM{XG4VxuDIcDxZ(qtxm=$B literal 0 HcmV?d00001 diff --git a/test/models/glTF2/wrongTypes/CesiumLogoFlat.png b/test/models/glTF2/wrongTypes/CesiumLogoFlat.png new file mode 100644 index 0000000000000000000000000000000000000000..45d502ed2610974189307ad542b05b70df14abd4 GIT binary patch literal 2433 zcmV-{34Zp8P)_WXpT zT&KN^W{xz{-NS2=JMQx8=kogA?fBT~_si45y2_@+&$ZLn&C25R(B<{I+3>&4t;yH7 z=k4F%=+?sB^4sOl<>lpUt>3WB;eWT|mc!+SyX3^v-*2zrrpV)YwBmHJ;knS^%huk~ z+1}aP+>XEHoyX^<&g`+&?tmWZ761SUi%CR5RCr$O)Jt;2APhy(Z08|~KgQ|(FDyew zsp=R5YT~nlTY~rX-#i|V$K!G2Wc)mlWh6Gnqti<0l$mFCVfs>HcWU= zU}N>)$8f~oL^fIRfxO@v@>XXMp-0e`br?ZEGI~bCo)PzOtm9az;!cu|<&TbVw_ui5 zwW3|B5@D2()+~cyB$>Y!g)thJ$B!@Lz8kOX+jvq7OjAoBdRRn8M8L$qkQNB4TV7-vHO9JAJQ8q${ zXt%jig;B>Q5c8=c(DHB(vwdB7l^b%9eG_ZB*eh` zKiF!esNH0e)(Q5M{vNJS(>_P z7E8|>Y{ee=q-j zwe7~}bCKVdc&GAaaxy)wQaF3r-P1o;Nh@5_%Rq_QCc}p;>PT)Ya7yW!QtBIp3$O8J zJ-5$rM{OvXImeYE+BRq^+^17@2G@bfcef4nAgl2iHB$;h1ulz}PCmyM7K*Pgam#Cz zbh0@h|EGo$Hx)4kqh8}{YNEJY4eox2vh)a7Aw*_VJcEQL)XA+soKt#8avR94FA)<0 zsg_paDI8oR4sj0WmIU>IQ^k(^v-HmLNUj*xHHJLfb^%}CnhO;skj$V~R0982UN0V=@%S|H5 z;uJ7>9bD|Rm4pZ56i4zoJ=IZa8&Ggv;Yi#>l7j|^50U~WW>Z$`E;0opoG?qS^dksQ zV*JN6Byf8NnFb-wcoF={DLg80PPJyG^kph>&J*9j`iGBTGfe0-wbPcVh6NAbUJ%ZN ztdz1$s|pqw@5&|N%z~zWqbgGr!=~g;l5oa2DkYzHjCdO%3wKgVF4HrZkUROUOf{~M z=Cn_G^Ob30e|2SgbvihOvrJ5;rPOP`dC$3%MtH~4JMp?6`=xmg(XZ1+9^S3ggNITr z$+W~p3hpEhXEc=JYmc?*+v(sg6Nl?7#q*vFq19=}GEq2@Hg>1zQ%$CBpS~2rnOS0& za#N}u@Vq;9lFP*8^xnIcOx^3#he|jz7F=WQ9y??bb?O_=m?Mn?1{t5-UBdr(x8okAfglRt)6NvGbb(TUq6aG6C1`*| zdzTZx{}(gHL?W~88yxWWH+fCNyxk_-f-<2>$;n?PZAF*f9N|EitF7($1=`4kbj+RB zq%{6;d!?4m^<^4zw-2Q39pSgH)E<;c>yvhHG!>eH7p~#y7M}N7(e^O;iAL+mBRC0FBn==#2$yIg$lQO zx(rK>#?!52%-DnNO}C99+-~-4tJF!&d&yW%5pOT9a4Za>mBRBr4~%6L^Xd%8MuwAt zW*I9JR#H@RWw^gmlAPefWm2)lBSjsq4)<3oIiREU(w1y^C$V`S3HLi!D*DZS^HKhB z-YaTx-Y~vTY~lV&rRnC+#&}!`n0Hhst+)K~?bp~HKBXYsQz>P#*{iFndB=5%t3}eu zyJaE*!X5h0647#1nX*kzZ9aFxt9r&pTWVEERI0)~!d3uv zx`oUh2o9eP_tNLr*oYmY2Zk@r_@=_57K0U=t*JxdSRZ`rgl9iokOGFUU25j#hC9S?^-C5lI_BP0Qt)<=X(jOL|W7pUt>r>A!o%X z@co2A{I5!eGgi>%WLzbR5@frDOy5;+oAY(11Qoxd=UJcsgKAsLS-P7pesWtkxv|1P z5P(Z6S#BfSAYj-4UhrO&oM7&Mv@BT$2KrjZJwk9{`B+pT8DmQDz@><>-+!L z>tEMSJWt_oVwj8Nd3MGDj!&X+h~abS8HX(O#RO^i6gmL41?6#A&u5qfhs~Og;BbQH z6ErwO49jO{oU+wZ5WAM8GlG-C#KNsOZL4PUc5ly;HG&U_dCcFnj?3HOLmf+{@G(`* zW72SF|pWh(6ha+1#Q zUegee-PT&(#yCL<-ocN&gH|ibA?|uuzwO#F3cw%?h2bn>D#Qi@U7{I!2=9Ni&V_=s zT@><7S%1j*5*tj$sQk!EVlXA6{mT=^!U_{Z0U#c|HIOG8+R)*Q> wrongTypes = { + { "/glTF2/wrongTypes/badArray.gltf", "array", "meshes[0]" }, + { "/glTF2/wrongTypes/badString.gltf", "string", "scenes[0]" }, + { "/glTF2/wrongTypes/badUint.gltf", "uint", "materials[0]" }, + { "/glTF2/wrongTypes/badNumber.gltf", "number", "materials[0]" }, + { "/glTF2/wrongTypes/badObject.gltf", "object", "materials[0]" }, + { "/glTF2/wrongTypes/badExtension.gltf", "object", "materials[0]" } + }; + for (const auto& tuple : wrongTypes) + { + const auto& file = std::get<0>(tuple); + const auto& type = std::get<1>(tuple); + const auto& context = std::get<2>(tuple); + Assimp::Importer importer; + const aiScene *scene = importer.ReadFile(ASSIMP_TEST_MODELS_DIR + file , aiProcess_ValidateDataStructure); + ASSERT_EQ(scene, nullptr); + const std::string error = importer.GetErrorString(); + EXPECT_FALSE(error.empty()); + EXPECT_NE(error.find(std::string("was not of type \"") + type + "\" when reading " + context), std::string::npos); + } +} + From 8d86bcf0fb1e871fc6e0087863118c07f9ebf2d8 Mon Sep 17 00:00:00 2001 From: Malcolm Tyrrell Date: Mon, 15 Mar 2021 16:32:17 +0000 Subject: [PATCH 3/5] Also check member --- test/unit/utglTF2ImportExport.cpp | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/test/unit/utglTF2ImportExport.cpp b/test/unit/utglTF2ImportExport.cpp index 1d65be888..4110edcfc 100644 --- a/test/unit/utglTF2ImportExport.cpp +++ b/test/unit/utglTF2ImportExport.cpp @@ -611,25 +611,26 @@ TEST_F(utglTF2ImportExport, import_dracoEncoded) { TEST_F(utglTF2ImportExport, wrongTypes) { // Deliberately broken version of the BoxTextured.gltf asset. - std::vector> wrongTypes = { - { "/glTF2/wrongTypes/badArray.gltf", "array", "meshes[0]" }, - { "/glTF2/wrongTypes/badString.gltf", "string", "scenes[0]" }, - { "/glTF2/wrongTypes/badUint.gltf", "uint", "materials[0]" }, - { "/glTF2/wrongTypes/badNumber.gltf", "number", "materials[0]" }, - { "/glTF2/wrongTypes/badObject.gltf", "object", "materials[0]" }, - { "/glTF2/wrongTypes/badExtension.gltf", "object", "materials[0]" } + std::vector> wrongTypes = { + { "/glTF2/wrongTypes/badArray.gltf", "array", "primitives", "meshes[0]" }, + { "/glTF2/wrongTypes/badString.gltf", "string", "name", "scenes[0]" }, + { "/glTF2/wrongTypes/badUint.gltf", "uint", "index", "materials[0]" }, + { "/glTF2/wrongTypes/badNumber.gltf", "number", "scale", "materials[0]" }, + { "/glTF2/wrongTypes/badObject.gltf", "object", "pbrMetallicRoughness", "materials[0]" }, + { "/glTF2/wrongTypes/badExtension.gltf", "object", "KHR_texture_transform", "materials[0]" } }; for (const auto& tuple : wrongTypes) { const auto& file = std::get<0>(tuple); const auto& type = std::get<1>(tuple); - const auto& context = std::get<2>(tuple); + const auto& member = std::get<2>(tuple); + const auto& context = std::get<3>(tuple); Assimp::Importer importer; const aiScene *scene = importer.ReadFile(ASSIMP_TEST_MODELS_DIR + file , aiProcess_ValidateDataStructure); ASSERT_EQ(scene, nullptr); const std::string error = importer.GetErrorString(); EXPECT_FALSE(error.empty()); - EXPECT_NE(error.find(std::string("was not of type \"") + type + "\" when reading " + context), std::string::npos); + EXPECT_NE(error.find(member + "\" was not of type \"" + type + "\" when reading " + context), std::string::npos); } } From e4983aa16e5f643a34f2c3b81b2c1c38ae6c0f25 Mon Sep 17 00:00:00 2001 From: Malcolm Tyrrell Date: Mon, 15 Mar 2021 16:46:41 +0000 Subject: [PATCH 4/5] Fix code issue --- code/AssetLib/glTF2/glTF2Asset.inl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/code/AssetLib/glTF2/glTF2Asset.inl b/code/AssetLib/glTF2/glTF2Asset.inl index 8a88255c4..daac549e9 100644 --- a/code/AssetLib/glTF2/glTF2Asset.inl +++ b/code/AssetLib/glTF2/glTF2Asset.inl @@ -185,7 +185,7 @@ inline void throwUnexpectedTypeError(const char (&expectedTypeName)[N], const ch std::string fullContext = context; if (extraContext && (strlen(extraContext) > 0)) { - fullContext += " (", extraContext, ")"; + fullContext = fullContext + " (" + extraContext + ")"; } throw DeadlyImportError("Member \"", memberId, "\" was not of type \"", expectedTypeName, "\" when reading ", fullContext); } From 3503252e12a26499dedaaa63e3efb3e431bf610b Mon Sep 17 00:00:00 2001 From: Malcolm Tyrrell Date: Tue, 16 Mar 2021 11:47:19 +0000 Subject: [PATCH 5/5] Strict check for accessor "count" --- code/AssetLib/glTF2/glTF2Asset.inl | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/code/AssetLib/glTF2/glTF2Asset.inl b/code/AssetLib/glTF2/glTF2Asset.inl index daac549e9..231637e8e 100644 --- a/code/AssetLib/glTF2/glTF2Asset.inl +++ b/code/AssetLib/glTF2/glTF2Asset.inl @@ -883,7 +883,14 @@ inline void Accessor::Read(Value &obj, Asset &r) { byteOffset = MemberOrDefault(obj, "byteOffset", size_t(0)); componentType = MemberOrDefault(obj, "componentType", ComponentType_BYTE); - count = MemberOrDefault(obj, "count", size_t(0)); + { + const Value* countValue = FindUInt(obj, "count"); + if (!countValue || countValue->GetInt() < 1) + { + throw DeadlyImportError("A strictly positive count value is required, when reading ", id.c_str(), name.empty() ? "" : " (" + name + ")"); + } + count = countValue->GetUint(); + } const char *typestr; type = ReadMember(obj, "type", typestr) ? AttribType::FromString(typestr) : AttribType::SCALAR;