Re: [PR] AVRO-4301: [js] Bound collection allocation when decoding arrays and maps [avro]

["Copilot (via GitHub)" <gi...@apache.org>]

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: issues-unsubscribe@avro.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org