Fix out-of-bounds reads when using OpenEXR decreasingY lineOrder.

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. Signed-off-by: Aaron Colwell <[email protected]>
AcademySoftwareFoundation · Apr 3, 2024 · 01d1b9f · 01d1b9f
1 parent fc5a454
commit 01d1b9f
Show file tree

Hide file tree

Showing 6 changed files with 174 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,20 @@ 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]);
+
+            const bool isDecreasingY = !strcmp(out->format_name(), "openexr")
+                                       && outspec.get_string_attribute(
+                                              "openexr:lineOrder")
+                                              == "decreasingY";
+            const int yStart = isDecreasingY
+                                   ? ((outspec.height - 1) / chunk) * chunk
+                                   : 0;
+            const int yDelta = isDecreasingY ? -chunk : chunk;
             for (int z = 0; z < outspec.depth; ++z) {
-                for (int y = 0; y < outspec.height && ok; y += chunk) {
+                for (int y = yStart; ((isDecreasingY && y >= 0)
+                                      || (!isDecreasingY && y < outspec.height))
+                                     && 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 +1442,11 @@ 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 - y) : y))
+                                / (outspec.height * outspec.depth)))
                         return ok;
                 }
             }

diff --git a/src/libOpenImageIO/imageoutput.cpp b/src/libOpenImageIO/imageoutput.cpp
@@ -518,17 +518,31 @@ 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);
+
+        const bool isDecreasingY = !strcmp(format_name(), "openexr")
+                                   && m_spec.get_string_attribute(
+                                          "openexr:lineOrder")
+                                          == "decreasingY";
+        const int yStart = isDecreasingY ? ((m_spec.height - 1) / chunk) * chunk
+                                         : 0;
+        const int yDelta = isDecreasingY ? -chunk : chunk;
+
         for (int z = 0; z < m_spec.depth; ++z)
-            for (int y = 0; y < m_spec.height && ok; y += chunk) {
+            for (int y = yStart; ((isDecreasingY && y >= 0)
+                                  || (!isDecreasingY && y < m_spec.height))
+                                 && 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 - 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,34 @@ 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";
+
+    int yChunkStart = isDecreasingY ? ybegin + ((yend - ybegin) / chunk) * chunk
+                                    : ybegin;
+    const int yDelta = isDecreasingY ? -chunk : chunk;
+    for (; ok
+           && ((isDecreasingY && yChunkStart >= ybegin)
+               || (!isDecreasingY && yChunkStart < yend));
+         yChunkStart += yDelta) {
+        int y1         = std::min(yChunkStart + chunk, yend);
+        int nscanlines = y1 - yChunkStart;
+
+        const void* dataStart = (const char*)data
+                                + (yChunkStart - ybegin) * ystride;
+        const void* d = to_native_rectangle(m_spec.x, m_spec.x + m_spec.width,
+                                            yChunkStart, 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
+                    - yChunkStart * scanlinebytes;
         try {
             Imf::FrameBuffer frameBuffer;
             size_t chanoffset = 0;
@@ -1527,8 +1540,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"