iemejia commented on code in PR #3929: URL: https://github.com/apache/avro/pull/3929#discussion_r3737381539
########## 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: Fixed in 710eace: the test now saves the prior `AVRO_MAX_DECODE_DEPTH` value and restores it (only deleting when it was previously unset). ########## 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: Fixed in 710eace: `_nested_read()` now reads `_max_decode_depth()` once into a local and reuses it for both the check and the error message. ########## 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: Good catch. Guarded the structural skip path in 710eace: `skip_data` now wraps `skip_array`/`skip_map`/`skip_union`/`skip_record` in `_nested_read()`, so skipping a writer-only field for a recursive schema during resolution is bounded too. Added a regression test that a deeply nested payload is rejected when skipped. -- 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]
