Merge pull request #512 from ctrlaltf2/oob-fix

Add bounds check to images loaded from bufferviews

CMakeLists.txt

@@ -16,6 +16,7 @@ option(TINYGLTF_BUILD_VALIDATOR_EXAMPLE "Build validator exampe" OFF)
 option(TINYGLTF_BUILD_BUILDER_EXAMPLE "Build glTF builder example" OFF)
 option(TINYGLTF_HEADER_ONLY "On: header-only mode. Off: create tinygltf library(No TINYGLTF_IMPLEMENTATION required in your project)" OFF)
 option(TINYGLTF_INSTALL "Install tinygltf files during install step. Usually set to OFF if you include tinygltf through add_subdirectory()" ON)
+option(TINYGLTF_INSTALL_VENDOR "Install vendored nlohmann/json and nothings/stb headers" ON)
 
 if (TINYGLTF_BUILD_LOADER_EXAMPLE)
   add_executable(loader_example
@@ -61,19 +62,26 @@ endif (TINYGLTF_HEADER_ONLY)
 
 if (TINYGLTF_INSTALL)
   install(TARGETS tinygltf EXPORT tinygltfTargets)
-  install(EXPORT tinygltfTargets NAMESPACE tinygltf:: FILE TinyGLTFTargets.cmake DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake)
+  install(EXPORT tinygltfTargets NAMESPACE tinygltf:: FILE TinyGLTFTargets.cmake DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/tinygltf)
   configure_package_config_file(${CMAKE_CURRENT_SOURCE_DIR}/cmake/TinyGLTFConfig.cmake.in ${CMAKE_CURRENT_BINARY_DIR}/TinyGLTFConfig.cmake INSTALL_DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake)
-  install(FILES ${CMAKE_CURRENT_BINARY_DIR}/TinyGLTFConfig.cmake DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake)
+  install(FILES ${CMAKE_CURRENT_BINARY_DIR}/TinyGLTFConfig.cmake DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/tinygltf)
   # Do not install .lib even if !TINYGLTF_HEADER_ONLY
 
   INSTALL ( FILES
-    json.hpp
-    stb_image.h
-    stb_image_write.h
     tiny_gltf.h
     ${TINYGLTF_EXTRA_SOUECES}
     DESTINATION
     include
     )
 
+    if(TINYGLTF_INSTALL_VENDOR)
+      INSTALL ( FILES
+        json.hpp
+        stb_image.h
+        stb_image_write.h
+        DESTINATION
+        include
+        )
+    endif()
+
 endif(TINYGLTF_INSTALL)

README.md

@@ -211,6 +211,11 @@ set(TINYGLTF_INSTALL OFF CACHE INTERNAL "" FORCE)
 add_subdirectory(/path/to/tinygltf)
 ```
 
+NOTE: Using tinygltf as a submodule doesn't automatically add the headers to your include path (as standard for many libraries). To get this functionality, add the following to the CMakeLists.txt file from above:
+
+```
+target_include_directories(${PROJECT_NAME} PRIVATE "/path/to/tinygltf")
+```
 
 ### Saving gltTF 2.0 model
 

tests/tester.cc

@@ -1199,4 +1199,51 @@ TEST_CASE("inverse-bind-matrices-optional", "[issue-492]") {
 
   REQUIRE(true == ret);
   REQUIRE(err.empty());
-}
+}
+
+bool LoadImageData(tinygltf::Image * /* image */, const int /* image_idx */, std::string * /* err */,
+                   std::string * /* warn */, int /* req_width */, int /* req_height */,
+                   const unsigned char * /* bytes */, int /* size */, void * /*user_data */) {
+    return true;
+}
+
+bool WriteImageData(const std::string * /* basepath */, const std::string * /* filename */,
+                    const tinygltf::Image *image, bool /* embedImages */,
+                    const tinygltf::FsCallbacks * /* fs_cb */, const tinygltf::URICallbacks * /* uri_cb */,
+                    std::string * /* out_uri */, void * user_pointer) {
+    REQUIRE(user_pointer != nullptr);
+    auto counter = static_cast<int*>(user_pointer);
+    *counter = *counter + 1;
+    return true;
+}
+
+TEST_CASE("empty-images-not-written", "[issue-495]") {
+  std::string err;
+  std::string warn;
+  tinygltf::Model model;
+  tinygltf::TinyGLTF ctx;
+
+  ctx.SetImageLoader(LoadImageData, nullptr);
+  bool ok = ctx.LoadASCIIFromFile(&model, &err, &warn, "../models/Cube/Cube.gltf");
+  REQUIRE(ok);
+  REQUIRE(err.empty());
+  REQUIRE(warn.empty());
+
+  CHECK(model.images.size() == 2);
+  for (const auto& image : model.images) {
+    // No data loaded or decoded
+    CHECK(image.image.empty());
+    // The URI is kept
+    CHECK_FALSE(image.uri.empty());
+    // The URI should not be a data URI
+    CHECK(image.uri.find("data:") != 0);
+  }
+
+  // Now write the loaded model
+  int counter = 0;
+  ctx.SetImageWriter(WriteImageData, &counter);
+  ok = ctx.WriteGltfSceneToFile(&model, "issue-495-external.gltf");
+  CHECK(ok);
+  // WriteImageData should be invoked for both images
+  CHECK(counter == 2);
+}

tiny_gltf.h

@@ -2767,6 +2767,12 @@ bool WriteImageData(const std::string *basepath, const std::string *filename,
                     const Image *image, bool embedImages,
                     const FsCallbacks* fs_cb, const URICallbacks *uri_cb,
                     std::string *out_uri, void *) {
+  // Early out on empty images, report the original uri if the image was not written.
+  if (image->image.empty()) {
+    *out_uri = *filename;
+    return true;
+  }
+
   const std::string ext = GetFilePathExtension(*filename);
 
   // Write image to temporary buffer
@@ -3309,11 +3315,12 @@ static bool UpdateImageObject(const Image &image, std::string &baseDir,
     filename = std::to_string(index) + "." + ext;
   }
 
-  // If callback is set and image data exists, modify image data object. If
-  // image data does not exist, this is not considered a failure and the
-  // original uri should be maintained.
+  // If callback is set, modify image data object.
+  // Note that the callback is also invoked for images without data.
+  // The default callback implementation simply returns true for
+  // empty images and sets the out URI to filename.
   bool imageWritten = false;
-  if (WriteImageData != nullptr && !filename.empty() && !image.image.empty()) {
+  if (WriteImageData != nullptr && !filename.empty()) {
     imageWritten = WriteImageData(&baseDir, &filename, &image, embedImages,
                                   fs_cb, uri_cb, out_uri, user_data);
     if (!imageWritten) {
@@ -4387,7 +4394,7 @@ static bool ParseImage(Image *image, const int image_idx, std::string *err,
     }
   } else {
     // Assume external file
-    // Keep texture path (for textures that cannot be decoded)
+    // Unconditionally keep the external URI of the image
     image->uri = uri;
 #ifdef TINYGLTF_NO_EXTERNAL_IMAGE
     return true;
@@ -4434,6 +4441,7 @@ static bool ParseImage(Image *image, const int image_idx, std::string *err,
     }
     return false;
   }
+
   return LoadImageData(image, image_idx, err, warn, 0, 0, &img.at(0),
                        static_cast<int>(img.size()), load_image_user_data);
 }
@@ -5597,7 +5605,7 @@ static bool ParseAnimation(Animation *animation, std::string *err,
         }
         sampler.input = inputIndex;
         sampler.output = outputIndex;
-        ParseExtrasAndExtensions(&sampler, err, o,
+        ParseExtrasAndExtensions(&sampler, err, s,
                                  store_original_json_for_extras_and_extensions);
 
         animation->samplers.emplace_back(std::move(sampler));
@@ -6445,6 +6453,15 @@ bool TinyGLTF::LoadFromString(Model *model, std::string *err, std::string *warn,
           return false;
         }
         const Buffer &buffer = model->buffers[size_t(bufferView.buffer)];
+        if (bufferView.byteOffset >= buffer.data.size()) {
+          if (err) {
+            std::stringstream ss;
+            ss << "image[" << idx << "] bufferView \"" << image.bufferView
+               << "\" indexed out of bounds of its buffer." << std::endl;
+            (*err) += ss.str();
+          }
+          return false;
+        }
 
         if (LoadImageData == nullptr) {
           if (err) {
@@ -7199,6 +7216,7 @@ static void SerializeGltfBufferData(const std::vector<unsigned char> &data,
 
 static bool SerializeGltfBufferData(const std::vector<unsigned char> &data,
                                     const std::string &binFilename) {
+#ifndef TINYGLTF_NO_FS
 #ifdef _WIN32
 #if defined(__GLIBCXX__)  // mingw
   int file_descriptor = _wopen(UTF8ToWchar(binFilename).c_str(),
@@ -7227,6 +7245,9 @@ static bool SerializeGltfBufferData(const std::vector<unsigned char> &data,
     // write empty file.
   }
   return true;
+#else
+  return false;
+#endif
 }
 
 #if 0  // FIXME(syoyo): not used. will be removed in the future release.
@@ -8448,6 +8469,7 @@ static bool WriteGltfStream(std::ostream &stream, const std::string &content) {
 
 static bool WriteGltfFile(const std::string &output,
                           const std::string &content) {
+#ifndef TINYGLTF_NO_FS
 #ifdef _WIN32
 #if defined(_MSC_VER)
   std::ofstream gltfFile(UTF8ToWchar(output).c_str());
@@ -8467,6 +8489,9 @@ static bool WriteGltfFile(const std::string &output,
   if (!gltfFile.is_open()) return false;
 #endif
   return WriteGltfStream(gltfFile, content);
+#else
+    return false;
+#endif
 }
 
 static bool WriteBinaryGltfStream(std::ostream &stream,
@@ -8533,6 +8558,7 @@ static bool WriteBinaryGltfStream(std::ostream &stream,
 static bool WriteBinaryGltfFile(const std::string &output,
                                 const std::string &content,
                                 const std::vector<unsigned char> &binBuffer) {
+#ifndef TINYGLTF_NO_FS
 #ifdef _WIN32
 #if defined(_MSC_VER)
   std::ofstream gltfFile(UTF8ToWchar(output).c_str(), std::ios::binary);
@@ -8549,6 +8575,9 @@ static bool WriteBinaryGltfFile(const std::string &output,
   std::ofstream gltfFile(output.c_str(), std::ios::binary);
 #endif
   return WriteBinaryGltfStream(gltfFile, content, binBuffer);
+#else
+    return false;
+#endif
 }
 
 bool TinyGLTF::WriteGltfSceneToStream(const Model *model, std::ostream &stream,