From 01d1b9f965e1031a7bdaaaf1993d53af74c01fcc Mon Sep 17 00:00:00 2001 From: Aaron Colwell <300262+acolwell@users.noreply.github.com> Date: Tue, 2 Apr 2024 15:27:51 -0700 Subject: [PATCH 1/4] 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 <300262+acolwell@users.noreply.github.com> --- src/cmake/testing.cmake | 2 +- src/libOpenImageIO/imagebuf.cpp | 23 +++++-- src/libOpenImageIO/imageoutput.cpp | 22 +++++-- src/openexr.imageio/exroutput.cpp | 33 ++++++---- testsuite/openexr-decreasingy/ref/out.txt | 74 +++++++++++++++++++++++ testsuite/openexr-decreasingy/run.py | 41 +++++++++++++ 6 files changed, 174 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..b2530b7d04 100644 --- 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 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 index 2724696223..53cba473df 100644 --- 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 index ea419d47c1..282f669196 100644 --- 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 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 From 3a07454f84408084286e9851ff85b04ef2f1c6bb Mon Sep 17 00:00:00 2001 From: Aaron Colwell <300262+acolwell@users.noreply.github.com> Date: Tue, 2 Apr 2024 20:49:06 -0700 Subject: [PATCH 2/4] Fix clang-format error. Signed-off-by: Aaron Colwell <300262+acolwell@users.noreply.github.com> --- src/openexr.imageio/exroutput.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/openexr.imageio/exroutput.cpp b/src/openexr.imageio/exroutput.cpp index 282f669196..3e8fdde477 100644 --- a/src/openexr.imageio/exroutput.cpp +++ b/src/openexr.imageio/exroutput.cpp @@ -1485,7 +1485,7 @@ 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; + bool ok = true; const bool isDecreasingY = m_spec.get_string_attribute("openexr:lineOrder") == "decreasingY"; From f43c19a669ce5ec4319cdccb7f6ec962e2e4cf71 Mon Sep 17 00:00:00 2001 From: Aaron Colwell <300262+acolwell@users.noreply.github.com> Date: Thu, 11 Apr 2024 13:43:22 -0700 Subject: [PATCH 3/4] Address CR comments. Signed-off-by: Aaron Colwell <300262+acolwell@users.noreply.github.com> --- src/libOpenImageIO/imagebuf.cpp | 19 +++++++++------- src/libOpenImageIO/imageoutput.cpp | 17 +++++++++------ src/openexr.imageio/exroutput.cpp | 35 +++++++++++++++--------------- 3 files changed, 38 insertions(+), 33 deletions(-) diff --git a/src/libOpenImageIO/imagebuf.cpp b/src/libOpenImageIO/imagebuf.cpp index b2530b7d04..f44091267b 100644 --- a/src/libOpenImageIO/imagebuf.cpp +++ b/src/libOpenImageIO/imagebuf.cpp @@ -1419,19 +1419,22 @@ ImageBuf::write(ImageOutput* out, ProgressCallback progress_callback, 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 yStart = isDecreasingY - ? ((outspec.height - 1) / chunk) * chunk - : 0; - const int yDelta = isDecreasingY ? -chunk : chunk; + 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 = yStart; ((isDecreasingY && y >= 0) - || (!isDecreasingY && y < outspec.height)) - && ok; - y += yDelta) { + 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, diff --git a/src/libOpenImageIO/imageoutput.cpp b/src/libOpenImageIO/imageoutput.cpp index 53cba473df..fb761a3033 100644 --- a/src/libOpenImageIO/imageoutput.cpp +++ b/src/libOpenImageIO/imageoutput.cpp @@ -519,19 +519,22 @@ ImageOutput::write_image(TypeDesc format, const void* data, stride_t xstride, 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 yStart = isDecreasingY ? ((m_spec.height - 1) / chunk) * chunk - : 0; - const int yDelta = isDecreasingY ? -chunk : chunk; + 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 = yStart; ((isDecreasingY && y >= 0) - || (!isDecreasingY && y < m_spec.height)) - && ok; - y += yDelta) { + 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; diff --git a/src/openexr.imageio/exroutput.cpp b/src/openexr.imageio/exroutput.cpp index 3e8fdde477..68c0f16ad0 100644 --- a/src/openexr.imageio/exroutput.cpp +++ b/src/openexr.imageio/exroutput.cpp @@ -1488,31 +1488,30 @@ OpenEXROutput::write_scanlines(int ybegin, int yend, int z, TypeDesc format, 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 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, - yChunkStart, y1, z, z + 1, format, - dataStart, xstride, ystride, - zstride, m_scratch); + 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 - - yChunkStart * scanlinebytes; + char* buf = (char*)d - m_spec.x * pixel_bytes - y * scanlinebytes; try { Imf::FrameBuffer frameBuffer; size_t chanoffset = 0; From 40a14bb12749478117ab582181c78f1fe54759a5 Mon Sep 17 00:00:00 2001 From: Aaron Colwell <300262+acolwell@users.noreply.github.com> Date: Thu, 11 Apr 2024 14:12:29 -0700 Subject: [PATCH 4/4] Fix progress count off by one bug. Signed-off-by: Aaron Colwell <300262+acolwell@users.noreply.github.com> --- src/libOpenImageIO/imagebuf.cpp | 3 ++- src/libOpenImageIO/imageoutput.cpp | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/libOpenImageIO/imagebuf.cpp b/src/libOpenImageIO/imagebuf.cpp index f44091267b..583d8e2c4b 100644 --- a/src/libOpenImageIO/imagebuf.cpp +++ b/src/libOpenImageIO/imagebuf.cpp @@ -1448,7 +1448,8 @@ ImageBuf::write(ImageOutput* out, ProgressCallback progress_callback, && progress_callback( progress_callback_data, (float)(z * outspec.height - + (isDecreasingY ? (outspec.height - y) : y)) + + (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 fb761a3033..34eb80ba32 100644 --- a/src/libOpenImageIO/imageoutput.cpp +++ b/src/libOpenImageIO/imageoutput.cpp @@ -544,7 +544,7 @@ ImageOutput::write_image(TypeDesc format, const void* data, stride_t xstride, && progress_callback( progress_callback_data, (float)(z * m_spec.height - + (isDecreasingY ? (m_spec.height - y) : y)) + + (isDecreasingY ? (m_spec.height - 1 - y) : y)) / (m_spec.height * m_spec.depth))) return ok; }