fix(openexr): Fix out-of-bounds reads when using OpenEXR decreasingY …

…lineOrder. (AcademySoftwareFoundation#4215) This change fixes out-of-bounds reads and incorrect scanline ordering when OpenEXR's decreasingY lineOrder mode is used. OpenEXR expects to process scanlines in decreasing order when the decreasingY lineOrder option is enabled. This change fixes the code that provides the chunks of scanlines to OpenEXR so that the proper scanlines are available when it accesses the FrameBuffer OIIO created. The old code was providing incorrect scanlines AND setting up the FrameBuffer with incorrect pointers so out-of-bounds reads were occurring. The key things to keep in mind are: - Chunks of scanlines need to be sent to OpenEXROutput::write_scanlines() from the bottom of the image to the top when decreasingY is enabled. If the chunk's scanline range does not contain the scanlines OpenEXR is expecting to fetch, then it can cause out-of-bound reads because the wrong information is used to construct the pointers for the Slices created when constructing a FrameBuffer. - OpenEXROutput::write_scanlines() does its own chunking of scanlines and this logic must also properly start from the bottom of the chunk data passed into this function when decreasingY is enabled. - The to_native_rectangle() call in OpenEXROutput::write_scanlines() may return a pointer to m_scratch if a format conversion is needed. This means the pointers passed to OpenEXR, in this situation, _WILL NOT_ be a full frame buffer. This caused OpenEXR to do out-of-bounds reads because the computations used to create the Slices were using the wrong scanline number to adjust the pointer. - The progress callback computation needed to be modified to handle traversing scanlines in decreasing order. I added an openexr-decreasingY test to verify that the code no longer crashes because of the out-of-bound reads and it also verifies that the decreasingY images match the increasingY images. --------- Signed-off-by: Aaron Colwell <[email protected]>
lgritz · Apr 13, 2024 · 020c6e6 · 020c6e6
1 parent 4d1c392
commit 020c6e6
Show file tree

Hide file tree

Showing 6 changed files with 180 additions and 21 deletions.
diff --git a/src/cmake/testing.cmake b/src/cmake/testing.cmake
@@ -278,7 +278,7 @@ macro (oiio_add_all_tests)
                     IMAGEDIR j2kp4files_v1_5
                     URL http://www.itu.int/net/ITU-T/sigdb/speimage/ImageForm-s.aspx?val=10100803)
     set (all_openexr_tests
-         openexr-suite openexr-multires openexr-chroma
+         openexr-suite openexr-multires openexr-chroma openexr-decreasingy
          openexr-v2 openexr-window perchannel oiiotool-deep)
     if (USE_PYTHON AND NOT SANITIZE)
         list (APPEND all_openexr_tests openexr-copy)

diff --git a/src/libOpenImageIO/imagebuf.cpp b/src/libOpenImageIO/imagebuf.cpp
@@ -1418,8 +1418,23 @@ ImageBuf::write(ImageOutput* out, ProgressCallback progress_callback,
             int chunk = clamp(round_to_multiple(int(budget / slsize), 64), 1,
                               1024);
             std::unique_ptr<char[]> tmp(new char[chunk * slsize]);
+
+            // Special handling for flipped vertical scanline order. Right now, OpenEXR
+            // is the only format that allows it, so we special case it by name. For
+            // just one format, trying to be more general just seems even more awkward.
+            const bool isDecreasingY = !strcmp(out->format_name(), "openexr")
+                                       && outspec.get_string_attribute(
+                                              "openexr:lineOrder")
+                                              == "decreasingY";
+            const int numChunks  = outspec.height > 0
+                                       ? 1 + ((outspec.height - 1) / chunk)
+                                       : 0;
+            const int yLoopStart = isDecreasingY ? (numChunks - 1) * chunk : 0;
+            const int yDelta     = isDecreasingY ? -chunk : chunk;
+            const int yLoopEnd   = yLoopStart + numChunks * yDelta;
+
             for (int z = 0; z < outspec.depth; ++z) {
-                for (int y = 0; y < outspec.height && ok; y += chunk) {
+                for (int y = yLoopStart; y != yLoopEnd && ok; y += yDelta) {
                     int yend = std::min(y + outspec.y + chunk,
                                         outspec.y + outspec.height);
                     ok &= get_pixels(ROI(outspec.x, outspec.x + outspec.width,
@@ -1430,10 +1445,12 @@ ImageBuf::write(ImageOutput* out, ProgressCallback progress_callback,
                                                z + outspec.z, bufformat,
                                                &tmp[0]);
                     if (progress_callback
-                        && progress_callback(progress_callback_data,
-                                             (float)(z * outspec.height + y)
-                                                 / (outspec.height
-                                                    * outspec.depth)))
+                        && progress_callback(
+                            progress_callback_data,
+                            (float)(z * outspec.height
+                                    + (isDecreasingY ? (outspec.height - 1 - y)
+                                                     : y))
+                                / (outspec.height * outspec.depth)))
                         return ok;
                 }
             }

diff --git a/src/libOpenImageIO/imageoutput.cpp b/src/libOpenImageIO/imageoutput.cpp
@@ -518,17 +518,34 @@ ImageOutput::write_image(TypeDesc format, const void* data, stride_t xstride,
         int rps   = m_spec.get_int_attribute("tiff:RowsPerStrip", 64);
         int chunk = std::max(1, (1 << 26) / int(m_spec.scanline_bytes(true)));
         chunk     = round_to_multiple(chunk, rps);
+
+        // Special handling for flipped vertical scanline order. Right now, OpenEXR
+        // is the only format that allows it, so we special case it by name. For
+        // just one format, trying to be more general just seems even more awkward.
+        const bool isDecreasingY = !strcmp(format_name(), "openexr")
+                                   && m_spec.get_string_attribute(
+                                          "openexr:lineOrder")
+                                          == "decreasingY";
+        const int numChunks  = m_spec.height > 0
+                                   ? 1 + ((m_spec.height - 1) / chunk)
+                                   : 0;
+        const int yLoopStart = isDecreasingY ? (numChunks - 1) * chunk : 0;
+        const int yDelta     = isDecreasingY ? -chunk : chunk;
+        const int yLoopEnd   = yLoopStart + numChunks * yDelta;
+
         for (int z = 0; z < m_spec.depth; ++z)
-            for (int y = 0; y < m_spec.height && ok; y += chunk) {
+            for (int y = yLoopStart; y != yLoopEnd && ok; y += yDelta) {
                 int yend      = std::min(y + m_spec.y + chunk,
                                          m_spec.y + m_spec.height);
                 const char* d = (const char*)data + z * zstride + y * ystride;
                 ok &= write_scanlines(y + m_spec.y, yend, z + m_spec.z, format,
                                       d, xstride, ystride);
                 if (progress_callback
-                    && progress_callback(progress_callback_data,
-                                         (float)(z * m_spec.height + y)
-                                             / (m_spec.height * m_spec.depth)))
+                    && progress_callback(
+                        progress_callback_data,
+                        (float)(z * m_spec.height
+                                + (isDecreasingY ? (m_spec.height - 1 - y) : y))
+                            / (m_spec.height * m_spec.depth)))
                     return ok;
             }
     }

diff --git a/src/openexr.imageio/exroutput.cpp b/src/openexr.imageio/exroutput.cpp
@@ -1485,21 +1485,33 @@ OpenEXROutput::write_scanlines(int ybegin, int yend, int z, TypeDesc format,
                               * 1024;  // Allocate 16 MB, or 1 scanline
     int chunk = std::max(1, int(limit / scanlinebytes));
 
-    bool ok = true;
-    for (; ok && ybegin < yend; ybegin += chunk) {
-        int y1         = std::min(ybegin + chunk, yend);
-        int nscanlines = y1 - ybegin;
-        const void* d  = to_native_rectangle(m_spec.x, m_spec.x + m_spec.width,
-                                             ybegin, y1, z, z + 1, format, data,
-                                             xstride, ystride, zstride,
-                                             m_scratch);
+    bool ok                  = true;
+    const bool isDecreasingY = m_spec.get_string_attribute("openexr:lineOrder")
+                               == "decreasingY";
+    const int nAvailableScanLines = yend - ybegin;
+    const int numChunks           = nAvailableScanLines > 0
+                                        ? 1 + ((nAvailableScanLines - 1) / chunk)
+                                        : 0;
+    const int yLoopStart = isDecreasingY ? ybegin + (numChunks - 1) * chunk
+                                         : ybegin;
+    const int yDelta     = isDecreasingY ? -chunk : chunk;
+    const int yLoopEnd   = yLoopStart + numChunks * yDelta;
+    for (int y = yLoopStart; ok && y != yLoopEnd; y += yDelta) {
+        int y1         = std::min(y + chunk, yend);
+        int nscanlines = y1 - y;
+
+        const void* dataStart = (const char*)data + (y - ybegin) * ystride;
+        const void* d = to_native_rectangle(m_spec.x, m_spec.x + m_spec.width,
+                                            y, y1, z, z + 1, format, dataStart,
+                                            xstride, ystride, zstride,
+                                            m_scratch);
 
         // Compute where OpenEXR needs to think the full buffers starts.
         // OpenImageIO requires that 'data' points to where client stored
         // the bytes to be written, but OpenEXR's frameBuffer.insert() wants
         // where the address of the "virtual framebuffer" for the whole
         // image.
-        char* buf = (char*)d - m_spec.x * pixel_bytes - ybegin * scanlinebytes;
+        char* buf = (char*)d - m_spec.x * pixel_bytes - y * scanlinebytes;
         try {
             Imf::FrameBuffer frameBuffer;
             size_t chanoffset = 0;
@@ -1527,8 +1539,6 @@ OpenEXROutput::write_scanlines(int ybegin, int yend, int z, TypeDesc format,
             errorf("Failed OpenEXR write: unknown exception");
             return false;
         }
-
-        data = (const char*)data + ystride * nscanlines;
     }
 
     // If we allocated more than 1M, free the memory.  It's not wasteful,

diff --git a/testsuite/openexr-decreasingy/ref/out.txt b/testsuite/openexr-decreasingy/ref/out.txt
@@ -0,0 +1,74 @@
+Reading ref/decreasingY-resize.exr
+ref/decreasingY-resize.exr : 4080 x 3072, 3 channel, half openexr
+    SHA-1: 34A9F9879CD89E718ABCEE718A779035F6F78666
+    channel list: R, G, B
+    compression: "zip"
+    DateTime: "2013:04:16 10:20:35"
+    Orientation: 1 (normal)
+    PixelAspectRatio: 1
+    ResolutionUnit: "in"
+    screenWindowCenter: 0, 0
+    screenWindowWidth: 1
+    Software: "OpenImageIO 2.6.2.0dev : oiiotool ../common/tahoe-tiny.tif --resize 4080x3072 -o ref/decreasingY-resize.exr"
+    XResolution: 72
+    YResolution: 72
+    Exif:ImageHistory: "oiiotool ../common/tahoe-tiny.tif --resize 4080x3072 -o ref/decreasingY-resize.exr"
+    oiio:ColorSpace: "Linear"
+    oiio:subimages: 1
+Reading ref/decreasingY-copy.exr
+ref/decreasingY-copy.exr : 4080 x 3072, 3 channel, half openexr
+    SHA-1: 34A9F9879CD89E718ABCEE718A779035F6F78666
+    channel list: R, G, B
+    compression: "zip"
+    DateTime: "2013:04:16 10:20:35"
+    Orientation: 1 (normal)
+    PixelAspectRatio: 1
+    ResolutionUnit: "in"
+    screenWindowCenter: 0, 0
+    screenWindowWidth: 1
+    Software: "OpenImageIO 2.6.2.0dev : oiiotool ref/decreasingY-resize.exr -o ref/decreasingY-copy.exr"
+    XResolution: 72
+    YResolution: 72
+    Exif:ImageHistory: "oiiotool ../common/tahoe-tiny.tif --resize 4080x3072 -o ref/decreasingY-resize.exr
+oiiotool ref/decreasingY-resize.exr -o ref/decreasingY-copy.exr"
+    oiio:ColorSpace: "Linear"
+    oiio:subimages: 1
+Reading decreasingY-resize.exr
+decreasingY-resize.exr : 4080 x 3072, 3 channel, half openexr
+    SHA-1: 34A9F9879CD89E718ABCEE718A779035F6F78666
+    channel list: R, G, B
+    compression: "zip"
+    DateTime: "2013:04:16 10:20:35"
+    Orientation: 1 (normal)
+    PixelAspectRatio: 1
+    ResolutionUnit: "in"
+    screenWindowCenter: 0, 0
+    screenWindowWidth: 1
+    Software: "OpenImageIO 2.6.2.0dev : oiiotool ../common/tahoe-tiny.tif --resize 4080x3072 --attrib openexr:lineOrder decreasingY -o decreasingY-resize.exr"
+    XResolution: 72
+    YResolution: 72
+    Exif:ImageHistory: "oiiotool ../common/tahoe-tiny.tif --resize 4080x3072 --attrib openexr:lineOrder decreasingY -o decreasingY-resize.exr"
+    oiio:ColorSpace: "Linear"
+    oiio:subimages: 1
+Reading decreasingY-copy.exr
+decreasingY-copy.exr : 4080 x 3072, 3 channel, half openexr
+    SHA-1: 34A9F9879CD89E718ABCEE718A779035F6F78666
+    channel list: R, G, B
+    compression: "zip"
+    DateTime: "2013:04:16 10:20:35"
+    Orientation: 1 (normal)
+    PixelAspectRatio: 1
+    ResolutionUnit: "in"
+    screenWindowCenter: 0, 0
+    screenWindowWidth: 1
+    Software: "OpenImageIO 2.6.2.0dev : oiiotool ref/decreasingY-resize.exr --attrib openexr:lineOrder decreasingY -o decreasingY-copy.exr"
+    XResolution: 72
+    YResolution: 72
+    Exif:ImageHistory: "oiiotool ../common/tahoe-tiny.tif --resize 4080x3072 -o ref/decreasingY-resize.exr
+oiiotool ref/decreasingY-resize.exr --attrib openexr:lineOrder decreasingY -o decreasingY-copy.exr"
+    oiio:ColorSpace: "Linear"
+    oiio:subimages: 1
+Comparing "decreasingY-resize.exr" and "ref/decreasingY-resize.exr"
+PASS
+Comparing "decreasingY-copy.exr" and "ref/decreasingY-copy.exr"
+PASS
diff --git a/testsuite/openexr-decreasingy/run.py b/testsuite/openexr-decreasingy/run.py
@@ -0,0 +1,41 @@
+#!/usr/bin/env python
+
+# Copyright Contributors to the OpenImageIO project.
+# SPDX-License-Identifier: Apache-2.0
+# https://github.com/AcademySoftwareFoundation/OpenImageIO
+
+import os, sys
+
+####################################################################
+# Verify decreasingY line order generates same image as increasingY (default).
+####################################################################
+
+# Capture error output
+redirect = " >> out.txt 2>&1 "
+
+# Create reference images stored in increasingY order (default)
+# Resizing to a large image size to ensure scanline chunking logic is triggered.
+command += oiiotool("../common/tahoe-tiny.tif --resize 4080x3072 -o ref/decreasingY-resize.exr")
+command += info_command("ref/decreasingY-resize.exr")
+command += oiiotool("ref/decreasingY-resize.exr -o ref/decreasingY-copy.exr")
+command += info_command("ref/decreasingY-copy.exr")
+
+# Create an image in decreasing order via resizing (Tests ImageOutput::write_image() logic)
+command += oiiotool("../common/tahoe-tiny.tif --resize 4080x3072 --attrib openexr:lineOrder decreasingY -o decreasingY-resize.exr")
+command += info_command("decreasingY-resize.exr")
+
+# Create an image in decreasing order via copying a reference image. (Tests ImageBuf::write() logic)
+command += oiiotool("ref/decreasingY-resize.exr --attrib openexr:lineOrder decreasingY -o decreasingY-copy.exr")
+command += info_command("decreasingY-copy.exr")
+
+# Outputs to check against references.
+# This makes sure the images look the same since the line order is a storage detail and should not
+# change how the image actually looks. These comparisons help verify chunk order and scanlines are
+# processed properly.
+outputs = [
+    "decreasingY-resize.exr",
+    "decreasingY-copy.exr",
+]
+
+
+#print "Running this command:\n" + command + "\n"