fix: race condition in TIFF reader (AcademySoftwareFoundation#3772)

* test: Add a corrupted test file

* fix(IC): upon tile read error, mark the file as broken

The idea is to keep fruitlessly reading from a corrupted file.

* fix: race condition in TIFF reader

TIFFInput::read_native_tiles had a very subtle race condition.  There
is a section where it has a task_set of decompression tasks that are
sent to the thread queue.  When the method ends, the task_set goes out
of scope, and its destructor will wait for all the remaining tasks to
finish. All fine, right? Except that other variables in the function
include unique_ptr's with allocated memory that the tasks need.  So
what can happen is that if the compiler generates the code such that
the destructor of the unique_ptr happens before the destructor of the
task_set, the memory that the unique_ptr held may be deallocated
before some of the tasks still running in threads have finished.

In practice, we seem only to hit this race condition for particular
cases involving corrupted files, because the early `return` seems
quite likely to run those destructors before all the threads are done,
but it was possible to encounter it for valid files as well, if the
timing is just right. Shocking that we never had it reported.

The solution is just to have an explitit tasks.wait(), which will
block until all the tasks have finished before hitting the actual end
of the method and letting the other destructors to run. And for the
"early return" case, change it to a break so that we go through the
wait call at the end.

I believe that this fix addresses TALOS-2023-1709 / CVE-2023-24472.

src/libtexture/imagecache.cpp

@@ -840,6 +840,7 @@ ImageCacheFile::read_tile(ImageCachePerThreadInfo* thread_info, int subimage,
         }
     }
     if (!ok) {
+        m_broken        = true;
         std::string err = inp->geterror();
         if (errors_should_issue()) {
             imagecache().error("{}",

src/tiff.imageio/tiffinput.cpp

@@ -2157,8 +2157,9 @@ TIFFInput::read_native_tiles(int subimage, int miplevel, int xbegin, int xend,
     //                  xbegin, xend, ybegin, yend, zbegin, zend);
     size_t tileidx = 0;
     for (int z = zbegin; z < zend; z += m_spec.tile_depth) {
-        for (int y = ybegin; y < yend; y += m_spec.tile_height) {
-            for (int x = xbegin; x < xend; x += m_spec.tile_width, ++tileidx) {
+        for (int y = ybegin; ok && y < yend; y += m_spec.tile_height) {
+            for (int x = xbegin; ok && x < xend;
+                 x += m_spec.tile_width, ++tileidx) {
                 char* cbuf = compressed_scratch.get() + tileidx * cbound;
                 char* ubuf = scratch.get() + tileidx * tile_bytes;
                 auto csize = TIFFReadRawTile(m_tif, tile_index(x, y, z), cbuf,
@@ -2168,7 +2169,8 @@ TIFFInput::read_native_tiles(int subimage, int miplevel, int xbegin, int xend,
                     errorf(
                         "TIFFReadRawTile failed reading tile x=%d,y=%d,z=%d: %s",
                         x, y, z, err.size() ? err.c_str() : "unknown error");
-                    return false;
+                    ok = false;
+                    break;
                 }
                 // Push the rest of the work onto the thread pool queue
                 auto out = this;
@@ -2193,6 +2195,7 @@ TIFFInput::read_native_tiles(int subimage, int miplevel, int xbegin, int xend,
             }
         }
     }
+    tasks.wait();
     return ok;
 }
 

testsuite/tiff-misc/ref/out-libtiff403.txt

@@ -22,6 +22,8 @@ Full command line was:
 > oiiotool --oiioattrib try_all_readers 0 --info -v src/crash-1633.tif
 oiiotool ERROR: read : File does not exist: "src/crash-1643.tif"
 Full command line was:
-> oiiotool --oiioattrib try_all_readers 0 --info -v src/crash-1643.tif
+> oiiotool --oiioattrib try_all_readers 0 --info src/crash-1643.tif -o out.exr
+iconvert ERROR copying "src/crash-1709.tif" to "crash-1709.exr" :
+	Decoding error at scanline 0, incorrect header check
 Comparing "check1.tif" and "ref/check1.tif"
 PASS

testsuite/tiff-misc/ref/out-libtiff410.txt

@@ -0,0 +1,29 @@
+Reading src/separate.tif
+src/separate.tif     :  128 x  128, 3 channel, uint8 tiff
+    SHA-1: 486088DECAE711C444FDCAB009C378F7783AD9C5
+    channel list: R, G, B
+    compression: "zip"
+    DateTime: "2020:10:25 15:32:04"
+    Orientation: 1 (normal)
+    planarconfig: "separate"
+    Software: "OpenImageIO 2.3.0dev : oiiotool --pattern fill:topleft=0,0,0:topright=1,0,0:bottomleft=0,1,0:bottomright=1,1,1 128x128 3 --planarconfig separate -scanline -attrib tiff:rowsperstrip 17 -d uint8 -o separate.tif"
+    oiio:BitsPerSample: 8
+    tiff:Compression: 8
+    tiff:PhotometricInterpretation: 2
+    tiff:PlanarConfiguration: 2
+    tiff:RowsPerStrip: 7
+Comparing "src/separate.tif" and "separate.tif"
+PASS
+oiiotool ERROR: read : No support for data format of "src/corrupt1.tif"
+Full command line was:
+> oiiotool --oiioattrib try_all_readers 0 --info -v src/corrupt1.tif
+oiiotool ERROR: read : File does not exist: "src/crash-1633.tif"
+Full command line was:
+> oiiotool --oiioattrib try_all_readers 0 --info -v src/crash-1633.tif
+oiiotool ERROR: read : File does not exist: "src/crash-1643.tif"
+Full command line was:
+> oiiotool --oiioattrib try_all_readers 0 --info src/crash-1643.tif -o out.exr
+iconvert ERROR copying "src/crash-1709.tif" to "crash-1709.exr" :
+	Decoding error at scanline 0, incorrect header check
+Comparing "check1.tif" and "ref/check1.tif"
+PASS

testsuite/tiff-misc/ref/out-libtiff450.txt

@@ -0,0 +1,29 @@
+Reading src/separate.tif
+src/separate.tif     :  128 x  128, 3 channel, uint8 tiff
+    SHA-1: 486088DECAE711C444FDCAB009C378F7783AD9C5
+    channel list: R, G, B
+    compression: "zip"
+    DateTime: "2020:10:25 15:32:04"
+    Orientation: 1 (normal)
+    planarconfig: "separate"
+    Software: "OpenImageIO 2.3.0dev : oiiotool --pattern fill:topleft=0,0,0:topright=1,0,0:bottomleft=0,1,0:bottomright=1,1,1 128x128 3 --planarconfig separate -scanline -attrib tiff:rowsperstrip 17 -d uint8 -o separate.tif"
+    oiio:BitsPerSample: 8
+    tiff:Compression: 8
+    tiff:PhotometricInterpretation: 2
+    tiff:PlanarConfiguration: 2
+    tiff:RowsPerStrip: 7
+Comparing "src/separate.tif" and "separate.tif"
+PASS
+oiiotool ERROR: read : No support for data format of "src/corrupt1.tif"
+Full command line was:
+> oiiotool --oiioattrib try_all_readers 0 --info -v src/corrupt1.tif
+oiiotool ERROR: read : File does not exist: "src/crash-1633.tif"
+Full command line was:
+> oiiotool --oiioattrib try_all_readers 0 --info -v src/crash-1633.tif
+oiiotool ERROR: read : File does not exist: "src/crash-1643.tif"
+Full command line was:
+> oiiotool --oiioattrib try_all_readers 0 --info src/crash-1643.tif -o out.exr
+iconvert ERROR copying "src/crash-1709.tif" to "crash-1709.exr" :
+	Decoding error at scanline 0
+Comparing "check1.tif" and "ref/check1.tif"
+PASS

testsuite/tiff-misc/ref/out.txt

@@ -22,6 +22,8 @@ Full command line was:
 > oiiotool --oiioattrib try_all_readers 0 --info -v src/crash-1633.tif
 oiiotool ERROR: read : File does not exist: "src/crash-1643.tif"
 Full command line was:
-> oiiotool --oiioattrib try_all_readers 0 --info -v src/crash-1643.tif
+> oiiotool --oiioattrib try_all_readers 0 --info src/crash-1643.tif -o out.exr
+iconvert ERROR copying "src/crash-1709.tif" to "crash-1709.exr" :
+
 Comparing "check1.tif" and "ref/check1.tif"
 PASS

testsuite/tiff-misc/run.py

@@ -17,6 +17,7 @@
 # Test bugs we had until OIIO 2.4 for these corrupt file
 command += oiiotool ("--oiioattrib try_all_readers 0 --info -v src/corrupt1.tif", failureok = True)
 command += oiiotool ("--oiioattrib try_all_readers 0 --info -v src/crash-1633.tif", failureok = True)
-command += oiiotool ("--oiioattrib try_all_readers 0 --info -v src/crash-1643.tif", failureok = True)
+command += oiiotool ("--oiioattrib try_all_readers 0 --info src/crash-1643.tif -o out.exr", failureok = True)
+command += iconvert ("src/crash-1709.tif crash-1709.exr", failureok=True)
 
 outputs = [ "check1.tif", "out.txt" ]