Copilot commented on code in PR #3866:
URL: https://github.com/apache/avro/pull/3866#discussion_r3567334237
##########
lang/js/lib/schemas.js:
##########
@@ -1303,14 +1357,30 @@ ArrayType.prototype._read = function (tap) {
};
ArrayType.prototype._skip = function (tap) {
+ var items = this._items;
+ var minBytes = getMinBytes(items);
+ var total = 0;
var len, n;
while ((n = tap.readLong())) {
if (n < 0) {
+ // Sized block: bound the item count too, so the cap cannot be bypassed
by
+ // encoding the block with a negative (byte-sized) count.
+ n = -n;
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;
Review Comment:
In the sized-block skip path (negative count), `len` is only validated as
non-negative before `tap.pos += len`. If the block byte-size is inconsistent
with the declared item count (e.g., `len` too small to hold `n` items at the
minimum on-wire size) or exceeds the remaining buffer, `_skip` can jump to the
wrong position and the caller may decode subsequent fields incorrectly without
`Tap.isValid()` failing. Add sanity checks against remaining bytes and (when
`minBytes > 0`) against `len / minBytes`.
##########
lang/js/lib/schemas.js:
##########
@@ -1162,12 +1193,27 @@ MapType.prototype._read = function (tap) {
MapType.prototype._skip = function (tap) {
var values = this._values;
+ var minBytes = 1 + getMinBytes(values); // Each entry carries a >=1-byte key.
+ var total = 0;
var len, n;
while ((n = tap.readLong())) {
if (n < 0) {
+ // Sized block: bound the item count too, so the cap cannot be bypassed
by
+ // encoding the block with a negative (byte-sized) count.
+ n = -n;
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;
Review Comment:
In the sized-block skip path (negative count), `len` is only checked for
negativity before `tap.pos += len`. If `len` is inconsistent with the declared
item count (e.g., too small to contain `n` entries at the minimum on-wire size)
or larger than the remaining bytes, the skip can silently misalign the tap
position and corrupt decoding of subsequent fields without tripping
`Tap.isValid()`. Add sanity checks to ensure `len` is within the remaining
buffer and that `n` fits within `len` at `minBytes` (when `minBytes > 0`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
