diff --git a/GLTFSDK.Test/Source/DeserializeTests.cpp b/GLTFSDK.Test/Source/DeserializeTests.cpp index f316fb3..ad0973e 100644 --- a/GLTFSDK.Test/Source/DeserializeTests.cpp +++ b/GLTFSDK.Test/Source/DeserializeTests.cpp @@ -301,6 +301,56 @@ namespace ], "asset": {"version": "2.0"} })"; + + // Regression inputs for variable-length array members that the deserializer + // reads with rapidjson array accessors: node "children" (an array of node + // indices) and node/mesh "weights" (arrays of numbers, via the shared + // RapidJsonUtils::ToFloatArray helper). A present member of the wrong + // container type - or an array containing an element of the wrong type - + // must be rejected with InvalidGLTFException rather than read with accessors + // that are only well-defined on a JSON array of the expected element type. + // These pass SchemaFlags::DisableSchemaRoot so the malformed value reaches + // the semantic deserializers (a consumer that disables JSON-schema + // validation must still get a clean exception). + const char* c_validNodeChildren = R"({ + "nodes": [{ "children": [1, 2] }, {}, {}], + "asset": {"version": "2.0"} +})"; + + const char* c_nodeChildrenIsString = R"({ + "nodes": [{ "children": "not-an-array" }], + "asset": {"version": "2.0"} +})"; + + const char* c_nodeChildrenNonNumericElement = R"({ + "nodes": [{ "children": ["not-a-node-index"] }], + "asset": {"version": "2.0"} +})"; + + const char* c_validNodeWeights = R"({ + "nodes": [{ "weights": [0.25, 0.75] }], + "asset": {"version": "2.0"} +})"; + + const char* c_nodeWeightsIsString = R"({ + "nodes": [{ "weights": "not-an-array" }], + "asset": {"version": "2.0"} +})"; + + const char* c_nodeWeightsNonNumericElement = R"({ + "nodes": [{ "weights": ["not-a-number"] }], + "asset": {"version": "2.0"} +})"; + + const char* c_meshWeightsIsString = R"({ + "meshes": [{ "weights": "not-an-array" }], + "asset": {"version": "2.0"} +})"; + + const char* c_meshWeightsNonNumericElement = R"({ + "meshes": [{ "weights": ["not-a-number"] }], + "asset": {"version": "2.0"} +})"; } namespace Microsoft @@ -586,6 +636,93 @@ namespace Microsoft Deserialize(c_missingDependentPropertyBufferView); }); } + + // Positive regression: a well-formed node "children" array of + // node indices must still deserialize to the expected ids. + GLTFSDK_TEST_METHOD(DeserializeTests, DeserializeSuccess_ValidNodeChildren) + { + auto doc = Deserialize(c_validNodeChildren); + Assert::AreEqual(size_t(3), doc.nodes.Size()); + const auto& node = doc.nodes.Front(); + Assert::AreEqual(size_t(2), node.children.size()); + Assert::AreEqual(std::string("1"), node.children[0]); + Assert::AreEqual(std::string("2"), node.children[1]); + } + + // Negative regression: a node "children" member that is a JSON + // string (or any non-array value) must throw rather than be read + // with array accessors that are only defined on arrays. + GLTFSDK_TEST_METHOD(DeserializeTests, DeserializeFail_NodeChildrenIsString) + { + Assert::ExpectException([]() + { + Deserialize(c_nodeChildrenIsString, DeserializeFlags::None, SchemaFlags::DisableSchemaRoot); + }); + } + + // Negative regression: a "children" array whose elements are not + // node indices must throw rather than read each element as a uint. + GLTFSDK_TEST_METHOD(DeserializeTests, DeserializeFail_NodeChildrenNonNumericElement) + { + Assert::ExpectException([]() + { + Deserialize(c_nodeChildrenNonNumericElement, DeserializeFlags::None, SchemaFlags::DisableSchemaRoot); + }); + } + + // Positive regression: a well-formed node "weights" array of + // numbers must still deserialize. Uses SchemaFlags::DisableSchemaRoot + // because node "weights" has a JSON-schema dependency on "mesh"; + // disabling schema validation exercises the parser's happy path + // (RapidJsonUtils::ToFloatArray) directly. + GLTFSDK_TEST_METHOD(DeserializeTests, DeserializeSuccess_ValidNodeWeights) + { + auto doc = Deserialize(c_validNodeWeights, DeserializeFlags::None, SchemaFlags::DisableSchemaRoot); + Assert::AreEqual(size_t(1), doc.nodes.Size()); + const auto& node = doc.nodes.Front(); + Assert::AreEqual(size_t(2), node.weights.size()); + } + + // Negative regression: a node "weights" member that is a JSON + // string must throw (shared RapidJsonUtils::ToFloatArray helper). + GLTFSDK_TEST_METHOD(DeserializeTests, DeserializeFail_NodeWeightsIsString) + { + Assert::ExpectException([]() + { + Deserialize(c_nodeWeightsIsString, DeserializeFlags::None, SchemaFlags::DisableSchemaRoot); + }); + } + + // Negative regression: a node "weights" array whose elements are + // not numbers must throw rather than read each element as a number. + GLTFSDK_TEST_METHOD(DeserializeTests, DeserializeFail_NodeWeightsNonNumericElement) + { + Assert::ExpectException([]() + { + Deserialize(c_nodeWeightsNonNumericElement, DeserializeFlags::None, SchemaFlags::DisableSchemaRoot); + }); + } + + // Negative regression: a mesh "weights" member that is a JSON + // string must throw. Mesh weights share the same ToFloatArray + // helper as node weights, so the guard must live in the helper. + GLTFSDK_TEST_METHOD(DeserializeTests, DeserializeFail_MeshWeightsIsString) + { + Assert::ExpectException([]() + { + Deserialize(c_meshWeightsIsString, DeserializeFlags::None, SchemaFlags::DisableSchemaRoot); + }); + } + + // Negative regression: a mesh "weights" array whose elements are + // not numbers must throw. + GLTFSDK_TEST_METHOD(DeserializeTests, DeserializeFail_MeshWeightsNonNumericElement) + { + Assert::ExpectException([]() + { + Deserialize(c_meshWeightsNonNumericElement, DeserializeFlags::None, SchemaFlags::DisableSchemaRoot); + }); + } }; } } diff --git a/GLTFSDK/Inc/GLTFSDK/RapidJsonUtils.h b/GLTFSDK/Inc/GLTFSDK/RapidJsonUtils.h index 861499c..d952a53 100644 --- a/GLTFSDK/Inc/GLTFSDK/RapidJsonUtils.h +++ b/GLTFSDK/Inc/GLTFSDK/RapidJsonUtils.h @@ -189,8 +189,20 @@ namespace Microsoft const auto& it = v.FindMember(key.c_str()); if (it != v.MemberEnd()) { + // The member's storage is only well-defined as an array of + // numbers when it actually is one. Reject any other type + // before reading array/number accessors, otherwise a value + // of the wrong JSON type would be walked as a forged array. + if (!it->value.IsArray()) + { + throw InvalidGLTFException("The " + key + " member must be a JSON array"); + } for (rapidjson::Value::ConstValueIterator ait = it->value.Begin(); ait != it->value.End(); ++ait) { + if (!ait->IsNumber()) + { + throw InvalidGLTFException("The " + key + " array elements must be JSON numbers"); + } result.push_back(static_cast(ait->GetDouble())); } } diff --git a/GLTFSDK/Source/Deserialize.cpp b/GLTFSDK/Source/Deserialize.cpp index 19afa72..1da2b45 100644 --- a/GLTFSDK/Source/Deserialize.cpp +++ b/GLTFSDK/Source/Deserialize.cpp @@ -393,9 +393,20 @@ namespace if (it != v.MemberEnd()) { const rapidjson::Value& a = it->value; - node.children.reserve(a.Capacity()); + // children is an array of node indices; anything else must be + // rejected before reading array accessors, which are only + // well-defined on a JSON array. + if (!a.IsArray()) + { + throw InvalidGLTFException("Node children must be a JSON array"); + } + node.children.reserve(a.Size()); for (rapidjson::Value::ConstValueIterator ait = a.Begin(); ait != a.End(); ++ait) { + if (!ait->IsUint()) + { + throw InvalidGLTFException("Node children array elements must be unsigned integers"); + } node.children.push_back(std::to_string(ait->GetUint())); } }