From e34239134e3bc07abde5dbf3357b6381a12b5cd4 Mon Sep 17 00:00:00 2001 From: Tania Mathern Date: Tue, 21 Jul 2026 15:53:02 -0700 Subject: [PATCH 1/7] fix: Refactor --- src/c2pa/c2pa.py | 124 ++++++++++++++++----------- tests/test_unit_tests.py | 177 ++++++++++++++++++++++++++++----------- 2 files changed, 205 insertions(+), 96 deletions(-) diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index b78e06d4..8fc916e4 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -306,11 +306,13 @@ def _release(self): """ def _safe_release(self): - """Run _release(), logging (never raising) if it fails. + """Run _release(), logging and swallowing any error including an + interrupt so it never raises. Callers rely on this to always reach the + free step that follows. """ try: self._release() - except Exception: + except BaseException: logger.error( "Failed to release %s resources", type(self).__name__, @@ -337,9 +339,8 @@ def _mark_consumed(self): def _release_handle(self): """Free a native handle, then close the object holding it. - Freeing synchronously at the error site leaves no window in which the - address could be reused and re-tracked. - Sets CLOSED before releasing and freeing. + Sets CLOSED before releasing and freeing; _safe_release swallows any + interrupt so the free below always runs. """ if is_foreign_process(self): @@ -419,49 +420,59 @@ def _swap_handle(self, new_handle): # so it is still ours to deal with. _PRE_CONSUME_ERROR_TAGS = ("UntrackedPointer:", "WrongPointerType:") - def _consume_and_swap(self, ffi_call, error_message): - """Run an FFI call that consumes this handle and returns a replacement. + def _invoke_consume(self, ffi_call, error_message): + """Run an FFI call that consumes this handle, returning its raw result. - On success the native lib consumed the handle and returned a new one, - which we swap in. - On failure (null return, or an exception from the callback) the input - is freed eagerly and synchronously right here, then the error is raised. - The error is read before the input is freed, so the message is not lost - (in case the release steps would set a pointer tracking error). - The native error slot is sticky and thread-local: the SDK does not - clear it before this call. - The error handling therefore trusts that a failing native path set - its own error (overwriting previous one). + A marshalling ArgumentError is re-raised untouched: the call never + reached the native side, so the handle is still ours and unchanged. Any + other exception from the call frees the handle before raising. + + The caller inspects the returned result to tell success from failure + (the convention differs per call) and routes a failure to + _raise_consume_failure. Args: - ffi_call: Callable taking the current handle, returning the - replacement or null. - error_message: Format string with one placeholder, used when the - native layer offers no error of its own. + ffi_call: Callable taking the current handle, returning the native + result (a replacement pointer, a status code, ...). + error_message: Format string with one placeholder, used to wrap a + callback exception. Raises: - C2paError: If the call fails, typed by native error when one. + ctypes.ArgumentError: If marshalling failed; handle untouched. + C2paError: If the call raised any other exception. """ try: - new_ptr = ffi_call(self._handle) + return ffi_call(self._handle) except ctypes.ArgumentError: - # Marshalling failed. The call never reached the native side, - # and the handle is untouched. raise except BaseException as e: self._release_handle() raise C2paError(error_message.format(e)) from e - if new_ptr: - self._swap_handle(new_ptr) - return + def _raise_consume_failure(self, error_message): + """Raise the error from a consume call that reached native and failed. + + The native error is read before any free so a defensive 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. - # Retrieve the error + A pre-consume tag means the native side rejected a pointer before taking + ownership, so the handle is still ours and is retained. Any other error + means ownership transferred before the failure, so the handle is freed + eagerly (the native lib handles not double-freeing) before raising. + + Args: + error_message: Format string with one placeholder, used when the + native layer offers no error of its own. + + Raises: + C2paError: Always; typed by the native error when there is one. + """ error = _read_native_error() if error: if any(tag in error for tag in ManagedResource._PRE_CONSUME_ERROR_TAGS): - # Rejected before ownership transferred: the handle is still ours logger.warning( "%s: native call rejected the handle before taking " "ownership (%s); handle retained", @@ -469,14 +480,37 @@ def _consume_and_swap(self, ffi_call, error_message): error) _raise_typed_c2pa_error(error) - # Ownership transferred and then the operation failed: - # free eagerly (native lib handles not double-freeing), then raise. self._release_handle() _raise_typed_c2pa_error(error) self._release_handle() raise C2paError(error_message.format("Unknown error")) + def _consume_and_swap(self, ffi_call, error_message): + """Run an FFI call that consumes this handle and returns a replacement. + + On success the native lib consumed the handle and returned a new one, + which we swap in. A null return is a failure routed to + _raise_consume_failure. + """ + new_ptr = self._invoke_consume(ffi_call, error_message) + if new_ptr: + self._swap_handle(new_ptr) + return + self._raise_consume_failure(error_message) + + def _consume_no_replacement(self, ffi_call, error_message): + """Run an FFI call that consumes this handle on success, when the native + call returns a status code (0 = success) rather than a replacement + handle. A non-zero status is a failure routed to + _raise_consume_failure. + """ + result = self._invoke_consume(ffi_call, error_message) + if result == 0: + self._mark_consumed() + return + self._raise_consume_failure(error_message) + @classmethod def _wrap_native_handle(cls, handle): """Build a brand-new instance around an already-valid, @@ -1720,29 +1754,21 @@ def __init__( if signer is not None: signer._ensure_valid_state() - # c2pa_context_builder_set_signer takes ownership of the - # signer pointer , on its error path as well as on success. + # _consume_no_replacement makes sure that a rejected + # signer is retained instead of being closed and leaked. self._signer_callback_cb = signer._callback_cb - try: - result = ( - _lib.c2pa_context_builder_set_signer( - builder_ptr, signer._handle, - ) - ) - except ctypes.ArgumentError: - # Marshalling failed, so the call never reached the - # native side and the signer was never taken. - raise - signer._mark_consumed() - if result != 0: - _parse_operation_result_for_error(None) + signer._consume_no_replacement( + lambda h: _lib.c2pa_context_builder_set_signer( + builder_ptr, h), + "Failed to set signer on Context: {}") self._has_signer = True - # Build consumes builder_ptr + # Build consumes the builder only on success. context_ptr = ( _lib.c2pa_context_builder_build(builder_ptr) ) - builder_ptr = None + if context_ptr: + builder_ptr = None _check_ffi_operation_result( context_ptr, "Failed to build Context" diff --git a/tests/test_unit_tests.py b/tests/test_unit_tests.py index 58db24d7..d33eec84 100644 --- a/tests/test_unit_tests.py +++ b/tests/test_unit_tests.py @@ -6934,9 +6934,10 @@ class TestManagedResourceLifecycle(unittest.TestCase): the _owner_pid stamp that governs which process may free a handle, and the ownership hand-offs between Python and the native library. - setUp records frees instead of performing them, so a miscount reads as a - leak or a double-free rather than a crash. - Tests holding real handles call _use_real_frees() first. + For testing: setUp records frees instead of performing them, + so a miscount reads as memory handling issue. + Tests releasing real handles call _use_real_frees() first to + restore release behavior. """ class _FakeHandleResource(ManagedResource): @@ -6961,10 +6962,10 @@ def _release(self): self.release_calls += 1 class _ExtenderResource(ManagedResource): - """Am extender that owns a raw handle and wraps it via + """For testing: An extender that owns a raw handle and wraps it via _wrap_native_handle. It carries several attributes of its own, all defaulted in _init_attrs (not __init__), and _release reads them, so a - missing attribute would surface as an AttributeError on teardown. + missing attribute would surface as an AttributeError on test teardown. """ def _init_attrs(self): @@ -6978,6 +6979,13 @@ def _release(self): self.buffer.append(self.label) self.released = True + class _InterruptingReleaseResource(ManagedResource): + """_release() raises KeyboardInterrupt, an interrupt _safe_release() + does not catch, to check the handle is still freed on the way out.""" + + def _release(self): + raise KeyboardInterrupt + def setUp(self): self.data_dir = FIXTURES_DIR self.freed = [] @@ -7080,8 +7088,7 @@ def test_swap_handle_does_not_free_consumed_handle(self): res._swap_handle(0xAAA2) - # The FFI already owns and frees the old pointer, - # so freeing it here would be a double-free. + # The FFI already owns and frees the old pointer. self.assertEqual(self.freed, []) self.assertEqual(res._handle, 0xAAA2) @@ -7188,7 +7195,7 @@ def test_foreign_child_skips_free_for_wrapped_and_swapped(self): self.assertEqual(swapped._lifecycle_state, LifecycleState.CLOSED) self.assertIsNone(swapped._handle) - # A second foreign teardown is a no-op: still nothing freed. + # A second foreign teardown is a no-op. wrapped.close() swapped.close() self.assertEqual(self.freed, []) @@ -7203,8 +7210,8 @@ def test_owning_process_frees_wrapped_and_swapped_exactly_once(self): swapped._swap_handle(0xC6) swapped.close() - # 0xC5 was consumed by the (simulated) FFI swap, - # so only the replacement is ours to free. + # 0xC5 was consumed by the test FFI swap. + # Only the replacement must be freed here. self.assertEqual(self._free_counts(), {0xC4: 1, 0xC6: 1}) def test_foreign_child_skips_release(self): @@ -7233,8 +7240,8 @@ def test_consumed_resource_frees_nothing_in_either_process(self): self.assertEqual(self.freed, []) - # Consuming a handle hands the native pointer to a new owner, - # but the Python-side resources are still ours and we need to free. + # Consuming a handle hands the native pointer to a new owner. + # The Python-side resources are still ours to free. def test_mark_consumed_releases_python_resources(self): res = self._ReleaseRecordingResource() res._activate(0xF1) @@ -7278,7 +7285,7 @@ def test_extender_wraps_handle_fully_built(self): self.assertTrue(obj.is_valid) self.assertEqual(obj._owner_pid, os.getpid()) - # _release reads those attributes, so a missing one would raise here. + # _release reads those attributes, so a missing one will raise here. obj.close() obj.close() @@ -7298,14 +7305,14 @@ def test_extender_foreign_teardown_skips_native_free(self): self.assertEqual(self.freed, [], "forked child freed a handle its parent still owns") self.assertFalse(obj.released, "foreign teardown ran _release") - # The child marks its own copy closed and nulls the handle: safe (the - # parent holds a separate copy) and it stops the child reusing a - # parent-owned handle. + # The child marks its own copy closed and nulls the handle: + # safe (the parent holds a separate copy) and it stops the + # child reusing a parent-owned handle. self.assertEqual(obj._lifecycle_state, LifecycleState.CLOSED) self.assertIsNone(obj._handle) - # A second foreign teardown stays a no-op, and any operation on the - # now-closed child copy fails loudly instead of reaching native code. + # A second foreign teardown stays a no-op, + # and any operation on the now-closed child copy fails. obj.close() self.assertEqual(self.freed, []) with self.assertRaises(Error): @@ -7368,7 +7375,7 @@ def test_context_with_signer_consumes_it_on_success(self): def test_construction_failure_leaves_nothing_to_free(self): # Activation happens after the null check, so a failed construction - # leaves no handle on the object for __del__ to find. + # has no handle on the object that __del__ can find. real_new = c2pa_module._lib.c2pa_context_new c2pa_module._lib.c2pa_context_new = lambda: None try: @@ -7385,6 +7392,91 @@ def test_construction_failure_leaves_nothing_to_free(self): finally: c2pa_module._lib.c2pa_builder_from_json = real_json + def test_context_build_null_return_frees_builder(self): + # build's only null path is a pre-consume rejection, which leaves the + # builder tracked and alive, so Context must free it on the way out. + settings = Settings() + real_build = c2pa_module._lib.c2pa_context_builder_build + c2pa_module._lib.c2pa_context_builder_build = lambda ptr: None + try: + with self.assertRaises(Error): + Context(settings=settings) + finally: + c2pa_module._lib.c2pa_context_builder_build = real_build + + # One free: the un-consumed builder. Settings borrows, so it is not + # freed here. + self.assertEqual(len(self.freed), 1, + "un-consumed builder leaked on build failure") + settings.close() + + def test_consume_no_replacement_marks_consumed_on_success(self): + res = self._FakeHandleResource() + res._activate(0xCAFE) + + res._consume_no_replacement(lambda h: 0, "set failed: {}") + + # Native took ownership: nothing freed here, handle nulled and closed. + self.assertEqual(self.freed, []) + self.assertIsNone(res._handle) + self.assertEqual(res._lifecycle_state, LifecycleState.CLOSED) + + def test_consume_no_replacement_retains_on_pre_consume_tag(self): + res = self._FakeHandleResource() + res._activate(0xCAFE) + real_read = c2pa_module._read_native_error + c2pa_module._read_native_error = lambda: "UntrackedPointer: rejected" + try: + with self.assertRaises(Error): + res._consume_no_replacement(lambda h: -1, "set failed: {}") + finally: + c2pa_module._read_native_error = real_read + + # Rejected before ownership transferred: handle retained and usable. + self.assertEqual(res._handle, 0xCAFE) + self.assertEqual(res._lifecycle_state, LifecycleState.ACTIVE) + self.assertEqual(self.freed, []) + res.close() + self.assertEqual(self.freed, [0xCAFE]) + + def test_consume_no_replacement_frees_on_other_error(self): + res = self._FakeHandleResource() + res._activate(0xCAFE) + real_read = c2pa_module._read_native_error + c2pa_module._read_native_error = lambda: "OtherError: boom" + try: + with self.assertRaises(Error): + res._consume_no_replacement(lambda h: -1, "set failed: {}") + finally: + c2pa_module._read_native_error = real_read + + # Ownership transferred, then the operation failed: freed once, closed. + self.assertEqual(self.freed, [0xCAFE]) + self.assertIsNone(res._handle) + self.assertEqual(res._lifecycle_state, LifecycleState.CLOSED) + + def test_interrupt_in_release_still_frees_handle(self): + res = self._InterruptingReleaseResource() + res._activate(0xDEAD) + + # _safe_release swallows the interrupt, so the free still runs and the + # handle is not stranded CLOSED-but-unfreed. + res._release_handle() + + self.assertEqual(self.freed, [0xDEAD]) + self.assertIsNone(res._handle) + self.assertEqual(res._lifecycle_state, LifecycleState.CLOSED) + + def test_interrupt_in_cleanup_still_frees_handle(self): + res = self._InterruptingReleaseResource() + res._activate(0xBEEF) + + res._cleanup_resources() + + self.assertEqual(self.freed, [0xBEEF]) + self.assertIsNone(res._handle) + self.assertEqual(res._lifecycle_state, LifecycleState.CLOSED) + class TestManagedResourceObjects(TestContextAPIs): """Tests native resource handling management when managed manually. @@ -7403,11 +7495,6 @@ def _ptr_addr(ptr): def _instrument_frees(self): """Record frees instead of performing them, and restore on teardown. - - This patches the base class, so every ManagedResource freed while the - patch is installed lands in the list, including objects the garbage - collector reclaims mid-test. Ask _free_count() about one handle rather - than asserting on the length of the list. """ freed = [] real_free = ManagedResource._free_native_ptr @@ -7545,7 +7632,7 @@ def test_builder_with_archive_swaps_the_handle(self): self.assertNotEqual(builder._handle, original_handle, "the native handle was not replaced") self.assertEqual(builder._lifecycle_state, LifecycleState.ACTIVE) - # The replacement came from this process, so the stamp still applies. + # The replacement came from this process, the stamp still applies. self.assertEqual(builder._owner_pid, original_stamp) self.assertEqual(builder._owner_pid, os.getpid()) builder.close() @@ -7589,8 +7676,7 @@ def test_swapped_builder_is_freed_exactly_once(self): builder.close() builder.close() - # Only the replacement is ours to free: the original was consumed by - # the FFI call that returned it. + # Only the replacement is must be freed here. self.assertEqual(self._free_count(freed, swapped_handle), 1) self.assertEqual(self._free_count(freed, original_handle), 0) @@ -7632,8 +7718,8 @@ def test_context_consumes_signer_but_not_settings(self): def test_consumed_signer_close_frees_nothing(self): signer = self._ctx_make_signer() - # Captured before the context consumes it: close() nulls the handle, - # so afterwards there is no pointer left to identify the free by. + # Captured before the context consumes it: + # close() nulls the handle, there is no pointer left to identify the free by. signer_handle = signer._handle context = Context(signer=signer) self.addCleanup(context.close) @@ -7663,15 +7749,15 @@ def test_builder_with_archive_null_return_frees_self(self): finally: c2pa_module._lib.c2pa_builder_with_archive = real_call - # Nothing left to own after failing + # Nothing left to own after failing. self.assertIsNone(builder._handle) self.assertEqual(builder._lifecycle_state, LifecycleState.CLOSED) - # The error path frees the old handle. + # Error frees the old handle. self.assertEqual(self._free_count(freed, released_handle), 1, "error path did not free the old handle exactly once") - # close() must not free it again. + # close() must not free again. builder.close() self.assertEqual(self._free_count(freed, released_handle), 1, "close() double-freed an already-released handle") @@ -7690,8 +7776,8 @@ def test_reader_with_fragment_null_return_frees_self(self): c2pa_module._lib.c2pa_reader_with_fragment = ( lambda r, f, s, frag: None) - # Instrument before the failure so the eager free on the error path is - # counted, not just whatever close() does afterwards. + # Instrument before failure so the eager free is counted, + # not just whatever close() does afterwards. freed = self._instrument_frees() try: with open(init_path, "rb") as init, \ @@ -7716,8 +7802,7 @@ def test_reader_with_fragment_null_return_frees_self(self): def test_reader_with_fragment_ffi_raise_frees_self(self): # If the ctypes call itself raises, the failure runs # through the except BaseException branch, which frees the handle. - # with_fragment must free exactly once and leave nothing for - # close() to double-free. + # with_fragment must free exactly once and leave nothing for close(). 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: @@ -7752,9 +7837,9 @@ def _raise(*_args): self.assertEqual(self._free_count(freed, released_handle), 1, "close() freed a handle the error path already freed") - # Consume-and-return ownership: the native call takes the handle partway - # through its body, so a null return does not say on its own whether the - # handle was consumed. These pin the classification down. + # Consume-and-return ownership: the native call can take the handle partway + # through the call, so a null return does not always say on its own + # whether the handle was consumed or not, warranting further checks/bookkeeping. @staticmethod def _is_pre_consume_rejection(error_message): @@ -7789,8 +7874,8 @@ def _untracked_reader_handle(): buf) def test_with_fragment_pre_consume_rejection_keeps_handle(self): - # Rejected before Box::from_raw, so nothing was consumed and the - # handle is still ours. Dropping it here would leak it. + # 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: @@ -7830,15 +7915,14 @@ def test_with_fragment_pre_consume_rejection_does_not_leak(self): reader.with_fragment("video/mp4", init, frag) finally: reader._handle = real_handle - # Still owns a working handle every time round, so nothing - # leaked: a dropped handle would leave the reader closed. + # Still owns a working handle every time round. self.assertEqual(reader._lifecycle_state, LifecycleState.ACTIVE) self.assertTrue(reader.json()) reader.close() def test_with_archive_post_consume_failure_consumes_handle(self): - # Ownership taken, then the operation failed: the handle is gone, - # so close() must not free it again. + # Ownership taken, then the operation failed: + # The handle is gone, so close() must not free it again. builder = Builder(json.dumps( {"claim_generator_info": [{"name": "test", "version": "0.1"}], "assertions": []})) @@ -7856,8 +7940,7 @@ def test_with_archive_post_consume_failure_consumes_handle(self): "close() freed a handle the FFI already consumed") def test_with_fragment_marshalling_error_keeps_handle(self): - # Never reaches native code, so nothing was consumed. The old - # blanket except marked it consumed here and leaked the handle. + # Never reaches native code, so nothing was consumed. init_path = os.path.join(FIXTURES_DIR, "dashinit.mp4") with open(init_path, "rb") as init: reader = Reader("video/mp4", init) From 4d4bc2acb7b06214b8469c4f46fc2dad2aa55e9c Mon Sep 17 00:00:00 2001 From: Tania Mathern Date: Tue, 21 Jul 2026 16:32:14 -0700 Subject: [PATCH 2/7] fix: Refactor --- src/c2pa/c2pa.py | 27 ++++++++------------------- tests/test_unit_tests.py | 12 +++++------- 2 files changed, 13 insertions(+), 26 deletions(-) diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index 8fc916e4..78687926 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -306,9 +306,7 @@ def _release(self): """ def _safe_release(self): - """Run _release(), logging and swallowing any error including an - interrupt so it never raises. Callers rely on this to always reach the - free step that follows. + """Run _release() safely (no raise), log on error. """ try: self._release() @@ -339,8 +337,7 @@ def _mark_consumed(self): def _release_handle(self): """Free a native handle, then close the object holding it. - Sets CLOSED before releasing and freeing; _safe_release swallows any - interrupt so the free below always runs. + Sets closed before releasing and freeing.. """ if is_foreign_process(self): @@ -450,18 +447,13 @@ def _invoke_consume(self, ffi_call, error_message): raise C2paError(error_message.format(e)) from e def _raise_consume_failure(self, error_message): - """Raise the error from a consume call that reached native and failed. + """Raise the error from an FFI handler consuming call. - The native error is read before any free so a defensive free's own + 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. - A pre-consume tag means the native side rejected a pointer before taking - ownership, so the handle is still ours and is retained. Any other error - means ownership transferred before the failure, so the handle is freed - eagerly (the native lib handles not double-freeing) before raising. - Args: error_message: Format string with one placeholder, used when the native layer offers no error of its own. @@ -488,10 +480,8 @@ def _raise_consume_failure(self, error_message): def _consume_and_swap(self, ffi_call, error_message): """Run an FFI call that consumes this handle and returns a replacement. - On success the native lib consumed the handle and returned a new one, - which we swap in. A null return is a failure routed to - _raise_consume_failure. + which we swap in. A null return is a failure. """ new_ptr = self._invoke_consume(ffi_call, error_message) if new_ptr: @@ -1754,8 +1744,8 @@ def __init__( if signer is not None: signer._ensure_valid_state() - # _consume_no_replacement makes sure that a rejected - # signer is retained instead of being closed and leaked. + # makes sure that a rejected signer is retained + # instead of being closed and leaked. self._signer_callback_cb = signer._callback_cb signer._consume_no_replacement( lambda h: _lib.c2pa_context_builder_set_signer( @@ -1763,13 +1753,12 @@ def __init__( "Failed to set signer on Context: {}") self._has_signer = True - # Build consumes the builder only on success. + # Build consumes builder_ptr. context_ptr = ( _lib.c2pa_context_builder_build(builder_ptr) ) if context_ptr: builder_ptr = None - _check_ffi_operation_result( context_ptr, "Failed to build Context" ) diff --git a/tests/test_unit_tests.py b/tests/test_unit_tests.py index d33eec84..f6a489a2 100644 --- a/tests/test_unit_tests.py +++ b/tests/test_unit_tests.py @@ -7404,8 +7404,8 @@ def test_context_build_null_return_frees_builder(self): finally: c2pa_module._lib.c2pa_context_builder_build = real_build - # One free: the un-consumed builder. Settings borrows, so it is not - # freed here. + # One free: the un-consumed builder. + # Settings borrows, so it is not freed here. self.assertEqual(len(self.freed), 1, "un-consumed builder leaked on build failure") settings.close() @@ -7416,7 +7416,7 @@ def test_consume_no_replacement_marks_consumed_on_success(self): res._consume_no_replacement(lambda h: 0, "set failed: {}") - # Native took ownership: nothing freed here, handle nulled and closed. + # Native took ownership. self.assertEqual(self.freed, []) self.assertIsNone(res._handle) self.assertEqual(res._lifecycle_state, LifecycleState.CLOSED) @@ -7432,7 +7432,7 @@ def test_consume_no_replacement_retains_on_pre_consume_tag(self): finally: c2pa_module._read_native_error = real_read - # Rejected before ownership transferred: handle retained and usable. + # Rejected before ownership transferred: handle retained. self.assertEqual(res._handle, 0xCAFE) self.assertEqual(res._lifecycle_state, LifecycleState.ACTIVE) self.assertEqual(self.freed, []) @@ -7450,7 +7450,7 @@ def test_consume_no_replacement_frees_on_other_error(self): finally: c2pa_module._read_native_error = real_read - # Ownership transferred, then the operation failed: freed once, closed. + # Ownership transferred, then the operation failed: freed, closed. self.assertEqual(self.freed, [0xCAFE]) self.assertIsNone(res._handle) self.assertEqual(res._lifecycle_state, LifecycleState.CLOSED) @@ -7459,8 +7459,6 @@ def test_interrupt_in_release_still_frees_handle(self): res = self._InterruptingReleaseResource() res._activate(0xDEAD) - # _safe_release swallows the interrupt, so the free still runs and the - # handle is not stranded CLOSED-but-unfreed. res._release_handle() self.assertEqual(self.freed, [0xDEAD]) From 1883b3ae8395aae6075660d6dda347c1c9a054cc Mon Sep 17 00:00:00 2001 From: Tania Mathern Date: Tue, 21 Jul 2026 16:40:34 -0700 Subject: [PATCH 3/7] fix: Typo --- tests/test_unit_tests.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_unit_tests.py b/tests/test_unit_tests.py index f6a489a2..d7ef3e38 100644 --- a/tests/test_unit_tests.py +++ b/tests/test_unit_tests.py @@ -7306,7 +7306,7 @@ def test_extender_foreign_teardown_skips_native_free(self): "forked child freed a handle its parent still owns") self.assertFalse(obj.released, "foreign teardown ran _release") # The child marks its own copy closed and nulls the handle: - # safe (the parent holds a separate copy) and it stops the + # the parent holds a separate copy and it stops the # child reusing a parent-owned handle. self.assertEqual(obj._lifecycle_state, LifecycleState.CLOSED) self.assertIsNone(obj._handle) From 927f84608b1404595d1387e4126b8e6ec2465888 Mon Sep 17 00:00:00 2001 From: Tania Mathern Date: Tue, 21 Jul 2026 17:30:22 -0700 Subject: [PATCH 4/7] fix: refactor --- docs/native-resources-management.md | 19 +-- src/c2pa/c2pa.py | 230 +++++++++++----------------- tests/test_unit_tests.py | 54 ++----- 3 files changed, 113 insertions(+), 190 deletions(-) diff --git a/docs/native-resources-management.md b/docs/native-resources-management.md index 1b01362e..87d1052d 100644 --- a/docs/native-resources-management.md +++ b/docs/native-resources-management.md @@ -142,7 +142,7 @@ Each transition has one method that performs it, and subclasses must go through | `_activate(handle)` | UNINITIALIZED to ACTIVE | Rejects a null handle, and refuses to run on an already-activated resource. A rejected activation leaves the object exactly as it was. | | `_swap_handle(new_handle)` | ACTIVE to ACTIVE | Requires the resource to already be active and the replacement to be non-null. Used when an FFI call consumed the old handle and returned a new one. | | `_mark_consumed()` | ACTIVE to CLOSED | Drops the handle without freeing it, for when ownership passed to the native side (e.g. `Signer` into `Context`). Runs `_release()` first, so subclass cleanup still happens. Unlike the other two, it validates nothing. | -| `_release_handle()` | ACTIVE to CLOSED | Frees the handle eagerly and closes the object, for the consume-and-swap failure path where the caller must free. Same post-state as `_mark_consumed()`; the difference is the extra `c2pa_free`. | +| `_release_handle()` | ACTIVE to CLOSED | Frees the handle eagerly and closes the object, for failure paths where ownership is unknown (the free is guarded, so a `-1` no-op if the native side already dropped it). Same post-state as `_mark_consumed()`; the difference is the extra `c2pa_free`. | Because activation is the only way in, no code path can leave an object ACTIVE while holding a null handle. @@ -357,21 +357,14 @@ The helper exists because a null return can be ambiguous. The native function va | Native error | Who owns the handle | What the helper does | | --- | --- | --- | | `UntrackedPointer:` or `WrongPointerType:` | Still ours: rejected before ownership moved | Handle kept, resource stays `ACTIVE`, typed error raised. Normal cleanup frees it later. | -| Any other error | Assumed taken, then the operation failed | `_release_handle()` frees the handle eagerly here, resource goes `CLOSED`, error typed from the native message. | -| No error at all | Same ownership conclusion, nothing to type from | `_release_handle()` frees eagerly, the caller's message is raised with `"Unknown error"` filled in. | - -Note the failure path frees eagerly (`_release_handle()`), it does not run `_mark_consumed()`. This is the dual-contract behaviour described below. +| Any other error | Taken, then the operation failed | `_mark_consumed()`: the native side already dropped the value, so nothing is freed here; resource goes `CLOSED`, error typed from the native message. | +| No error at all | Unknown (no released path reaches here) | `_release_handle()` guarded free, the caller's message is raised with `"Unknown error"` filled in. | This triage relies on the native error still being readable after the call returns. Reading an error copies the message out and frees the copy, but leaves the native slot set until the next error overwrites it, and the SDK does not clear it before these calls. -#### Why the failure path frees eagerly (dual contract) - -Two native contracts are in play, and the eager free is correct under both: - -- **Try-assign native (incoming):** the consuming call restores the original value to the caller on error and hands the pointer back, so the caller *must* free it. The eager `_release_handle()` is mandatory here or the handle leaks on every failed `with_fragment`/`with_archive`. -- **Released native (`c2pa-v0.90.0`):** the consuming call has already dropped the value by the time null returns, so the address is untracked. A redundant `c2pa_free` against it is a guarded no-op (returns `-1`, memory untouched), not a double-free. +#### Why a non-tag error does not free -One Python path, correct under both. The native-side details (which release path drops vs. restores) are not visible from this repo — the native library is consumed as a prebuilt binary — so treat the two contracts above as the assumed native behaviour this code is written against. +The binding targets one native contract, the released C FFI (`c2pa-v0.90.0`). Its consuming calls reject a borrowed pointer up front with a `_PRE_CONSUME_ERROR_TAGS` tag (handle retained), or take ownership and, on any later failure, drop the value themselves. So a non-tag error means the value is already gone: `_mark_consumed()` is exact, and a `c2pa_free` there would be a guarded no-op that only dirties the sticky error slot and risks racing a recycled address in another thread. `_release_handle()` stays only where ownership is genuinely unknown — an async exception mid-call, or the no-error fallthrough no released path produces — where the guarded free is the right default (a real free if the handle is ours, a `-1` no-op if not). The native-side details are not visible from this repo (the library is a prebuilt binary), so treat the contract above as the assumed native behaviour this code is written against. ### Adopting the handle before giving it away @@ -525,7 +518,7 @@ class NativeResource(ManagedResource): - `_release()` can be called more than once (via `close()` then `__del__`, or multiple `close()` calls). Make sure it handles being called on an already-cleaned-up object. Setting attributes to `None` after closing them is the standard pattern. -- Calling `c2pa_free` directly is not recommended. `ManagedResource` handles this. A redundant free of an already-released pointer is not a crash: the native pointer registry rejects an untracked address without touching memory and returns `-1`. `ManagedResource` relies on this guard so the consume-and-swap failure path can free eagerly without risking a double-free. Still, do not free manually — the lifecycle owns the pointer and bypassing it defeats the state checks. +- Calling `c2pa_free` directly is not recommended. `ManagedResource` handles this. A redundant free of an already-released pointer is not a crash: the native pointer registry rejects an untracked address without touching memory and returns `-1`. `ManagedResource` relies on this guard so the unknown-ownership failure paths can free eagerly without risking a double-free. Still, do not free manually — the lifecycle owns the pointer and bypassing it defeats the state checks. - If a subclass inherits from both `ManagedResource` and an ABC like `ContextProvider`, and both define a property with the same name (e.g. `is_valid`), Python resolves it using the MRO. The parent listed first in the class definition wins. If the ABC is listed first, Python finds the abstract property before the concrete one and raises `TypeError: Can't instantiate abstract class`. Always list the class with the concrete implementation first (e.g. `class Context(ManagedResource, ContextProvider)`, not `class Context(ContextProvider, ManagedResource)`). diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index 78687926..7dbd1d71 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -236,11 +236,12 @@ class ManagedResource: - Call `_swap_handle(new_handle)` instead when an FFI call consumed the current handle and returned a replacement (the success side of `_consume_and_swap`). - - Call `_mark_consumed()` when an FFI call took ownership of the handle - without returning a replacement (e.g. `Signer` into `Context`). - - Call `_release_handle()` when a consuming FFI call fails and the caller - must free the handle: it frees eagerly, then closes like - `_mark_consumed()`. `_consume_and_swap` uses it on the failure path. + - Call `_teardown(free_handle=False)` when an FFI call took ownership of + the handle without returning a replacement (e.g. `Signer` into + `Context`): the new owner frees it, so this does not. + - Call `_release_handle()` when a consuming FFI call fails with ownership + unknown: it frees eagerly (guarded), then closes. `_consume_and_swap` + uses it on that failure path. - Override `_release()` to free class-specific resources (streams, caches, callbacks, etc.), called before the native pointer is freed. @@ -306,55 +307,34 @@ def _release(self): """ def _safe_release(self): - """Run _release() safely (no raise), log on error. + """Run _release(), logging and swallowing ordinary errors so teardown + continues. Interrupts (KeyboardInterrupt, SystemExit) propagate. """ try: self._release() - except BaseException: + except Exception: logger.error( "Failed to release %s resources", type(self).__name__, exc_info=True, ) - def _mark_consumed(self): - """Mark as consumed by an FFI call that took ownership - of native resources e.g. pointers. This means we should not - call clean-up here anymore, and leave it to the new owner. - """ - - if is_foreign_process(self): - self._handle = None - self._lifecycle_state = LifecycleState.CLOSED - return - - # Callers raise straight after consuming, so a failing _release() - # here would mask the error they are reporting. - self._safe_release() - - self._handle = None - self._lifecycle_state = LifecycleState.CLOSED + def _teardown(self, free_handle: bool): + """Close the object: run _release, optionally free the handle, null it. - def _release_handle(self): - """Free a native handle, then close the object holding it. - Sets closed before releasing and freeing.. + The one place that owns the foreign-process rule, the CLOSED ordering + and the guarded free. free_handle=False (consumed) frees nothing; the + new owner does. """ - if is_foreign_process(self): self._handle = None self._lifecycle_state = LifecycleState.CLOSED return - # Nothing to free, normalize states. - if self._lifecycle_state != LifecycleState.ACTIVE: - self._handle = None - self._lifecycle_state = LifecycleState.CLOSED - return - self._lifecycle_state = LifecycleState.CLOSED self._safe_release() - if self._handle: + if free_handle and self._handle: try: ManagedResource._free_native_ptr(self._handle) except Exception: @@ -364,6 +344,18 @@ def _release_handle(self): exc_info=True) finally: self._handle = None + else: + self._handle = None + + def _release_handle(self): + """Free this handle, then close the object. Used only where ownership is + unknown (a guarded free is a real free if ours, a no-op if not). + """ + if self._lifecycle_state != LifecycleState.ACTIVE: + self._handle = None + self._lifecycle_state = LifecycleState.CLOSED + return + self._teardown(free_handle=True) def _activate(self, handle): """Attach a native handle to self and mark it active. @@ -441,8 +433,10 @@ def _invoke_consume(self, ffi_call, error_message): try: return ffi_call(self._handle) except ctypes.ArgumentError: + # Marshalling failed: the call never reached native, so the handle + # is untouched and still ours. Re-raise as-is. raise - except BaseException as e: + except Exception as e: self._release_handle() raise C2paError(error_message.format(e)) from e @@ -472,9 +466,14 @@ def _raise_consume_failure(self, error_message): error) _raise_typed_c2pa_error(error) - self._release_handle() + # 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 + # recycled address in other threads). + self._teardown(free_handle=False) _raise_typed_c2pa_error(error) + # No error in the slot: ownership is unknown, so free defensively. self._release_handle() raise C2paError(error_message.format("Unknown error")) @@ -497,10 +496,22 @@ def _consume_no_replacement(self, ffi_call, error_message): """ result = self._invoke_consume(ffi_call, error_message) if result == 0: - self._mark_consumed() + self._teardown(free_handle=False) return self._raise_consume_failure(error_message) + def _consume_into(self, ffi_call, error_message): + """Run an FFI call that consumes this handle and returns a *different* + object's pointer. On success this handle is consumed (mark, don't free) + and the new pointer is returned for the caller to own. A null return is + a failure routed to _raise_consume_failure. + """ + result = self._invoke_consume(ffi_call, error_message) + if result: + self._teardown(free_handle=False) + return result + self._raise_consume_failure(error_message) + @classmethod def _wrap_native_handle(cls, handle): """Build a brand-new instance around an already-valid, @@ -544,18 +555,7 @@ def _cleanup_resources(self): hasattr(self, '_lifecycle_state') and self._lifecycle_state != LifecycleState.CLOSED ): - self._lifecycle_state = LifecycleState.CLOSED - self._safe_release() - if hasattr(self, '_handle') and self._handle: - try: - ManagedResource._free_native_ptr(self._handle) - except Exception: - logger.error( - "Failed to free native %s resources", - type(self).__name__, - ) - finally: - self._handle = None + self._teardown(free_handle=True) except Exception: pass @@ -1292,41 +1292,6 @@ def _raise_typed_c2pa_error(error_str: str) -> None: raise C2paError(error_str) -def _parse_operation_result_for_error( - result: ctypes.c_void_p | None, - check_error: bool = True) -> Optional[str]: - """Helper function to handle string results from C2PA functions. - - When result is falsy and check_error is True, this function retrieves the - error from the native library, parses it, and raises a typed C2paError. - - When result is truthy (a pointer to an error string), this function - converts it to a Python string, parses it, and raises a typed C2paError. - - Args: - result: A pointer to a result string, or None/falsy on error - check_error: Whether to check for errors when result is falsy - - Returns: - None if no error occurred - - Raises: - C2paError subclass: The appropriate typed exception if an error occurred - """ - if not result: # pragma: no cover - if check_error: - error_str = _read_native_error() - if error_str: - _raise_typed_c2pa_error(error_str) - return None - - # In the case result would be a string already (error message) - error_str = _convert_to_py_string(result) - if error_str: - _raise_typed_c2pa_error(error_str) - return None - - def _check_ffi_operation_result( result, fallback_msg, @@ -1353,9 +1318,9 @@ def _check_ffi_operation_result( C2paError: If the check indicates failure """ if check(result): - error = _parse_operation_result_for_error(_lib.c2pa_error()) + error = _read_native_error() if error: - raise C2paError(error) + _raise_typed_c2pa_error(error) raise C2paError(fallback_msg.format("Unknown error")) return result @@ -1554,7 +1519,7 @@ def __init__(self): try: _check_ffi_operation_result( settings_ptr, "Failed to create Settings") - except BaseException: + except Exception: if settings_ptr: ManagedResource._free_native_ptr(settings_ptr) raise @@ -1602,11 +1567,11 @@ def set(self, path: str, value: str) -> 'Settings': path_bytes = _to_utf8_bytes(path, "settings path") value_bytes = _to_utf8_bytes(value, "settings value") - result = _lib.c2pa_settings_set_value( - self._handle, path_bytes, value_bytes - ) - if result != 0: - _parse_operation_result_for_error(None) + _check_ffi_operation_result( + _lib.c2pa_settings_set_value( + self._handle, path_bytes, value_bytes), + "Failed to set settings value", + check=lambda r: r != 0) return self @@ -1627,11 +1592,11 @@ def update( data_bytes = _to_utf8_bytes(data, "settings data") - result = _lib.c2pa_settings_update_from_string( - self._handle, data_bytes, b"json" - ) - if result != 0: - _parse_operation_result_for_error(None) + _check_ffi_operation_result( + _lib.c2pa_settings_update_from_string( + self._handle, data_bytes, b"json"), + "Failed to update settings", + check=lambda r: r != 0) return self @@ -1699,6 +1664,18 @@ class Context(ManagedResource, ContextProvider): used directly again after that. """ + class _NativeBuilder(ManagedResource): + """Short-lived wrapper so the native context builder rides the normal + lifecycle: any failure inside its `with` block frees it via close() + unless a consuming call already took it. + """ + + def __init__(self): + super().__init__() + ptr = _lib.c2pa_context_builder_new() + _check_ffi_operation_result(ptr, "Failed to create ContextBuilder") + self._activate(ptr) + def __init__( self, settings: Optional['Settings'] = None, @@ -1726,52 +1703,31 @@ def __init__( ) self._activate(context_ptr) else: - # Use ContextBuilder for settings/signer - builder_ptr = _lib.c2pa_context_builder_new() - _check_ffi_operation_result( - builder_ptr, "Failed to create ContextBuilder" - ) - - try: + # Any failure inside the with frees the builder via close(); + # a successful build consumes it, so close() is then a no-op. + with self._NativeBuilder() as nb: if settings is not None: - result = ( + _check_ffi_operation_result( _lib.c2pa_context_builder_set_settings( - builder_ptr, settings._c_settings, - ) - ) - if result != 0: - _parse_operation_result_for_error(None) + nb._handle, settings._c_settings), + "Failed to set settings on Context", + check=lambda r: r != 0) if signer is not None: signer._ensure_valid_state() - # makes sure that a rejected signer is retained - # instead of being closed and leaked. + # A rejected signer is retained, not closed and leaked. self._signer_callback_cb = signer._callback_cb signer._consume_no_replacement( lambda h: _lib.c2pa_context_builder_set_signer( - builder_ptr, h), + nb._handle, h), "Failed to set signer on Context: {}") self._has_signer = True - # Build consumes builder_ptr. - context_ptr = ( - _lib.c2pa_context_builder_build(builder_ptr) - ) - if context_ptr: - builder_ptr = None - _check_ffi_operation_result( - context_ptr, "Failed to build Context" - ) + context_ptr = nb._consume_into( + lambda h: _lib.c2pa_context_builder_build(h), + "Failed to build Context: {}") - self._activate(context_ptr) - except BaseException: - # Free builder if build was not reached - if builder_ptr is not None: - try: - ManagedResource._free_native_ptr(builder_ptr) - except Exception: - pass - raise + self._activate(context_ptr) def _init_attrs(self): super()._init_attrs() @@ -2554,7 +2510,7 @@ def _init_from_context(self, context, format_or_path, 'reader_error' ] ) - except BaseException: + except Exception: if reader_ptr: ManagedResource._free_native_ptr(reader_ptr) raise @@ -2590,7 +2546,7 @@ def _init_from_context(self, context, format_or_path, self._own_stream._stream, ), Reader._ERROR_MESSAGES['reader_error']) - except BaseException: + except Exception: self._close_streams() raise @@ -3263,7 +3219,7 @@ def from_archive( try: # A builder from an archive here carries no context. return cls._wrap_native_handle(handle) - except BaseException: + except Exception: # No instance took ownership, so the handle is still ours. ManagedResource._free_native_ptr(handle) raise @@ -3340,7 +3296,7 @@ def _init_from_context(self, context, json_str): 'builder_error' ] ) - except BaseException: + except Exception: if builder_ptr: ManagedResource._free_native_ptr(builder_ptr) raise @@ -3700,7 +3656,7 @@ def _sign_internal( # Closing here ensures resources clean up, # and single use/single sign done by a Builder. self.close() - except BaseException as e: + except Exception as e: self.close() raise C2paError(f"Error during signing: {e}") from e diff --git a/tests/test_unit_tests.py b/tests/test_unit_tests.py index d7ef3e38..a6a2c9cf 100644 --- a/tests/test_unit_tests.py +++ b/tests/test_unit_tests.py @@ -6979,13 +6979,6 @@ def _release(self): self.buffer.append(self.label) self.released = True - class _InterruptingReleaseResource(ManagedResource): - """_release() raises KeyboardInterrupt, an interrupt _safe_release() - does not catch, to check the handle is still freed on the way out.""" - - def _release(self): - raise KeyboardInterrupt - def setUp(self): self.data_dir = FIXTURES_DIR self.freed = [] @@ -7229,12 +7222,12 @@ def test_foreign_child_skips_release(self): def test_consumed_resource_frees_nothing_in_either_process(self): owned = self._FakeHandleResource() owned._activate(0xE1) - owned._mark_consumed() + owned._teardown(free_handle=False) owned.close() foreign = self._FakeHandleResource() foreign._activate(0xE2) - foreign._mark_consumed() + foreign._teardown(free_handle=False) foreign._owner_pid = os.getpid() + 1 foreign.close() @@ -7242,33 +7235,33 @@ def test_consumed_resource_frees_nothing_in_either_process(self): # Consuming a handle hands the native pointer to a new owner. # The Python-side resources are still ours to free. - def test_mark_consumed_releases_python_resources(self): + def test_teardown_consumed_releases_python_resources(self): res = self._ReleaseRecordingResource() res._activate(0xF1) - res._mark_consumed() + res._teardown(free_handle=False) self.assertEqual(res.release_calls, 1) self.assertEqual(res._lifecycle_state, LifecycleState.CLOSED) self.assertIsNone(res._handle) self.assertEqual(self.freed, []) - def test_mark_consumed_swallows_failing_release(self): + def test_teardown_consumed_swallows_failing_release(self): res = self._CallbackHoldingResource() res._activate(0xF2) with self.assertLogs("c2pa", level="ERROR"): - res._mark_consumed() + res._teardown(free_handle=False) self.assertEqual(res._lifecycle_state, LifecycleState.CLOSED) self.assertIsNone(res._handle) - def test_mark_consumed_in_foreign_process_skips_release(self): + def test_teardown_consumed_in_foreign_process_skips_release(self): res = self._ReleaseRecordingResource() res._activate(0xF3) res._owner_pid = os.getpid() + 1 - res._mark_consumed() + res._teardown(free_handle=False) self.assertEqual(res.release_calls, 0) self.assertEqual(res._lifecycle_state, LifecycleState.CLOSED) @@ -7439,7 +7432,7 @@ def test_consume_no_replacement_retains_on_pre_consume_tag(self): res.close() self.assertEqual(self.freed, [0xCAFE]) - def test_consume_no_replacement_frees_on_other_error(self): + def test_consume_no_replacement_marks_consumed_on_other_error(self): res = self._FakeHandleResource() res._activate(0xCAFE) real_read = c2pa_module._read_native_error @@ -7450,28 +7443,9 @@ def test_consume_no_replacement_frees_on_other_error(self): finally: c2pa_module._read_native_error = real_read - # Ownership transferred, then the operation failed: freed, closed. - self.assertEqual(self.freed, [0xCAFE]) - self.assertIsNone(res._handle) - self.assertEqual(res._lifecycle_state, LifecycleState.CLOSED) - - def test_interrupt_in_release_still_frees_handle(self): - res = self._InterruptingReleaseResource() - res._activate(0xDEAD) - - res._release_handle() - - self.assertEqual(self.freed, [0xDEAD]) - self.assertIsNone(res._handle) - self.assertEqual(res._lifecycle_state, LifecycleState.CLOSED) - - def test_interrupt_in_cleanup_still_frees_handle(self): - res = self._InterruptingReleaseResource() - res._activate(0xBEEF) - - res._cleanup_resources() - - self.assertEqual(self.freed, [0xBEEF]) + # A non-tag error means native took ownership then failed and dropped + # the value itself: mark consumed, do not free. + self.assertEqual(self.freed, []) self.assertIsNone(res._handle) self.assertEqual(res._lifecycle_state, LifecycleState.CLOSED) @@ -8586,8 +8560,8 @@ def test_check_ffi_operation_result_passes_success_through(self): c2pa_module._check_ffi_operation_result(42, "unused"), 42) def test_stream_creation_failure_reports_a_real_message(self): - # Regression: used to raise bare Exception("...: None"), because - # _parse_operation_result_for_error never returns a message. + # Regression: used to raise bare Exception("...: None") instead of the + # native error message. real = c2pa_module._lib.c2pa_create_stream c2pa_module._lib.c2pa_create_stream = lambda *a: None try: From 587d95e97cde9801e80ecbfa3b9899d6b84c7dd3 Mon Sep 17 00:00:00 2001 From: tmathern <60901087+tmathern@users.noreply.github.com> Date: Tue, 21 Jul 2026 18:36:52 -0700 Subject: [PATCH 5/7] fix: Refactor --- docs/native-resources-management.md | 2 +- pr297-review.md | 349 ++++++++++++++++++++++++++++ src/c2pa/c2pa.py | 39 ++-- tests/test_unit_tests.py | 43 ++-- 4 files changed, 396 insertions(+), 37 deletions(-) create mode 100644 pr297-review.md diff --git a/docs/native-resources-management.md b/docs/native-resources-management.md index 87d1052d..f5c453dc 100644 --- a/docs/native-resources-management.md +++ b/docs/native-resources-management.md @@ -142,7 +142,7 @@ Each transition has one method that performs it, and subclasses must go through | `_activate(handle)` | UNINITIALIZED to ACTIVE | Rejects a null handle, and refuses to run on an already-activated resource. A rejected activation leaves the object exactly as it was. | | `_swap_handle(new_handle)` | ACTIVE to ACTIVE | Requires the resource to already be active and the replacement to be non-null. Used when an FFI call consumed the old handle and returned a new one. | | `_mark_consumed()` | ACTIVE to CLOSED | Drops the handle without freeing it, for when ownership passed to the native side (e.g. `Signer` into `Context`). Runs `_release()` first, so subclass cleanup still happens. Unlike the other two, it validates nothing. | -| `_release_handle()` | ACTIVE to CLOSED | Frees the handle eagerly and closes the object, for failure paths where ownership is unknown (the free is guarded, so a `-1` no-op if the native side already dropped it). Same post-state as `_mark_consumed()`; the difference is the extra `c2pa_free`. | +| `_release_handle()` | ACTIVE to CLOSED | Frees the handle eagerly and closes the object. Same post-state as `_mark_consumed()`. | Because activation is the only way in, no code path can leave an object ACTIVE while holding a null handle. diff --git a/pr297-review.md b/pr297-review.md new file mode 100644 index 00000000..4fc645de --- /dev/null +++ b/pr297-review.md @@ -0,0 +1,349 @@ +# Review: c2pa-python PR #297 (`mathern/open-up-api-3`, at `1883b3a`) — single-contract update + +**Context change:** the restore-on-error native branch (`try-assign`) is no longer +coming; the binding targets only the released C FFI semantics (v0.90.0-style). That +collapses the dual contract to one, and it changes what the eager free is *for* — which +affects one branch of the new code. + +## Integrated correctly (verified) + +- **`build` leak fixed** exactly as planned: `builder_ptr` is nulled only when + `context_ptr` is non-null, so the pre-consume rejection (the only null path in the + released FFI) leaves the pointer for the recovery block to free. Covered by + `test_context_build_null_return_frees_builder`. +- **`set_signer` triage** landed via a better factoring than the plan's sketch: + `_invoke_consume` (shared call wrapper) + `_raise_consume_failure` (shared triage) + + thin `_consume_and_swap` / `_consume_no_replacement`. The ordering trap is explicitly + internalized — the docstring states the error is read before any free "so a free's own + pointer-tracking error cannot overwrite it." The call-site comment now correctly says a + rejected signer is retained. Success/retain/consumed paths each have a test. +- Both consume flavors share one `_PRE_CONSUME_ERROR_TAGS` triage, which is the + centralization PR #294 promised. + +## Findings + +### 1. MEDIUM — `_safe_release()` now swallows `KeyboardInterrupt` and `SystemExit` + +The change from `except Exception` to `except BaseException` makes the +interrupt-in-release tests pass by *suppressing* the interrupt: it is logged as "Failed +to release resources" and the program continues. A user pressing Ctrl-C while `close()` +runs a slow `_release()` (stream teardown) gets their interrupt eaten; `sys.exit()` +raised anywhere under a teardown path is likewise cancelled. That is a behavioral +regression reaching well beyond memory handling, and it isn't needed to fix the +stranded-handle problem — free-and-propagate does both: + +```python +self._lifecycle_state = LifecycleState.CLOSED +try: + self._safe_release() # back to catching Exception only +finally: + if self._handle: + try: + ManagedResource._free_native_ptr(self._handle) + except Exception: + logger.error("Failed to free native %s resources", + type(self).__name__, exc_info=True) + finally: + self._handle = None +``` + +The interrupt then still frees the handle on the way out (the existing +`test_interrupt_in_release_still_frees_handle` should pass unchanged if it asserts the +free, not the suppression) *and* propagates. Apply the same shape in +`_cleanup_resources`. Worth noting the stakes are low under the single contract — on +every post-native-failure use of `_release_handle` the native side already dropped the +memory, so an escaped interrupt strands nothing real; the only genuine leak case is a +second interrupt landing inside teardown that a first interrupt triggered. Suppressing +Ctrl-C globally is a high price for that corner. + +### 2. MEDIUM — with restore-on-error gone, the "ownership transferred" branch should stop freeing + +The eager `_release_handle()` on the non-tag error branch of `_raise_consume_failure` +was justified as forward-compatibility: mandatory under try-assign, guarded no-op under +the release. With try-assign off the table, that branch's free is **permanently** a +guaranteed `-1` that frees nothing (the released FFI drops the value inside the failing +call), and it permanently keeps two side effects: + +- it writes `UntrackedPointer:` into the sticky thread-local slot on every such failure + (harmless to the raised error, which is copied first, but it leaves the slot dirty for + any later reader without a fresh error); +- it keeps the recycled-address race alive in multithreaded use: between the native + internal drop (GIL released during the failing call) and our `c2pa_free`, another + thread inside its own FFI call can allocate a tracked object at the recycled address — + our `-1`-intended free then finds a live registry entry and destroys the other + thread's object. Transitional before; permanent now. + +Under the single contract, ownership on this branch is *known*, not defensive: a +non-tag error means the native side consumed and dropped the value. The exact action is +`_mark_consumed()` — no free call, no slot write, no race: + +```python +# in _raise_consume_failure, non-tag branch: +self._mark_consumed() +_raise_typed_c2pa_error(error) +``` + +Keep `_release_handle()` where ownership is genuinely unknown, because there the guarded +free is the right both-ways default (we own it → real free; consumed → `-1`): +- the `except BaseException` branch of `_invoke_consume` (async exception, unknown + timing), and +- the unknown-error fallthrough (null/-1 with an empty slot — no released-FFI path does + this, so it's defensive by definition). + +Update `test_consume_no_replacement_frees_on_other_error` to assert `_mark_consumed` +semantics (closed, handle nulled, **no** free call) — its current name and assertion +encode the dual-contract behavior that no longer applies. + +### 3. LOW — docs and comments to realign with the single contract + +- `_release_handle`'s docstring lost its rationale entirely (and has a ".." typo). With + finding 2 applied its remaining roles are: unknown-ownership error paths and + known-owned frees; one sentence on why the guarded free is correct for both is enough. +- `_safe_release`'s docstring ("safely (no raise)") should be restored to match the + `except Exception` behavior from finding 1. +- The earlier suggestion to convert `with_definition` in the native branch is moot — + drop it. Its Python handling is already correct under the single contract, and with + finding 2 its post-consume failures also stop producing permanent `-1` debug noise. +- The dual-contract explanation drafted for `docs/native-resources-management.md` should + be written single-contract instead: pre-consume tags ⇒ retained; any other error ⇒ + consumed and dropped natively, nothing to free; unknown ⇒ guarded free. + +## Order + +1 (`_safe_release` revert + `finally`-guarded frees) and 2 (`_mark_consumed` on the +known-consumed branch + test update) are independent and small; then 3 (doc realignment) +in the same PR. + +--- + +# Refactoring opportunities (simplification pass) + +The single-contract decision doesn't just permit simplification — several defensive +structures existed only to serve the contract ambiguity. Ranked by leverage: + +## R1 — Wrap the native context builder in a `ManagedResource`; delete the recovery block + +The raw-pointer juggling in `Context.__init__` (manual `builder_ptr`, the +`except BaseException` recovery block, and the `if context_ptr: builder_ptr = None` +subtlety that hosted the leak bug) exists because the native builder is the one handle +in the codebase *not* managed by the lifecycle machinery this PR series built. Close +that gap with a private ~10-line class and a third consume flavor: + +```python +class _NativeContextBuilder(ManagedResource): + """Short-lived wrapper so builder teardown rides the normal lifecycle.""" + def __init__(self): + super().__init__() + ptr = _lib.c2pa_context_builder_new() + _check_ffi_operation_result(ptr, "Failed to create ContextBuilder") + self._activate(ptr) +``` + +```python +def _consume_into(self, ffi_call, error_message): + """Consuming call that produces a *different* object's pointer. + + On success this handle was consumed (mark, don't free) and the new + pointer is returned for the caller to own. Failure triage is shared. + """ + result = self._invoke_consume(ffi_call, error_message) + if result: + self._mark_consumed() + return result + self._raise_consume_failure(error_message) +``` + +`Context.__init__` then reads as the sequence it is: + +```python +with _NativeContextBuilder() 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) + if signer is not None: + signer._ensure_valid_state() + self._signer_callback_cb = signer._callback_cb + signer._consume_no_replacement( + lambda h: _lib.c2pa_context_builder_set_signer(nb._handle, h), + "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: {}") +self._activate(context_ptr) +``` + +Every failure path — settings error, retained signer, build rejection, async interrupt — +is handled by `__exit__` → `close()`, which frees iff the builder wasn't consumed. The +leak I found earlier becomes *structurally impossible* rather than patched, and the +`_consume_into` triage means a `build` pre-consume rejection is retained-then-freed-once +instead of relying on call-site ordering. (`ManagedResource` already provides +`close`/`__enter__`/`__exit__`, so the wrapper costs nothing extra. `_activate` after the +`with` keeps ownership transfer to the `Context` outside the builder's scope, mirroring +`_wrap_native_handle`'s ownership-on-success rule.) + +This also gives the three consume flavors a complete, symmetric story worth stating in +the class docstring: `_consume_and_swap` (replacement for self), `_consume_no_replacement` +(status code), `_consume_into` (pointer for someone else) — all three = `_invoke_consume` ++ result check + `_raise_consume_failure`. + +## R2 — One teardown implementation instead of three + +`_mark_consumed`, `_release_handle`, and `_cleanup_resources` each hand-roll the same +shape: foreign-process check → CLOSED → `_safe_release()` → maybe free → null handle. +Three copies is where the next drift starts (finding 1's `finally` fix would currently +need to be applied twice). Fold them: + +```python +def _teardown(self, free_handle: bool): + if is_foreign_process(self): + self._handle = None + self._lifecycle_state = LifecycleState.CLOSED + return + self._lifecycle_state = LifecycleState.CLOSED + try: + self._safe_release() # except Exception (finding 1) + finally: + 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 _mark_consumed(self): + self._teardown(free_handle=False) + +def _release_handle(self): + if self._lifecycle_state != LifecycleState.ACTIVE: + self._handle = None + self._lifecycle_state = LifecycleState.CLOSED + return + self._teardown(free_handle=True) +``` + +`_cleanup_resources` keeps its own not-already-CLOSED / `hasattr` guards and delegates +the rest to `_teardown(True)`. One place now owns the foreign-process rule, the CLOSED +ordering, and the interrupt-safe free — and finding 1's fix lands exactly once. + +## R3 — Retire `_parse_operation_result_for_error` + +Call-site census: `_check_ffi_operation_result` is the workhorse (29 uses); +`_parse_operation_result_for_error` has 5, and its dual behavior (result-as-error-string +vs read-slot-and-raise) is the kind of overloading the PR description already started +pruning ("removed function that did not clear previous error"). Its slot-reading mode is +now `_read_native_error()` + `_raise_typed_c2pa_error()` — the exact pair +`_raise_consume_failure` uses — and its string-parsing mode is `_raise_typed_c2pa_error` +directly. Migrating the 5 sites leaves three primitives with one job each (read, raise, +check-and-raise) plus the consume-triage built on them. Fewer names, one error-reading +discipline. + +## R4 — Small cleanups (batch into one commit) + +- The `except ctypes.ArgumentError: raise` clause in `_invoke_consume` is purely + documentary (catch-and-reraise). Fine to keep for the comment's sake, but the comment + can live on the `BaseException` clause and the clause can go — one branch fewer to + read. +- `_release_handle`'s "normalize states" pre-ACTIVE branch and the foreign-process + branch both collapse into R2's structure; after R2, `_release_handle` is three lines. +- With finding 2 applied, grep for remaining `_release_handle` callers: only the two + unknown-ownership sites should survive. Any other caller is a smell — a site that + knows ownership and should say `_mark_consumed` or rely on `close()`. +- The interrupt tests should assert the *free happened and the interrupt propagated* + (`pytest.raises(KeyboardInterrupt)` around the close), pinning finding 1's semantics + instead of the current suppression. + +## Updated order + +Finding 1 + R2 land together (the `finally` fix is written once, inside `_teardown`). +Then finding 2 (one-line branch change + test update). Then R1 (`_NativeContextBuilder` ++ `_consume_into`, deleting the recovery block). R3 and R4 as follow-up cleanup commits. + +--- + +# Re-review at `927f846` ("fix: refactor") + +## Resolved (verified branch-by-branch) + +- **R2 landed**: `_teardown(free_handle)` is now the single owner of the + foreign-process rule, CLOSED ordering, and the guarded free; `_release_handle` is a + guard plus `_teardown(True)`; `_cleanup_resources` delegates. `_mark_consumed` is + gone with no dangling callers (tests renamed). +- **Finding 2 landed**: the known-consumed branch uses `_teardown(free_handle=False)` + with the rationale (guarded no-op, slot dirtying, recycled-address race) in the + comment. `_release_handle` survives only at the two unknown-ownership sites, exactly + as intended. +- **R1 landed**: `Context._NativeBuilder` + `_consume_into` replace the raw-pointer + recovery block. Checked every path: pre-consume rejection → retained → freed exactly + once by `__exit__`; consumed-then-failed → close is a no-op; success → close no-op, + `_activate(context_ptr)` outside the `with`. The build-leak class is structurally + gone. The three consume flavors now share `_invoke_consume` + `_raise_consume_failure`. +- **R3 landed**: `_parse_operation_result_for_error` deleted; its 5 sites migrated to + `_check_ffi_operation_result` / `_read_native_error` + `_raise_typed_c2pa_error`. + Side benefit: `_check_ffi_operation_result` now raises *typed* errors where it + previously raised bare `C2paError` — compatible (subclasses are `C2paError`) and + strictly more informative; worth one line in the PR description. +- `_safe_release` is back to `except Exception` with an accurate docstring. + +## New findings + +### A. MEDIUM — `_teardown` took half of finding 1: the `finally` is missing + +Finding 1 had two parts: revert `_safe_release` to `Exception` (done) *and* make the +free un-skippable via `try/finally` (not done). As written, a `KeyboardInterrupt` +inside a subclass `_release()` escapes `_teardown` with the state already CLOSED and +the handle still set; every later teardown path early-returns on CLOSED, so an +**owned** handle leaks — and because `_teardown` is now the single teardown path, this +applies to every `close()`, `__del__`, and consume-failure in the codebase. One-line +shape fix: + +```python +self._lifecycle_state = LifecycleState.CLOSED +try: + self._safe_release() +finally: + 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) +``` + +The two interrupt tests were deleted in this commit rather than repointed. Reinstate +them with the corrected assertion: the handle is freed **and** the interrupt +propagates (`pytest.raises(KeyboardInterrupt)` around the close) — that pins both +halves of the intended semantics and would have caught this. + +### B. MEDIUM — the `BaseException → Exception` sweep overcorrected in raw-pointer recovery blocks + +The narrowing was right where the old code *suppressed or wrapped* the exception +(`_safe_release`; the signing path that wrapped everything in `C2paError`). It is wrong +where the block **frees a raw pre-activation pointer and re-raises**: Settings +`__init__`, both Reader construction sites, `Builder.from_archive`'s +`_wrap_native_handle` recovery, and the Builder construction site. Those pointers have +no owning object yet — if a `KeyboardInterrupt` lands there, `except Exception` skips +the free and nothing (not GC, not a guarded close) can ever reclaim it. The rule worth +writing into the class docstring so the next sweep doesn't reintroduce either bug: + +> *catch-and-suppress must be `Exception`; catch-free-reraise on an unowned pointer +> must be `BaseException` (or `try/finally` with a consumed flag).* + +Revert those five blocks to `except BaseException:` — they re-raise, so no interrupt +is ever swallowed. + +Related, LOW: `_invoke_consume`'s narrowing to `Exception` is a reasonable choice +(interrupts propagate promptly; the still-ACTIVE object is later cleaned by +`close()`/GC with a guarded free), but its docstring still says "any other exception +from the call frees the handle before raising" — no longer true for interrupts. One +sentence: interrupts propagate untouched and teardown happens at the next +close/GC. + +## Order + +A (one-line `finally` + reinstated tests) and B (five-block revert + docstring rule) +are both small; land them in one commit and this PR is, from a memory-handling +standpoint, done. diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index 7dbd1d71..04e65af9 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -237,8 +237,8 @@ class ManagedResource: current handle and returned a replacement (the success side of `_consume_and_swap`). - Call `_teardown(free_handle=False)` when an FFI call took ownership of - the handle without returning a replacement (e.g. `Signer` into - `Context`): the new owner frees it, so this does not. + the handle without returning a replacement: the new owner frees it, + so this does not. - Call `_release_handle()` when a consuming FFI call fails with ownership unknown: it frees eagerly (guarded), then closes. `_consume_and_swap` uses it on that failure path. @@ -332,20 +332,21 @@ def _teardown(self, free_handle: bool): return self._lifecycle_state = LifecycleState.CLOSED - self._safe_release() - - if free_handle and self._handle: - try: - ManagedResource._free_native_ptr(self._handle) - except Exception: - logger.error( - "Failed to free native %s resources", - type(self).__name__, - exc_info=True) - finally: - self._handle = None - else: - self._handle = None + try: + self._safe_release() + finally: + # Free in finally so an interrupt escaping _release() (a BaseException + # _safe_release does not swallow) still frees an owned handle. State + # is already CLOSED, so no later teardown path would retry it. + 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 _release_handle(self): """Free this handle, then close the object. Used only where ownership is @@ -413,8 +414,10 @@ def _invoke_consume(self, ffi_call, error_message): """Run an FFI call that consumes this handle, returning its raw result. A marshalling ArgumentError is re-raised untouched: the call never - reached the native side, so the handle is still ours and unchanged. Any - other exception from the call frees the handle before raising. + reached the native side, so the handle is still ours and unchanged. An + ordinary Exception from the call frees the handle before raising; an + interrupt (KeyboardInterrupt/SystemExit) propagates untouched and the + still-ACTIVE handle is freed later at close()/GC (a guarded free). The caller inspects the returned result to tell success from failure (the convention differs per call) and routes a failure to diff --git a/tests/test_unit_tests.py b/tests/test_unit_tests.py index a6a2c9cf..644df3c9 100644 --- a/tests/test_unit_tests.py +++ b/tests/test_unit_tests.py @@ -7388,7 +7388,11 @@ def test_construction_failure_leaves_nothing_to_free(self): def test_context_build_null_return_frees_builder(self): # build's only null path is a pre-consume rejection, which leaves the # builder tracked and alive, so Context must free it on the way out. + # Set a pre-consume tag in the error slot so the rejection is + # unambiguous and does not depend on ambient slot state. 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 try: @@ -7703,12 +7707,13 @@ def test_consumed_signer_close_frees_nothing(self): "closing a consumed Signer freed a pointer the " "context now owns") - def test_builder_with_archive_null_return_frees_self(self): + def test_builder_with_archive_null_return_marks_consumed(self): builder = Builder(self.test_manifest) released_handle = builder._handle archive = self._make_archive() - # Mimic an error. + # 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 @@ -7725,31 +7730,33 @@ def test_builder_with_archive_null_return_frees_self(self): self.assertIsNone(builder._handle) self.assertEqual(builder._lifecycle_state, LifecycleState.CLOSED) - # Error frees the old handle. - self.assertEqual(self._free_count(freed, released_handle), 1, - "error path did not free the old handle exactly once") + # A non-tag error marks consumed: no free (a free here would be a + # guarded no-op that dirties the error slot and races a recycled + # address in other threads). + self.assertEqual(self._free_count(freed, released_handle), 0, + "consumed handle was freed instead of marked consumed") - # close() must not free again. + # close() must not free it either. builder.close() - self.assertEqual(self._free_count(freed, released_handle), 1, - "close() double-freed an already-released handle") + self.assertEqual(self._free_count(freed, released_handle), 0, + "close() freed a handle already marked consumed") - def test_reader_with_fragment_null_return_frees_self(self): + def test_reader_with_fragment_null_return_marks_consumed(self): 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) released_handle = reader._handle - # Mimic an error. + # 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) - # Instrument before failure so the eager free is counted, - # not just whatever close() does afterwards. + # Instrument before failure so any free would be counted. freed = self._instrument_frees() try: with open(init_path, "rb") as init, \ @@ -7762,14 +7769,14 @@ def test_reader_with_fragment_null_return_frees_self(self): self.assertIsNone(reader._handle) self.assertEqual(reader._lifecycle_state, LifecycleState.CLOSED) - # The error path frees the old handle. - self.assertEqual(self._free_count(freed, released_handle), 1, - "error path did not free the old handle exactly once") + # A non-tag error marks consumed: no free. + self.assertEqual(self._free_count(freed, released_handle), 0, + "consumed handle was freed instead of marked consumed") - # close() must not free it again. + # close() must not free it either. reader.close() - self.assertEqual(self._free_count(freed, released_handle), 1, - "close() freed a handle the error path already freed") + self.assertEqual(self._free_count(freed, released_handle), 0, + "close() freed a handle already marked consumed") def test_reader_with_fragment_ffi_raise_frees_self(self): # If the ctypes call itself raises, the failure runs From 8bb7ac2e58568fc977cd55ba1d95b885e463bc0c Mon Sep 17 00:00:00 2001 From: tmathern <60901087+tmathern@users.noreply.github.com> Date: Tue, 21 Jul 2026 18:39:14 -0700 Subject: [PATCH 6/7] fix: Refactor 2 --- tests/test_unit_tests.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/tests/test_unit_tests.py b/tests/test_unit_tests.py index 644df3c9..4cc05aa1 100644 --- a/tests/test_unit_tests.py +++ b/tests/test_unit_tests.py @@ -7386,10 +7386,7 @@ 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): - # build's only null path is a pre-consume rejection, which leaves the - # builder tracked and alive, so Context must free it on the way out. - # Set a pre-consume tag in the error slot so the rejection is - # unambiguous and does not depend on ambient slot state. + # Set a pre-consume tag in the error slot to mock a pointer rejection. settings = Settings() c2pa_module._lib.c2pa_error_set_last( b"UntrackedPointer: mocked pre-consume rejection") From 08892a799e7f4beaae8b793c5212940179778485 Mon Sep 17 00:00:00 2001 From: tmathern <60901087+tmathern@users.noreply.github.com> Date: Tue, 21 Jul 2026 18:40:30 -0700 Subject: [PATCH 7/7] fix: Clean up debug --- pr297-review.md | 349 ------------------------------------------------ 1 file changed, 349 deletions(-) delete mode 100644 pr297-review.md diff --git a/pr297-review.md b/pr297-review.md deleted file mode 100644 index 4fc645de..00000000 --- a/pr297-review.md +++ /dev/null @@ -1,349 +0,0 @@ -# Review: c2pa-python PR #297 (`mathern/open-up-api-3`, at `1883b3a`) — single-contract update - -**Context change:** the restore-on-error native branch (`try-assign`) is no longer -coming; the binding targets only the released C FFI semantics (v0.90.0-style). That -collapses the dual contract to one, and it changes what the eager free is *for* — which -affects one branch of the new code. - -## Integrated correctly (verified) - -- **`build` leak fixed** exactly as planned: `builder_ptr` is nulled only when - `context_ptr` is non-null, so the pre-consume rejection (the only null path in the - released FFI) leaves the pointer for the recovery block to free. Covered by - `test_context_build_null_return_frees_builder`. -- **`set_signer` triage** landed via a better factoring than the plan's sketch: - `_invoke_consume` (shared call wrapper) + `_raise_consume_failure` (shared triage) + - thin `_consume_and_swap` / `_consume_no_replacement`. The ordering trap is explicitly - internalized — the docstring states the error is read before any free "so a free's own - pointer-tracking error cannot overwrite it." The call-site comment now correctly says a - rejected signer is retained. Success/retain/consumed paths each have a test. -- Both consume flavors share one `_PRE_CONSUME_ERROR_TAGS` triage, which is the - centralization PR #294 promised. - -## Findings - -### 1. MEDIUM — `_safe_release()` now swallows `KeyboardInterrupt` and `SystemExit` - -The change from `except Exception` to `except BaseException` makes the -interrupt-in-release tests pass by *suppressing* the interrupt: it is logged as "Failed -to release resources" and the program continues. A user pressing Ctrl-C while `close()` -runs a slow `_release()` (stream teardown) gets their interrupt eaten; `sys.exit()` -raised anywhere under a teardown path is likewise cancelled. That is a behavioral -regression reaching well beyond memory handling, and it isn't needed to fix the -stranded-handle problem — free-and-propagate does both: - -```python -self._lifecycle_state = LifecycleState.CLOSED -try: - self._safe_release() # back to catching Exception only -finally: - if self._handle: - try: - ManagedResource._free_native_ptr(self._handle) - except Exception: - logger.error("Failed to free native %s resources", - type(self).__name__, exc_info=True) - finally: - self._handle = None -``` - -The interrupt then still frees the handle on the way out (the existing -`test_interrupt_in_release_still_frees_handle` should pass unchanged if it asserts the -free, not the suppression) *and* propagates. Apply the same shape in -`_cleanup_resources`. Worth noting the stakes are low under the single contract — on -every post-native-failure use of `_release_handle` the native side already dropped the -memory, so an escaped interrupt strands nothing real; the only genuine leak case is a -second interrupt landing inside teardown that a first interrupt triggered. Suppressing -Ctrl-C globally is a high price for that corner. - -### 2. MEDIUM — with restore-on-error gone, the "ownership transferred" branch should stop freeing - -The eager `_release_handle()` on the non-tag error branch of `_raise_consume_failure` -was justified as forward-compatibility: mandatory under try-assign, guarded no-op under -the release. With try-assign off the table, that branch's free is **permanently** a -guaranteed `-1` that frees nothing (the released FFI drops the value inside the failing -call), and it permanently keeps two side effects: - -- it writes `UntrackedPointer:` into the sticky thread-local slot on every such failure - (harmless to the raised error, which is copied first, but it leaves the slot dirty for - any later reader without a fresh error); -- it keeps the recycled-address race alive in multithreaded use: between the native - internal drop (GIL released during the failing call) and our `c2pa_free`, another - thread inside its own FFI call can allocate a tracked object at the recycled address — - our `-1`-intended free then finds a live registry entry and destroys the other - thread's object. Transitional before; permanent now. - -Under the single contract, ownership on this branch is *known*, not defensive: a -non-tag error means the native side consumed and dropped the value. The exact action is -`_mark_consumed()` — no free call, no slot write, no race: - -```python -# in _raise_consume_failure, non-tag branch: -self._mark_consumed() -_raise_typed_c2pa_error(error) -``` - -Keep `_release_handle()` where ownership is genuinely unknown, because there the guarded -free is the right both-ways default (we own it → real free; consumed → `-1`): -- the `except BaseException` branch of `_invoke_consume` (async exception, unknown - timing), and -- the unknown-error fallthrough (null/-1 with an empty slot — no released-FFI path does - this, so it's defensive by definition). - -Update `test_consume_no_replacement_frees_on_other_error` to assert `_mark_consumed` -semantics (closed, handle nulled, **no** free call) — its current name and assertion -encode the dual-contract behavior that no longer applies. - -### 3. LOW — docs and comments to realign with the single contract - -- `_release_handle`'s docstring lost its rationale entirely (and has a ".." typo). With - finding 2 applied its remaining roles are: unknown-ownership error paths and - known-owned frees; one sentence on why the guarded free is correct for both is enough. -- `_safe_release`'s docstring ("safely (no raise)") should be restored to match the - `except Exception` behavior from finding 1. -- The earlier suggestion to convert `with_definition` in the native branch is moot — - drop it. Its Python handling is already correct under the single contract, and with - finding 2 its post-consume failures also stop producing permanent `-1` debug noise. -- The dual-contract explanation drafted for `docs/native-resources-management.md` should - be written single-contract instead: pre-consume tags ⇒ retained; any other error ⇒ - consumed and dropped natively, nothing to free; unknown ⇒ guarded free. - -## Order - -1 (`_safe_release` revert + `finally`-guarded frees) and 2 (`_mark_consumed` on the -known-consumed branch + test update) are independent and small; then 3 (doc realignment) -in the same PR. - ---- - -# Refactoring opportunities (simplification pass) - -The single-contract decision doesn't just permit simplification — several defensive -structures existed only to serve the contract ambiguity. Ranked by leverage: - -## R1 — Wrap the native context builder in a `ManagedResource`; delete the recovery block - -The raw-pointer juggling in `Context.__init__` (manual `builder_ptr`, the -`except BaseException` recovery block, and the `if context_ptr: builder_ptr = None` -subtlety that hosted the leak bug) exists because the native builder is the one handle -in the codebase *not* managed by the lifecycle machinery this PR series built. Close -that gap with a private ~10-line class and a third consume flavor: - -```python -class _NativeContextBuilder(ManagedResource): - """Short-lived wrapper so builder teardown rides the normal lifecycle.""" - def __init__(self): - super().__init__() - ptr = _lib.c2pa_context_builder_new() - _check_ffi_operation_result(ptr, "Failed to create ContextBuilder") - self._activate(ptr) -``` - -```python -def _consume_into(self, ffi_call, error_message): - """Consuming call that produces a *different* object's pointer. - - On success this handle was consumed (mark, don't free) and the new - pointer is returned for the caller to own. Failure triage is shared. - """ - result = self._invoke_consume(ffi_call, error_message) - if result: - self._mark_consumed() - return result - self._raise_consume_failure(error_message) -``` - -`Context.__init__` then reads as the sequence it is: - -```python -with _NativeContextBuilder() 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) - if signer is not None: - signer._ensure_valid_state() - self._signer_callback_cb = signer._callback_cb - signer._consume_no_replacement( - lambda h: _lib.c2pa_context_builder_set_signer(nb._handle, h), - "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: {}") -self._activate(context_ptr) -``` - -Every failure path — settings error, retained signer, build rejection, async interrupt — -is handled by `__exit__` → `close()`, which frees iff the builder wasn't consumed. The -leak I found earlier becomes *structurally impossible* rather than patched, and the -`_consume_into` triage means a `build` pre-consume rejection is retained-then-freed-once -instead of relying on call-site ordering. (`ManagedResource` already provides -`close`/`__enter__`/`__exit__`, so the wrapper costs nothing extra. `_activate` after the -`with` keeps ownership transfer to the `Context` outside the builder's scope, mirroring -`_wrap_native_handle`'s ownership-on-success rule.) - -This also gives the three consume flavors a complete, symmetric story worth stating in -the class docstring: `_consume_and_swap` (replacement for self), `_consume_no_replacement` -(status code), `_consume_into` (pointer for someone else) — all three = `_invoke_consume` -+ result check + `_raise_consume_failure`. - -## R2 — One teardown implementation instead of three - -`_mark_consumed`, `_release_handle`, and `_cleanup_resources` each hand-roll the same -shape: foreign-process check → CLOSED → `_safe_release()` → maybe free → null handle. -Three copies is where the next drift starts (finding 1's `finally` fix would currently -need to be applied twice). Fold them: - -```python -def _teardown(self, free_handle: bool): - if is_foreign_process(self): - self._handle = None - self._lifecycle_state = LifecycleState.CLOSED - return - self._lifecycle_state = LifecycleState.CLOSED - try: - self._safe_release() # except Exception (finding 1) - finally: - 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 _mark_consumed(self): - self._teardown(free_handle=False) - -def _release_handle(self): - if self._lifecycle_state != LifecycleState.ACTIVE: - self._handle = None - self._lifecycle_state = LifecycleState.CLOSED - return - self._teardown(free_handle=True) -``` - -`_cleanup_resources` keeps its own not-already-CLOSED / `hasattr` guards and delegates -the rest to `_teardown(True)`. One place now owns the foreign-process rule, the CLOSED -ordering, and the interrupt-safe free — and finding 1's fix lands exactly once. - -## R3 — Retire `_parse_operation_result_for_error` - -Call-site census: `_check_ffi_operation_result` is the workhorse (29 uses); -`_parse_operation_result_for_error` has 5, and its dual behavior (result-as-error-string -vs read-slot-and-raise) is the kind of overloading the PR description already started -pruning ("removed function that did not clear previous error"). Its slot-reading mode is -now `_read_native_error()` + `_raise_typed_c2pa_error()` — the exact pair -`_raise_consume_failure` uses — and its string-parsing mode is `_raise_typed_c2pa_error` -directly. Migrating the 5 sites leaves three primitives with one job each (read, raise, -check-and-raise) plus the consume-triage built on them. Fewer names, one error-reading -discipline. - -## R4 — Small cleanups (batch into one commit) - -- The `except ctypes.ArgumentError: raise` clause in `_invoke_consume` is purely - documentary (catch-and-reraise). Fine to keep for the comment's sake, but the comment - can live on the `BaseException` clause and the clause can go — one branch fewer to - read. -- `_release_handle`'s "normalize states" pre-ACTIVE branch and the foreign-process - branch both collapse into R2's structure; after R2, `_release_handle` is three lines. -- With finding 2 applied, grep for remaining `_release_handle` callers: only the two - unknown-ownership sites should survive. Any other caller is a smell — a site that - knows ownership and should say `_mark_consumed` or rely on `close()`. -- The interrupt tests should assert the *free happened and the interrupt propagated* - (`pytest.raises(KeyboardInterrupt)` around the close), pinning finding 1's semantics - instead of the current suppression. - -## Updated order - -Finding 1 + R2 land together (the `finally` fix is written once, inside `_teardown`). -Then finding 2 (one-line branch change + test update). Then R1 (`_NativeContextBuilder` -+ `_consume_into`, deleting the recovery block). R3 and R4 as follow-up cleanup commits. - ---- - -# Re-review at `927f846` ("fix: refactor") - -## Resolved (verified branch-by-branch) - -- **R2 landed**: `_teardown(free_handle)` is now the single owner of the - foreign-process rule, CLOSED ordering, and the guarded free; `_release_handle` is a - guard plus `_teardown(True)`; `_cleanup_resources` delegates. `_mark_consumed` is - gone with no dangling callers (tests renamed). -- **Finding 2 landed**: the known-consumed branch uses `_teardown(free_handle=False)` - with the rationale (guarded no-op, slot dirtying, recycled-address race) in the - comment. `_release_handle` survives only at the two unknown-ownership sites, exactly - as intended. -- **R1 landed**: `Context._NativeBuilder` + `_consume_into` replace the raw-pointer - recovery block. Checked every path: pre-consume rejection → retained → freed exactly - once by `__exit__`; consumed-then-failed → close is a no-op; success → close no-op, - `_activate(context_ptr)` outside the `with`. The build-leak class is structurally - gone. The three consume flavors now share `_invoke_consume` + `_raise_consume_failure`. -- **R3 landed**: `_parse_operation_result_for_error` deleted; its 5 sites migrated to - `_check_ffi_operation_result` / `_read_native_error` + `_raise_typed_c2pa_error`. - Side benefit: `_check_ffi_operation_result` now raises *typed* errors where it - previously raised bare `C2paError` — compatible (subclasses are `C2paError`) and - strictly more informative; worth one line in the PR description. -- `_safe_release` is back to `except Exception` with an accurate docstring. - -## New findings - -### A. MEDIUM — `_teardown` took half of finding 1: the `finally` is missing - -Finding 1 had two parts: revert `_safe_release` to `Exception` (done) *and* make the -free un-skippable via `try/finally` (not done). As written, a `KeyboardInterrupt` -inside a subclass `_release()` escapes `_teardown` with the state already CLOSED and -the handle still set; every later teardown path early-returns on CLOSED, so an -**owned** handle leaks — and because `_teardown` is now the single teardown path, this -applies to every `close()`, `__del__`, and consume-failure in the codebase. One-line -shape fix: - -```python -self._lifecycle_state = LifecycleState.CLOSED -try: - self._safe_release() -finally: - 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) -``` - -The two interrupt tests were deleted in this commit rather than repointed. Reinstate -them with the corrected assertion: the handle is freed **and** the interrupt -propagates (`pytest.raises(KeyboardInterrupt)` around the close) — that pins both -halves of the intended semantics and would have caught this. - -### B. MEDIUM — the `BaseException → Exception` sweep overcorrected in raw-pointer recovery blocks - -The narrowing was right where the old code *suppressed or wrapped* the exception -(`_safe_release`; the signing path that wrapped everything in `C2paError`). It is wrong -where the block **frees a raw pre-activation pointer and re-raises**: Settings -`__init__`, both Reader construction sites, `Builder.from_archive`'s -`_wrap_native_handle` recovery, and the Builder construction site. Those pointers have -no owning object yet — if a `KeyboardInterrupt` lands there, `except Exception` skips -the free and nothing (not GC, not a guarded close) can ever reclaim it. The rule worth -writing into the class docstring so the next sweep doesn't reintroduce either bug: - -> *catch-and-suppress must be `Exception`; catch-free-reraise on an unowned pointer -> must be `BaseException` (or `try/finally` with a consumed flag).* - -Revert those five blocks to `except BaseException:` — they re-raise, so no interrupt -is ever swallowed. - -Related, LOW: `_invoke_consume`'s narrowing to `Exception` is a reasonable choice -(interrupts propagate promptly; the still-ACTIVE object is later cleaned by -`close()`/GC with a guarded free), but its docstring still says "any other exception -from the call frees the handle before raising" — no longer true for interrupts. One -sentence: interrupts propagate untouched and teardown happens at the next -close/GC. - -## Order - -A (one-line `finally` + reinstated tests) and B (five-block revert + docstring rule) -are both small; land them in one commit and this PR is, from a memory-handling -standpoint, done.