Fast fail array decoding by Ryang-21 · Pull Request #132 · stellar/js-xdr

Ryang-21 · 2026-02-12T22:57:04Z

What

Decoding a malformed array can be fast failed when the array length is greater than the remaining bytes to be decoded. This prevents unnecessary consumption of memory.

Copilot

Pull request overview

This pull request attempts to add fast-fail protection for array decoding to prevent unnecessary memory consumption when decoding malformed XDR arrays. It adds a remainingBytes() method to XdrReader and uses it to check if there are enough bytes available before attempting to decode array elements.

Changes:

Added remainingBytes() method to XdrReader to check unread bytes
Added fast-fail check in Array.read() to compare array length with remaining bytes
Added fast-fail check in VarArray.read() to compare array length with remaining bytes
Added tests for the new fast-fail behavior
Updated CHANGELOG (with extensive formatting changes)

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
src/serialization/xdr-reader.js	Adds new `remainingBytes()` method to return unread bytes
src/array.js	Adds fast-fail check before decoding array elements
src/var-array.js	Adds fast-fail check after reading array length
test/unit/array_test.js	Adds test for fast-fail behavior and updates existing test expectations
test/unit/var-array_test.js	Adds test for fast-fail behavior
CHANGELOG.md	Documents the change (with significant formatting modifications)

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

test/unit/var-array_test.js

CHANGELOG.md

Copilot · 2026-02-12T23:03:02Z

src/array.js

+    if (this._length > reader.remainingBytes()) {
+      throw new XdrReaderError(
+        `insufficient bytes to decode Array of length ${this._length}`
+      );
+    }
+


The fast-fail check compares array length (number of elements) with remaining bytes, which are incompatible units. This check will only catch cases where the number of elements exceeds the number of bytes, but XDR elements typically consume multiple bytes (e.g., Int consumes 4 bytes).

For example, an Array of 2 Int elements requires 8 bytes, but this check would pass if 4 bytes remain (since 2 ≤ 4), leading to a later "read outside boundary" error instead of the intended fast-fail behavior.

This check is too weak to provide the intended protection. Consider either:

Checking if the child type has a known fixed size and multiplying accordingly

Removing this check if the child size cannot be reliably determined

Documenting that this check only provides minimal protection for egregious cases

Suggested change

if (this._length > reader.remainingBytes()) {

throw new XdrReaderError(

`insufficient bytes to decode Array of length ${this._length}`

);

}

// Fast-fail only when the child type has a known fixed byte size.

let elementSize = null;

if (typeof this._childType?.getFixedSize === 'function') {

const size = this._childType.getFixedSize();

if (typeof size === 'number' && size >= 0) {

elementSize = size;

}

} else if (typeof this._childType?.fixedSize === 'number' && this._childType.fixedSize >= 0) {

elementSize = this._childType.fixedSize;

}

if (elementSize !== null) {

const requiredBytes = elementSize * this._length;

if (requiredBytes > reader.remainingBytes()) {

throw new XdrReaderError(

`insufficient bytes to decode Array of length ${this._length}`

);

}

}

Copilot · 2026-02-12T23:03:02Z

test/unit/array_test.js

+  it('fast-fails before decoding child elements when remaining bytes are insufficient', function () {
+    let calls = 0;
+    class FixedSizeChild {
+      static read() {
+        calls += 1;
+        return 0;
+      }
+
+      static write() {}
+
+      static isValid() {
+        return true;
+      }
+    }
+
+    const fixed = new XDR.Array(FixedSizeChild, 2);
+    const reader = new XdrReader([0x00, 0x00, 0x00, 0x01]);
+
+    expect(() => fixed.read(reader)).to.throw(/insufficient bytes/i);
+    expect(calls).to.eql(0);
+  });


The test expects a fast-fail error when decoding an Array of 2 elements with only 4 bytes available. However, the fast-fail check compares 2 > 4 (array length vs bytes), which is false, so no error will be thrown. The test will fail because:

No exception is thrown (line 48 expectation fails)

The mock FixedSizeChild.read() is called twice, setting calls=2 instead of the expected 0 (line 49 expectation fails)

This test appears to be based on an incorrect understanding of how the fast-fail check works. The check only triggers when array length exceeds remaining bytes as numbers, not when insufficient bytes exist for the actual data.

Copilot · 2026-02-12T23:03:02Z

src/var-array.js

+    if (length > reader.remainingBytes()) {
+      throw new XdrReaderError(
+        `insufficient bytes to decode VarArray of length ${length}`
+      );
+    }


The fast-fail check compares array length (number of elements) with remaining bytes, which are incompatible units. While this check works for the current test case (0 bytes remaining), it provides insufficient protection in general.

For example, if 6 bytes remain after reading the length prefix and the array declares 2 Int elements, the check evaluates 2 > 6 (false), so no error is thrown. However, 2 Int elements require 8 bytes, causing a later "read outside boundary" error instead of an immediate fast-fail.

The check should either:

Account for the known fixed size of child elements when applicable

Be documented as providing only minimal protection for egregious cases where element count exceeds byte count

leighmcculloch · 2026-02-12T23:14:59Z

CHANGELOG.md

This file looks like it has changed and been reformatted which seems unrelated to the issue.

Hm prettier must have done that I can revert the fmting changes

Shaptic · 2026-02-18T19:40:45Z

src/serialization/xdr-reader.js

+   * Remaining unread bytes in the source buffer
+   * @return {Number}
+   */
+  remainingBytes() {


this could be a get property since there are no side effects, just an idea though

src/array.js

fast fail when array length exceeds bytes left to decode

672e956

Ryang-21 requested review from Shaptic, Copilot, leighmcculloch and quietbits February 12, 2026 22:57

Copilot started reviewing on behalf of Ryang-21 February 12, 2026 22:57 View session

Copilot AI reviewed Feb 12, 2026

View reviewed changes

fix array test to have length 1 greater than bytes to decode

26c9d4f

leighmcculloch approved these changes Feb 12, 2026

View reviewed changes

Ryang-21 added 2 commits February 12, 2026 15:20

add additional testing for fast failing

aa0e33e

revert changelog formatting

41e8900

Ryang-21 linked an issue Feb 17, 2026 that may be closed by this pull request

Improve code hardiness when decoding #133

Closed

quietbits approved these changes Feb 18, 2026

View reviewed changes

Shaptic reviewed Feb 18, 2026

View reviewed changes

src/array.js Show resolved Hide resolved

Ryang-21 and others added 4 commits February 20, 2026 11:38

Merge branch 'master' into fast-fail-array-decode

7ca72f0

fix merge mistake

fe2685a

add changelog entry

34525b4

clarify error message and add comment to clarify upper bound fast fail

8abd8be

Ryang-21 merged commit 03a7c07 into master Feb 24, 2026
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fast fail array decoding#132

Fast fail array decoding#132
Ryang-21 merged 8 commits intomasterfrom
fast-fail-array-decode

Ryang-21 commented Feb 12, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI Feb 12, 2026

Uh oh!

Copilot AI Feb 12, 2026

Uh oh!

Copilot AI Feb 12, 2026

Uh oh!

leighmcculloch Feb 12, 2026

Uh oh!

Ryang-21 Feb 12, 2026

Uh oh!

Shaptic Feb 18, 2026

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

-    if (this._length > reader.remainingBytes()) {
-      throw new XdrReaderError(
-        `insufficient bytes to decode Array of length ${this._length}`
-      );
-    }
+    // Fast-fail only when the child type has a known fixed byte size.
+    let elementSize = null;
+    if (typeof this._childType?.getFixedSize === 'function') {
+      const size = this._childType.getFixedSize();
+      if (typeof size === 'number' && size >= 0) {
+        elementSize = size;
+      }
+    } else if (typeof this._childType?.fixedSize === 'number' && this._childType.fixedSize >= 0) {
+      elementSize = this._childType.fixedSize;
+    }
+    if (elementSize !== null) {
+      const requiredBytes = elementSize * this._length;
+      if (requiredBytes > reader.remainingBytes()) {
+        throw new XdrReaderError(
+          `insufficient bytes to decode Array of length ${this._length}`
+        );
+      }
+    }

Conversation

Ryang-21 commented Feb 12, 2026

What

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

leighmcculloch Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Ryang-21 Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Shaptic Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants