openexr icon indicating copy to clipboard operation
openexr copied to clipboard

OpenEXR 3.3.2 crashes when reading a subset of channels with half to float conversion

Open adambowen-foundry opened this issue 9 months ago • 4 comments

OpenEXR 3.3.2 crashes when attempting to read a single channel from a half encoded RGB EXR file. The crash can be reproduced with the following snippet with the sample BrightRings.exr image.

#include "OpenEXR/ImfInputFile.h"
#include "OpenEXR/ImfArray.h"
#include "OpenEXR/ImfHeader.h"
#include "OpenEXR/ImfFrameBuffer.h"

int main(int argc, char *argv[]) {
    const char *exrPath = "BrightRings.exr";
    Imf::Array2D<float>  rPixels;

    // derived from https://openexr.com/en/latest/ReadingAndWritingImageFiles.html#reading-an-image-file
    Imf::InputFile file(exrPath);
    Imath::Box2i dw = file.header().dataWindow();
    const int width = dw.max.x - dw.min.x + 1;
    const int height = dw.max.y - dw.min.y + 1;

    rPixels.resizeErase(height, width);

    Imf::FrameBuffer frameBuffer;

    // Note that the crash occurs when we only request _some_ channels AND we request conversion from half to float
    frameBuffer.insert("R",                // name
        Imf::Slice(Imf::FLOAT,             // type
            (char*)(&rPixels[0][0] -       // base
                dw.min.x -
                dw.min.y * width),
            sizeof(rPixels[0][0]) * 1,     // xStride
            sizeof(rPixels[0][0]) * width, // yStride
            1, 1,                          // x/y sampling
            0.0));                         // fillValue

    file.setFrameBuffer(frameBuffer);

    const int minScanLine = dw.min.y;
    const int maxScanLine = dw.max.y;
    file.readPixels(minScanLine, maxScanLine);

    return 0;
}

Steps to reproduce

  1. Download the BrightRings.exr file
  2. Compile the above snippet, linking to OpenEXR 3.3.2
  3. Execute the program (with BrightRings.exr in the current working directory)

Expected behaviour

The red channel is decoded from the image into the float buffer.

Actual behaviour

OpenEXR crashes attempting to convert from half to float.

Conditions

  • We have observed this problem in OpenEXR 3.3.2 on Windows, Linux, Mac Intel, and Mac Arm.
  • The same snippet does not crash in OpenEXR 3.2.1 on any platform.

Notes

I believe the problem occurs when internal_exr_match_decode selects the optimised decoding routine. Although it considers the number of channels in the input file, it does not consider the number of channels that were requested. Hence, it uses the optimised RGB path to decode all three channels when only one output buffer is available, causing it to try to write to invalid memory.

This appears to be a problem for every optimised path in that routine, since chanstounpack is never consulted. Ensuring chanstounpack == decode->channel_count before selecting the optimised decode routine fixes the crash, but I am not sure if this is the correct condition because I can envisage edge cases existing depending on how channel_count and chanstounpack are calculated.

adambowen-foundry avatar Feb 28 '25 08:02 adambowen-foundry

I reported the same issue to openexr-dev on 27 Mar.

I'm using OpenEXRCore, and I'm getting a crash if I attempt decode one channel of a 4-channel half file to a float output.

Issue is that internal_exr_match_decode() notes we're doing half->float, and that sameoutinc == 4 meaning the output is float, and that decode->channel_count == 4 meaning the file data is 4-channel, so it decides to use unpack_half_to_float_4chan_planar.

But then unpack_half_to_float_4chan_planar() blindly writes to decode->channels[0..3].decode_to_ptr, even though some of those pointers can legitimately be NULL.

A fix is for internal_exr_match_decode() to check the decode_to_ptr values and only use the optimised half<->float routines if we're reading all 3/4 channels, and otherwise to fallback to generic_unpack.

barretpj avatar Mar 03 '25 11:03 barretpj

I believe this was fixed in 8bc3faebc66b92805f7309fa7a2f46a66e5cc5c9.

fnordware avatar Mar 08 '25 07:03 fnordware

yes indeed, that unpack choosing routine needs a clean up to avoid such logic whoopsies as we add bespoke optimizations, but yes, I fixed this in looking at issue #1949 in January with PR #1950 - we are going to release a patch release shortly with this and other fixes

kdt3rd avatar Mar 10 '25 08:03 kdt3rd

Thank you, I cherry picked 8bc3fae onto our clone of v3.3.2 and can confirm the crash reported above is fixed.

adambowen-foundry avatar Mar 10 '25 10:03 adambowen-foundry