From ac6a4dac60bd614ace85bfed6974bcb91ea238b1 Mon Sep 17 00:00:00 2001 From: Tania Mathern Date: Mon, 24 Aug 2026 21:52:52 -0700 Subject: [PATCH 01/17] fix: Error sentinel --- src/c2pa/c2pa.py | 39 +++++++++----- tests/test_unit_tests.py | 107 ++++++++++++++++++++++++++------------- 2 files changed, 100 insertions(+), 46 deletions(-) diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index 035335c9..5d64bb70 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -502,6 +502,10 @@ def _swap_handle(self, new_handle): # so it is still ours to deal with. _PRE_CONSUME_ERROR_TAGS = ("UntrackedPointer:", "WrongPointerType:") + # Planted right before a consuming call so that a failure which + # sets no error of its own is distinguishable from a stale error. + _NO_NATIVE_ERROR = b"Other: c2pa-python-no-native-error" + def _invoke_consume(self, ffi_call, error_message): """Run an FFI call that consumes this handle, returning its raw result. @@ -521,6 +525,8 @@ def _invoke_consume(self, ffi_call, error_message): ctypes.ArgumentError: If marshalling failed; handle untouched. C2paError: If the call raised any other exception. """ + # Same thread that makes the call, same thread-local slot. + _lib.c2pa_error_set_last(ManagedResource._NO_NATIVE_ERROR) try: return ffi_call(self._handle) except ctypes.ArgumentError: @@ -535,16 +541,13 @@ def _raise_consume_failure(self, error_message): """Raise the error from an FFI handler consuming call. The native error is read before any free so a free's own - pointer-tracking error cannot overwrite it: the native error slot is - sticky and thread-local and the SDK does not clear it before the call, - so this trusts that the failing native path set its own error. - - That ordering is required: - c2pa_free on a handle the registry no longer tracks returns -1 and - overwrites the slot with its own "Other: UntrackedPointer: 0x..." - message. Freeing first would therefore replace the real failure - with another one and, because that substitute carries a pre-consume - tag, invert the retain/consume decision made below. + pointer-tracking error cannot overwrite it. + The native error slot is sticky and thread-local, + and the native SDK does not clear it before the call. + _invoke_consume plants a sentinel here right before a consuming call, + so a failure that sets no error of its own is read back as + the sentinel rather than a stale tag left by an earlier call + on the same pooled thread. Args: error_message: Format string with one placeholder, used when the @@ -564,6 +567,16 @@ def _raise_consume_failure(self, error_message): error) _raise_typed_c2pa_error(error) + if error == ManagedResource._NO_NATIVE_ERROR.decode('utf-8'): + # The planted sentinel survives: + # This failure set no error of its own. Treat as consumed. + logger.debug( + "%s: consuming call failed without setting its own " + "native error; treating as consumed", + type(self).__name__) + self._teardown(free_handle=False) + raise C2paError(error_message.format("Unknown error")) + # A non-tag error means the native side took ownership then failed, # dropping the value itself: mark consumed, do not free (a free here # would be a guarded no-op that dirties the error slot and races a @@ -571,8 +584,9 @@ def _raise_consume_failure(self, error_message): self._teardown(free_handle=False) _raise_typed_c2pa_error(error) - # No error in the slot: ownership is unknown, so free defensively. - self._release_handle() + # No error in the slot at all. Ownership is unknown, so treat as consumed: + # a free here can race a recycled address in other threads. + self._teardown(free_handle=False) raise C2paError(error_message.format("Unknown error")) def _consume_and_swap(self, ffi_call, error_message): @@ -909,6 +923,7 @@ def _setup_function(func, argtypes, restype=None): # Set up function prototypes not attached to an API object _setup_function(_lib.c2pa_version, [], ctypes.c_void_p) _setup_function(_lib.c2pa_error, [], ctypes.c_void_p) +_setup_function(_lib.c2pa_error_set_last, [ctypes.c_char_p], ctypes.c_int) _setup_function(_lib.c2pa_string_free, [ctypes.c_void_p], None) _setup_function( _lib.c2pa_load_settings, [ diff --git a/tests/test_unit_tests.py b/tests/test_unit_tests.py index 4bff6dbb..06ea631b 100644 --- a/tests/test_unit_tests.py +++ b/tests/test_unit_tests.py @@ -50,6 +50,15 @@ ALTERNATIVE_INGREDIENT_TEST_FILE = os.path.join(FIXTURES_DIR, "cloud.jpg") +def _fail_with_native_error(tag_bytes): + """Build a mock FFI callable that sets a native error and returns None. + """ + def _mock(*args): + c2pa_module._lib.c2pa_error_set_last(tag_bytes) + return None + return _mock + + def load_test_settings_json(): """ Load default (legacy) trust configuration test settings from a @@ -8276,12 +8285,11 @@ def test_construction_failure_leaves_nothing_to_free(self): c2pa_module._lib.c2pa_builder_from_json = real_json def test_context_build_null_return_frees_builder(self): - # Set a pre-consume tag in the error slot to mock a pointer rejection. + # Mock a pointer rejection. settings = Settings() - c2pa_module._lib.c2pa_error_set_last( - b"UntrackedPointer: mocked pre-consume rejection") real_build = c2pa_module._lib.c2pa_context_builder_build - c2pa_module._lib.c2pa_context_builder_build = lambda ptr: None + c2pa_module._lib.c2pa_context_builder_build = _fail_with_native_error( + b"UntrackedPointer: mocked pre-consume rejection") try: with self.assertRaises(Error): Context(settings=settings) @@ -8340,6 +8348,39 @@ def test_consume_no_replacement_marks_consumed_on_other_error(self): self.assertIsNone(res._handle) self.assertEqual(res._lifecycle_state, LifecycleState.CLOSED) + def test_invoke_consume_success_does_not_consult_error_slot(self): + """A successful consuming call must not read the error slot at all: + only a failure inspects it.""" + res = self._FakeHandleResource() + res._activate(0xCAFE) + + res._consume_no_replacement(lambda h: 0, "set failed: {}") + + self.assertEqual( + c2pa_module._read_native_error(), + ManagedResource._NO_NATIVE_ERROR.decode('utf-8')) + + def test_consume_no_replacement_retains_on_tag_set_by_the_call_itself(self): + """Only a *stale* tag left over from before the call is the + thing being defended against.""" + res = self._FakeHandleResource() + res._activate(0xCAFE) + + def fake_call(handle): + c2pa_module._lib.c2pa_error_set_last( + b"UntrackedPointer: rejected by the call itself") + return -1 + + with self.assertRaises(Error): + res._consume_no_replacement(fake_call, "set failed: {}") + + # Rejected before ownership transferred: handle retained. + self.assertEqual(res._handle, 0xCAFE) + self.assertEqual(res._lifecycle_state, LifecycleState.ACTIVE) + self.assertEqual(self.freed, []) + res.close() + self.assertEqual(self.freed, [0xCAFE]) + class TestManagedResourceObjects(TestContextAPIs): """Tests native resource handling management when managed manually. @@ -8601,9 +8642,9 @@ def test_builder_with_archive_null_return_marks_consumed(self): # Mimic a non-tag error: native took ownership then failed and dropped # the value itself, so the handle is marked consumed, not freed. - c2pa_module._lib.c2pa_error_set_last(b"Other: mocked test error") real_call = c2pa_module._lib.c2pa_builder_with_archive - c2pa_module._lib.c2pa_builder_with_archive = lambda b, s: None + c2pa_module._lib.c2pa_builder_with_archive = _fail_with_native_error( + b"Other: mocked test error") # Instrument before the failure... freed = self._instrument_frees() @@ -8637,11 +8678,9 @@ def test_reader_with_fragment_null_return_marks_consumed(self): # Mimic a non-tag error: native took ownership then failed and dropped # the value itself, so the handle is marked consumed, not freed. - c2pa_module._lib.c2pa_error_set_last(b"Other: mocked test error") - real_call = c2pa_module._lib.c2pa_reader_with_fragment - c2pa_module._lib.c2pa_reader_with_fragment = ( - lambda r, f, s, frag: None) + c2pa_module._lib.c2pa_reader_with_fragment = _fail_with_native_error( + b"Other: mocked test error") # Instrument before failure so any free would be counted. freed = self._instrument_frees() @@ -8843,10 +8882,9 @@ def test_unknown_failure_drops_handle_without_freeing(self): consumed_handle = reader._handle # Simulate an error being set - c2pa_module._lib.c2pa_error_set_last(b"Other: mocked test error") real_call = c2pa_module._lib.c2pa_reader_with_fragment - c2pa_module._lib.c2pa_reader_with_fragment = ( - lambda r, f, s, frag: None) + c2pa_module._lib.c2pa_reader_with_fragment = _fail_with_native_error( + b"Other: mocked test error") try: with open(init_path, "rb") as init, \ open(fragment_path, "rb") as frag: @@ -9069,22 +9107,29 @@ def test_read_native_error_returns_none_for_an_empty_message(self): finally: c2pa_module._lib.c2pa_error = original - def test_mocked_null_without_error_is_a_known_limitation(self): - # A null with no error of its own is the case that breaks: the slot - # still holds whatever came before. No native path does this, so it - # is pinned here rather than defended in _consume_and_swap. + def test_null_return_with_no_native_error_is_treated_as_consumed(self): + # A null with no error of its own used to be the case that broke: + # the slot still held whatever an unrelated, earlier call on this same + # (pooled) thread left behind, and a stale UntrackedPointer/ + # WrongPointerType tag would make this call believe it still owned a + # handle the native side already dropped. init_path = os.path.join(FIXTURES_DIR, "dashinit.mp4") fragment_path = os.path.join(FIXTURES_DIR, "dash1.m4s") + # A stale, unrelated tag left by a prior call on this thread. c2pa_module._lib.c2pa_error_set_last( b"UntrackedPointer: 0xdeadbeef") with open(init_path, "rb") as init: reader = Reader("video/mp4", init) + consumed_handle = reader._handle real_call = c2pa_module._lib.c2pa_reader_with_fragment + # The fake native call sets no error of its own, + # the planted sentinel _invoke_consume is left in the slot. c2pa_module._lib.c2pa_reader_with_fragment = ( lambda r, f, s, frag: None) + freed = self._instrument_frees() try: with open(init_path, "rb") as init, \ open(fragment_path, "rb") as frag: @@ -9092,16 +9137,12 @@ def test_mocked_null_without_error_is_a_known_limitation(self): reader.with_fragment("video/mp4", init, frag) finally: c2pa_module._lib.c2pa_reader_with_fragment = real_call - # Nothing clears the slot, so a planted tag would follow other - # tests around and change how their failures are classified. - c2pa_module._lib.c2pa_error_set_last( - b"Other: cleared by test teardown") - # The stale tag wins, so the handle is kept. Safe here (the mock - # consumed nothing), and the reader is still usable. - self.assertIsNotNone(reader._handle) - self.assertEqual(reader._lifecycle_state, LifecycleState.ACTIVE) - reader.close() + # The sentinel survived, not the stale tag. + self.assertIsNone(reader._handle) + self.assertEqual(reader._lifecycle_state, LifecycleState.CLOSED) + self.assertEqual(self._free_count(freed, consumed_handle), 0, + "consumed handle was freed instead of marked consumed") # Backfilling a pointer minted by a direct FFI call. Builder.from_archive # is the only production caller of _wrap_native_handle, so these are the @@ -9250,10 +9291,9 @@ def test_consumed_reader_closes_backing_file(self): self.assertFalse(backing_file.closed) # Simulate an error being set - c2pa_module._lib.c2pa_error_set_last(b"Other: mocked test error") real_call = c2pa_module._lib.c2pa_reader_with_fragment - c2pa_module._lib.c2pa_reader_with_fragment = ( - lambda r, f, s, frag: None) + c2pa_module._lib.c2pa_reader_with_fragment = _fail_with_native_error( + b"Other: mocked test error") try: with open(DEFAULT_TEST_FILE, "rb") as main, \ open(DEFAULT_TEST_FILE, "rb") as frag: @@ -9272,9 +9312,9 @@ def test_consumed_builder_releases_context(self): archive = self._make_archive() # Simulate an error being set - c2pa_module._lib.c2pa_error_set_last(b"Other: mocked test error") real_call = c2pa_module._lib.c2pa_builder_with_archive - c2pa_module._lib.c2pa_builder_with_archive = lambda b, s: None + c2pa_module._lib.c2pa_builder_with_archive = _fail_with_native_error( + b"Other: mocked test error") try: with self.assertRaises(Error): builder.with_archive(archive) @@ -9321,10 +9361,9 @@ def test_consumed_reader_clears_caches(self): self.assertIsNotNone(reader._manifest_json_str_cache) # Simulate an error being set - c2pa_module._lib.c2pa_error_set_last(b"Other: mocked test error") real_call = c2pa_module._lib.c2pa_reader_with_fragment - c2pa_module._lib.c2pa_reader_with_fragment = ( - lambda r, f, s, frag: None) + c2pa_module._lib.c2pa_reader_with_fragment = _fail_with_native_error( + b"Other: mocked test error") try: with open(DEFAULT_TEST_FILE, "rb") as main, \ open(DEFAULT_TEST_FILE, "rb") as frag: From f64af884fceb4c3fb176307a429cbc74749e7c4e Mon Sep 17 00:00:00 2001 From: Tania Mathern Date: Tue, 25 Aug 2026 15:46:02 -0700 Subject: [PATCH 02/17] fix: Error slot handling --- docs/native-resources-management.md | 11 +- src/c2pa/c2pa.py | 339 ++++++++++++++++++---------- tests/test_unit_tests.py | 117 ++++++++++ tests/test_unit_tests_threaded.py | 100 +++++--- 4 files changed, 420 insertions(+), 147 deletions(-) diff --git a/docs/native-resources-management.md b/docs/native-resources-management.md index 1cf057f0..81859568 100644 --- a/docs/native-resources-management.md +++ b/docs/native-resources-management.md @@ -456,7 +456,16 @@ Always calling the guarded free instead, even where the value is known to be gon `_release_handle()` (a guarded free) is reserved for the two branches where ownership is not known for certain: a Python exception raised before native reports anything, and a failure that leaves the error slot empty (which no defined native failure is expected to produce). In both, a guarded free is a good default, since it is a real free when the handle is still ours and a `-1` no-op when the native side already took it. -None of this is protected by a lock on the Python side: `ManagedResource` has no thread-safety mechanism of its own, and the retained-vs-consumed guarantee comes entirely from the native pointer registry and its thread-local error slot. As noted under [Which double-free risks this layer guards](#double-free-risk-mitigations), sharing one instance across threads without external synchronization is the caller's responsibility. This is a different hazard from [Fork safety](#fork-safety), which concerns a forked child process, not a thread within the same process. +The retained-vs-consumed guarantee above assumes the sentinel and the tag it distinguishes are still there to read: nothing else must write to the same thread-local slot between `_invoke_consume` planting it and `_raise_consume_failure` reading it back. That assumption used to be silent. In CPython an object's `__del__` runs synchronously the instant its refcount hits zero -- no other thread, no `gc.collect()`, needed -- so if some *unrelated* `ManagedResource` (a Stream wrapper, a Signer, a temporary argument) had its last reference dropped anywhere in that window, its own `_teardown` could call `c2pa_free`, which can call `c2pa_error_set_last` on the exact slot the current call is about to read. A stray `UntrackedPointer:` tag from that unrelated free would then be misread as this call's own pre-consume rejection. + +This is closed with two gates sharing one deferred-teardown slot (`_pending_teardown`) rather than by inspecting error content after the fact: + +- `_inflight` (added for a separate crash: a resource closed from one thread while another thread is still using its handle in a native call) blocks a resource's *own* teardown while its handle is in use. +- `_native_section()` (module-level, `threading.local()`-scoped) additionally blocks *any* resource's teardown on a thread that is between an FFI call and reading back its error, regardless of whose handle it is. `_lock()` and `_native_call()` both open this section for their duration, which is why the same wrapping sigsev-sigabort already introduced at nearly every consuming-call and `_check_ffi_operation_result` site (`docs/native-resources-management.md` cross-reference: see `_native_call`'s docstring) covers both hazards at once. + +`_teardown` defers whenever either gate is up, and whichever gate clears last calls `_maybe_flush_pending()`, which re-checks both before actually freeing. A resource's own bookkeeping (`_teardown`, `_native_call`, `_maybe_flush_pending`) uses a separate raw accessor, `_state_lock()`, precisely so it does not itself open a section -- if it did, every teardown would see itself as "inside a section" and nothing would ever free. One known, accepted limit of the section gate: it cannot tell a genuine pre-consume rejection from a stray free whose freed address happens to coincide with the handle under test (an immediately-reused allocation). That narrower residual mirrors the recycled-address risk described above and is tracked separately, not solved by this mechanism. + +None of the double-free protection above extends to unsynchronized concurrent use of the *same* Python object beyond what `_op_lock` and the two gates provide for teardown-vs-native-call ordering: as noted under [Which double-free risks this layer guards](#double-free-risk-mitigations), sharing one instance across threads still calls for care from the caller. This is a different hazard from [Fork safety](#fork-safety), which concerns a forked child process, not a thread within the same process. A consuming C FFI function first removes the pointer from its registry, then reconstructs the owned value from it. `untrack_or_return!` runs ahead of `Box::from_raw` in `c2pa_c_ffi`. If the address is unknown or the wrong type, the untrack step fails before ownership is taken and sets an error whose prefix (`UntrackedPointer:` or `WrongPointerType:`) identifies it as a pre-consume rejection. Once the value has been reconstructed, a later failure simply drops it, the same as any owned value going out of scope. The Python side stays defensive (and as generic as possible) rather than assuming any exact behavior: it retains the handle when it recognizes one of those rejection prefixes, and where the outcome is unclear it falls back to the guarded free. A native side that behaved differently would degrade in one of two bounded ways: If it kept a pointer the Python side treated as consumed, nothing would free that pointer and it would leak. If it had already released a pointer the Python side then tried to free, the registry would not find the address and the free would return `-1` without touching memory. diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index 5d64bb70..620f2daf 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -271,8 +271,9 @@ def __init__(self): self._pending_teardown = None record_owner_pid(self) - def _lock(self): - """Return this resource's operation lock. + def _state_lock(self): + """Return this resource's raw operation lock, with no side effects + beyond mutual exclusion. Reentrant because it is possible to run a finalizer at any bytecode boundary, including inside a region this thread has already locked, @@ -280,12 +281,7 @@ def _lock(self): locked region. Falls back to a fresh lock when the attribute is missing. - - Never hold this across a native call that drives stream callbacks - (construction, resource_to_stream, the Builder stream methods, - signing). Those calls release the GIL and re-enter caller-supplied - Python, which may call back into this API on another thread. - Only calls that touch no callbacks are serialized here. + Unlike _lock(), this does not open a native-error section. """ lock = getattr(self, '_op_lock', None) if lock is None: @@ -296,6 +292,19 @@ def _lock(self): pass return lock + @contextlib.contextmanager + def _lock(self): + """Hold this resource's operation lock its duration, + and mark this thread as inside a native-error section. + + Never hold this across a native call that drives stream callbacks. + Those calls release the GIL and re-enter caller-supplied + code, which may call back into this API on another thread. + Only calls that touch no callbacks are serialized here. + """ + with self._state_lock(), _native_section(): + yield + @contextlib.contextmanager def _native_call(self): """Hold the handle valid across a native call that goes back @@ -309,25 +318,22 @@ def _native_call(self): The resource is marked closed as soon as the teardown is recorded, so a caller that closed it cannot keep using it while the free is pending. + + Also opens a native-error section around the yielded body (see + _lock()): the in-flight guard alone only protects this resource's + own handle, not the shared thread-local error slot a caller inside + the block is about to read. """ - with self._lock(): + with self._state_lock(): self._ensure_valid_state() self._inflight = getattr(self, '_inflight', 0) + 1 try: - yield + with _native_section(): + yield finally: - with self._lock(): + with self._state_lock(): self._inflight -= 1 - pending = (self._pending_teardown - if self._inflight == 0 else None) - if pending is not None: - self._pending_teardown = None - # Released the lock before the free: - # _teardown takes it again, and keeping the two acquisitions - # separate means the counter update is never held across - # the release work. - if pending is not None: - self._teardown(pending) + self._maybe_flush_pending() @staticmethod def _free_native_ptr(ptr): @@ -387,33 +393,62 @@ def _teardown(self, free_handle: bool): Holds the operation lock so the free cannot happen between another thread's state check and its use of the handle in a native call. + + Deferred (instead of run now) when either gate is blocking: + - this resource's own handle is in flight in a native call + - this thread is inside a native-error section for some call, + that may access the native error slot. """ - with self._lock(): - if getattr(self, '_inflight', 0) > 0: - # A native call is running that re-enters caller Python and - # is still using this handle. Record the intent and whichever - # caller leaves _native_call last performs the free. - # Mark the resource closed now so it cannot be used - # while the free is pending. + with self._state_lock(): + if getattr(self, '_inflight', 0) > 0 or _in_native_section(): + # Mark the resource closed now so it cannot be used while + # the free is pending, but record the intent rather than + # freeing: whichever gate is blocking will call + # _maybe_flush_pending() once it clears. self._pending_teardown = free_handle self._lifecycle_state = LifecycleState.CLOSED + if _in_native_section(): + _register_for_section_flush(self) return - if is_foreign_process(self): - self._handle = None - self._lifecycle_state = LifecycleState.CLOSED - return + self._finish_teardown(free_handle) + + def _finish_teardown(self, free_handle: bool): + """The part of _teardown that only runs once nothing is blocking + teardown. Steps: release, null the handle, free if requested. + Not called directly outside _teardown/_maybe_flush_pending: + callers that want to close a resource still go through _teardown, + which decides whether this can run now or must be deferred. + """ + if is_foreign_process(self): + self._handle = None self._lifecycle_state = LifecycleState.CLOSED - self._safe_release() + return - handle, self._handle = self._handle, None - if free_handle and handle: - try: - ManagedResource._free_native_ptr(handle) - except Exception: - logger.error("Failed to free native %s resources", - type(self).__name__, exc_info=True) + self._lifecycle_state = LifecycleState.CLOSED + self._safe_release() + + handle, self._handle = self._handle, None + if free_handle and handle: + try: + ManagedResource._free_native_ptr(handle) + except Exception: + logger.error("Failed to free native %s resources", + type(self).__name__, exc_info=True) + + def _maybe_flush_pending(self): + """Called when a gate that may have been blocking a deferred + teardown clears (this resource's own _inflight dropping to 0, or + this thread's native-error section closing). + """ + with self._state_lock(): + if self._pending_teardown is None: + return + if getattr(self, '_inflight', 0) > 0 or _in_native_section(): + return + free_handle, self._pending_teardown = self._pending_teardown, None + self._finish_teardown(free_handle) def _release_handle(self): """Free this handle, then close the object. Used only where ownership is @@ -463,9 +498,11 @@ def _create_and_activate(self, ffi_call, error_message, *, Raises: C2paError: If the pointer fails validation; it is freed first. """ - ptr = ffi_call() + ptr = None try: - _check_ffi_operation_result(ptr, error_message, check=check) + with self._lock(): + ptr = ffi_call() + _check_ffi_operation_result(ptr, error_message, check=check) self._activate(ptr) except Exception: if ptr: @@ -805,6 +842,49 @@ def _read_native_error() -> Optional[str]: return message or None +_native_section_state = threading.local() + + +def _in_native_section() -> bool: + """True while this thread is between an FFI call and reading back the + native error it may have set (see _native_section()).""" + return getattr(_native_section_state, 'depth', 0) > 0 + + +def _register_for_section_flush(resource): + """Record that `resource`'s teardown was deferred only because this + thread's native-error section was open.""" + pending = getattr(_native_section_state, 'pending_resources', None) + if pending is not None: + pending.append(resource) + + +@contextlib.contextmanager +def _native_section(): + """Mark this thread as inside a section where a native call's result is + about to be read back: an error-slot check, or a consuming call's + success/failure classification. + + Reentrant: a call whose own native call triggers another one + recursively (same thread) nests correctly here -- only the outermost + span flushes, so nothing is freed before an inner, still-open span has + finished reading its own error. + """ + state = _native_section_state + depth = getattr(state, 'depth', 0) + state.depth = depth + 1 + if depth == 0: + state.pending_resources = [] + try: + yield + finally: + state.depth -= 1 + if state.depth == 0: + pending, state.pending_resources = state.pending_resources, [] + for resource in pending: + resource._maybe_flush_pending() + + class C2paSignerInfo(ctypes.Structure): """Configuration for a Signer.""" _fields_ = [ @@ -1549,11 +1629,12 @@ def load_settings(settings: Union[str, dict], format: str = "json") -> None: except (AttributeError, UnicodeEncodeError) as e: raise C2paError(f"Failed to encode settings to UTF-8: {e}") - result = _lib.c2pa_load_settings(settings_bytes, format_bytes) - _check_ffi_operation_result( - result, - "Error loading settings", - check=lambda r: r != 0) + with _native_section(): + result = _lib.c2pa_load_settings(settings_bytes, format_bytes) + _check_ffi_operation_result( + result, + "Error loading settings", + check=lambda r: r != 0) class ContextProvider(ABC): @@ -1786,11 +1867,12 @@ def __init__( # a successful build consumes it, so close() is then a no-op. with self._NativeBuilder() as nb: if settings is not None: - _check_ffi_operation_result( - _lib.c2pa_context_builder_set_settings( - nb._handle, settings._c_settings), - "Failed to set settings on Context", - check=lambda r: r != 0) + with nb._lock(): + _check_ffi_operation_result( + _lib.c2pa_context_builder_set_settings( + nb._handle, settings._c_settings), + "Failed to set settings on Context", + check=lambda r: r != 0) if signer is not None: # The signer's in-flight guard: @@ -1811,9 +1893,10 @@ def __init__( "Failed to set signer on Context: {}") self._has_signer = True - context_ptr = nb._consume_into( - lambda h: _lib.c2pa_context_builder_build(h), - "Failed to build Context: {}") + with nb._native_call(): + context_ptr = nb._consume_into( + lambda h: _lib.c2pa_context_builder_build(h), + "Failed to build Context: {}") self._activate(context_ptr) @@ -2742,25 +2825,27 @@ def _init_from_context(self, context, format_or_path, # Consume current reader, # with manifest data and stream (C FFI pattern), # to create a new one (switch out) - self._consume_and_swap( - lambda handle: ( - _lib.c2pa_reader_with_manifest_data_and_stream( - handle, - format_arg, - self._own_stream._stream, - manifest_array, - len(manifest_data), - ) - ), - Reader._ERROR_MESSAGES['reader_error']) + with self._native_call(): + self._consume_and_swap( + lambda handle: ( + _lib.c2pa_reader_with_manifest_data_and_stream( + handle, + format_arg, + self._own_stream._stream, + manifest_array, + len(manifest_data), + ) + ), + Reader._ERROR_MESSAGES['reader_error']) else: # Consume reader with stream - self._consume_and_swap( - lambda handle: _lib.c2pa_reader_with_stream( - handle, format_arg, - self._own_stream._stream, - ), - Reader._ERROR_MESSAGES['reader_error']) + with self._native_call(): + self._consume_and_swap( + lambda handle: _lib.c2pa_reader_with_stream( + handle, format_arg, + self._own_stream._stream, + ), + Reader._ERROR_MESSAGES['reader_error']) except Exception: self._close_streams() raise @@ -3175,10 +3260,12 @@ def from_info(cls, signer_info: C2paSignerInfo) -> 'Signer': Raises: C2paError: If there was an error creating the signer """ - signer_ptr = _lib.c2pa_signer_from_info(ctypes.byref(signer_info)) + with _native_section(): + signer_ptr = _lib.c2pa_signer_from_info(ctypes.byref(signer_info)) - _check_ffi_operation_result( - signer_ptr, "Failed to create signer from configured signer_info") + _check_ffi_operation_result( + signer_ptr, + "Failed to create signer from configured signer_info") try: return cls(signer_ptr) @@ -3301,16 +3388,17 @@ def wrapped_callback( callback_cb = SignerCallback(wrapped_callback) # Create the signer with the wrapped callback - signer_ptr = _lib.c2pa_signer_create( - None, - callback_cb, - alg, - certs_bytes, - tsa_url_bytes - ) + with _native_section(): + signer_ptr = _lib.c2pa_signer_create( + None, + callback_cb, + alg, + certs_bytes, + tsa_url_bytes + ) - _check_ffi_operation_result(signer_ptr, - "Failed to create signer") + _check_ffi_operation_result(signer_ptr, + "Failed to create signer") try: # Create and return the signer instance with the callback @@ -3472,11 +3560,11 @@ def from_archive( stream_obj = Stream(stream) try: - handle = _lib.c2pa_builder_from_archive(stream_obj._stream) + with _native_section(): + handle = _lib.c2pa_builder_from_archive(stream_obj._stream) - _check_ffi_operation_result(handle, - "Failed to create builder from archive" - ) + _check_ffi_operation_result( + handle, "Failed to create builder from archive") try: # A builder from an archive here carries no context. @@ -3555,10 +3643,11 @@ def _init_from_context(self, context, json_str): context.execution_context), Builder._ERROR_MESSAGES['builder_error']) - self._consume_and_swap( - lambda handle: _lib.c2pa_builder_with_definition( - handle, json_str), - Builder._ERROR_MESSAGES['builder_error']) + with self._native_call(): + self._consume_and_swap( + lambda handle: _lib.c2pa_builder_with_definition( + handle, json_str), + Builder._ERROR_MESSAGES['builder_error']) def _init_attrs(self): super()._init_attrs() @@ -3890,9 +3979,10 @@ def _sign_internal( try: # _native_call covers the signing call only. - # The close() below is deliberately outside it, - # so the deferred teardown it records is performed - # on the way out rather than being deferred forever. + # The result check and the close() are deliberately + # outside of it: the check needs its own, later section, + # and close() runs only once that check has read whatever + # error this call set. with self._native_call(): if signer is not None: # Signer needs its own in-flight guard. @@ -3915,18 +4005,25 @@ def _sign_internal( dest_stream._stream, ctypes.byref(manifest_bytes_ptr), ) - # Sign borrows the Builder without taking ownership. - # Closing here ensures resources clean up, - # and single use/single sign done by a Builder. - self.close() except Exception as e: self.close() raise C2paError(f"Error during signing: {e}") from e - _check_ffi_operation_result( - result, - "Error during signing", - check=lambda r: r < 0) + try: + # Own section (the native_call already closed, so its + # own reads are done): close() can free this Builder, + # and freeing can write to the same thread-local error slot + # this check reads. + with _native_section(): + _check_ffi_operation_result( + result, + "Error during signing", + check=lambda r: r < 0) + finally: + # Sign borrows the Builder without taking ownership. + # Closing here ensures resources clean up, and single + # use/single sign done by a Builder. + self.close() # Capture the manifest bytes if available manifest_bytes = b"" @@ -4158,17 +4255,18 @@ def format_embeddable(format: str, manifest_bytes: bytes) -> tuple[int, bytes]: ) result_bytes_ptr = ctypes.POINTER(ctypes.c_ubyte)() - result = _lib.c2pa_format_embeddable( - format_str, - manifest_array, - len(manifest_bytes), - ctypes.byref(result_bytes_ptr) - ) + with _native_section(): + result = _lib.c2pa_format_embeddable( + format_str, + manifest_array, + len(manifest_bytes), + ctypes.byref(result_bytes_ptr) + ) - _check_ffi_operation_result( - result, - "Failed to format embeddable manifest", - check=lambda r: r < 0) + _check_ffi_operation_result( + result, + "Failed to format embeddable manifest", + check=lambda r: r < 0) size = result try: @@ -4290,14 +4388,15 @@ def ed25519_sign(data: bytes, private_key: str) -> bytes: f"Invalid UTF-8 characters in private key: {str(e)}") # Perform the signing operation - signature_ptr = _lib.c2pa_ed25519_sign( - data_array, - data_size, - key_bytes - ) + with _native_section(): + signature_ptr = _lib.c2pa_ed25519_sign( + data_array, + data_size, + key_bytes + ) - _check_ffi_operation_result(signature_ptr, - "Failed to sign data with Ed25519") + _check_ffi_operation_result(signature_ptr, + "Failed to sign data with Ed25519") try: # Ed25519 signatures are always 64 bytes diff --git a/tests/test_unit_tests.py b/tests/test_unit_tests.py index 06ea631b..ea98da81 100644 --- a/tests/test_unit_tests.py +++ b/tests/test_unit_tests.py @@ -8381,6 +8381,123 @@ def fake_call(handle): res.close() self.assertEqual(self.freed, [0xCAFE]) + def test_native_section_defers_unrelated_finalizer_free(self): + """A finalizer for a completely unrelated resource firing mid + native-call must not free immediately. + """ + victim = self._FakeHandleResource() + victim._activate(0xCAFE) + bystander = self._FakeHandleResource() + bystander._activate(0xB00B) + + def clobbering_free(ptr): + self.freed.append(ptr) + # Stands in for c2pa_free's real behavior: freeing an + # untracked/foreign pointer writes its own error into the + # same thread-local slot. + c2pa_module._lib.c2pa_error_set_last( + "Other: UntrackedPointer: {:#x}".format(ptr).encode()) + return -1 + ManagedResource._free_native_ptr = staticmethod(clobbering_free) + + def ffi_call(handle): + nonlocal bystander + del bystander # last reference dropped: __del__ fires right here + return None # the real call failed but set no error of its own + + with victim._native_call(): + with self.assertRaises(Error): + victim._consume_no_replacement(ffi_call, "op failed: {}") + + self.assertIsNone( + victim._handle, + "victim was wrongly retained: bystander's deferred free still " + "clobbered the sentinel before it was read") + self.assertEqual(victim._lifecycle_state, LifecycleState.CLOSED) + self.assertEqual(self.freed, [0xB00B], + "bystander's deferred free did not run exactly once") + + def test_teardown_deferred_by_own_inflight_and_section_together(self): + """A resource blocked by its own handle being in-flight, + and a wholly separate native-error section is also open on this thread + must not free until both clear, and must free exactly once.""" + res = self._FakeHandleResource() + res._activate(0xCAFE) + + call_cm = res._native_call() + call_cm.__enter__() + try: + section_cm = c2pa_module._native_section() + section_cm.__enter__() + try: + res.close() + self.assertEqual(res._lifecycle_state, LifecycleState.CLOSED) + self.assertEqual(self.freed, [], + "freed while still in flight") + finally: + section_cm.__exit__(None, None, None) + # The independent section closed, but res's own in-flight + # guard is still up: still not freed. + self.assertEqual(self.freed, [], + "flushed while the in-flight guard still held") + finally: + call_cm.__exit__(None, None, None) + # Both gates clear only once native_call's own exit drops inflight + # to 0 -- that is what should finally trigger the free. + self.assertEqual(self.freed, [0xCAFE]) + + def test_nested_native_sections_flush_only_at_outermost_close(self): + """A native-error section opened inside another, already-open one + on the same thread must not flush anything until the outermost + one closes.""" + res = self._FakeHandleResource() + res._activate(0xCAFE) + + outer = c2pa_module._native_section() + outer.__enter__() + try: + inner = c2pa_module._native_section() + inner.__enter__() + try: + res.close() + self.assertEqual(self.freed, []) + finally: + inner.__exit__(None, None, None) + # Inner closed, outer is still open: still deferred. + self.assertEqual(self.freed, [], + "inner section flushed before the outer closed") + finally: + outer.__exit__(None, None, None) + self.assertEqual(self.freed, [0xCAFE]) + + def test_native_section_flush_isolates_exceptions(self): + """One deferred free raising during a section's flush must not + stop the rest of that flush from running.""" + good = self._FakeHandleResource() + good._activate(0xC0FFEE) + bad = self._FakeHandleResource() + bad._activate(0xBAD) + + def flaky_free(ptr): + if ptr == 0xBAD: + raise RuntimeError("simulated free failure") + self.freed.append(ptr) + return 0 + ManagedResource._free_native_ptr = staticmethod(flaky_free) + + with self.assertLogs('c2pa', level='ERROR') as captured: + with c2pa_module._native_section(): + bad.close() + good.close() + + self.assertEqual(self.freed, [0xC0FFEE], + "a failing deferred free stopped the rest") + self.assertTrue( + any('Failed to free native' in line + for line in captured.output), + "the failing deferred free was not logged: " + "{}".format(captured.output)) + class TestManagedResourceObjects(TestContextAPIs): """Tests native resource handling management when managed manually. diff --git a/tests/test_unit_tests_threaded.py b/tests/test_unit_tests_threaded.py index 5cbccea2..16016145 100644 --- a/tests/test_unit_tests_threaded.py +++ b/tests/test_unit_tests_threaded.py @@ -32,7 +32,7 @@ from c2pa import Builder, C2paError as Error, Reader, C2paSigningAlg as SigningAlg, C2paSignerInfo, Signer, sdk_version # noqa: E501 from c2pa import Context, Settings -from c2pa.c2pa import ManagedResource, Stream, LifecycleState +from c2pa.c2pa import ManagedResource, Stream, LifecycleState, _native_section from c2pa.lib import is_foreign_process, record_owner_pid PROJECT_PATH = os.getcwd() @@ -3375,31 +3375,35 @@ def test_no_nested_op_locks(self): held = threading.local() violations = [] real_lock = ManagedResource._lock - - def tracking_lock(resource): - lock = real_lock(resource) - depth = getattr(held, 'stack', None) - if depth is None: - depth = held.stack = [] - - class Tracked: - def __enter__(self): - others = [r for r in depth if r is not resource] - if others: - violations.append( - "{} while holding {}".format( - type(resource).__name__, - [type(o).__name__ for o in others])) - depth.append(resource) - return lock.__enter__() - - def __exit__(self, *exc): - depth.pop() - return lock.__exit__(*exc) - - return Tracked() - - ManagedResource._lock = tracking_lock + real_state_lock = ManagedResource._state_lock + + def make_tracking(real): + def tracking(resource): + lock = real(resource) + depth = getattr(held, 'stack', None) + if depth is None: + depth = held.stack = [] + + class Tracked: + def __enter__(self): + others = [r for r in depth if r is not resource] + if others: + violations.append( + "{} while holding {}".format( + type(resource).__name__, + [type(o).__name__ for o in others])) + depth.append(resource) + return lock.__enter__() + + def __exit__(self, *exc): + depth.pop() + return lock.__exit__(*exc) + + return Tracked() + return tracking + + ManagedResource._lock = make_tracking(real_lock) + ManagedResource._state_lock = make_tracking(real_state_lock) try: reader = Reader("image/jpeg", io.BytesIO(data)) reader.json() @@ -3409,6 +3413,7 @@ def __exit__(self, *exc): reader.close() finally: ManagedResource._lock = real_lock + ManagedResource._state_lock = real_state_lock self.assertEqual(violations, [], "a thread held two operation locks at once") @@ -3450,6 +3455,49 @@ def closer_worker(): self._join_all(threads, "concurrent storm") self.assertEqual(errors, []) + def test_native_section_deferred_free_is_thread_local(self): + """Two threads each with their own open native-error section: one + thread's section closing must not flush a free deferred inside + the other thread's still-open section. + """ + freed = self._counted_free() + resource = _ConcreteResource() + resource._activate(0x1001) + + thread_ready = threading.Event() + release_thread = threading.Event() + + def worker(): + with _native_section(): + resource.close() + thread_ready.set() + release_thread.wait(self.JOIN_TIMEOUT) + # Flush happens here, on the worker thread, once its own + # section closes. + + thread = threading.Thread(target=worker) + thread.start() + try: + self.assertTrue( + thread_ready.wait(self.JOIN_TIMEOUT), + "worker thread did not reach its open section in time") + + # A section opened and closed entirely on this (main) thread, + # while the worker's section is still open on its own thread. + with _native_section(): + pass + + self.assertEqual( + freed, [], + "a different thread's section flushed this thread's " + "pending resource") + finally: + release_thread.set() + self._join_all([thread], "native-section worker") + + self.assertEqual(freed, [0x1001], + "worker thread's own section never flushed") + def _counted_free(self): """Patch _free_native_ptr to count frees; returns the list.""" freed = [] From ed506d61a1591cc885f5225b36aed2ede8f6adf9 Mon Sep 17 00:00:00 2001 From: tmathern <60901087+tmathern@users.noreply.github.com> Date: Tue, 25 Aug 2026 17:36:11 -0700 Subject: [PATCH 03/17] fix: Error slots --- src/c2pa/c2pa.py | 67 ++++++++++++--------- tests/test_unit_tests.py | 126 ++++++++++++++++++++++++++++++++------- 2 files changed, 144 insertions(+), 49 deletions(-) diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index 620f2daf..e9c8e1da 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -539,10 +539,6 @@ def _swap_handle(self, new_handle): # so it is still ours to deal with. _PRE_CONSUME_ERROR_TAGS = ("UntrackedPointer:", "WrongPointerType:") - # Planted right before a consuming call so that a failure which - # sets no error of its own is distinguishable from a stale error. - _NO_NATIVE_ERROR = b"Other: c2pa-python-no-native-error" - def _invoke_consume(self, ffi_call, error_message): """Run an FFI call that consumes this handle, returning its raw result. @@ -563,7 +559,7 @@ def _invoke_consume(self, ffi_call, error_message): C2paError: If the call raised any other exception. """ # Same thread that makes the call, same thread-local slot. - _lib.c2pa_error_set_last(ManagedResource._NO_NATIVE_ERROR) + _lib.c2pa_error_set_last(_NO_NATIVE_ERROR) try: return ffi_call(self._handle) except ctypes.ArgumentError: @@ -581,10 +577,9 @@ def _raise_consume_failure(self, error_message): pointer-tracking error cannot overwrite it. The native error slot is sticky and thread-local, and the native SDK does not clear it before the call. - _invoke_consume plants a sentinel here right before a consuming call, - so a failure that sets no error of its own is read back as - the sentinel rather than a stale tag left by an earlier call - on the same pooled thread. + _invoke_consume marks the slot as carrying no error right before a + consuming call, so a failure that sets no error of its own reads back + as no error rather than as a stale one left by an earlier call. Args: error_message: Format string with one placeholder, used when the @@ -604,16 +599,6 @@ def _raise_consume_failure(self, error_message): error) _raise_typed_c2pa_error(error) - if error == ManagedResource._NO_NATIVE_ERROR.decode('utf-8'): - # The planted sentinel survives: - # This failure set no error of its own. Treat as consumed. - logger.debug( - "%s: consuming call failed without setting its own " - "native error; treating as consumed", - type(self).__name__) - self._teardown(free_handle=False) - raise C2paError(error_message.format("Unknown error")) - # A non-tag error means the native side took ownership then failed, # dropping the value itself: mark consumed, do not free (a free here # would be a guarded no-op that dirties the error slot and races a @@ -621,8 +606,14 @@ def _raise_consume_failure(self, error_message): self._teardown(free_handle=False) _raise_typed_c2pa_error(error) - # No error in the slot at all. Ownership is unknown, so treat as consumed: - # a free here can race a recycled address in other threads. + # The call failed without setting an error of its own, + # so ownership is unknown. + # Treat as consumed: a free here can race a recycled address + # in other threads. + logger.debug( + "%s: consuming call failed without setting its own " + "native error; treating as consumed", + type(self).__name__) self._teardown(free_handle=False) raise C2paError(error_message.format("Unknown error")) @@ -821,16 +812,32 @@ class C2paStream(ctypes.Structure): ] +# Written into the native slot to mark it as carrying no error of our own. +# Planted before a consuming call so a failure that sets no error is +# distinguishable from a stale one, and written back after every read so an +# error is reportable only by the caller that observes it. +# _read_native_error() maps it to None, so it never reaches a caller. +_NO_NATIVE_ERROR_MARKER = b"c2pa-python-no-native-error" +_NO_NATIVE_ERROR = b"Other: " + _NO_NATIVE_ERROR_MARKER + + +def _is_no_native_error(message: str) -> bool: + """True for the marker meaning "no error of our own", in either spelling.""" + marker = _NO_NATIVE_ERROR_MARKER.decode('utf-8') + return message == marker or message == f"Other: {marker}" + + def _read_native_error() -> Optional[str]: """Read the last error from the native library, or None if unset. - Peeks: the error stays in the native slot, - until the next error overwrites it. - + The slot is marked as carrying no error before returning, so a + given error is reported once, by the caller that observes it. The native + slot is thread-local and sticky, so a message left in place stays readable + indefinitely and is available to be reported again by a later, + unrelated call that failed without setting an error of its own. With no error set the native side still returns an owned pointer to an empty string, so the pointer alone does not tell us whether there is an - error. Only a non-empty message counts as one; the empty string still - has to be freed. + error. Only a non-empty message counts as one. """ error = _lib.c2pa_error() if not error: @@ -839,7 +846,13 @@ def _read_native_error() -> Optional[str]: message = ctypes.string_at(error).decode('utf-8') finally: _lib.c2pa_string_free(error) - return message or None + if not message: + return None + + _lib.c2pa_error_set_last(_NO_NATIVE_ERROR) + if _is_no_native_error(message): + return None + return message _native_section_state = threading.local() diff --git a/tests/test_unit_tests.py b/tests/test_unit_tests.py index ea98da81..1b172fef 100644 --- a/tests/test_unit_tests.py +++ b/tests/test_unit_tests.py @@ -8356,9 +8356,7 @@ def test_invoke_consume_success_does_not_consult_error_slot(self): res._consume_no_replacement(lambda h: 0, "set failed: {}") - self.assertEqual( - c2pa_module._read_native_error(), - ManagedResource._NO_NATIVE_ERROR.decode('utf-8')) + self.assertIsNone(c2pa_module._read_native_error()) def test_consume_no_replacement_retains_on_tag_set_by_the_call_itself(self): """Only a *stale* tag left over from before the call is the @@ -9112,17 +9110,14 @@ def test_perf_scenario_bogus_handle_is_rejected(self): reader.close() def test_every_null_return_sets_its_own_error(self): - # Reading the slot without clearing it is only sound because every - # null return sets an error. Check each path reports its own. + # Each null-returning path must report the error it set itself, never + # one left behind by an earlier call. init_path = os.path.join(FIXTURES_DIR, "dashinit.mp4") fragment_path = os.path.join(FIXTURES_DIR, "dash1.m4s") - # Leave a recognisable error behind, so anything stale shows up. - try: - Reader("image/jpeg", io.BytesIO(b"not an image")).json() - except Error: - pass - self.assertIn("NotSupported", c2pa_module._read_native_error() or "") + # Set a recognizable error, so anything stale shows up below. + c2pa_module._lib.c2pa_error_set_last( + b"NotSupported: planted by the test") # Pre-consume rejection: reports UntrackedPointer, not NotSupported. with open(init_path, "rb") as init: @@ -9192,21 +9187,19 @@ def worker(): self.assertEqual(problems, [], "ownership was misjudged under concurrency") - def test_reading_the_native_error_does_not_empty_the_slot(self): - # c2pa_error() peeks, so nothing Python can call empties the slot. - # _consume_and_swap depends on this. - try: - Reader("image/jpeg", io.BytesIO(b"not an image")).json() - except Error: - pass + def test_reading_the_native_error_consumes_it(self): + # c2pa_error() itself peeks, so _read_native_error marks the slot as + # carrying no error once it has read one. + # An error belongs to the caller that observes it; + # leaving it readable lets a later, unrelated failure report it as its own. + c2pa_module._lib.c2pa_error_set_last(b"Io: read me exactly once") first = c2pa_module._read_native_error() self.assertTrue(first, "expected a native error to have been set") - self.assertEqual( - c2pa_module._read_native_error(), first, - "reading emptied the native slot; the comments in " - "_consume_and_swap about a persistent error are now wrong") + self.assertIsNone( + c2pa_module._read_native_error(), + "the native error stayed readable after being reported once") def test_read_native_error_returns_none_for_an_empty_message(self): # c2pa_error() returns an owned pointer to "" when no error is set, @@ -9552,6 +9545,36 @@ def _boom(*args): self.assertIs(ctx.exception.__cause__, sentinel, "signing error dropped the original exception") + def test_sign_reports_the_native_error_it_set(self): + """sign() reads its error in a later section than the call itself. + The signing call runs inside one _native_call() block and the result + check runs in a separate _native_section() afterwards, so anything + that marks the slot as carrying no error on section exit would discard + the real message between the two. + """ + builder = Builder(self.test_manifest) + signer = self._ctx_make_signer() + self.addCleanup(signer.close) + + real_sign = c2pa_module._lib.c2pa_builder_sign + + def _fail(*args): + c2pa_module._lib.c2pa_error_set_last( + b"Signature: native signing refused") + return -1 + + c2pa_module._lib.c2pa_builder_sign = _fail + try: + with self.assertRaises(Error) as ctx: + builder.sign(signer, "image/jpeg", + io.BytesIO(b"x"), io.BytesIO()) + finally: + c2pa_module._lib.c2pa_builder_sign = real_sign + + self.assertIn("native signing refused", str(ctx.exception), + "the native signing error was lost before it was read") + self.assertIsInstance(ctx.exception, Error.Signature) + class TestErrorPlumbing(unittest.TestCase): """Covers the error helpers themselves, which had no direct tests.""" @@ -9683,6 +9706,65 @@ def test_supported_mime_types_reports_the_native_message(self): c2pa_module._get_supported_mime_types(lambda count: None, None) self.assertIn("mime lookup failed", str(ctx.exception)) + def test_reading_an_error_does_not_leave_it_readable(self): + """An error is reportable once, by the reader that observes it. + """ + self._set_native_error("Io: read me once") + + self.assertEqual( + c2pa_module._read_native_error(), "Io: read me once") + self.assertIsNone( + c2pa_module._read_native_error(), + "the same native error was reported a second time") + + def test_handled_error_does_not_survive_later_operations(self): + """A caught failure must not leave its error in-place + (tests the slot is cleaned up). + """ + with self.assertRaises(Error): + Reader("image/jpeg", io.BytesIO(b"not an image")) + + for _ in range(20): + c2pa_module.Stream(io.BytesIO(b"x")) + + self.assertIsNone( + c2pa_module._read_native_error(), + "a handled error was still resident after 20 successful calls") + + def test_later_failure_does_not_inherit_a_handled_errors_type(self): + """A failure with no error of its own must not see an older one. + """ + with self.assertRaises(Error) as first: + Reader("image/jpeg", io.BytesIO(b"not an image")) + self.assertIsInstance(first.exception, Error.NotSupported) + + with self.assertRaises(Error) as second: + c2pa_module._check_ffi_operation_result( + None, "Later unrelated failure: {}") + + self.assertNotIsInstance( + second.exception, Error.NotSupported, + "the later failure inherited the handled error's type") + self.assertIn("Unknown error", str(second.exception)) + self.assertNotIn( + "type is unsupported", str(second.exception), + "the later failure reported the handled error's message") + + def test_the_no_native_error_sentinel_never_reaches_a_caller(self): + """The sentinel is an internal marker, not a message for users.""" + sentinel = c2pa_module._NO_NATIVE_ERROR.decode("utf-8") + + c2pa_module._lib.c2pa_error_set_last(c2pa_module._NO_NATIVE_ERROR) + self.assertIsNone( + c2pa_module._read_native_error(), + "the sentinel was reported as if it were a native error") + + c2pa_module._lib.c2pa_error_set_last(c2pa_module._NO_NATIVE_ERROR) + with self.assertRaises(Error) as ctx: + c2pa_module._check_ffi_operation_result(None, "fallback: {}") + self.assertNotIn(sentinel, str(ctx.exception)) + self.assertIn("Unknown error", str(ctx.exception)) + class TestErrorsStillRaiseAfterCleanup(unittest.TestCase): """Each surface that lost a _clear_error_state() call still reports.""" From 9525b24dbd7998fa6a346b2d391c8e9afacc37ac Mon Sep 17 00:00:00 2001 From: tmathern <60901087+tmathern@users.noreply.github.com> Date: Tue, 25 Aug 2026 22:18:42 -0700 Subject: [PATCH 04/17] fix: Merge commit --- src/c2pa/c2pa.py | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index 08b28ac1..ef2c2946 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -410,6 +410,11 @@ def _teardown(self, free_handle: bool): Holds the operation lock so the free cannot happen between another thread's state check and its use of the handle in a native call. + Deferred (instead of run now) when either gate is blocking: + - this resource's own handle is in flight in a native call + - this thread is inside a native-error section for some call, + that may access the native error slot. + The forked-child case is handled before the lock is taken, because _lock() refuses in a child: this path has to finish rather than report an error, so it cannot rely on acquiring. @@ -422,13 +427,11 @@ def _teardown(self, free_handle: bool): return with self._state_lock(): - if getattr(self, '_inflight', 0) > 0: - # A native call is running that re-enters calling non-native code - # and is still using this handle. - # Record the intent and whichever caller leaves - # _native_call last performs the free. - # Mark the resource closed now so it cannot be used - # while the free is pending. + if getattr(self, '_inflight', 0) > 0 or _in_native_section(): + # Mark the resource closed now so it cannot be used while + # the free is pending, but record the intent: + # whichever check is blocking will call + # _maybe_flush_pending() once it clears. self._pending_teardown = free_handle self._lifecycle_state = LifecycleState.CLOSED if _in_native_section(): From 9ece3d6dc0da6babbbd48495c54809b3e2f271bc Mon Sep 17 00:00:00 2001 From: tmathern <60901087+tmathern@users.noreply.github.com> Date: Tue, 25 Aug 2026 22:50:07 -0700 Subject: [PATCH 05/17] fix: Set an error as sentinel --- PLAN-error-slot-marker-via-c2pa-free.md | 319 ++++++++++++++++++++++++ src/c2pa/c2pa.py | 80 ++++-- tests/test_unit_tests.py | 48 +++- 3 files changed, 427 insertions(+), 20 deletions(-) create mode 100644 PLAN-error-slot-marker-via-c2pa-free.md diff --git a/PLAN-error-slot-marker-via-c2pa-free.md b/PLAN-error-slot-marker-via-c2pa-free.md new file mode 100644 index 00000000..c0467b5e --- /dev/null +++ b/PLAN-error-slot-marker-via-c2pa-free.md @@ -0,0 +1,319 @@ +# Plan: plant the error-slot marker via c2pa_free, drop the c2pa_error_set_last runtime dependency + +Implementation handoff. All facts below were verified against the current +checkout of this branch (`mathern/error-slot-sentinel`, merge commit +`f84c088` plus the `_teardown` gate restoration) and against the c2pa-rs +sources in `../c2pa-rs`. Line numbers refer to the current state of +`src/c2pa/c2pa.py`; re-grep before editing if the file has moved. + +## Why + +The error-slot fix on this branch plants a marker into the native +thread-local error slot before every consuming call, and re-plants it after +every read, so a stale message left by an earlier call on the same pooled +thread is never misread as the current failure's error. That marker decides +whether a failed consuming call retains or consumes the native handle, so a +stale read can cause a wrong free decision. + +Planting currently uses `c2pa_error_set_last`, an export added to c2pa-rs +for this purpose. Any native build that predates the export cannot load this +module (the unconditional prototype setup raises `AttributeError` at import). + +Planting cannot be avoided altogether: the slot is a single sticky +thread-local cell, `c2pa_error()` only peeks, and no export clears it. +Detecting "this call wrote nothing" requires starting from a state no +genuine call can produce, and creating that state is planting. What can be +avoided is the new-export dependency: + +`c2pa_free` on an address the registry does not track deterministically +writes an error into the same slot. Verified in c2pa-rs: +`c2pa_c_ffi/src/c_api.rs:995` routes to `cimpl_free` +(`c2pa_c_ffi/src/cimpl/utils.rs:320-345`), and a registry miss executes +`CimplError::untracked_pointer(ptr).set_last()`, whose message is +`format!("UntrackedPointer: 0x{:x}", ptr)` (`cimpl/cimpl_error.rs:101-103`). +So `_lib.c2pa_free(1)` plants a fixed, known text using only exports every +shipped native lib already has (`c2pa_free`, `c2pa_error`). Address 1 is +never a real handle: heap allocations are aligned, and the Python layer only +ever passes real handles or this constant, so the planted text cannot +collide with a genuine error about a real pointer. + +The exact wire text (with or without an `"Other: "` prefix, exact hex casing) +is a native implementation detail, so the module learns it once at import by +planting and reading back, instead of hardcoding it. + +## Changes to src/c2pa/c2pa.py + +### 1. Marker constants and helpers (replace lines 852-856) + +Delete: + +```python +_NO_NATIVE_ERROR_MARKER = b"c2pa-python-no-native-error" +_NO_NATIVE_ERROR = b"Other: " + _NO_NATIVE_ERROR_MARKER + + +def _is_no_native_error(message: str) -> bool: + """True for the marker meaning "no error of our own", in either spelling.""" + marker = _NO_NATIVE_ERROR_MARKER.decode('utf-8') + return message == marker or message == f"Other: {marker}" +``` + +Replace with: + +```python +# Address deliberately passed to c2pa_free to plant a marker in the native +# error slot. Never a real handle: allocations are aligned, and the Python +# layer only passes real handles or this constant to c2pa_free. +_MARKER_ADDR = 1 + +# Exact text the native lib writes for a failed free of _MARKER_ADDR. +# Learned at import by _learn_no_error_text(); the format is a native +# implementation detail, so it is read back rather than hardcoded. +_NO_NATIVE_ERROR_TEXT = None + + +def _plant_no_error_marker(): + """Write the no-error marker into this thread's native error slot. + + A c2pa_free of an address the registry does not track writes + "UntrackedPointer: 0x1" (learned exactly at import) into the + thread-local error slot and returns -1, which is expected here. + Calls _lib.c2pa_free directly: _free_native_ptr would log each plant. + """ + _lib.c2pa_free(_MARKER_ADDR) + + +def _is_no_native_error(message: str) -> bool: + """True for the planted marker meaning "no error of our own".""" + return message == _NO_NATIVE_ERROR_TEXT +``` + +### 2. Import-time learning (new code, placed immediately after line 1254's `_setup_function(_lib.c2pa_free, [ctypes.c_void_p], ctypes.c_int)`) + +The learning must run after the prototypes for `c2pa_free`, `c2pa_error`, +and `c2pa_string_free` are configured. `c2pa_error`/`c2pa_string_free` are +set up at lines 1049-1051; `c2pa_free` at line 1254 is the last of the +three, so the snippet goes right below it: + +```python +def _learn_no_error_text(): + """Plant the marker once and read back the exact text the native lib + produces for it, so equality checks match this build of the lib. + + Runs on the importing thread; the text is a format constant, so the + learned value holds for every thread. Raises at import when the read + back text is empty, because the marker mechanism cannot work then. + """ + _plant_no_error_marker() + raw = _lib.c2pa_error() + if not raw: + raise ImportError( + "c2pa native library did not report an error for a free of " + "an untracked pointer; the error-slot marker cannot work") + try: + text = ctypes.string_at(raw).decode('utf-8') + finally: + _lib.c2pa_string_free(raw) + if not text: + raise ImportError( + "c2pa native library reported an empty error for a free of " + "an untracked pointer; the error-slot marker cannot work") + return text + + +_NO_NATIVE_ERROR_TEXT = _learn_no_error_text() +``` + +Note: `_read_native_error` cannot be reused for learning — it maps the +marker to `None` and replants, and at learning time the marker text is not +yet known. The raw read above is intentional. + +Sanity check to add right after (a one-line assert is fine): the learned +text must contain the hex form of `_MARKER_ADDR` +(`assert "0x1" in _NO_NATIVE_ERROR_TEXT`), so a native change that breaks +the assumption fails loudly at import, not silently at the first consume +failure. + +### 3. Replace both planting call sites + +- Line 594 in `_invoke_consume`: + + ```python + # Same thread that makes the call, same thread-local slot. + _lib.c2pa_error_set_last(_NO_NATIVE_ERROR) + ``` + + becomes + + ```python + # Same thread that makes the call, same thread-local slot. + _plant_no_error_marker() + ``` + +- Line 884 at the end of `_read_native_error`: + + ```python + _lib.c2pa_error_set_last(_NO_NATIVE_ERROR) + ``` + + becomes + + ```python + _plant_no_error_marker() + ``` + + The docstring of `_read_native_error` (lines 860-869) stays accurate as + written; no change needed there. The comment block above the deleted + constants (lines 844-848) is replaced by the new constants' comments in + change 1. + +### 4. Make the c2pa_error_set_last prototype conditional (line 1051) + +```python +_setup_function(_lib.c2pa_error_set_last, [ctypes.c_char_p], ctypes.c_int) +``` + +becomes + +```python +# Optional: only newer native builds export this. The runtime does not +# call it; tests use it, when present, to simulate native error writes. +if getattr(_lib, 'c2pa_error_set_last', None) is not None: + _setup_function( + _lib.c2pa_error_set_last, [ctypes.c_char_p], ctypes.c_int) +``` + +Caveat for the implementer: `ctypes` raises `AttributeError` on missing +symbols at attribute access, and `getattr` with a default swallows exactly +that. Confirm with the vendored dylib (symbol present) that the guarded +branch still executes. + +### 5. Grep afterward + +`grep -n c2pa_error_set_last src/c2pa/c2pa.py` must show only the guarded +prototype setup from change 4. `grep -n _NO_NATIVE_ERROR src/c2pa/c2pa.py` +must show only `_NO_NATIVE_ERROR_TEXT`. + +## Changes to tests/test_unit_tests.py + +The test file references the old constant and the old mechanism in a few +places. Current anchors: + +- Line 57 (inside the `_fail_with_native_error` mock-builder) and lines + 8368, 8396, 9246, 9322, 9357, 9689, 9710: these use `c2pa_error_set_last` + to *simulate native code writing an error* (stand-ins for what failing + native calls do). They keep using it — the vendored dylib exports the + symbol, and the simulation is test-only. Do not rewrite these. + +- Lines 9880-9894, `test_the_no_native_error_sentinel_never_reaches_a_caller`: + this test plants the marker with + `c2pa_module._lib.c2pa_error_set_last(c2pa_module._NO_NATIVE_ERROR)` + (twice) and derives the leak-check string via + `sentinel = c2pa_module._NO_NATIVE_ERROR.decode("utf-8")`. Rewrite it to: + `sentinel = c2pa_module._NO_NATIVE_ERROR_TEXT` (already a str, no decode) + and replace both plant lines with + `c2pa_module._plant_no_error_marker()`. The assertions themselves + (marker read maps to None; marker text never appears in a raised + message) stay exactly as they are. + +### New tests (add near the existing sentinel tests, same class) + +Test 1 — the plant writes the learned text: + +```python +def test_plant_marker_writes_learned_text(self): + c2pa_module._plant_no_error_marker() + raw = c2pa_module._lib.c2pa_error() + try: + text = ctypes.string_at(raw).decode('utf-8') + finally: + c2pa_module._lib.c2pa_string_free(raw) + self.assertEqual(text, c2pa_module._NO_NATIVE_ERROR_TEXT) +``` + +Test 2 — the planted marker reads back as "no error": + +```python +def test_read_native_error_maps_marker_to_none(self): + c2pa_module._plant_no_error_marker() + self.assertIsNone(c2pa_module._read_native_error()) +``` + +Test 3 — a stale error is not misattributed to a failure that set nothing. +This is the scenario the whole mechanism exists for; it may already be +covered by the existing sentinel tests around line 9884 once they are +switched to the helper — if so, verify that coverage instead of duplicating +it. The shape, if needed: + +```python +def test_stale_error_not_misattributed_after_plant(self): + # A realistic stale tag from an earlier, unrelated call. + c2pa_module._lib.c2pa_error_set_last( + b"Other: UntrackedPointer: 0xdeadbeef") + + res = self._FakeHandleResource() + res._activate(0xCAFE) + + # Fails without setting any error of its own. The plant inside + # _invoke_consume must have cleared the stale tag, so this routes + # to the "no error of our own" branch: consumed, not retained. + with self.assertRaises(Error): + res._consume_no_replacement(lambda h: -1, "op failed: {}") + + self.assertIsNone(res._handle) + self.assertEqual(res._lifecycle_state, LifecycleState.CLOSED) + self.assertEqual(self.freed, [], "consumed branch must not free") +``` + +(`_FakeHandleResource`, `self.freed`, and the free instrumentation already +exist in that test class — reuse them, do not reinvent. Check the class +`setUp` for how `_free_native_ptr` is patched and restored.) + +Test 4 — the runtime no longer depends on the export. A source-inspection +test, since the symbol cannot be removed from a loaded dylib: + +```python +def test_runtime_does_not_call_error_set_last(self): + import inspect + for fn in (c2pa_module.ManagedResource._invoke_consume, + c2pa_module._read_native_error, + c2pa_module._plant_no_error_marker): + self.assertNotIn( + 'c2pa_error_set_last', inspect.getsource(fn)) +``` + +## Testing — run all of it, in this order + +1. The new tests by name, reading each test's own result line: + `python -m unittest tests.test_unit_tests.. -v` for each. +2. The full plain suite: `python -m unittest tests.test_unit_tests -v`. +3. The full threaded suite (the marker interacts with the native-section + deferral machinery, and the threaded suite is what caught the last merge + regression): `python -m unittest tests.test_unit_tests_threaded -v`. + All 543 tests across both suites currently pass; that number must hold. +4. Red proof for test 3 (only if test 3 was added): temporarily comment out + the `_plant_no_error_marker()` line inside `_invoke_consume`, run test 3 + by name, confirm it fails (the stale tag is then read and the handle is + wrongly retained), restore the line, confirm green. One inversion, one + targeted run — do not replay the whole suite around it. +5. The subprocess-based crash tests in the threaded suite + (`TestSharedSignerTeardownRace`, `TestForkedChildDoesNotDeadlock`) run as + part of step 3; they cover the free/error-slot interplay under real + threads. Do not skip them for speed. + +## Out of scope + +- The registry's address-only keying (no generation counter) is a native + c2pa-rs gap and cannot be fixed here. +- c2pa-rs v0.91.0 makes pointers always-consumed, which removes the whole + retain-vs-consume decision this marker feeds. When the binding moves to + that version, `_plant_no_error_marker`, `_NO_NATIVE_ERROR_TEXT`, + `_learn_no_error_text`, and `_is_no_native_error` all become removable — + worth a code comment on `_plant_no_error_marker` saying so. +- Performance: each plant briefly takes the native registry mutex (a + `HashMap` lookup miss under `Mutex`), where `c2pa_error_set_last` only + touched thread-local storage. Consuming calls are not hot-path; if the + perf suite (`tests/perf`) disagrees, the fallback is to prefer + `c2pa_error_set_last` when the symbol exists. Run + `tests/perf/scenarios.py` only if the baseline is already set up locally; + do not treat it as a gate. diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index ef2c2946..0dbc24a0 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -591,7 +591,7 @@ def _invoke_consume(self, ffi_call, error_message): C2paError: If the call raised any other exception. """ # Same thread that makes the call, same thread-local slot. - _lib.c2pa_error_set_last(_NO_NATIVE_ERROR) + _mark_sentinel_no_native_error() try: return ffi_call(self._handle) except ctypes.ArgumentError: @@ -844,19 +844,36 @@ class C2paStream(ctypes.Structure): ] -# Written into the native slot to mark it as carrying no error of our own. -# Planted before a consuming call so a failure that sets no error is -# distinguishable from a stale one, and written back after every read so an -# error is reportable only by the caller that observes it. -# _read_native_error() maps it to None, so it never reaches a caller. -_NO_NATIVE_ERROR_MARKER = b"c2pa-python-no-native-error" -_NO_NATIVE_ERROR = b"Other: " + _NO_NATIVE_ERROR_MARKER +# Unaligned address passed to c2pa_free to plant a marker +# in the native error slot. +# Never a real handle: allocations are aligned, and the Python +# layer only passes real handles or this constant to c2pa_free. +_MARKER_ADDR = 1 + +# Exact text the native lib writes for a failed free of _MARKER_ADDR. +# Learned at import by _learn_sentinel_no_native_error_text(). +# The format is a native implementation detail, +# so it is read back rather than hardcoded. +_NO_NATIVE_ERROR_TEXT = None + + +def _mark_sentinel_no_native_error(): + """Write the no-error marker into this thread's native error slot. + + A c2pa_free of an address the registry does not track writes + an expected error message learned at import into the + thread-local error slot and returns -1. + + This marker mechanism exists to distinguish a consuming call that + failed without setting its own error from a stale message left + by an earlier call on the same thread. + """ + _lib.c2pa_free(_MARKER_ADDR) def _is_no_native_error(message: str) -> bool: - """True for the marker meaning "no error of our own", in either spelling.""" - marker = _NO_NATIVE_ERROR_MARKER.decode('utf-8') - return message == marker or message == f"Other: {marker}" + """True for the sentinel marker meaning "no current error of our own".""" + return message == _NO_NATIVE_ERROR_TEXT def _read_native_error() -> Optional[str]: @@ -866,10 +883,8 @@ def _read_native_error() -> Optional[str]: given error is reported once, by the caller that observes it. The native slot is thread-local and sticky, so a message left in place stays readable indefinitely and is available to be reported again by a later, - unrelated call that failed without setting an error of its own. - With no error set the native side still returns an owned pointer to an - empty string, so the pointer alone does not tell us whether there is an - error. Only a non-empty message counts as one. + unrelated call that failed without setting an error of its own + (or a missing clear of an error slot). """ error = _lib.c2pa_error() if not error: @@ -881,7 +896,7 @@ def _read_native_error() -> Optional[str]: if not message: return None - _lib.c2pa_error_set_last(_NO_NATIVE_ERROR) + _mark_sentinel_no_native_error() if _is_no_native_error(message): return None return message @@ -1048,7 +1063,6 @@ def _setup_function(func, argtypes, restype=None): # Set up function prototypes not attached to an API object _setup_function(_lib.c2pa_version, [], ctypes.c_void_p) _setup_function(_lib.c2pa_error, [], ctypes.c_void_p) -_setup_function(_lib.c2pa_error_set_last, [ctypes.c_char_p], ctypes.c_int) _setup_function(_lib.c2pa_string_free, [ctypes.c_void_p], None) _setup_function( _lib.c2pa_load_settings, [ @@ -1253,6 +1267,38 @@ def _setup_function(func, argtypes, restype=None): ) _setup_function(_lib.c2pa_free, [ctypes.c_void_p], ctypes.c_int) + +def _learn_sentinel_no_native_error_text(): + """Plant the marker once and read back the exact text the native lib + produces for it, so equality checks match this build of the lib. + + Runs on the importing thread; the text is a format constant, so the + learned value holds for every thread. Raises at import when the read + back text is empty, because the marker mechanism cannot work then. + """ + _mark_sentinel_no_native_error() + raw = _lib.c2pa_error() + if not raw: + raise ImportError( + "c2pa native library did not report an error for a free of " + "an untracked pointer; the error-slot marker cannot work") + try: + text = ctypes.string_at(raw).decode('utf-8') + finally: + _lib.c2pa_string_free(raw) + if not text: + raise ImportError( + "c2pa native library reported an empty error for a free of " + "an untracked pointer; the error-slot marker cannot work") + return text + + +_NO_NATIVE_ERROR_TEXT = _learn_sentinel_no_native_error_text() +assert "0x1" in _NO_NATIVE_ERROR_TEXT, ( + "c2pa native library's untracked-pointer error text no longer " + "includes the planted address; the error-slot marker assumption " + "no longer holds") + _setup_function( _lib.c2pa_context_builder_set_signer, [ctypes.POINTER(C2paContextBuilder), ctypes.POINTER(C2paSigner)], diff --git a/tests/test_unit_tests.py b/tests/test_unit_tests.py index eb5525f6..e5a60787 100644 --- a/tests/test_unit_tests.py +++ b/tests/test_unit_tests.py @@ -8496,6 +8496,26 @@ def flaky_free(ptr): "the failing deferred free was not logged: " "{}".format(captured.output)) + def test_stale_error_not_misattributed_after_preset_error(self): + """A stale tag left by an earlier, unrelated call on this thread + must not be read as this call's own error.""" + # A stale tag from an earlier, unrelated call. + c2pa_module._lib.c2pa_error_set_last( + b"Other: UntrackedPointer: 0xdeadbeef") + + res = self._FakeHandleResource() + res._activate(0xCAFE) + + # Fails without setting any error of its own. + # The sentinel inside _invoke_consume must have cleared + # the stale tag, so this routes to the "no error of our own" branch. + with self.assertRaises(Error): + res._consume_no_replacement(lambda h: -1, "op failed: {}") + + self.assertIsNone(res._handle) + self.assertEqual(res._lifecycle_state, LifecycleState.CLOSED) + self.assertEqual(self.freed, [], "consumed branch must not free") + class TestManagedResourceObjects(TestContextAPIs): """Tests native resource handling management when managed manually. @@ -9879,19 +9899,41 @@ def test_later_failure_does_not_inherit_a_handled_errors_type(self): def test_the_no_native_error_sentinel_never_reaches_a_caller(self): """The sentinel is an internal marker, not a message for users.""" - sentinel = c2pa_module._NO_NATIVE_ERROR.decode("utf-8") + sentinel = c2pa_module._NO_NATIVE_ERROR_TEXT - c2pa_module._lib.c2pa_error_set_last(c2pa_module._NO_NATIVE_ERROR) + c2pa_module._mark_sentinel_no_native_error() self.assertIsNone( c2pa_module._read_native_error(), "the sentinel was reported as if it were a native error") - c2pa_module._lib.c2pa_error_set_last(c2pa_module._NO_NATIVE_ERROR) + c2pa_module._mark_sentinel_no_native_error() with self.assertRaises(Error) as ctx: c2pa_module._check_ffi_operation_result(None, "fallback: {}") self.assertNotIn(sentinel, str(ctx.exception)) self.assertIn("Unknown error", str(ctx.exception)) + def test_mark_sentinel_writes_the_learned_text(self): + c2pa_module._mark_sentinel_no_native_error() + raw = c2pa_module._lib.c2pa_error() + try: + text = ctypes.string_at(raw).decode('utf-8') + finally: + c2pa_module._lib.c2pa_string_free(raw) + self.assertEqual(text, c2pa_module._NO_NATIVE_ERROR_TEXT) + + def test_read_native_error_maps_sentinel_to_none(self): + c2pa_module._mark_sentinel_no_native_error() + self.assertIsNone(c2pa_module._read_native_error()) + + def test_runtime_does_not_call_error_set_last(self): + """The marker mechanism must not depend on c2pa_error_set_last, + so this module loads against native builds that lack it.""" + for fn in (c2pa_module.ManagedResource._invoke_consume, + c2pa_module._read_native_error, + c2pa_module._mark_sentinel_no_native_error): + self.assertNotIn( + 'c2pa_error_set_last', inspect.getsource(fn)) + class TestErrorsStillRaiseAfterCleanup(unittest.TestCase): """Each surface that lost a _clear_error_state() call still reports.""" From c8cf6c42947dfe551ad426670d75b21307ea0a68 Mon Sep 17 00:00:00 2001 From: tmathern <60901087+tmathern@users.noreply.github.com> Date: Tue, 25 Aug 2026 22:57:22 -0700 Subject: [PATCH 06/17] fix: Add error handling sentinel tests --- PLAN-error-slot-marker-via-c2pa-free.md | 319 ------------------------ tests/test_unit_tests.py | 98 ++++++++ 2 files changed, 98 insertions(+), 319 deletions(-) delete mode 100644 PLAN-error-slot-marker-via-c2pa-free.md diff --git a/PLAN-error-slot-marker-via-c2pa-free.md b/PLAN-error-slot-marker-via-c2pa-free.md deleted file mode 100644 index c0467b5e..00000000 --- a/PLAN-error-slot-marker-via-c2pa-free.md +++ /dev/null @@ -1,319 +0,0 @@ -# Plan: plant the error-slot marker via c2pa_free, drop the c2pa_error_set_last runtime dependency - -Implementation handoff. All facts below were verified against the current -checkout of this branch (`mathern/error-slot-sentinel`, merge commit -`f84c088` plus the `_teardown` gate restoration) and against the c2pa-rs -sources in `../c2pa-rs`. Line numbers refer to the current state of -`src/c2pa/c2pa.py`; re-grep before editing if the file has moved. - -## Why - -The error-slot fix on this branch plants a marker into the native -thread-local error slot before every consuming call, and re-plants it after -every read, so a stale message left by an earlier call on the same pooled -thread is never misread as the current failure's error. That marker decides -whether a failed consuming call retains or consumes the native handle, so a -stale read can cause a wrong free decision. - -Planting currently uses `c2pa_error_set_last`, an export added to c2pa-rs -for this purpose. Any native build that predates the export cannot load this -module (the unconditional prototype setup raises `AttributeError` at import). - -Planting cannot be avoided altogether: the slot is a single sticky -thread-local cell, `c2pa_error()` only peeks, and no export clears it. -Detecting "this call wrote nothing" requires starting from a state no -genuine call can produce, and creating that state is planting. What can be -avoided is the new-export dependency: - -`c2pa_free` on an address the registry does not track deterministically -writes an error into the same slot. Verified in c2pa-rs: -`c2pa_c_ffi/src/c_api.rs:995` routes to `cimpl_free` -(`c2pa_c_ffi/src/cimpl/utils.rs:320-345`), and a registry miss executes -`CimplError::untracked_pointer(ptr).set_last()`, whose message is -`format!("UntrackedPointer: 0x{:x}", ptr)` (`cimpl/cimpl_error.rs:101-103`). -So `_lib.c2pa_free(1)` plants a fixed, known text using only exports every -shipped native lib already has (`c2pa_free`, `c2pa_error`). Address 1 is -never a real handle: heap allocations are aligned, and the Python layer only -ever passes real handles or this constant, so the planted text cannot -collide with a genuine error about a real pointer. - -The exact wire text (with or without an `"Other: "` prefix, exact hex casing) -is a native implementation detail, so the module learns it once at import by -planting and reading back, instead of hardcoding it. - -## Changes to src/c2pa/c2pa.py - -### 1. Marker constants and helpers (replace lines 852-856) - -Delete: - -```python -_NO_NATIVE_ERROR_MARKER = b"c2pa-python-no-native-error" -_NO_NATIVE_ERROR = b"Other: " + _NO_NATIVE_ERROR_MARKER - - -def _is_no_native_error(message: str) -> bool: - """True for the marker meaning "no error of our own", in either spelling.""" - marker = _NO_NATIVE_ERROR_MARKER.decode('utf-8') - return message == marker or message == f"Other: {marker}" -``` - -Replace with: - -```python -# Address deliberately passed to c2pa_free to plant a marker in the native -# error slot. Never a real handle: allocations are aligned, and the Python -# layer only passes real handles or this constant to c2pa_free. -_MARKER_ADDR = 1 - -# Exact text the native lib writes for a failed free of _MARKER_ADDR. -# Learned at import by _learn_no_error_text(); the format is a native -# implementation detail, so it is read back rather than hardcoded. -_NO_NATIVE_ERROR_TEXT = None - - -def _plant_no_error_marker(): - """Write the no-error marker into this thread's native error slot. - - A c2pa_free of an address the registry does not track writes - "UntrackedPointer: 0x1" (learned exactly at import) into the - thread-local error slot and returns -1, which is expected here. - Calls _lib.c2pa_free directly: _free_native_ptr would log each plant. - """ - _lib.c2pa_free(_MARKER_ADDR) - - -def _is_no_native_error(message: str) -> bool: - """True for the planted marker meaning "no error of our own".""" - return message == _NO_NATIVE_ERROR_TEXT -``` - -### 2. Import-time learning (new code, placed immediately after line 1254's `_setup_function(_lib.c2pa_free, [ctypes.c_void_p], ctypes.c_int)`) - -The learning must run after the prototypes for `c2pa_free`, `c2pa_error`, -and `c2pa_string_free` are configured. `c2pa_error`/`c2pa_string_free` are -set up at lines 1049-1051; `c2pa_free` at line 1254 is the last of the -three, so the snippet goes right below it: - -```python -def _learn_no_error_text(): - """Plant the marker once and read back the exact text the native lib - produces for it, so equality checks match this build of the lib. - - Runs on the importing thread; the text is a format constant, so the - learned value holds for every thread. Raises at import when the read - back text is empty, because the marker mechanism cannot work then. - """ - _plant_no_error_marker() - raw = _lib.c2pa_error() - if not raw: - raise ImportError( - "c2pa native library did not report an error for a free of " - "an untracked pointer; the error-slot marker cannot work") - try: - text = ctypes.string_at(raw).decode('utf-8') - finally: - _lib.c2pa_string_free(raw) - if not text: - raise ImportError( - "c2pa native library reported an empty error for a free of " - "an untracked pointer; the error-slot marker cannot work") - return text - - -_NO_NATIVE_ERROR_TEXT = _learn_no_error_text() -``` - -Note: `_read_native_error` cannot be reused for learning — it maps the -marker to `None` and replants, and at learning time the marker text is not -yet known. The raw read above is intentional. - -Sanity check to add right after (a one-line assert is fine): the learned -text must contain the hex form of `_MARKER_ADDR` -(`assert "0x1" in _NO_NATIVE_ERROR_TEXT`), so a native change that breaks -the assumption fails loudly at import, not silently at the first consume -failure. - -### 3. Replace both planting call sites - -- Line 594 in `_invoke_consume`: - - ```python - # Same thread that makes the call, same thread-local slot. - _lib.c2pa_error_set_last(_NO_NATIVE_ERROR) - ``` - - becomes - - ```python - # Same thread that makes the call, same thread-local slot. - _plant_no_error_marker() - ``` - -- Line 884 at the end of `_read_native_error`: - - ```python - _lib.c2pa_error_set_last(_NO_NATIVE_ERROR) - ``` - - becomes - - ```python - _plant_no_error_marker() - ``` - - The docstring of `_read_native_error` (lines 860-869) stays accurate as - written; no change needed there. The comment block above the deleted - constants (lines 844-848) is replaced by the new constants' comments in - change 1. - -### 4. Make the c2pa_error_set_last prototype conditional (line 1051) - -```python -_setup_function(_lib.c2pa_error_set_last, [ctypes.c_char_p], ctypes.c_int) -``` - -becomes - -```python -# Optional: only newer native builds export this. The runtime does not -# call it; tests use it, when present, to simulate native error writes. -if getattr(_lib, 'c2pa_error_set_last', None) is not None: - _setup_function( - _lib.c2pa_error_set_last, [ctypes.c_char_p], ctypes.c_int) -``` - -Caveat for the implementer: `ctypes` raises `AttributeError` on missing -symbols at attribute access, and `getattr` with a default swallows exactly -that. Confirm with the vendored dylib (symbol present) that the guarded -branch still executes. - -### 5. Grep afterward - -`grep -n c2pa_error_set_last src/c2pa/c2pa.py` must show only the guarded -prototype setup from change 4. `grep -n _NO_NATIVE_ERROR src/c2pa/c2pa.py` -must show only `_NO_NATIVE_ERROR_TEXT`. - -## Changes to tests/test_unit_tests.py - -The test file references the old constant and the old mechanism in a few -places. Current anchors: - -- Line 57 (inside the `_fail_with_native_error` mock-builder) and lines - 8368, 8396, 9246, 9322, 9357, 9689, 9710: these use `c2pa_error_set_last` - to *simulate native code writing an error* (stand-ins for what failing - native calls do). They keep using it — the vendored dylib exports the - symbol, and the simulation is test-only. Do not rewrite these. - -- Lines 9880-9894, `test_the_no_native_error_sentinel_never_reaches_a_caller`: - this test plants the marker with - `c2pa_module._lib.c2pa_error_set_last(c2pa_module._NO_NATIVE_ERROR)` - (twice) and derives the leak-check string via - `sentinel = c2pa_module._NO_NATIVE_ERROR.decode("utf-8")`. Rewrite it to: - `sentinel = c2pa_module._NO_NATIVE_ERROR_TEXT` (already a str, no decode) - and replace both plant lines with - `c2pa_module._plant_no_error_marker()`. The assertions themselves - (marker read maps to None; marker text never appears in a raised - message) stay exactly as they are. - -### New tests (add near the existing sentinel tests, same class) - -Test 1 — the plant writes the learned text: - -```python -def test_plant_marker_writes_learned_text(self): - c2pa_module._plant_no_error_marker() - raw = c2pa_module._lib.c2pa_error() - try: - text = ctypes.string_at(raw).decode('utf-8') - finally: - c2pa_module._lib.c2pa_string_free(raw) - self.assertEqual(text, c2pa_module._NO_NATIVE_ERROR_TEXT) -``` - -Test 2 — the planted marker reads back as "no error": - -```python -def test_read_native_error_maps_marker_to_none(self): - c2pa_module._plant_no_error_marker() - self.assertIsNone(c2pa_module._read_native_error()) -``` - -Test 3 — a stale error is not misattributed to a failure that set nothing. -This is the scenario the whole mechanism exists for; it may already be -covered by the existing sentinel tests around line 9884 once they are -switched to the helper — if so, verify that coverage instead of duplicating -it. The shape, if needed: - -```python -def test_stale_error_not_misattributed_after_plant(self): - # A realistic stale tag from an earlier, unrelated call. - c2pa_module._lib.c2pa_error_set_last( - b"Other: UntrackedPointer: 0xdeadbeef") - - res = self._FakeHandleResource() - res._activate(0xCAFE) - - # Fails without setting any error of its own. The plant inside - # _invoke_consume must have cleared the stale tag, so this routes - # to the "no error of our own" branch: consumed, not retained. - with self.assertRaises(Error): - res._consume_no_replacement(lambda h: -1, "op failed: {}") - - self.assertIsNone(res._handle) - self.assertEqual(res._lifecycle_state, LifecycleState.CLOSED) - self.assertEqual(self.freed, [], "consumed branch must not free") -``` - -(`_FakeHandleResource`, `self.freed`, and the free instrumentation already -exist in that test class — reuse them, do not reinvent. Check the class -`setUp` for how `_free_native_ptr` is patched and restored.) - -Test 4 — the runtime no longer depends on the export. A source-inspection -test, since the symbol cannot be removed from a loaded dylib: - -```python -def test_runtime_does_not_call_error_set_last(self): - import inspect - for fn in (c2pa_module.ManagedResource._invoke_consume, - c2pa_module._read_native_error, - c2pa_module._plant_no_error_marker): - self.assertNotIn( - 'c2pa_error_set_last', inspect.getsource(fn)) -``` - -## Testing — run all of it, in this order - -1. The new tests by name, reading each test's own result line: - `python -m unittest tests.test_unit_tests.. -v` for each. -2. The full plain suite: `python -m unittest tests.test_unit_tests -v`. -3. The full threaded suite (the marker interacts with the native-section - deferral machinery, and the threaded suite is what caught the last merge - regression): `python -m unittest tests.test_unit_tests_threaded -v`. - All 543 tests across both suites currently pass; that number must hold. -4. Red proof for test 3 (only if test 3 was added): temporarily comment out - the `_plant_no_error_marker()` line inside `_invoke_consume`, run test 3 - by name, confirm it fails (the stale tag is then read and the handle is - wrongly retained), restore the line, confirm green. One inversion, one - targeted run — do not replay the whole suite around it. -5. The subprocess-based crash tests in the threaded suite - (`TestSharedSignerTeardownRace`, `TestForkedChildDoesNotDeadlock`) run as - part of step 3; they cover the free/error-slot interplay under real - threads. Do not skip them for speed. - -## Out of scope - -- The registry's address-only keying (no generation counter) is a native - c2pa-rs gap and cannot be fixed here. -- c2pa-rs v0.91.0 makes pointers always-consumed, which removes the whole - retain-vs-consume decision this marker feeds. When the binding moves to - that version, `_plant_no_error_marker`, `_NO_NATIVE_ERROR_TEXT`, - `_learn_no_error_text`, and `_is_no_native_error` all become removable — - worth a code comment on `_plant_no_error_marker` saying so. -- Performance: each plant briefly takes the native registry mutex (a - `HashMap` lookup miss under `Mutex`), where `c2pa_error_set_last` only - touched thread-local storage. Consuming calls are not hot-path; if the - perf suite (`tests/perf`) disagrees, the fallback is to prefer - `c2pa_error_set_last` when the symbol exists. Run - `tests/perf/scenarios.py` only if the baseline is already set up locally; - do not treat it as a gate. diff --git a/tests/test_unit_tests.py b/tests/test_unit_tests.py index e5a60787..c5f52360 100644 --- a/tests/test_unit_tests.py +++ b/tests/test_unit_tests.py @@ -30,6 +30,7 @@ import shutil import ctypes import threading +import concurrent.futures # Suppress deprecation warnings warnings.simplefilter("ignore", category=DeprecationWarning) @@ -9935,6 +9936,103 @@ def test_runtime_does_not_call_error_set_last(self): 'c2pa_error_set_last', inspect.getsource(fn)) +class TestMarkerOutlivesPointerConsumptionSemantics(unittest.TestCase): + """The marker is needed for reasons independent of pointer ownership. + + The native error slot is sticky and thread-local, so failure paths + that carry no still need to tell an error this call set from an + earlier, unrelated call left behind. + """ + + def setUp(self): + # Leave no message from an earlier test in this thread's slot. + c2pa_module._mark_sentinel_no_native_error() + + def test_non_consuming_failure_does_not_inherit_a_read_error(self): + c2pa_module._lib.c2pa_error_set_last(b"Signature: earlier task") + # The rightful owner reports it, which re-marks the slot. + self.assertEqual( + c2pa_module._read_native_error(), "Signature: earlier task") + + # A later, unrelated failure that sets no error of its own must + # report its own fallback, not the message above. + with self.assertRaises(Error) as ctx: + c2pa_module._check_ffi_operation_result( + 0, "later op failed: {}", check=lambda r: r == 0) + + self.assertNotIn("earlier task", str(ctx.exception)) + self.assertIn("Unknown error", str(ctx.exception)) + self.assertNotIsInstance(ctx.exception, Error.Signature) + + def test_settings_set_failure_reports_its_own_error(self): + settings = Settings() + self.addCleanup(settings.close) + + c2pa_module._lib.c2pa_error_set_last(b"Signature: earlier task") + self.assertEqual( + c2pa_module._read_native_error(), "Signature: earlier task") + + with self.assertRaises(Error) as ctx: + settings.set("builder.thumbnail.enabled", "not-a-json-value") + + self.assertNotIn("earlier task", str(ctx.exception)) + + def test_marker_is_per_thread_across_pooled_reuse(self): + """The slot is thread-local, so a pooled worker must not hand one + task's error to the next task that runs on it.""" + def failing_task(): + c2pa_module._lib.c2pa_error_set_last(b"Io: first task") + return c2pa_module._read_native_error() + + def quiet_task(): + # Sets no error; must not see the previous task's message. + return c2pa_module._read_native_error() + + # One worker guarantees both tasks run on the same OS thread. + with concurrent.futures.ThreadPoolExecutor(max_workers=1) as pool: + self.assertEqual(pool.submit(failing_task).result(), + "Io: first task") + self.assertIsNone( + pool.submit(quiet_task).result(), + "a pooled thread carried an error across unrelated tasks") + + def test_one_thread_marker_does_not_clear_another_threads_error(self): + """Marking on one thread must leave another thread's pending error + readable: the slot is per thread, and so is the marker.""" + set_on_worker = threading.Event() + marked_on_main = threading.Event() + seen = {} + + def worker(): + c2pa_module._lib.c2pa_error_set_last(b"Io: worker error") + set_on_worker.set() + self.assertTrue(marked_on_main.wait(5)) + seen["worker"] = c2pa_module._read_native_error() + + thread = threading.Thread(target=worker, daemon=True) + thread.start() + self.assertTrue(set_on_worker.wait(5)) + + c2pa_module._mark_sentinel_no_native_error() + marked_on_main.set() + thread.join(5) + + self.assertEqual(seen.get("worker"), "Io: worker error") + + def test_marker_path_is_reached_without_any_consuming_call(self): + """The non-consuming path reaches the marker through _read_native_error, + never through _invoke_consume.""" + self.assertIn("_read_native_error", + inspect.getsource( + c2pa_module._check_ffi_operation_result)) + self.assertNotIn("_invoke_consume", + inspect.getsource( + c2pa_module._check_ffi_operation_result)) + # _read_native_error is what re-marks the slot after every read. + self.assertIn("_mark_sentinel_no_native_error", + inspect.getsource(c2pa_module._read_native_error)) + + class TestErrorsStillRaiseAfterCleanup(unittest.TestCase): """Each surface that lost a _clear_error_state() call still reports.""" From 385b28c5585707604ddbf4b899beefc22827446d Mon Sep 17 00:00:00 2001 From: tmathern <60901087+tmathern@users.noreply.github.com> Date: Tue, 25 Aug 2026 23:00:12 -0700 Subject: [PATCH 07/17] fix: Add error handling sentinel tests 2 --- src/c2pa/c2pa.py | 6 +++--- tests/test_unit_tests.py | 10 ++++------ 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index 0dbc24a0..b6bc1931 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -926,9 +926,9 @@ def _native_section(): success/failure classification. Reentrant: a call whose own native call triggers another one - recursively (same thread) nests correctly here -- only the outermost - span flushes, so nothing is freed before an inner, still-open span has - finished reading its own error. + recursively (same thread) nests correctly here. Only the outermost + span flushes, so nothing is freed before an inner, still-open span is + done reading its own error. """ state = _native_section_state depth = getattr(state, 'depth', 0) diff --git a/tests/test_unit_tests.py b/tests/test_unit_tests.py index c5f52360..59e679b3 100644 --- a/tests/test_unit_tests.py +++ b/tests/test_unit_tests.py @@ -8389,15 +8389,14 @@ def test_native_section_defers_unrelated_finalizer_free(self): bystander = self._FakeHandleResource() bystander._activate(0xB00B) - def clobbering_free(ptr): + def polluting_free(ptr): self.freed.append(ptr) - # Stands in for c2pa_free's real behavior: freeing an - # untracked/foreign pointer writes its own error into the + # Freeing and untracked/ pointer writes its own error into the # same thread-local slot. c2pa_module._lib.c2pa_error_set_last( "Other: UntrackedPointer: {:#x}".format(ptr).encode()) return -1 - ManagedResource._free_native_ptr = staticmethod(clobbering_free) + ManagedResource._free_native_ptr = staticmethod(polluting_free) def ffi_call(handle): nonlocal bystander @@ -8410,8 +8409,7 @@ def ffi_call(handle): self.assertIsNone( victim._handle, - "victim was wrongly retained: bystander's deferred free still " - "clobbered the sentinel before it was read") + "victim was wrongly retained") self.assertEqual(victim._lifecycle_state, LifecycleState.CLOSED) self.assertEqual(self.freed, [0xB00B], "bystander's deferred free did not run exactly once") From 94b95b1a77c4640f821a9aa7b34bef4ce3b5dded Mon Sep 17 00:00:00 2001 From: tmathern <60901087+tmathern@users.noreply.github.com> Date: Wed, 26 Aug 2026 19:52:04 -0700 Subject: [PATCH 08/17] fix: Review comments --- .../README.md | 110 ++++++++++++++++++ .../faulthandler-output.txt | 16 +++ .../repro.py | 60 ++++++++++ src/c2pa/c2pa.py | 31 ++--- tests/test_unit_tests.py | 4 +- 5 files changed, 206 insertions(+), 15 deletions(-) create mode 100644 crashes/context-close-drops-signer-callback-mid-sign/README.md create mode 100644 crashes/context-close-drops-signer-callback-mid-sign/faulthandler-output.txt create mode 100644 crashes/context-close-drops-signer-callback-mid-sign/repro.py diff --git a/crashes/context-close-drops-signer-callback-mid-sign/README.md b/crashes/context-close-drops-signer-callback-mid-sign/README.md new file mode 100644 index 00000000..2166453f --- /dev/null +++ b/crashes/context-close-drops-signer-callback-mid-sign/README.md @@ -0,0 +1,110 @@ +# SIGSEGV: Context.close() drops the signer callback while a context-sign is calling it + +## Symptom + +The process dies with SIGSEGV (exit code 139, no Python exception) when a +`Context` built from a callback signer is closed on one thread while another +thread runs a context-sign (`Builder(manifest, context=ctx)` followed by +`builder.sign(format, source, dest)`) through it. + +Reproduced on commit `aac1f3b`, macOS arm64, Python 3.13. 5 out of 5 runs of +the multi-thread form crash; the minimal single-worker form crashes at close +delays of 0 to 10 ms after the sign enters (`repro.py`). + +faulthandler places the faulting thread inside the native sign call: + +``` +Current thread (most recent call first): + File "src/c2pa/c2pa.py", line 4032 in _sign_internal # c2pa_builder_sign_context + File "src/c2pa/c2pa.py", line 4110 in _sign_common + File "src/c2pa/c2pa.py", line 4186 in sign +``` + +The full capture is in `faulthandler-output.txt`. + +## Root cause + +`Context.__init__` pins the consumed signer's ctypes callback so it outlives +the `Signer` object: + +```python +self._signer_callback_cb = signer._callback_cb # c2pa.py:1893 +``` + +`Context._release()` is the only thing that later drops that pin: + +```python +def _release(self): + """Release Context-specific resources.""" + self._signer_callback_cb = None # c2pa.py:1911 area +``` + +`Builder._sign_internal`'s context-sign branch wraps `self._native_call()` +around `c2pa_builder_sign_context` but takes no guard on the Context. A +concurrent `ctx.close()` therefore runs `_teardown` -> `_safe_release` -> +`_release()` while the native signer is invoking the pinned callback. Dropping +the last Python reference deallocates the ctypes trampoline, and the native +side's next invocation jumps through freed memory. + +The native `Arc` inside the Builder keeps the Rust context alive, so +the pointer lifetimes on the Rust side are sound; the freed object is the +Python-owned callback trampoline. + +## Evidence for the mechanism + +Each variant run 40-120 trials: + +| Variant | Result | +|---|---| +| Close during concurrent context-sign, callback signer | SIGSEGV, reproducible | +| Same race, `Context._release` patched to keep the callback reference alive | 80/80 clean | +| Same race, context's native free suppressed (release still runs) | still SIGSEGV | +| Same race, info signer (`Signer.from_info`, no Python callback) | 120/120 clean | +| Single-threaded close-then-sign | clean (errors, no crash) | +| Dropping the last `ctx` reference mid-sign (finalizer close) | 80/80 clean | + +The pin-the-callback patch removing the crash while the suppress-the-free +patch does not isolates the trampoline drop, not the native handle free, as +the faulting object. The info-signer run shows the race window itself is +otherwise survivable. + +The finalizer variant does not crash because `__del__` can only run once the +signing thread's `Builder` no longer references the Context; an explicit +`close()` has no such ordering. + +## Reproduction + +``` +python3 crashes/context-close-drops-signer-callback-mid-sign/repro.py +``` + +Exit code 139 within a few trials. The script: build a `Context` from +`Signer.from_callback(...)`, start a thread running a context-sign, sleep +~2 ms after the sign begins, call `ctx.close()` from the main thread, join, +repeat. + +## Direction for a fix + +The callback must stay alive until no native call can invoke it. Options that +fit the existing design: + +- Give the context-sign branch a `context._native_call()` guard (the Builder + already holds `self._context`), so `close()` defers its teardown the same + way `signer.close()` defers during a borrowed sign. The deferral machinery + in `_teardown`/`_native_call` already exists. +- Alternatively, keep `_signer_callback_cb` out of `_release()` and let it die + with the Python `Context` object; the cost is the callback living as long + as the object rather than until `close()`. + +The first option also covers any other Context state a future native call +might reach mid-close. + +## Both context-sign entry points are affected + +The crash reproduces through `Builder.sign(format, source, dest)` and through +`Builder.sign_file(source, dest)` when the Builder was constructed from a +context whose signer is a callback signer. Both reach +`c2pa_builder_sign_context` with no guard on the Context. `sign_file` with an +explicit signer and correct argument order (`sign_file(source, dest, signer)`) +is unaffected, as is any explicit-signer sign, because those hold +`signer._native_call()`. diff --git a/crashes/context-close-drops-signer-callback-mid-sign/faulthandler-output.txt b/crashes/context-close-drops-signer-callback-mid-sign/faulthandler-output.txt new file mode 100644 index 00000000..fa1c6173 --- /dev/null +++ b/crashes/context-close-drops-signer-callback-mid-sign/faulthandler-output.txt @@ -0,0 +1,16 @@ +Fatal Python error: Segmentation fault + +Current thread 0x000000016e3ab000 (most recent call first): + File "/Users/taniamathern/Desktop/code/c2pa-python/src/c2pa/c2pa.py", line 4032 in _sign_internal + File "/Users/taniamathern/Desktop/code/c2pa-python/src/c2pa/c2pa.py", line 4110 in _sign_common + File "/Users/taniamathern/Desktop/code/c2pa-python/src/c2pa/c2pa.py", line 4186 in sign + File "/private/tmp/claude-501/-Users-taniamathern-Desktop-code-c2pa-python/a1e731b8-8f71-4e3d-a858-5b5d2dfe2dfb/scratchpad/crash/min.py", line 24 in w + File "/Library/Frameworks/Python.framework/Versions/3.13/lib/python3.13/threading.py", line 994 in run + File "/Library/Frameworks/Python.framework/Versions/3.13/lib/python3.13/threading.py", line 1043 in _bootstrap_inner + File "/Library/Frameworks/Python.framework/Versions/3.13/lib/python3.13/threading.py", line 1014 in _bootstrap + +Thread 0x00000001efdc1d80 (most recent call first): + File "/Library/Frameworks/Python.framework/Versions/3.13/lib/python3.13/threading.py", line 1094 in join + File "/private/tmp/claude-501/-Users-taniamathern-Desktop-code-c2pa-python/a1e731b8-8f71-4e3d-a858-5b5d2dfe2dfb/scratchpad/crash/min.py", line 31 in + +Extension modules: _cffi_backend (total: 1) diff --git a/crashes/context-close-drops-signer-callback-mid-sign/repro.py b/crashes/context-close-drops-signer-callback-mid-sign/repro.py new file mode 100644 index 00000000..61f6f195 --- /dev/null +++ b/crashes/context-close-drops-signer-callback-mid-sign/repro.py @@ -0,0 +1,60 @@ +"""SIGSEGV reproduction: Context.close() racing a context-sign that uses a +callback signer. Run from the repository root: + + python3 crashes/context-close-drops-signer-callback-mid-sign/repro.py + +Expected: the process dies with SIGSEGV (exit 139) within a few trials. +The crash needs the `cryptography` package for the ES256 callback. +""" +import sys, io, os, threading, time, faulthandler + +sys.path.insert(0, "src") +faulthandler.enable() + +from c2pa import Builder, Signer, Context, C2paSigningAlg as Alg +from cryptography.hazmat.primitives import hashes, serialization +from cryptography.hazmat.primitives.asymmetric import ec + +FIXTURES = "tests/fixtures" +certs = open(os.path.join(FIXTURES, "es256_certs.pem"), "rb").read().decode() +key_bytes = open(os.path.join(FIXTURES, "es256_private.key"), "rb").read() +image = open(os.path.join(FIXTURES, "C.jpg"), "rb").read() +MANIFEST = {"claim_generator_info": [{"name": "repro", "version": "0.1"}], + "assertions": []} + +private_key = serialization.load_pem_private_key(key_bytes, password=None) + + +def sign_callback(data: bytes) -> bytes: + return private_key.sign(data, ec.ECDSA(hashes.SHA256())) + + +def make_context() -> Context: + signer = Signer.from_callback(sign_callback, Alg.ES256, certs, + "http://timestamp.digicert.com") + return Context(signer=signer) # consumes the signer + + +for trial in range(80): + ctx = make_context() + entered = threading.Event() + + def worker(): + try: + builder = Builder(dict(MANIFEST), context=ctx) + entered.set() + builder.sign("image/jpeg", io.BytesIO(image), io.BytesIO()) + builder.close() + except Exception: + entered.set() + + t = threading.Thread(target=worker) + t.start() + entered.wait(5) + time.sleep(0.002) # let the sign enter the native call + ctx.close() # drops _signer_callback_cb mid-invocation + t.join(20) + if trial % 20 == 0: + print("trial", trial, "still alive") + +print("survived 80 trials (crash did not reproduce this run)") diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index b6bc1931..f91d9c0b 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -272,8 +272,11 @@ def __init__(self): record_owner_pid(self) def _state_lock(self): - """Return this resource's raw operation lock, with no side effects - beyond mutual exclusion. + """Return this resource's operation lock. + + Acquiring it only provides mutual exclusion; unlike _lock(), + it does not mark the thread as being inside a native-error + section. Reentrant because it is possible to run a finalizer at any bytecode boundary, including inside a region this thread has already locked, @@ -281,13 +284,12 @@ def _state_lock(self): locked region. Falls back to a fresh lock when the attribute is missing. - Unlike _lock(), this does not open a native-error section. Never hold this across a native call that drives stream callbacks (construction, resource_to_stream, the Builder stream methods, signing). Those calls release the GIL and re-enter caller-supplied Python, which may call back into this API on another thread. - Only calls that touch no callbacks are serialized here. + Only calls that don't touch callbacks are serialized here. Raises in a forked child rather than returning the lock. A child inherits this lock in whatever state it had at fork(), @@ -316,7 +318,7 @@ def _lock(self): Never hold this across a native call that drives stream callbacks. Those calls release the GIL and re-enter caller-supplied code, which may call back into this API on another thread. - Only calls that touch no callbacks are serialized here. + Only calls that don't touch callbacks are serialized here. """ with self._state_lock(), _native_section(): yield @@ -854,7 +856,7 @@ class C2paStream(ctypes.Structure): # Learned at import by _learn_sentinel_no_native_error_text(). # The format is a native implementation detail, # so it is read back rather than hardcoded. -_NO_NATIVE_ERROR_TEXT = None +_NATIVE_NO_ERROR_TEXT = None def _mark_sentinel_no_native_error(): @@ -873,7 +875,7 @@ def _mark_sentinel_no_native_error(): def _is_no_native_error(message: str) -> bool: """True for the sentinel marker meaning "no current error of our own".""" - return message == _NO_NATIVE_ERROR_TEXT + return message == _NATIVE_NO_ERROR_TEXT def _read_native_error() -> Optional[str]: @@ -1274,7 +1276,8 @@ def _learn_sentinel_no_native_error_text(): Runs on the importing thread; the text is a format constant, so the learned value holds for every thread. Raises at import when the read - back text is empty, because the marker mechanism cannot work then. + back text is empty, or when it does not carry the planted address, + because the marker mechanism cannot work in either case. """ _mark_sentinel_no_native_error() raw = _lib.c2pa_error() @@ -1290,14 +1293,16 @@ def _learn_sentinel_no_native_error_text(): raise ImportError( "c2pa native library reported an empty error for a free of " "an untracked pointer; the error-slot marker cannot work") + marker_hex = hex(_MARKER_ADDR) + if marker_hex not in text: + raise ImportError( + "c2pa native library's untracked-pointer error text no longer " + f"includes the planted address {marker_hex}; the error-slot " + "marker assumption no longer holds") return text -_NO_NATIVE_ERROR_TEXT = _learn_sentinel_no_native_error_text() -assert "0x1" in _NO_NATIVE_ERROR_TEXT, ( - "c2pa native library's untracked-pointer error text no longer " - "includes the planted address; the error-slot marker assumption " - "no longer holds") +_NATIVE_NO_ERROR_TEXT = _learn_sentinel_no_native_error_text() _setup_function( _lib.c2pa_context_builder_set_signer, diff --git a/tests/test_unit_tests.py b/tests/test_unit_tests.py index 59e679b3..315e4eb6 100644 --- a/tests/test_unit_tests.py +++ b/tests/test_unit_tests.py @@ -9898,7 +9898,7 @@ def test_later_failure_does_not_inherit_a_handled_errors_type(self): def test_the_no_native_error_sentinel_never_reaches_a_caller(self): """The sentinel is an internal marker, not a message for users.""" - sentinel = c2pa_module._NO_NATIVE_ERROR_TEXT + sentinel = c2pa_module._NATIVE_NO_ERROR_TEXT c2pa_module._mark_sentinel_no_native_error() self.assertIsNone( @@ -9918,7 +9918,7 @@ def test_mark_sentinel_writes_the_learned_text(self): text = ctypes.string_at(raw).decode('utf-8') finally: c2pa_module._lib.c2pa_string_free(raw) - self.assertEqual(text, c2pa_module._NO_NATIVE_ERROR_TEXT) + self.assertEqual(text, c2pa_module._NATIVE_NO_ERROR_TEXT) def test_read_native_error_maps_sentinel_to_none(self): c2pa_module._mark_sentinel_no_native_error() From ba4435a3c016481b37c260625f61da26fdb8ef30 Mon Sep 17 00:00:00 2001 From: tmathern <60901087+tmathern@users.noreply.github.com> Date: Fri, 28 Aug 2026 18:43:59 -0700 Subject: [PATCH 09/17] fix: Added error handling --- src/c2pa/c2pa.py | 5 +++++ tests/test_unit_tests.py | 45 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+) diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index 19deb581..ecd612b9 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -1005,12 +1005,17 @@ def _read_native_error() -> Optional[str]: """ error = _lib.c2pa_error() if not error: + # c2pa_error renders the stored message into a new C string and + # returns NULL when that fails, leaving the message in the slot. + # The slot is sticky, so it is marked here too. + _mark_sentinel_no_native_error() return None try: message = ctypes.string_at(error).decode('utf-8') finally: _lib.c2pa_string_free(error) if not message: + _mark_sentinel_no_native_error() return None _mark_sentinel_no_native_error() diff --git a/tests/test_unit_tests.py b/tests/test_unit_tests.py index 0dc11669..8597f7e9 100644 --- a/tests/test_unit_tests.py +++ b/tests/test_unit_tests.py @@ -9935,6 +9935,51 @@ def test_read_native_error_maps_sentinel_to_none(self): c2pa_module._mark_sentinel_no_native_error() self.assertIsNone(c2pa_module._read_native_error()) + def test_read_native_error_marks_the_slot_when_the_pointer_is_null(self): + """A NULL from c2pa_error must still leave the slot marked. + + c2pa_error returns NULL when the stored message cannot be rendered as + a C string. The message stays in the thread-local slot, which is + sticky, so returning without planting the marker leaves that message + readable by the next call that fails without setting an error of its + own, which then reports it as its own failure. + """ + c2pa_module._lib.c2pa_error_set_last(b"Io: unreadable original") + + original = c2pa_module._lib.c2pa_error + try: + c2pa_module._lib.c2pa_error = lambda: None + self.assertIsNone( + c2pa_module._read_native_error(), + "a NULL pointer must read as no error") + finally: + c2pa_module._lib.c2pa_error = original + + self.assertIsNone( + c2pa_module._read_native_error(), + "the message left behind by the NULL branch stayed readable " + "and is now reportable by an unrelated later failure") + + def test_a_failure_after_a_null_read_does_not_inherit_the_old_message(self): + """The message surviving a NULL read must not become someone's error.""" + c2pa_module._lib.c2pa_error_set_last(b"Io: belongs to an earlier call") + + original = c2pa_module._lib.c2pa_error + try: + c2pa_module._lib.c2pa_error = lambda: None + c2pa_module._read_native_error() + finally: + c2pa_module._lib.c2pa_error = original + + with self.assertRaises(Error) as ctx: + c2pa_module._check_ffi_operation_result( + None, "Later unrelated failure: {}") + + self.assertNotIn( + "belongs to an earlier call", str(ctx.exception), + "a later failure reported a message left by an earlier call") + self.assertIn("Unknown error", str(ctx.exception)) + def test_runtime_does_not_call_error_set_last(self): """The marker mechanism must not depend on c2pa_error_set_last, so this module loads against native builds that lack it.""" From 33d3f7d202be0c2e9b0d922a2326e9506d90be73 Mon Sep 17 00:00:00 2001 From: tmathern <60901087+tmathern@users.noreply.github.com> Date: Fri, 28 Aug 2026 19:23:07 -0700 Subject: [PATCH 10/17] fix: Added error handling 2 --- scenario.md | 116 +++++++++++++++++++++++++++++++++ src/c2pa/c2pa.py | 37 +++++++++-- tests/test_unit_tests.py | 135 ++++++++++++++++++++++++++++++++++++++- 3 files changed, 279 insertions(+), 9 deletions(-) create mode 100644 scenario.md diff --git a/scenario.md b/scenario.md new file mode 100644 index 00000000..6d60f90d --- /dev/null +++ b/scenario.md @@ -0,0 +1,116 @@ +# Stale error read in a threaded environment + +## The mechanism + +The native error slot is a `thread_local!` `RefCell` in +`c2pa_c_ffi/src/cimpl/cimpl_error.rs`. `c2pa_error()` calls `last_message()`, +which peeks at the slot and returns a copy. Nothing clears it. A message written +by one call stays readable until some later call on the same thread overwrites +it. + +That matters because not every failing native call sets an error. A call can +return NULL, or a negative status, and leave the slot holding whatever the +previous call put there. Python then reads the slot, finds a message, and +reports it as the reason this call failed. The exception type is derived from +that text too, so a caller branching on the error type branches on the wrong +one. + +The Python layer handles this by writing a marker into the slot before a native +call and treating that marker as "no error of my own". `c2pa_free` of address +`0x1` is never a real handle, so it fails and stores the text +`Other: UntrackedPointer: 0x1`, which `_read_native_error` maps back to `None`. + +## Where it breaks + +`_read_native_error` reads the slot, then plants the marker, so each message is +reported once. Two of its paths returned before reaching the marker: + +```python +error = _lib.c2pa_error() +if not error: + return None # slot still holds the old message +``` + +`c2pa_error()` renders the stored message into a fresh C string through +`to_c_string`, which returns `null_mut()` when the message contains an interior +NUL byte. The slot keeps the message; the caller sees NULL and returns `None` +without marking. The same applies to a message that decodes to empty. + +## What a user sees + +A worker thread in a pool, running two unrelated operations: + +1. An operation fails and writes `Io: belongs to an earlier call` into the slot. +2. `_read_native_error` runs and hits the NULL branch. It returns `None`, so the + first operation reports no native detail. The message stays in the slot. +3. A later, unrelated operation on the same thread fails without setting an + error. It reads the slot, finds the message from step 1, and raises it. + +The third operation raises `Io: belongs to an earlier call`. The thread pool is +what makes this reachable in practice: the two operations share a thread and +never share anything else. + +Both tests added in `tests/test_unit_tests.py` fail against the unfixed code: + +```text +AssertionError: 'belongs to an earlier call' unexpectedly found in +'Io: belongs to an earlier call' : a later failure reported a message +left by an earlier call +``` + +## The fix + +Plant the marker on both early returns, so a message that cannot be rendered or +decoded is still consumed: + +```python +error = _lib.c2pa_error() +if not error: + _mark_sentinel_no_native_error() + return None +``` + +Every path out of `_read_native_error` now leaves the slot carrying the marker, +which is what the function's docstring already described. + +## Caller text forging a pointer rejection + +After a consuming call fails, `_raise_consume_failure` decides who owns the +handle by looking for one of four tags in the native message. A tag names a +rejection that happened before native took ownership, so matching one means the +handle is still ours and the resource goes back into service. + +The match was a substring search, and native messages quote caller-supplied +strings verbatim. A JSON parse failure repeats the offending value: + +```text +Json: invalid type: string "NullParameter: injected", expected a sequence at line 1 column 50 +``` + +That message described a bad settings value, and the classifier read it as a +pointer rejection. The resource is then restored to usable while native may +already own and have dropped its handle. An `Io:` error naming a path that +contains a tag does the same thing. + +Real rejections occupy two positions and never appear mid-message: either the +tag starts the message, or it follows the one `Other:` wrapper. + +```text +NullParameter: format +Other: UntrackedPointer: 0x9999 +``` + +Stripping that wrapper and requiring the tag at the start of what remains keeps +every real rejection and rejects the forged ones. + +## A second stale read, in the teardown queue + +`_native_section` defers teardowns that arrive while a section is open and +flushes them when the outermost span closes. The flush loop was unguarded, so +one resource raising skipped every resource queued behind it, and the deferral +was the only remaining path to those handles' frees. + +`_finish_teardown` and `_safe_release` both catch `Exception`, which leaves a +`KeyboardInterrupt` arriving during a flush able to escape and strand the queue. +Each flush now runs under its own guard, with the first exception re-raised once +the queue is drained. diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index ecd612b9..66831640 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -608,6 +608,22 @@ def _swap_handle(self, new_handle): "InvalidBufferSize:", ) + # An error tag starts the message or follows this one wrapper. + _NATIVE_ERROR_WRAPPER = "Other: " + + @staticmethod + def _is_pre_consume_rejection(error: str) -> bool: + """True when native rejected the handle before taking ownership. + + Anchored, not a substring search: native quotes caller text verbatim, + so a tag mid-message describes the caller's input, not ownership. + """ + body = error + if body.startswith(ManagedResource._NATIVE_ERROR_WRAPPER): + body = body[len(ManagedResource._NATIVE_ERROR_WRAPPER):] + return any(body.startswith(tag) + for tag in ManagedResource._PRE_CONSUME_ERROR_TAGS) + def _invoke_consume(self, ffi_call, error_message, *, reserved=False): """Run an FFI call that consumes this handle, returning its raw result. @@ -677,8 +693,7 @@ def _raise_consume_failure(self, error_message, previous_state=None): """ error = _read_native_error() if error: - if any(tag in error - for tag in ManagedResource._PRE_CONSUME_ERROR_TAGS): + if ManagedResource._is_pre_consume_rejection(error): logger.warning( "%s: native call rejected the handle before taking " "ownership (%s); handle retained", @@ -1005,9 +1020,8 @@ def _read_native_error() -> Optional[str]: """ error = _lib.c2pa_error() if not error: - # c2pa_error renders the stored message into a new C string and - # returns NULL when that fails, leaving the message in the slot. - # The slot is sticky, so it is marked here too. + # NULL means the message could not be rendered, not that the slot + # is empty, so it still has to be marked. _mark_sentinel_no_native_error() return None try: @@ -1051,6 +1065,10 @@ def _native_section(): recursively (same thread) nests correctly here. Only the outermost span flushes, so nothing is freed before an inner, still-open span is done reading its own error. + + Each flush is guarded: the deferral is the only remaining path to that + resource's free, so one raising would strand the rest. The first + exception is re-raised once the queue is drained. """ state = _native_section_state depth = getattr(state, 'depth', 0) @@ -1063,8 +1081,15 @@ def _native_section(): state.depth -= 1 if state.depth == 0: pending, state.pending_resources = state.pending_resources, [] + first_error = None for resource in pending: - resource._maybe_flush_pending() + try: + resource._maybe_flush_pending() + except BaseException as e: # noqa: BLE001 + if first_error is None: + first_error = e + if first_error is not None: + raise first_error class C2paSignerInfo(ctypes.Structure): diff --git a/tests/test_unit_tests.py b/tests/test_unit_tests.py index 8597f7e9..b984527f 100644 --- a/tests/test_unit_tests.py +++ b/tests/test_unit_tests.py @@ -8891,11 +8891,11 @@ def _raise(*_args): @staticmethod def _is_pre_consume_rejection(error_message): - """True if this native error means ownership never transferred.""" + """True if this native error means ownership never transferred. + """ if not error_message: return False - return any(tag in error_message - for tag in ManagedResource._PRE_CONSUME_ERROR_TAGS) + return ManagedResource._is_pre_consume_rejection(error_message) def _stale_reader_handle(self): """A freed, untracked pointer, captured before close() nulls it. @@ -9980,6 +9980,135 @@ def test_a_failure_after_a_null_read_does_not_inherit_the_old_message(self): "a later failure reported a message left by an earlier call") self.assertIn("Unknown error", str(ctx.exception)) + def test_every_real_rejection_wording_is_classified_as_pre_consume(self): + """The four tags arrive bare or behind the "Other: " wrapper.""" + wrapper = c2pa_module.ManagedResource._NATIVE_ERROR_WRAPPER + classify = c2pa_module.ManagedResource._is_pre_consume_rejection + + for tag in c2pa_module.ManagedResource._PRE_CONSUME_ERROR_TAGS: + bare = f"{tag} some detail" + wrapped = f"{wrapper}{tag} some detail" + self.assertTrue( + classify(bare), + f"a bare {tag} rejection was read as a consumed handle") + self.assertTrue( + classify(wrapped), + f"a wrapped {tag} rejection was read as a consumed handle") + + def test_native_rejections_observed_from_the_library_still_classify(self): + """Pins the two shapes the loaded library actually produces.""" + classify = c2pa_module.ManagedResource._is_pre_consume_rejection + + # A null argument the native layer refuses before doing any work. + c2pa_module._lib.c2pa_reader_from_stream(None, None) + bare = c2pa_module._read_native_error() + + # A free of an address the pointer registry does not track. + c2pa_module._lib.c2pa_free(0x9999) + wrapped = c2pa_module._read_native_error() + + for message in (bare, wrapped): + self.assertTrue( + message, + "the native library stopped reporting these rejections") + self.assertTrue( + classify(message), + f"the native rejection wording changed: {message!r}") + + def test_caller_text_quoting_a_tag_is_not_a_rejection(self): + """A tag inside the message body describes the caller's input. + + Native errors quote caller-supplied strings verbatim: a JSON parse + failure repeats the offending value, an Io failure names the path. + Reading one of those as a pre-consume rejection hands the resource back + as usable after native may already own and have dropped its handle. + """ + classify = c2pa_module.ManagedResource._is_pre_consume_rejection + + forged = ( + 'Json: invalid type: string "NullParameter: x", expected a ' + 'sequence at line 1 column 43', + 'Json: invalid type: string "WrongPointerType: y", expected a ' + 'sequence at line 1 column 46', + "Io: cannot open /tmp/UntrackedPointer: 0xdead.jpg", + "Other: manifest text mentions InvalidBufferSize: in passing", + ) + for message in forged: + self.assertFalse( + classify(message), + f"caller text was read as a pointer rejection: {message!r}") + + def test_caller_text_quoting_a_tag_reaches_the_error_slot(self): + """The forged wording above is what the library really produces.""" + c2pa_module._lib.c2pa_builder_from_json( + b'{"claim_generator_info": "NullParameter: injected"}') + message = c2pa_module._read_native_error() + + self.assertIn( + "NullParameter:", message, + "caller text no longer reaches the error slot verbatim, so this " + "test no longer exercises the case it was written for") + self.assertFalse( + c2pa_module.ManagedResource._is_pre_consume_rejection(message), + f"a caller-supplied string forged a pointer rejection: {message!r}") + + def test_a_failing_flush_does_not_strand_the_rest_of_the_queue(self): + """One resource raising must not skip the resources queued behind it. + """ + flushed = [] + + class Recorder: + def __init__(self, name, raises=None): + self.name = name + self.raises = raises + + def _maybe_flush_pending(self): + if self.raises is not None: + raise self.raises + flushed.append(self.name) + + first = Recorder("first") + middle = Recorder("middle", raises=KeyboardInterrupt()) + last = Recorder("last") + + with self.assertRaises(KeyboardInterrupt): + with c2pa_module._native_section(): + for resource in (first, middle, last): + c2pa_module._register_for_section_flush(resource) + + self.assertEqual( + flushed, ["first", "last"], + "a resource queued behind a failing one was never flushed, " + "so its handle leaks") + + def test_a_failing_flush_still_reports_the_first_exception(self): + """Draining the queue must not swallow the failure. + """ + flushed = [] + + class Recorder: + def __init__(self, name, raises=None): + self.name = name + self.raises = raises + + def _maybe_flush_pending(self): + if self.raises is not None: + raise self.raises + flushed.append(self.name) + + with self.assertRaises(RuntimeError) as ctx: + with c2pa_module._native_section(): + for resource in ( + Recorder("boom", raises=RuntimeError("first failure")), + Recorder("survivor"), + Recorder("later", raises=RuntimeError("second failure"))): + c2pa_module._register_for_section_flush(resource) + + self.assertIn("first failure", str(ctx.exception)) + self.assertEqual( + flushed, ["survivor"], + "a resource between two failing ones was never flushed") + def test_runtime_does_not_call_error_set_last(self): """The marker mechanism must not depend on c2pa_error_set_last, so this module loads against native builds that lack it.""" From cca44d7236835ae549d463f6e99cf1c17e1c662d Mon Sep 17 00:00:00 2001 From: tmathern <60901087+tmathern@users.noreply.github.com> Date: Fri, 28 Aug 2026 19:40:13 -0700 Subject: [PATCH 11/17] fix: Debug clean up --- scenario.md | 116 ---------------------------------------------------- 1 file changed, 116 deletions(-) delete mode 100644 scenario.md diff --git a/scenario.md b/scenario.md deleted file mode 100644 index 6d60f90d..00000000 --- a/scenario.md +++ /dev/null @@ -1,116 +0,0 @@ -# Stale error read in a threaded environment - -## The mechanism - -The native error slot is a `thread_local!` `RefCell` in -`c2pa_c_ffi/src/cimpl/cimpl_error.rs`. `c2pa_error()` calls `last_message()`, -which peeks at the slot and returns a copy. Nothing clears it. A message written -by one call stays readable until some later call on the same thread overwrites -it. - -That matters because not every failing native call sets an error. A call can -return NULL, or a negative status, and leave the slot holding whatever the -previous call put there. Python then reads the slot, finds a message, and -reports it as the reason this call failed. The exception type is derived from -that text too, so a caller branching on the error type branches on the wrong -one. - -The Python layer handles this by writing a marker into the slot before a native -call and treating that marker as "no error of my own". `c2pa_free` of address -`0x1` is never a real handle, so it fails and stores the text -`Other: UntrackedPointer: 0x1`, which `_read_native_error` maps back to `None`. - -## Where it breaks - -`_read_native_error` reads the slot, then plants the marker, so each message is -reported once. Two of its paths returned before reaching the marker: - -```python -error = _lib.c2pa_error() -if not error: - return None # slot still holds the old message -``` - -`c2pa_error()` renders the stored message into a fresh C string through -`to_c_string`, which returns `null_mut()` when the message contains an interior -NUL byte. The slot keeps the message; the caller sees NULL and returns `None` -without marking. The same applies to a message that decodes to empty. - -## What a user sees - -A worker thread in a pool, running two unrelated operations: - -1. An operation fails and writes `Io: belongs to an earlier call` into the slot. -2. `_read_native_error` runs and hits the NULL branch. It returns `None`, so the - first operation reports no native detail. The message stays in the slot. -3. A later, unrelated operation on the same thread fails without setting an - error. It reads the slot, finds the message from step 1, and raises it. - -The third operation raises `Io: belongs to an earlier call`. The thread pool is -what makes this reachable in practice: the two operations share a thread and -never share anything else. - -Both tests added in `tests/test_unit_tests.py` fail against the unfixed code: - -```text -AssertionError: 'belongs to an earlier call' unexpectedly found in -'Io: belongs to an earlier call' : a later failure reported a message -left by an earlier call -``` - -## The fix - -Plant the marker on both early returns, so a message that cannot be rendered or -decoded is still consumed: - -```python -error = _lib.c2pa_error() -if not error: - _mark_sentinel_no_native_error() - return None -``` - -Every path out of `_read_native_error` now leaves the slot carrying the marker, -which is what the function's docstring already described. - -## Caller text forging a pointer rejection - -After a consuming call fails, `_raise_consume_failure` decides who owns the -handle by looking for one of four tags in the native message. A tag names a -rejection that happened before native took ownership, so matching one means the -handle is still ours and the resource goes back into service. - -The match was a substring search, and native messages quote caller-supplied -strings verbatim. A JSON parse failure repeats the offending value: - -```text -Json: invalid type: string "NullParameter: injected", expected a sequence at line 1 column 50 -``` - -That message described a bad settings value, and the classifier read it as a -pointer rejection. The resource is then restored to usable while native may -already own and have dropped its handle. An `Io:` error naming a path that -contains a tag does the same thing. - -Real rejections occupy two positions and never appear mid-message: either the -tag starts the message, or it follows the one `Other:` wrapper. - -```text -NullParameter: format -Other: UntrackedPointer: 0x9999 -``` - -Stripping that wrapper and requiring the tag at the start of what remains keeps -every real rejection and rejects the forged ones. - -## A second stale read, in the teardown queue - -`_native_section` defers teardowns that arrive while a section is open and -flushes them when the outermost span closes. The flush loop was unguarded, so -one resource raising skipped every resource queued behind it, and the deferral -was the only remaining path to those handles' frees. - -`_finish_teardown` and `_safe_release` both catch `Exception`, which leaves a -`KeyboardInterrupt` arriving during a flush able to escape and strand the queue. -Each flush now runs under its own guard, with the first exception re-raised once -the queue is drained. From 26ed0e778f734c865bb04770e5aaaf3b7632efac Mon Sep 17 00:00:00 2001 From: tmathern <60901087+tmathern@users.noreply.github.com> Date: Fri, 28 Aug 2026 21:54:17 -0700 Subject: [PATCH 12/17] fix: Re-baseline --- tests/perf/baseline.json | 355 ++++++++++++++++++------------------ tests/perf/reports/.gitkeep | 0 tests/perf/scenarios.py | 39 +++- 3 files changed, 217 insertions(+), 177 deletions(-) delete mode 100644 tests/perf/reports/.gitkeep diff --git a/tests/perf/baseline.json b/tests/perf/baseline.json index c151efe5..5d7017ee 100644 --- a/tests/perf/baseline.json +++ b/tests/perf/baseline.json @@ -2,299 +2,304 @@ "_meta": { "memray_version": "1.19.3", "python_version": "3.12.13", - "c2pa_native_version": "c2pa-v0.90.0", + "c2pa_native_version": "c2pa-v0.90.16", "iterations": 200, "perf_env": "python-3.12-slim", "arch": "aarch64" }, "reader_jpeg_legacy": { - "peak_bytes": 3851610, - "leaked_bytes": 3351823, - "total_allocations": 1362322 + "peak_bytes": 3912724, + "leaked_bytes": 3414162, + "total_allocations": 1324293 }, "reader_jpeg_with_context": { - "peak_bytes": 3845367, - "leaked_bytes": 3345097, - "total_allocations": 1349879 + "peak_bytes": 3907256, + "leaked_bytes": 3407478, + "total_allocations": 1333897 }, "reader_manifest_data_context": { - "peak_bytes": 7636730, - "leaked_bytes": 3468040, - "total_allocations": 1147359 + "peak_bytes": 7692972, + "leaked_bytes": 3524955, + "total_allocations": 1132827 }, "reader_mp4": { - "peak_bytes": 4222601, - "leaked_bytes": 3345724, - "total_allocations": 4095915 + "peak_bytes": 4272788, + "leaked_bytes": 3406355, + "total_allocations": 4018933 }, "reader_wav": { - "peak_bytes": 4523095, - "leaked_bytes": 3355666, - "total_allocations": 742391 + "peak_bytes": 4573253, + "leaked_bytes": 3416313, + "total_allocations": 773409 }, "builder_sign_jpeg_legacy": { - "peak_bytes": 7785129, - "leaked_bytes": 3468507, - "total_allocations": 1041412 + "peak_bytes": 7844930, + "leaked_bytes": 3530986, + "total_allocations": 1046607 }, "builder_sign_jpeg_with_context": { - "peak_bytes": 7779538, - "leaked_bytes": 3463042, - "total_allocations": 1027485 + "peak_bytes": 7839456, + "leaked_bytes": 3524460, + "total_allocations": 1058202 }, "builder_sign_png_legacy": { - "peak_bytes": 8023081, - "leaked_bytes": 3468300, - "total_allocations": 3883115 + "peak_bytes": 8082891, + "leaked_bytes": 3530892, + "total_allocations": 3888499 }, "builder_sign_png_with_context": { - "peak_bytes": 8017008, - "leaked_bytes": 3462829, - "total_allocations": 3869515 + "peak_bytes": 8077349, + "leaked_bytes": 3524774, + "total_allocations": 3900456 }, "builder_sign_jpeg_parallel_split_pool": { - "peak_bytes": 45854797, - "leaked_bytes": 3840928, - "total_allocations": 1035646 + "peak_bytes": 45936892, + "leaked_bytes": 3893837, + "total_allocations": 1062520 }, "builder_sign_jpeg_parallel_split_barrier": { - "peak_bytes": 45844809, - "leaked_bytes": 3861014, - "total_allocations": 1037741 + "peak_bytes": 45905378, + "leaked_bytes": 3892593, + "total_allocations": 1061149 }, "builder_sign_png_parallel_split_pool": { - "peak_bytes": 46586728, - "leaked_bytes": 3868054, - "total_allocations": 3877696 + "peak_bytes": 46673225, + "leaked_bytes": 3929128, + "total_allocations": 3904507 }, "builder_sign_png_parallel_split_barrier": { - "peak_bytes": 46082548, - "leaked_bytes": 3879161, - "total_allocations": 3879780 + "peak_bytes": 46143013, + "leaked_bytes": 3910964, + "total_allocations": 3903125 }, "builder_sign_gif": { - "peak_bytes": 14635465, - "leaked_bytes": 3461270, - "total_allocations": 17017654 + "peak_bytes": 14696646, + "leaked_bytes": 3524513, + "total_allocations": 17048351 }, "builder_sign_heic": { - "peak_bytes": 4698434, - "leaked_bytes": 3469086, - "total_allocations": 1563419 + "peak_bytes": 4759642, + "leaked_bytes": 3532315, + "total_allocations": 1582361 }, "builder_sign_m4a": { - "peak_bytes": 18833496, - "leaked_bytes": 3469085, - "total_allocations": 5194205 + "peak_bytes": 18895243, + "leaked_bytes": 3532373, + "total_allocations": 5213365 }, "builder_sign_webp": { - "peak_bytes": 8991237, - "leaked_bytes": 3461271, - "total_allocations": 916145 + "peak_bytes": 9052463, + "leaked_bytes": 3524559, + "total_allocations": 950737 }, "builder_sign_avi": { - "peak_bytes": 7130933, - "leaked_bytes": 3461270, - "total_allocations": 89982012 + "peak_bytes": 7192106, + "leaked_bytes": 3524502, + "total_allocations": 90011891 }, "builder_sign_mp4": { - "peak_bytes": 6245379, - "leaked_bytes": 3469085, - "total_allocations": 3788717 + "peak_bytes": 6306630, + "leaked_bytes": 3532325, + "total_allocations": 3805992 }, "builder_sign_tiff": { - "peak_bytes": 13213169, - "leaked_bytes": 3461271, - "total_allocations": 10862700 + "peak_bytes": 13274395, + "leaked_bytes": 3524559, + "total_allocations": 10898003 }, "builder_sign_jpeg_parent_of": { - "peak_bytes": 14265295, - "leaked_bytes": 3461665, - "total_allocations": 2506107 + "peak_bytes": 14324563, + "leaked_bytes": 3525074, + "total_allocations": 2495210 }, "builder_sign_jpeg_component_of": { - "peak_bytes": 14266996, - "leaked_bytes": 3462012, - "total_allocations": 2551180 + "peak_bytes": 14325966, + "leaked_bytes": 3524803, + "total_allocations": 2538825 }, "builder_sign_jpeg_parent_and_component": { - "peak_bytes": 14665241, - "leaked_bytes": 3614613, - "total_allocations": 4523960 + "peak_bytes": 14605703, + "leaked_bytes": 3610907, + "total_allocations": 4464450 }, "builder_sign_jpeg_parent_and_component_mixed_mime": { - "peak_bytes": 14568780, - "leaked_bytes": 3462718, - "total_allocations": 5517180 + "peak_bytes": 14627596, + "leaked_bytes": 3525478, + "total_allocations": 5516963 }, "builder_sign_jpeg_two_components_same_mime": { - "peak_bytes": 14559274, - "leaked_bytes": 3564233, - "total_allocations": 4497379 + "peak_bytes": 14602589, + "leaked_bytes": 3610897, + "total_allocations": 4436718 }, "builder_sign_jpeg_two_components_mixed_mime": { - "peak_bytes": 14564839, - "leaked_bytes": 3461873, - "total_allocations": 5490592 + "peak_bytes": 14624276, + "leaked_bytes": 3525287, + "total_allocations": 5489138 }, "builder_sign_jpeg_archive_roundtrip": { - "peak_bytes": 14297571, - "leaked_bytes": 3481212, - "total_allocations": 3467149 + "peak_bytes": 14356429, + "leaked_bytes": 3545507, + "total_allocations": 3432412 }, "builder_from_archive_roundtrip": { - "peak_bytes": 14297349, - "leaked_bytes": 3480475, - "total_allocations": 3101030 + "peak_bytes": 14353258, + "leaked_bytes": 3542427, + "total_allocations": 3024501 }, "builder_with_archive_swap": { - "peak_bytes": 3681081, - "leaked_bytes": 3350198, - "total_allocations": 704373 + "peak_bytes": 3753525, + "leaked_bytes": 3421374, + "total_allocations": 744039 }, "reader_with_fragment_swap": { - "peak_bytes": 3778159, - "leaked_bytes": 3353205, - "total_allocations": 3787587 + "peak_bytes": 3839453, + "leaked_bytes": 3414104, + "total_allocations": 3806570 }, "with_fragment_pre_consume_rejection": { - "peak_bytes": 3778057, - "leaked_bytes": 3354795, - "total_allocations": 2094004 + "peak_bytes": 3841126, + "leaked_bytes": 3417826, + "total_allocations": 2128201 }, "with_archive_post_consume_failure": { - "peak_bytes": 3350600, - "leaked_bytes": 3308056, - "total_allocations": 175290 + "peak_bytes": 3423615, + "leaked_bytes": 3380332, + "total_allocations": 208578 }, "with_fragment_marshalling_error": { - "peak_bytes": 3708068, - "leaked_bytes": 3352335, - "total_allocations": 2077090 + "peak_bytes": 3767911, + "leaked_bytes": 3413267, + "total_allocations": 2094661 }, "with_fragment_mixed_outcomes": { - "peak_bytes": 3779175, - "leaked_bytes": 3356294, - "total_allocations": 2656787 + "peak_bytes": 3840297, + "leaked_bytes": 3417238, + "total_allocations": 2686269 }, "builder_to_archive_with_ingredient": { - "peak_bytes": 14069232, - "leaked_bytes": 3337316, - "total_allocations": 1830896 + "peak_bytes": 14142488, + "leaked_bytes": 3409388, + "total_allocations": 1790801 }, "builder_sign_jpeg_archive_roundtrip_ingredient_in_archive": { - "peak_bytes": 14287046, - "leaked_bytes": 3481977, - "total_allocations": 5879957 + "peak_bytes": 14345054, + "leaked_bytes": 3543673, + "total_allocations": 5769615 }, "builder_write_ingredient_archive": { - "peak_bytes": 14069289, - "leaked_bytes": 3337377, - "total_allocations": 1805304 + "peak_bytes": 14142437, + "leaked_bytes": 3409341, + "total_allocations": 1767419 }, "builder_sign_jpeg_add_ingredient_from_archive": { - "peak_bytes": 14133742, - "leaked_bytes": 3480831, - "total_allocations": 3415920 + "peak_bytes": 14207929, + "leaked_bytes": 3544945, + "total_allocations": 3383511 }, "builder_ingredient_archive_roundtrip": { - "peak_bytes": 14284443, - "leaked_bytes": 3480809, - "total_allocations": 5132060 + "peak_bytes": 14345163, + "leaked_bytes": 3545508, + "total_allocations": 5061203 }, "builder_sign_jpeg_two_ingredient_archives": { - "peak_bytes": 14134560, - "leaked_bytes": 3481604, - "total_allocations": 4215728 + "peak_bytes": 14208534, + "leaked_bytes": 3545923, + "total_allocations": 4185124 }, "reader_error_no_manifest": { - "peak_bytes": 3564471, - "leaked_bytes": 3323629, - "total_allocations": 276175 + "peak_bytes": 3622191, + "leaked_bytes": 3383889, + "total_allocations": 291735 }, "builder_error_invalid_manifest": { - "peak_bytes": 3352053, - "leaked_bytes": 3297079, - "total_allocations": 113926 + "peak_bytes": 3421406, + "leaked_bytes": 3365544, + "total_allocations": 126199 }, "reader_string_apis": { - "peak_bytes": 3978113, - "leaked_bytes": 3346111, - "total_allocations": 2287335 + "peak_bytes": 4039136, + "leaked_bytes": 3407512, + "total_allocations": 2238705 }, "signer_construction": { - "peak_bytes": 3350893, - "leaked_bytes": 3288137, - "total_allocations": 153245 + "peak_bytes": 3421644, + "leaked_bytes": 3358098, + "total_allocations": 159717 }, "builder_from_context_construction": { - "peak_bytes": 3350600, - "leaked_bytes": 3288582, - "total_allocations": 112688 + "peak_bytes": 3423152, + "leaked_bytes": 3360708, + "total_allocations": 146014 }, "fork_reader_collect": { - "peak_bytes": 3850530, - "leaked_bytes": 3353063, - "total_allocations": 1328122 + "peak_bytes": 3911936, + "leaked_bytes": 3413740, + "total_allocations": 1284294 }, "fork_contended_mutex": { - "peak_bytes": 7679019, - "leaked_bytes": 3482128, - "total_allocations": 67472694 + "peak_bytes": 7700288, + "leaked_bytes": 3510073, + "total_allocations": 66621221 }, "fork_thread_local_orphan": { - "peak_bytes": 3936170, - "leaked_bytes": 3439733, - "total_allocations": 1381055 + "peak_bytes": 4073730, + "leaked_bytes": 3581333, + "total_allocations": 1339630 }, "fork_gc_cycle": { - "peak_bytes": 3850434, - "leaked_bytes": 3353160, - "total_allocations": 1332098 + "peak_bytes": 3913068, + "leaked_bytes": 3414664, + "total_allocations": 1289268 }, "fork_parent_frees_after_fork": { - "peak_bytes": 5447584, - "leaked_bytes": 3350400, - "total_allocations": 24829257 + "peak_bytes": 5602620, + "leaked_bytes": 3423572, + "total_allocations": 23965279 }, "fork_child_closes_then_parent_frees": { - "peak_bytes": 5446620, - "leaked_bytes": 3350407, - "total_allocations": 24829254 + "peak_bytes": 5603711, + "leaked_bytes": 3424393, + "total_allocations": 23965271 }, "fork_child_sys_exit": { - "peak_bytes": 3850546, - "leaked_bytes": 3353234, - "total_allocations": 1335925 + "peak_bytes": 3911952, + "leaked_bytes": 3413956, + "total_allocations": 1301497 }, "fork_stream_cleanup": { - "peak_bytes": 3464063, - "leaked_bytes": 3291969, - "total_allocations": 105340 + "peak_bytes": 3532824, + "leaked_bytes": 3361106, + "total_allocations": 110397 }, "fork_swap_cleanup": { - "peak_bytes": 3681171, - "leaked_bytes": 3350696, - "total_allocations": 714376 + "peak_bytes": 3753679, + "leaked_bytes": 3421936, + "total_allocations": 754042 }, "fork_contended_mutex_swap": { - "peak_bytes": 7302379, - "leaked_bytes": 3475147, - "total_allocations": 35948516 + "peak_bytes": 7360409, + "leaked_bytes": 3525035, + "total_allocations": 37359891 }, "fork_contended_mutex_wrap": { - "peak_bytes": 7288748, - "leaked_bytes": 3463411, - "total_allocations": 34847186 + "peak_bytes": 7140341, + "leaked_bytes": 3522965, + "total_allocations": 34204380 }, "fork_consumed_signer": { - "peak_bytes": 3350894, - "leaked_bytes": 3288906, - "total_allocations": 175055 + "peak_bytes": 3421645, + "leaked_bytes": 3359803, + "total_allocations": 206540 }, "swap_chain_churn": { - "peak_bytes": 3681161, - "leaked_bytes": 3350287, - "total_allocations": 672537 + "peak_bytes": 3753669, + "leaked_bytes": 3421527, + "total_allocations": 679964 + }, + "deferred_teardown_flush_queue": { + "peak_bytes": 4103247, + "leaked_bytes": 3412690, + "total_allocations": 2448668 } } \ No newline at end of file diff --git a/tests/perf/reports/.gitkeep b/tests/perf/reports/.gitkeep deleted file mode 100644 index e69de29b..00000000 diff --git a/tests/perf/scenarios.py b/tests/perf/scenarios.py index 23300aed..87dd512e 100644 --- a/tests/perf/scenarios.py +++ b/tests/perf/scenarios.py @@ -587,8 +587,8 @@ def scenario_reader_with_fragment_pre_consume_rejection( # Fail loudly: without these the scenario still runs when the # ownership logic regresses, and a rejection that stops being # recognised looks identical to a pass. - if not any(tag in str(e) for tag in - c2pa_module.ManagedResource._PRE_CONSUME_ERROR_TAGS): + if not c2pa_module.ManagedResource._is_pre_consume_rejection( + str(e)): raise AssertionError( f"expected a pre-consume rejection, got: {e}") from e if reader._handle is None: @@ -1297,6 +1297,40 @@ def scenario_swap_chain_churn(iterations: int = 100) -> None: context.close() +def scenario_deferred_teardown_flush_queue(iterations: int = 100) -> None: + """Close resources from inside an open native-error section, so their + teardowns defer onto one pending list and are drained together when the + section closes. + + Two resources per iteration rather than one: a single-element queue cannot + show a resource stranded behind its predecessor. + """ + signed_bytes = SIGNED_JPEG.read_bytes() + real_free = c2pa_module.ManagedResource._free_native_ptr + for _ in _iterate(iterations): + first = Reader("image/jpeg", io.BytesIO(signed_bytes)) + second = Reader("image/jpeg", io.BytesIO(signed_bytes)) + + freed = [] + c2pa_module.ManagedResource._free_native_ptr = staticmethod( + lambda ptr: (freed.append(ptr), real_free(ptr))[1]) + try: + with c2pa_module._native_section(): + first.close() + second.close() + # Fail loudly: a free here means the teardown was not deferred. + if freed: + raise AssertionError( + "teardown inside a section freed immediately " + "instead of deferring") + if len(freed) != 2: + raise AssertionError( + f"drain freed {len(freed)} of 2 deferred handles; " + f"the rest leak") + finally: + c2pa_module.ManagedResource._free_native_ptr = real_free + + def scenario_fork_swap_cleanup(iterations: int = 100) -> None: """Fork safety benchmark scenario: the handle a Builder owns at fork time came from with_archive(), which @@ -1403,6 +1437,7 @@ def scenario_fork_stream_cleanup(iterations: int = 100) -> None: "fork_contended_mutex_wrap": scenario_fork_contended_mutex_wrap, "fork_consumed_signer": scenario_fork_consumed_signer, "swap_chain_churn": scenario_swap_chain_churn, + "deferred_teardown_flush_queue": scenario_deferred_teardown_flush_queue, } From df29505548178f736affaf661905e4be7109bf34 Mon Sep 17 00:00:00 2001 From: tmathern <60901087+tmathern@users.noreply.github.com> Date: Fri, 28 Aug 2026 21:59:25 -0700 Subject: [PATCH 13/17] fix: Restore gitkeep file --- tests/perf/reports/.gitkeep | 0 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 tests/perf/reports/.gitkeep diff --git a/tests/perf/reports/.gitkeep b/tests/perf/reports/.gitkeep new file mode 100644 index 00000000..e69de29b From 9f62daf56d0bd89d0d49155d08082f29d61cc9bf Mon Sep 17 00:00:00 2001 From: tmathern <60901087+tmathern@users.noreply.github.com> Date: Sat, 29 Aug 2026 05:50:45 -0700 Subject: [PATCH 14/17] fix: Remove NullParameter and InvalidBufferSize tags Removed unused error tags from the list, as they were wrongfully added --- src/c2pa/c2pa.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index 66831640..67e83004 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -604,8 +604,6 @@ def _swap_handle(self, new_handle): _PRE_CONSUME_ERROR_TAGS = ( "UntrackedPointer:", "WrongPointerType:", - "NullParameter:", - "InvalidBufferSize:", ) # An error tag starts the message or follows this one wrapper. From a912ddb90086ec065b9ebc308606580162409cc5 Mon Sep 17 00:00:00 2001 From: tmathern <60901087+tmathern@users.noreply.github.com> Date: Sat, 29 Aug 2026 07:19:02 -0700 Subject: [PATCH 15/17] fix: Refactor --- c2pa-2559-review-reasoning.md | 680 ++++++++++++++++++++++++++++++ pr312-fix-plan.md | 240 +++++++++++ pr312-verified-plan.md | 257 +++++++++++ src/c2pa/c2pa.py | 61 ++- tests/test_unit_tests.py | 143 ------- tests/test_unit_tests_threaded.py | 94 ++++- 6 files changed, 1317 insertions(+), 158 deletions(-) create mode 100644 c2pa-2559-review-reasoning.md create mode 100644 pr312-fix-plan.md create mode 100644 pr312-verified-plan.md diff --git a/c2pa-2559-review-reasoning.md b/c2pa-2559-review-reasoning.md new file mode 100644 index 00000000..7349b7b1 --- /dev/null +++ b/c2pa-2559-review-reasoning.md @@ -0,0 +1,680 @@ +# c2pa-rs PR #2559 — adversarial review, downstream impact, and c2pa-python #312 re-review + +Full reasoning record. Everything below is **static analysis only** — there is no Rust +toolchain in the working container (`static.rust-lang.org` is not on the egress allowlist), +so nothing here was compiled or executed. Where a claim rests on an assumption rather than +on traced code, that is stated inline. + +Artifacts read: + +| Repo | Ref | How | +|---|---|---| +| `contentauth/c2pa-rs` | head `145b754`, base `b3cd390` (PR #2559, branch `gpeacock/c_ffi_opaque_ids`) | full tarball + diff | +| `contentauth/c2pa-cpp` | `main` HEAD | full tarball | +| `contentauth/c2pa-python` | `main` HEAD | full tarball | +| `contentauth/c2pa-python` | `385b28c` (PR #312, branch `mathern/error-slot-sentinel`, base `mathern/sigsev-sigabort`) | full tarball + diff vs main | + +--- + +## Part 1 — What #2559 actually does + +### 1.1 The change + +`PointerRegistry` previously keyed its `HashMap` on the **real address** of every tracked +allocation. The pointer handed to C *was* the object's address. #2559 splits the key space +in two: + +- **Handles** (`C2paReader`, `C2paBuilder`, `C2paSigner`, `C2paSettings`, `C2paContext`, + `C2paContextBuilder`, `C2paStream`, `C2paHttpResolver`) are now keyed by a synthetic + **opaque id** produced by `scramble_to_odd_id`, and it is the id — not the address — + that crosses the FFI boundary. `track_by_id` (utils.rs:~100-120). +- **Buffers** returned by `to_c_string` (utils.rs:512) and `to_c_bytes` (utils.rs:547) stay + keyed by their **real address**, because C dereferences them. `track_by_address` + (utils.rs:117-125). + +`scramble_to_odd_id(counter) = (2·counter + 1) · M mod 2^N`, with +`M = 0x9e3779b97f4a7c15` on 64-bit and `M = 0x9e3779b9` on 32-bit. Producing only odd +values is what keeps the two key spaces from overlapping: real allocations are assumed +always even. + +`validate_pointer` / `untrack_pointer` change return type from `Result<(), Error>` to +`Result<*mut T, Error>`, so every deref macro now goes id → registry → real address → +deref. `PointerRegistry::validate` is renamed to `resolve`. Two new macros, +`deref_mut_option!` and `deref_mut_option_or_return!` (macros.rs:331-357, 358-380), give a +non-erroring `Option` form for cleanup paths. + +### 1.2 The vulnerability it fixes — confirmed + +Under address keying: object X at address P is freed (removed from the map, `Box` dropped); +a new object Y of the same type is later allocated at P and tracked; a **stale handle P now +resolves to Y**. Same type, wrong live instance. That is a textbook ABA, and it is real. + +The fix works for that case: ids are drawn from a monotonic counter and are never recycled, +so a stale id fails lookup with `UntrackedPointer:`. + +An extra detail that makes the old bug much more likely than it first looks, and that +matters for the Python analysis in Part 3: consuming calls such as +`c2pa_builder_with_archive` do `untrack_or_return_null!` → `Box::from_raw` → drop → +`box_tracked!(new Box)`. The new `Box` is the same size as the one just freed, and glibc's +tcache is LIFO, so **the replacement handle very often had the identical numeric value as +the consumed one**. Downstream code holding the old pointer was frequently, not rarely, +holding a pointer to the live replacement. + +--- + +## Part 2 — Findings on #2559 + +Severity, anchor, and where each comment can physically go. GitHub only allows inline +comments on lines inside a diff hunk, so a few of these are forced into the review body. + +### F1 — MEDIUM. "Ids are never reused" is false on 32-bit; the period is 2^(N−1) + +`(2c+1) mod 2^N` takes only **2^(N−1)** distinct values, because `c` and `c + 2^(N−1)` map +to the same value. Multiplying by an odd `M` is a bijection and does not restore the lost +bit. So the id sequence has period 2^(N−1), not 2^N. + +- 64-bit: 2^63. Unreachable. +- wasm32-unknown-emscripten (a supported target — `c2pa_c_ffi/Cargo.toml` has a + `cfg(target_arch = "wasm32")` dependency table, and `maybe_send_sync.rs` exists for it): + **2^31 ≈ 2.1e9 handles**, reachable in a long-running process. At wrap, the exact ABA + this PR fixes returns, silently. + +The doc at utils.rs:55-59 states distinct counters are "mathematically guaranteed" to +scramble to distinct odd ids. Off by exactly this factor of two. + +Secondary point for the same comment: the PR argues that a stray deref of a handle produces +an immediate obvious crash. On x86-64 that holds — ids land in non-canonical address space +(high bits derived from `0x9e3779b9…`), so the deref faults. On wasm32 it does **not** +generally hold: ids are uniformly distributed 32-bit odd values, and any id smaller than +the current linear-memory size falls inside valid memory and reads garbage instead of +trapping. + +→ **Inline, utils.rs:60** (`fn scramble_to_odd_id`, inside the `@@ -33,33 +36,97` hunk). +Ask for the real bound in the doc plus a hard stop on 32-bit once the counter passes 2^31. + +### F2 — MEDIUM. The odd/even non-collision invariant is asserted, never enforced, and rests on the allocator rather than on Rust + +utils.rs:51-54 justifies non-collision with "real Rust allocations always land on at least a +2-byte boundary, so their addresses are always even." + +Both `track_by_address` callers allocate **align-1** memory: `CString::into_raw` +(utils.rs:512) produces a `Vec`, and `to_c_bytes` (utils.rs:547) produces a +`Box<[u8]>`. Rust guarantees only `align_of::() == 1`. The claim is true in practice +only because `std`'s `System` allocator forwards to malloc/dlmalloc, which align to 8 or +16 — an allocator property, not a language one. A downstream `#[global_allocator]` (bump +and arena allocators are common in wasm builds) can hand back odd addresses. + +If it ever broke, the failure would be **silent, not an error**: `track_by_id`'s +`tracked.insert(id, …)` (utils.rs:114) would overwrite the string's entry, drop its +`CleanupFn` (leak), and make `c2pa_free(that_string)` free the *object* instead. + +→ **Inline, utils.rs:117** (`fn track_by_address`). Suggest +`debug_assert_eq!(real_addr & 1, 0)` there, and checking `insert`'s return value in +`track_by_id` so a collision is detected rather than assumed away. + +### F3 — MEDIUM. The ABA class is only half fixed, and the new doc does not say so + +`to_c_string` / `to_c_bytes` stay address-keyed. A stale `char*` still resolves to whatever +buffer lands at that address next, so `c2pa_free` on it can free a *different* live string. +Same ABA, and undetectable in the CString→CString case since the `TypeId` check passes. + +The keying cannot change — C dereferences these — but the registry doc at utils.rs:66-84 +explains why handles are safe without noting that the address-keyed half remains exposed, +and the PR description reads as if ABA is closed generally. + +→ **Inline, utils.rs:77-84** (the `track_by_address` bullet in the registry doc comment). + +### F4 — MEDIUM. Breaking Rust API change, source-compatible in the dangerous direction + +`validate_pointer` and `untrack_pointer` change signature; `PointerRegistry::validate` is +renamed to `resolve`. Both functions are re-exported at the crate root +(`cimpl/mod.rs:72-75`) and `cimpl::utils` is a `pub mod` reachable through +`pub use cimpl::*` in `lib.rs`. So this is a breaking change to the `c2pa-c-ffi` crate's +Rust API. + +The dangerous part: existing downstream `untrack_pointer(p)?;` **still compiles**. `*mut T` +is not `#[must_use]`, so the returned real pointer is silently discarded, and the caller's +subsequent `Box::from_raw(p)` operates on the handle id. UB with no compile error. + +`.github/workflows/semver-checks.yml` lists `c2pa-c-ffi` as a public-API crate but only +runs on PRs targeting `stable` / `v0.*`, so it will **not** fire on this PR against `main`. +It will surface at the release PR instead. + +→ **Review body** for the semver/CHANGELOG point (`c2pa_c_ffi/CHANGELOG.md` has an empty +`## [Unreleased]`), plus **inline `#[must_use]` asks at utils.rs:316 and utils.rs:343**. + +### F5 — MEDIUM. Nothing tests the new invariants + +Codecov reports 83.6% patch coverage, 7 uncovered lines in `utils.rs`. Every test change in +the PR is mechanical (`untrack_pointer(...).unwrap()` adjusted for the new return type). +Grepping the test module in `cimpl/utils.rs` finds no reference to `scramble`, `odd`, +`next_id`, `track_by_id`, or `track_by_address`. + +Nothing asserts: + +- an id differs from the real address; +- ids are odd; +- a stale handle fails `resolve` after its address is reused by a new same-type object — + **the actual regression test for the bug in the title**, and it needs no threads: track, + `cimpl_free`, `track_box` a new `T`, assert `validate_pointer(old_id).is_err()`; +- wrong-type resolve returns `WrongPointerType`; +- `free` on a stale id returns −1. + +→ **Inline, utils.rs:~620** (the `@@ -543,14 +620,15` test-module hunk). + +### F6 — LOW. One missed call site: `C2paStream::extract_context` (c2pa_stream.rs:110) + +Still does `Box::from_raw(self.context)` on what is now a handle id, and never untracks, so +the registry entry persists. + +Honest severity: `StreamContext` is a unit struct (c2pa_stream.rs:27), i.e. a ZST, so +`Box` drop never calls the allocator — a non-null value is trivially aligned for +align-1, and this will not crash or double-free today. It also has no in-repo callers. But +it is `pub` via `pub use c2pa_stream::*`, and it is precisely the pattern the PR swept for. +Delete it or route it through `untrack_pointer`. + +→ **Review body only** — line 110 falls outside every diff hunk. + +### F7 — LOW. `resolve` / `untrack` / `free` are `pub` on a `pub` struct in a `pub` module + +The premise of the change is that the real address never leaves the registry, yet +`pub fn resolve(&self, id: usize, …) -> Result` lets any downstream crate turn +a handle back into an address. `pub(crate)` preserves the property. + +→ **Inline, utils.rs:128.** + +### F8 — LOW. The TOCTOU window is narrowed, not closed + +`resolve` drops the `MutexGuard` before returning; the caller then dereferences the real +address outside the lock (macros.rs:248, 304, 354, 378). A concurrent `cimpl_free` of the +*same* handle between resolve and deref is still a use-after-free. The PR fixes +wrong-object; same-object-freed-underneath remains. Pre-existing and arguably out of scope, +but the PR body reads as though the threading hazard is handled. + +→ **Inline, macros.rs:304.** + +### F9–F11 — NITs + +- Error messages lost the real address: `wrong_pointer_type(id as u64)` / + `untracked_pointer(id as u64)` (utils.rs:~136-140) report the opaque id, which means + nothing to a debugger. In the `wrong_pointer_type` arm the real address is in hand. + → inline utils.rs:136-140. +- `deref_mut_option!` is `#[macro_export]` (macros.rs:346) while its own doc calls it + internal-only. `#[doc(hidden)]` at minimum. → inline macros.rs:346. +- `deref_or_return!` / `deref_mut_or_return!` still evaluate `$ptr` twice + (`ptr_or_return!($ptr, …)` then `validate_pointer($ptr)`), whereas the two new macros + correctly bind it once. Harmless for current call sites (all simple locals or casts), but + both lines are already open in this diff. → inline macros.rs:246-247, 302-303. + +### 2.1 Cleared while reviewing — checked and dismissed + +- **All 79 `extern "C"` fns scanned programmatically.** Every parameter typed + `*mut C2paStream / C2paSigner / C2paBuilder / C2paReader / C2paSettings / C2paContext / + C2paContextBuilder / C2paHttpResolver` passes through a `deref_*`, `untrack_or_return_*`, + or `cimpl_free!` macro. No unvalidated handle params remain. +- The `&mut *stream` / `&mut *source` / `&mut *dest` occurrences at c_api.rs:1888, 1927, + 1969, 2011-2012, 2061, 2482 are **reborrows of the macro-produced `&mut C2paStream`**, not + raw derefs. They look like missed call sites and are not. +- The only two remaining raw-pointer derefs in the crate are c_api.rs:5077 + (`&*(context as *const AtomicU32)`, a test counter) and json_api.rs:71 (`&*signer`, an + `Arc` deref). Neither is a tracked handle. +- `TestC2paStream::reader` and `seeker` already used `deref_mut_or_return_int!` on main; + only `writer` needed the fix. Verified against the diff hunks. +- `to_c_bytes` returns NULL for empty input (utils.rs:543), so no `NonNull::dangling()` + value of `1` can ever be tracked by address and collide with the odd id space. +- `drop_c_stream`: the `if let Some(real_stream)` binding scope ends before + `cimpl_free(c_stream)`, so there is no live `&mut` across the free. +- `test_c2pa_create_stream` frees `context` exactly once — `c2pa_release_stream` does not + touch the context. +- `untrack`'s `tracked.get(&id)` followed by `tracked.remove(&id)` inside the matched arm: + no scrutinee binding is used in the arm body, so NLL should end the immutable borrow + before the `remove`. Should compile; CI would catch it otherwise. The destructuring + `let (real_addr, _, _) = …` drops the `CleanupFn`, which for `track_box` captures only a + `usize` — harmless. +- `next_id` starts at 0, so the first id is `1 · M = 0x9e3779b97f4a7c15`, non-zero and odd. + `Ordering::Relaxed` on `fetch_add` is fine: the RMW is atomic, so values are unique + regardless of ordering. +- `arc_tracked!` is unused, so the pre-existing `untrack_or_return!` → `Box::from_raw` + mismatch for `Arc`-tracked entries is not reachable. Not this PR's concern. +- `mergeable_state` was `unstable`; the GitHub API rate-limited before the check-runs list + could be read, so the failing check is unidentified. Codecov's patch gate at 83.6% is the + likely candidate but that is a guess. + +--- + +## Part 3 — Downstream impact + +### 3.1 c2pa-cpp — one confirmed hard break + +`tests/c-app-test/file_stream.h:114`: + +```c +int close_file_stream(C2paStream *stream) +{ + if (stream == NULL) { return -1; } + FILE *file = (FILE *)stream->context; // <-- breaks + int result = fclose(file); + c2pa_release_stream(stream); + return result; +} +``` + +`stream` is the pointer returned by `c2pa_create_stream`, which is now an opaque id. On +x86-64 that address is non-canonical, so this is an immediate SIGSEGV — exactly the +"obvious crash" the PR intends, but in a downstream consumer rather than internal code. + +This is the empirical proof for a review point worth adding to #2559: `C2paStream` is +`#[repr(C)]`, cbindgen emits its fields into `c2pa.h`, and the header therefore advertises +a layout the returned pointer no longer has. **Ask for `C2paStream` (and the other handle +types) to go into cbindgen's `opaque_types`** — that converts this whole class of downstream +break from a runtime fault into a compile error. c2pa-cpp's own fix is to keep the `FILE*` +alongside the handle instead of reading it back out. + +Everything else in c2pa-cpp is clean: + +- The C++ wrappers (`include/c2pa.hpp`, `src/c2pa_*.cpp`) only store handles and hand them + back to `c2pa_free` / `c2pa_release_stream`. No field access, no arithmetic, no use as map + keys, no `unique_ptr`, no `delete`. +- Stream contexts it passes in — `reinterpret_cast(&istream)` at + c2pa.hpp:571, 633, 693 — are **C++-owned and never enter the registry**. `c2pa_create_stream` + stores the context verbatim and the C++ callbacks cast it straight back. Untouched by #2559. + (The `deref_mut_or_return_int!` on `context` inside `c2pa_stream.rs` applies only to the + Rust-side `TestC2paStream` helper.) + +**wasm relevance:** `Makefile:151` downloads a `wasm32-unknown-emscripten` build from +c2pa-rs releases, pinned at `CMakeLists.txt:20` → `C2PA_VERSION "0.90.16"`. That is the +32-bit target where F1 applies: 2^31 id period, and stray derefs that read garbage instead +of trapping. + +### 3.2 c2pa-python (main) — net leak fix, one latent footgun + +The binding's ownership model rests on *"a guarded free is a real free if ours, a no-op if +not"* (`ManagedResource._free_native_ptr`, c2pa.py:268-289; `_release_handle`, 341-349). +Under address keying that was only **probabilistically** true, for the tcache reason in §1.2: +a stale free could hit the live replacement, or a recycled object on another thread. That is +a use-after-free, not a leak. After #2559 the old id is dead forever and `c2pa_free` +deterministically returns −1. The comment at c2pa.py:490-493 about *"races a recycled address +in other threads"* describes precisely the hazard this closes. +`tests/test_unit_tests_threaded.py` is where it would have surfaced. + +**The error-tag routing survives unchanged.** `_PRE_CONSUME_ERROR_TAGS` matches on +`"UntrackedPointer:"` / `"WrongPointerType:"` (c2pa.py:421); #2559 only swaps the numeric +value inside those messages, not the tag text. The retain-vs-consume decision in +`_raise_consume_failure` behaves identically. + +Leak behaviour changes in two small ways, both benign: + +1. The retain branch fires more often — a stale handle reaching native is now *always* + rejected with `UntrackedPointer:`, so the Python object stays `ACTIVE` with a dead + handle. Nothing leaks in Rust; the previous behaviour (succeeding against a recycled + object) was strictly worse. +2. Ids are never recycled, so a genuinely leaked handle now leaks faithfully and its + registry entry persists. Address recycling used to clean some of these up by accident. + **A latent leak — e.g. `__del__` not firing under a reference cycle — may become visible + for the first time in soak/perf runs.** That is a diagnostic win, not a new regression, + but it is worth expecting. + +**Latent footgun:** `class C2paStream(ctypes.Structure)` at c2pa.py:658-692 declares the real +`_fields_` (`context`, `reader`, `seeker`, `writer`, `flusher`), unlike every other opaque +type in the file which uses `_fields_ = []`. Nothing dereferences it today — +`c2pa_create_stream` is called with `context=None` (c2pa.py:2003-2009) and the callbacks +close over a weakref — so there is no live bug. But it is the same shape as the c2pa-cpp +break. Change it to `_fields_ = []`. + +**Checked and clear:** `_convert_to_py_string` (c2pa.py:1220-1254) and the mime-type array +paths operate on `to_c_string` / `to_c_bytes` pointers, which stay address-keyed. +`ctypes.addressof(data.contents)` at c2pa.py:1879 is the read callback's real `*mut u8` +buffer, not a handle. `if not handle` truthiness is safe since ids are never 0. No +pointer-identity maps, no arithmetic, no reconstruction of pointers from stored ints. + +--- + +## Part 4 — c2pa-python PR #312 re-review + +PR #312, *"fix: Put a sentinel in the native thread local error slot"*, head `385b28c`, +base `mathern/sigsev-sigabort` (not `main`), 8 commits, approved by ale-adobe, awaiting +ok-nick. + +### 4.1 What it does + +`_MARKER_ADDR = 1` is passed to `c2pa_free` to plant a known error into the thread-local +`LAST_ERROR` slot. The exact text is **learned at import** rather than hardcoded +(`_learn_sentinel_no_native_error_text`, c2pa.py:1271-1296), since the format is a native +implementation detail. `_invoke_consume` marks the slot before every consuming call +(c2pa.py:594), and `_read_native_error` re-marks after reading (c2pa.py:899), so an error is +consumed exactly once by the caller that observes it. + +### 4.2 Is #312 still warranted given #2559? — split answer + +**The sentinel core: yes, and #2559 makes it *more* necessary.** + +`LAST_ERROR` stickiness is orthogonal to how the registry is keyed. #2559 does not clear the +slot, does not change thread-locality, and does not change the message text for a failed +free — so `_learn_sentinel_no_native_error_text()` keeps working unchanged. + +Second-order effect worth adding to the PR description: after #2559, a guarded free of a +dead handle **always** returns −1 and **always** writes `UntrackedPointer:` into the slot. +Under address keying, a fraction of those frees silently succeeded and set nothing, because +the address had been recycled. So pre-consume-tag pollution of the sticky slot becomes +strictly more frequent once #2559 lands, and the misclassification #312 fixes gets more +likely, not less. + +Mapping the PR body's two motivations: + +| Motivation in #312 body | Status after #2559 | +|---|---| +| Stale tag from a finished task on a pooled worker thread | **Untouched.** This is the load-bearing one. | +| Address reuse: stale free finds a live entry and destroys another thread's object | **Eliminated.** | + +**The leak flip: warranted today, obsolete once #2559 ships.** + +This is the substantive difference from `main`. On `main`, `_raise_consume_failure`'s +"no error in the slot" branch did `self._release_handle()` — free defensively. On #312 +(c2pa.py:648-658) it does `_teardown(free_handle=False)`, and the justification is verbatim +*"a free here can race a recycled address in other threads."* The same reasoning appears in +the non-tag branch at 641-646. + +That is precisely and only the hazard #2559 removes. #312 trades a possible UAF for a +certain leak — which the PR body concedes: *"On any consume failure where the verdict is not +certain, this takes the consumed branch, meaning leaks could appear."* Once a +#2559-containing c2pa-rs is the floor, both branches can revert to `_release_handle()` and +recover the leak, **independently of the 0.91.0 always-consumed contract the PR body is +waiting on**. Recommendation: land #312 as-is, with a TODO / issue link on those two +branches so it is not forgotten — the existing note points at 0.91.0, which is a different +mechanism arriving later. + +### 4.3 New interaction to flag on #312 — `_MARKER_ADDR = 1` + +```python +# Unaligned address passed to c2pa_free to plant a marker +# in the native error slot. +# Never a real handle: allocations are aligned, and the Python +# layer only passes real handles or this constant to c2pa_free. +_MARKER_ADDR = 1 +``` + +That justification is exactly the invariant #2559 inverts. After it, registry keys are no +longer all real addresses: handle ids are `(2c+1)·M mod 2^N`, i.e. **always odd** — the same +namespace as `1`. + +Multiplication by an odd constant is a bijection mod 2^N, so there is exactly one counter +value per period producing id `1`. Solve `(2c+1)·M ≡ 1 (mod 2^N)`, i.e. `2c+1 ≡ M⁻¹`: + +| Width | `M` | `M⁻¹ mod 2^N` | counter yielding id `1` | +|---|---|---|---| +| 64-bit | `0x9e3779b97f4a7c15` | `0xf1de83e19937733d` | `8714256306465913246` ≈ 2^63 | +| 32-bit | `0x9e3779b9` | `0x144cbc89` | **`170286660`** ≈ 2^28 | + +Reachability, stated honestly: **c2pa-python ships 64-bit wheels only.** +`scripts/download_artifacts.py` maps to `x86_64` / `aarch64` across +`apple-darwin`, `pc-windows-msvc`, `unknown-linux-gnu` — no i686, no wasm. So this is **not +reachable for Python in practice**. It is reachable on c2pa-cpp's emscripten path, where +170M tracked handles is a soak-test-scale number rather than an astronomical one. + +Consequence if it ever hit: `_mark_sentinel_no_native_error()` → `c2pa_free(1)` frees a +**live object** belonging to another thread, returns 0, and sets no error. And it is called +on every `_invoke_consume` and every `_read_native_error`, so it is a hot path. + +Fix: pick a marker that is outside every key space under **both** the current v0.90 +scheme and #2559, rather than one justified by whichever scheme happens to be loaded. That +is `_MARKER_ADDR = 8` — see §5.1 for the two independent properties that make it safe under +each, the constant-derived assert that replaces the hardcoded `"0x1"` at c2pa.py:1297, and +why `0` must not be used. + +### 4.4 Carried over unchanged into #312 + +- `C2paStream._fields_` at c2pa.py:833-844 still declares the real layout while every other + opaque type uses `_fields_ = []`. Still no live bug (`c2pa_create_stream` is called with + `context=None` at c2pa.py:2282-2288), still the same shape as the confirmed c2pa-cpp + break at `file_stream.h:114`. Still worth `_fields_ = []`. +- `_PRE_CONSUME_ERROR_TAGS` grew to four entries (c2pa.py:567-572), adding `NullParameter:` + and `InvalidBufferSize:`. All four still match after #2559; `resolve()` returning + `null_parameter("pointer")` for id 0 keeps the `NullParameter:` tag meaningful, and ids + are never 0, so it only fires on genuine nulls. + +--- + +## Part 5 — Making the plan work against v0.90 *and* #2559 + +**Revision note.** The first version of §4.3 recommended `_MARKER_ADDR = 2` justified by +"handle ids are always odd." That reasoning only holds *after* #2559 and says nothing about +v0.90. This part replaces it with a set of choices that are correct under both, and +separates the items that need no version-awareness at all from the one that genuinely does. + +Three native behaviours are in play: + +| | key space | ownership on a failed consuming call | +|---|---|---| +| **v0.90.x (today)** | real addresses only, recyclable | ambiguous | +| **v0.90.x + #2559** | odd synthetic ids for handles, real addresses for buffers | ambiguous | +| **v0.91.0 (announced)** | as above | always consumed by native | + +Only the *middle* column differs from today in a way that changes a Python decision, and +only for one branch. Everything else can be made version-blind. + +### 5.1 The marker address — version-blind, and worth changing now + +`_MARKER_ADDR` must be a value that the registry can never legitimately hold as a key, +under any of the three columns. Two independent properties give that: + +1. **Below the first page.** No allocator returns an address in the null page, under any + scheme, so it can never be a real allocation and therefore never an address-keyed buffer + entry. This is the property that covers v0.90, where *all* keys are addresses. +2. **Even.** Under #2559, ids are `(2c+1)·M` with `M` odd, so every id is odd *by + construction* — not by allocator convention. An even value can therefore never be a + synthetic id. + +`1` has property 1 but not property 2. Use **`_MARKER_ADDR = 8`**: it satisfies both, and +each property alone is sufficient for one of the two schemes, so the marker is safe whether +or not #2559 is present in the loaded library. Do not use `0` — `PointerRegistry::free` +short-circuits `key == 0` to `Ok(())` (utils.rs), so a zero marker would return 0 and set no +error, silently disabling the whole mechanism. + +Rewrite the justification comment accordingly: + +```python +# Address passed to c2pa_free purely to plant a known marker in the +# native thread-local error slot. +# +# Safe under every native key scheme: +# - below the first page, so never a real allocation and never an +# address-keyed buffer entry; +# - even, and synthetic handle ids are odd by construction, so never +# a handle id either. +# Must not be 0: the registry treats a 0 key as a successful no-op. +_MARKER_ADDR = 8 +``` + +**Derive the assert rather than hardcoding the literal.** c2pa.py:1297 currently reads +`assert "0x1" in _NO_NATIVE_ERROR_TEXT`, which both hardcodes the constant and is a loose +substring test (`"0x1"` matches `0x1a2b…`). Replace it with something tied to the constant +and anchored, and move it inside `_learn_sentinel_no_native_error_text()` — which also +answers ale-adobe's review comment: + +```python +def _learn_sentinel_no_native_error_text(): + ... + if f"0x{_MARKER_ADDR:x}" not in text: + raise ImportError( + "c2pa native library's untracked-pointer error text no longer " + "includes the planted address; the error-slot marker assumption " + "no longer holds") + return text +``` + +The message text itself needs no version handling: v0.90 formats +`untracked_pointer(ptr as u64)` and #2559 formats `untracked_pointer(id as u64)`, and for +the marker the value passed *is* the key in both cases, so the learned string is identical. +Learning at import already makes this robust; the only thing that was version-specific was +the choice of constant. + +### 5.2 The free-vs-leak branches — the one place version-awareness is needed + +`_raise_consume_failure`'s two `_teardown(free_handle=False)` branches (c2pa.py:641-646 and +648-658) leak on an ambiguous failure, to avoid a defensive free racing a recycled address. +That trade is **correct on v0.90 and unnecessary after #2559**. + +Before building any machinery for this, check the release sequencing, because it may be +moot: #2559 targets `main`, and the announced always-consumed contract is v0.91.0. If both +ship in 0.91.0, there is never a release where ids are opaque *and* ownership is ambiguous — +the middle column of the table above never exists — and the right answer is simply to leave +#312's leak branches alone permanently, since under always-consumed they are correct by +contract rather than as a workaround. **Resolve that question first.** Everything in the +rest of this subsection is contingent on the middle window being real. + +If it is real, detect the scheme behaviourally rather than by version string. A version +parse has to encode which release contains #2559 and breaks on backports; a behavioural +probe describes the property it actually depends on: + +```python +def _detect_opaque_handles(probes=4): + """True when the native library returns synthetic handle ids rather + than real addresses. + + Synthetic ids are odd by construction; real allocation addresses are + aligned to at least 8 bytes by every allocator this library ships + against. Requiring every probe to come back odd means a stray odd + address cannot flip the verdict, and any failure falls to the + conservative (address-keyed) answer. + """ + handles = [] + try: + for _ in range(probes): + h = _lib.c2pa_settings_new() + if not h: + return False + handles.append(h) + return all( + ctypes.cast(h, ctypes.c_void_p).value & 1 for h in handles) + except Exception: + return False + finally: + for h in handles: + _lib.c2pa_free(h) +``` + +`c2pa_settings_new` is `box_tracked!(C2paSettings::new())` — a single `Box`, no I/O, present +in both the v0.90 FFI and the #2559 head. Allocating several before freeing any prevents the +allocator from handing back the same address each round, which would make the probe a test +of one address rather than of the scheme. + +Failure directions are asymmetric and the probe is oriented safely: + +- **False negative** (says address-keyed when it is id-keyed): keeps the leak branch. The + status quo of #312. Harmless. +- **False positive** (says id-keyed when it is address-keyed): re-enables the defensive + free, which is the UAF #312 exists to prevent. This requires *every* probe to return an + odd address — impossible with malloc/dlmalloc alignment, and made vanishingly unlikely by + the `all()` over several probes. Any exception path also returns `False`. + +Then gate only the branches, leaving the tag routing untouched: + +```python +_HANDLES_ARE_OPAQUE = _detect_opaque_handles() + +# ... inside _raise_consume_failure, both ambiguous branches: +if _HANDLES_ARE_OPAQUE: + # Freeing a dead handle is a guaranteed no-op: ids are never + # recycled, so this cannot reach another thread's object. + self._release_handle() +else: + # Address keys are recyclable; a defensive free could destroy a + # live object at a reused address. Accept the leak. + self._teardown(free_handle=False) +``` + +**Import ordering.** Run `_detect_opaque_handles()` *before* +`_learn_sentinel_no_native_error_text()`. The probe's frees succeed and set no error, so +they cannot disturb the slot, but learning the sentinel last leaves the error slot in the +known state the rest of the module assumes. + +**Test it under both.** A unit test can force each branch by monkeypatching +`_HANDLES_ARE_OPAQUE`, so the leak path and the free path are both covered on a single +native build. Add one assertion that the probe itself agrees with `sdk_version()` on the CI +matrix, so a future native change that breaks the odd-id invariant is caught loudly rather +than silently downgrading to the leak branch forever. + +### 5.3 Items that are already version-blind + +No change needed for compatibility; they behave identically under all three columns. + +- `C2paStream._fields_ = []` (c2pa.py:833-844). Nothing derefs it under either scheme; the + change only removes the ability to. +- `_PRE_CONSUME_ERROR_TAGS` (c2pa.py:567-572). All four tags are produced by both v0.90 and + #2559, with the same text. +- The c2pa-cpp fix to `tests/c-app-test/file_stream.h:114` — keeping the `FILE*` alongside + the handle instead of reading `stream->context` back — is correct under both, since it + simply stops depending on the struct layout. +- Every c2pa-rs finding in Part 2 is a comment on #2559 itself and has no v0.90 dimension. + +### 5.4 One sequencing constraint created by the cbindgen ask + +The recommendation to move `C2paStream` and the other handle types into cbindgen's +`opaque_types` (Part 2, review-body item) turns the c2pa-cpp break from a runtime segfault +into a compile error. That is the desired outcome, but it means **`file_stream.h` stops +compiling the moment c2pa-cpp bumps to a release containing the change**. Land the c2pa-cpp +fix first, or land both in a coordinated bump, and call the header change out in the +c2pa-c-ffi changelog alongside the F4 semver note — it is a source-breaking change for any C +consumer that touches those structs, not only for this one test helper. + +--- + +## Part 6 — Consolidated action list + +**On c2pa-rs #2559** + +1. Inline utils.rs:60 — F1, period is 2^(N−1); 32-bit wraps at 2^31; the crash-loudly + argument does not carry to wasm32. +2. Inline utils.rs:117 — F2, `debug_assert_eq!(real_addr & 1, 0)`; check `insert`'s return. +3. Inline utils.rs:77-84 — F3, document that address-keyed buffers remain ABA-prone. +4. Inline utils.rs:316 and utils.rs:343 — F4, `#[must_use]`. +5. Inline utils.rs:~620 — F5, add the stale-handle regression test. +6. Inline utils.rs:128 — F7, `pub(crate)` on `resolve` / `untrack` / `free`. +7. Inline macros.rs:304 — F8, note the residual resolve→deref window. +8. Inline utils.rs:136-140, macros.rs:346, macros.rs:246-247 / 302-303 — F9–F11 nits. +9. **Review body:** F6 (`extract_context`, outside all hunks); F4's semver/CHANGELOG point; + and the cbindgen `opaque_types` ask, citing the c2pa-cpp break as the motivating case and + noting the §5.4 sequencing constraint. + +**On c2pa-cpp** + +10. Fix `tests/c-app-test/file_stream.h:114` — keep the `FILE*` alongside the handle. Land + before, or with, the version bump that carries the cbindgen change. + +**On c2pa-python #312 — safe under v0.90 today, and after #2559** + +11. `_MARKER_ADDR = 8` with the two-property justification from §5.1. Not `1`, not `0`. +12. Derive the sentinel assert from `_MARKER_ADDR` and move it inside + `_learn_sentinel_no_native_error_text()` (also answers the open review comment). +13. `C2paStream._fields_ = []` at c2pa.py:833-844. + +**On c2pa-python — deferred, and only if the middle release window turns out to be real** + +14. Confirm with the c2pa-rs team whether #2559 and the always-consumed contract ship in the + same release. If yes, stop here and leave the leak branches permanently. +15. If no: add `_detect_opaque_handles()` (§5.2), gate the two ambiguous branches on it, + order it before the sentinel learn, and add both-branch coverage plus a probe-versus- + `sdk_version()` consistency check in CI. + +--- + +## Appendix — verification notes and limits + +- No Rust toolchain available; nothing compiled or run. Findings are static traces through + the tarballs listed at the top. +- GitHub's REST API rate-limited partway through, so the #2559 check-runs list was never + read. `mergeable_state: unstable` is all that is known about CI. +- The `untrack` borrow-check question (§2.1) is a reasoned NLL argument, not a compiler + result. +- The modular-inverse figures in §4.3 were computed directly (`pow(M, -1, 2**N)`) and + round-tripped: `((2c+1)·M) mod 2^N == 1` for both widths. +- Reachability claims about counter exhaustion assume one counter increment per tracked + handle, which matches `track_by_id` being the single id source for `track_box`, + `track_arc`, and `track_arc_mutex`. +- The `_detect_opaque_handles` probe in §5.2 is proposed, not tested. Its v0.90 side rests + on malloc/dlmalloc returning 8- or 16-byte-aligned addresses — an allocator property, the + same one F2 flags as unenforced upstream. That is acceptable here only because the probe + fails toward the conservative branch; it should not be reused anywhere the failure + direction is reversed. +- The release-sequencing question in §5.2 and item 14 is unresolved and cannot be settled + from the repositories alone. It is stated as an open question, not an assumption. diff --git a/pr312-fix-plan.md b/pr312-fix-plan.md new file mode 100644 index 00000000..97e0b159 --- /dev/null +++ b/pr312-fix-plan.md @@ -0,0 +1,240 @@ +# PR #312 — minimal fix plan + +**Target:** `src/c2pa/c2pa.py` on `mathern/error-slot-sentinel` (`df29505`). Line numbers from that revision. + +**Goal:** four small edits. Three fix defects confirmed by reproduction against the pinned +native build (v0.90.15); one makes the ownership logic harmless when v0.91 ships. Nothing +else changes. + +All four were applied to a working copy and re-verified — before/after output in each +section. + +--- + +## Fix 1 — After a consuming call has been issued, never free the handle + +**Why:** the ownership decision currently depends on the order in which the native library +validates arguments versus taking ownership, and that order is changing. + +| | `c2pa_reader_with_manifest_data_and_stream` | +|---|---| +| v0.90.15 / v0.90.16 | validate `format`/`stream`/`manifest_data`, **then** `untrack_or_return_null!(reader)` | +| `c2pa-rs` `main` (→ v0.91) | `untrack_or_return_null!(reader)` **first**; every early return drops it | + +Confirmed on the pinned build: + +``` +with_manifest_data_and_stream(size=0) -> "Other: InvalidBufferSize: 0 for 'manifest_data'" +c2pa_free(old reader) -> 0 => handle STILL TRACKED +``` + +So `_PRE_CONSUME_ERROR_TAGS` is right today and wrong after the bump: the same messages will +mean "native already dropped this" while the binding keeps the pointer and frees it later. +That is the recycled-address free the PR exists to prevent. + +Rather than teach the classifier to track the native ordering, remove ownership from the +classifier entirely. Under v0.91 the contract is "always consumed by native calls", so the +rule that is correct there and merely lossy on v0.90 is: **once `ffi_call` has been issued, +this binding never frees that handle.** + +### Edits + +`_raise_consume_failure` (lines 666–724) — all three branches collapse to the same action: + +```python + error = _read_native_error() + if error: + if ManagedResource._is_pre_consume_rejection(error): + logger.warning( + "%s: native call rejected the handle (%s); " + "marked consumed, not freed", + type(self).__name__, error) + self._teardown(free_handle=False) + _raise_typed_c2pa_error(error) + + logger.debug("%s: consuming call failed without setting error", + type(self).__name__) + self._teardown(free_handle=False) + raise C2paError(error_message.format("Unknown error")) +``` + +`_invoke_consume` (lines 657–664) — same rule; `ctypes.ArgumentError` is still re-raised +above, so any other exception means the call reached native: + +```python + except Exception as e: + # The call reached native, so ownership is native's to account for. + self._teardown(free_handle=False) + raise C2paError(error_message.format(e)) from e +``` + +Follow-on deletions, all mechanical: + +- `_raise_consume_failure`'s `previous_state` parameter and its two call sites + (lines 799, 817). +- `_invoke_consume`'s `reserved` keyword and its two call sites (lines 791, 809). +- `_abort_consume` (lines 745–753) is no longer reached from the failure classification. + It stays only for the `except` branches of `_consume_no_replacement` / `_consume_into`, + where the handle demonstrably never reached native. + +`_PRE_CONSUME_ERROR_TAGS` and `_is_pre_consume_rejection` stay exactly as they are. After +this change they only pick a log line. They no longer decide whether anything is freed, so +their correctness against a given native version stops mattering. + +### What this costs + +On v0.90 a pre-consume rejection leaves a handle the registry still tracks and we abandon +it — a bounded leak on an error path, in exchange for making a free of a possibly-recycled +address structurally impossible. On v0.91 it is simply correct. + +Retryability after a pre-consume rejection goes away: the resource is marked closed instead +of restored to ACTIVE. That promise is already false under v0.91, so it has to go regardless. +Update: + +- `Reader.with_fragment` docstring (lines 3234–3239) — drop the "can be retried" wording. +- `tests/perf/scenarios.py` — `with_fragment_pre_consume_rejection`. +- the unit test asserting the resource is restored (`test_pre_consume_rejection_restores_the_resource`). + +### Verified + +``` +Reader('image/jpeg', BytesIO(b'not an image')) -> NotSupported: type is unsupported +Reader(..., manifest_data=b"") -> Other: InvalidBufferSize: 0 for 'manifest_data' +``` + +Both raise the same typed errors as before; neither frees the handle. + +--- + +## Fix 2 — `_maybe_flush_pending` must re-register when a section blocks it + +**Why:** it returns early on `_in_native_section()` without calling +`_register_for_section_flush(self)`, so the deferral has no remaining flush path. +`Builder.sign` nests the guards — `signer._native_call()` (line 4313) inside +`self._native_call()` (line 4308) — so the inner exit runs while the outer section is still +open. A `signer.close()` from another thread is then stranded, and since +`_cleanup_resources` skips a CLOSED resource, `close()` and `__del__` are both no-ops +afterwards. Same for `_context_guard(self._context)` at line 4332. + +### Edit (lines 504–515) + +```python + def _maybe_flush_pending(self): + if is_foreign_process(self): + return + with self._state_lock(): + if self._pending_teardown is None: + return + if getattr(self, '_inflight', 0) > 0: + return + if _in_native_section(): + _register_for_section_flush(self) + return + free_handle, self._pending_teardown = self._pending_teardown, None + self._finish_teardown(free_handle) +``` + +Three changes: the `is_foreign_process` guard `_teardown` already has at line 438 (this +function is called from the section drain, where a child process would otherwise raise +`C2paError` out of an unrelated `with`); the missing registration; and moving +`_finish_teardown` inside the lock (Fix 3). + +### Verified + +Two `Context` objects, nested `_native_call`, `close()` from another thread: + +``` +before: pending=True handle set=True released=False (second close() also a no-op) +after: pending=None handle set=False released=True +``` + +The existing `test_context_close_during_sign_defers_teardown` covers only one nesting level +— adding an enclosing `with builder._native_call():` makes it fail before this edit. + +--- + +## Fix 3 — `_finish_teardown` idempotency + +**Why:** `_released` is set as the *first* statement of `_finish_teardown` (line 492) and +`_maybe_flush_pending` calls it outside the lock, so a second entrant that arrives before +that assignment runs the whole teardown again. This breaks the invariant +`test_concurrent_close_runs_release_once` states explicitly. + +### Edit (line 492) + +```python + if self._released: + return + self._released = True +``` + +Combined with running it under the lock (Fix 2), this closes both the duplicate `_release()` +and the narrow double-free window at line 496. + +### Verified + +Forced interleave, one resource, two threads: + +``` +before: _release ran 2 times -> ['B', 'A'] +after: _release ran 1 time -> ['A'] +``` + +--- + +## Fix 4 — Re-mark the slot after a failed free + +**Why:** `_free_native_ptr` logs a non-zero result and leaves +`UntrackedPointer: 0x` in the thread-local slot — and its own docstring calls that +"expected on the eager-free path". The next non-consuming failure that sets no error of its +own reads it back, with the wrong exception type. That is the defect this PR set out to +remove, displaced rather than fixed. + +### Edit (lines 385–390) + +```python + result = _lib.c2pa_free(ptr) + if result != 0: + logger.debug("c2pa_free returned %s for an untracked pointer ", result) + _mark_sentinel_no_native_error() + return result +``` + +### Verified + +``` +_free_native_ptr(0x9999) -> -1 +before: next unrelated failure -> _C2paOther: Other: UntrackedPointer: 0x9999 +after: next unrelated failure -> C2paError: unrelated later op failed: Unknown error +``` + +--- + +## Deliberately out of scope + +Raised in review, not fixed here — each is either subsumed or too large for this PR: + +- **`_abort_consume` reviving a resource with a queued teardown.** Reachable state + (`is_valid == True` with `_pending_teardown` set), but after Fix 1 it is no longer on the + failure-classification path. A one-line `if self._pending_teardown is not None: return` + can be added if the reviewer wants it; it is not required for correctness of the paths + that exist today. +- **`_release_handle` mutating state outside the lock** (lines 517–525). After Fix 1 its + last caller in the consume paths is gone. +- **`_native_section`'s `finally` masking the body's exception** (lines 1091–1092). Real, + but the fix touches the section generator's control flow; not worth the blast radius here. +- **`with_fragment` same-thread re-entrancy** (`RLock` + `blocking=False`, lines 3142, 3256). + Under Fix 1 a re-entered call no longer leads to a free of the reused handle. +- **Import-time `ImportError` from `_learn_sentinel_no_native_error_text`** (line 1450) and + the registry-mutex cost of planting the marker. Both are policy calls for the author. +- **Argument preflighting in Python** (rejecting `manifest_data=b""` and similar before the + FFI call). Worth doing, but it is a behavioural change across several public entry points + and Fix 1 already removes the ownership hazard it was proposed to close. + +--- + +## Landing order + +Fixes 2, 3 and 4 are independent of the native version and of each other; they can go in +first. Fix 1 carries the behaviour change and the test updates, and is the one that has to +land before v0.91. diff --git a/pr312-verified-plan.md b/pr312-verified-plan.md new file mode 100644 index 00000000..62559e1c --- /dev/null +++ b/pr312-verified-plan.md @@ -0,0 +1,257 @@ +# PR #312 — verified plan (compatible with v0.90.15 today and v0.91.0 next) + +Everything below was checked against running code, not read off the two source +documents. Evidence tier is marked per claim: **ran** (executed, read the output), +**read** (opened the source at a named revision), **inferred** (reasoned, not confirmed). + +Revisions used: + +| Repo | Ref | Meaning | +|---|---|---| +| c2pa-python | `9f62daf` (`mathern/error-slot-sentinel`) | branch under review | +| c2pa-rs | `stable` = `0.90.15` | what the binding loads today | +| c2pa-rs | `origin/main` = `0.91.0-dev` (`be7f5ea2`) | what ships next | +| c2pa-rs | `pr2559` = `145b754a`, base `b3cd390a` | opaque-ids PR, not merged | + +Local native library after `make rebuild`: `c2pa-c-ffi/0.90.15 c2pa-rs/0.90.15`. + +--- + +## What the adversarial pass changed + +Three corrections to the input documents, each of which moves the plan. + +### The release-sequencing question is answered, and the answer is the opposite of the guess + +The review document (§5.2, item 14) leaves open whether #2559 and the always-consumed +contract ship together, and recommends building a behavioural probe +(`_detect_opaque_handles`) if they do not. That machinery is not needed. + +**Read** — the always-consumed ordering came from PR #2344, merged 2026-07-23, and is +already on `main`, independent of #2559: + +``` +78b5b709 2026-07-23 fix: builder style c_ffi_api functions will now consistently + consume the self parameter (#2344) +``` + +**Ran** — `git merge-base --is-ancestor 78b5b709 stable` reports #2344 is *not* in the +0.90 line, and `main` is `0.91.0-dev`. **Read** — the reordered body is identical on +`origin/main` and on `pr2559`, and its base `b3cd390a` already contains it, so #2559 did +not introduce it. + +Both changes therefore arrive in the same 0.91.0 release. The middle column of the review +document's table — opaque ids *with* ambiguous ownership — never exists as a shipped +release. **Drop `_detect_opaque_handles` entirely.** It is roughly 30 lines of probe plus +a CI consistency check built for a window that does not occur, and its own author's note +concedes its v0.90 side rests on the same unenforced allocator assumption that finding F2 +criticises upstream. + +### The ordering flip is real, and it is the reason Fix 1 is not optional + +**Read** — `c2pa_reader_with_manifest_data_and_stream`, the two revisions side by side: + +```rust +// stable (0.90.15) — validate first, reader still tracked on early return +let format = cstr_or_return_null!(format); +let stream = deref_mut_or_return_null!(stream, C2paStream); +let manifest_bytes = bytes_or_return_null!(manifest_data, manifest_size, "manifest_data"); +untrack_or_return_null!(reader, C2paReader); + +// main (0.91.0-dev) — take ownership first, every early return drops it +let reader = untrack_or_return_null!(reader, C2paReader); +let format = cstr_or_return_null!(format); +``` + +**Ran** — against the pinned 0.90.15 library, a rejected call leaves the handle alive: + +``` +c2pa_reader_with_manifest_data_and_stream(reader, "image/jpeg", NULL, NULL, 0) + -> NULL, error = 'NullParameter: stream' +c2pa_free(reader) -> 0 # 0 = still tracked, native did NOT consume it +``` + +After the bump the same message means the opposite. A classifier that decides ownership +from message text is therefore correct today and wrong on 0.91.0. + +### The address-reuse hazard is certain, not probabilistic + +The review document argues from glibc tcache LIFO that a stale free "very often" hits the +live replacement. **Ran** — on this macOS build it is not "often", it is total: + +``` +allocate -> free -> allocate (same type), 200 trials +address reuse: 200/200 = 100% +``` + +So on v0.90 a defensive free of a handle whose ownership is uncertain reliably destroys +the object that took its address. This is the strongest available justification for the +leak-over-free trade, and it is what makes Fix 1 the correct direction rather than merely +a forward-compatibility hedge. + +--- + +## Confirmed by execution before writing any code + +- **Marker candidates.** `c2pa_free(0) -> 0` with an empty error string, so `0` silently + disables the sentinel; `c2pa_free(8) -> -1` with `Other: UntrackedPointer: 0x8`. + **Read** — `PointerRegistry::free` short-circuits `key == 0` to `Ok(())`, which is the + mechanism behind that result. +- **Handle shape on v0.90.** Eight consecutive `c2pa_settings_new()` handles were all real + addresses, all 256-byte aligned, none below the first page. Both properties the marker + argument depends on hold on the library we actually load. +- **Scramble arithmetic**, recomputed independently rather than taken from the document: + `scr(0) == scr(2^(N-1))` on both widths, so the id period is 2^(N-1), confirming F1; the + 32-bit counter producing id `1` is `170286660`; no id is ever even, on any input. +- **Fix 2 and Fix 3 defects**, both **read** in the current source: `_maybe_flush_pending` + (line ~511) returns on `_in_native_section()` without calling + `_register_for_section_flush(self)`, while `_teardown` (line 474) does register on the + same condition — the deferral has no remaining flush path. `_finish_teardown` writes + `self._released = True` as its first statement (line 492) and is called outside the lock, + so a second entrant arriving before that write repeats the whole teardown. + +## Already done on this branch — no action + +- The sentinel assert is **already** derived from the constant (`marker_hex = + hex(_MARKER_ADDR)`) and **already** inside `_learn_sentinel_no_native_error_text()`. + Review items 11-part-two and 12 are complete; the document describes an older revision. +- `_PRE_CONSUME_ERROR_TAGS` is back to two entries. Commit `9f62daf` removed + `NullParameter:` and `InvalidBufferSize:`, so §4.4's four-entry description is stale. + +## Pre-existing breakage this plan must absorb + +**Ran** — on a clean tree after `make rebuild`: **4 failed, 482 passed**. All four trace to +`9f62daf` removing the two tags without updating the tests that assert the old behaviour. +They fail independently of anything proposed here. + +``` +test_invalid_buffer_size_rejection_retains_the_handle AssertionError: the retained handle was dropped +test_null_parameter_rejection_retains_the_handle AssertionError: the retained handle was dropped +test_repeated_rejections_do_not_accumulate_handles 10 of 10 handles leaked +test_native_rejections_observed_from_the_library_still_classify +``` + +The version test that failed before the rebuild (`'0.90.16' not found in '0.90.15'`) was a +stale-artifact problem and is now green. + +--- + +## The plan + +### 1. Fix 1 — once a consuming call has been issued, never free the handle + +Collapse the three branches of `_raise_consume_failure` and the `except` path of +`_invoke_consume` to `self._teardown(free_handle=False)`. Ownership stops being derived +from error text; the tags survive only to select a log line. + +Correct on 0.91.0 by contract. On 0.90.15 it accepts a bounded leak on an error path in +exchange for making a free of a reused address structurally impossible — which the 200/200 +measurement shows is the real hazard, not a theoretical one. + +Mechanical follow-ons: drop `_raise_consume_failure`'s `previous_state` parameter and +`_invoke_consume`'s `reserved` keyword with their call sites; leave `_abort_consume` for +the two `except` paths where the handle provably never reached native. + +### 2. Fix 2 — register for section flush when a native section blocks the drain + +Add the missing `_register_for_section_flush(self)`, add the `is_foreign_process` guard +`_teardown` already has, and move `_finish_teardown` inside the lock. + +### 3. Fix 3 — make `_finish_teardown` idempotent + +Guard on `self._released` before setting it. With Fix 2's locking this closes both the +duplicate `_release()` and the double-free window. + +### 4. Marker address `1` -> `8` + +Not reachable on shipped Python wheels (64-bit only; the colliding counter is ~2^62), so +this is hardening, not a live bug — I would rather say that plainly than overstate it. It +costs one line and removes a justification comment that is already wrong in its reasoning: +"allocations are aligned" stops being the relevant property once ids are synthetic. `8` is +below the first page (safe under v0.90 address keys) and even (safe under #2559 odd ids). +Not `0`, for the measured reason above. + +### 5. `C2paStream._fields_ = []` + +**Read** — every other opaque type in the file uses `_fields_ = []`; this one declares the +real layout. Nothing dereferences it today (`c2pa_create_stream` is called with +`context=None`), so there is no live bug — but it is the same shape as the confirmed +c2pa-cpp break at `file_stream.h:114`, where reading `stream->context` back out segfaults +once the pointer is an opaque id. + +### 6. Tests — remove what is obsolete, keep what still holds + +Per your instruction, and split deliberately rather than deleting all four failures: + +Remove — these assert retain-and-retry, a promise deleted by `9f62daf` and contradicted by +the 0.91.0 ordering: +- `test_null_parameter_rejection_retains_the_handle` +- `test_invalid_buffer_size_rejection_retains_the_handle` +- `test_pre_consume_rejection_restores_the_resource` +- `test_repeated_rejections_do_not_accumulate_handles` (asserts no leak; Fix 1 trades that + away knowingly) +- `test_with_fragment_pre_consume_rejection_keeps_handle` + +Amend rather than delete — `test_native_rejections_observed_from_the_library_still_classify` +pins the wording the library actually emits, which is worth keeping. Drop its +`c2pa_reader_from_stream(None, None)` half, whose `NullParameter` message is deliberately +no longer classified, and keep the `c2pa_free(0x9999)` half. + +Keep untouched: `test_pre_consume_tags_still_match_the_native_wording`, +`test_pre_consume_tag_match_is_substring_not_prefix`, +`test_caller_text_quoting_a_tag_is_not_a_rejection`, +`test_concurrent_close_runs_release_once`. + +Add: a regression test for Fix 2 that wraps the existing +`test_context_close_during_sign_defers_teardown` in a second `_native_call()`, since one +nesting level does not exercise the stranded path. + +### 7. Perf — the part that will fail silently if skipped + +**Read** — `run_profile.py` gates the run on `leaked_bytes` at a 1.1x threshold, and +`scenario_reader_with_fragment_pre_consume_rejection` asserts: + +```python +if reader._handle is None: + raise AssertionError("handle was dropped on a pre-consume rejection; ...") +``` + +Fix 1 makes that assertion fire on every iteration, and raises the scenario's +`leaked_bytes` (baseline `3417826`) by design. Both must move together: + +- Invert the scenario's assertion to expect the consumed-and-not-freed outcome, and drop + the `_is_pre_consume_rejection` requirement now that the tags no longer decide ownership. +- Re-measure and re-baseline that one entry, rather than raising the global threshold — + a threshold bump would mask unrelated regressions across the other 59 scenarios. +- Add a scenario covering the Fix 1 leak on a **non**-rejection consuming failure, so the + bounded leak has a tracked ceiling instead of being asserted in prose. +- Add a repeated close-under-nested-section scenario for Fix 2: the stranded-deferral bug + leaks a whole native object per occurrence, which is a memory signal the unit tests do + not quantify. + +### Out of scope, stated rather than silently dropped + +`_abort_consume` reviving a resource with a queued teardown; `_native_section`'s `finally` +masking the body's exception; Python-side argument preflighting. All three are noted in the +fix plan and none is required for the paths that exist after Fix 1. + +### Landing order + +Fixes 2, 3, 4 and 5 are independent of native version and of each other. Fix 1 carries the +behaviour change, the test removals and the perf re-baseline, and is the one that must land +before the 0.91.0 bump. + +--- + +## Limits + +- No 0.91.0 build exists to test against; the ordering claim is **read** from `origin/main` + source, and the runtime behaviour under 0.91.0 is **inferred** from it. +- #2559 is unmerged. If it lands in a later release than 0.91.0, the ownership conclusion + is unaffected — that rests on #2344, which is already on main — but the marker-address + reasoning in step 4 would be hardening ahead of a scheme not yet shipped. +- The 100% address-reuse measurement is this macOS allocator on this machine. The direction + generalises; the exact rate does not. +- Findings F1–F11 in the review document are comments on c2pa-rs #2559, not work in this + repository. I verified F1's arithmetic, F2's mechanism and F4's empty changelog; I did not + re-derive the rest, and none of them gates this plan. diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index 67e83004..e83018ad 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -489,6 +489,10 @@ def _finish_teardown(self, free_handle: bool): self._lifecycle_state = LifecycleState.CLOSED return + if getattr(self, '_released', False): + # A concurrent caller already ran this. + return + self._released = True self._lifecycle_state = LifecycleState.CLOSED self._safe_release() @@ -506,13 +510,21 @@ def _maybe_flush_pending(self): teardown clears (this resource's own _inflight dropping to 0, or this thread's native-error section closing). """ + if is_foreign_process(self): + return + with self._state_lock(): if self._pending_teardown is None: return - if getattr(self, '_inflight', 0) > 0 or _in_native_section(): + if getattr(self, '_inflight', 0) > 0: + return + if _in_native_section(): + # An enclosing section is still open. + # Re-register, since the deferral is the only path to this free. + _register_for_section_flush(self) return free_handle, self._pending_teardown = self._pending_teardown, None - self._finish_teardown(free_handle) + self._finish_teardown(free_handle) def _release_handle(self): """Free this handle, then close the object. Used only where ownership is @@ -745,8 +757,13 @@ def _abort_consume(self, previous_state): A pre-consume rejection leaves the handle ours, so the resource has to become usable again. + + A deferred free still happens when the section drains, so a resource + with a queued teardown stays closed. """ with self._state_lock(): + if self._pending_teardown is not None: + return if self._lifecycle_state == LifecycleState.CLOSED and self._handle: self._lifecycle_state = previous_state @@ -1066,28 +1083,44 @@ def _native_section(): Each flush is guarded: the deferral is the only remaining path to that resource's free, so one raising would strand the rest. The first - exception is re-raised once the queue is drained. + exception is re-raised once the queue is drained. A body that raised + keeps its own exception, and the flush failure is logged. """ state = _native_section_state depth = getattr(state, 'depth', 0) state.depth = depth + 1 if depth == 0: state.pending_resources = [] + + def _drain(): + """Flush every deferred resource. Returns the first error raised.""" + pending, state.pending_resources = state.pending_resources, [] + first_error = None + for resource in pending: + try: + resource._maybe_flush_pending() + except BaseException as e: # noqa: BLE001 + if first_error is None: + first_error = e + return first_error + try: yield - finally: + except BaseException: state.depth -= 1 if state.depth == 0: - pending, state.pending_resources = state.pending_resources, [] - first_error = None - for resource in pending: - try: - resource._maybe_flush_pending() - except BaseException as e: # noqa: BLE001 - if first_error is None: - first_error = e - if first_error is not None: - raise first_error + drain_error = _drain() + if drain_error is not None: + logger.error( + "Deferred teardown failed while unwinding: %s", + drain_error) + raise + else: + state.depth -= 1 + if state.depth == 0: + drain_error = _drain() + if drain_error is not None: + raise drain_error class C2paSignerInfo(ctypes.Structure): diff --git a/tests/test_unit_tests.py b/tests/test_unit_tests.py index b984527f..a4e565ae 100644 --- a/tests/test_unit_tests.py +++ b/tests/test_unit_tests.py @@ -1372,7 +1372,6 @@ def test_sign_and_read_is_not_embedded(self): # Direct the Builder not to embed the manifest into the asset builder.set_no_embed() - with open(temp_file_path, "wb") as temp_file: manifest_data = builder.sign( signer, "image/jpeg", file, temp_file) @@ -8921,31 +8920,6 @@ def _untracked_reader_handle(): return (ctypes.cast(buf, ctypes.POINTER(c2pa_module.C2paReader)), buf) - def test_with_fragment_pre_consume_rejection_keeps_handle(self): - # Rejected before native lib took ownership, - # so nothing was consumed and the handle is still ours. - init_path = os.path.join(FIXTURES_DIR, "dashinit.mp4") - fragment_path = os.path.join(FIXTURES_DIR, "dash1.m4s") - with open(init_path, "rb") as init: - reader = Reader("video/mp4", init) - real_handle = reader._handle - - reader._handle = self._stale_reader_handle() - try: - with open(init_path, "rb") as init, \ - open(fragment_path, "rb") as frag: - with self.assertRaises(Error) as caught: - reader.with_fragment("video/mp4", init, frag) - finally: - reader._handle = real_handle - - self.assertIn("UntrackedPointer", str(caught.exception)) - # Ownership never transferred, so the resource stays usable. - self.assertIsNotNone(reader._handle) - self.assertEqual(reader._lifecycle_state, LifecycleState.ACTIVE) - self.assertTrue(reader.json()) - reader.close() - def test_with_fragment_pre_consume_rejection_does_not_leak(self): # A handle dropped on this path leaks one reader per call. init_path = os.path.join(FIXTURES_DIR, "dashinit.mp4") @@ -8986,81 +8960,6 @@ def _reader_from_context(self): "Failed to create reader: {}") return reader - def test_null_parameter_rejection_retains_the_handle(self): - """A null argument is rejected before the reader is untracked. - Ownership never transferred, so the handle is still ours to free. - Treating it as consumed leaks one reader per call. - """ - reader = self._reader_from_context() - handle = reader._handle - freed = self._instrument_frees() - - with self.assertRaises(Error) as caught: - with reader._native_call(): - reader._consume_and_swap( - lambda h: c2pa_module._lib.c2pa_reader_with_stream( - h, b"image/jpeg", None), - "Failed to configure reader: {}") - - self.assertIn("NullParameter", str(caught.exception)) - self.assertIsNotNone(reader._handle, "the retained handle was dropped") - self.assertEqual(reader._lifecycle_state, LifecycleState.ACTIVE) - - reader.close() - self.assertEqual( - self._free_count(freed, handle), 1, - "a handle the native side never took was leaked") - - def test_invalid_buffer_size_rejection_retains_the_handle(self): - """A zero-length manifest buffer is rejected before the untrack.. - """ - reader = self._reader_from_context() - handle = reader._handle - freed = self._instrument_frees() - empty = (ctypes.c_ubyte * 4)() - - with Stream(io.BytesIO(b"abc")) as stream_obj: - with self.assertRaises(Error) as caught: - with reader._native_call(): - reader._consume_and_swap( - lambda h: ( - c2pa_module._lib - .c2pa_reader_with_manifest_data_and_stream( - h, b"image/jpeg", stream_obj._stream, - empty, 0) - ), - "Failed to configure reader: {}") - - self.assertIn("InvalidBufferSize", str(caught.exception)) - self.assertIsNotNone(reader._handle, "the retained handle was dropped") - self.assertEqual(reader._lifecycle_state, LifecycleState.ACTIVE) - - reader.close() - self.assertEqual( - self._free_count(freed, handle), 1, - "a handle the native side never took was leaked") - - def test_repeated_rejections_do_not_accumulate_handles(self): - """Every rejected call must give its handle back, not just the first. - """ - handles = [] - freed = self._instrument_frees() - - for _ in range(10): - reader = self._reader_from_context() - handles.append(reader._handle) - with self.assertRaises(Error): - with reader._native_call(): - reader._consume_and_swap( - lambda h: c2pa_module._lib.c2pa_reader_with_stream( - h, b"image/jpeg", None), - "Failed to configure reader: {}") - reader.close() - - leaked = [h for h in handles if self._free_count(freed, h) == 0] - self.assertEqual( - leaked, [], f"{len(leaked)} of {len(handles)} handles leaked") - def test_repeated_with_fragment_does_not_accumulate_streams(self): """Repeated calls on one Reader must not pile up fragment streams. @@ -9995,26 +9894,6 @@ def test_every_real_rejection_wording_is_classified_as_pre_consume(self): classify(wrapped), f"a wrapped {tag} rejection was read as a consumed handle") - def test_native_rejections_observed_from_the_library_still_classify(self): - """Pins the two shapes the loaded library actually produces.""" - classify = c2pa_module.ManagedResource._is_pre_consume_rejection - - # A null argument the native layer refuses before doing any work. - c2pa_module._lib.c2pa_reader_from_stream(None, None) - bare = c2pa_module._read_native_error() - - # A free of an address the pointer registry does not track. - c2pa_module._lib.c2pa_free(0x9999) - wrapped = c2pa_module._read_native_error() - - for message in (bare, wrapped): - self.assertTrue( - message, - "the native library stopped reporting these rejections") - self.assertTrue( - classify(message), - f"the native rejection wording changed: {message!r}") - def test_caller_text_quoting_a_tag_is_not_a_rejection(self): """A tag inside the message body describes the caller's input. @@ -10296,28 +10175,6 @@ def bad_marshal(handle): self.assertIsNotNone(resource._handle) self.assertEqual(resource._lifecycle_state, LifecycleState.ACTIVE) - def test_pre_consume_rejection_restores_the_resource(self): - """A handle native rejected before taking ownership stays usable. - - The reservation is held until _raise_consume_failure classifies the - error, so no other thread sees the resource as ACTIVE while its - ownership is still undetermined. - """ - resource = Settings() - self.freed.clear() - real_read = c2pa_module._read_native_error - c2pa_module._read_native_error = ( - lambda: "Other: UntrackedPointer: 0x1234") - try: - with self.assertRaises(Error): - resource._consume_no_replacement(lambda h: 1, "consume: {}") - finally: - c2pa_module._read_native_error = real_read - - self.assertEqual(resource._lifecycle_state, LifecycleState.ACTIVE) - self.assertIsNotNone(resource._handle) - self.assertEqual(self.freed, []) - def test_post_consume_failure_keeps_the_resource_closed(self): """An error without a pre-consume tag means native took ownership. diff --git a/tests/test_unit_tests_threaded.py b/tests/test_unit_tests_threaded.py index ba1d010b..16e409e4 100644 --- a/tests/test_unit_tests_threaded.py +++ b/tests/test_unit_tests_threaded.py @@ -4941,7 +4941,6 @@ def visit(node, active): "borrowed handles used without their own guard:\n " + "\n ".join(unguarded)) - def test_consume_during_concurrent_sign_does_not_crash(self): """Consuming a shared Signer must not free it under a live sign. @@ -5113,6 +5112,99 @@ def test_context_close_during_sign_defers_teardown(self): "the deferred teardown never ran") self.assertIsNone(context._pending_teardown) + def test_deferred_teardown_survives_a_flush_inside_a_section(self): + """A flush blocked by a section must re-register, not drop the free. + + The teardown defers on _inflight, so it is queued for the in-flight + call rather than for a section. When that call finishes inside a + section opened later on this thread, the flush cannot free yet, and + without re-registering nothing would ever free this handle. + """ + context = Context() + freed = [] + real_free = ManagedResource._free_native_ptr + ManagedResource._free_native_ptr = staticmethod( + lambda ptr: (freed.append(ptr), real_free(ptr))[1]) + try: + with context._native_call(): + closer = threading.Thread(target=context.close) + closer.start() + closer.join() + self.assertIsNotNone( + context._pending_teardown, + "close() during a native call should defer") + section = _native_section() + section.__enter__() + + self.assertEqual( + freed, [], + "the flush freed while a native section was still open") + self.assertIsNotNone( + context._pending_teardown, + "the deferral was dropped instead of re-registered") + + section.__exit__(None, None, None) + self.assertEqual( + len(freed), 1, + "the deferred teardown was stranded and never freed") + self.assertIsNone(context._pending_teardown) + finally: + ManagedResource._free_native_ptr = real_free + + def test_abort_consume_leaves_a_queued_teardown_closed(self): + """A resource whose free is already queued must not become usable. + + The deferred free still runs when the section drains, so restoring + ACTIVE would hand the caller a resource that closes underneath it. + """ + context = Context() + with _native_section(): + context.close() + self.assertIsNotNone(context._pending_teardown) + + context._abort_consume(LifecycleState.ACTIVE) + self.assertEqual( + context._lifecycle_state, LifecycleState.CLOSED, + "a resource with a queued teardown was revived") + self.assertFalse( + context.is_valid, + "a resource with a queued teardown reported itself usable") + + def test_section_drain_error_does_not_mask_the_body_error(self): + """The body's exception is what the caller asked for, so it wins.""" + + class FlushRaises: + _pending_teardown = True + + def _maybe_flush_pending(self): + raise RuntimeError("flush failed") + + class BodyError(Exception): + pass + + with self.assertLogs('c2pa', level='ERROR') as logs: + with self.assertRaises(BodyError): + with _native_section(): + c2pa_module._register_for_section_flush(FlushRaises()) + raise BodyError("the error the caller cares about") + + self.assertTrue( + any("flush failed" in line for line in logs.output), + "the flush failure was swallowed instead of logged") + + def test_section_drain_error_still_raises_when_the_body_succeeds(self): + """With no body error, a failed flush is still reported.""" + + class FlushRaises: + _pending_teardown = True + + def _maybe_flush_pending(self): + raise RuntimeError("flush failed") + + with self.assertRaises(RuntimeError): + with _native_section(): + c2pa_module._register_for_section_flush(FlushRaises()) + def test_context_sign_after_close_raises_rather_than_skipping_signer(self): """Signing through a closed Context must raise, not silently succeed. From 9c91987698e39a202368ed4078e3a83f5c227e07 Mon Sep 17 00:00:00 2001 From: tmathern <60901087+tmathern@users.noreply.github.com> Date: Sat, 29 Aug 2026 07:21:32 -0700 Subject: [PATCH 16/17] Delete c2pa-2559-review-reasoning.md --- c2pa-2559-review-reasoning.md | 680 ---------------------------------- 1 file changed, 680 deletions(-) delete mode 100644 c2pa-2559-review-reasoning.md diff --git a/c2pa-2559-review-reasoning.md b/c2pa-2559-review-reasoning.md deleted file mode 100644 index 7349b7b1..00000000 --- a/c2pa-2559-review-reasoning.md +++ /dev/null @@ -1,680 +0,0 @@ -# c2pa-rs PR #2559 — adversarial review, downstream impact, and c2pa-python #312 re-review - -Full reasoning record. Everything below is **static analysis only** — there is no Rust -toolchain in the working container (`static.rust-lang.org` is not on the egress allowlist), -so nothing here was compiled or executed. Where a claim rests on an assumption rather than -on traced code, that is stated inline. - -Artifacts read: - -| Repo | Ref | How | -|---|---|---| -| `contentauth/c2pa-rs` | head `145b754`, base `b3cd390` (PR #2559, branch `gpeacock/c_ffi_opaque_ids`) | full tarball + diff | -| `contentauth/c2pa-cpp` | `main` HEAD | full tarball | -| `contentauth/c2pa-python` | `main` HEAD | full tarball | -| `contentauth/c2pa-python` | `385b28c` (PR #312, branch `mathern/error-slot-sentinel`, base `mathern/sigsev-sigabort`) | full tarball + diff vs main | - ---- - -## Part 1 — What #2559 actually does - -### 1.1 The change - -`PointerRegistry` previously keyed its `HashMap` on the **real address** of every tracked -allocation. The pointer handed to C *was* the object's address. #2559 splits the key space -in two: - -- **Handles** (`C2paReader`, `C2paBuilder`, `C2paSigner`, `C2paSettings`, `C2paContext`, - `C2paContextBuilder`, `C2paStream`, `C2paHttpResolver`) are now keyed by a synthetic - **opaque id** produced by `scramble_to_odd_id`, and it is the id — not the address — - that crosses the FFI boundary. `track_by_id` (utils.rs:~100-120). -- **Buffers** returned by `to_c_string` (utils.rs:512) and `to_c_bytes` (utils.rs:547) stay - keyed by their **real address**, because C dereferences them. `track_by_address` - (utils.rs:117-125). - -`scramble_to_odd_id(counter) = (2·counter + 1) · M mod 2^N`, with -`M = 0x9e3779b97f4a7c15` on 64-bit and `M = 0x9e3779b9` on 32-bit. Producing only odd -values is what keeps the two key spaces from overlapping: real allocations are assumed -always even. - -`validate_pointer` / `untrack_pointer` change return type from `Result<(), Error>` to -`Result<*mut T, Error>`, so every deref macro now goes id → registry → real address → -deref. `PointerRegistry::validate` is renamed to `resolve`. Two new macros, -`deref_mut_option!` and `deref_mut_option_or_return!` (macros.rs:331-357, 358-380), give a -non-erroring `Option` form for cleanup paths. - -### 1.2 The vulnerability it fixes — confirmed - -Under address keying: object X at address P is freed (removed from the map, `Box` dropped); -a new object Y of the same type is later allocated at P and tracked; a **stale handle P now -resolves to Y**. Same type, wrong live instance. That is a textbook ABA, and it is real. - -The fix works for that case: ids are drawn from a monotonic counter and are never recycled, -so a stale id fails lookup with `UntrackedPointer:`. - -An extra detail that makes the old bug much more likely than it first looks, and that -matters for the Python analysis in Part 3: consuming calls such as -`c2pa_builder_with_archive` do `untrack_or_return_null!` → `Box::from_raw` → drop → -`box_tracked!(new Box)`. The new `Box` is the same size as the one just freed, and glibc's -tcache is LIFO, so **the replacement handle very often had the identical numeric value as -the consumed one**. Downstream code holding the old pointer was frequently, not rarely, -holding a pointer to the live replacement. - ---- - -## Part 2 — Findings on #2559 - -Severity, anchor, and where each comment can physically go. GitHub only allows inline -comments on lines inside a diff hunk, so a few of these are forced into the review body. - -### F1 — MEDIUM. "Ids are never reused" is false on 32-bit; the period is 2^(N−1) - -`(2c+1) mod 2^N` takes only **2^(N−1)** distinct values, because `c` and `c + 2^(N−1)` map -to the same value. Multiplying by an odd `M` is a bijection and does not restore the lost -bit. So the id sequence has period 2^(N−1), not 2^N. - -- 64-bit: 2^63. Unreachable. -- wasm32-unknown-emscripten (a supported target — `c2pa_c_ffi/Cargo.toml` has a - `cfg(target_arch = "wasm32")` dependency table, and `maybe_send_sync.rs` exists for it): - **2^31 ≈ 2.1e9 handles**, reachable in a long-running process. At wrap, the exact ABA - this PR fixes returns, silently. - -The doc at utils.rs:55-59 states distinct counters are "mathematically guaranteed" to -scramble to distinct odd ids. Off by exactly this factor of two. - -Secondary point for the same comment: the PR argues that a stray deref of a handle produces -an immediate obvious crash. On x86-64 that holds — ids land in non-canonical address space -(high bits derived from `0x9e3779b9…`), so the deref faults. On wasm32 it does **not** -generally hold: ids are uniformly distributed 32-bit odd values, and any id smaller than -the current linear-memory size falls inside valid memory and reads garbage instead of -trapping. - -→ **Inline, utils.rs:60** (`fn scramble_to_odd_id`, inside the `@@ -33,33 +36,97` hunk). -Ask for the real bound in the doc plus a hard stop on 32-bit once the counter passes 2^31. - -### F2 — MEDIUM. The odd/even non-collision invariant is asserted, never enforced, and rests on the allocator rather than on Rust - -utils.rs:51-54 justifies non-collision with "real Rust allocations always land on at least a -2-byte boundary, so their addresses are always even." - -Both `track_by_address` callers allocate **align-1** memory: `CString::into_raw` -(utils.rs:512) produces a `Vec`, and `to_c_bytes` (utils.rs:547) produces a -`Box<[u8]>`. Rust guarantees only `align_of::() == 1`. The claim is true in practice -only because `std`'s `System` allocator forwards to malloc/dlmalloc, which align to 8 or -16 — an allocator property, not a language one. A downstream `#[global_allocator]` (bump -and arena allocators are common in wasm builds) can hand back odd addresses. - -If it ever broke, the failure would be **silent, not an error**: `track_by_id`'s -`tracked.insert(id, …)` (utils.rs:114) would overwrite the string's entry, drop its -`CleanupFn` (leak), and make `c2pa_free(that_string)` free the *object* instead. - -→ **Inline, utils.rs:117** (`fn track_by_address`). Suggest -`debug_assert_eq!(real_addr & 1, 0)` there, and checking `insert`'s return value in -`track_by_id` so a collision is detected rather than assumed away. - -### F3 — MEDIUM. The ABA class is only half fixed, and the new doc does not say so - -`to_c_string` / `to_c_bytes` stay address-keyed. A stale `char*` still resolves to whatever -buffer lands at that address next, so `c2pa_free` on it can free a *different* live string. -Same ABA, and undetectable in the CString→CString case since the `TypeId` check passes. - -The keying cannot change — C dereferences these — but the registry doc at utils.rs:66-84 -explains why handles are safe without noting that the address-keyed half remains exposed, -and the PR description reads as if ABA is closed generally. - -→ **Inline, utils.rs:77-84** (the `track_by_address` bullet in the registry doc comment). - -### F4 — MEDIUM. Breaking Rust API change, source-compatible in the dangerous direction - -`validate_pointer` and `untrack_pointer` change signature; `PointerRegistry::validate` is -renamed to `resolve`. Both functions are re-exported at the crate root -(`cimpl/mod.rs:72-75`) and `cimpl::utils` is a `pub mod` reachable through -`pub use cimpl::*` in `lib.rs`. So this is a breaking change to the `c2pa-c-ffi` crate's -Rust API. - -The dangerous part: existing downstream `untrack_pointer(p)?;` **still compiles**. `*mut T` -is not `#[must_use]`, so the returned real pointer is silently discarded, and the caller's -subsequent `Box::from_raw(p)` operates on the handle id. UB with no compile error. - -`.github/workflows/semver-checks.yml` lists `c2pa-c-ffi` as a public-API crate but only -runs on PRs targeting `stable` / `v0.*`, so it will **not** fire on this PR against `main`. -It will surface at the release PR instead. - -→ **Review body** for the semver/CHANGELOG point (`c2pa_c_ffi/CHANGELOG.md` has an empty -`## [Unreleased]`), plus **inline `#[must_use]` asks at utils.rs:316 and utils.rs:343**. - -### F5 — MEDIUM. Nothing tests the new invariants - -Codecov reports 83.6% patch coverage, 7 uncovered lines in `utils.rs`. Every test change in -the PR is mechanical (`untrack_pointer(...).unwrap()` adjusted for the new return type). -Grepping the test module in `cimpl/utils.rs` finds no reference to `scramble`, `odd`, -`next_id`, `track_by_id`, or `track_by_address`. - -Nothing asserts: - -- an id differs from the real address; -- ids are odd; -- a stale handle fails `resolve` after its address is reused by a new same-type object — - **the actual regression test for the bug in the title**, and it needs no threads: track, - `cimpl_free`, `track_box` a new `T`, assert `validate_pointer(old_id).is_err()`; -- wrong-type resolve returns `WrongPointerType`; -- `free` on a stale id returns −1. - -→ **Inline, utils.rs:~620** (the `@@ -543,14 +620,15` test-module hunk). - -### F6 — LOW. One missed call site: `C2paStream::extract_context` (c2pa_stream.rs:110) - -Still does `Box::from_raw(self.context)` on what is now a handle id, and never untracks, so -the registry entry persists. - -Honest severity: `StreamContext` is a unit struct (c2pa_stream.rs:27), i.e. a ZST, so -`Box` drop never calls the allocator — a non-null value is trivially aligned for -align-1, and this will not crash or double-free today. It also has no in-repo callers. But -it is `pub` via `pub use c2pa_stream::*`, and it is precisely the pattern the PR swept for. -Delete it or route it through `untrack_pointer`. - -→ **Review body only** — line 110 falls outside every diff hunk. - -### F7 — LOW. `resolve` / `untrack` / `free` are `pub` on a `pub` struct in a `pub` module - -The premise of the change is that the real address never leaves the registry, yet -`pub fn resolve(&self, id: usize, …) -> Result` lets any downstream crate turn -a handle back into an address. `pub(crate)` preserves the property. - -→ **Inline, utils.rs:128.** - -### F8 — LOW. The TOCTOU window is narrowed, not closed - -`resolve` drops the `MutexGuard` before returning; the caller then dereferences the real -address outside the lock (macros.rs:248, 304, 354, 378). A concurrent `cimpl_free` of the -*same* handle between resolve and deref is still a use-after-free. The PR fixes -wrong-object; same-object-freed-underneath remains. Pre-existing and arguably out of scope, -but the PR body reads as though the threading hazard is handled. - -→ **Inline, macros.rs:304.** - -### F9–F11 — NITs - -- Error messages lost the real address: `wrong_pointer_type(id as u64)` / - `untracked_pointer(id as u64)` (utils.rs:~136-140) report the opaque id, which means - nothing to a debugger. In the `wrong_pointer_type` arm the real address is in hand. - → inline utils.rs:136-140. -- `deref_mut_option!` is `#[macro_export]` (macros.rs:346) while its own doc calls it - internal-only. `#[doc(hidden)]` at minimum. → inline macros.rs:346. -- `deref_or_return!` / `deref_mut_or_return!` still evaluate `$ptr` twice - (`ptr_or_return!($ptr, …)` then `validate_pointer($ptr)`), whereas the two new macros - correctly bind it once. Harmless for current call sites (all simple locals or casts), but - both lines are already open in this diff. → inline macros.rs:246-247, 302-303. - -### 2.1 Cleared while reviewing — checked and dismissed - -- **All 79 `extern "C"` fns scanned programmatically.** Every parameter typed - `*mut C2paStream / C2paSigner / C2paBuilder / C2paReader / C2paSettings / C2paContext / - C2paContextBuilder / C2paHttpResolver` passes through a `deref_*`, `untrack_or_return_*`, - or `cimpl_free!` macro. No unvalidated handle params remain. -- The `&mut *stream` / `&mut *source` / `&mut *dest` occurrences at c_api.rs:1888, 1927, - 1969, 2011-2012, 2061, 2482 are **reborrows of the macro-produced `&mut C2paStream`**, not - raw derefs. They look like missed call sites and are not. -- The only two remaining raw-pointer derefs in the crate are c_api.rs:5077 - (`&*(context as *const AtomicU32)`, a test counter) and json_api.rs:71 (`&*signer`, an - `Arc` deref). Neither is a tracked handle. -- `TestC2paStream::reader` and `seeker` already used `deref_mut_or_return_int!` on main; - only `writer` needed the fix. Verified against the diff hunks. -- `to_c_bytes` returns NULL for empty input (utils.rs:543), so no `NonNull::dangling()` - value of `1` can ever be tracked by address and collide with the odd id space. -- `drop_c_stream`: the `if let Some(real_stream)` binding scope ends before - `cimpl_free(c_stream)`, so there is no live `&mut` across the free. -- `test_c2pa_create_stream` frees `context` exactly once — `c2pa_release_stream` does not - touch the context. -- `untrack`'s `tracked.get(&id)` followed by `tracked.remove(&id)` inside the matched arm: - no scrutinee binding is used in the arm body, so NLL should end the immutable borrow - before the `remove`. Should compile; CI would catch it otherwise. The destructuring - `let (real_addr, _, _) = …` drops the `CleanupFn`, which for `track_box` captures only a - `usize` — harmless. -- `next_id` starts at 0, so the first id is `1 · M = 0x9e3779b97f4a7c15`, non-zero and odd. - `Ordering::Relaxed` on `fetch_add` is fine: the RMW is atomic, so values are unique - regardless of ordering. -- `arc_tracked!` is unused, so the pre-existing `untrack_or_return!` → `Box::from_raw` - mismatch for `Arc`-tracked entries is not reachable. Not this PR's concern. -- `mergeable_state` was `unstable`; the GitHub API rate-limited before the check-runs list - could be read, so the failing check is unidentified. Codecov's patch gate at 83.6% is the - likely candidate but that is a guess. - ---- - -## Part 3 — Downstream impact - -### 3.1 c2pa-cpp — one confirmed hard break - -`tests/c-app-test/file_stream.h:114`: - -```c -int close_file_stream(C2paStream *stream) -{ - if (stream == NULL) { return -1; } - FILE *file = (FILE *)stream->context; // <-- breaks - int result = fclose(file); - c2pa_release_stream(stream); - return result; -} -``` - -`stream` is the pointer returned by `c2pa_create_stream`, which is now an opaque id. On -x86-64 that address is non-canonical, so this is an immediate SIGSEGV — exactly the -"obvious crash" the PR intends, but in a downstream consumer rather than internal code. - -This is the empirical proof for a review point worth adding to #2559: `C2paStream` is -`#[repr(C)]`, cbindgen emits its fields into `c2pa.h`, and the header therefore advertises -a layout the returned pointer no longer has. **Ask for `C2paStream` (and the other handle -types) to go into cbindgen's `opaque_types`** — that converts this whole class of downstream -break from a runtime fault into a compile error. c2pa-cpp's own fix is to keep the `FILE*` -alongside the handle instead of reading it back out. - -Everything else in c2pa-cpp is clean: - -- The C++ wrappers (`include/c2pa.hpp`, `src/c2pa_*.cpp`) only store handles and hand them - back to `c2pa_free` / `c2pa_release_stream`. No field access, no arithmetic, no use as map - keys, no `unique_ptr`, no `delete`. -- Stream contexts it passes in — `reinterpret_cast(&istream)` at - c2pa.hpp:571, 633, 693 — are **C++-owned and never enter the registry**. `c2pa_create_stream` - stores the context verbatim and the C++ callbacks cast it straight back. Untouched by #2559. - (The `deref_mut_or_return_int!` on `context` inside `c2pa_stream.rs` applies only to the - Rust-side `TestC2paStream` helper.) - -**wasm relevance:** `Makefile:151` downloads a `wasm32-unknown-emscripten` build from -c2pa-rs releases, pinned at `CMakeLists.txt:20` → `C2PA_VERSION "0.90.16"`. That is the -32-bit target where F1 applies: 2^31 id period, and stray derefs that read garbage instead -of trapping. - -### 3.2 c2pa-python (main) — net leak fix, one latent footgun - -The binding's ownership model rests on *"a guarded free is a real free if ours, a no-op if -not"* (`ManagedResource._free_native_ptr`, c2pa.py:268-289; `_release_handle`, 341-349). -Under address keying that was only **probabilistically** true, for the tcache reason in §1.2: -a stale free could hit the live replacement, or a recycled object on another thread. That is -a use-after-free, not a leak. After #2559 the old id is dead forever and `c2pa_free` -deterministically returns −1. The comment at c2pa.py:490-493 about *"races a recycled address -in other threads"* describes precisely the hazard this closes. -`tests/test_unit_tests_threaded.py` is where it would have surfaced. - -**The error-tag routing survives unchanged.** `_PRE_CONSUME_ERROR_TAGS` matches on -`"UntrackedPointer:"` / `"WrongPointerType:"` (c2pa.py:421); #2559 only swaps the numeric -value inside those messages, not the tag text. The retain-vs-consume decision in -`_raise_consume_failure` behaves identically. - -Leak behaviour changes in two small ways, both benign: - -1. The retain branch fires more often — a stale handle reaching native is now *always* - rejected with `UntrackedPointer:`, so the Python object stays `ACTIVE` with a dead - handle. Nothing leaks in Rust; the previous behaviour (succeeding against a recycled - object) was strictly worse. -2. Ids are never recycled, so a genuinely leaked handle now leaks faithfully and its - registry entry persists. Address recycling used to clean some of these up by accident. - **A latent leak — e.g. `__del__` not firing under a reference cycle — may become visible - for the first time in soak/perf runs.** That is a diagnostic win, not a new regression, - but it is worth expecting. - -**Latent footgun:** `class C2paStream(ctypes.Structure)` at c2pa.py:658-692 declares the real -`_fields_` (`context`, `reader`, `seeker`, `writer`, `flusher`), unlike every other opaque -type in the file which uses `_fields_ = []`. Nothing dereferences it today — -`c2pa_create_stream` is called with `context=None` (c2pa.py:2003-2009) and the callbacks -close over a weakref — so there is no live bug. But it is the same shape as the c2pa-cpp -break. Change it to `_fields_ = []`. - -**Checked and clear:** `_convert_to_py_string` (c2pa.py:1220-1254) and the mime-type array -paths operate on `to_c_string` / `to_c_bytes` pointers, which stay address-keyed. -`ctypes.addressof(data.contents)` at c2pa.py:1879 is the read callback's real `*mut u8` -buffer, not a handle. `if not handle` truthiness is safe since ids are never 0. No -pointer-identity maps, no arithmetic, no reconstruction of pointers from stored ints. - ---- - -## Part 4 — c2pa-python PR #312 re-review - -PR #312, *"fix: Put a sentinel in the native thread local error slot"*, head `385b28c`, -base `mathern/sigsev-sigabort` (not `main`), 8 commits, approved by ale-adobe, awaiting -ok-nick. - -### 4.1 What it does - -`_MARKER_ADDR = 1` is passed to `c2pa_free` to plant a known error into the thread-local -`LAST_ERROR` slot. The exact text is **learned at import** rather than hardcoded -(`_learn_sentinel_no_native_error_text`, c2pa.py:1271-1296), since the format is a native -implementation detail. `_invoke_consume` marks the slot before every consuming call -(c2pa.py:594), and `_read_native_error` re-marks after reading (c2pa.py:899), so an error is -consumed exactly once by the caller that observes it. - -### 4.2 Is #312 still warranted given #2559? — split answer - -**The sentinel core: yes, and #2559 makes it *more* necessary.** - -`LAST_ERROR` stickiness is orthogonal to how the registry is keyed. #2559 does not clear the -slot, does not change thread-locality, and does not change the message text for a failed -free — so `_learn_sentinel_no_native_error_text()` keeps working unchanged. - -Second-order effect worth adding to the PR description: after #2559, a guarded free of a -dead handle **always** returns −1 and **always** writes `UntrackedPointer:` into the slot. -Under address keying, a fraction of those frees silently succeeded and set nothing, because -the address had been recycled. So pre-consume-tag pollution of the sticky slot becomes -strictly more frequent once #2559 lands, and the misclassification #312 fixes gets more -likely, not less. - -Mapping the PR body's two motivations: - -| Motivation in #312 body | Status after #2559 | -|---|---| -| Stale tag from a finished task on a pooled worker thread | **Untouched.** This is the load-bearing one. | -| Address reuse: stale free finds a live entry and destroys another thread's object | **Eliminated.** | - -**The leak flip: warranted today, obsolete once #2559 ships.** - -This is the substantive difference from `main`. On `main`, `_raise_consume_failure`'s -"no error in the slot" branch did `self._release_handle()` — free defensively. On #312 -(c2pa.py:648-658) it does `_teardown(free_handle=False)`, and the justification is verbatim -*"a free here can race a recycled address in other threads."* The same reasoning appears in -the non-tag branch at 641-646. - -That is precisely and only the hazard #2559 removes. #312 trades a possible UAF for a -certain leak — which the PR body concedes: *"On any consume failure where the verdict is not -certain, this takes the consumed branch, meaning leaks could appear."* Once a -#2559-containing c2pa-rs is the floor, both branches can revert to `_release_handle()` and -recover the leak, **independently of the 0.91.0 always-consumed contract the PR body is -waiting on**. Recommendation: land #312 as-is, with a TODO / issue link on those two -branches so it is not forgotten — the existing note points at 0.91.0, which is a different -mechanism arriving later. - -### 4.3 New interaction to flag on #312 — `_MARKER_ADDR = 1` - -```python -# Unaligned address passed to c2pa_free to plant a marker -# in the native error slot. -# Never a real handle: allocations are aligned, and the Python -# layer only passes real handles or this constant to c2pa_free. -_MARKER_ADDR = 1 -``` - -That justification is exactly the invariant #2559 inverts. After it, registry keys are no -longer all real addresses: handle ids are `(2c+1)·M mod 2^N`, i.e. **always odd** — the same -namespace as `1`. - -Multiplication by an odd constant is a bijection mod 2^N, so there is exactly one counter -value per period producing id `1`. Solve `(2c+1)·M ≡ 1 (mod 2^N)`, i.e. `2c+1 ≡ M⁻¹`: - -| Width | `M` | `M⁻¹ mod 2^N` | counter yielding id `1` | -|---|---|---|---| -| 64-bit | `0x9e3779b97f4a7c15` | `0xf1de83e19937733d` | `8714256306465913246` ≈ 2^63 | -| 32-bit | `0x9e3779b9` | `0x144cbc89` | **`170286660`** ≈ 2^28 | - -Reachability, stated honestly: **c2pa-python ships 64-bit wheels only.** -`scripts/download_artifacts.py` maps to `x86_64` / `aarch64` across -`apple-darwin`, `pc-windows-msvc`, `unknown-linux-gnu` — no i686, no wasm. So this is **not -reachable for Python in practice**. It is reachable on c2pa-cpp's emscripten path, where -170M tracked handles is a soak-test-scale number rather than an astronomical one. - -Consequence if it ever hit: `_mark_sentinel_no_native_error()` → `c2pa_free(1)` frees a -**live object** belonging to another thread, returns 0, and sets no error. And it is called -on every `_invoke_consume` and every `_read_native_error`, so it is a hot path. - -Fix: pick a marker that is outside every key space under **both** the current v0.90 -scheme and #2559, rather than one justified by whichever scheme happens to be loaded. That -is `_MARKER_ADDR = 8` — see §5.1 for the two independent properties that make it safe under -each, the constant-derived assert that replaces the hardcoded `"0x1"` at c2pa.py:1297, and -why `0` must not be used. - -### 4.4 Carried over unchanged into #312 - -- `C2paStream._fields_` at c2pa.py:833-844 still declares the real layout while every other - opaque type uses `_fields_ = []`. Still no live bug (`c2pa_create_stream` is called with - `context=None` at c2pa.py:2282-2288), still the same shape as the confirmed c2pa-cpp - break at `file_stream.h:114`. Still worth `_fields_ = []`. -- `_PRE_CONSUME_ERROR_TAGS` grew to four entries (c2pa.py:567-572), adding `NullParameter:` - and `InvalidBufferSize:`. All four still match after #2559; `resolve()` returning - `null_parameter("pointer")` for id 0 keeps the `NullParameter:` tag meaningful, and ids - are never 0, so it only fires on genuine nulls. - ---- - -## Part 5 — Making the plan work against v0.90 *and* #2559 - -**Revision note.** The first version of §4.3 recommended `_MARKER_ADDR = 2` justified by -"handle ids are always odd." That reasoning only holds *after* #2559 and says nothing about -v0.90. This part replaces it with a set of choices that are correct under both, and -separates the items that need no version-awareness at all from the one that genuinely does. - -Three native behaviours are in play: - -| | key space | ownership on a failed consuming call | -|---|---|---| -| **v0.90.x (today)** | real addresses only, recyclable | ambiguous | -| **v0.90.x + #2559** | odd synthetic ids for handles, real addresses for buffers | ambiguous | -| **v0.91.0 (announced)** | as above | always consumed by native | - -Only the *middle* column differs from today in a way that changes a Python decision, and -only for one branch. Everything else can be made version-blind. - -### 5.1 The marker address — version-blind, and worth changing now - -`_MARKER_ADDR` must be a value that the registry can never legitimately hold as a key, -under any of the three columns. Two independent properties give that: - -1. **Below the first page.** No allocator returns an address in the null page, under any - scheme, so it can never be a real allocation and therefore never an address-keyed buffer - entry. This is the property that covers v0.90, where *all* keys are addresses. -2. **Even.** Under #2559, ids are `(2c+1)·M` with `M` odd, so every id is odd *by - construction* — not by allocator convention. An even value can therefore never be a - synthetic id. - -`1` has property 1 but not property 2. Use **`_MARKER_ADDR = 8`**: it satisfies both, and -each property alone is sufficient for one of the two schemes, so the marker is safe whether -or not #2559 is present in the loaded library. Do not use `0` — `PointerRegistry::free` -short-circuits `key == 0` to `Ok(())` (utils.rs), so a zero marker would return 0 and set no -error, silently disabling the whole mechanism. - -Rewrite the justification comment accordingly: - -```python -# Address passed to c2pa_free purely to plant a known marker in the -# native thread-local error slot. -# -# Safe under every native key scheme: -# - below the first page, so never a real allocation and never an -# address-keyed buffer entry; -# - even, and synthetic handle ids are odd by construction, so never -# a handle id either. -# Must not be 0: the registry treats a 0 key as a successful no-op. -_MARKER_ADDR = 8 -``` - -**Derive the assert rather than hardcoding the literal.** c2pa.py:1297 currently reads -`assert "0x1" in _NO_NATIVE_ERROR_TEXT`, which both hardcodes the constant and is a loose -substring test (`"0x1"` matches `0x1a2b…`). Replace it with something tied to the constant -and anchored, and move it inside `_learn_sentinel_no_native_error_text()` — which also -answers ale-adobe's review comment: - -```python -def _learn_sentinel_no_native_error_text(): - ... - if f"0x{_MARKER_ADDR:x}" not in text: - raise ImportError( - "c2pa native library's untracked-pointer error text no longer " - "includes the planted address; the error-slot marker assumption " - "no longer holds") - return text -``` - -The message text itself needs no version handling: v0.90 formats -`untracked_pointer(ptr as u64)` and #2559 formats `untracked_pointer(id as u64)`, and for -the marker the value passed *is* the key in both cases, so the learned string is identical. -Learning at import already makes this robust; the only thing that was version-specific was -the choice of constant. - -### 5.2 The free-vs-leak branches — the one place version-awareness is needed - -`_raise_consume_failure`'s two `_teardown(free_handle=False)` branches (c2pa.py:641-646 and -648-658) leak on an ambiguous failure, to avoid a defensive free racing a recycled address. -That trade is **correct on v0.90 and unnecessary after #2559**. - -Before building any machinery for this, check the release sequencing, because it may be -moot: #2559 targets `main`, and the announced always-consumed contract is v0.91.0. If both -ship in 0.91.0, there is never a release where ids are opaque *and* ownership is ambiguous — -the middle column of the table above never exists — and the right answer is simply to leave -#312's leak branches alone permanently, since under always-consumed they are correct by -contract rather than as a workaround. **Resolve that question first.** Everything in the -rest of this subsection is contingent on the middle window being real. - -If it is real, detect the scheme behaviourally rather than by version string. A version -parse has to encode which release contains #2559 and breaks on backports; a behavioural -probe describes the property it actually depends on: - -```python -def _detect_opaque_handles(probes=4): - """True when the native library returns synthetic handle ids rather - than real addresses. - - Synthetic ids are odd by construction; real allocation addresses are - aligned to at least 8 bytes by every allocator this library ships - against. Requiring every probe to come back odd means a stray odd - address cannot flip the verdict, and any failure falls to the - conservative (address-keyed) answer. - """ - handles = [] - try: - for _ in range(probes): - h = _lib.c2pa_settings_new() - if not h: - return False - handles.append(h) - return all( - ctypes.cast(h, ctypes.c_void_p).value & 1 for h in handles) - except Exception: - return False - finally: - for h in handles: - _lib.c2pa_free(h) -``` - -`c2pa_settings_new` is `box_tracked!(C2paSettings::new())` — a single `Box`, no I/O, present -in both the v0.90 FFI and the #2559 head. Allocating several before freeing any prevents the -allocator from handing back the same address each round, which would make the probe a test -of one address rather than of the scheme. - -Failure directions are asymmetric and the probe is oriented safely: - -- **False negative** (says address-keyed when it is id-keyed): keeps the leak branch. The - status quo of #312. Harmless. -- **False positive** (says id-keyed when it is address-keyed): re-enables the defensive - free, which is the UAF #312 exists to prevent. This requires *every* probe to return an - odd address — impossible with malloc/dlmalloc alignment, and made vanishingly unlikely by - the `all()` over several probes. Any exception path also returns `False`. - -Then gate only the branches, leaving the tag routing untouched: - -```python -_HANDLES_ARE_OPAQUE = _detect_opaque_handles() - -# ... inside _raise_consume_failure, both ambiguous branches: -if _HANDLES_ARE_OPAQUE: - # Freeing a dead handle is a guaranteed no-op: ids are never - # recycled, so this cannot reach another thread's object. - self._release_handle() -else: - # Address keys are recyclable; a defensive free could destroy a - # live object at a reused address. Accept the leak. - self._teardown(free_handle=False) -``` - -**Import ordering.** Run `_detect_opaque_handles()` *before* -`_learn_sentinel_no_native_error_text()`. The probe's frees succeed and set no error, so -they cannot disturb the slot, but learning the sentinel last leaves the error slot in the -known state the rest of the module assumes. - -**Test it under both.** A unit test can force each branch by monkeypatching -`_HANDLES_ARE_OPAQUE`, so the leak path and the free path are both covered on a single -native build. Add one assertion that the probe itself agrees with `sdk_version()` on the CI -matrix, so a future native change that breaks the odd-id invariant is caught loudly rather -than silently downgrading to the leak branch forever. - -### 5.3 Items that are already version-blind - -No change needed for compatibility; they behave identically under all three columns. - -- `C2paStream._fields_ = []` (c2pa.py:833-844). Nothing derefs it under either scheme; the - change only removes the ability to. -- `_PRE_CONSUME_ERROR_TAGS` (c2pa.py:567-572). All four tags are produced by both v0.90 and - #2559, with the same text. -- The c2pa-cpp fix to `tests/c-app-test/file_stream.h:114` — keeping the `FILE*` alongside - the handle instead of reading `stream->context` back — is correct under both, since it - simply stops depending on the struct layout. -- Every c2pa-rs finding in Part 2 is a comment on #2559 itself and has no v0.90 dimension. - -### 5.4 One sequencing constraint created by the cbindgen ask - -The recommendation to move `C2paStream` and the other handle types into cbindgen's -`opaque_types` (Part 2, review-body item) turns the c2pa-cpp break from a runtime segfault -into a compile error. That is the desired outcome, but it means **`file_stream.h` stops -compiling the moment c2pa-cpp bumps to a release containing the change**. Land the c2pa-cpp -fix first, or land both in a coordinated bump, and call the header change out in the -c2pa-c-ffi changelog alongside the F4 semver note — it is a source-breaking change for any C -consumer that touches those structs, not only for this one test helper. - ---- - -## Part 6 — Consolidated action list - -**On c2pa-rs #2559** - -1. Inline utils.rs:60 — F1, period is 2^(N−1); 32-bit wraps at 2^31; the crash-loudly - argument does not carry to wasm32. -2. Inline utils.rs:117 — F2, `debug_assert_eq!(real_addr & 1, 0)`; check `insert`'s return. -3. Inline utils.rs:77-84 — F3, document that address-keyed buffers remain ABA-prone. -4. Inline utils.rs:316 and utils.rs:343 — F4, `#[must_use]`. -5. Inline utils.rs:~620 — F5, add the stale-handle regression test. -6. Inline utils.rs:128 — F7, `pub(crate)` on `resolve` / `untrack` / `free`. -7. Inline macros.rs:304 — F8, note the residual resolve→deref window. -8. Inline utils.rs:136-140, macros.rs:346, macros.rs:246-247 / 302-303 — F9–F11 nits. -9. **Review body:** F6 (`extract_context`, outside all hunks); F4's semver/CHANGELOG point; - and the cbindgen `opaque_types` ask, citing the c2pa-cpp break as the motivating case and - noting the §5.4 sequencing constraint. - -**On c2pa-cpp** - -10. Fix `tests/c-app-test/file_stream.h:114` — keep the `FILE*` alongside the handle. Land - before, or with, the version bump that carries the cbindgen change. - -**On c2pa-python #312 — safe under v0.90 today, and after #2559** - -11. `_MARKER_ADDR = 8` with the two-property justification from §5.1. Not `1`, not `0`. -12. Derive the sentinel assert from `_MARKER_ADDR` and move it inside - `_learn_sentinel_no_native_error_text()` (also answers the open review comment). -13. `C2paStream._fields_ = []` at c2pa.py:833-844. - -**On c2pa-python — deferred, and only if the middle release window turns out to be real** - -14. Confirm with the c2pa-rs team whether #2559 and the always-consumed contract ship in the - same release. If yes, stop here and leave the leak branches permanently. -15. If no: add `_detect_opaque_handles()` (§5.2), gate the two ambiguous branches on it, - order it before the sentinel learn, and add both-branch coverage plus a probe-versus- - `sdk_version()` consistency check in CI. - ---- - -## Appendix — verification notes and limits - -- No Rust toolchain available; nothing compiled or run. Findings are static traces through - the tarballs listed at the top. -- GitHub's REST API rate-limited partway through, so the #2559 check-runs list was never - read. `mergeable_state: unstable` is all that is known about CI. -- The `untrack` borrow-check question (§2.1) is a reasoned NLL argument, not a compiler - result. -- The modular-inverse figures in §4.3 were computed directly (`pow(M, -1, 2**N)`) and - round-tripped: `((2c+1)·M) mod 2^N == 1` for both widths. -- Reachability claims about counter exhaustion assume one counter increment per tracked - handle, which matches `track_by_id` being the single id source for `track_box`, - `track_arc`, and `track_arc_mutex`. -- The `_detect_opaque_handles` probe in §5.2 is proposed, not tested. Its v0.90 side rests - on malloc/dlmalloc returning 8- or 16-byte-aligned addresses — an allocator property, the - same one F2 flags as unenforced upstream. That is acceptable here only because the probe - fails toward the conservative branch; it should not be reused anywhere the failure - direction is reversed. -- The release-sequencing question in §5.2 and item 14 is unresolved and cannot be settled - from the repositories alone. It is stated as an open question, not an assumption. From a37c10fb551434af9e6691e2aa094bca096f76a3 Mon Sep 17 00:00:00 2001 From: tmathern <60901087+tmathern@users.noreply.github.com> Date: Sat, 29 Aug 2026 07:22:10 -0700 Subject: [PATCH 17/17] fix: Debug cleanup --- pr312-fix-plan.md | 240 -------------------------------------- pr312-verified-plan.md | 257 ----------------------------------------- 2 files changed, 497 deletions(-) delete mode 100644 pr312-fix-plan.md delete mode 100644 pr312-verified-plan.md diff --git a/pr312-fix-plan.md b/pr312-fix-plan.md deleted file mode 100644 index 97e0b159..00000000 --- a/pr312-fix-plan.md +++ /dev/null @@ -1,240 +0,0 @@ -# PR #312 — minimal fix plan - -**Target:** `src/c2pa/c2pa.py` on `mathern/error-slot-sentinel` (`df29505`). Line numbers from that revision. - -**Goal:** four small edits. Three fix defects confirmed by reproduction against the pinned -native build (v0.90.15); one makes the ownership logic harmless when v0.91 ships. Nothing -else changes. - -All four were applied to a working copy and re-verified — before/after output in each -section. - ---- - -## Fix 1 — After a consuming call has been issued, never free the handle - -**Why:** the ownership decision currently depends on the order in which the native library -validates arguments versus taking ownership, and that order is changing. - -| | `c2pa_reader_with_manifest_data_and_stream` | -|---|---| -| v0.90.15 / v0.90.16 | validate `format`/`stream`/`manifest_data`, **then** `untrack_or_return_null!(reader)` | -| `c2pa-rs` `main` (→ v0.91) | `untrack_or_return_null!(reader)` **first**; every early return drops it | - -Confirmed on the pinned build: - -``` -with_manifest_data_and_stream(size=0) -> "Other: InvalidBufferSize: 0 for 'manifest_data'" -c2pa_free(old reader) -> 0 => handle STILL TRACKED -``` - -So `_PRE_CONSUME_ERROR_TAGS` is right today and wrong after the bump: the same messages will -mean "native already dropped this" while the binding keeps the pointer and frees it later. -That is the recycled-address free the PR exists to prevent. - -Rather than teach the classifier to track the native ordering, remove ownership from the -classifier entirely. Under v0.91 the contract is "always consumed by native calls", so the -rule that is correct there and merely lossy on v0.90 is: **once `ffi_call` has been issued, -this binding never frees that handle.** - -### Edits - -`_raise_consume_failure` (lines 666–724) — all three branches collapse to the same action: - -```python - error = _read_native_error() - if error: - if ManagedResource._is_pre_consume_rejection(error): - logger.warning( - "%s: native call rejected the handle (%s); " - "marked consumed, not freed", - type(self).__name__, error) - self._teardown(free_handle=False) - _raise_typed_c2pa_error(error) - - logger.debug("%s: consuming call failed without setting error", - type(self).__name__) - self._teardown(free_handle=False) - raise C2paError(error_message.format("Unknown error")) -``` - -`_invoke_consume` (lines 657–664) — same rule; `ctypes.ArgumentError` is still re-raised -above, so any other exception means the call reached native: - -```python - except Exception as e: - # The call reached native, so ownership is native's to account for. - self._teardown(free_handle=False) - raise C2paError(error_message.format(e)) from e -``` - -Follow-on deletions, all mechanical: - -- `_raise_consume_failure`'s `previous_state` parameter and its two call sites - (lines 799, 817). -- `_invoke_consume`'s `reserved` keyword and its two call sites (lines 791, 809). -- `_abort_consume` (lines 745–753) is no longer reached from the failure classification. - It stays only for the `except` branches of `_consume_no_replacement` / `_consume_into`, - where the handle demonstrably never reached native. - -`_PRE_CONSUME_ERROR_TAGS` and `_is_pre_consume_rejection` stay exactly as they are. After -this change they only pick a log line. They no longer decide whether anything is freed, so -their correctness against a given native version stops mattering. - -### What this costs - -On v0.90 a pre-consume rejection leaves a handle the registry still tracks and we abandon -it — a bounded leak on an error path, in exchange for making a free of a possibly-recycled -address structurally impossible. On v0.91 it is simply correct. - -Retryability after a pre-consume rejection goes away: the resource is marked closed instead -of restored to ACTIVE. That promise is already false under v0.91, so it has to go regardless. -Update: - -- `Reader.with_fragment` docstring (lines 3234–3239) — drop the "can be retried" wording. -- `tests/perf/scenarios.py` — `with_fragment_pre_consume_rejection`. -- the unit test asserting the resource is restored (`test_pre_consume_rejection_restores_the_resource`). - -### Verified - -``` -Reader('image/jpeg', BytesIO(b'not an image')) -> NotSupported: type is unsupported -Reader(..., manifest_data=b"") -> Other: InvalidBufferSize: 0 for 'manifest_data' -``` - -Both raise the same typed errors as before; neither frees the handle. - ---- - -## Fix 2 — `_maybe_flush_pending` must re-register when a section blocks it - -**Why:** it returns early on `_in_native_section()` without calling -`_register_for_section_flush(self)`, so the deferral has no remaining flush path. -`Builder.sign` nests the guards — `signer._native_call()` (line 4313) inside -`self._native_call()` (line 4308) — so the inner exit runs while the outer section is still -open. A `signer.close()` from another thread is then stranded, and since -`_cleanup_resources` skips a CLOSED resource, `close()` and `__del__` are both no-ops -afterwards. Same for `_context_guard(self._context)` at line 4332. - -### Edit (lines 504–515) - -```python - def _maybe_flush_pending(self): - if is_foreign_process(self): - return - with self._state_lock(): - if self._pending_teardown is None: - return - if getattr(self, '_inflight', 0) > 0: - return - if _in_native_section(): - _register_for_section_flush(self) - return - free_handle, self._pending_teardown = self._pending_teardown, None - self._finish_teardown(free_handle) -``` - -Three changes: the `is_foreign_process` guard `_teardown` already has at line 438 (this -function is called from the section drain, where a child process would otherwise raise -`C2paError` out of an unrelated `with`); the missing registration; and moving -`_finish_teardown` inside the lock (Fix 3). - -### Verified - -Two `Context` objects, nested `_native_call`, `close()` from another thread: - -``` -before: pending=True handle set=True released=False (second close() also a no-op) -after: pending=None handle set=False released=True -``` - -The existing `test_context_close_during_sign_defers_teardown` covers only one nesting level -— adding an enclosing `with builder._native_call():` makes it fail before this edit. - ---- - -## Fix 3 — `_finish_teardown` idempotency - -**Why:** `_released` is set as the *first* statement of `_finish_teardown` (line 492) and -`_maybe_flush_pending` calls it outside the lock, so a second entrant that arrives before -that assignment runs the whole teardown again. This breaks the invariant -`test_concurrent_close_runs_release_once` states explicitly. - -### Edit (line 492) - -```python - if self._released: - return - self._released = True -``` - -Combined with running it under the lock (Fix 2), this closes both the duplicate `_release()` -and the narrow double-free window at line 496. - -### Verified - -Forced interleave, one resource, two threads: - -``` -before: _release ran 2 times -> ['B', 'A'] -after: _release ran 1 time -> ['A'] -``` - ---- - -## Fix 4 — Re-mark the slot after a failed free - -**Why:** `_free_native_ptr` logs a non-zero result and leaves -`UntrackedPointer: 0x` in the thread-local slot — and its own docstring calls that -"expected on the eager-free path". The next non-consuming failure that sets no error of its -own reads it back, with the wrong exception type. That is the defect this PR set out to -remove, displaced rather than fixed. - -### Edit (lines 385–390) - -```python - result = _lib.c2pa_free(ptr) - if result != 0: - logger.debug("c2pa_free returned %s for an untracked pointer ", result) - _mark_sentinel_no_native_error() - return result -``` - -### Verified - -``` -_free_native_ptr(0x9999) -> -1 -before: next unrelated failure -> _C2paOther: Other: UntrackedPointer: 0x9999 -after: next unrelated failure -> C2paError: unrelated later op failed: Unknown error -``` - ---- - -## Deliberately out of scope - -Raised in review, not fixed here — each is either subsumed or too large for this PR: - -- **`_abort_consume` reviving a resource with a queued teardown.** Reachable state - (`is_valid == True` with `_pending_teardown` set), but after Fix 1 it is no longer on the - failure-classification path. A one-line `if self._pending_teardown is not None: return` - can be added if the reviewer wants it; it is not required for correctness of the paths - that exist today. -- **`_release_handle` mutating state outside the lock** (lines 517–525). After Fix 1 its - last caller in the consume paths is gone. -- **`_native_section`'s `finally` masking the body's exception** (lines 1091–1092). Real, - but the fix touches the section generator's control flow; not worth the blast radius here. -- **`with_fragment` same-thread re-entrancy** (`RLock` + `blocking=False`, lines 3142, 3256). - Under Fix 1 a re-entered call no longer leads to a free of the reused handle. -- **Import-time `ImportError` from `_learn_sentinel_no_native_error_text`** (line 1450) and - the registry-mutex cost of planting the marker. Both are policy calls for the author. -- **Argument preflighting in Python** (rejecting `manifest_data=b""` and similar before the - FFI call). Worth doing, but it is a behavioural change across several public entry points - and Fix 1 already removes the ownership hazard it was proposed to close. - ---- - -## Landing order - -Fixes 2, 3 and 4 are independent of the native version and of each other; they can go in -first. Fix 1 carries the behaviour change and the test updates, and is the one that has to -land before v0.91. diff --git a/pr312-verified-plan.md b/pr312-verified-plan.md deleted file mode 100644 index 62559e1c..00000000 --- a/pr312-verified-plan.md +++ /dev/null @@ -1,257 +0,0 @@ -# PR #312 — verified plan (compatible with v0.90.15 today and v0.91.0 next) - -Everything below was checked against running code, not read off the two source -documents. Evidence tier is marked per claim: **ran** (executed, read the output), -**read** (opened the source at a named revision), **inferred** (reasoned, not confirmed). - -Revisions used: - -| Repo | Ref | Meaning | -|---|---|---| -| c2pa-python | `9f62daf` (`mathern/error-slot-sentinel`) | branch under review | -| c2pa-rs | `stable` = `0.90.15` | what the binding loads today | -| c2pa-rs | `origin/main` = `0.91.0-dev` (`be7f5ea2`) | what ships next | -| c2pa-rs | `pr2559` = `145b754a`, base `b3cd390a` | opaque-ids PR, not merged | - -Local native library after `make rebuild`: `c2pa-c-ffi/0.90.15 c2pa-rs/0.90.15`. - ---- - -## What the adversarial pass changed - -Three corrections to the input documents, each of which moves the plan. - -### The release-sequencing question is answered, and the answer is the opposite of the guess - -The review document (§5.2, item 14) leaves open whether #2559 and the always-consumed -contract ship together, and recommends building a behavioural probe -(`_detect_opaque_handles`) if they do not. That machinery is not needed. - -**Read** — the always-consumed ordering came from PR #2344, merged 2026-07-23, and is -already on `main`, independent of #2559: - -``` -78b5b709 2026-07-23 fix: builder style c_ffi_api functions will now consistently - consume the self parameter (#2344) -``` - -**Ran** — `git merge-base --is-ancestor 78b5b709 stable` reports #2344 is *not* in the -0.90 line, and `main` is `0.91.0-dev`. **Read** — the reordered body is identical on -`origin/main` and on `pr2559`, and its base `b3cd390a` already contains it, so #2559 did -not introduce it. - -Both changes therefore arrive in the same 0.91.0 release. The middle column of the review -document's table — opaque ids *with* ambiguous ownership — never exists as a shipped -release. **Drop `_detect_opaque_handles` entirely.** It is roughly 30 lines of probe plus -a CI consistency check built for a window that does not occur, and its own author's note -concedes its v0.90 side rests on the same unenforced allocator assumption that finding F2 -criticises upstream. - -### The ordering flip is real, and it is the reason Fix 1 is not optional - -**Read** — `c2pa_reader_with_manifest_data_and_stream`, the two revisions side by side: - -```rust -// stable (0.90.15) — validate first, reader still tracked on early return -let format = cstr_or_return_null!(format); -let stream = deref_mut_or_return_null!(stream, C2paStream); -let manifest_bytes = bytes_or_return_null!(manifest_data, manifest_size, "manifest_data"); -untrack_or_return_null!(reader, C2paReader); - -// main (0.91.0-dev) — take ownership first, every early return drops it -let reader = untrack_or_return_null!(reader, C2paReader); -let format = cstr_or_return_null!(format); -``` - -**Ran** — against the pinned 0.90.15 library, a rejected call leaves the handle alive: - -``` -c2pa_reader_with_manifest_data_and_stream(reader, "image/jpeg", NULL, NULL, 0) - -> NULL, error = 'NullParameter: stream' -c2pa_free(reader) -> 0 # 0 = still tracked, native did NOT consume it -``` - -After the bump the same message means the opposite. A classifier that decides ownership -from message text is therefore correct today and wrong on 0.91.0. - -### The address-reuse hazard is certain, not probabilistic - -The review document argues from glibc tcache LIFO that a stale free "very often" hits the -live replacement. **Ran** — on this macOS build it is not "often", it is total: - -``` -allocate -> free -> allocate (same type), 200 trials -address reuse: 200/200 = 100% -``` - -So on v0.90 a defensive free of a handle whose ownership is uncertain reliably destroys -the object that took its address. This is the strongest available justification for the -leak-over-free trade, and it is what makes Fix 1 the correct direction rather than merely -a forward-compatibility hedge. - ---- - -## Confirmed by execution before writing any code - -- **Marker candidates.** `c2pa_free(0) -> 0` with an empty error string, so `0` silently - disables the sentinel; `c2pa_free(8) -> -1` with `Other: UntrackedPointer: 0x8`. - **Read** — `PointerRegistry::free` short-circuits `key == 0` to `Ok(())`, which is the - mechanism behind that result. -- **Handle shape on v0.90.** Eight consecutive `c2pa_settings_new()` handles were all real - addresses, all 256-byte aligned, none below the first page. Both properties the marker - argument depends on hold on the library we actually load. -- **Scramble arithmetic**, recomputed independently rather than taken from the document: - `scr(0) == scr(2^(N-1))` on both widths, so the id period is 2^(N-1), confirming F1; the - 32-bit counter producing id `1` is `170286660`; no id is ever even, on any input. -- **Fix 2 and Fix 3 defects**, both **read** in the current source: `_maybe_flush_pending` - (line ~511) returns on `_in_native_section()` without calling - `_register_for_section_flush(self)`, while `_teardown` (line 474) does register on the - same condition — the deferral has no remaining flush path. `_finish_teardown` writes - `self._released = True` as its first statement (line 492) and is called outside the lock, - so a second entrant arriving before that write repeats the whole teardown. - -## Already done on this branch — no action - -- The sentinel assert is **already** derived from the constant (`marker_hex = - hex(_MARKER_ADDR)`) and **already** inside `_learn_sentinel_no_native_error_text()`. - Review items 11-part-two and 12 are complete; the document describes an older revision. -- `_PRE_CONSUME_ERROR_TAGS` is back to two entries. Commit `9f62daf` removed - `NullParameter:` and `InvalidBufferSize:`, so §4.4's four-entry description is stale. - -## Pre-existing breakage this plan must absorb - -**Ran** — on a clean tree after `make rebuild`: **4 failed, 482 passed**. All four trace to -`9f62daf` removing the two tags without updating the tests that assert the old behaviour. -They fail independently of anything proposed here. - -``` -test_invalid_buffer_size_rejection_retains_the_handle AssertionError: the retained handle was dropped -test_null_parameter_rejection_retains_the_handle AssertionError: the retained handle was dropped -test_repeated_rejections_do_not_accumulate_handles 10 of 10 handles leaked -test_native_rejections_observed_from_the_library_still_classify -``` - -The version test that failed before the rebuild (`'0.90.16' not found in '0.90.15'`) was a -stale-artifact problem and is now green. - ---- - -## The plan - -### 1. Fix 1 — once a consuming call has been issued, never free the handle - -Collapse the three branches of `_raise_consume_failure` and the `except` path of -`_invoke_consume` to `self._teardown(free_handle=False)`. Ownership stops being derived -from error text; the tags survive only to select a log line. - -Correct on 0.91.0 by contract. On 0.90.15 it accepts a bounded leak on an error path in -exchange for making a free of a reused address structurally impossible — which the 200/200 -measurement shows is the real hazard, not a theoretical one. - -Mechanical follow-ons: drop `_raise_consume_failure`'s `previous_state` parameter and -`_invoke_consume`'s `reserved` keyword with their call sites; leave `_abort_consume` for -the two `except` paths where the handle provably never reached native. - -### 2. Fix 2 — register for section flush when a native section blocks the drain - -Add the missing `_register_for_section_flush(self)`, add the `is_foreign_process` guard -`_teardown` already has, and move `_finish_teardown` inside the lock. - -### 3. Fix 3 — make `_finish_teardown` idempotent - -Guard on `self._released` before setting it. With Fix 2's locking this closes both the -duplicate `_release()` and the double-free window. - -### 4. Marker address `1` -> `8` - -Not reachable on shipped Python wheels (64-bit only; the colliding counter is ~2^62), so -this is hardening, not a live bug — I would rather say that plainly than overstate it. It -costs one line and removes a justification comment that is already wrong in its reasoning: -"allocations are aligned" stops being the relevant property once ids are synthetic. `8` is -below the first page (safe under v0.90 address keys) and even (safe under #2559 odd ids). -Not `0`, for the measured reason above. - -### 5. `C2paStream._fields_ = []` - -**Read** — every other opaque type in the file uses `_fields_ = []`; this one declares the -real layout. Nothing dereferences it today (`c2pa_create_stream` is called with -`context=None`), so there is no live bug — but it is the same shape as the confirmed -c2pa-cpp break at `file_stream.h:114`, where reading `stream->context` back out segfaults -once the pointer is an opaque id. - -### 6. Tests — remove what is obsolete, keep what still holds - -Per your instruction, and split deliberately rather than deleting all four failures: - -Remove — these assert retain-and-retry, a promise deleted by `9f62daf` and contradicted by -the 0.91.0 ordering: -- `test_null_parameter_rejection_retains_the_handle` -- `test_invalid_buffer_size_rejection_retains_the_handle` -- `test_pre_consume_rejection_restores_the_resource` -- `test_repeated_rejections_do_not_accumulate_handles` (asserts no leak; Fix 1 trades that - away knowingly) -- `test_with_fragment_pre_consume_rejection_keeps_handle` - -Amend rather than delete — `test_native_rejections_observed_from_the_library_still_classify` -pins the wording the library actually emits, which is worth keeping. Drop its -`c2pa_reader_from_stream(None, None)` half, whose `NullParameter` message is deliberately -no longer classified, and keep the `c2pa_free(0x9999)` half. - -Keep untouched: `test_pre_consume_tags_still_match_the_native_wording`, -`test_pre_consume_tag_match_is_substring_not_prefix`, -`test_caller_text_quoting_a_tag_is_not_a_rejection`, -`test_concurrent_close_runs_release_once`. - -Add: a regression test for Fix 2 that wraps the existing -`test_context_close_during_sign_defers_teardown` in a second `_native_call()`, since one -nesting level does not exercise the stranded path. - -### 7. Perf — the part that will fail silently if skipped - -**Read** — `run_profile.py` gates the run on `leaked_bytes` at a 1.1x threshold, and -`scenario_reader_with_fragment_pre_consume_rejection` asserts: - -```python -if reader._handle is None: - raise AssertionError("handle was dropped on a pre-consume rejection; ...") -``` - -Fix 1 makes that assertion fire on every iteration, and raises the scenario's -`leaked_bytes` (baseline `3417826`) by design. Both must move together: - -- Invert the scenario's assertion to expect the consumed-and-not-freed outcome, and drop - the `_is_pre_consume_rejection` requirement now that the tags no longer decide ownership. -- Re-measure and re-baseline that one entry, rather than raising the global threshold — - a threshold bump would mask unrelated regressions across the other 59 scenarios. -- Add a scenario covering the Fix 1 leak on a **non**-rejection consuming failure, so the - bounded leak has a tracked ceiling instead of being asserted in prose. -- Add a repeated close-under-nested-section scenario for Fix 2: the stranded-deferral bug - leaks a whole native object per occurrence, which is a memory signal the unit tests do - not quantify. - -### Out of scope, stated rather than silently dropped - -`_abort_consume` reviving a resource with a queued teardown; `_native_section`'s `finally` -masking the body's exception; Python-side argument preflighting. All three are noted in the -fix plan and none is required for the paths that exist after Fix 1. - -### Landing order - -Fixes 2, 3, 4 and 5 are independent of native version and of each other. Fix 1 carries the -behaviour change, the test removals and the perf re-baseline, and is the one that must land -before the 0.91.0 bump. - ---- - -## Limits - -- No 0.91.0 build exists to test against; the ordering claim is **read** from `origin/main` - source, and the runtime behaviour under 0.91.0 is **inferred** from it. -- #2559 is unmerged. If it lands in a later release than 0.91.0, the ownership conclusion - is unaffected — that rests on #2344, which is already on main — but the marker-address - reasoning in step 4 would be hardening ahead of a scheme not yet shipped. -- The 100% address-reuse measurement is this macOS allocator on this machine. The direction - generalises; the exact rate does not. -- Findings F1–F11 in the review document are comments on c2pa-rs #2559, not work in this - repository. I verified F1's arithmetic, F2's mechanism and F4's empty changelog; I did not - re-derive the rest, and none of them gates this plan.