Self-referential attribute value crashes the instrumented application with RecursionError

.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

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():

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)

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(

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()