Copilot commented on code in PR #3929:
URL: https://github.com/apache/avro/pull/3929#discussion_r3737319497
##########
lang/py/avro/io.py:
##########
@@ -845,8 +887,33 @@ def read(self, decoder: "BinaryDecoder") -> object:
# many small collection fields cannot bypass it (see
# _ensure_collection_available).
self._zero_byte_items_read = 0
+ # Start a fresh nesting-depth budget for this datum (see _nested_read).
+ self._read_depth = 0
return self.read_data(self.writers_schema, reader_schema, decoder)
+ @contextlib.contextmanager
+ def _nested_read(self) -> Generator[None, None, None]:
+ """Track one level of structural nesting while decoding.
+
+ Entering a record, array, map or union grows the (recursive) Python
call
+ stack. Bounding the nesting depth turns a recursive schema fed a deeply
+ nested payload into a clean, catchable error instead of a
RecursionError
+ or a fatal interpreter crash from C-stack exhaustion. The check runs
+ before incrementing so the counter stays balanced as the exception
+ unwinds the enclosing (already-entered) levels, and the depth is
restored
+ on exit so a reader instance can be reused.
+ """
+ if self._read_depth >= _max_decode_depth():
+ raise avro.errors.AvroException(
+ f"Decode nesting depth exceeds the maximum allowed of
{_max_decode_depth()} "
+ f"(configure with the {MAX_DECODE_DEPTH_ENV} environment
variable)"
+ )
+ self._read_depth += 1
Review Comment:
_nested_read() calls _max_decode_depth() twice inside the failure path,
which re-reads/parses the env var and could produce an inconsistent error
message if the environment changes between calls. Compute the limit once and
reuse it for the check and message.
##########
lang/py/avro/test/test_decode_recursion_depth.py:
##########
@@ -0,0 +1,84 @@
+#!/usr/bin/env python3
+
+##
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# https://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+"""AVRO-4302: bound decode recursion depth to prevent stack exhaustion."""
+
+import io
+import json
+import unittest
+
+import avro.errors
+import avro.io
+import avro.schema
+
+
+def _encode_long(value: int) -> bytes:
+ """Zig-zag + varint encode a long, matching BinaryEncoder.write_long."""
+ datum = (value << 1) ^ (value >> 63)
+ out = bytearray()
+ while (datum & ~0x7F) != 0:
+ out.append((datum & 0x7F) | 0x80)
+ datum >>= 7
+ out.append(datum)
+ return bytes(out)
+
+
+class TestDecodeRecursionDepth(unittest.TestCase):
+ # A self-referencing linked-list schema: the classic recursion-bomb shape.
+ NODE = avro.schema.parse(json.dumps({"type": "record", "name": "Node",
"fields": [{"name": "next", "type": ["null", "Node"]}]}))
+
+ @staticmethod
+ def _linked_list(depth: int) -> bytes:
+ """Encode a Node linked list nested ``depth`` levels deep.
+
+ Each level selects the ``Node`` union branch (index 1); the final level
+ selects ``null`` (index 0) to terminate. ~1 byte per level.
+ """
+ return _encode_long(1) * depth + _encode_long(0)
+
+ def _read(self, data: bytes) -> object:
+ reader = avro.io.DatumReader(self.NODE, self.NODE)
+ return reader.read(avro.io.BinaryDecoder(io.BytesIO(data)))
+
+ def test_deeply_nested_input_rejected_with_bounded_error(self) -> None:
+ # ~100k levels: far beyond the default depth limit and enough to
overflow
+ # the stack if it were left unbounded, yet only ~100kB of input. Must
fail
+ # with a bounded AvroException rather than a RecursionError / crash.
+ bomb = self._linked_list(100_000)
+ self.assertRaises(avro.errors.AvroException, self._read, bomb)
+
+ def test_moderately_nested_input_within_limit_still_decodes(self) -> None:
+ # Two structural descents (union + record) are counted per list level,
so
+ # keep the level count well under half the default limit.
+ result = self._read(self._linked_list(20))
+ self.assertIsInstance(result, dict)
+
+ def test_custom_depth_limit_env_is_honored(self) -> None:
+ import os
+
+ os.environ[avro.io.MAX_DECODE_DEPTH_ENV] = "6"
+ try:
+ # 6 allows only 3 list levels (2 descents each); 10 levels must
fail.
+ self.assertRaises(avro.errors.AvroException, self._read,
self._linked_list(10))
+ finally:
+ del os.environ[avro.io.MAX_DECODE_DEPTH_ENV]
Review Comment:
This test mutates AVRO_MAX_DECODE_DEPTH but always deletes it in the finally
block. If the variable was already set in the environment (e.g., in a developer
shell or CI), this will clobber that configuration for subsequent tests.
Preserve and restore the prior value instead of unconditionally deleting it.
##########
lang/py/avro/io.py:
##########
@@ -918,12 +986,15 @@ def read_data(self, writers_schema: avro.schema.Schema,
readers_schema: avro.sch
if isinstance(writers_schema, avro.schema.EnumSchema) and
isinstance(readers_schema, avro.schema.EnumSchema):
return self.read_enum(writers_schema, readers_schema, decoder)
if isinstance(writers_schema, avro.schema.ArraySchema) and
isinstance(readers_schema, avro.schema.ArraySchema):
- return self.read_array(writers_schema, readers_schema, decoder)
+ with self._nested_read():
+ return self.read_array(writers_schema, readers_schema, decoder)
if isinstance(writers_schema, avro.schema.MapSchema) and
isinstance(readers_schema, avro.schema.MapSchema):
- return self.read_map(writers_schema, readers_schema, decoder)
+ with self._nested_read():
+ return self.read_map(writers_schema, readers_schema, decoder)
if isinstance(writers_schema, avro.schema.RecordSchema) and
isinstance(readers_schema, avro.schema.RecordSchema):
# .type in ["record", "error", "request"]:
- return self.read_record(writers_schema, readers_schema, decoder)
+ with self._nested_read():
+ return self.read_record(writers_schema, readers_schema,
decoder)
Review Comment:
The new depth budget is enforced only via read_data() wrappers. However,
schema resolution can call skip_data() when the writer has fields absent in the
reader (read_record uses skip_data for unknown fields), and
skip_data/skip_union/skip_record recurse structurally without any depth check.
A hostile payload could still trigger RecursionError/stack exhaustion through
the skip path. Consider applying the same depth bound to structural skip
operations as well (union/record/array/map).
--
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]
