Skip to content

fix: bound array and map reads on a corrupt block count - #510

Open
spokodev wants to merge 1 commit into
mtth:masterfrom
spokodev:fix/bound-array-map-block-count
Open

spokodev wants to merge 1 commit into
mtth:masterfrom
spokodev:fix/bound-array-map-block-count

Conversation

@spokodev

@spokodev spokodev commented Sep 2, 2026

Copy link
Copy Markdown

Closes the hang reported in #306.

An array/map block header carries its element count as a zig-zag long. ArrayType._read / MapType._read loop that many times, and the array path also pre-allocates new Array(n) up front, with no check that the count fits the bytes that remain:

val = val || new Array(n);
while (n--) { val[i++] = items._read(tap); }

Because the count comes straight from the input, a corrupt or malicious block count forces a large allocation and a long loop from a tiny buffer.

Repro

avsc.Type.forSchema({type: array, items: long})
  .fromBuffer(Buffer.from([0x80, 0x84, 0xaf, 0x5f])); // count ~1e8, no elements

On master this builds a ~1e8-element array (~1.4 GB, ~3 s) before finally throwing truncated buffer, and the reader walks ~100k+ bytes past a 4-byte input. Measured growth at constant ~4-byte input: n=1e6 → 7.9 MB/111 ms, n=1e8 → 1.4 GB/3.05 s. This is the "bad topic" hang from #306 — where the conclusion at the time was that there was no clean way to bound it.

Fix

Every element consumes at least one byte, so a count larger than the bytes remaining in the tap is corrupt. Cap the initial new Array(...) to that bound, and stop the loop once the tap runs out of data (tap.isValid()) — which surfaces the existing truncated buffer error immediately, before any large allocation, rather than after it. The same root cause is fixed in both ArrayType and MapType. Bytes/string lengths are already bounded by readFixed and are left untouched.

Valid multi-block data is unaffected: for well-formed input the cumulative count never exceeds the bytes present, so neither the cap nor the isValid() check changes behaviour (the existing multi-block round-trip tests still pass).

Test

Adds a bounds reads on a corrupt block count case to both the ArrayType and MapType suites: a block count of 100000 with no entries must throw truncated buffer without over-reading. Both fail on master (over-read: 100004 / 200004) and pass with the fix. Full suite: 504 passing.

An array or map block header carries an element count as a zig-zag long.
The read loops that many times, and the array path also pre-allocates
`new Array(n)` up front, without checking the count against the bytes
that remain. A corrupt or malicious count therefore forces a huge
allocation and a long loop from a tiny input: a 4-byte buffer declaring
~10^8 elements allocates a ~1.4 GB array and spins for seconds before
the underlying read finally reports a truncated buffer, and the loop can
walk far past the end of the input. This is the hang reported in mtth#306,
where there was no clean way to bound it at the time.

Each element consumes at least one byte, so a count larger than the
bytes remaining is corrupt: cap the initial allocation to that bound and
stop the loop once the tap runs out (tap.isValid()), which surfaces the
existing 'truncated buffer' error immediately instead of after the
allocation. Applies to both ArrayType and MapType; valid multi-block
data is unaffected.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant