AVRO-4301: [js] Bound collection allocation when decoding arrays and maps by iemejia · Pull Request #3866 · apache/avro

iemejia · 2026-07-12T14:36:14Z

What is the purpose of the change

An array or map block is encoded as an item count followed by that many items. A malicious or truncated input can declare a very large count while carrying little or no data, which causes a correspondingly large allocation before the shortfall is noticed.

The JavaScript SDK already bounds length-prefixed bytes/string/fixed values against its in-memory buffer (Tap.readFixed/readString check the position against the buffer length before allocating), which is why it had no available-bytes sub-task under AVRO-4292. Collections, however, were not bounded: ArrayType._read and MapType._read read the block item count and push elements incrementally into a plain array/object with no cap and no bytes-remaining check.

Because zero-byte elements (e.g. null) consume no input, a ~6 byte payload such as {"type":"array","items":"null"} declaring a block count of 200,000,000 builds a 200M-entry array and exhausts the heap. Reproduced on main:

FATAL ERROR: Ineffective mark-compacts near heap limit Allocation failed -
JavaScript heap out of memory

under node --max-old-space-size=256. Even truncated non-zero-byte collections are affected, because an out-of-range Buffer read returns undefined rather than throwing, so the loop runs to the full declared count; the tap.isValid() check only happens after _read returns.

Fix

ArrayType._read/_skip and MapType._read/_skip now bound each block, consistent with the other SDKs:

reject a block whose element count could not be backed by the bytes remaining, using getMinBytes() (the minimum on-wire size of the element schema) so a zero-byte element type is never falsely rejected;
cap the cumulative count of zero-byte elements (MAX_COLLECTION_ITEMS, default 10,000,000);
apply a structural cap to every collection (MAX_COLLECTION_STRUCTURAL, default Integer.MAX_VALUE - 8);
additionally reject overlong varints in Tap.readLong/Tap.skipLong and bounds-check the branch index on the union skip path (UnionType._skip).

When set, the AVRO_MAX_COLLECTION_ITEMS environment variable caps both limits. Under schema resolution the minimum is computed from the writer element type (what is actually on the wire, precomputed in _updateResolver), so legitimate collections are not falsely rejected.

This is a sub-task of AVRO-4292 and resolves AVRO-4301.

Verifying this change

This change added tests and can be verified as follows:

Extended lang/js/test/test_schemas.js: for arrays, a huge zero-byte block count rejected; for both arrays and maps, a huge non-zero-byte block count rejected (above the remaining bytes), a small collection that still decodes, the skip path bounded, and rejection plus a legitimate decode under schema resolution; plus an array INT64_MIN block-count rejection and a union skip invalid-index test. Also added overlong-varint tests in lang/js/test/test_utils.js.
Manually verified against the PoC (array<null>, block count 200,000,000) under --max-old-space-size=256: rejected with a clean error instead of an OOM crash.
Run: cd lang/js && npm test (391 passing) and npm run lint (clean).

Documentation

Does this pull request introduce a new feature? (no — hardening / robustness)
If yes, how is the feature documented? (not applicable; adds the AVRO_MAX_COLLECTION_ITEMS environment override, consistent with the other SDKs)

…maps The JavaScript SDK already bounds length-prefixed bytes/string/fixed values against its in-memory buffer (Tap.readFixed/readString check the position against the buffer length before allocating), but collections were not bounded. ArrayType._read and MapType._read read the block item count and push elements incrementally into a plain array/object with no cap and no bytes-remaining check. Because zero-byte elements (e.g. null) consume no input, a ~6 byte payload such as {"type":"array","items":"null"} declaring a block count of 200,000,000 builds a 200M-entry array and exhausts the heap (reproduced: "FATAL ERROR: JavaScript heap out of memory" under --max-old-space-size=256). Even truncated non-zero-byte collections are affected, because an out-of-range Buffer read returns undefined rather than throwing, so the loop runs to the full declared count; the tap validity check only happens after the read returns. Bound the array/map read and skip paths, consistent with the other SDKs: - reject a block whose element count could not be backed by the bytes remaining, using getMinBytes() (the minimum on-wire size of the element schema) so a zero-byte element type is never falsely rejected; - cap the cumulative count of zero-byte elements (MAX_COLLECTION_ITEMS, default 10,000,000); - apply a structural cap to every collection (MAX_COLLECTION_STRUCTURAL, default Integer.MAX_VALUE - 8). When set, the AVRO_MAX_COLLECTION_ITEMS environment variable caps both limits. Under schema resolution the minimum is computed from the writer element type (what is actually on the wire) so legitimate collections are not falsely rejected. Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot

Pull request overview

This PR hardens the JavaScript Avro decoder against denial-of-service scenarios where malicious or truncated inputs declare extremely large array/map block counts, leading to unbounded allocations or excessive iteration before the buffer shortfall is detected.

Changes:

Adds collection-size limits (structural and zero-byte-item caps) with an AVRO_MAX_COLLECTION_ITEMS environment override.
Bounds array/map block reads (and resolver-aware reads) using a minimum-on-wire-size computation and remaining-bytes checks.
Extends JS schema tests to cover rejection of huge block counts and continued decoding of small collections.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
lang/js/lib/schemas.js	Introduces collection caps, min-bytes calculation, and enforces block bounds in `ArrayType`/`MapType` read/skip and resolver paths.
lang/js/test/test_schemas.js	Adds tests for rejecting huge declared block counts and verifying small collections still decode correctly.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

iemejia · 2026-07-12T17:32:44Z

+  } else {
+    // Booleans, ints, longs, strings, bytes, enums (index), unions (index),
+    // and arrays/maps (empty block terminator) all occupy at least one byte.
+    mb = 1;
+  }


Fixed in 6ae9c61: getMinBytes now handles unions as 1 (branch index) + the smallest branch minimum, so ['int','long'] is at least 2 bytes and the available-bytes check isn't underestimated.

iemejia · 2026-07-12T17:32:45Z

  while ((n = tap.readLong())) {
    if (n < 0) {
      len = tap.readLong();
      tap.pos += len;
    } else {


Fixed in 6ae9c61: the negative (byte-sized) block branch of _skip now applies the cumulative total and checkCollectionBlock too, so the caps can't be bypassed with a negative count. Added a regression test.

iemejia · 2026-07-12T17:32:46Z

  while ((n = tap.readLong())) {
    if (n < 0) {
      len = tap.readLong();
      tap.pos += len;
    } else {


Fixed in 6ae9c61: same fix applied to the other collection's _skip negative-block branch.

JavaScript numbers are IEEE-754 doubles, so negating an INT64_MIN block count yields ~2^63 (no wrap to a negative), which the existing structural cap already rejects. Add a regression test decoding the 10-byte INT64_MIN varint as an array<null> block count and asserting the collection-size rejection, matching the negation-overflow coverage added to the other SDKs. Assisted-by: GitHub Copilot:claude-opus-4.8

Addresses review feedback: - getMinBytes now handles unions explicitly as 1 (branch index) + the smallest branch minimum, instead of a flat 1. A union without a zero-byte branch (e.g. ['int','long']) is at least 2 bytes, so the available-bytes check is no longer underestimated for arrays/maps of such unions. - ArrayType._skip / MapType._skip now apply the cumulative total and checkCollectionBlock to the negative (byte-sized) block branch too, so the structural/item caps cannot be bypassed by encoding a block with a negative count during skip/resolution. Adds a regression test. Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

iemejia · 2026-07-12T18:44:25Z

+      n = -n;
      len = tap.readLong();
+      total += n;
+      checkCollectionBlock(tap, n, minBytes, total);
      tap.pos += len;


Fixed in b50e676: the sized-block skip path now rejects a negative block byte-size before tap.pos += len, so it can't move the position backwards and bypass truncation detection (Tap.isValid only checks pos <= buf.length).

iemejia · 2026-07-12T18:44:27Z

+      n = -n;
      len = tap.readLong();
+      total += n;
+      checkCollectionBlock(tap, n, minBytes, total);
      tap.pos += len;


Fixed in b50e676: same negative-byte-size guard added to the other collection's _skip sized-block branch.

iemejia · 2026-07-12T18:44:28Z

+    it('rejects a huge block count', function () {
+      var t = new types.MapType({type: 'map', values: 'long'});
+      var buf = Buffer.from([0x80, 0x88, 0xde, 0xbe, 0x01, 0x00]);
+      assert.throws(function () { t.fromBuffer(buf); }, /collection/);
+    });
+
+    it('reads a small map', function () {
+      var t = new types.MapType({type: 'map', values: 'int'});
+      var buf = t.toBuffer({a: 1, b: 2});
+      assert.deepEqual(t.fromBuffer(buf), {a: 1, b: 2});
+    });


Fixed in b50e676: added MapType coverage mirroring the array cases — huge block count while skipping, huge negative (sized) block count while skipping, a rejected negative block byte-size, and a small map decoded under schema resolution.

… tests Addresses review feedback: - ArrayType._skip / MapType._skip now reject a negative block byte-size in the sized-block branch before `tap.pos += len`. A negative len would move the read position backwards, which Tap.isValid() (pos <= buf.length) does not catch, bypassing truncation detection. - Add MapType coverage mirroring the array cases: huge block count while skipping, huge negative (sized) block count while skipping, a rejected negative block byte-size, and a small map decoded under schema resolution. Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

iemejia · 2026-07-12T20:52:29Z

Preallocation (AVRO-4292 follow-up) evaluated — no change needed here. ArrayType._read/MapType._read append via val.push(...), growing the array on demand rather than preallocating to the declared block count, so there is no up-front over-allocation to bound (unlike the Java/C++/C# readers). The existing structural and zero-byte caps already bound the growth.

UnionType._read already rejects an out-of-range union branch index, but UnionType._skip indexed this._types[index] directly, so a malformed index while skipping a union (e.g. a writer field absent from the reader schema) threw a raw TypeError on undefined instead of a clear "invalid union index" error. Validate the index and throw the same error as the read path. Adds a skip test for an out-of-range union index. (EnumType._skip only skips a long and does not index, so it is unaffected; both read paths were already guarded.) Assisted-by: GitHub Copilot:claude-opus-4.8

The integer part of readLong is bounded to 4 bytes (k < 28), but the float fallback loop and skipLong read continuation bytes with no cap, so an overlong varint was silently accepted (as a lossy float) rather than rejected. A 64-bit value uses at most 10 bytes; reject beyond that with an error, matching the Java, C and C++ SDKs. Adds read and skip regression tests. Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

iemejia · 2026-07-12T23:38:53Z

      len = tap.readLong();
+      if (len < 0) {
+        // A negative byte-size would move the read position backwards, which
+        // Tap.isValid() (pos <= buf.length) would not catch, bypassing
+        // truncation detection.
+        throw new Error('negative collection block size');
+      }
+      total += n;
+      checkCollectionBlock(tap, n, minBytes, total);
      tap.pos += len;


Fixed in MapType._skip — the sized-block path now rejects a len that exceeds the remaining buffer, and (when minBytes > 0) a len too small to hold n entries at their minimum on-wire size, so the skip can't misalign subsequent decoding.

iemejia · 2026-07-12T23:38:54Z

      len = tap.readLong();
+      if (len < 0) {
+        // A negative byte-size would move the read position backwards, which
+        // Tap.isValid() (pos <= buf.length) would not catch, bypassing
+        // truncation detection.
+        throw new Error('negative collection block size');
+      }
+      total += n;
+      checkCollectionBlock(tap, n, minBytes, total);
      tap.pos += len;


Fixed the same way in ArrayType._skip — added the len <= remaining and n <= len / minBytes sanity checks before tap.pos += len.

…paths The negative (byte-sized) block skip path only checked len for negativity before `tap.pos += len`. A len larger than the remaining buffer would jump past the end (misaligning subsequent decoding), and a len too small to hold n entries at their minimum on-wire size would land mid-entry -- neither tripping Tap.isValid(). Reject both: len must be within the remaining buffer, and (when minBytes > 0) n must fit within len at minBytes. Applied to ArrayType._skip and MapType._skip. Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

iemejia · 2026-07-13T01:41:53Z

 MapType.prototype._updateResolver = function (resolver, type, opts) {
  if (type instanceof MapType) {
    resolver._values = this._values.createResolver(type._values, opts);
+    // Bound the block count using the writer's entry size (see ArrayType).
+    resolver._valuesMinBytes = 1 + getMinBytes(type._values);
    resolver._read = this._read;


Fixed — added _itemsMinBytes and _valuesMinBytes (initialized to undefined) to the Resolver constructor so all resolvers keep the same hidden class; _updateResolver now just assigns existing fields. The !== undefined fallback checks in the read paths still work.

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

iemejia · 2026-07-13T01:50:58Z

+    seen = seen || [];
+    if (~seen.indexOf(type)) {
+      return 0; // Cycle; see note above.
+    }


This is a deliberate conservative lower bound: for a cyclic schema whose true minimum can't be computed without unbounded recursion, returning 0 can only make the bytes-remaining check more permissive (it never falsely rejects valid data) — matching how the other SDKs treat recursion. Computing the exact minimum for a recursive union branch isn't decidable in general here. I've reworded the docstring to state this explicitly rather than implying the bound is tight.

iemejia · 2026-07-13T01:50:59Z

+ * Used to bound collection block counts against the bytes actually remaining.
+ * Memoized on the type; recursive records are handled with a `seen` guard
+ * (a directly self-referential required field cannot encode in finite bytes,
+ * and self-references reached through an array/map/union already contribute at
+ * least one byte before recursing, so returning 0 on a cycle is safe).


Reworded the docstring — it now says the 0-on-cycle result is a conservative lower bound that may under-estimate (e.g. a union whose only small branch is a recursive record) but only ever makes the check more permissive, never rejecting valid data.

…d hidden class _updateResolver set _itemsMinBytes / _valuesMinBytes on the resolver at runtime, but the Resolver constructor predefines all fields so resolvers share one hidden class. Adding these later forced a shape transition. Declare both in the constructor (undefined) so the hidden class stays shared; the `!== undefined` checks in the read paths are unaffected. Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

… bound Reword the recursive-record note: returning 0 on a cycle is a deliberately conservative lower bound that can only make the bytes-remaining check more permissive (never rejecting valid data), even though it may under-estimate the true minimum for a union whose only small branch is a recursive record. Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

github-actions Bot added the JS label Jul 12, 2026

iemejia requested a review from Copilot July 12, 2026 14:39

Copilot started reviewing on behalf of iemejia July 12, 2026 14:40 View session

Copilot AI reviewed Jul 12, 2026

View reviewed changes

iemejia added 2 commits July 12, 2026 18:27

iemejia requested a review from Copilot July 12, 2026 17:33

Copilot started reviewing on behalf of iemejia July 12, 2026 17:34 View session

Copilot AI reviewed Jul 12, 2026

View reviewed changes

iemejia requested a review from Copilot July 12, 2026 18:47

Copilot started reviewing on behalf of iemejia July 12, 2026 18:47 View session

Copilot AI reviewed Jul 12, 2026

View reviewed changes

iemejia added 2 commits July 12, 2026 23:38

iemejia requested a review from Copilot July 12, 2026 23:17

Copilot started reviewing on behalf of iemejia July 12, 2026 23:17 View session

Copilot AI reviewed Jul 12, 2026

View reviewed changes

iemejia requested a review from Copilot July 12, 2026 23:43

Copilot started reviewing on behalf of iemejia July 12, 2026 23:43 View session

Copilot AI reviewed Jul 12, 2026

View reviewed changes

iemejia requested a review from Copilot July 12, 2026 23:51

Copilot started reviewing on behalf of iemejia July 12, 2026 23:51 View session

Copilot AI reviewed Jul 12, 2026

View reviewed changes

iemejia requested a review from Copilot July 13, 2026 00:13

Copilot started reviewing on behalf of iemejia July 13, 2026 00:13 View session

Copilot AI reviewed Jul 13, 2026

View reviewed changes

iemejia requested a review from Copilot July 13, 2026 00:29

Copilot started reviewing on behalf of iemejia July 13, 2026 00:29 View session

Copilot AI reviewed Jul 13, 2026

View reviewed changes

iemejia requested a review from Copilot July 13, 2026 00:44

Copilot started reviewing on behalf of iemejia July 13, 2026 00:45 View session

Copilot AI reviewed Jul 13, 2026

View reviewed changes

iemejia requested a review from Copilot July 13, 2026 00:55

Copilot started reviewing on behalf of iemejia July 13, 2026 00:56 View session

Copilot AI reviewed Jul 13, 2026

View reviewed changes

iemejia requested a review from Copilot July 13, 2026 01:08

Copilot started reviewing on behalf of iemejia July 13, 2026 01:08 View session

Copilot AI reviewed Jul 13, 2026

View reviewed changes

iemejia requested a review from Copilot July 13, 2026 01:19

Copilot started reviewing on behalf of iemejia July 13, 2026 01:20 View session

Copilot AI reviewed Jul 13, 2026

View reviewed changes

iemejia requested a review from Copilot July 13, 2026 01:25

Copilot started reviewing on behalf of iemejia July 13, 2026 01:25 View session

Copilot AI reviewed Jul 13, 2026

View reviewed changes

iemejia requested a review from Copilot July 13, 2026 01:42

Copilot started reviewing on behalf of iemejia July 13, 2026 01:42 View session

Copilot AI reviewed Jul 13, 2026

View reviewed changes

iemejia requested a review from Copilot July 13, 2026 01:53

Copilot started reviewing on behalf of iemejia July 13, 2026 01:54 View session

Copilot AI reviewed Jul 13, 2026

View reviewed changes

iemejia requested a review from Copilot July 13, 2026 04:52

Copilot started reviewing on behalf of iemejia July 13, 2026 04:52 View session

Copilot AI reviewed Jul 13, 2026

View reviewed changes

Uh oh!

Conversation

iemejia commented Jul 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is the purpose of the change

Fix

Verifying this change

Documentation

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

iemejia commented Jul 12, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

iemejia commented Jul 12, 2026 •

edited

Loading