From e8504bf2fa087213c89d8b81b17c0c2a006988df Mon Sep 17 00:00:00 2001 From: tmathern <60901087+tmathern@users.noreply.github.com> Date: Sun, 19 Jul 2026 22:59:13 -0700 Subject: [PATCH 1/5] fix: Docs --- docs/native-resources-management.md | 238 +++++++++++++++++++++++----- 1 file changed, 198 insertions(+), 40 deletions(-) diff --git a/docs/native-resources-management.md b/docs/native-resources-management.md index 100936d0..21384c3b 100644 --- a/docs/native-resources-management.md +++ b/docs/native-resources-management.md @@ -2,6 +2,9 @@ `ManagedResource` is the internal base class used by the C2PA Python SDK to wrap native (Rust/FFI) pointers. When adding new wrappers around native resources `ManagedResource` should be subclassed and follow the documented lifecycle rules. +> [!NOTE] +> `ManagedResource` and the lifecycle machinery described here are internal to the SDK, not part of its public API. In most cases, code that reads and writes C2PA data should use the public wrappers (`Reader`, `Builder`, `Signer`, `Context`, `Settings`). + ## Why `ManagedResource`? `ManagedResource` is the internal base class responsible for managing native pointers owned by the C2PA Python SDK. It guarantees: @@ -85,8 +88,9 @@ Notes: | --- | --- | | **Pointer freed exactly once** | Each native pointer is passed to `c2pa_free` at most once. No leak (zero frees) and no double-free. | | **Cleanup is idempotent** | Calling `close()` (or exiting a `with` block) multiple times is safe; after the first successful cleanup, further calls do nothing. | -| **Cleanup never raises** | The cleanup path (including `_release()` and `c2pa_free`) is wrapped so that exceptions are caught and logged, never re-raised. The original exception from the `with` block (if any) is never masked. | -| **State transitions are one-way** | Lifecycle moves only from UNINITIALIZED → ACTIVE → CLOSED. A closed resource cannot be reactivated. | +| **Cleanup never raises** | The cleanup path is wrapped so that exceptions are caught and logged, never re-raised. `_release()` runs inside `_safe_release()`, which logs and swallows; the `c2pa_free` call has its own handler; and `_cleanup_resources()` wraps both. The original exception from the `with` block (if any) is never masked. | +| **State transitions are one-way** | Lifecycle moves only from UNINITIALIZED to ACTIVE to CLOSED. A closed resource cannot be reactivated. | +| **Transitions go through helper methods** | Subclasses call `_activate()`, `_swap_handle()` or `_mark_consumed()` and never assign `_handle` or `_lifecycle_state` directly. `_activate()` and `_swap_handle()` validate before mutating, so an object cannot end up active with a null handle. | | **Ownership transfer is safe** | When a pointer is transferred elsewhere (e.g. via `_mark_consumed()`), the object stops managing it and does not call `c2pa_free` on it. | | **Public methods validate lifecycle state** | Every public API calls `_ensure_valid_state()` before use; closed or invalid state yields `C2paError` instead of undefined behavior or crashes. | @@ -105,10 +109,10 @@ The native Rust library exposes a single C FFI function, `c2pa_free`, that deall ```python @staticmethod def _free_native_ptr(ptr): - _lib.c2pa_free(ctypes.cast(ptr, ctypes.c_void_p)) + _lib.c2pa_free(ptr) ``` -All native pointers are freed through this single path, regardless of which constructor created them (`c2pa_reader_from_stream`, `c2pa_builder_from_json`, `c2pa_signer_from_info`, etc.). The `ctypes.cast` to `c_void_p` is needed because the C function accepts a generic void pointer regardless of the original type. +All native pointers are freed through this single path, regardless of which constructor created them (`c2pa_reader_from_stream`, `c2pa_builder_from_json`, `c2pa_signer_from_info`, etc.). No explicit `ctypes.cast` is needed: `c2pa_free`'s declared argtype is `c_void_p`, so ctypes converts any pointer instance on the way in. Casting explicitly with `ctypes.cast(ptr, c_void_p)` performs the same conversion but leaves a reference cycle behind on every call, so avoid it here. `ManagedResource` guarantees that `c2pa_free` is called exactly once per pointer: not zero times (leak), not twice (double-free). @@ -120,7 +124,8 @@ Each `ManagedResource` tracks its state with a `LifecycleState` enum: stateDiagram-v2 direction LR [*] --> UNINITIALIZED : __init__() - UNINITIALIZED --> ACTIVE : native pointer created + UNINITIALIZED --> ACTIVE : _activate(handle) + ACTIVE --> ACTIVE : _swap_handle(new_handle) ACTIVE --> CLOSED : close() / __exit__ / __del__ / _mark_consumed() ``` @@ -130,7 +135,17 @@ stateDiagram-v2 The transition from ACTIVE to CLOSED is one-way. Once closed, an object cannot be reactivated. -Every public method calls `_ensure_valid_state()` before doing any work. Besides checking the lifecycle state, this method also calls `_clear_error_state()`, which resets any stale error left over from a previous native library call. Without this, an error from one operation could leak into the next one and produce a misleading error message. +Each transition has one method that performs it, and subclasses must go through them rather than assigning `_handle` or `_lifecycle_state` directly: + +| Method | Transition | What it enforces | +| --- | --- | --- | +| `_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. Runs `_release()` first, so subclass cleanup still happens. Unlike the other two, it validates nothing. | + +Because activation is the only way in, no code path can leave an object ACTIVE while holding a null handle. + +Every public method calls `_ensure_valid_state()` before doing any work, which raises `C2paError` unless the resource is ACTIVE with a non-null handle. ## Ways to clean up @@ -165,10 +180,29 @@ If neither of the above is used, `__del__` attempts to free the native pointer w Cleanup must never raise an exception. A failure during cleanup (for example, the native library crashing on free) should not mask the original exception that caused the `with` block to exit. `ManagedResource` enforces this: - `close()` delegates to `_cleanup_resources()`, which wraps the entire cleanup sequence in a try/except that catches and silences all exceptions. +- `_release()` is never called directly during cleanup. It runs inside `_safe_release()`, which logs any failure with a traceback and returns normally, so a subclass whose `_release()` raises cannot stop the native pointer from being freed afterwards. - If freeing the native pointer fails, the error is logged via Python's `logging` module but not re-raised. - The state is set to `CLOSED` as the very first step, before attempting to free anything. If cleanup fails halfway, the object is still marked closed, preventing a second attempt from doing further damage. - Cleanup is idempotent. Calling `close()` on an already-closed object returns immediately. +All three cleanup entry points converge on the same method, and the exception handling sits at three different levels inside it: + +```mermaid +flowchart TD + E["close() / __exit__ / __del__"] --> CR["_cleanup_resources()"] + CR --> FP{"foreign process?"} + FP -->|yes| N["null the handle, set CLOSED,
do not free"] --> DONE([return]) + FP -->|no| ST{"already CLOSED?"} + ST -->|yes| DONE + ST -->|no| SET["set CLOSED first"] + SET --> REL["_safe_release()
logs and swallows"] + REL --> H{"handle set?"} + H -->|no| DONE + H -->|yes| FREE["_free_native_ptr()
logs on failure"] --> NULL["_handle = None"] --> DONE +``` + +The `foreign process` branch is explained under [Fork safety](#fork-safety) below. + ## Nesting resources When multiple native resources are in play at once, they can share a single `with` statement or use nested blocks. Either way, Python cleans them up in reverse order (right to left, or inner to outer). @@ -208,7 +242,7 @@ When the Reader is closed, it first releases its own resources (open file handle ## Builder lifecycle -A `Builder` follows the same pattern as Reader, with one difference: **signing consumes the builder**. The native library takes ownership of the builder's pointer during the sign operation. After signing, the builder is closed and cannot be reused. +A `Builder` follows the same pattern as Reader, with one difference: **signing closes the builder**. A Builder is single-use, so after signing it cannot be reused. ```mermaid stateDiagram-v2 @@ -219,14 +253,14 @@ stateDiagram-v2 CLOSED --> [*] note left of CLOSED - .sign() consumes the pointer - close() frees it + .sign() closes the builder + to enforce single use end note ``` -While `ACTIVE`, callers can use `.add_ingredient()`, `.add_action()`, etc. repeatedly. `.sign()` consumes the native pointer (ownership transfers to the native library), so the Builder cannot be reused afterward. Closing without signing frees the pointer normally. +While `ACTIVE`, callers can use `.add_ingredient()`, `.add_action()`, etc. repeatedly. `.sign()` closes the Builder when it returns, on both the success and the failure path. Closing without signing frees the pointer the same way. -After `.sign()`, the builder calls `_mark_consumed()`, which sets the handle to `None` and the state to `CLOSED`. Because the native library now owns the pointer, `ManagedResource` does not call `c2pa_free`. That would double-free memory the native library already manages. +The native sign call borrows the builder's pointer rather than taking ownership of it, so `Builder` never calls `_mark_consumed()` and the pointer is freed normally through `c2pa_free`. The close enforces single use; it is not a memory-management requirement. ## Ownership transfer @@ -234,13 +268,55 @@ Some operations transfer a native pointer from one object to another. When this `_mark_consumed()` handles this. It sets `_handle = None` and `_lifecycle_state = CLOSED` in one step. -There are two cases where this is relevant: +In the SDK this happens in one place: passing a `Signer` to a `Context`. The Context takes ownership of the Signer's native pointer, and the Signer must not be used again directly after that. + +The order of operations matters, because `c2pa_context_builder_set_signer` takes ownership on its error path as well as on success: + +```mermaid +sequenceDiagram + participant C as Caller + participant S as Signer + participant X as Context + participant N as Native lib + + C->>X: Context(settings, signer) + X->>S: _ensure_valid_state() + X->>X: copy signer._callback_cb to _signer_callback_cb + Note right of X: Pin the callback first:
the Signer is about to close + X->>N: c2pa_context_builder_set_signer(builder_ptr, handle) + + alt ctypes.ArgumentError + Note over X,N: Marshalling failed, native side never ran + X-->>C: re-raise, Signer keeps its handle + else call returned + N-->>X: result + X->>S: _mark_consumed() + Note right of S: Unconditional, before checking result:
set_signer takes ownership either way + X->>X: raise if result != 0 + end + + X->>N: c2pa_context_builder_build(builder_ptr) + N-->>X: context_ptr + X->>X: _activate(context_ptr) +``` + +Details in that sequence that are easy to get wrong: -- When a `Signer` is passed to a `Context`, the Context takes ownership of the Signer's native pointer. The Signer is marked consumed and must not be used again. +- The callback is copied to the Context *before* the transfer. `_mark_consumed()` runs `_release()`, so consuming the Signer drops its reference to the callback; a Context that copied it afterwards would be pointing at a callback nothing keeps alive. +- `_mark_consumed()` runs before the result is checked, not after. A failed `set_signer` has still taken the pointer, so waiting for a successful result would leak it. +- `ctypes.ArgumentError` is the exception. It means ctypes could not marshal the arguments, so the native function never ran and never saw the pointer. The Signer still owns its handle and is left untouched. -- When `Builder.sign()` is called, the native library consumes the Builder's pointer. The Builder marks itself consumed regardless of whether the sign operation succeeds or fails, because in both cases the native library has taken the pointer. +Both `c2pa_context_builder_set_signer` and `c2pa_context_builder_build` consume what they are given, so the `builder_ptr` is freed by the error handler only when the build was never reached. -## Consume-and-return +### Adopting a handle the SDK already owns + +Ownership can also arrive from the other direction: a native call returns a pointer that needs a Python wrapper around it. `_wrap_native_handle()` is the classmethod for that. It builds an instance with `object.__new__`, runs `ManagedResource.__init__` on it (which sets the lifecycle fields and stamps the owning process ID), runs `_init_attrs()` for the subclass attribute defaults, and calls `_activate()` with the handle. + +It deliberately skips `__init__`, because `__init__` would try to create a *new* native resource. That is why attribute defaults belong in `_init_attrs()`: it is the only initialization step this path runs. + +Ownership transfers only if the call returns. If `_wrap_native_handle()` raises, no wrapper exists to free the pointer, so the caller still owns it and must free it. + +## Consume-and-swap `_mark_consumed()` closes an object permanently. A different pattern is needed when the native library must replace an object's internal state without discarding the Python-side object. This happens with fragmented media: `Reader.with_fragment()` feeds a new BMFF fragment (used in DASH/HLS streaming) into an existing Reader, and the native library must rebuild its internal representation to account for the new data. The native API does this by consuming the old pointer and returning a new one. Creating a fresh `Reader` from scratch would not work because the native library needs the accumulated state from prior fragments. @@ -260,14 +336,46 @@ stateDiagram-v2 end note ``` +The object stays `ACTIVE` throughout because the Python-side object is still valid: it has a live native pointer, its public methods still work, and callers may continue using it (e.g. reading the updated manifest or feeding in another fragment). The lifecycle state does not change because from `ManagedResource`'s perspective nothing has closed. Only the underlying native pointer has been swapped. This is different from `_mark_consumed()`, where the object transitions to `CLOSED` and becomes unusable. The old pointer must not be freed by `ManagedResource` because the native library already consumed it as part of the FFI call. + +### `_consume_and_swap()` + +Every call of this shape goes through one helper, which takes the FFI call as a callable and handles the outcomes: + ```python # Reader.with_fragment() internally does: -new_ptr = _lib.c2pa_reader_with_fragment(self._handle, ...) -# self._handle (old pointer) is now invalid -self._handle = new_ptr +self._consume_and_swap( + lambda handle: _lib.c2pa_reader_with_fragment(handle, format_bytes, stream), + Reader._ERROR_MESSAGES['reader_error']) ``` -The object stays `ACTIVE` throughout because the Python-side object is still valid: it has a live native pointer, its public methods still work, and callers may continue using it (e.g. reading the updated manifest or feeding in another fragment). The lifecycle state does not change because from `ManagedResource`'s perspective nothing has closed. Only the underlying native pointer has been swapped. This is different from `_mark_consumed()`, where the object transitions to `CLOSED` and becomes unusable. The old pointer must not be freed by `ManagedResource` because the native library already consumed it as part of the FFI call. +The call is passed as a lambda because the helper supplies the handle and, on success, replaces it via `_swap_handle()`. + +The helper exists because a null return can be ambiguous. The native function validates the borrowed pointer first, then takes ownership, then does the work, so a null result can mean either "rejected your pointer, never took it" or "took your pointer, then failed". The native error message is what tells them apart: + +| Native error | Who owns the handle | +| --- | --- | +| `UntrackedPointer:` or `WrongPointerType:` | Still ours: rejected before ownership moved, so the handle is kept and the resource stays `ACTIVE` | +| Any other error | Assumed taken, so `_mark_consumed()` runs and the resource goes `CLOSED`. The raised error is typed from the native message. | +| No error at all | Same ownership conclusion, but nothing to type the error from, so 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. + +### Adopting the handle before giving it away + +`Reader._init_from_context` and `Builder._init_from_context` both create a native object, immediately `_activate()` it, and only then make the consuming call. Reduced to its shape: + +```python +self._activate(reader_ptr) + +self._consume_and_swap( + lambda handle: _lib.c2pa_reader_with_stream( + handle, format_bytes, self._own_stream._stream, + ), + Reader._ERROR_MESSAGES['reader_error']) +``` + +Activating a handle that is about to be handed to the native library looks backwards, and there are two reasons for it. `_consume_and_swap` needs an active resource to read the handle from and swap the result into. It also puts the intermediate pointer under normal cleanup before anything can go wrong with it: whichever way the consuming call goes, `close()` and `__del__` will free the pointer if the native side did not take it. The alternative, holding the pointer in a local variable across the call, means every failure path has to decide for itself whether to free it. ## Subclass-specific cleanup with `_release()` @@ -277,14 +385,56 @@ Examples from the codebase: | Class | What `_release()` cleans up | | --- | --- | -| Reader | Closes owned file handles and stream wrappers | -| Context | Drops the reference to the signer callback | +| Reader | Drops the manifest caches, closes owned file handles and stream wrappers, and drops the reference to the Context | +| Builder | Drops the reference to the Context | +| Context | Drops the reference to the signer callback. `has_signer` is left as it was: it records how the Context was configured, and stays readable after close. | | Signer | Drops the reference to the signing callback | | Settings | (no override, nothing extra to clean up) | -| Builder | (no override, nothing extra to clean up) | The cleanup order matters: `_release()` runs first (closing streams, dropping callbacks), then `c2pa_free` frees the native pointer. This order prevents the native library from accessing Python objects that no longer exist. +### Dropping a Context reference + +`Reader` and `Builder` both keep a `_context` attribute that is written once and never read. It is not dead code: it is what keeps the Context alive while the native handle depends on it. Without that reference, `Reader("image/jpeg", stream, context=Context())` would let the Context become collectable as soon as the constructor returned, even though the reader is still using it. + +Clearing it in `_release()` is the other half of that. A closed Reader has no further use for the Context, and holding the reference would keep alive an object nothing can reach through the Reader's public API. + +Dropping the reference before the native pointer is freed is safe because the native side does not depend on the Python object staying alive. `Reader::from_shared_context` clones the underlying `Arc`, so the native reader holds its own count on the context and does not care whether Python still points at it. + +## Fork safety + +`fork()` copies the calling process, including every Python object holding a native pointer. The child gets its own copy of the wrapper object, but there is still only one native allocation, and the parent owns it. + +If the child's copy were cleaned up normally, two things would go wrong. The obvious one is a double-free: the child frees a pointer the parent is still using. The subtler one is a deadlock. `fork()` only carries over the calling thread, so a native mutex held by any other thread at the moment of the fork stays locked forever in the child. Calling into the native library to free anything can block on that mutex and never return. + +So the SDK does not free native memory in a process that did not allocate it. `ManagedResource.__init__` stamps the creating process ID onto the object (`record_owner_pid`), and `is_foreign_process()` compares it against the current PID during cleanup: + +```mermaid +sequenceDiagram + participant P as Parent process + participant O as Reader object + participant F as Forked child + + P->>O: __init__ stamps _owner_pid + P->>F: fork() + Note over O,F: Child inherits a copy of the object.
One native allocation, two Python copies. + + F->>F: child's copy is cleaned up + F->>F: is_foreign_process() is true + Note right of F: Do not free: the parent owns it,
and a native mutex may be locked
by a thread that did not survive the fork + F->>F: null the handle, mark CLOSED + + P->>O: close() + P->>P: frees the native pointer normally +``` + +Both `_cleanup_resources()` and `_mark_consumed()` take this branch. Neither simply skips the work: they null the handle and mark the object `CLOSED` so the child cannot go on to use it or try to free it later. Mutating the child's copy has no effect on the parent's, which is untouched and still valid. + +The memory the child skips is not lost for good. A child that calls `exec()` replaces its address space; a child that exits has its memory reclaimed by the OS. Even a long-lived child (a `multiprocessing` worker using the fork start method) retains at most the objects it inherited at fork time, which is a bounded, one-off amount rather than a growing leak. Anything the child allocates itself carries the child's own PID and is freed normally. + +> [!NOTE] +> `is_foreign_process()` returns `False` when no owner PID was ever recorded, so an object that somehow missed the stamp is cleaned up as before rather than leaking silently. + ## Why is `Stream` not a `ManagedResource`? `Stream` wraps a Python stream-like object (file stream or memory stream) so the native library can read from and write to it via callbacks. It does not inherit from `ManagedResource`, and it uses `c2pa_release_stream()` instead of `c2pa_free()` for cleanup. @@ -299,26 +449,32 @@ To wrap a new native resource, inherit from `ManagedResource` and follow these r ```python class NativeResource(ManagedResource): - def __init__(self, arg): - super().__init__() - - # 1. Initialize ALL instance attributes before any code - # that can raise. If __init__ fails partway through, - # __del__ will call _release(), which accesses these - # attributes. If they don't exist, _release() raises AttributeError. + def _init_attrs(self): + # 1. Declare ALL instance attributes here, not in __init__. + # _wrap_native_handle() builds instances around an existing + # handle without running __init__, and calls this instead. + # An attribute set only in __init__ would be missing there. + # This also runs before anything that can raise, so a + # half-constructed object still has what _release() reads. + super()._init_attrs() self._my_stream = None self._my_cache = None - # 2. Create the native pointer. - ptr = _lib.c2pa_my_resource_new(arg) - _check_ffi_operation_result(ptr, "Failed to create MyResource") + def __init__(self, arg): + super().__init__() + self._init_attrs() + + # 2. Create the native pointer. Pass the error message as a + # template: _check_ffi_operation_result fills in the native + # error, or "Unknown error" when there is none. + handle = _lib.c2pa_my_resource_new(arg) + _check_ffi_operation_result(handle, "Failed to create MyResource: {}") - # 3. Only set _handle and activate AFTER the FFI call - # succeeded. If it raised, _lifecycle_state stays - # UNINITIALIZED and cleanup won't try to free a - # pointer that doesn't exist. - self._handle = ptr - self._lifecycle_state = LifecycleState.ACTIVE + # 3. Take ownership only after the FFI call succeeded. + # _activate() rejects a null handle and refuses to run twice, + # so the object is never ACTIVE without a live pointer. + # Never assign self._handle or self._lifecycle_state directly. + self._activate(handle) def _release(self): # 4. Clean up class-specific resources. @@ -347,9 +503,11 @@ class NativeResource(ManagedResource): ### Troubleshooting -- If `self._my_callback = None` is set after the FFI call that can raise, and the call fails, `_release()` will try to access `self._my_callback` and crash with `AttributeError`. Always initialize attributes right after `super().__init__()`. +- If an attribute is set only in `__init__`, an instance built by `_wrap_native_handle()` will not have it, because that path never runs `__init__`. The failure shows up later as an `AttributeError` from whichever method reads the attribute, often `_release()` during cleanup. Declare attributes in `_init_attrs()` and call it from `__init__`. + +- If `_init_attrs()` is called after an FFI call that can raise, and the call fails, `_release()` will access attributes that do not exist yet and crash with `AttributeError`. Call it immediately after `super().__init__()`, before anything that can fail. -- If `_lifecycle_state = ACTIVE` is set before the FFI call and the call fails, cleanup will try to free a null or invalid pointer. Activation should happen only after a valid handle exists. +- Assigning `self._handle` or `self._lifecycle_state` directly bypasses the checks that make the lifecycle safe. `_activate()` refuses a null handle and refuses to run on an already-active object; `_swap_handle()` requires the resource to be active and the replacement non-null. Assigning the fields yourself gives up both, and the resulting bugs (an ACTIVE object with a null handle, or a silently discarded pointer) surface far from their cause. - If `_release()` raises, the exception is silently swallowed by `_cleanup_resources()`. It will not be visible unless logs are checked. Define a lifecycle for managed resources so `_release()` can check whether they need releasing. Wrap the actual release call in try/except as a fallback for unexpected failures. From bdf671c99d8d2039a2429dc784cba866c359ad72 Mon Sep 17 00:00:00 2001 From: tmathern <60901087+tmathern@users.noreply.github.com> Date: Mon, 20 Jul 2026 21:53:18 -0700 Subject: [PATCH 2/5] fix: Error handling memory --- src/c2pa/c2pa.py | 52 ++++++++++++++++++++++++++++++---------- tests/test_unit_tests.py | 30 ++++++++++++++--------- 2 files changed, 58 insertions(+), 24 deletions(-) diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index 02447e67..c0b90c71 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -319,6 +319,32 @@ def _mark_consumed(self): self._handle = None self._lifecycle_state = LifecycleState.CLOSED + 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. Post-state matches _mark_consumed, + so later cleanup will not free again. + """ + + if is_foreign_process(self): + self._handle = None + self._lifecycle_state = LifecycleState.CLOSED + return + + 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__) + + self._handle = None + self._lifecycle_state = LifecycleState.CLOSED + def _activate(self, handle): """Attach a native handle to self and mark it active. Ownership of `handle` transfers here. @@ -374,14 +400,13 @@ def _swap_handle(self, new_handle): def _consume_and_swap(self, ffi_call, error_message): """Run an FFI call that consumes this handle and returns a replacement. - The native lib may take ownership partway through the call. - The native error tells if ownership was transferred: - - a pointer rejection (`_PRE_CONSUME_ERROR_TAGS`) precedes the - transfer, so the handle is still ours and is kept; - - any other error means it was taken, then the operation failed. + On success the native lib consumed our handle and returns 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 without clearing it first. - The slot is sticky: c2pa_error() peeks and nothing empties it. + The error is read (into an owned string, freeing the native c-string) + before the input is freed, so the message is not lost. Args: ffi_call: Callable taking the current handle, returning the @@ -399,19 +424,19 @@ def _consume_and_swap(self, ffi_call, error_message): # and the handle is untouched. raise except BaseException as e: - self._mark_consumed() + self._release_handle() raise C2paError(error_message.format(e)) from e if new_ptr: self._swap_handle(new_ptr) return + # Retrieve the error error = _read_native_error() if error: if any(tag in error for tag in ManagedResource._PRE_CONSUME_ERROR_TAGS): - # Rejected before ownership transferred, - # so the handle is still ours. + # Rejected before ownership transferred: the handle is still ours logger.warning( "%s: native call rejected the handle before taking " "ownership (%s); handle retained", @@ -419,11 +444,12 @@ def _consume_and_swap(self, ffi_call, error_message): error) _raise_typed_c2pa_error(error) - # Ownership transferred and then an operation failed. - self._mark_consumed() + # Ownership transferred and then the operation failed: + # free eagerly (c2pa_free handles not double-freeing), then raise. + self._release_handle() _raise_typed_c2pa_error(error) - self._mark_consumed() + self._release_handle() raise C2paError(error_message.format("Unknown error")) @classmethod diff --git a/tests/test_unit_tests.py b/tests/test_unit_tests.py index f440d115..a1f24729 100644 --- a/tests/test_unit_tests.py +++ b/tests/test_unit_tests.py @@ -7645,9 +7645,12 @@ 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_consumes_self(self): + def test_builder_with_archive_null_return_frees_self(self): builder = Builder(self.test_manifest) - consumed_handle = builder._handle + released_handle = builder._handle + + # Simulate an error being set + c2pa_module._lib.c2pa_error_set_last(b"Other: mocked test error") real_call = c2pa_module._lib.c2pa_builder_with_archive c2pa_module._lib.c2pa_builder_with_archive = lambda b, s: None try: @@ -7656,15 +7659,15 @@ def test_builder_with_archive_null_return_consumes_self(self): finally: c2pa_module._lib.c2pa_builder_with_archive = real_call - # The FFI consumed the old handle and returned no replacement, - # so there is nothing left for this object to own... + # Nothing left to own after failing self.assertIsNone(builder._handle) self.assertEqual(builder._lifecycle_state, LifecycleState.CLOSED) + # The eager free-on-error already ran, so close() must not free again. freed = self._instrument_frees() builder.close() - self.assertEqual(self._free_count(freed, consumed_handle), 0, - "close() freed a handle the FFI already consumed") + self.assertEqual(self._free_count(freed, released_handle), 0, + "close() double-freed an already-released handle") def test_reader_with_fragment_null_return_consumes_self(self): init_path = os.path.join(FIXTURES_DIR, "dashinit.mp4") @@ -7673,9 +7676,8 @@ def test_reader_with_fragment_null_return_consumes_self(self): reader = Reader("video/mp4", init) consumed_handle = reader._handle - # The mock sets no error, which no native path does. Plant an - # operation-style one so a stale tag from another test is not read. - c2pa_module._lib.c2pa_error_set_last(b"Other: mocked null return") + # Simulate an error being set + c2pa_module._lib.c2pa_error_set_last(b"Other: mocked test error") real_call = c2pa_module._lib.c2pa_reader_with_fragment c2pa_module._lib.c2pa_reader_with_fragment = ( @@ -7869,8 +7871,8 @@ def test_unknown_failure_drops_handle_without_freeing(self): reader = Reader("video/mp4", init) consumed_handle = reader._handle - # Not a pre-consume tag, so the mocked failure reads as unplaceable. - c2pa_module._lib.c2pa_error_set_last(b"Other: mocked null return") + # Simulate an error being set + c2pa_module._lib.c2pa_error_set_last(b"Other: mocked test error") real_call = c2pa_module._lib.c2pa_reader_with_fragment c2pa_module._lib.c2pa_reader_with_fragment = ( lambda r, f, s, frag: None) @@ -8276,6 +8278,8 @@ def test_consumed_reader_closes_backing_file(self): backing_file = reader._backing_file self.assertFalse(backing_file.closed) + # Simulate an error being set + c2pa_module._lib.c2pa_error_set_last(b"Other: mocked test error") real_call = c2pa_module._lib.c2pa_reader_with_fragment c2pa_module._lib.c2pa_reader_with_fragment = ( lambda r, f, s, frag: None) @@ -8296,6 +8300,8 @@ def test_consumed_builder_releases_context(self): builder = Builder(self.test_manifest, context=context) archive = self._make_archive() + # Simulate an error being set + c2pa_module._lib.c2pa_error_set_last(b"Other: mocked test error") real_call = c2pa_module._lib.c2pa_builder_with_archive c2pa_module._lib.c2pa_builder_with_archive = lambda b, s: None try: @@ -8343,6 +8349,8 @@ def test_consumed_reader_clears_caches(self): reader.json() self.assertIsNotNone(reader._manifest_json_str_cache) + # Simulate an error being set + c2pa_module._lib.c2pa_error_set_last(b"Other: mocked test error") real_call = c2pa_module._lib.c2pa_reader_with_fragment c2pa_module._lib.c2pa_reader_with_fragment = ( lambda r, f, s, frag: None) From f0a4ee1e7780a1cec5bb9ccff6094658aa309fc4 Mon Sep 17 00:00:00 2001 From: Tania Mathern Date: Tue, 21 Jul 2026 10:03:23 -0700 Subject: [PATCH 3/5] fix: Update the docs --- docs/native-resources-management.md | 28 +++++++++---- src/c2pa/c2pa.py | 48 ++++++++++++++++++---- tests/test_unit_tests.py | 64 +++++++++++++++++++---------- 3 files changed, 104 insertions(+), 36 deletions(-) diff --git a/docs/native-resources-management.md b/docs/native-resources-management.md index 21384c3b..36cac053 100644 --- a/docs/native-resources-management.md +++ b/docs/native-resources-management.md @@ -141,7 +141,8 @@ 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. Runs `_release()` first, so subclass cleanup still happens. Unlike the other two, it validates nothing. | +| `_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`. | Because activation is the only way in, no code path can leave an object ACTIVE while holding a null handle. @@ -336,7 +337,7 @@ stateDiagram-v2 end note ``` -The object stays `ACTIVE` throughout because the Python-side object is still valid: it has a live native pointer, its public methods still work, and callers may continue using it (e.g. reading the updated manifest or feeding in another fragment). The lifecycle state does not change because from `ManagedResource`'s perspective nothing has closed. Only the underlying native pointer has been swapped. This is different from `_mark_consumed()`, where the object transitions to `CLOSED` and becomes unusable. The old pointer must not be freed by `ManagedResource` because the native library already consumed it as part of the FFI call. +On success the object stays `ACTIVE` because the Python-side object is still valid: it has a live native pointer, its public methods still work, and callers may continue using it (e.g. reading the updated manifest or feeding in another fragment). The lifecycle state does not change because from `ManagedResource`'s perspective nothing has closed. Only the underlying native pointer has been swapped. This is different from `_mark_consumed()`, where the object transitions to `CLOSED` and becomes unusable. On the success path the old pointer must not be freed by `ManagedResource` because the native library already consumed it as part of the FFI call. The failure path is different and is covered by the triage below. ### `_consume_and_swap()` @@ -353,14 +354,25 @@ The call is passed as a lambda because the helper supplies the handle and, on su The helper exists because a null return can be ambiguous. The native function validates the borrowed pointer first, then takes ownership, then does the work, so a null result can mean either "rejected your pointer, never took it" or "took your pointer, then failed". The native error message is what tells them apart: -| Native error | Who owns the handle | -| --- | --- | -| `UntrackedPointer:` or `WrongPointerType:` | Still ours: rejected before ownership moved, so the handle is kept and the resource stays `ACTIVE` | -| Any other error | Assumed taken, so `_mark_consumed()` runs and the resource goes `CLOSED`. The raised error is typed from the native message. | -| No error at all | Same ownership conclusion, but nothing to type the error from, so the caller's message is raised with `"Unknown error"` filled in. | +| 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. 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. + +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. + ### Adopting the handle before giving it away `Reader._init_from_context` and `Builder._init_from_context` both create a native object, immediately `_activate()` it, and only then make the consuming call. Reduced to its shape: @@ -513,7 +525,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. If the pointer is freed manually and `ManagedResource` frees it again, the process crashes (double-free). +- 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. - 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 c0b90c71..fdf04c44 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -234,9 +234,13 @@ class ManagedResource: validated, which takes ownership of it and marks the resource active. Never assign `self._handle` or `self._lifecycle_state` directly. - Call `_swap_handle(new_handle)` instead when an FFI call consumed the - current handle and returned a replacement. + 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. + 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. - Override `_release()` to free class-specific resources (streams, caches, callbacks, etc.), called before the native pointer is freed. @@ -268,8 +272,21 @@ def _free_native_ptr(ptr): c2pa_free's argtype is c_void_p, so ctypes converts any pointer instance directly. (ctypes.cast(ptr, c_void_p) would do the same conversion but leaves a reference cycle behind on every call.) + + Returns c2pa_free's status code: 0 when the pointer was really freed + (a still-owned handle, or the try-assign native), -1 when the pointer + registry rejected an already-consumed address without touching memory. + A -1 is expected on the eager-free path when the released native has + already dropped the value, so it is logged at debug, not treated as an + error. """ - _lib.c2pa_free(ptr) + result = _lib.c2pa_free(ptr) + if result != 0: + logger.debug( + "c2pa_free returned %s for an untracked pointer " + "(already consumed; registry-guarded no-op)", + result) + return result def _ensure_valid_state(self): """Raise if the resource is closed or uninitialized.""" @@ -325,6 +342,10 @@ def _release_handle(self): Freeing synchronously at the error site leaves no window in which the address could be reused and re-tracked. Post-state matches _mark_consumed, so later cleanup will not free again. + + Sets CLOSED before releasing and freeing (the same order as the close + path in _cleanup_resources) so an ill-timed interrupt can never leave an + ACTIVE object holding a freed handle. """ if is_foreign_process(self): @@ -332,6 +353,13 @@ def _release_handle(self): self._lifecycle_state = LifecycleState.CLOSED return + # Nothing to free unless we are ACTIVE; still normalize the post-state. + 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: @@ -340,10 +368,10 @@ def _release_handle(self): except Exception: logger.error( "Failed to free native %s resources", - type(self).__name__) - - self._handle = None - self._lifecycle_state = LifecycleState.CLOSED + type(self).__name__, + exc_info=True) + finally: + self._handle = None def _activate(self, handle): """Attach a native handle to self and mark it active. @@ -408,6 +436,12 @@ def _consume_and_swap(self, ffi_call, error_message): The error is read (into an owned string, freeing the native c-string) before the input is freed, so the message is not lost. + The native error slot is sticky and thread-local: it stays set until + the next error on the same thread overwrites it, and the SDK does not + clear it before this call. The triage below therefore trusts that a + failing native path set its own error; a stale message from an earlier + call on this thread could otherwise be misread as this call's. + Args: ffi_call: Callable taking the current handle, returning the replacement or null. diff --git a/tests/test_unit_tests.py b/tests/test_unit_tests.py index a1f24729..80272f18 100644 --- a/tests/test_unit_tests.py +++ b/tests/test_unit_tests.py @@ -7648,14 +7648,18 @@ def test_consumed_signer_close_frees_nothing(self): def test_builder_with_archive_null_return_frees_self(self): builder = Builder(self.test_manifest) released_handle = builder._handle + archive = self._make_archive() - # Simulate an error being set + # Mimic an error. 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 + + # Instrument before the failure... + freed = self._instrument_frees() try: with self.assertRaises(Error): - builder.with_archive(self._make_archive()) + builder.with_archive(archive) finally: c2pa_module._lib.c2pa_builder_with_archive = real_call @@ -7663,25 +7667,32 @@ def test_builder_with_archive_null_return_frees_self(self): self.assertIsNone(builder._handle) self.assertEqual(builder._lifecycle_state, LifecycleState.CLOSED) - # The eager free-on-error already ran, so close() must not free again. - freed = self._instrument_frees() + # The error path frees the old handle exactly once. + 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. builder.close() - self.assertEqual(self._free_count(freed, released_handle), 0, + self.assertEqual(self._free_count(freed, released_handle), 1, "close() double-freed an already-released handle") - def test_reader_with_fragment_null_return_consumes_self(self): + def test_reader_with_fragment_null_return_frees_self(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) - consumed_handle = reader._handle + released_handle = reader._handle - # Simulate an error being set + # Mimic an error. 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. + freed = self._instrument_frees() try: with open(init_path, "rb") as init, \ open(fragment_path, "rb") as frag: @@ -7693,27 +7704,34 @@ def test_reader_with_fragment_null_return_consumes_self(self): self.assertIsNone(reader._handle) self.assertEqual(reader._lifecycle_state, LifecycleState.CLOSED) - freed = self._instrument_frees() - reader.close() - self.assertEqual(self._free_count(freed, consumed_handle), 0, - "close() freed a handle the FFI already consumed") + # The error path frees the old handle exactly once. + self.assertEqual(self._free_count(freed, released_handle), 1, + "error path did not free the old handle exactly once") - def test_reader_with_fragment_ffi_raise_consumes_self(self): - # If the ctypes call itself raises (not a null return), the callee has - # already consumed the old handle, so with_fragment must mark self - # consumed rather than leave a dangling pointer that close() would - # double-free. + # close() must not free it again. + reader.close() + self.assertEqual(self._free_count(freed, released_handle), 1, + "close() freed a handle the error path already freed") + + def test_reader_with_fragment_ffi_raise_frees_self(self): + # If the ctypes call itself raises (not a null return), the failure runs + # through the except BaseException branch, which frees the handle + # eagerly. with_fragment must free exactly once and leave nothing for + # close() to double-free. 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) - consumed_handle = reader._handle + released_handle = reader._handle def _raise(*_args): raise RuntimeError("boom") real_call = c2pa_module._lib.c2pa_reader_with_fragment c2pa_module._lib.c2pa_reader_with_fragment = _raise + + # Instrument before the failure so the eager free is counted. + freed = self._instrument_frees() try: with open(init_path, "rb") as init, \ open(fragment_path, "rb") as frag: @@ -7725,10 +7743,14 @@ def _raise(*_args): self.assertIsNone(reader._handle) self.assertEqual(reader._lifecycle_state, LifecycleState.CLOSED) - freed = self._instrument_frees() + # The error path frees the old handle exactly once. + 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. reader.close() - self.assertEqual(self._free_count(freed, consumed_handle), 0, - "close() freed a handle the FFI already consumed") + 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 From b6fcf9ce7c3d8768361f465b378c90038a1e25be Mon Sep 17 00:00:00 2001 From: Tania Mathern Date: Tue, 21 Jul 2026 11:25:24 -0700 Subject: [PATCH 4/5] fix: Refactor --- docs/native-resources-management.md | 4 +-- src/c2pa/c2pa.py | 47 ++++++++++++----------------- tests/test_unit_tests.py | 12 ++++---- 3 files changed, 27 insertions(+), 36 deletions(-) diff --git a/docs/native-resources-management.md b/docs/native-resources-management.md index 36cac053..1b01362e 100644 --- a/docs/native-resources-management.md +++ b/docs/native-resources-management.md @@ -3,7 +3,7 @@ `ManagedResource` is the internal base class used by the C2PA Python SDK to wrap native (Rust/FFI) pointers. When adding new wrappers around native resources `ManagedResource` should be subclassed and follow the documented lifecycle rules. > [!NOTE] -> `ManagedResource` and the lifecycle machinery described here are internal to the SDK, not part of its public API. In most cases, code that reads and writes C2PA data should use the public wrappers (`Reader`, `Builder`, `Signer`, `Context`, `Settings`). +> `ManagedResource` and the lifecycle machinery described here are internal to the SDK. In most cases, code that reads and writes C2PA data should use the public wrappers (`Reader`, `Builder`, `Signer`, `Context`, `Settings`). ## Why `ManagedResource`? @@ -112,7 +112,7 @@ def _free_native_ptr(ptr): _lib.c2pa_free(ptr) ``` -All native pointers are freed through this single path, regardless of which constructor created them (`c2pa_reader_from_stream`, `c2pa_builder_from_json`, `c2pa_signer_from_info`, etc.). No explicit `ctypes.cast` is needed: `c2pa_free`'s declared argtype is `c_void_p`, so ctypes converts any pointer instance on the way in. Casting explicitly with `ctypes.cast(ptr, c_void_p)` performs the same conversion but leaves a reference cycle behind on every call, so avoid it here. +All native pointers are freed through this single path, regardless of which constructor created them (`c2pa_reader_from_stream`, `c2pa_builder_from_json`, `c2pa_signer_from_info`, etc.). No explicit `ctypes.cast` is needed: `c2pa_free`'s declared argtype is `c_void_p`, so ctypes converts any pointer instance on the way in. Casting explicitly with `ctypes.cast(ptr, c_void_p)` performs the same conversion but leaves a reference cycle behind on every call, which creates additional load on the (Python) garbage collector. `ManagedResource` guarantees that `c2pa_free` is called exactly once per pointer: not zero times (leak), not twice (double-free). diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index fdf04c44..b78e06d4 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -273,18 +273,17 @@ def _free_native_ptr(ptr): instance directly. (ctypes.cast(ptr, c_void_p) would do the same conversion but leaves a reference cycle behind on every call.) - Returns c2pa_free's status code: 0 when the pointer was really freed - (a still-owned handle, or the try-assign native), -1 when the pointer - registry rejected an already-consumed address without touching memory. - A -1 is expected on the eager-free path when the released native has - already dropped the value, so it is logged at debug, not treated as an - error. + Returns c2pa_free's status code: + 0 when the pointer was really freed, + -1 when the pointer registry rejected an already-consumed address. + A -1 can be expected on the eager-free path when the candidate released + native memory has already dropped the value, and is gracefully handled by + the native lib too. """ result = _lib.c2pa_free(ptr) if result != 0: logger.debug( - "c2pa_free returned %s for an untracked pointer " - "(already consumed; registry-guarded no-op)", + "c2pa_free returned %s for an untracked pointer ", result) return result @@ -338,14 +337,9 @@ 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. Post-state matches _mark_consumed, - so later cleanup will not free again. - - Sets CLOSED before releasing and freeing (the same order as the close - path in _cleanup_resources) so an ill-timed interrupt can never leave an - ACTIVE object holding a freed handle. + address could be reused and re-tracked. + Sets CLOSED before releasing and freeing. """ if is_foreign_process(self): @@ -353,7 +347,7 @@ def _release_handle(self): self._lifecycle_state = LifecycleState.CLOSED return - # Nothing to free unless we are ACTIVE; still normalize the post-state. + # Nothing to free, normalize states. if self._lifecycle_state != LifecycleState.ACTIVE: self._handle = None self._lifecycle_state = LifecycleState.CLOSED @@ -428,19 +422,16 @@ def _swap_handle(self, new_handle): 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 our handle and returns a new one, + 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 (into an owned string, freeing the native c-string) - before the input is freed, so the message is not lost. - - The native error slot is sticky and thread-local: it stays set until - the next error on the same thread overwrites it, and the SDK does not - clear it before this call. The triage below therefore trusts that a - failing native path set its own error; a stale message from an earlier - call on this thread could otherwise be misread as this call's. + 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). Args: ffi_call: Callable taking the current handle, returning the @@ -454,7 +445,7 @@ def _consume_and_swap(self, ffi_call, error_message): try: new_ptr = ffi_call(self._handle) except ctypes.ArgumentError: - # Marshalling failed, so the call never reached the native side + # Marshalling failed. The call never reached the native side, # and the handle is untouched. raise except BaseException as e: @@ -479,7 +470,7 @@ def _consume_and_swap(self, ffi_call, error_message): _raise_typed_c2pa_error(error) # Ownership transferred and then the operation failed: - # free eagerly (c2pa_free handles not double-freeing), then raise. + # free eagerly (native lib handles not double-freeing), then raise. self._release_handle() _raise_typed_c2pa_error(error) diff --git a/tests/test_unit_tests.py b/tests/test_unit_tests.py index 80272f18..58db24d7 100644 --- a/tests/test_unit_tests.py +++ b/tests/test_unit_tests.py @@ -7667,7 +7667,7 @@ def test_builder_with_archive_null_return_frees_self(self): self.assertIsNone(builder._handle) self.assertEqual(builder._lifecycle_state, LifecycleState.CLOSED) - # The error path frees the old handle exactly once. + # 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") @@ -7704,7 +7704,7 @@ 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 exactly once. + # 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") @@ -7714,9 +7714,9 @@ def test_reader_with_fragment_null_return_frees_self(self): "close() freed a handle the error path already freed") def test_reader_with_fragment_ffi_raise_frees_self(self): - # If the ctypes call itself raises (not a null return), the failure runs - # through the except BaseException branch, which frees the handle - # eagerly. with_fragment must free exactly once and leave nothing for + # 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. init_path = os.path.join(FIXTURES_DIR, "dashinit.mp4") fragment_path = os.path.join(FIXTURES_DIR, "dash1.m4s") @@ -7743,7 +7743,7 @@ def _raise(*_args): self.assertIsNone(reader._handle) self.assertEqual(reader._lifecycle_state, LifecycleState.CLOSED) - # The error path frees the old handle exactly once. + # 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") From ccce1a273b43ca2af96cb7dfb3dfebef55ab6660 Mon Sep 17 00:00:00 2001 From: Tania Mathern Date: Tue, 21 Jul 2026 11:45:16 -0700 Subject: [PATCH 5/5] fix: Fix script --- tests/perf/entrypoint.sh | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/tests/perf/entrypoint.sh b/tests/perf/entrypoint.sh index f0f1f917..e0cbf737 100644 --- a/tests/perf/entrypoint.sh +++ b/tests/perf/entrypoint.sh @@ -16,8 +16,14 @@ else PLATFORM="x86_64-unknown-linux-gnu" fi -echo "Downloading c2pa native lib: $C2PA_VERSION / $PLATFORM" -C2PA_LIBS_PLATFORM=$PLATFORM python scripts/download_artifacts.py "$C2PA_VERSION" +# Skip the GitHub API round-trip when the lib is already on disk +# Set C2PA_FORCE_DOWNLOAD=1 to override. +if [ -z "$C2PA_FORCE_DOWNLOAD" ] && [ -f "artifacts/$PLATFORM/libc2pa_c.so" ]; then + echo "Using cached c2pa native lib: artifacts/$PLATFORM (set C2PA_FORCE_DOWNLOAD=1 to re-download)" +else + echo "Downloading c2pa native lib: $C2PA_VERSION / $PLATFORM" + C2PA_LIBS_PLATFORM=$PLATFORM python scripts/download_artifacts.py "$C2PA_VERSION" +fi # Replicate what setup.py copy_platform_libraries() does: # So the correct Linux library is here for the Dockerfile