From a862a9267f67bbf5100654712a83fc50556f2820 Mon Sep 17 00:00:00 2001 From: ExPikaPaka Date: Wed, 30 Sep 2026 08:59:38 +0200 Subject: [PATCH] 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 | 72 ++++++++++++++------- tests/libslic3r/CMakeLists.txt | 1 + tests/libslic3r/test_png_read_write.cpp | 83 +++++++++++++++++++++++++ 3 files changed, 132 insertions(+), 24 deletions(-) create mode 100644 tests/libslic3r/test_png_read_write.cpp diff --git a/src/libslic3r/PNGReadWrite.cpp b/src/libslic3r/PNGReadWrite.cpp index 32b9a8f79e..9f6a8b739d 100644 --- a/src/libslic3r/PNGReadWrite.cpp +++ b/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(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(outBytes), byteCountToRead) != byteCountToRead) + png_error(png_ptr, "PNG data is truncated"); +} - reader->read(static_cast(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(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(&in_buf), png_read_callback); - - // 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); + if (!png_read_header_guarded(dsc.png, dsc.info, &in_buf, PNG_SIG_BYTES)) + return false; 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(out_img.buf.data()); - for (size_t r = 0; r < out_img.rows; ++r) - png_read_row(dsc.png, readbuf + r * out_img.cols, nullptr); - - return true; + return png_read_rows_guarded(dsc.png, dsc.info, static_cast(out_img.buf.data()), out_img.rows, + out_img.cols, /* bottom_up */ false, /* read_end */ false); } 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(&in_buf), png_read_callback); - - // 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); + 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"; + return false; + } 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(out_img.buf.data()); - for (size_t r = out_img.rows; r > 0; r--) - { - png_read_row(dsc.png, readbuf + (r - 1) * rowbytes, nullptr); + if (!png_read_rows_guarded(dsc.png, dsc.info, static_cast(out_img.buf.data()), out_img.rows, rowbytes, + /* bottom_up */ true, /* read_end */ true)) { + BOOST_LOG_TRIVIAL(error) << "decode_colored_png: corrupt or truncated PNG data"; + return false; } - png_read_end(dsc.png, dsc.info); png_destroy_read_struct(&dsc.png, &dsc.info, NULL); return true; diff --git a/tests/libslic3r/CMakeLists.txt b/tests/libslic3r/CMakeLists.txt index 911e23d5c7..5ee3ca6b9b 100644 --- a/tests/libslic3r/CMakeLists.txt +++ b/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 diff --git a/tests/libslic3r/test_png_read_write.cpp b/tests/libslic3r/test_png_read_write.cpp new file mode 100644 index 0000000000..2aa28e1a3a --- /dev/null +++ b/tests/libslic3r/test_png_read_write.cpp @@ -0,0 +1,83 @@ +#include + +#include +#include +#include +#include + +#include + +#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 encoded_png(size_t w, size_t h) +{ + std::vector 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 bytes; + { + std::ifstream ifs(path.string(), std::ios::binary); + bytes.assign(std::istreambuf_iterator(ifs), std::istreambuf_iterator()); + } + boost::system::error_code ec; + boost::filesystem::remove(path, ec); + REQUIRE(bytes.size() > 64); + return bytes; +} + +png::ReadBuf buf_of(const std::vector &bytes, size_t size) +{ + return png::ReadBuf{ bytes.data(), size }; +} + +} // namespace + +TEST_CASE("A whole PNG decodes", "[PNG]") { + const std::vector 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 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 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)); +}