diff --git a/.changelog/5562.fixed b/.changelog/5562.fixed new file mode 100644 index 00000000000..8bbb6c4388b --- /dev/null +++ b/.changelog/5562.fixed @@ -0,0 +1 @@ +`opentelemetry-api`: replace an attribute value nested more than 100 levels deep, including a self-referential one, with `None` instead of raising `RecursionError` into the caller diff --git a/opentelemetry-api/src/opentelemetry/attributes/__init__.py b/opentelemetry-api/src/opentelemetry/attributes/__init__.py index 2ba2d393200..c8cce95ebe9 100644 --- a/opentelemetry-api/src/opentelemetry/attributes/__init__.py +++ b/opentelemetry-api/src/opentelemetry/attributes/__init__.py @@ -16,6 +16,22 @@ _logger = logging.getLogger(__name__) +# Attribute values may be arbitrarily nested Sequences and Mappings, and +# cleaning them recurses. Cap the depth: without it a self-referential value +# raises RecursionError out of set_attribute()/Resource.create()/Counter.add() +# and takes down the instrumented application. +_MAX_NESTING_DEPTH = 100 + + +class _AttributeValueTooDeepError(Exception): + """Internal signal that a value exceeded `_MAX_NESTING_DEPTH`. + + Raised by the recursive worker and caught at the boundary of a single + attribute value, so one unusable value becomes None without discarding the + attributes beside it. + """ + + # Calling str(x) will use an object's `__str__` method if it exists, otherwise it will use it's `__repr__` method. # If neither is defined it uses the base class's `object.__repr__` method, which returns a string that is hard to understand. @@ -24,18 +40,44 @@ def _is_non_custom_str(key: Any) -> bool: return type(key).__str__ is not object.__str__ or type(key).__repr__ is not object.__repr__ -@overload -def _clean_attribute_value( - value: Mapping[str, types.AnyValue], - max_string_value_length: int | None, -) -> Mapping[str, types.AnyValue]: ... +def _clean_key(key: Any) -> str | None: + """Validate an attribute key, returning None if it must be dropped.""" + if not key: + _logger.warning( + "invalid attribute key `%s`. must be non-empty string. Dropping key from attributes.", + key, + ) + return None + # Spec says to convert unknown types to strings if possible (here and below too). + if not isinstance(key, str): + _logger.warning( + "Invalid type `%s` for attribute key `%s`, must be a str. Key's `__str__/__repr__` method will be called if it exists, otherwise the key/value pair will be dropped.", + type(key), + key, + ) + if _is_non_custom_str(key): + return str(key) + return None + return key -@overload -def _clean_attribute_value( - value: types.AnyValue, +def _clean_attributes( + attributes: Mapping[str, types.AnyValue], max_string_value_length: int | None, -) -> types.AnyValue: ... +) -> dict[str, types.AnyValue]: + """Clean a mapping of attributes, one attribute value at a time. + + Unlike a Mapping nested inside a value, the attributes container itself + survives a bad entry: a value that cannot be represented becomes None + rather than discarding the attributes beside it. + """ + cleaned: dict[str, types.AnyValue] = {} + for key, value in attributes.items(): + cleaned_key = _clean_key(key) + if cleaned_key is None: + continue + cleaned[cleaned_key] = _clean_attribute_value(value, max_string_value_length) + return cleaned def _clean_attribute_value( @@ -47,11 +89,35 @@ def _clean_attribute_value( String values are truncated to max_string_value_length if provided. Anything that isn't of `types.AnyValue`, we attempt to cast to `str`. If this fails, the value is replaced with None. Sequence's are converted to tuples and mappings - are copied into new dicts. + are copied into new dicts. A value nested more than `_MAX_NESTING_DEPTH` + levels deep, including a self-referential one, is replaced with None. Returns: The recursively cleaned AnyValue. """ + try: + return _clean_attribute_value_at_depth(value, max_string_value_length, 0) + except _AttributeValueTooDeepError: + _logger.warning( + "Attribute value is nested more than %d levels deep or is self-referential. Replacing it with None.", + _MAX_NESTING_DEPTH, + ) + return None + + +def _clean_attribute_value_at_depth( + value: types.AnyValue, + max_string_value_length: int | None, + depth: int, +) -> types.AnyValue: + """Worker for `_clean_attribute_value` that tracks the nesting depth. + + Raises `_AttributeValueTooDeepError` past `_MAX_NESTING_DEPTH`. Nothing in + here catches it, so a value with an unrepresentable branch is rejected + whole rather than left as a truncated husk. + """ + if depth > _MAX_NESTING_DEPTH: + raise _AttributeValueTooDeepError if isinstance(value, (NoneType, bool, int, float, bytes)): return value if isinstance(value, str): @@ -63,28 +129,14 @@ def _clean_attribute_value( value = value[:max_string_value_length] return value if isinstance(value, Sequence): - return tuple(_clean_attribute_value(v, max_string_value_length) for v in value) + return tuple(_clean_attribute_value_at_depth(v, max_string_value_length, depth + 1) for v in value) if isinstance(value, Mapping): cleaned_mapping: dict[str, types.AnyValue] = {} for key, val in value.items(): - if not key: - _logger.warning( - "invalid attribute key `%s`. must be non-empty string. Dropping key from attributes.", - key, - ) + cleaned_key = _clean_key(key) + if cleaned_key is None: continue - # Spec says to convert unknown types to strings if possible (here and below too). - if not isinstance(key, str): - _logger.warning( - "Invalid type `%s` for attribute key `%s`, must be a str. Key's `__str__/__repr__` method will be called if it exists, otherwise the key/value pair will be dropped.", - type(key), - key, - ) - if _is_non_custom_str(key): - key = str(key) - else: - continue - cleaned_mapping[key] = _clean_attribute_value(val, max_string_value_length) + cleaned_mapping[cleaned_key] = _clean_attribute_value_at_depth(val, max_string_value_length, depth + 1) return cleaned_mapping if TYPE_CHECKING: assert_never(value) @@ -191,7 +243,7 @@ def _set_items(self, attributes: Mapping[str, types.AnyValue]) -> None: with self._lock: self.dropped += len(attributes) return - cleaned_attributes: Mapping[str, types.AnyValue] = _clean_attribute_value(attributes, self.max_value_len) + cleaned_attributes: Mapping[str, types.AnyValue] = _clean_attributes(attributes, self.max_value_len) with self._lock: self.dropped += len(attributes) - len(cleaned_attributes) for key, value in cleaned_attributes.items(): diff --git a/opentelemetry-api/tests/attributes/test_attributes.py b/opentelemetry-api/tests/attributes/test_attributes.py index 29e9826481d..992166b2f82 100644 --- a/opentelemetry-api/tests/attributes/test_attributes.py +++ b/opentelemetry-api/tests/attributes/test_attributes.py @@ -257,3 +257,56 @@ def test_deepcopy_preserves_immutability(self): with self.assertRaises(TypeError): bdict_copy["invalid"] = "invalid" + + +class TestAttributeValueNestingDepth(unittest.TestCase): + """`AnyValue` allows arbitrarily nested Sequence/Mapping values. + + Cleaning them recurses, so a self-referential or pathologically deep value + must be rejected rather than allowed to exhaust the interpreter stack. + Telemetry must never raise into the calling application. + """ + + @staticmethod + def _cyclic_list(): + value = [1, 2] + value.append(value) + return value + + @staticmethod + def _cyclic_dict(): + value = {"a": 1} + value["self"] = value + return value + + @staticmethod + def _deep_list(depth): + value = "x" + for _ in range(depth): + value = [value] + return value + + def test_clean_attribute_value_handles_cyclic_sequence(self): + self.assertIsNone(_clean_attribute_value(self._cyclic_list(), None)) + + def test_clean_attribute_value_handles_cyclic_mapping(self): + self.assertIsNone(_clean_attribute_value(self._cyclic_dict(), None)) + + def test_clean_attribute_value_handles_excessive_depth(self): + self.assertIsNone(_clean_attribute_value(self._deep_list(5000), None)) + + def test_clean_attribute_value_keeps_reasonable_nesting(self): + """Nesting within the limit must still be cleaned normally.""" + self.assertEqual( + _clean_attribute_value([[["a"]]], None), + ((("a",),),), + ) + + def test_bounded_attributes_accepts_cyclic_value(self): + attributes = BoundedAttributes(attributes={"k": self._cyclic_list()}) + self.assertIn("k", attributes) + + def test_bounded_attributes_setitem_accepts_cyclic_value(self): + attributes = BoundedAttributes(immutable=False) + attributes["k"] = self._cyclic_dict() + self.assertIn("k", attributes) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/measurement.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/measurement.py index 4e9d5eea603..6ef73a8a0af 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/measurement.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/measurement.py @@ -8,7 +8,7 @@ from logging import getLogger from typing import TYPE_CHECKING -from opentelemetry.attributes import _clean_attribute_value +from opentelemetry.attributes import _clean_attributes from opentelemetry.context import Context from opentelemetry.util.types import Attributes @@ -44,7 +44,7 @@ def __post_init__(self) -> None: object.__setattr__( self, "attributes", - _clean_attribute_value(self.attributes, None), + _clean_attributes(self.attributes, None), ) else: _logger.warning( diff --git a/opentelemetry-sdk/tests/test_attribute_nesting_depth.py b/opentelemetry-sdk/tests/test_attribute_nesting_depth.py new file mode 100644 index 00000000000..218a900a93f --- /dev/null +++ b/opentelemetry-sdk/tests/test_attribute_nesting_depth.py @@ -0,0 +1,85 @@ +# Copyright The OpenTelemetry Authors +# SPDX-License-Identifier: Apache-2.0 + +"""Telemetry calls must never raise into the instrumented application. + +`AnyValue` allows arbitrarily nested Sequence/Mapping attribute values, so a +user handing the SDK a self-referential structure must not blow the stack. +""" + +import unittest + +from opentelemetry.sdk._logs import LoggerProvider +from opentelemetry.sdk.metrics import MeterProvider +from opentelemetry.sdk.metrics.export import InMemoryMetricReader +from opentelemetry.sdk.resources import Resource +from opentelemetry.sdk.trace import TracerProvider +from opentelemetry.sdk.trace.export import SimpleSpanProcessor +from opentelemetry.sdk.trace.export.in_memory_span_exporter import ( + InMemorySpanExporter, +) + + +def _cyclic_list(): + value = [1, 2] + value.append(value) + return value + + +def _cyclic_dict(): + value = {"a": 1} + value["self"] = value + return value + + +class TestAttributeNestingDepthDoesNotRaise(unittest.TestCase): + def setUp(self): + self.exporter = InMemorySpanExporter() + self.tracer_provider = TracerProvider(shutdown_on_exit=False) + self.tracer_provider.add_span_processor(SimpleSpanProcessor(self.exporter)) + self.tracer = self.tracer_provider.get_tracer(__name__) + + def test_set_attribute_with_cyclic_sequence(self): + with self.tracer.start_as_current_span("span") as span: + span.set_attribute("cyclic", _cyclic_list()) + self.assertIsNone(self.exporter.get_finished_spans()[0].attributes["cyclic"]) + + def test_set_attribute_with_cyclic_mapping(self): + with self.tracer.start_as_current_span("span") as span: + span.set_attribute("cyclic", _cyclic_dict()) + self.assertIsNone(self.exporter.get_finished_spans()[0].attributes["cyclic"]) + + def test_set_attributes_keeps_sibling_attributes(self): + """One unusable value must not discard the attributes beside it.""" + with self.tracer.start_as_current_span("span") as span: + span.set_attributes({"good": "kept", "cyclic": _cyclic_list()}) + attributes = self.exporter.get_finished_spans()[0].attributes + self.assertEqual(attributes["good"], "kept") + self.assertIsNone(attributes["cyclic"]) + + def test_add_event_with_cyclic_value(self): + with self.tracer.start_as_current_span("span") as span: + span.add_event("event", {"cyclic": _cyclic_list()}) + event = self.exporter.get_finished_spans()[0].events[0] + self.assertIsNone(event.attributes["cyclic"]) + + def test_resource_create_with_cyclic_value(self): + resource = Resource.create({"cyclic": _cyclic_list()}) + self.assertIsNone(resource.attributes["cyclic"]) + + def test_counter_add_with_cyclic_value(self): + reader = InMemoryMetricReader() + meter_provider = MeterProvider(metric_readers=[reader]) + meter_provider.get_meter(__name__).create_counter("counter").add(1, {"cyclic": _cyclic_list()}) + metrics_data = reader.get_metrics_data() + data_point = metrics_data.resource_metrics[0].scope_metrics[0].metrics[0].data.data_points[0] + self.assertIsNone(dict(data_point.attributes)["cyclic"]) + meter_provider.shutdown() + + def test_logger_emit_with_cyclic_value(self): + logger_provider = LoggerProvider(shutdown_on_exit=False) + # Must not raise; the record is dropped at the processor, not here. + logger_provider.get_logger(__name__).emit(body="body", attributes={"cyclic": _cyclic_list()}) + + def tearDown(self): + self.tracer_provider.shutdown()