appendGainMap: EXIF offset derived from marker_list is wrong when unsaved markers precede EXIF — corrupts base JPEG, fails with "SOS marker not found" #474

Summary

JpegR::appendGainMap removes the EXIF segment from a compressed base image and
re-emits it at the front. It locates that segment using a byte offset computed by
jpeg_extract_marker_payload, which walks cinfo->marker_list and sums
original_length. That is only a valid file offset if every marker before the
EXIF is in the list.

JpegDecoderHelper::decode only requests APP0, APP1 and APP2
(jpegdecoderhelper.cpp:219-221), so any other marker before the EXIF — COM,
APP13/Photoshop, APP14/Adobe — contributes bytes to the file that are never
counted. The offset then points into an unrelated segment, copyJpegWithoutExif
(jpegr.cpp:164) splices out the wrong byte range, and the reorder loop added in
2.0 can no longer find SOS:

uhdr: SOS marker not found while reordering base jpeg segments, unable to append gainmap
(UHDR_CODEC_INVALID_PARAM)

Affects v2.0.2 and main (jpegr.cpp:1345 / main:1347). Reachable from the
public API whenever a compressed base carrying EXIF is used — i.e.
uhdr_enc_set_compressed_image(..., UHDR_BASE_IMG) +
uhdr_enc_set_gainmap_image(...), encode scenario 4.

Measurements

Offsets computed for a base JPEG under a stock libjpeg-turbo build — no
unusual configuration, no preloading:

The shortfall is exactly the size of the segments that were never saved.

Reproducer (no code required)

Take any JPEG with EXIF and insert one COM segment before it:

#!/usr/bin/env python3
"""Insert a COM segment immediately before the EXIF APP1 of a JPEG."""
import sys

src, dst = sys.argv[1], sys.argv[2]
d = open(src, "rb").read()
assert d[:2] == b"\xff\xd8", "not a JPEG"

i = 2
while i < len(d) - 3:
    assert d[i] == 0xFF, f"marker desync at {i}"
    m = d[i + 1]
    if m in (0xD8, 0xD9) or 0xD0 <= m <= 0xD7:
        i += 2
        continue
    if m == 0xDA:
        sys.exit("no EXIF APP1 in this file")
    L = (d[i + 2] << 8) | d[i + 3]
    if m == 0xE1 and d[i + 4 : i + 10] == b"Exif\x00\x00":
        payload = b"a marker libultrahdr does not save\x00"
        com = b"\xff\xfe" + (len(payload) + 2).to_bytes(2, "big") + payload
        open(dst, "wb").write(d[:i] + com + d[i:])
        print(f"inserted {len(com)}-byte COM at {i}; EXIF now starts at {i + len(com)}")
        break
    i += 2 + L

python3 inject_com.py base.jpg base-with-com.jpg
ultrahdr_app -m 0 -i base-with-com.jpg -g gainmap.jpg -f metadata.cfg

gainmap.jpg can be any small JPEG; metadata.cfg any valid set
(--maxContentBoost, --minContentBoost, --gamma, --offsetSdr,
--offsetHdr, --hdrCapacityMin, --hdrCapacityMax, --useBaseColorSpace).

Expected: an Ultra HDR JPEG whose EXIF has been moved to the front.
Actual: UHDR_CODEC_INVALID_PARAM, "SOS marker not found while reordering
base jpeg segments".

On one measured file the true EXIF payload offset was 45, the walk returned
6 (short by the 39-byte COM), and after copyJpegWithoutExif the byte where
the next marker should start was 0x00 — the scan desyncs immediately and
sos_offset stays 0.

A second way to trigger the same arithmetic

The walk also assumes marker_list is in stream order. That is specified —
libjpeg.txt, on jpeg_save_markers:

All the special markers in the file appear in this list, in order of their
occurrence in the file (but omitting any markers of types you didn't ask for).

but it is not true of every libjpeg API implementation. jpegli
ships as a libjpeg.so.62 drop-in and prepends each saved marker
(lib/jpegli/decode_marker.cc, "Insert new saved marker to the head of the
list") where libjpeg-turbo appends (jdmarker.c, "Add new marker to end of
list"). Built or preloaded against jpegli, the offset is wrong for every
base JPEG carrying EXIF, with the same outcome. That is arguably jpegli's bug to
fix, and it is being reported there separately — but it is a second route into
the same arithmetic, and a fix here closes both.

Severity note

The reorder loop is new in 2.0; v1.4.0 copied the base verbatim. The bad splice
happened there too — it just produced a base JPEG with garbage between SOI and
the first table, which decoders resynchronised past. So this is not a 2.0
regression: the added SOS check is what makes an existing silent corruption
visible. Worth keeping that in mind when weighing the fix.

Suggested fix

Locate the segment in the source buffer instead of deriving a position from
marker_list. The payloads copied out of the marker structs are correct either
way; only the offset is affected. A scan is ~30 lines, order-independent, and
immune to unsaved markers:

static long findExifPayloadOffset(const uint8_t* data, size_t length, size_t* payload_size) {
  *payload_size = 0;
  if (data == nullptr || length < 4 || data[0] != 0xFF || data[1] != 0xD8) return -1;

  size_t pos = 2;
  while (pos + 4 <= length) {
    if (data[pos] != 0xFF) return -1;
    const uint8_t marker = data[pos + 1];
    if (marker == 0xFF) { pos += 1; continue; }                          // fill byte
    if (marker == 0xD8 || (marker >= 0xD0 && marker <= 0xD7)) { pos += 2; continue; }
    if (marker == 0xD9 || marker == 0xDA) return -1;                     // EOI / scan
    const size_t seg_len = (static_cast<size_t>(data[pos + 2]) << 8) | data[pos + 3];
    if (seg_len < 2 || pos + 2 + seg_len > length) return -1;
    const size_t this_payload = seg_len - 2;
    if (marker == 0xE1 && this_payload >= sizeof kExifIdCode &&
        !memcmp(data + pos + 4, kExifIdCode, sizeof kExifIdCode)) {
      *payload_size = this_payload;
      return static_cast<long>(pos + 4);
    }
    pos += 2 + seg_len;
  }
  return -1;
}

Worth accepting the offset only when the scanned segment's size matches the
payload already copied into mEXIFBuffer — copyJpegWithoutExif cuts exactly
size + 4 bytes from offset - 4, so any disagreement between the two would
corrupt the base. On a mismatch, leaving the offset at -1 drops the EXIF from the
output rather than damaging the file.

Related

#469 concerns the same function from the other direction: it asks for user XMP to
survive the APP-marker skip in the reorder loop. This report is about the one APP
marker that is not skipped — EXIF is lifted out and re-emitted deliberately, and
the arithmetic that locates it can be wrong, so the base is corrupted rather than
stripped. The two would be fixed in the same place but are independent.

Happy to open a PR if this shape looks right.