Copilot commented on code in PR #3866:
URL: https://github.com/apache/avro/pull/3866#discussion_r3566509117
##########
lang/js/lib/schemas.js:
##########
@@ -1162,12 +1188,16 @@ 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) {
len = tap.readLong();
tap.pos += len;
} else {
Review Comment:
In the negative-size block path (`n < 0`), the code skips `len` bytes but
doesn't apply the new `total`/cap accounting, so the structural/item caps can
be bypassed by encoding blocks with negative counts. It also doesn't validate
that the declared `len` can actually contain `n` entries at the minimum on-wire
size, so an inconsistent (count, len) pair can be accepted during
skip/resolution.
##########
lang/js/lib/schemas.js:
##########
@@ -1303,14 +1341,19 @@ 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) {
len = tap.readLong();
tap.pos += len;
} else {
Review Comment:
In the negative-size block path (`n < 0`), the code skips `len` bytes
without updating `total` or applying `checkCollectionBlock`, so the new caps
can be bypassed by encoding blocks with negative counts. For non-zero-byte
element types, it also doesn't validate that `len` can contain `n` elements at
the minimum on-wire size, allowing inconsistent (count, len) pairs to be
silently accepted during skip/resolution.
##########
lang/js/lib/schemas.js:
##########
@@ -2153,6 +2199,74 @@ function readArraySize(tap) {
return n;
}
+/**
+ * Minimum number of bytes a value of the given type can occupy on the wire.
+ *
+ * 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).
+ */
+function getMinBytes(type, seen) {
+ if (type._minBytes !== undefined) {
+ return type._minBytes;
+ }
+ var mb;
+ if (type instanceof NullType) {
+ mb = 0;
+ } else if (type instanceof FixedType) {
+ mb = type._size;
+ } else if (type instanceof FloatType) {
+ mb = 4;
+ } else if (type instanceof DoubleType) {
+ mb = 8;
+ } else if (type instanceof LogicalType) {
+ mb = getMinBytes(type._underlyingType, seen);
+ } else if (type instanceof RecordType) {
+ seen = seen || [];
+ if (~seen.indexOf(type)) {
+ return 0; // Cycle; see note above.
+ }
+ seen.push(type);
+ mb = 0;
+ var fields = type._fields;
+ for (var i = 0, l = fields.length; i < l; i++) {
+ mb += getMinBytes(fields[i]._type, seen);
+ }
+ seen.pop();
+ } else {
+ // Booleans, ints, longs, strings, bytes, enums (index), unions (index),
+ // and arrays/maps (empty block terminator) all occupy at least one byte.
+ mb = 1;
+ }
Review Comment:
`getMinBytes` treats unions as occupying only the 1-byte branch index. For
unions without any 0-byte branch (e.g. ['int','long']), the true on-wire
minimum is `1 + min(branchMin)`. Underestimating here can let
`checkCollectionBlock` accept a block count that isn't actually backed by the
remaining bytes, allowing truncated inputs to still iterate/push many items and
potentially exhaust memory/CPU.
--
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]
