From dd7b1b9bf167c0d2dd9425e52f585d7aa8520ac1 Mon Sep 17 00:00:00 2001 From: Larry Gritz Date: Thu, 29 Oct 2026 08:09:35 -0700 Subject: [PATCH] fix(jpeg2000): don't overread sYCC conversion with < 3 channels The reader unconditionally applied the yuv_to_rgb conversion to all OPJ_CLRSPC_SYCC tagged images. The conversion assumes three samples per pixel, so a 1-channel image tagged sYCC read and wrote two samples past the end of every scanline buffer. Solution: Only do the yuv_to_rgb conversion if channels >= 3. A lone channel is decoded as plain luma. This matches the behavior of OpenJPEG's own reference decoder. It's a one-line code fix, and all the rest of the changes in this patch are tests and documenting what went wrong. Assisted-by: Claude Code / claude-opus-5-5 Signed-off-by: Larry Gritz --- .../references/bug-catalog.md | 6 ++++++ .../references/format-notes.md | 4 +++- docs/dev/format-hardening-deferrals.md | 1 + src/fuzz/populate_corpora.py | 1 + src/jpeg2000.imageio/jpeg2000input.cpp | 2 +- testsuite/jpeg2000/ref/out-icc.txt | 11 +++++++++++ testsuite/jpeg2000/ref/out.txt | 11 +++++++++++ testsuite/jpeg2000/run.py | 3 +++ testsuite/jpeg2000/src/sycc-1chan.jp2 | Bin 0 -> 226 bytes 9 files changed, 37 insertions(+), 2 deletions(-) create mode 100644 testsuite/jpeg2000/src/sycc-1chan.jp2 diff --git a/.agents/skills/oiio-input-security-audit/references/bug-catalog.md b/.agents/skills/oiio-input-security-audit/references/bug-catalog.md index d0c334dea0..e38270d5f9 100644 --- a/.agents/skills/oiio-input-security-audit/references/bug-catalog.md +++ b/.agents/skills/oiio-input-security-audit/references/bug-catalog.md @@ -188,6 +188,12 @@ Streams and rotated images can change geometry after the header. values into 3-channel buffer). Fix: compare decoder-reported dims, channel count, and stride against the spec before every copy-out; size scratch buffers from the decoder-reported values, not the header. +- Colorspace tag selecting a fixed-width conversion: a file-supplied tag + picks a converter that touches N samples per pixel with no check that + nchannels >= N. Find: per-pixel `p[i+1]`/`p[i+2]` in `*_to_rgb` helpers; + trace what gates the call. Seen: jpeg2000 sYCC (`colr` EnumCS 18) on a + 1-channel image, 2 samples OOB read+write past every scanline. + Fix: gate on channel count too (`nc >= 3`). - Uninitialized packet structs: `av_packet_alloc()`, not stack AVPacket, because `av_read_frame` does not init on failure. diff --git a/.agents/skills/oiio-input-security-audit/references/format-notes.md b/.agents/skills/oiio-input-security-audit/references/format-notes.md index c9240911e9..574e5279fd 100644 --- a/.agents/skills/oiio-input-security-audit/references/format-notes.md +++ b/.agents/skills/oiio-input-security-audit/references/format-notes.md @@ -79,7 +79,9 @@ OIIO validates geometry, checks returns, bounds its own buffers. were missing from `check_open`; library bounds its own framebuffer (C6 left). - jpeg2000: two codepaths (OpenJPEG, OpenJPH/HTJ2K) - guard both before decode; HTJ2K error handling continued after failure; zero components read - past an array; OpenJPH global error handler needs static lifetime. + past an array; OpenJPH global error handler needs static lifetime; + sYCC conversion ran on 1-channel images (OpenJPEG sets SYCC from the + `colr` box alone). Open: >4 components out of step; truncated codestream decodes silently (no completeness signal from OpenJPH). - webp: check_open + C2 moved before the decoded-image allocation; no real diff --git a/docs/dev/format-hardening-deferrals.md b/docs/dev/format-hardening-deferrals.md index f31f1eddcc..5f02bba333 100644 --- a/docs/dev/format-hardening-deferrals.md +++ b/docs/dev/format-hardening-deferrals.md @@ -41,6 +41,7 @@ Newest first. One row per deferred finding. | Date | Format | Location (file:line / function) | Kind | Description | Why deferred | Severity | |------|--------|---------------------------------|------|-------------|--------------|----------| +| 2026-10-29 | jpeg2000 | src/jpeg2000.imageio/jpeg2000output.cpp / close() | writer | `close()` calls `save_image()` and ignores its return, so an OpenJPEG encode failure is reported as success. Seen with a 16x8 image (default resolution count too high for that size): `oiiotool` exits 0 and leaves a 77-byte file with no codestream. | Writer-side and not a read hazard; the reader rejects the resulting file cleanly ("Expected a SOC marker"). Fix is to propagate the return, plus possibly clamp `numresolution` to what the image size supports. | P2 | | 2026-07-26 | xmp | pugixml (`xml_document::load_buffer`, reached from src/libOpenImageIO/xmp.cpp / decode_xmp()) | other | The packet is materialized twice before any cap applies: the caller copies it into a `std::string` and pugixml copies it again into its own buffer plus a node pool. Measured 652 MB peak RSS for a 75 MB packet of tiny elements, i.e. ~8.7x, all of it before `decode_xmp_node()` runs. Linear in the input and bounded by whatever the container hands over, so resource use, not a safety defect. JPEG caps an APP1 marker at 64 KB, but PNG `zTXt`/`iTXt`, TIFF `XMLPACKET`, PSD resources and HEIF are all effectively unbounded. | Capping the packet size in the shared decoder is the same policy question the exif row above raises -- any ceiling risks dropping legitimate XMP, which has no specified upper bound, and a `limits:*`-style metadata ceiling needs maintainer sign-off. The container is the better place to bound the blob, which is a per-format decision. | P2 | | 2026-07-26 | xmp | src/libOpenImageIO/xmp.cpp / decode_xmp() (callers: src/psd.imageio/psdinput.cpp:1405) | reader-nonblocking | `decode_xmp()` returns `true` unconditionally on every path, so the failure posture (C8) is "best effort, never report" and there is nothing for `imageinput:strict` to gate. PSD's `if (!decode_xmp(...)) { errorfmt(...); return false; }` is therefore dead code, and the new budget/depth caps silently truncate rather than reporting. | Deliberately unchanged in this phase. Giving the function a real `false` return means deciding what counts as a failure for a decoder whose whole design is to tolerate the minor malformations common in real files (see the comment in `decode_xmp`), and PSD would then hard-error on files it reads today. That is a posture change for five containers at once, which wants a maintainer's call. | P2 | | 2026-07-26 | xmp | src/libOpenImageIO/xmp.cpp / add_attrib() | reader-nonblocking | `oiiotype == TypeRational \|\| string_is_rational(xmlname)` tests the XML attribute *name* for an "int/int" shape, not the value -- almost certainly meant to be `xmlvalue`. As written the branch is nearly unreachable (no table entry is `TypeRational`, and an attribute literally named `1/2` is not well-formed XML), so rational-valued XMP attributes fall through to the catch-all and are stored as strings. The same branch then stores under `xmlname` rather than the mapped `oiioname`. | Not a hazard: `parse_rational` fills the array only on success and the attribute is written only inside that branch, so nothing uninitialized escapes. Fixing the typo changes which attributes valid files publish and under what name and type, which is a metadata-compatibility change, not a hardening one . | P2 | diff --git a/src/fuzz/populate_corpora.py b/src/fuzz/populate_corpora.py index 57c9db90ea..02f51ac5c3 100644 --- a/src/fuzz/populate_corpora.py +++ b/src/fuzz/populate_corpora.py @@ -131,6 +131,7 @@ def _find_images_root() -> Path: ], "jpeg2000": [ ("testsuite/htj2k/src", ["j2c"]), + ("testsuite/jpeg2000/src", ["jp2"]), ("../oiio-images/jpeg2000", ["jp2", "j2k"]), ("../j2kp4files_v1_5/codestreams_profile0", ["j2k"]), ], diff --git a/src/jpeg2000.imageio/jpeg2000input.cpp b/src/jpeg2000.imageio/jpeg2000input.cpp index 197f513bd3..1f11494604 100644 --- a/src/jpeg2000.imageio/jpeg2000input.cpp +++ b/src/jpeg2000.imageio/jpeg2000input.cpp @@ -1004,7 +1004,7 @@ Jpeg2000Input::copy_scanline(int y, int /*z*/, void* data) } } } - if (m_image->color_space == OPJ_CLRSPC_SYCC) + if (m_image->color_space == OPJ_CLRSPC_SYCC && nc >= 3) yuv_to_rgb(scanline); } diff --git a/testsuite/jpeg2000/ref/out-icc.txt b/testsuite/jpeg2000/ref/out-icc.txt index a65442b281..c1e398912f 100644 --- a/testsuite/jpeg2000/ref/out-icc.txt +++ b/testsuite/jpeg2000/ref/out-icc.txt @@ -6,6 +6,17 @@ idiff ERROR: Could not read ../oiio-images/jpeg2000/broken/issue_3427.jp2: Failed to decode the codestream in the JP2 file Invalid image file "../oiio-images/jpeg2000/broken/issue_3427.jp2": Tile part length size inconsistent with stream length Failed to decode the codestream in the JP2 file + 32 x 32, 1 channel, float jpeg2000 + Stats Min: 128 (of 255) + Stats Max: 128 (of 255) + Stats Avg: 128.00 (of 255) + Stats StdDev: 0.00 (of 255) + Stats NanCount: 0 + Stats InfCount: 0 + Stats FiniteCount: 1024 + Constant: Yes + Constant Color: 128.00 (of 255) + Monochrome: Yes ICCProfile of tahoe-icc.jp2 ICCProfile: 0, 0, 2, 48, 65, 68, 66, 69, 2, 16, 0, 0, 109, 110, 116, 114, ... [560 x uint8] ICCProfile:attributes: "Reflective, Glossy, Positive, Color" diff --git a/testsuite/jpeg2000/ref/out.txt b/testsuite/jpeg2000/ref/out.txt index 49df15bdff..eef1bc1c21 100644 --- a/testsuite/jpeg2000/ref/out.txt +++ b/testsuite/jpeg2000/ref/out.txt @@ -6,3 +6,14 @@ idiff ERROR: Could not read ../oiio-images/jpeg2000/broken/issue_3427.jp2: Failed to decode the codestream in the JP2 file Invalid image file "../oiio-images/jpeg2000/broken/issue_3427.jp2": Tile part length size inconsistent with stream length Failed to decode the codestream in the JP2 file + 32 x 32, 1 channel, float jpeg2000 + Stats Min: 128 (of 255) + Stats Max: 128 (of 255) + Stats Avg: 128.00 (of 255) + Stats StdDev: 0.00 (of 255) + Stats NanCount: 0 + Stats InfCount: 0 + Stats FiniteCount: 1024 + Constant: Yes + Constant Color: 128.00 (of 255) + Monochrome: Yes diff --git a/testsuite/jpeg2000/run.py b/testsuite/jpeg2000/run.py index d91767fb8b..84bc8c6545 100755 --- a/testsuite/jpeg2000/run.py +++ b/testsuite/jpeg2000/run.py @@ -16,6 +16,9 @@ for f in files: command += rw_command (imagedir, f, printinfo=False) +# sYCC colr box on a 1-channel image must not run the 3-channel conversion +command += oiiotool("src/sycc-1chan.jp2 --printstats") + # Try to parse library_list to find the OpenJPEG version. If found, return it as # a single number, e.g. 2.5.3 -> 20503. Return -1 if not found. def get_openjp_ver(): diff --git a/testsuite/jpeg2000/src/sycc-1chan.jp2 b/testsuite/jpeg2000/src/sycc-1chan.jp2 new file mode 100644 index 0000000000000000000000000000000000000000..a04fa7c0368790b2f29ac4e9046a7986bc60d751 GIT binary patch literal 226 zcmZQzVBpCLP*C9IYUg5LU=T?wsVvAUFj4@r8KAT-kj?;d#WFKeih#5N7&Ef718D{Z z{^b0eB1SM%2uMx^Do+0I|38pH6D+0xBtaMk6o3p6h5$x(M#ld^3_L&{BLgc73j^c- z7zSYn2M><`Wb{9UL6w2gxhOTUBsE1LsZzneAT`e`z|~!$EVZaOGe1wkNY7Ny