Allow zero-sized BIN chunk. Fixes #440

Small security and overflow patch

.travis-before-install.sh

@@ -1,10 +0,0 @@
-#!/bin/bash
-
-if [[ "$TRAVIS_OS_NAME" == "osx" ]]
-then
-    brew upgrade
-    curl -o premake5.tar.gz https://github.com/premake/premake-core/releases/download/v5.0.0-alpha12/premake-5.0.0-alpha12-macosx.tar.gz
-else
-    wget https://github.com/premake/premake-core/releases/download/v5.0.0-alpha12/premake-5.0.0-alpha12-linux.tar.gz -O premake5.tar.gz
-fi
-tar xzf premake5.tar.gz

.travis.yml

@@ -1,63 +0,0 @@
-language: cpp
-sudo: false
-matrix:
-  include:
-  - addons: &1
-      apt:
-        sources:
-        - george-edison55-precise-backports
-        - ubuntu-toolchain-r-test
-        - llvm-toolchain-trusty-3.9
-        packages:
-        - g++-4.9
-        - clang-3.9
-    compiler: clang
-    env: COMPILER_VERSION=3.9 BUILD_TYPE=Debug
-  - addons: *1
-    compiler: clang
-    env: COMPILER_VERSION=3.9 BUILD_TYPE=Release
-  - addons: &2
-      apt:
-        sources:
-        - george-edison55-precise-backports
-        - ubuntu-toolchain-r-test
-        packages:
-        - g++-4.9
-    compiler: gcc
-    env: COMPILER_VERSION=4.9 BUILD_TYPE=Debug EXTRA_CXXFLAGS="-fsanitize=address"
-  - addons: *2
-    compiler: gcc
-    env: COMPILER_VERSION=4.9 BUILD_TYPE=Release EXTRA_CXXFLAGS="-fsanitize=address"
-  - addons: *1
-    compiler: clang
-    env: COMPILER_VERSION=3.9 BUILD_TYPE=Debug CFLAGS="-O0" CXXFLAGS="-O0"
-  - addons: &3
-      apt:
-        sources:
-        - ubuntu-toolchain-r-test
-        packages:
-        - g++-4.8
-    compiler: gcc
-    env: COMPILER_VERSION=4.8 BUILD_TYPE=Debug
-  - addons: *3
-    compiler: gcc
-    env: COMPILER_VERSION=4.8 BUILD_TYPE=Release
-
-before_install:
-  - ./.travis-before-install.sh
-
-
-script:
-  - export CC="${CC}-${COMPILER_VERSION}"
-  - export CXX="${CXX}-${COMPILER_VERSION}"
-  - ${CC} -v
-  - ${CXX} ${EXTRA_CXXFLAGS} -std=c++11 -Wall -g -o loader_example loader_example.cc
-  - ./loader_example ./models/Cube/Cube.gltf
-  - cd tests
-  - clang++ -v
-  - make
-  - ./tester
-  - ./tester_noexcept
-  - cd ../examples/raytrace
-  - ../../premake5 gmake
-  - make

README.md

@@ -26,8 +26,6 @@ Currently TinyGLTF is stable and maintenance mode. No drastic changes and featur
 
 ## Builds
 
-[![Build Status](https://travis-ci.org/syoyo/tinygltf.svg?branch=devel)](https://travis-ci.org/syoyo/tinygltf)
-
 [![Build status](https://ci.appveyor.com/api/projects/status/warngenu9wjjhlm8?svg=true)](https://ci.appveyor.com/project/syoyo/tinygltf)
 
 ![C/C++ CI](https://github.com/syoyo/tinygltf/workflows/C/C++%20CI/badge.svg)
@@ -109,7 +107,7 @@ WASI build example is located in [wasm](wasm) .
 * [SanityEngine](https://github.com/DethRaid/SanityEngine) - A C++/D3D12 renderer focused on the personal and professional development of its developer
 * [Open3D](http://www.open3d.org/) - A Modern Library for 3D Data Processing
 * [Supernova Engine](https://github.com/supernovaengine/supernova) - Game engine for 2D and 3D projects with Lua or C++ in data oriented design.
-* [Wicked Engine<img src="https://github.com/turanszkij/WickedEngine/blob/master/Content/logo_small.png" width="28px" align="center"/>](https://github.com/turanszkij/WickedEngine) - 3D engine with modern graphics 
+* [Wicked Engine<img src="https://github.com/turanszkij/WickedEngine/blob/master/Content/logo_small.png" width="28px" align="center"/>](https://github.com/turanszkij/WickedEngine) - 3D engine with modern graphics
 * Your projects here! (Please send PR)
 
 ## TODOs

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,174 @@ 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);
+  }
+}
+
+TEST_CASE("zero-sized-bin-chunk-glb", "[issue-440]") {
+
+  tinygltf::Model model;
+  tinygltf::TinyGLTF ctx;
+  std::string err;
+  std::string warn;
+
+  // Input glb has zero-sized data in bin chunk(8 bytes for BIN chunk, and chunksize == 0)
+  // The spec https://registry.khronos.org/glTF/specs/2.0/glTF-2.0.html#binary-buffer says
+  //
+  // When the binary buffer is empty or when it is stored by other means, this chunk SHOULD be omitted.
+  //
+  // 'SHOULD' mean 'RECOMMENDED', so we'll need to allow such zero-sized bin chunk is NOT omitted.
+  bool ret = ctx.LoadBinaryFromFile(&model, &err, &warn, "../models/regression/zero-sized-bin-chunk-issue-440.glb");
+  if (!warn.empty()) {
+    std::cout << "WARN: " << warn << "\n";
+  }
+  if (!err.empty()) {
+    std::cerr << err << std::endl;
+  }
+
+  REQUIRE(true == ret);
+}

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;
@@ -6149,7 +6149,7 @@ bool TinyGLTF::LoadFromString(Model *model, std::string *err, std::string *warn,
             model->accessors[size_t(primitive.indices)].bufferView;
         if (bufferView < 0) {
           // skip, bufferView could be null(-1) for certain extensions
-        } else if (size_t(bufferView) >= model->bufferViews.size()) { 
+        } else if (size_t(bufferView) >= model->bufferViews.size()) {
           if (err) {
             (*err) += "accessor[" + std::to_string(primitive.indices) +
                       "] invalid bufferView";
@@ -6652,7 +6652,7 @@ bool TinyGLTF::LoadBinaryFromMemory(Model *model, std::string *err,
 
   memcpy(&version, bytes + 4, 4);
   swap4(&version);
-  memcpy(&length, bytes + 8, 4);
+  memcpy(&length, bytes + 8, 4); // Total glb size, including header and all chunks.
   swap4(&length);
   memcpy(&chunk0_length, bytes + 12, 4);  // JSON data length
   swap4(&chunk0_length);
@@ -6669,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) ||
@@ -6690,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";
@@ -6704,69 +6708,87 @@ bool TinyGLTF::LoadBinaryFromMemory(Model *model, std::string *err,
     bin_size_ = 0;
   } else {
     // Read Chunk1 info(BIN data)
-    // At least Chunk1 should have 12 bytes(8 bytes(header) + 4 bytes(bin
-    // payload could be 1~3 bytes, but need to be aligned to 4 bytes)
-    if ((header_and_json_size + 12ull) > uint64_t(length)) {
+    //
+    // issue-440:
+    // 'SHOULD' in glTF spec means 'RECOMMENDED',
+    // So there is a situation that Chunk1(BIN) is composed of zero-sized BIN data
+    // (chunksize(0) + binformat(BIN) = 8bytes).
+    // 
+    if ((header_and_json_size + 8ull) > uint64_t(length)) {
       if (err) {
         (*err) =
             "Insufficient storage space for Chunk1(BIN data). At least Chunk1 "
-            "Must have 4 or more bytes, but got " +
+            "Must have 8 or more bytes, but got " +
             std::to_string((header_and_json_size + 8ull) - uint64_t(length)) +
             ".\n";
       }
       return false;
     }
 
-    unsigned int chunk1_length;  // 4 bytes
-    unsigned int chunk1_format;  // 4 bytes;
+    unsigned int chunk1_length{0};  // 4 bytes
+    unsigned int chunk1_format{0};  // 4 bytes;
     memcpy(&chunk1_length, bytes + header_and_json_size,
-           4);  // JSON data length
+           4);  // Bin data length
     swap4(&chunk1_length);
     memcpy(&chunk1_format, bytes + header_and_json_size + 4, 4);
     swap4(&chunk1_format);
 
-    // std::cout << "chunk1_length = " << chunk1_length << "\n";
-
-    if (chunk1_length < 4) {
+    if (chunk1_format != 0x004e4942) {
       if (err) {
-        (*err) = "Insufficient Chunk1(BIN) data size.";
+        (*err) = "Invalid chunkType for Chunk1.";
       }
       return false;
     }
 
-    if ((chunk1_length % 4) != 0) {
-      if (strictness_==ParseStrictness::Permissive) {
-        if (warn) {
-          (*warn) += "BIN Chunk end is not aligned to a 4-byte boundary.\n";
-        }
-      }
-      else {
+    if (chunk1_length == 0) {
+
+      if (header_and_json_size + 8 > uint64_t(length)) {
         if (err) {
-          (*err) = "BIN Chunk end is not aligned to a 4-byte boundary.";
+          (*err) = "BIN Chunk header location exceeds the GLB size.";
         }
         return false;
       }
-    }
 
-    if (uint64_t(chunk1_length) + header_and_json_size > uint64_t(length)) {
-      if (err) {
-        (*err) = "BIN Chunk data length exceeds the GLB size.";
+      bin_data_ = nullptr;
+
+    } else {
+
+      // When BIN chunk size is not zero, at least Chunk1 should have 12 bytes(8 bytes(header) + 4 bytes(bin
+      // payload could be 1~3 bytes, but need to be aligned to 4 bytes)
+
+      if (chunk1_length < 4) {
+        if (err) {
+          (*err) = "Insufficient Chunk1(BIN) data size.";
+        }
+        return false;
       }
-      return false;
-    }
 
-    if (chunk1_format != 0x004e4942) {
-      if (err) {
-        (*err) = "Invalid type for chunk1 data.";
+      if ((chunk1_length % 4) != 0) {
+        if (strictness_==ParseStrictness::Permissive) {
+          if (warn) {
+            (*warn) += "BIN Chunk end is not aligned to a 4-byte boundary.\n";
+          }
+        }
+        else {
+          if (err) {
+            (*err) = "BIN Chunk end is not aligned to a 4-byte boundary.";
+          }
+          return false;
+        }
       }
-      return false;
+
+      // +8 chunk1 header size.
+      if (uint64_t(chunk1_length) + header_and_json_size + 8 > uint64_t(length)) {
+        if (err) {
+          (*err) = "BIN Chunk data length exceeds the GLB size.";
+        }
+        return false;
+      }
+
+      bin_data_ = bytes + header_and_json_size +
+                  8;  // 4 bytes (bin_buffer_length) + 4 bytes(bin_buffer_format)
     }
 
-    // std::cout << "chunk1_length = " << chunk1_length << "\n";
-
-    bin_data_ = bytes + header_and_json_size +
-                8;  // 4 bytes (bin_buffer_length) + 4 bytes(bin_buffer_format)
-
     bin_size_ = size_t(chunk1_length);
   }
 
@@ -7752,7 +7774,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 {
@@ -8036,6 +8058,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));
@@ -8053,6 +8085,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));