diff --git a/docs/native-resources-management.md b/docs/native-resources-management.md index 1b01362e..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 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. 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. @@ -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 b78e06d4..04e65af9 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: 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,7 +307,8 @@ def _release(self): """ def _safe_release(self): - """Run _release(), logging (never raising) if it fails. + """Run _release(), logging and swallowing ordinary errors so teardown + continues. Interrupts (KeyboardInterrupt, SystemExit) propagate. """ try: self._release() @@ -317,55 +319,44 @@ def _safe_release(self): 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. - """ + def _teardown(self, free_handle: bool): + """Close the object: run _release, optionally free the handle, null it. + 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 - # 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 + 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 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. + """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 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: - 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 + self._teardown(free_handle=True) def _activate(self, handle): """Attach a native handle to self and mark it active. @@ -419,49 +410,58 @@ 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. 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 + _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. + # 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 - if new_ptr: - self._swap_handle(new_ptr) - return + def _raise_consume_failure(self, error_message): + """Raise the error from an FFI handler consuming call. + + The native error is read before any free so a free's own + pointer-tracking error cannot overwrite it: the native error slot is + sticky and thread-local and the SDK does not clear it before the call, + so this trusts that the failing native path set its own error. - # Retrieve the error + 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 +469,52 @@ 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() + # 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")) + 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. + """ + 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._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, @@ -520,18 +558,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 @@ -1268,41 +1295,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, @@ -1329,9 +1321,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 @@ -1530,7 +1522,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 @@ -1578,11 +1570,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 @@ -1603,11 +1595,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 @@ -1675,6 +1667,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, @@ -1702,61 +1706,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() - # c2pa_context_builder_set_signer takes ownership of the - # signer pointer , on its error path as well as on success. + # A rejected signer is retained, not 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( + 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) - ) - 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() @@ -2539,7 +2513,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 @@ -2575,7 +2549,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 @@ -3248,7 +3222,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 @@ -3325,7 +3299,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 @@ -3685,7 +3659,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 58db24d7..4cc05aa1 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): @@ -7080,8 +7081,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 +7188,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 +7203,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): @@ -7222,46 +7222,46 @@ 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() 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. - def test_mark_consumed_releases_python_resources(self): + # Consuming a handle hands the native pointer to a new owner. + # The Python-side resources are still ours to free. + 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) @@ -7278,7 +7278,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 +7298,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: + # 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 +7368,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 +7385,71 @@ 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): + # 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") + 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. + 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. + 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_marks_consumed_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 + + # 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) + class TestManagedResourceObjects(TestContextAPIs): """Tests native resource handling management when managed manually. @@ -7403,11 +7468,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 +7605,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 +7649,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 +7691,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) @@ -7645,12 +7704,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 @@ -7663,35 +7723,37 @@ 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. - 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 it 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 the failure so the eager free on the error path 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, \ @@ -7704,20 +7766,19 @@ 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 # 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 +7813,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 +7850,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 +7891,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 +7916,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) @@ -8505,8 +8564,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: