From 90a0af52a0823ee904e8b9c93ff5da62c3816c5f Mon Sep 17 00:00:00 2001 From: Larry Gritz Date: Fri, 3 Jul 2026 18:32:23 -0400 Subject: [PATCH] fix(softimage): support channel packets with differing bit depths A Softimage PIC file stores a chain of channel packets, each carrying its own bit depth (8 or 16). The reader assumed one uniform per-channel size for the whole image, so a file mixing depths (e.g. 16-bit R with 8-bit G) wrote past the end of the scanline buffer -- a heap overflow reachable via iconvert poc.pic -o out.exr. Fix: always expose one uniform pixel format, the widest depth among the file's packets, and promote narrower channels to it on read via exact bit replication (v*257 for 8->16), which preserves the normalized value exactly. This keeps the pixel layout naturally aligned; representing channels with their true differing formats instead would pack them at non-uniform, sometimes misaligned byte offsets. Assisted-by: Claude Code / claude-sonnet-5 Signed-off-by: Larry Gritz --- src/softimage.imageio/softimageinput.cpp | 204 ++++++++++++--------- testsuite/softimage/ref/out.txt | 15 ++ testsuite/softimage/run.py | 6 + testsuite/softimage/src/mixed-bitdepth.pic | Bin 0 -> 118 bytes 4 files changed, 141 insertions(+), 84 deletions(-) create mode 100644 testsuite/softimage/src/mixed-bitdepth.pic diff --git a/src/softimage.imageio/softimageinput.cpp b/src/softimage.imageio/softimageinput.cpp index c316523d1c..69c9956b0e 100644 --- a/src/softimage.imageio/softimageinput.cpp +++ b/src/softimage.imageio/softimageinput.cpp @@ -2,6 +2,8 @@ // SPDX-License-Identifier: Apache-2.0 // https://github.com/AcademySoftwareFoundation/OpenImageIO +#include + #include "softimage_pvt.h" OIIO_PLUGIN_NAMESPACE_BEGIN @@ -10,6 +12,40 @@ using namespace softimage_pvt; +namespace { + +// Decode n (1 or 2) on-disk big-endian bytes into a native integer. +inline uint16_t +assemble_be(const uint8_t* bytes, size_t n) +{ + return n == 2 ? uint16_t((uint16_t(bytes[0]) << 8) | bytes[1]) : bytes[0]; +} + + +// Widen 8-bit to 16-bit via exact bit replication (v*257, since +// 255*257==65535); the only possible promotion, since packet depths are +// only ever 8 or 16 bits. +inline uint16_t +promote_depth(uint16_t value, size_t packetBytes, size_t storageBytes) +{ + return packetBytes == storageBytes ? value : uint16_t(value * 257); +} + + +// Store storageBytes of value at dst in native machine byte order. +inline void +store_native(uint8_t* dst, uint16_t value, size_t storageBytes) +{ + if (storageBytes == 1) + dst[0] = uint8_t(value); + else + memcpy(dst, &value, sizeof(uint16_t)); +} + +} // namespace + + + class SoftimageInput final : public ImageInput { public: SoftimageInput() { init(); } @@ -51,8 +87,16 @@ class SoftimageInput final : public ImageInput { std::string m_filename; std::vector m_scanline_markers; // Maps absolute channel index (0=R,1=G,2=B,3=A) to sequential output - // offset within the scanline buffer. Initialized to -1 (unused). + // channel number in the ImageSpec. Initialized to -1 (unused). int m_channel_map[4]; + // Byte offset of each absolute channel within a pixel. Valid only + // where m_channel_map is >= 0. + size_t m_channel_byte_offset[4]; + // Uniform storage size (bytes) for all channels: the widest packet + // depth in the file. Narrower channels are promoted to it on read. + size_t m_storage_bytes; + // Native bytes per pixel (nchannels * m_storage_bytes). + size_t m_pixel_bytes; }; @@ -80,6 +124,9 @@ SoftimageInput::init() m_channel_packets.clear(); m_scanline_markers.clear(); std::fill(m_channel_map, m_channel_map + 4, -1); + std::fill(m_channel_byte_offset, m_channel_byte_offset + 4, size_t(0)); + m_storage_bytes = 0; + m_pixel_bytes = 0; } @@ -145,30 +192,39 @@ SoftimageInput::open(const std::string& name, ImageSpec& spec) } } while (curPacket.chained); - // Build channel map: absolute RGBA index -> sequential output offset - int nchannels = 0; - { - for (auto& cp : m_channel_packets) - for (int ch : cp.channels()) - if (m_channel_map[ch] == -1) - m_channel_map[ch] = nchannels++; + // Build channel map: absolute RGBA index -> sequential output channel. + // Packets may have different bit depths; rather than expose that as + // per-channel formats (a legacy format isn't worth the resulting + // non-uniform, potentially misaligned pixel layout), we always store + // a single uniform format -- the widest depth present -- and promote + // narrower channels (see promote_depth()) as they're read. + int nchannels = 0; + TypeDesc widest = TypeDesc::UINT8; + for (auto& cp : m_channel_packets) { + if (cp.size == 16) + widest = TypeDesc::UINT16; + for (int ch : cp.channels()) + if (m_channel_map[ch] == -1) + m_channel_map[ch] = nchannels++; } - // Get the depth per pixel per channel - TypeDesc chanType = TypeDesc::UINT8; - if (curPacket.size == 16) - chanType = TypeDesc::UINT16; - // Set the details in the ImageSpec m_spec = ImageSpec(m_pic_header.width, m_pic_header.height, nchannels, - chanType); + widest); if (!check_open(m_spec, { 0, 65535, 0, 65535, 0, 1, 0, 4 })) { close(); return false; } - m_spec.attribute("BitsPerSample", (int)curPacket.size); + // Precompute the uniform, naturally-aligned byte layout. + m_storage_bytes = widest.size(); + m_pixel_bytes = nchannels * m_storage_bytes; + for (int ch = 0; ch < 4; ++ch) + if (m_channel_map[ch] >= 0) + m_channel_byte_offset[ch] = m_channel_map[ch] * m_storage_bytes; + + m_spec.attribute("BitsPerSample", (int)(widest.size() * 8)); m_spec.attribute("softimage:compression", Strutil::join(encodings, ",")); @@ -326,22 +382,15 @@ SoftimageInput::read_pixels_uncompressed( uint8_t* scanlineData = (uint8_t*)data; for (size_t pixelX = 0; pixelX < m_pic_header.width; pixelX++) { for (int channel : channels) { - for (size_t byte = 0; byte < pixelChannelSize; byte++) { - // Get which byte we should be placing this in depending on endianness - size_t curByte = byte; - if (littleendian()) - curByte = ((pixelChannelSize)-1) - curByte; - - //read the data into the correct place - if (fread(&scanlineData[(pixelX * pixelChannelSize - * m_spec.nchannels) - + (m_channel_map[channel] - * pixelChannelSize) - + curByte], - 1, 1, m_fd) - != 1) - return false; - } + uint8_t raw[2] = { 0, 0 }; + if (fread(raw, 1, pixelChannelSize, m_fd) != pixelChannelSize) + return false; + uint16_t value + = promote_depth(assemble_be(raw, pixelChannelSize), + pixelChannelSize, m_storage_bytes); + store_native(&scanlineData[(pixelX * m_pixel_bytes) + + m_channel_byte_offset[channel]], + value, m_storage_bytes); } } } else { @@ -392,26 +441,23 @@ SoftimageInput::read_pixels_pure_run_length( if (fread(pixelData, 1, pixelSize, m_fd) != pixelSize) return false; - // Now we've got the pixel value we need to push it into the data + // Decode and promote each channel's value once per run, then + // repeat it for each pixel in the run. + uint16_t chanValues[4]; + for (size_t curChan = 0; curChan < channels.size(); curChan++) + chanValues[curChan] = promote_depth( + assemble_be(pixelData + curChan * pixelChannelSize, + pixelChannelSize), + pixelChannelSize, m_storage_bytes); + uint8_t* scanlineData = (uint8_t*)data; for (size_t pixelX = linePixelCount; pixelX < linePixelCount + curCount; pixelX++) { - for (size_t curChan = 0; curChan < channels.size(); curChan++) { - for (size_t byte = 0; byte < pixelChannelSize; byte++) { - // Get which byte we should be placing this in depending on endianness - size_t curByte = byte; - if (littleendian()) - curByte = ((pixelChannelSize)-1) - curByte; - - //put the data into the correct place - scanlineData[(pixelX * pixelChannelSize - * m_spec.nchannels) - + (m_channel_map[channels[curChan]] - * pixelChannelSize) - + curByte] - = pixelData[(curChan * pixelChannelSize) + curByte]; - } - } + for (size_t curChan = 0; curChan < channels.size(); curChan++) + store_native( + &scanlineData[(pixelX * m_pixel_bytes) + + m_channel_byte_offset[channels[curChan]]], + chanValues[curChan], m_storage_bytes); } } else { // data pointer is null so we should just seek to the next scanline @@ -464,22 +510,17 @@ SoftimageInput::read_pixels_mixed_run_length( for (size_t pixelX = linePixelCount; pixelX < linePixelCount + curCount; pixelX++) { for (int channel : channels) { - for (size_t byte = 0; byte < pixelChannelSize; byte++) { - // Get which byte we should be placing this in depending on endianness - size_t curByte = byte; - if (littleendian()) - curByte = ((pixelChannelSize)-1) - curByte; - - //read the data into the correct place - if (fread(&scanlineData[(pixelX * pixelChannelSize - * m_spec.nchannels) - + (m_channel_map[channel] - * pixelChannelSize) - + curByte], - 1, 1, m_fd) - != 1) - return false; - } + uint8_t raw[2] = { 0, 0 }; + if (fread(raw, 1, pixelChannelSize, m_fd) + != pixelChannelSize) + return false; + uint16_t value + = promote_depth(assemble_be(raw, pixelChannelSize), + pixelChannelSize, m_storage_bytes); + store_native( + &scanlineData[(pixelX * m_pixel_bytes) + + m_channel_byte_offset[channel]], + value, m_storage_bytes); } } } else { @@ -526,30 +567,25 @@ SoftimageInput::read_pixels_mixed_run_length( if (fread(pixelData, 1, pixelSize, m_fd) != pixelSize) return false; - // Now we've got the pixel value we need to push it into - // the data. + // Decode and promote each channel's value once per run, + // then repeat it for each pixel in the run. + uint16_t chanValues[4]; + for (size_t curChan = 0; curChan < channels.size(); curChan++) + chanValues[curChan] = promote_depth( + assemble_be(pixelData + curChan * pixelChannelSize, + pixelChannelSize), + pixelChannelSize, m_storage_bytes); + uint8_t* scanlineData = (uint8_t*)data; for (size_t pixelX = linePixelCount; pixelX < linePixelCount + longCount; pixelX++) { for (size_t curChan = 0; curChan < channels.size(); - curChan++) { - for (size_t byte = 0; byte < pixelChannelSize; byte++) { - // Get which byte we should be placing this - // in depending on endianness. - size_t curByte = byte; - if (littleendian()) - curByte = ((pixelChannelSize)-1) - curByte; - - //put the data into the correct place - scanlineData[(pixelX * pixelChannelSize - * m_spec.nchannels) - + (m_channel_map[channels[curChan]] - * pixelChannelSize) - + curByte] - = pixelData[(curChan * pixelChannelSize) - + curByte]; - } - } + curChan++) + store_native( + &scanlineData + [(pixelX * m_pixel_bytes) + + m_channel_byte_offset[channels[curChan]]], + chanValues[curChan], m_storage_bytes); } } else { // data pointer is null so we should just seek to the diff --git a/testsuite/softimage/ref/out.txt b/testsuite/softimage/ref/out.txt index 3aa72d15ca..471e196acf 100644 --- a/testsuite/softimage/ref/out.txt +++ b/testsuite/softimage/ref/out.txt @@ -44,3 +44,18 @@ src/broken01.pic : 16 x 4, 1 channel, uint8 softimage Stats FiniteCount: 64 Constant: No Monochrome: Yes +src/mixed-bitdepth.pic : 2 x 1, 2 channel, uint16 softimage + SHA-1: 52D6B72AAD7084E2ECF1550D500A66C42F408ED1 + channel list: R, G + BitsPerSample: 16 + ImageDescription: "Mixed packet bit-depth overflow PoC" + softimage:compression: "none,none" + Stats Min: 4386 21845 (of 65535) + Stats Max: 13124 26214 (of 65535) + Stats Avg: 8755.00 24029.50 (of 65535) + Stats StdDev: 4369.00 2184.50 (of 65535) + Stats NanCount: 0 0 + Stats InfCount: 0 0 + Stats FiniteCount: 2 2 + Constant: No + Monochrome: No diff --git a/testsuite/softimage/run.py b/testsuite/softimage/run.py index bd71a74b87..8c80afe8d6 100755 --- a/testsuite/softimage/run.py +++ b/testsuite/softimage/run.py @@ -13,3 +13,9 @@ # Regression testing of error handling and corrupt files command += info_command ("--stats src/broken01.pic", info_program="iinfo", failureok=True) +# A file whose channel packets use different bit depths (here a 16-bit R +# packet and an 8-bit G packet): narrower channels get promoted to the +# widest depth present. This used to overrun the scanline buffer because a +# single bit depth was assumed for all channels. +command += info_command ("--stats src/mixed-bitdepth.pic", + info_program="iinfo", failureok=True) diff --git a/testsuite/softimage/src/mixed-bitdepth.pic b/testsuite/softimage/src/mixed-bitdepth.pic new file mode 100644 index 0000000000000000000000000000000000000000..d70c1c141bea558633dfa64fe0d77669d256ad9e GIT binary patch literal 118 zcmWG;_-10?z`)>}S&^EeP>`6Mom!%hlv$#il3Gxbp^#seT9lTPU#<|4@61372=H_c aVPIll1epy2i~*v&f=b3Np=kgsMibKj literal 0 HcmV?d00001