IiqDecoder: bounds-check col before correctBadColumn

[Alearner12]

Bug

IiqDecoder::correctSensorDefects (src/librawspeed/decoders/IiqDecoder.cpp:501) only validates col >= mRaw->dim.x before dispatching to correctBadColumn(col) for tag types 131 and 137. correctBadColumn (line 530) then reads neighbours at col-1, col+1, col-2, col+2 over a loop that ranges row in [2, dim.y-3].

Two failure modes once correctBadColumn is entered with a near-edge col:

col	Path	Read site	Effect
0 or 1	non-green (line 570-572)	img(row-2, col-2) at row=2	data[(0)*W + (col-2)] = data[-2] or data[-1] — heap OOB read before buffer
dim.x-1, dim.x-2	non-green (line 570-572)	img(row+2, col+2) at row=dim.y-3	data[(dim.y-1)*W + (W or W+1)] — heap OOB read past buffer end
0	green (line 547-550)	img(row-1, col-1) at row=2	data[1*W - 1] — in-bounds wrap to previous row's last element; silent data corruption (not OOB)

The Array2DRef::operator() invariant invariant(col >= 0) (adt/Array2DRef.h:~67) compiles to __builtin_assume in release builds (NDEBUG defined; see adt/Invariant.h:36), so the OOB is reached rather than asserted on. ASan flags it cleanly.

Reproduction

Standalone extract of the access pattern (no librawspeed dependency, no IIQ file). Mirrors the non-green-path reads at lines 570-572. Build with g++ -O1 -g -fsanitize=address,undefined -DNDEBUG minimal_poc.cpp -o minimal_poc:

col = 50 (in-bounds, expected OK): OK
col = 0 (left edge, expect ASan heap-buffer-overflow):
==NNNN==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x52a0000001fc
READ of size 2 at 0x52a0000001fc thread T0
    #0 correctBadColumn_nongreen_reads minimal_poc.cpp:68
0x52a0000001fc is located 4 bytes to the left of 20000-byte region [0x52a000000200,0x52a000005020)

Matches the predicted img(row-2, col-2) at row=2, col=0 = data[-2] — 2 uint16_t elements = 4 bytes before the buffer start.

Fix

Caller-side guard, sitting right after the existing col >= mRaw->dim.x check and inside the type-131/137 dispatch arm so the bad-pixel (type 129) path is unaffected. correctBadColumn is a private member with only this one caller, so caller-side and function-side give the same coverage; the caller-side shape matches the existing upper-bound check style and is the smaller diff.

Verification

Same access pattern guarded by the equivalent check (minimal_poc_patched.cpp):

col = 50: OK (no ASan trip)
col = 0: OK (no ASan trip)
col = 1: OK (no ASan trip)
col = 98: OK (no ASan trip)
col = 99: OK (no ASan trip)
col = 100: OK (no ASan trip)

All previously-OOB columns now skipped, in-bounds column still processed.

The full librawspeed test suite was not run in this engagement (missing libjpeg/libpugixml dev packages in my environment blocked the full cmake build). The change is additive within the existing case 131/137: arm; tag types 129 and unknown types, the upper-bound check, and the rest of the loop are unchanged, so no existing-input-path regression is possible by construction.

[LebedevRI]

As far as the callsite is concerned, correctBadColumn() is a black-box,
we can't randomly skip it.

[Alearner12]

Thanks for the feedback. You're right that the callsite shouldn't encode knowledge of correctBadColumn's internal access pattern.

Would moving the guard inside correctBadColumn itself work?

void IiqDecoder::correctBadColumn(int col) const {
  if (col < 2 || col + 2 >= mRaw->dim.x)
    return;
  // ... rest unchanged ...
}

This keeps the callsite clean and makes the function self-contained about what inputs it can handle. Happy to update the PR if this direction looks good.

[LebedevRI]

Thanks for the feedback. You're right that the callsite shouldn't encode knowledge of correctBadColumn's internal access pattern.

Would moving the guard inside correctBadColumn itself work?

I mean, it's not like there is some third place where that check could be placed? :)

void IiqDecoder::correctBadColumn(int col) const {
  if (col < 2 || col + 2 >= mRaw->dim.x)
    return;
  // ... rest unchanged ...
}

This keeps the callsite clean and makes the function self-contained about what inputs it can handle. Happy to update the PR if this direction looks good.

[Alearner12]

Haha, fair point! Moved the guard inside correctBadColumn in the latest commit callsite is now clean.

[LebedevRI]

@Alearner12 thank you!