Fast fail array decoding #132
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,5 +1,5 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { XdrCompositeType } from './xdr-type'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { XdrReaderError, XdrWriterError } from './errors'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| export class Array extends XdrCompositeType { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| constructor(childType, length) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -12,6 +12,12 @@ export class Array extends XdrCompositeType { | |||||||||||||||||||||||||||||||||||||||||||||||||||
| * @inheritDoc | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| read(reader) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (this._length > reader.remainingBytes()) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
Shaptic marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| throw new XdrReaderError( | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| `insufficient bytes to decode Array of length ${this._length}` | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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}` | |
| ); | |
| } | |
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,12 @@ export class VarArray extends XdrCompositeType { | |
| `saw ${length} length VarArray, max allowed is ${this._maxLength}` | ||
| ); | ||
|
|
||
| if (length > reader.remainingBytes()) { | ||
| throw new XdrReaderError( | ||
| `insufficient bytes to decode VarArray of length ${length}` | ||
| ); | ||
| } | ||
|
Comment on lines
+29
to
+33
There was a problem hiding this comment. 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 The check should either:
|
||
|
|
||
| const result = new Array(length); | ||
| for (let i = 0; i < length; i++) { | ||
| result[i] = this._childType.read(reader); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,10 +23,32 @@ describe('Array#read', function () { | |
|
|
||
| it("throws XdrReaderError when the byte stream isn't large enough", function () { | ||
| expect(() => read(many, [0x00, 0x00, 0x00, 0x00])).to.throw( | ||
| /(read outside the boundary|insufficient bytes)/i | ||
| ); | ||
| }); | ||
|
|
||
| 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); | ||
| }); | ||
|
Comment on lines
+52
to
+96
There was a problem hiding this comment. 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
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. |
||
|
|
||
| function read(arr, bytes) { | ||
| let reader = new XdrReader(bytes); | ||
| return arr.read(reader); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file looks like it has changed and been reformatted which seems unrelated to the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm prettier must have done that I can revert the fmting changes