Bound the RLE decode writes, and signal malformed streams to the caller

[ggorman]

The defect

Run_Length_Decode_Slow loops while num < num_expected_vals, but the two run-length branches write a whole run without re-checking that bound:

else if (ival == RLESC3) {
    int rle = *((unsigned int*)p) >> 8;              // 24-bit run length
    for (int j = 0; j < rle; ++j) vals[num+j] = 0.0f;
    num += rle; p += 3;
}

The only bound is assert(num+rle <= num_expected_vals) guarded by #ifdef DEBUG_DECODE, and DEBUG_DECODE is commented out at Run_Length_Encode_Slow.cpp:14. So in every shipped build one escape byte can write up to 16,777,215 floats — 64 MB — past vals. RLESC1 has the same shape with an 8-bit run.

Reproduction

256-float destination; stream of eight literals then one RLESC3 requesting 65,536. Built from this translation unit with clang++ -O1 -g -fsanitize=address, DEBUG_DECODE undefined:

AddressSanitizer: heap-buffer-overflow
WRITE of size 262144 at ... Run_Length_Encode_Slow.cpp:443

262144 is exactly 65536 × 4. A control stream whose run lands exactly on the array end (248 after the same eight literals) enters the same RLESC3 branch at the same num=8 and completes cleanly, so the run length is the only thing that differs. A -DDEBUG_DECODE build confirms it from the project's own instrumentation:

control:  RLESC3 rle=248,   num=8, num_expected_vals=256     (no assert)
trigger:  RLESC3 rle=65536, num=8, num_expected_vals=256
          Assertion failed: (num+rle <= num_expected_vals), line 441

The repository already treats this input as a violation — it just does not compile the check into shipped builds.

What this changes

1. Clamp both run-length writes to the caller's remaining capacity.

2. Return -1 when a run was clamped, and check it at the call site. This second half matters. Clamping alone converts an out-of-bounds write into a silently accepted malformed stream: num += rle still runs, so the function returned 65,544 for a 256-element array, and CvxCompress::Decompress discarded the return value entirely (CvxCompress.cpp:561). A truncated or partially-written block would decode to an unreliable volume with nothing to indicate it. Decompress now counts malformed blocks across the parallel loop (reduction(+:)) and reports once on stderr.

Well-formed input is bit-for-bit unaffected: the clamp is a no-op whenever the run fits, and the return value is unchanged.

Verified against the reproducer with the patched files:

	well-formed	oversized run
before	clean, returns 256	heap-buffer-overflow, abort
clamp only	clean, returns 256	no overflow, returns 65544 (indistinguishable from success)
this PR	clean, returns 256	no overflow, returns -1

Context

Found while investigating an intermittent double free or corruption in an MPI seismic gradient using compressed disk snapshots. The proximate cause there was separately root-caused to concurrent writes to a shared snapshot path producing sparse files; this decoder behaviour is what turned those files into heap corruption instead of an error. Fixing it is defence in depth, and is independent of that root cause.

Two adjacent issues NOT addressed here

Raised for separate consideration rather than bundled in:

1. The decoder receives no length for compressed. Its look-ahead reads — the 8-byte SIMD load, and the 3-byte run field — can read past the end of a truncated stream regardless of this write clamp. CvxCompress::Decompress does receive compressed_length but does not pass it down.
2. int rle = *((unsigned int*)p) >> 8 is an unaligned, type-punned load whose value is endianness-dependent. Benign on the little-endian targets in use, but not portable.

I'm happy to follow up on either if useful.

[ggorman]

@mloubout — could you take a look when you get a chance? Flagging you since you've merged here before, and this touches the decoder path DevitoPRO's compressed snapshot save/restore goes through.

I don't have permissions to add reviewers on this repo, hence the mention rather than a review request.

The two-line clamp is the part that matters for the out-of-bounds write. The return-value change is the part I'd most like a second opinion on: it makes Run_Length_Decode_Slow return -1 on a malformed run and has Decompress report it, on the grounds that clamping alone leaves the condition undetectable. If you'd rather keep the return strictly a count, I can split that into a separate flag/out-parameter instead.

[ggorman]

Follow-up 1 in the description — the missing input length — now has a reproducer rather than just an assertion, in case it helps decide whether it's worth a second PR.

The decoder's only termination condition is a property of the output:

for (;  num < num_expected_vals;  ++p)
    int val0 = ((int*)p)[0];        // :397, 8-byte look-ahead

so a stream shorter than the output demands runs p off the end while num is still far below the limit. Under ASan, with the destination deliberately over-sized so the write side can't contribute, and an all-zero stream so the SIMD fast path is taken:

decoder	control (1024-byte stream)	trigger (32-byte stream)
unpatched	clean, num=256	heap-buffer-overflow, READ of size 4, abort
write clamp only	clean, num=256	same READ overflow
this PR	clean, num=256	same READ overflow

Identical on all three arms, which is the point — clamping bounds vals and cannot bound compressed, since the function is never told how long it is. So this PR genuinely doesn't address it.

Geometry is hand-checkable: the fast path does p += 8 and the loop header adds ++p, so p advances 9 bytes per 8 values. On a 32-byte stream, iteration 3 starts at p=27 and reads bytes 27–34 with num still at 24.

One detail worth flagging if you do take this on: the look-ahead bound isn't 8 bytes. VLESC3_8x reads _mm_loadu_si128((__m128i*)(p+13)), so the widest read is 29 bytes from p, and a guard would need p + 29 <= end.

I've left it out of this PR because it changes a public signature and touches every call path, so it seemed like your call on the interface rather than something to bundle into a bounds fix. Decompress already receives compressed_length, and per-block offsets are in glob_blkoffs, so the length is derivable at the call site. Happy to put it up as a separate PR if you'd like it.