Fail cleanly on a truncated or corrupt PNG

libpng reports a bad file by longjmp()ing back to the buffer set with
setjmp(), and the frame it lands in must own nothing that needs
destroying: with exceptions enabled MSVC unwinds the stack as part of
longjmp, and returning from a frame unwound that way crashes. It did on
Windows while working everywhere else.

The read callback also returned quietly on a short read, leaving libpng
to decode whatever happened to be in the output buffer.

The calls that can fail now sit in two helpers that own nothing but
pointers, so every C++ object the decoders need stays in their own
frames, and a short read is reported through png_error().

src/libslic3r/PNGReadWrite.cpp

@@ -54,9 +54,43 @@ static void png_read_callback(png_struct *png_ptr,
     // Retrieve our input buffer through the png_ptr
     auto reader = static_cast<IStream *>(png_get_io_ptr(png_ptr));
 
-    if (!reader || !reader->is_ok()) return;
+    // libpng expects a short read to be reported through png_error(); returning quietly would leave
+    // it decoding whatever happened to be in outBytes.
+    if (!reader || !reader->is_ok() ||
+        reader->read(static_cast<std::uint8_t *>(outBytes), byteCountToRead) != byteCountToRead)
+        png_error(png_ptr, "PNG data is truncated");
+}
 
-    reader->read(static_cast<std::uint8_t *>(outBytes), byteCountToRead);
+// libpng reports a corrupt or truncated image by longjmp()ing back to the jump buffer set with
+// setjmp(). The frame it lands in must own nothing that needs destroying: with exceptions enabled
+// MSVC unwinds the stack as part of longjmp, and returning from a frame unwound that way crashes -
+// which is what a truncated texture did on Windows while working everywhere else. So the calls that
+// can fail live in these two helpers, which hold nothing but pointers, and every C++ object the
+// decoders need stays in their own frames.
+static bool png_read_header_guarded(png_struct *png, png_info *info, IStream *in_buf, int sig_bytes)
+{
+    if (setjmp(png_jmpbuf(png)))
+        return false;
+
+    png_set_read_fn(png, static_cast<void *>(in_buf), png_read_callback);
+    // Tell that we have already read the first bytes to check the signature
+    png_set_sig_bytes(png, sig_bytes);
+    png_read_info(png, info);
+    return true;
+}
+
+// `bottom_up` fills the buffer last row first, which is the order the colour decoder hands back.
+static bool png_read_rows_guarded(png_struct *png, png_info *info, png_bytep dst, size_t rows, size_t rowbytes,
+                                  bool bottom_up, bool read_end)
+{
+    if (setjmp(png_jmpbuf(png)))
+        return false;
+
+    for (size_t i = 0; i < rows; ++i)
+        png_read_row(png, dst + (bottom_up ? rows - 1 - i : i) * rowbytes, nullptr);
+    if (read_end)
+        png_read_end(png, info);
+    return true;
 }
 
 bool decode_png(IStream &in_buf, ImageGreyscale &out_img)
@@ -77,12 +111,8 @@ bool decode_png(IStream &in_buf, ImageGreyscale &out_img)
     dsc.info = png_create_info_struct(dsc.png);
     if(!dsc.info) return false;
 
-    png_set_read_fn(dsc.png, static_cast<void *>(&in_buf), png_read_callback);
+    if (!png_read_header_guarded(dsc.png, dsc.info, &in_buf, PNG_SIG_BYTES))
-
+        return false;
-    // Tell that we have already read the first bytes to check the signature
-    png_set_sig_bytes(dsc.png, PNG_SIG_BYTES);
-
-    png_read_info(dsc.png, dsc.info);
 
     out_img.cols = png_get_image_width(dsc.png, dsc.info);
     out_img.rows = png_get_image_height(dsc.png, dsc.info);
@@ -94,11 +124,8 @@ bool decode_png(IStream &in_buf, ImageGreyscale &out_img)
 
     out_img.buf.resize(out_img.rows * out_img.cols);
 
-    auto readbuf = static_cast<png_bytep>(out_img.buf.data());
+    return png_read_rows_guarded(dsc.png, dsc.info, static_cast<png_bytep>(out_img.buf.data()), out_img.rows,
-    for (size_t r = 0; r < out_img.rows; ++r)
+                                 out_img.cols, /* bottom_up */ false, /* read_end */ false);
-        png_read_row(dsc.png, readbuf + r * out_img.cols, nullptr);
-
-    return true;
 }
 
 bool decode_colored_png(IStream &in_buf, ImageColorscale &out_img)
@@ -128,12 +155,10 @@ bool decode_colored_png(IStream &in_buf, ImageColorscale &out_img)
         return false;
     }
 
-    png_set_read_fn(dsc.png, static_cast<void *>(&in_buf), png_read_callback);
+    if (!png_read_header_guarded(dsc.png, dsc.info, &in_buf, PNG_SIG_BYTES)) {
-
+        BOOST_LOG_TRIVIAL(error) << "decode_colored_png: corrupt or truncated PNG data";
-    // Tell that we have already read the first bytes to check the signature
+        return false;
-    png_set_sig_bytes(dsc.png, PNG_SIG_BYTES);
+    }
-
-    png_read_info(dsc.png, dsc.info);
 
     out_img.cols = png_get_image_width(dsc.png, dsc.info);
     out_img.rows = png_get_image_height(dsc.png, dsc.info);
@@ -162,13 +187,12 @@ bool decode_colored_png(IStream &in_buf, ImageColorscale &out_img)
     int interlace_type = png_get_interlace_type(dsc.png, dsc.info);
     BOOST_LOG_TRIVIAL(info) << boost::format("filter_type %1%, compression_type %2%, interlace_type %3%, rowbytes %4%")%filter_type %compression_type %interlace_type %rowbytes;
 
-    auto readbuf = static_cast<png_bytep>(out_img.buf.data());
+    if (!png_read_rows_guarded(dsc.png, dsc.info, static_cast<png_bytep>(out_img.buf.data()), out_img.rows, rowbytes,
-    for (size_t r = out_img.rows; r > 0; r--)
+                               /* bottom_up */ true, /* read_end */ true)) {
-    {
+        BOOST_LOG_TRIVIAL(error) << "decode_colored_png: corrupt or truncated PNG data";
-        png_read_row(dsc.png, readbuf + (r - 1) * rowbytes, nullptr);
+        return false;
     }
 
-    png_read_end(dsc.png, dsc.info);
     png_destroy_read_struct(&dsc.png, &dsc.info, NULL);
 
     return true;

tests/libslic3r/CMakeLists.txt

@@ -32,6 +32,7 @@ add_executable(${_TEST_NAME}_tests
     test_geometry.cpp
     test_multimaterial_segmentation.cpp
     test_placeholder_parser.cpp
+    test_png_read_write.cpp
     test_polygon.cpp
     test_mutable_polygon.cpp
     test_mutable_priority_queue.cpp

tests/libslic3r/test_png_read_write.cpp

@@ -0,0 +1,83 @@
+#include <catch2/catch_all.hpp>
+
+#include <cstdint>
+#include <fstream>
+#include <iterator>
+#include <vector>
+
+#include <boost/filesystem.hpp>
+
+#include "libslic3r/PNGReadWrite.hpp"
+
+using namespace Slic3r;
+
+// libpng reports a corrupt or truncated file by longjmp()ing out of the decoder, so the decoders have
+// to come back with false rather than crash or hand back a half filled image.
+namespace {
+
+// A real PNG, produced by the writer next door, so the bytes are a file libpng accepts.
+std::vector<uint8_t> encoded_png(size_t w, size_t h)
+{
+    std::vector<uint8_t> pixels(w * h);
+    for (size_t i = 0; i < pixels.size(); ++ i)
+        pixels[i] = uint8_t((i * 7) % 256);
+
+    const boost::filesystem::path path = boost::filesystem::temp_directory_path() /
+                                         boost::filesystem::unique_path("png_rw_%%%%%%%%.png");
+    REQUIRE(png::write_gray_to_file(path.string(), w, h, pixels));
+    std::vector<uint8_t> bytes;
+    {
+        std::ifstream ifs(path.string(), std::ios::binary);
+        bytes.assign(std::istreambuf_iterator<char>(ifs), std::istreambuf_iterator<char>());
+    }
+    boost::system::error_code ec;
+    boost::filesystem::remove(path, ec);
+    REQUIRE(bytes.size() > 64);
+    return bytes;
+}
+
+png::ReadBuf buf_of(const std::vector<uint8_t> &bytes, size_t size)
+{
+    return png::ReadBuf{ bytes.data(), size };
+}
+
+} // namespace
+
+TEST_CASE("A whole PNG decodes", "[PNG]") {
+    const std::vector<uint8_t> bytes = encoded_png(24, 16);
+
+    png::ImageGreyscale grey;
+    REQUIRE(png::decode_png(buf_of(bytes, bytes.size()), grey));
+    CHECK(grey.cols == 24);
+    CHECK(grey.rows == 16);
+    CHECK(grey.buf.size() == 24 * 16);
+}
+
+TEST_CASE("A truncated PNG is refused instead of crashing", "[PNG]") {
+    const std::vector<uint8_t> bytes = encoded_png(64, 64);
+
+    // Cut past the signature: inside the header, and inside the pixel data. Not in the trailing
+    // chunks - decode_png() does not read those, so a file missing only its IEND still decodes, and
+    // that is the pre-existing contract rather than anything this change touches.
+    const size_t size = GENERATE_COPY(size_t(16), size_t(40), bytes.size() / 2, bytes.size() * 3 / 4);
+    REQUIRE(size < bytes.size());
+
+    png::ImageGreyscale grey;
+    CHECK_FALSE(png::decode_png(buf_of(bytes, size), grey));
+
+    png::ImageColorscale colour;
+    CHECK_FALSE(png::decode_colored_png(buf_of(bytes, size), colour));
+}
+
+TEST_CASE("A PNG whose body is garbage is refused", "[PNG]") {
+    std::vector<uint8_t> bytes = encoded_png(32, 32);
+    // Keep the signature, scribble over everything after it.
+    for (size_t i = 8; i < bytes.size(); ++ i)
+        bytes[i] = uint8_t(0xA5);
+
+    png::ImageGreyscale grey;
+    CHECK_FALSE(png::decode_png(buf_of(bytes, bytes.size()), grey));
+
+    png::ImageColorscale colour;
+    CHECK_FALSE(png::decode_colored_png(buf_of(bytes, bytes.size()), colour));
+}