From 7f41a6a38308844f39cba941eb597e1cb47c6db0 Mon Sep 17 00:00:00 2001 From: Aaron Colwell <300262+acolwell@users.noreply.github.com> Date: Fri, 12 Apr 2024 12:53:48 -0700 Subject: [PATCH] fix(openexr): Fix out-of-bounds reads when using OpenEXR decreasingY lineOrder. (#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 <300262+acolwell@users.noreply.github.com> --- src/cmake/testing.cmake | 2 +- src/libOpenImageIO/imagebuf.cpp | 27 +++++++-- src/libOpenImageIO/imageoutput.cpp | 25 ++++++-- src/openexr.imageio/exroutput.cpp | 32 ++++++---- testsuite/openexr-decreasingy/ref/out.txt | 74 +++++++++++++++++++++++ testsuite/openexr-decreasingy/run.py | 41 +++++++++++++ 6 files changed, 180 insertions(+), 21 deletions(-) create mode 100644 testsuite/openexr-decreasingy/ref/out.txt create mode 100644 testsuite/openexr-decreasingy/run.py diff --git a/src/cmake/testing.cmake b/src/cmake/testing.cmake index 2cd32dc58f..cad2882cf1 100644 --- 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 index f211fc9021..583d8e2c4b 100644 --- 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 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 index 2724696223..34eb80ba32 100644 --- 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 index ea419d47c1..68c0f16ad0 100644 --- 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 new file mode 100644 index 0000000000..2eabcaf460 --- /dev/null +++ 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 new file mode 100644 index 0000000000..cdc2480d36 --- /dev/null +++ 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" \ No newline at end of file