Merge pull request #467 from rhiskey/release

Small security and overflow patch

stb_image_write.h

@@ -773,7 +773,7 @@ static int stbi_write_hdr_core(stbi__write_context *s, int x, int y, int comp, f
 #ifdef __STDC_LIB_EXT1__
       len = sprintf_s(buffer, sizeof(buffer), "EXPOSURE=          1.0000000000000\n\n-Y %d +X %d\n", y, x);
 #else
-      len = sprintf(buffer, "EXPOSURE=          1.0000000000000\n\n-Y %d +X %d\n", y, x);
+      len = snprintf(buffer, sizeof(buffer), "EXPOSURE=          1.0000000000000\n\n-Y %d +X %d\n", y, x);
 #endif
       s->func(s->context, buffer, len);
 

tests/tester.cc

@@ -494,25 +494,23 @@ TEST_CASE("image-uri-spaces", "[issue-236]") {
 }
 
 TEST_CASE("serialize-empty-material", "[issue-294]") {
-
   tinygltf::Model m;
-
-  tinygltf::Material mat;
-  mat.pbrMetallicRoughness.baseColorFactor = {1.0f, 1.0f, 1.0f, 1.0f}; // default baseColorFactor
-  m.materials.push_back(mat);
-
+  // Add default constructed material to model
+  m.materials.push_back({});
+  // Serialize model to output stream
   std::stringstream os;
-
   tinygltf::TinyGLTF ctx;
   bool ret = ctx.WriteGltfSceneToStream(&m, os, false, false);
   REQUIRE(true == ret);
-
-  // use nlohmann json
+  // Parse serialized model
   nlohmann::json j = nlohmann::json::parse(os.str());
-
+  // Serialized materials shall hold an empty object that
+  // represents the default constructed material
+  REQUIRE(j.find("materials") != j.end());
+  REQUIRE(j["materials"].is_array());
   REQUIRE(1 == j["materials"].size());
-  REQUIRE(j["materials"][0].is_object());
-
+  CHECK(j["materials"][0].is_object());
+  CHECK(j["materials"][0].empty());
 }
 
 TEST_CASE("empty-skeleton-id", "[issue-321]") {
@@ -757,3 +755,150 @@ TEST_CASE("load-issue-416-model", "[issue-416]") {
   // external file load fails, but reading glTF itself is ok.
   REQUIRE(true == ret);
 }
+
+TEST_CASE("serialize-empty-node", "[issue-457]") {
+  tinygltf::Model m;
+  // Add default constructed node to model
+  m.nodes.push_back({});
+  // Add scene to model
+  m.scenes.push_back({});
+  // The scene's only node is the empty node
+  m.scenes.front().nodes.push_back(0);
+
+  // Serialize model to output stream
+  std::stringstream os;
+  tinygltf::TinyGLTF ctx;
+  bool ret = ctx.WriteGltfSceneToStream(&m, os, false, false);
+  REQUIRE(true == ret);
+
+  // Parse serialized model
+  nlohmann::json j = nlohmann::json::parse(os.str());
+
+  // Serialized nodes shall hold an empty object that
+  // represents the default constructed node
+  REQUIRE(j.find("nodes") != j.end());
+  REQUIRE(j["nodes"].is_array());
+  REQUIRE(1 == j["nodes"].size());
+  CHECK(j["nodes"][0].is_object());
+  CHECK(j["nodes"][0].empty());
+
+  // We also want to make sure that the serialized scene
+  // is referencing the empty node.
+
+  // There shall be a single serialized scene
+  auto scenes = j.find("scenes");
+  REQUIRE(scenes != j.end());
+  REQUIRE(scenes->is_array());
+  REQUIRE(1 == scenes->size());
+  auto scene = scenes->at(0);
+  REQUIRE(scene.is_object());
+  // The scene's nodes array shall hold a reference
+  // to the single node
+  auto nodes = scene.find("nodes");
+  REQUIRE(nodes != scene.end());
+  REQUIRE(nodes->is_array());
+  REQUIRE(1 == nodes->size());
+  auto node = nodes->at(0);
+  CHECK(node.is_number_integer());
+  int idx = -1;
+  node.get_to(idx);
+  CHECK(0 == idx);
+}
+
+TEST_CASE("serialize-light-index", "[issue-458]") {
+
+  // Create the light
+  tinygltf::Light light;
+  light.type = "point";
+  light.intensity = 0.75;
+  light.color = std::vector<double>{1.0, 0.8, 0.95};
+
+  // Stream to serialize to
+  std::stringstream os;
+
+  {
+    tinygltf::Model m;
+    tinygltf::Scene scene;
+    // Add the light to the model
+    m.lights.push_back(light);
+    // Create a node that uses the light
+    tinygltf::Node node;
+    node.light = 0;
+    // Add the node to the model
+    m.nodes.push_back(node);
+    // Add the node to the scene
+    scene.nodes.push_back(0);
+    // Add the scene to the model
+    m.scenes.push_back(scene);
+    // Serialize model to output stream
+    tinygltf::TinyGLTF ctx;
+    bool ret = ctx.WriteGltfSceneToStream(&m, os, false, false);
+    REQUIRE(true == ret);
+  }
+
+  {
+    tinygltf::Model m;
+    tinygltf::TinyGLTF ctx;
+    // Parse the serialized model
+    bool ok = ctx.LoadASCIIFromString(&m, nullptr, nullptr, os.str().c_str(), os.str().size(), "");
+    REQUIRE(true == ok);
+    // Check if the light was correctly serialized
+    REQUIRE(1 == m.lights.size());
+    CHECK(m.lights[0] == light);
+    // Check that the node properly references the light
+    REQUIRE(1 == m.nodes.size());
+    CHECK(m.nodes[0].light == 0);
+  }
+}
+
+TEST_CASE("default-material", "[issue-459]") {
+  const std::vector<double> default_emissive_factor{ 0.0, 0.0, 0.0 };
+  const std::vector<double> default_base_color_factor{ 1.0, 1.0, 1.0, 1.0 };
+  const std::string default_alpha_mode = "OPAQUE";
+  const double default_alpha_cutoff = 0.5;
+  const bool default_double_sided = false;
+  const double default_metallic_factor = 1.0;
+  const double default_roughness_factor = 1.0;
+  // Check that default constructed material
+  // holds actual default GLTF material properties
+  tinygltf::Material mat;
+  CHECK(mat.alphaMode == default_alpha_mode);
+  CHECK(mat.alphaCutoff == default_alpha_cutoff);
+  CHECK(mat.doubleSided == default_double_sided);
+  CHECK(mat.emissiveFactor == default_emissive_factor);
+  CHECK(mat.pbrMetallicRoughness.baseColorFactor == default_base_color_factor);
+  CHECK(mat.pbrMetallicRoughness.metallicFactor == default_metallic_factor);
+  CHECK(mat.pbrMetallicRoughness.roughnessFactor == default_roughness_factor);
+  // None of the textures should be set
+  CHECK(mat.normalTexture.index == -1);
+  CHECK(mat.occlusionTexture.index == -1);
+  CHECK(mat.emissiveTexture.index == -1);
+}
+
+TEST_CASE("serialize-empty-scene", "[issue-464]") {
+  // Stream to serialize to
+  std::stringstream os;
+
+  {
+    tinygltf::Model m;
+    // Add empty scene to the model
+    m.scenes.push_back({});
+    // Serialize model to output stream
+    tinygltf::TinyGLTF ctx;
+    bool ret = ctx.WriteGltfSceneToStream(&m, os, false, false);
+    REQUIRE(true == ret);
+  }
+
+  {
+    tinygltf::Model m;
+    tinygltf::TinyGLTF ctx;
+    // Parse the serialized model
+    bool ok = ctx.LoadASCIIFromString(&m, nullptr, nullptr, os.str().c_str(), os.str().size(), "");
+    REQUIRE(true == ok);
+    // Make sure the empty scene is there
+    REQUIRE(1 == m.scenes.size());
+    tinygltf::Scene scene{};
+    // Check that the scene is empty
+    CHECK(m.scenes[0] == scene);
+  }
+}

tiny_gltf.h

@@ -735,7 +735,7 @@ struct OcclusionTextureInfo {
 
 // pbrMetallicRoughness class defined in glTF 2.0 spec.
 struct PbrMetallicRoughness {
-  std::vector<double> baseColorFactor;  // len = 4. default [1,1,1,1]
+  std::vector<double> baseColorFactor{1.0, 1.0, 1.0, 1.0};  // len = 4. default [1,1,1,1]
   TextureInfo baseColorTexture;
   double metallicFactor{1.0};   // default 1
   double roughnessFactor{1.0};  // default 1
@@ -748,9 +748,9 @@ struct PbrMetallicRoughness {
   std::string extras_json_string;
   std::string extensions_json_string;
 
-  PbrMetallicRoughness()
-      : baseColorFactor(std::vector<double>{1.0, 1.0, 1.0, 1.0}) {}
+  PbrMetallicRoughness() = default;
   DEFAULT_METHODS(PbrMetallicRoughness)
+
   bool operator==(const PbrMetallicRoughness &) const;
 };
 
@@ -760,10 +760,10 @@ struct PbrMetallicRoughness {
 struct Material {
   std::string name;
 
-  std::vector<double> emissiveFactor;  // length 3. default [0, 0, 0]
-  std::string alphaMode;               // default "OPAQUE"
-  double alphaCutoff{0.5};             // default 0.5
-  bool doubleSided{false};             // default false;
+  std::vector<double> emissiveFactor{0.0, 0.0, 0.0};  // length 3. default [0, 0, 0]
+  std::string alphaMode{"OPAQUE"}; // default "OPAQUE"
+  double alphaCutoff{0.5};        // default 0.5
+  bool doubleSided{false};        // default false
 
   PbrMetallicRoughness pbrMetallicRoughness;
 
@@ -783,7 +783,7 @@ struct Material {
   std::string extras_json_string;
   std::string extensions_json_string;
 
-  Material() : alphaMode("OPAQUE") {}
+  Material() = default;
   DEFAULT_METHODS(Material)
 
   bool operator==(const Material &) const;
@@ -4926,13 +4926,13 @@ static bool ParseDracoExtension(Primitive *primitive, Model *model,
       }
 
       if (supposedComponentType > model->accessors[primitive->indices].componentType) {
-        model->accessors[primitive->indices].componentType = supposedComponentType;
         if (warn) {
           (*warn) +=
               "GLTF component type " + std::to_string(model->accessors[primitive->indices].componentType) +
               " is not sufficient for number of stored points,"
               " treating as " + std::to_string(supposedComponentType) + "\n";
         }
+        model->accessors[primitive->indices].componentType = supposedComponentType;
       }
     }
 
@@ -6145,20 +6145,22 @@ bool TinyGLTF::LoadFromString(Model *model, std::string *err, std::string *warn,
           return false;
         }
 
-        auto bufferView =
+        const auto bufferView =
             model->accessors[size_t(primitive.indices)].bufferView;
-        if (bufferView < 0 || size_t(bufferView) >= model->bufferViews.size()) {
+        if (bufferView < 0) {
+          // skip, bufferView could be null(-1) for certain extensions
+        } else if (size_t(bufferView) >= model->bufferViews.size()) {
           if (err) {
             (*err) += "accessor[" + std::to_string(primitive.indices) +
                       "] invalid bufferView";
           }
           return false;
+        } else {
+          model->bufferViews[size_t(bufferView)].target =
+              TINYGLTF_TARGET_ELEMENT_ARRAY_BUFFER;
+          // we could optionally check if accessors' bufferView type is Scalar, as
+          // it should be
         }
-
-        model->bufferViews[size_t(bufferView)].target =
-            TINYGLTF_TARGET_ELEMENT_ARRAY_BUFFER;
-        // we could optionally check if accessors' bufferView type is Scalar, as
-        // it should be
       }
 
       for (auto &attribute : primitive.attributes) {
@@ -6667,9 +6669,12 @@ bool TinyGLTF::LoadBinaryFromMemory(Model *model, std::string *err,
   // Use 64bit uint to avoid integer overflow.
   uint64_t header_and_json_size = 20ull + uint64_t(chunk0_length);
 
-  if (header_and_json_size > std::numeric_limits<uint32_t>::max()) {
+  if (header_and_json_size > (std::numeric_limits<uint32_t>::max)()) {
     // Do not allow 4GB or more GLB data.
-    (*err) = "Invalid glTF binary. GLB data exceeds 4GB.";
+    if (err) {
+      (*err) = "Invalid glTF binary. GLB data exceeds 4GB.";
+    }
+    return false;
   }
 
   if ((header_and_json_size > uint64_t(size)) || (chunk0_length < 1) ||
@@ -6688,6 +6693,7 @@ bool TinyGLTF::LoadBinaryFromMemory(Model *model, std::string *err,
     if (err) {
       (*err) = "JSON Chunk end does not aligned to a 4-byte boundary.";
     }
+    return false;
   }
 
   // std::cout << "header_and_json_size = " << header_and_json_size << "\n";
@@ -7750,7 +7756,7 @@ static void SerializeGltfNode(const Node &node, detail::json &o) {
       detail::JsonSetObject(lights_punctual);
       detail::JsonAddMember(extensions, "KHR_lights_punctual",
                             std::move(lights_punctual));
-      detail::FindMember(o, "KHR_lights_punctual", it);
+      detail::FindMember(extensions, "KHR_lights_punctual", it);
     }
     SerializeNumberProperty("light", node.light, detail::GetValue(it));
   } else {
@@ -8034,6 +8040,16 @@ static void SerializeGltfModel(const Model *model, detail::json &o) {
     for (unsigned int i = 0; i < model->nodes.size(); ++i) {
       detail::json node;
       SerializeGltfNode(model->nodes[i], node);
+
+      if (detail::JsonIsNull(node)) {
+        // Issue 457.
+        // `node` does not have any required parameters,
+        // so the result may be null(unmodified) when all node parameters
+        // have default value.
+        //
+        // null is not allowed thus we create an empty JSON object.
+        detail::JsonSetObject(node);
+      }
       detail::JsonPushBack(nodes, std::move(node));
     }
     detail::JsonAddMember(o, "nodes", std::move(nodes));
@@ -8051,6 +8067,15 @@ static void SerializeGltfModel(const Model *model, detail::json &o) {
     for (unsigned int i = 0; i < model->scenes.size(); ++i) {
       detail::json currentScene;
       SerializeGltfScene(model->scenes[i], currentScene);
+      if (detail::JsonIsNull(currentScene)) {
+        // Issue 464.
+        // `scene` does not have any required parameters,
+        // so the result may be null(unmodified) when all scene parameters
+        // have default value.
+        //
+        // null is not allowed thus we create an empty JSON object.
+        detail::JsonSetObject(currentScene);
+      }
       detail::JsonPushBack(scenes, std::move(currentScene));
     }
     detail::JsonAddMember(o, "scenes", std::move(scenes));