From 7682998c048c446b8a5339114635ad0c7855c9ac Mon Sep 17 00:00:00 2001 From: Robert KOFLER Date: Mon, 29 Jun 2026 09:29:46 +0200 Subject: [PATCH] Harden mergeTiffParts against part files missing geometry tags (SIGFPE) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit mergeTiffParts read TIFFTAG_IMAGEWIDTH/IMAGELENGTH/TILEWIDTH/TILELENGTH (and bps/spp/sf/comp) from the first part file into uninitialized locals without checking the TIFFGetField return value. A part file from a killed or corrupt distributed render job can be missing those tags; in particular a stripped (non-tiled) TIFF has no TILEWIDTH/TILELENGTH, leaving tw/th uninitialized, and the tile grid computation `(w + tw - 1) / tw` then divides by zero — a SIGFPE that takes down the whole merge. Initialize the geometry/format locals to 0, require the four geometry tags via the TIFFGetField return value, and skip the group (count it as a failure and continue) when any are missing or zero, instead of crashing. Adds a test_tiff_merge case that writes a stripped part TIFF and merges it: the unfixed merge SIGFPEs (signal 8), the hardened one returns false and writes nothing. Existing happy-path merge tests still pass. --- volume-cartographer/core/src/Tiff.cpp | 23 ++++++++--- .../core/test/test_tiff_merge.cpp | 40 +++++++++++++++++++ 2 files changed, 58 insertions(+), 5 deletions(-) diff --git a/volume-cartographer/core/src/Tiff.cpp b/volume-cartographer/core/src/Tiff.cpp index a09ad79af5..16dbc90f39 100644 --- a/volume-cartographer/core/src/Tiff.cpp +++ b/volume-cartographer/core/src/Tiff.cpp @@ -311,15 +311,28 @@ bool mergeTiffParts(const std::string& outputPath, int numParts) std::sort(partFiles.begin(), partFiles.end()); TIFF* first = TIFFOpen(partFiles[0].c_str(), "r"); if (!first) { std::cerr << "Cannot open " << partFiles[0] << "\n"; failures++; continue; } - uint32_t w, h, tw, th; uint16_t bps, spp, sf, comp; - TIFFGetField(first, TIFFTAG_IMAGEWIDTH, &w); - TIFFGetField(first, TIFFTAG_IMAGELENGTH, &h); - TIFFGetField(first, TIFFTAG_TILEWIDTH, &tw); - TIFFGetField(first, TIFFTAG_TILELENGTH, &th); + // Initialize and check TIFFGetField: a part file from a killed/corrupt + // render job may be missing tags. Reading an unset tag leaves the + // variable uninitialized, and a missing TILEWIDTH/TILELENGTH makes tw/th + // zero -> the (w + tw - 1) / tw tiling below is a divide-by-zero. Skip + // such a group cleanly instead of crashing. + uint32_t w = 0, h = 0, tw = 0, th = 0; uint16_t bps = 0, spp = 0, sf = 0, comp = 0; + const bool haveGeom = + TIFFGetField(first, TIFFTAG_IMAGEWIDTH, &w) + && TIFFGetField(first, TIFFTAG_IMAGELENGTH, &h) + && TIFFGetField(first, TIFFTAG_TILEWIDTH, &tw) + && TIFFGetField(first, TIFFTAG_TILELENGTH, &th); TIFFGetField(first, TIFFTAG_BITSPERSAMPLE, &bps); TIFFGetField(first, TIFFTAG_SAMPLESPERPIXEL, &spp); TIFFGetField(first, TIFFTAG_SAMPLEFORMAT, &sf); TIFFGetField(first, TIFFTAG_COMPRESSION, &comp); + if (!haveGeom || w == 0 || h == 0 || tw == 0 || th == 0) { + std::cerr << "Skipping " << finalPath + << ": first part missing/zero geometry tags\n"; + TIFFClose(first); + failures++; + continue; + } uint16_t resUnit = 0; float xRes = 0, yRes = 0; bool hasRes = TIFFGetField(first, TIFFTAG_RESOLUTIONUNIT, &resUnit) && TIFFGetField(first, TIFFTAG_XRESOLUTION, &xRes) && diff --git a/volume-cartographer/core/test/test_tiff_merge.cpp b/volume-cartographer/core/test/test_tiff_merge.cpp index 256dfbb4c0..c17639810c 100644 --- a/volume-cartographer/core/test/test_tiff_merge.cpp +++ b/volume-cartographer/core/test/test_tiff_merge.cpp @@ -9,10 +9,13 @@ #include #include +#include + #include #include #include #include +#include namespace fs = std::filesystem; @@ -69,3 +72,40 @@ TEST_CASE("mergeTiffParts: path is a file, parent dir is scanned") CHECK(fs::exists(finalFile)); fs::remove_all(d); } + +namespace { +// Write a STRIPPED (non-tiled) TIFF, i.e. one with no TILEWIDTH/TILELENGTH +// tags — what a corrupt or truncated render part might look like. +void writeStrippedTiff(const fs::path& path, int rows, int cols, uint8_t v) +{ + TIFF* t = TIFFOpen(path.string().c_str(), "w"); + REQUIRE(t != nullptr); + TIFFSetField(t, TIFFTAG_IMAGEWIDTH, uint32_t(cols)); + TIFFSetField(t, TIFFTAG_IMAGELENGTH, uint32_t(rows)); + TIFFSetField(t, TIFFTAG_BITSPERSAMPLE, uint16_t(8)); + TIFFSetField(t, TIFFTAG_SAMPLESPERPIXEL, uint16_t(1)); + TIFFSetField(t, TIFFTAG_PLANARCONFIG, PLANARCONFIG_CONTIG); + TIFFSetField(t, TIFFTAG_PHOTOMETRIC, PHOTOMETRIC_MINISBLACK); + // No TILEWIDTH/TILELENGTH -> libtiff writes scanline strips. + std::vector row(size_t(cols), v); + for (int r = 0; r < rows; ++r) + TIFFWriteScanline(t, row.data(), uint32_t(r), 0); + TIFFClose(t); +} +} // namespace + +TEST_CASE("mergeTiffParts: a part missing tile-geometry tags is skipped, not crashed") +{ + auto d = tmpDir("stripped"); + // A stripped part TIFF has no TILEWIDTH/TILELENGTH. On the unfixed merge, + // tw/th stay uninitialized and (w + tw - 1) / tw divides by zero / reads + // garbage. The hardened merge must skip the group and report failure + // (returns false) without crashing. + writeStrippedTiff(d / "bad.part0.tif", 32, 32, 7); + + auto finalPath = (d / "bad.tif").string(); + bool ok = mergeTiffParts(finalPath, /*numParts=*/1); + CHECK_FALSE(ok); // the only group failed + CHECK_FALSE(fs::exists(finalPath)); // nothing written + fs::remove_all(d); +}