diff --git a/.github/workflows/threading-benchmark.yml b/.github/workflows/threading-benchmark.yml new file mode 100644 index 00000000..ec44306a --- /dev/null +++ b/.github/workflows/threading-benchmark.yml @@ -0,0 +1,52 @@ +name: Python SDK threading checks + +on: + pull_request: + types: + - opened + - reopened + - synchronize + - labeled + +permissions: + contents: read + +jobs: + threading-benchmark: + name: Python SDK threading checks + runs-on: ubuntu-24.04-arm + # Backstop: In case there is a hang. + timeout-minutes: 20 + if: | + contains(github.event.pull_request.labels.*.name, 'check-threading-benchmark') && + ( + github.event.pull_request.author_association == 'COLLABORATOR' || + github.event.pull_request.author_association == 'MEMBER' || + github.event.pull_request.author_association == 'OWNER' + ) + steps: + - uses: actions/checkout@v4 + + - name: Build perf image + run: make perf-image + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + + # Make sure crashes would be reported. + - name: Check the harness can detect failures + run: make threading-bench-self-test + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + + - name: Run thread-safety invariants + run: make threading-bench + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + + - name: Upload failure logs + if: always() + uses: actions/upload-artifact@v4 + with: + name: threading-invariant-logs + path: tests/perf/reports/*-threads.log + if-no-files-found: ignore diff --git a/.gitignore b/.gitignore index 147e8357..4c4140d5 100644 --- a/.gitignore +++ b/.gitignore @@ -126,3 +126,6 @@ src/c2pa/libs/ # Memory profiling reports tests/perf/reports/*.html tests/perf/reports/*.bin + +# Threading failure logs +tests/perf/reports/*.log diff --git a/Makefile b/Makefile index d4e7fffd..e53e1f2e 100644 --- a/Makefile +++ b/Makefile @@ -172,3 +172,25 @@ memory-use-bench: clean-memory-perf-reports: rm -f tests/perf/reports/*.html tests/perf/reports/*.bin @echo "Cleared tests/perf/reports/" + +# Thread-safety invariants (runs in Docker, same image as the memory benchmark) +# More details for usage are in tests/perf/README.md +THREAD_ROUNDS ?= 20 + +# Checks that the harness itself reports crashes, hangs and plain exceptions +# correctly. A harness that cannot see a failure is indistinguishable from a +# passing run, so this gates the real suite rather than documenting it. +.PHONY: threading-bench-self-test +threading-bench-self-test: perf-image + docker run --rm -v $(PWD):/workspace -e PYTHONPATH=/workspace/src -e GITHUB_TOKEN c2pa-memray-$(PERF_ENV) python -m tests.perf.run_thread_profile --self-test + +# Runs the thread-safety invariant scenarios. Pre-requisite: Docker image built +# using `make perf-image` (or `perf-image-rebuild`). +.PHONY: threading-bench +threading-bench: threading-bench-self-test + docker run --rm -v $(PWD):/workspace $(GH_SUMMARY_MOUNT) -e PYTHONPATH=/workspace/src -e PERF_ENV=$(PERF_ENV) -e THREAD_ROUNDS=$(THREAD_ROUNDS) -e THREAD_HANG_TIMEOUT -e GITHUB_TOKEN -e GITHUB_STEP_SUMMARY c2pa-memray-$(PERF_ENV) python -m tests.perf.run_thread_profile $(SCENARIO_ARG) $(PERF_ARGS) + +.PHONY: clean-threading-reports +clean-threading-reports: + rm -f tests/perf/reports/*-threads.log + @echo "Cleared tests/perf/reports/*-threads.log" diff --git a/docs/native-resources-management.md b/docs/native-resources-management.md index 1cf057f0..d3723948 100644 --- a/docs/native-resources-management.md +++ b/docs/native-resources-management.md @@ -1,162 +1,72 @@ # Native resource management -`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. +`ManagedResource` is the internal base class the C2PA Python SDK uses to wrap native (Rust/C FFI) pointers. `Reader`, `Builder`, `Signer`, `Context`, and `Settings` all subclass it. -## Why ManagedResource? +A `Reader`, for example, holds a pointer to memory the native library allocated. Python's garbage collector tracks the `Reader` object, but has no visibility into that native memory, so it can never free it. `ManagedResource` closes that gap: it frees the native pointer once, however the object stops being used. -`ManagedResource` is the internal base class responsible for managing native pointers owned by the C2PA Python SDK. It guarantees: +## Vocabulary -- Native memory is freed exactly once (no double-free). -- Resources are cleaned up deterministically via context managers or explicit `close()`. -- Ownership transfers (e.g. signer to context) are handled so the same pointer is not freed twice (and the objects/classes know which one owns what). -- Cleanup never raises (trade-off to avoid raising errors on clean-up only, but errors are logged). +A **native pointer** is an address that says where a piece of memory lives. The C2PA SDK wraps a Rust library, the "native" library, which allocates (native) memory Python cannot see. -A new wrapper around a native resource inherits from `ManagedResource` and follows the documented lifecycle rules. +A **handle** is a native pointer a `ManagedResource` object holds at a time, stored in its `_handle` attribute. -## Definitions +**Ownership** answers one question: who must free native memory (the handle), exactly one time. Freeing it zero times leaks memory. Freeing it twice corrupts the allocator and can crash the process. -A **native pointer** is an address that says where a piece of memory lives. The C2PA SDK is a Python wrapper around a Rust library (the "native" library), and that library allocates memory Python cannot see. When the SDK creates a `Reader`, `Builder`, `Signer`, `Context`, or `Settings`, the Python object holds a native pointer to memory that the native library allocated and manages. Python's garbage collector tracks the Python object but knows nothing about the native memory behind the pointer, so it cannot free it. +A pointer is **consumed** when a native call takes ownership of it, often returning a replacement pointer in its place (updating the handle). Once consumed, Python must never free the original. -The **native side** of the Rust library is reached through its C FFI. +## Garbage collection -A **handle** is a single native pointer a `ManagedResource` object holds and manages, stored in its `_handle` attribute. Each object owns one handle at a time. The lifecycle machinery is mostly about tracking that one handle: creating it, swapping it, and freeing it. +Python's garbage collector works by reference counting: each object counts how many references point to it, and reaching zero frees the object. This works for pure Python objects, but a `Reader`'s native pointer sits outside that system. The collector sees the `Reader` wrapper and tracks references to it, but does not know the `_handle` attribute points at memory of its own (allocated by the native library), and the garbage collector never calls the native free function. -**Ownership** answers one question: who is responsible for freeing a piece of native memory. Native memory has to be freed (exactly once). The owner is whoever must free it. If nobody frees it, the memory leaks. If two owners each free it, the same memory is freed twice, which corrupts the allocator and can crash the process. So exactly one side owns each pointer at any moment, and that side frees it. +### About the finalizer hook `__del__` -**Taking ownership** or **transferring ownership** means that responsibility moves from one holder to another. When Python hands a pointer to the native side and the native side takes ownership, the old holder must stop trying to free it, or the pointer gets freed twice. The [Ownership transfer](#ownership-transfer) section covers how the Python SDK handles this. +`__del__`, Python's finalizer hook, could free the native pointer whenever an object is collected, and `ManagedResource` uses it too. But the timing is unpredictable: garbage collection is non-deterministic, an object caught in a reference cycle waits for a separate cycle collector to run, and during interpreter shutdown Python may collect objects in any order, so a `__del__` that reads global state can find it already gone. Every class that holds a native pointer should inherit from `ManagedResource` rather than rely on `__del__` alone. -A pointer is **consumed** when a native call takes ownership of the pointer passed to it, often returning a replacement pointer. Once a pointer is consumed, Python must not free it again, since the native side now owns it. +## Releasing memory -## Why is native resources management needed? +`ManagedResource` gives every object three ways to release its native pointer: a `with` statement, an explicit `close()`, or, as a fallback, the destructor. -### Native pointers in a Python wrapper +### `with` statement -The C2PA Python SDK is a wrapper around a native Rust library that exposes a C FFI. When the SDK creates a `Reader`, `Builder`, `Signer`, `Context`, or `Settings` object, that object holds a **pointer** to memory allocated on the native side (by the native library). - -### How Python's garbage collector works - -Python manages its own objects' memory automatically through garbage collection. In CPython (the standard interpreter), this works primarily through reference counting: each object has a counter tracking how many references point to it, and when that counter reaches zero the object is deallocated. A secondary cycle-detecting collector handles the case where objects reference each other in a loop and their counts never reach zero on their own. - -### Why garbage collection is not enough for native memory - -This system works well for pure Python objects, but native memory sits outside of it entirely. The garbage collector sees the Python wrapper object (e.g. a `Reader` instance) and tracks references to it, but it has no visibility into the native memory that the wrapper's `_handle` attribute points to. Memory allocated by native libraries is invisible to the garbage collector: it does not know the size of that native allocation, cannot tell when it is no longer needed, and will not call the native library's `c2pa_free` function to release it. If the Python wrapper of those native resources is collected without first calling `c2pa_free`, the native memory is never released and leaks. - -### Why __del__ is not reliable enough - -Python does offer `__del__` as a hook that runs when an object is collected (finalizer), and `ManagedResource` uses it as a fallback to possibly clean up leftover resources at that point. But `__del__` cannot be relied on as the primary cleanup mechanism: its timing is unpredictable (due to being called when the garbage collection runs, which is non-deterministic itself), it may not run at all during interpreter shutdown, and other Python implementations (PyPy, GraalPy) that do not use reference counting make its behavior even less deterministic. - -In CPython, `__del__` runs synchronously when the last reference to an object disappears, which in simple cases happens at a predictable point (e.g. when a local variable goes out of scope). But if the object is part of a reference cycle, its reference count never reaches zero on its own. The cycle collector must discover and break the cycle first, and it runs periodically rather than immediately. An object caught in a cycle might sit in memory for an arbitrary amount of time before `__del__` fires. CPython's cycle collector does not guarantee an order when finalizing groups of objects in a cycle, so `__del__` methods that depend on other objects in the same cycle may find those objects already partially torn down. During interpreter shutdown, the situation is even less reliable: CPython clears module globals and may collect objects in an arbitrary order, and `__del__` methods that reference global state (like the `_lib` handle to the native library) can fail silently because those globals have already been set to `None`. PyPy and GraalPy use tracing garbage collectors (which periodically walk the object graph to find unreachable objects, rather than tracking individual reference counts) instead of reference counting, so `__del__` does not run when the last reference disappears. It runs at some later point when the GC happens to trace that region of the heap, which could be seconds or minutes later, or not at all if the process exits first. - -`ManagedResource` is the internal base class that handles managed resources, especially their lifecycle and clean-up. Every class that holds a native pointer should inherit from it. - -## Class hierarchy - -```mermaid -classDiagram - class ManagedResource { - <> - } - - class ContextProvider { - <> - } - - ManagedResource <|-- Settings - ManagedResource <|-- Context - ManagedResource <|-- Reader - ManagedResource <|-- Builder - ManagedResource <|-- Signer - - ContextProvider <|-- Context +```python +with Reader("image.jpg") as reader: + print(reader.json()) +# reader is automatically closed here. ``` -Notes: - -- `Context` inherits from both `ManagedResource` and `ContextProvider` (Python supports multiple inheritance). -- `Settings` inherits from `ManagedResource` only. -- `ContextProvider` is an ABC (abstract base class) that requires two properties: `is_valid` and `execution_context`. The `is_valid` implementation lives on `ManagedResource`, so `Context` satisfies that part of the `ContextProvider` contract without duplicating the property. - -> [!NOTE] -> **How `is_valid` resolves across both parents for Context** -> -> Python's MRO (Method Resolution Order) is the order in which Python searches parent classes when looking up a method or property. For `Context(ManagedResource, ContextProvider)`, the MRO is `Context then ManagedResource then ContextProvider then ABC then object (base class)`. When `context.is_valid` is accessed, Python walks the MRO left-to-right and finds `ManagedResource.is_valid` first. Since `ContextProvider.is_valid` is abstract (it declares the requirement but has no implementation), `ManagedResource`'s concrete version both provides the behavior and satisfies the ABC contract. -> -> The MRO is computed using C3 linearization, which enforces two rules: children appear before their parents, and left-to-right order from the class definition is preserved. For `class Context(ManagedResource, ContextProvider)`: -> -> 1. `Context`: the class itself always comes first. -> 2. `ManagedResource`: first listed parent, nothing else requires it to appear later. -> 3. `ContextProvider`: second listed parent, must come after `ManagedResource` to preserve declaration order. -> 4. `ABC`: parent of `ContextProvider`, must come after its child. -> 5. `object`: root of everything (all objects), always last. -> -> Putting `ManagedResource` first in the declaration matters: the concrete `is_valid` implementation is found immediately during lookup, rather than hitting the abstract declaration on `ContextProvider` first. - -## Python frees only what Python owns - -The C FFI is consistent about one thing that shapes this whole layer: some calls consume the pointer passed to them and hand back a replacement, because the native side may free and reallocate the underlying value. A pointer that went into a consuming call must never be freed by Python afterwards. Its address may already have been reallocated to a different object (address space is not infinite, so addresses get reused). +On exit, `__exit__` calls `close()`, freeing the native pointer even if the block raised. -Python owns and frees two kinds of things: the **single current native handle** for each object, and **Python-side resources it created itself** (stream wrappers, callbacks pinned so the native side can call back into them, caches). It swaps that one tracked handle to whatever a consuming call returns and, on the success path, never frees the value the call took. Beyond those owned resources it also carries bookkeeping it never frees (lifecycle state, the owning process ID, a borrowed reference to a caller-supplied `Context`), and it does not manage native reallocation itself: it swaps handles and, on the ambiguous failure paths, reads the native error tags to decide who still owns the pointer rather than assuming. - -Therefore, the managed resources have the following principles: - -- Each `ManagedResource` holds exactly one `_handle`. `_swap_handle()` replaces it with the pointer a consuming call returned and does not free the old value, since the native side took it (see [Consume-and-swap](#consume-and-swap)). -- `_teardown(free_handle=False)`, `_consume_no_replacement()`, and `_consume_into()` all close or advance the object without calling `c2pa_free`, because ownership moved to the native side. -- Only a few sites free a live handle. Two of them free a pointer this layer still provably owns: normal teardown (`_teardown(free_handle=True)`), and the create-then-validate path, which frees a freshly created pointer if activation fails. The third, `_release_handle()`, is a *guarded* free used only when ownership is genuinely unknown (a consuming call failed without setting an error, or a Python exception was raised before the native side reported anything): if the native side already took the pointer, its address is no longer in the registry and `c2pa_free` is a `-1` no-op, so the free touches no memory. No path frees a pointer known to have been consumed and reallocated (see [Why an ownership-taken failure does not free](#why-an-ownership-taken-failure-does-not-free)). -- `_release()` drops stream wrappers, callbacks, and caches before the native pointer is freed (see [Subclass-specific cleanup with `_release()`](#subclass-specific-cleanup)). - -### Double-free risk mitigations - -Three distinct risks. Two have a mechanism in this layer; the third is the caller's to synchronize: - -| Hazard | Covered by | How | -| --- | --- | --- | -| Freeing a pointer a consuming call already took (single flow) | `_swap_handle` / `_teardown(free_handle=False)` triage | The consumed pointer is abandoned, never freed. The retained-vs-consumed decision reads the native error tag (`UntrackedPointer:` / `WrongPointerType:` mean not taken). | -| A forked child freeing a pointer its parent owns | PID stamp (`record_owner_pid` / `is_foreign_process`) | Cleanup in a process that did not allocate the pointer nulls the handle and marks `CLOSED` without freeing (see [Fork safety](#fork-safety)). | -| Two **threads** in one process racing frees on distinct objects, where the allocator recycles a just-freed address | Not covered here | `ManagedResource` has no lock and no thread stamping. The PID stamp cannot see it: sibling threads share a PID. Safety for genuinely shared handles must come from the caller's own synchronization or from the native registry, not this layer. | - -The PID stamp is fork-only: it compares process IDs, and two threads in the same process always match. Sharing one `ManagedResource` instance across threads without external synchronization is outside what this layer protects against. - -## Guarantees provided by ManagedResource - -`ManagedResource` provides the following guarantees, invariants must be maintained when subclassing the `ManagedResource` class in new implementation/new native resources handlers: - -| Guarantee | Description | -| --- | --- | -| **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 (ordinary errors)** | The cleanup path catches and logs `Exception`, never re-raising it. `_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. **Asynchronous interrupts are the deliberate exception.** The cleanup handlers catch `Exception`, which excludes the `BaseException` signals the interpreter raises to unwind a process (a cancellation request or an exit in progress). Those propagate through cleanup untouched, and the remaining free may not run. Such a signal means the process is being torn down and its address space, native allocations included, is about to be reclaimed as a whole. Catching it would suppress a shutdown the caller asked for in order to complete a free that is about to become irrelevant, so the handlers stay scoped to `Exception`. | -| **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 `_teardown()` 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 `_teardown(free_handle=False)`), 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. | +### Explicit close -## Preventing garbage collection of live references +```python +reader = Reader("image.jpg") +try: + print(reader.json()) +finally: + reader.close() +``` -When a Python object passes a callback or pointer to the native library, that reference must stay alive for as long as the native side might use it. Python's garbage collector has no way to know that native code is still holding a reference to a Python callback. +Calling `close()` directly is equivalent to exiting a `with` block. `close()` is idempotent: a second call does nothing. -The SDK solves this by storing these references as instance attributes on the owning object. For example, `Stream` stores its four callback objects (`_read_cb`, `_seek_cb`, `_write_cb`, `_flush_cb`) as instance attributes. As long as the `Stream` object is alive, its callbacks have a nonzero reference count and will not be collected. Similarly, when a `Signer` is consumed by a `Context`, the Context copies the signer's `_callback_cb` to its own `_signer_callback_cb` attribute so the callback survives even though the Signer object is now closed. +### Destructor -During cleanup, `_release()` sets these attributes to `None`, which drops the reference count on the callback objects and allows them to be collected. In the cleanup sequence, `_release()` runs first, then `c2pa_free` frees the native pointer. `_release()` goes first so that subclass-specific resources (open file handles, stream wrappers) are torn down before the native pointer they depend on is freed. +Without `with` or `.close()`, `__del__` attempts the free when Python garbage-collects the object. When that happens is not predictable. -## How native memory is freed +### Nesting -The native Rust library exposes a single C FFI function, `c2pa_free`, that deallocates memory it previously allocated. `ManagedResource` wraps this in a static method: +Multiple resources can share one `with` statement or nest in separate `with` blocks. They are cleaned up in reverse order: right to left (when sharing one statement), or inner to outer (when statements are nested). ```python -@staticmethod -def _free_native_ptr(ptr): - return _lib.c2pa_free(ptr) +with open("photo.jpg", "rb") as file, Reader("image/jpeg", file) as reader: + manifest = reader.json() +# reader is closed first, then file ``` -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. - -It returns `c2pa_free`'s status code: `0` when the pointer was really freed, `-1` when the native registry rejected an already-consumed or untracked address. That `-1` is expected on the guarded-free paths and is handled gracefully by the native lib too. - -`ManagedResource` guarantees that `c2pa_free` is called exactly once per pointer: not zero times (leak), not twice (double-free). +`with` guarantees a release order: whatever is listed later, or nested deeper, is torn down first. ## Lifecycle states -Each `ManagedResource` tracks its state with a `LifecycleState` enum: +Every `ManagedResource` has 3 states: ```mermaid stateDiagram-v2 @@ -164,351 +74,260 @@ stateDiagram-v2 [*] --> UNINITIALIZED : __init__() UNINITIALIZED --> ACTIVE : _activate(handle) UNINITIALIZED --> CLOSED : close() before activation - ACTIVE --> ACTIVE : _swap_handle(new_handle) - ACTIVE --> CLOSED : close() / __exit__ / __del__ / _teardown() + ACTIVE --> CLOSED : close() / __exit__ / __del__ + CLOSED --> [*] ``` -- `UNINITIALIZED`: The Python object exists but the native pointer has not been set yet. This is a transient state during construction. -- `ACTIVE`: The native pointer is valid. The object can be used. -- `CLOSED`: The native pointer has been freed (or ownership was transferred). Any further use raises `C2paError`. - -`CLOSED` is a one-way state: once closed, an object cannot be reactivated. It is normally reached from `ACTIVE`, but a construction that fails before `_activate()` closes straight from `UNINITIALIZED` when `close()` or `__del__` runs (nothing to free, just marked closed). - -Each transition has one method that performs it, and subclasses must go through them rather than assigning `_handle` or `_lifecycle_state` directly: +- `UNINITIALIZED`: the (Python) object exists but has no native pointer (handle) yet. This is transient, lasting only for the duration of construction. +- `ACTIVE`: the native pointer is valid, and the object can be used. +- `CLOSED`: the native pointer has been freed, or ownership of it has moved elsewhere. Any further use raises `C2paError`. -| 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. | -| `_teardown(free_handle=False)` | 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 (guarded, via `_teardown(free_handle=True)`) and closes the object. Same post-state as the consumed teardown. | - -Because activation is the only way in, no code path can leave an object ACTIVE while holding a null handle. - -Two terms recur throughout this document. An **owned free** calls `c2pa_free` on a pointer this layer still provably holds: the normal `close()` / `__del__` path and the create-then-validate failure path both do this. A **guarded free** is the same call made when ownership is uncertain, which is what `_release_handle()` does: the native pointer registry tolerates being asked to free an address it no longer tracks, returning `-1` instead of crashing, so the free does not double-free a pointer the native side already took. That tolerance makes a guarded free safe to *issue*, but it is not free of consequence under concurrency — on a branch where the value is already known to be consumed, the layer skips the free rather than relying on the `-1`, because a stale free can race a recycled address (see [Why an ownership-taken failure does not free](#why-an-ownership-taken-failure-does-not-free)). - -`_teardown(free_handle)` is the one method that performs the ACTIVE to CLOSED transition, and the boolean decides the only thing that varies between the two exit paths: whether the native pointer is freed. Both paths run `_release()`, set `CLOSED`, and null the handle. - -| `free_handle` | When | What it does with the pointer | -| --- | --- | --- | -| `True` | Either the pointer is still provably ours (normal `close()`, `__del__`) — an owned free — or ownership is unknown after a failure (`_release_handle()`) — a guarded free. | Calls `c2pa_free`. On the owned paths the pointer is really freed; on the unknown-ownership path the registry returns `-1` without touching memory if the native side already took it. | -| `False` | The native side already took ownership: a consuming FFI call swallowed the pointer, or it passed to another object. | Frees nothing; the new owner does. A `c2pa_free` here would double-free (or hit the guarded `-1` no-op that dirties the error slot and risks racing a recycled address). | +Once `CLOSED`, an object never becomes `ACTIVE` again. A construction that fails before activation can also close directly from `UNINITIALIZED`, since there is nothing to free, only a state to record. -Every public method calls `_ensure_valid_state()` before doing any work, which raises `C2paError` unless the resource is ACTIVE with a non-null handle. +## Closing during a (native) call -## Ways to clean up +A native call checks the object is usable, then hands the pointer to native code. Two threads sharing one object can interleave those steps: -You can clean up using a `with` statement, explicitly by calling `close()`, or by using destructor fallback. +1. Thread A calls `reader.json()`, checks the Reader is usable, enters the native call. +2. Thread B calls `reader.close()` while that call is still running, freeing the pointer. +3. Thread A's native code reads through the now-freed pointer. Crash. -### Using a "with" statement - -The recommended cleanup method is to use a `with` statement, like this: +A thread pool with a timeout can hit this too: ```python with Reader("image.jpg") as reader: - print(reader.json()) -# reader is automatically closed here, even if an exception occurs + future = thread_pool.submit(reader.json) + try: + future.result(timeout=2.0) + except TimeoutError: + pass +# the `with` block exits here, calling reader.close(), +# whether or not the pool thread's reader.json() finished ``` -When the `with` block exits, `__exit__` calls `close()`, which frees the native pointer. This is the safest approach because cleanup happens even if the code inside the block raises an exception. - -### Explicit close - -```python -reader = Reader("image.jpg") -try: - print(reader.json()) -finally: - reader.close() -``` +`future.result(timeout=...)` gives up waiting but does not stop the pool thread, so `reader.json()` can still be mid-call when the `with` block's `close()` runs on the timeout path. Same race as thread A and B, reached through a timeout instead of a hand-rolled thread. -Calling `close()` directly is equivalent to exiting a `with` block. It is idempotent: calling it multiple times is safe and does nothing after the first call. +A Python object has no owning thread. Any reference holder can call any method on it at any time, from any thread. -### Destructor fallback - -If neither the context manager nor an explicit `.close()` is used, `__del__` attempts to free the native pointer when Python garbage-collects the object. Per [Why `__del__` is not reliable enough](#why-__del__-is-not-reliable-enough), its timing is unpredictable and it may not run at all, so it is a safety net rather than a primary cleanup mechanism. - -## Error handling during cleanup - -Cleanup must not raise an *ordinary* 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 `Exception`. -- `_release()` is never called directly during cleanup. It runs inside `_safe_release()`, which logs any `Exception` with a traceback and returns normally, so a subclass whose `_release()` raises an ordinary error 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. - -These handlers catch `Exception`, not `BaseException`. The signals the interpreter raises to unwind a process (a cancellation request, or an exit already in progress) are `BaseException`, so they pass through cleanup untouched and the remaining free may not run. That is intentional: the signal means the whole process is going away, and its address space, native allocations included, is reclaimed on exit. Holding the interpreter in cleanup to finish a free that is about to become irrelevant would only delay the shutdown the caller asked for. - -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 interpreter switches threads between bytecode instructions, and a native call spans many of them, so thread B's `close()` can land at any point during thread A's call. `ManagedResource` guards against that. -The `foreign process` branch is explained under [Fork safety](#fork-safety). +Depending on what the allocator did with the freed memory, this crashes the process or returns another object's bytes, corrupting state far from the code responsible. -## Nesting resources +Any two threads sharing a `Reader`, `Builder`, `Signer`, or `Context` reference can hit this, so [Lifecycle states](#lifecycle-states) alone is not the whole model. A resource needs a way to say "a call is using me, don't free me out from under it," which in-flight tracking adds next. -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). +## In-flight calls -```python -with open("photo.jpg", "rb") as file, Reader("image/jpeg", file) as reader: - manifest = reader.json() -# reader is closed first, then file -``` +Alongside its lifecycle state, every `ManagedResource` counts calls currently running against it. This is separate from the `ACTIVE`/`CLOSED` state: a resource can be `ACTIVE` and idle, or `ACTIVE` with one or more calls in flight. -The same can be written with nested blocks if readability is better: +A call in flight can be either: -```python -with open("photo.jpg", "rb") as file: - with Reader("image/jpeg", file) as reader: - manifest = reader.json() -``` +- **Shared**: several threads can run this kind of call on the same object at once. Reading a manifest with `.json()` is shared: nothing stops two threads from reading the same data at the same time. +- **Mutating**: only one thread may run this kind of call at a time, and no shared call may start while it runs, because a concurrent read could see a half-updated result or a pointer being replaced out from under it. -The order matters because resources often depend on each other. In both examples, the `Reader` holds a native pointer that references the file's data through a `Stream` wrapper. If the file handle were closed first, the native library would still hold a pointer into the stream's read callbacks, and any subsequent access (including cleanup) could read freed memory or trigger a segfault. By closing the Reader first, the native pointer is freed while the underlying file is still open and valid. Python's `with` statement guarantees this ordering: resources listed later (or nested deeper) are torn down first. +A `close()` arriving while any call, shared or mutating, is in flight does not free the pointer immediately. It marks the resource so no new caller can start using it, and defers the free until all in-flight calls have returned. For instance, thread B's `close()` takes effect at once from its own point of view, but the memory thread A is reading stays valid until thread A's call returns. -## Reader lifecycle +## Lifecycle overview -A `Reader` wraps a stream (or opens a file), passes it to the native library, and holds the returned pointer. While active, callers can use `.json()`, `.detailed_json()`, `.resource_to_stream()`, and other methods. Each of these checks state via `_ensure_valid_state()` before making the FFI call. +Lifecycle state and in-flight calls work together to manage a native resource. A resource is always in one lifecycle state, `UNINITIALIZED`, `ACTIVE`, or `CLOSED`. Independently, while it is `ACTIVE`, zero or more calls may be in flight on it right now, and if any of them is mutating, no other call may start. Together, this is the full picture every guard in `ManagedResource` checks before letting a call through: ```mermaid stateDiagram-v2 - direction LR - [*] --> UNINITIALIZED : __init__() - UNINITIALIZED --> ACTIVE : Reader("image.jpg") - ACTIVE --> CLOSED : close() / exit with block + [*] --> UNINITIALIZED + UNINITIALIZED --> ACTIVE: _activate(handle) + UNINITIALIZED --> CLOSED: close() before activation + + state ACTIVE { + [*] --> Idle + Idle --> SharedBorrow: a shared call starts + SharedBorrow --> Idle: it returns + Idle --> Mutating: a mutating call starts + Mutating --> Idle: it returns, or a consume-and-swap succeeds + Mutating --> [*]: a consume-and-close succeeds + } + + ACTIVE --> CLOSED: close() / __del__ CLOSED --> [*] ``` -While `ACTIVE`, callers can use `.json()`, `.detailed_json()`, etc. repeatedly without changing state. Calling `.close()` on an already-closed Reader is a no-op. Any other method call on a closed Reader raises `C2paError`. +`Mutating` is entered by any mutating call, but the two ways out differ by what kind of call it was. An ordinary mutating call, or a consume-and-swap, returns to `Idle`: the resource stays `ACTIVE`, either unchanged or holding a new pointer. Only a consume-and-close exits `ACTIVE` into `CLOSED`, since that is the one kind of consuming call that leaves nothing to wrap. Both consuming shapes are covered in [Consuming](#consuming). -When the Reader is closed, it first releases its own resources (open file handles, stream wrappers) via `_release()`, then frees the native pointer via `c2pa_free`. +| State | `is_valid` | What a caller sees | +| --- | --- | --- | +| `UNINITIALIZED` | False | `C2paError`: "not properly initialized" | +| `ACTIVE`, idle | True | normal operation | +| `ACTIVE`, shared call(s) in flight | True | normal operation; more shared calls may join | +| `ACTIVE`, a mutating call in flight | **False** | `C2paError`: "running a mutating operation" | +| `CLOSED` | False | `C2paError`: "is closed" | -## Builder lifecycle +`is_valid` defines if a call would be accepted right now. It is `ACTIVE`, holding a handle, and no mutating call in flight. It is a lock-free snapshot, so a passing check does not keep the handle alive. Only a guarded call does that. -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. +A `Reader`'s read methods, `.json()`, `.detailed_json()`, `.resource_to_stream()`, are shared. A `Builder`'s `.sign()` is mutating, and ends by consuming the Builder: signing closes it, so a `Builder` is single-use (see [Consuming](#consuming)). -```mermaid -stateDiagram-v2 - direction LR - [*] --> UNINITIALIZED : __init__() - UNINITIALIZED --> ACTIVE : Builder.from_json(manifest) - ACTIVE --> CLOSED : .sign() or close() - CLOSED --> [*] +## Crashes - note left of CLOSED - .sign() closes the builder - to enforce single use - end note -``` +Usually, native memory bug terminates the process, and the operating system reports it as a signal, since the failure happens outside anything Python's exception machinery watches. -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. +SIGSEGV, segmentation fault, comes from the hardware: the CPU traps on a read or write to memory the process is not allowed to touch, and the kernel delivers SIGSEGV. This fires when a pointer no longer points at accessible memory, for instance freed memory the allocator has unmapped. Freeing memory does not always unmap it. Allocators often keep the pages and reuse them for a later allocation, so reading through a freed pointer can succeed, returning another object's bytes and corrupting state far from the code responsible. SIGSEGV happens when the pages are gone. -The native sign call borrows the builder's pointer rather than taking ownership of it, so `Builder` never marks it consumed and the pointer is freed normally through `c2pa_free`. The close enforces single use; it is not a memory-management requirement. +SIGABRT, abort, comes from software: a program calls `abort()` on itself after detecting a broken invariant. Allocators do this when `free()` is handed a pointer it never issued, the same pointer twice, or a heap whose bookkeeping a stray write has damaged. -## Ownership transfer +In every case, the process terminates from inside native code. No exception, no `finally` block, no traceback can be run by the Python code. -Some operations transfer a native pointer from one object to another. When this happens, the original object must stop managing the pointer (e.g. so it is not freed twice). +## Locking -`_teardown(free_handle=False)` handles this. It runs `_release()`, then sets `_handle = None` and `_lifecycle_state = CLOSED` without freeing the pointer. +Freeing the same native pointer twice corrupts the allocator's bookkeeping. A detecting allocator stops the process. An allocator that misses it lets the damage surface later, somewhere unrelated to the code responsible. Two threads racing a `close()` against an in-flight call on the same object is one way this happens: without a guard, thread B's `close()` could free the pointer while thread A's call is still reading through it, and if thread A's own cleanup runs afterward, that same pointer gets freed a second time. -In the SDK this happens in one place: passing a `Signer` to a `Context`. The Context runs a short-lived native context builder, feeds the signer into it, builds the context, and activates the result. The builder itself is wrapped in `_NativeBuilder` (a small `ManagedResource`), so every failure inside the `with` block frees it through `close()` unless a consuming call already took it. There is no raw pointer held across the calls and no bespoke error handler. +Each `ManagedResource` holds a reentrant lock, `_op_lock`, and the in-flight counters from [Lifecycle overview](#lifecycle-overview). Together they guard against that: a mutating call excludes every other call, and a `close()` arriving mid-call is deferred rather than applied immediately. -The signer transfer goes through `_consume_no_replacement()`, which routes any failure to the shared triage (see [Why an ownership-taken failure does not free](#why-an-ownership-taken-failure-does-not-free)). That triage decides per error whether the signer was actually consumed: `set_signer` does *not* unconditionally take ownership. +`_op_lock` is a `threading.RLock`: a finalizer can run at any point, including inside a method already holding the lock on that thread, and a consuming call tears the handle down from inside a region it already holds the lock in. -```mermaid -sequenceDiagram - participant C as Caller - participant S as Signer - participant X as Context - participant B as _NativeBuilder - participant N as Native lib - - C->>X: Context(settings, signer) - X->>B: with _NativeBuilder() (owns the builder, close() frees it on any failure) - 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 be consumed - X->>S: _consume_no_replacement(set_signer) - S->>N: c2pa_context_builder_set_signer(builder_ptr, handle) - - alt status 0 (success) - S->>S: _teardown(free_handle=False) - Note right of S: Consumed: native took the signer - else pre-consume rejection (UntrackedPointer / WrongPointerType) - Note right of S: Rejected before ownership moved:
Signer retained, typed error raised - else other error - S->>S: _teardown(free_handle=False) - Note right of S: Native took it then failed and dropped it - end +The lock is never held across a native call that drives a stream callback, since that callback can call back into this API on the same thread. Those calls increment an in-flight counter, release the lock, run the native call, then decrement the counter, the mechanism the [state diagram](#lifecycle-overview) calls "a call in flight." - X->>B: _consume_into(build) - B->>N: c2pa_context_builder_build(builder_ptr) - N-->>X: context_ptr (builder consumed) - X->>X: _activate(context_ptr) (outside the with) -``` +The GIL does not make this safe on its own: a foreign function call through ctypes releases it for the call's duration, so another thread runs while native code runs, and a `close()` can land inside that window. Free-threaded Python builds do not change this either, since `ctypes` runs with no GIL protection whether or not the GIL exists elsewhere in the process; a native call already ran with the GIL released. -Details in that sequence that are easy to get wrong: +The native library's own pointer bookkeeping guards the pointer, not what Python does with it. A callback belongs to Python, so the native library tracks nothing for it: if Python frees the callback's object while the call runs, the next invocation reaches memory Python gave up. `_op_lock` and the in-flight counters keep it alive for the call. -- The callback is copied to the Context *before* the transfer. A successful consume runs `_release()`, which drops the Signer's reference to the callback; a Context that copied it afterwards would be pointing at a callback nothing keeps alive. -- `set_signer` does not always take the pointer. A pre-consume rejection (`UntrackedPointer:` / `WrongPointerType:`) leaves the Signer `ACTIVE` and retained, so the triage must read the native error before deciding to close it. Treating every failure as "consumed" would close a signer the native side never took. -- A `ctypes.ArgumentError` from `set_signer` is re-raised untouched by `_invoke_consume`: marshalling failed, the native function never ran, and the Signer still owns its handle. Only calls that reached native go through the consumed/retained triage. -- The builder is never held as a raw local across the signer and build calls. `_NativeBuilder`'s `with` block owns it: a settings error, a retained-signer error, a build rejection, or an async interrupt all free it through `close()`, and a successful build consumes it so `close()` is then a no-op. The old raw-pointer recovery block that used to free `builder_ptr` on the un-reached-build path is gone. +Using a handle takes two steps: check it, then act on it. Native bookkeeping guards only the second. A second thread can free the same address, or have it reissued to another object, in the gap between them. Holding `_op_lock` across both steps closes that gap. -### Adopting a handle the SDK already owns +### Lock ordering -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. +Two threads acquiring the same lock pair in opposite orders can deadlock. This SDK fixes the order, outermost to innermost: a method-specific lock, then the lock of the object the method is called on, then the lock of any object it borrows. A lock held across a native call must be one no callback path acquires. -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. +### Borrowing vs consuming -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. +A shared call, a **borrow**, passes the handle to native and gets it back unchanged, validated once on entry and never rechecked. A consuming call hands ownership to native, which frees the original pointer, so a consume starting midway through a borrow would free memory the borrow is reading. -## Consume-and-swap +`ManagedResource` refuses to start a consume while a borrow is in flight, and reserves the handle as a mutating call for the whole duration of the consume. Every entry point checks the in-flight counters before starting, so a racing call is refused with "running a mutating operation" instead. `Reader.with_fragment()` also serializes against other calls to itself with a lock of its own. -`_teardown(free_handle=False)` 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. +## Consuming -`Builder.with_archive()` follows the same pattern: it loads an archive into an existing Builder, replacing the manifest definition while preserving the Builder's context and settings. +Consuming a handle hands the pointer to native, which takes ownership and either returns a replacement pointer or frees the original. Python never frees a pointer handed to a consuming call, whichever way the call ends; freeing it would double-free, so the consumed pointer is abandoned instead. -In both cases the FFI call consumes the current pointer and returns a replacement: +Two shapes: -```mermaid -stateDiagram-v2 - state "ACTIVE (ptr A)" as A - state "ACTIVE (ptr B)" as B - - A --> B : C FFI call consumes ptr A, returns ptr B - note right of B - Same Python object, - new native pointer - end note -``` +**Consume-and-swap** replaces the object's internal state without discarding the wrapper. `Reader.with_fragment()` feeds a new BMFF fragment into an existing Reader so native can rebuild its representation from prior fragments plus the new one. `Builder.with_archive()` loads an archive into an existing Builder the same way, keeping its context and settings. On success the object stays `ACTIVE`, only the pointer underneath changes. -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 a consumed teardown (`_teardown(free_handle=False)`), 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 in [`_consume_and_swap()`](#_consume_and_swap). +**Consume-and-close** takes the pointer and leaves nothing to wrap. Signing a `Builder`, or handing a `Signer` to a `Context`, both end this way: the object goes `CLOSED` without freeing the pointer, since native still owns it. -### `_consume_and_swap()` +### Example: signing a Builder -Every call of this shape goes through one helper, which takes the FFI call as a callable and handles the outcomes: +`Builder.sign()` is a consume-and-close call: ```python -# Reader.with_fragment() internally does: -self._consume_and_swap( - lambda handle: _lib.c2pa_reader_with_fragment(handle, format_bytes, stream), - Reader._ERROR_MESSAGES['reader_error']) +builder = Builder(manifest_json) +builder.sign(signer, "image/jpeg", source, dest) +# builder is now CLOSED: sign() consumed its handle ``` -The call is passed as a lambda because the helper supplies the handle and, on success, replaces it via `_swap_handle()`. +`sign()` hands the handle to `c2pa_builder_sign`, which takes ownership and writes the signed asset. No replacement pointer, so `ManagedResource` marks the Builder `CLOSED` without freeing anything: native already owns it. This is why a `Builder` is single-use; calling `sign()` again raises `C2paError`. -The helper exists because a failed return can be ambiguous. The native functions run in phases: it validates the **borrowed pointer** (passed in without transferring ownership; the caller still owns it unless the callee explicitly takes it over), then takes ownership, then does the work. A failure in the first phase and a failure after the second come back to Python as the same value (a null pointer, or a non-zero status), but they leave ownership in opposite places. +Each call below is a real `ManagedResource` method, in the order `sign()` calls them: ```mermaid -flowchart TD - CALL["FFI call(handle)"] --> V{"validate borrowed handle"} - V -->|invalid| R["reject: handle NOT taken
sets UntrackedPointer / WrongPointerType"] --> F1["returns a failure value
(null, or non-zero status)"] - V -->|valid| TAKE["take ownership of handle"] - TAKE --> WORK{"execute function logic"} - WORK -->|fails| DROP["native drops the value itself
sets some other error"] --> F2["returns a failure value
(null, or non-zero status)"] - WORK -->|succeeds| OK["returns replacement / 0 / new pointer"] - - F1 -.failure value returned to Python.- AMB(["needs to consult error to know failure mode from Python"]) - F2 -.failure value returned to Python.- AMB +sequenceDiagram + participant C as Caller + participant B as Builder + + C->>B: sign(signer, format, source, dest) + B->>B: _ensure_valid_state() + Note right of B: Raises C2paError if not ACTIVE.
Builder is still ACTIVE here. + + B->>B: _exclusive_native_call() + Note right of B: Reserves the handle as a mutating call.
Builder is ACTIVE, mutating. + + B->>B: c2pa_builder_sign(handle, ...) + Note right of B: Native call runs.
On return, native owns the handle
whether this succeeded or raised. + + alt native call raised + B->>B: close() + Note right of B: Builder is now CLOSED. + B-->>C: re-raises as C2paError + else native call returned + B->>B: close() + Note right of B: Builder is now CLOSED,
unconditionally, in a finally block. + B-->>C: returns manifest bytes + end ``` -The two failure paths are indistinguishable from the return value alone. Only the native error message set alongside them tells the phases apart: - -| 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 | Taken, then the operation failed | `_teardown(free_handle=False)`: 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 | `_release_handle()` guarded free, the caller's message is raised with `"Unknown error"` filled in. | - -This error and ownership triage relies on the native error still being readable (and correctly being the last error encountered) 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. +`close()` runs either way, so the Builder is always `CLOSED` once `sign()` returns or raises. `close()` never frees the pointer here: native already took it in `_exclusive_native_call()`'s reservation. -Three consume helpers share this triage; they differ only in what the FFI call returns on success: +### Adopting a handle -| Helper | Success return | Success action | -| --- | --- | --- | -| `_consume_and_swap()` | a replacement pointer | `_swap_handle()`, resource stays `ACTIVE` | -| `_consume_no_replacement()` | a status code (`0` = ok) | `_teardown(free_handle=False)`, resource `CLOSED` | -| `_consume_into()` | a *different* object's pointer | `_teardown(free_handle=False)`, the pointer returned for the caller to own | +A native call can return a pointer needing a Python wrapper with no `__init__` call, since `__init__` would create a new resource rather than wrap an existing one. `_wrap_native_handle()` builds a bare instance, sets lifecycle bookkeeping, runs `_init_attrs()`, and activates the handle. Ownership transfers once that call returns; if it raises, the caller still owns the pointer and must free it. -`_consume_no_replacement()` is how a `Signer` is fed to a `Context` (`set_signer` returns a status code); `_consume_into()` is how that same `Context` build returns the new context pointer. A failure in any of the three is handled by the same native-error triage, so a pre-consume rejection retains the handle rather than assuming it was taken. +`Reader._init_from_context` and `Builder._init_from_context` activate a native object before the consuming call that feeds it data. Activating first puts the intermediate pointer under normal cleanup right away, so `close()` and `__del__` free it correctly whichever way the consuming call goes. A raw pointer in a local variable would leave failure paths to decide that themselves. -#### Why an ownership-taken failure does not free +## Object usability checks -A consuming FFI call can fail. It may reject the borrowed pointer before taking it, or it may take ownership first and then, on a later failure, drop the value itself. +`is_valid`, defined in [Lifecycle overview](#lifecycle-overview), is a lock-free snapshot that does not keep the handle alive. `Context` implements the abstract `ContextProvider.is_valid` by inheriting the concrete one from `ManagedResource`, found first by method resolution order as long as `ManagedResource` is listed before `ContextProvider` (`class Context(ManagedResource, ContextProvider)`). Reversed, the abstract declaration comes first and raises `TypeError` at class definition time. -The native error message indicates which of the errors happened. A rejection carries one of the `_PRE_CONSUME_ERROR_TAGS` (`UntrackedPointer:` or `WrongPointerType:`), which means the handle was never taken and is retained. Any other error message means the native side may have taken ownership and already dropped the value. On top of those, preparing the call's own arguments can fail in Python before the native function ever runs (for example, encoding a bad value or a ctypes marshalling error other than `ArgumentError`), and that outcome is handled separately. +Every subclass gets these guarantees from `ManagedResource`, and must not break them: -The two settled branches each take the exact action their ownership implies. A pre-consume rejection (an error prefixed `UntrackedPointer:` or `WrongPointerType:`) means the handle is still the caller's, so it is retained and freed later by normal cleanup. Any other native error means the value is already gone, so `_teardown(free_handle=False)` runs the Python-side cleanup without freeing anything. +| Guarantee | What it means | +| --- | --- | +| Freed exactly once | Every native pointer reaches `c2pa_free` at most once: no leak, no double-free. | +| Cleanup is idempotent | Calling `close()`, or exiting a `with` block, more than once does nothing after the first time. | +| Cleanup never raises | An ordinary exception during cleanup is caught and logged, never re-raised, so it cannot mask an exception the `with` block itself raised. An interpreter shutdown signal is the one exception to this: it is allowed to interrupt cleanup, since the process is already going away. | +| State transitions are one-way | Lifecycle only ever moves `UNINITIALIZED` to `ACTIVE` to `CLOSED`. Nothing reactivates a closed resource. | +| Public methods check state first | Every method that uses the handle validates it before the native call, raising `C2paError` on anything but a usable `ACTIVE` resource rather than risking undefined behavior. | -Always calling the guarded free instead, even where the value is known to be gone, is tempting because a stale free looks like a harmless `-1` no-op. It is only harmless while the freed address stays unclaimed. The native registry rejects an address it no longer tracks, but once another thread allocates a fresh tracked object at that recycled address, the registry does track it again — and a stale free aimed at the old value would now find a live entry and destroy a different thread's object. The scenario is unlikely, but not unreachable: it needs a second thread inside its own FFI call, an allocator that hands back the exact address just freed, and that reuse to happen during the (narrow) window between the native drop and this free. But the window is real under concurrent use. The failure is a silent cross-thread corruption rather than a clean error, and the free is not needed in the first place on this branch. So where the value is known to be consumed, the free is skipped rather than issued and left to the registry to reject. The native error slot stays sticky: it holds whatever it last held until the next error overwrites it, and nothing clears it in between. Issuing an unneeded free would set an untracked-pointer error there that a later caller could mistake for the failure it actually asked about, so skipping the free keeps the slot free for the next real error. +## Keeping references alive -`_release_handle()` (a guarded free) is reserved for the two branches where ownership is not known for certain: a Python exception raised before native reports anything, and a failure that leaves the error slot empty (which no defined native failure is expected to produce). In both, a guarded free is a good default, since it is a real free when the handle is still ours and a `-1` no-op when the native side already took it. +A callback or pointer passed to the native library must stay alive as long as native code might use it, but the garbage collector only sees Python references and has no way to know that. -None of this is protected by a lock on the Python side: `ManagedResource` has no thread-safety mechanism of its own, and the retained-vs-consumed guarantee comes entirely from the native pointer registry and its thread-local error slot. As noted under [Which double-free risks this layer guards](#double-free-risk-mitigations), sharing one instance across threads without external synchronization is the caller's responsibility. This is a different hazard from [Fork safety](#fork-safety), which concerns a forked child process, not a thread within the same process. +This SDK keeps such references as plain instance attributes on the owning object. A `Stream` stores its four callback objects this way (see [Reference cycles](#reference-cycles) for how they avoid keeping the `Stream` alive in return). A `Signer` consumed by a `Context` has its callback copied onto the `Context`, so it survives the `Signer` closing. -A consuming C FFI function first removes the pointer from its registry, then reconstructs the owned value from it. `untrack_or_return!` runs ahead of `Box::from_raw` in `c2pa_c_ffi`. If the address is unknown or the wrong type, the untrack step fails before ownership is taken and sets an error whose prefix (`UntrackedPointer:` or `WrongPointerType:`) identifies it as a pre-consume rejection. Once the value has been reconstructed, a later failure simply drops it, the same as any owned value going out of scope. The Python side stays defensive (and as generic as possible) rather than assuming any exact behavior: it retains the handle when it recognizes one of those rejection prefixes, and where the outcome is unclear it falls back to the guarded free. A native side that behaved differently would degrade in one of two bounded ways: If it kept a pointer the Python side treated as consumed, nothing would free that pointer and it would leak. If it had already released a pointer the Python side then tried to free, the registry would not find the address and the free would return `-1` without touching memory. +`_release()` nulls these attributes during cleanup, before the native pointer is freed, so anything the pointer depends on is torn down first. `Stream` releases in the reverse order, covered in [`Stream` cleanup](#stream-cleanup). -### Adopting the handle before giving it away +## Freeing memory -`Reader._init_from_context` and `Builder._init_from_context` both create a native object, immediately activate it, and only then make the consuming call. `_create_and_activate()` handles the create-then-activate half: it calls the FFI constructor, validates the result with `_check_ffi_operation_result`, and `_activate()`s it, freeing the pointer if either step fails so a rejected creation leaks nothing. Reduced to its shape: +The native library exposes one C function, `c2pa_free`, that deallocates memory it previously allocated. Every native pointer, whatever kind of object created it, is freed through this one path: ```python -self._create_and_activate( - lambda: _lib.c2pa_reader_from_context(context.execution_context), - Reader._ERROR_MESSAGES['reader_error']) - -self._consume_and_swap( - lambda handle: _lib.c2pa_reader_with_stream( - handle, format_bytes, self._own_stream._stream, - ), - Reader._ERROR_MESSAGES['reader_error']) +@staticmethod +def _free_native_ptr(ptr): + return _lib.c2pa_free(ptr) ``` -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. +It returns `0` when the pointer was freed, and `-1` when the registry rejected an already-consumed or untracked address. `ManagedResource` guarantees this is called once per pointer. -## Subclass-specific cleanup +## Cleanup errors -Each subclass can override `_release()` to clean up its own resources before the native pointer is freed. The base implementation does nothing. +Cleanup must not let an ordinary exception mask the exception that caused a `with` block to exit in the first place. `ManagedResource` enforces this at every level: -Examples from the codebase: +- `close()` delegates to `_cleanup_resources()`, which wraps the whole sequence in a try/except that catches and logs `Exception` rather than re-raising it. +- `_release()` runs inside a wrapper that logs any exception it raises and continues, so a subclass's `_release()` failing cannot stop the native pointer from being freed afterward. +- A failed free of the native pointer is logged, not re-raised. +- The object is marked `CLOSED` before `_release()` runs or anything is freed, so a cleanup that fails partway still leaves the object closed, and a second attempt does not repeat the damage. +- `close()` on an already-closed object returns immediately. -| Class | What `_release()` cleans up | -| --- | --- | -| 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) | - -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 +These handlers catch `Exception`, not `BaseException`. An interpreter unwinding signal (cancellation, shutdown) passes through untouched, and the remaining free may not run: the process is going away regardless, so holding cleanup open would only delay the shutdown. -`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. +All three cleanup entry points converge on one method: -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. When a reader is created from a shared context, the native side takes its own reference-counted handle on that context (an `Arc` clone in the Rust library), so the native reader keeps the context alive independently of whether Python still points at it. (That native behavior is not visible from this repo, which ships a prebuilt binary; it is the contract this code is written against.) +```mermaid +flowchart TD + E["close() / __exit__ / __del__"] --> CR["_cleanup_resources()"] + CR --> FP{"foreign process?"} + FP -->|yes| N["null the handle, mark CLOSED,
do not free"] --> DONE([return]) + FP -->|no| ST{"already CLOSED?"} + ST -->|yes| DONE + ST -->|no| TD["run teardown"] + TD --> SET["mark released, CLOSED"] + SET --> REL["release subclass resources
(logs, never raises)"] + REL --> NULL["take the pointer into a local,
null the handle"] + NULL --> H{"pointer was set?"} + H -->|no| DONE + H -->|yes| FREE["free the native pointer
(logs on failure)"] --> DONE +``` ## 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. +`fork()` copies the calling process, including every Python object holding a native pointer, but the underlying native allocation is not duplicated: there is still only one, 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. +A forked child cleaning up its copy normally would double-free a pointer the parent is using. Worse, `fork()` copies only the calling thread: any lock another thread held at fork time comes over still locked, with no thread left to release it. If that lock is one the native library uses internally, freeing anything in the child can then block forever. -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: +So this SDK never frees native memory in a process that did not allocate it. Every object is stamped with its creating process's ID at construction; cleanup compares that stamp against the current process first: ```mermaid sequenceDiagram @@ -516,39 +335,82 @@ sequenceDiagram participant O as Reader object participant F as Forked child - P->>O: __init__ stamps _owner_pid + P->>O: __init__ stamps the owning process ID 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: process ID does not match + Note right of F: Do not free: the parent owns it,
and a native lock may be held
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 the consumed teardown 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 child's copy is nulled and marked `CLOSED` rather than just skipped, so nothing in the child can use or free it. The parent's copy is untouched. -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. +Nothing here leaks for good: `exec()` replaces the child's address space, and exit reclaims its memory. A long-lived forked worker retains at most the objects inherited at fork time, a bounded amount, since anything it allocates itself carries its own process ID and frees 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. +> It is recommended to instantiate objects in the using process. Objects created before `fork()` report closed in the created child, to avoid memory corruption. + +## Class hierarchy + +```mermaid +classDiagram + class ManagedResource { + <> + } -## Why is `Stream` not a `ManagedResource`? + class ContextProvider { + <> + } -`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. + ManagedResource <|-- Settings + ManagedResource <|-- Context + ManagedResource <|-- Reader + ManagedResource <|-- Builder + ManagedResource <|-- Signer -The reason is that ownership runs in the opposite direction. A `Reader` or `Builder` holds a native resource that Python code calls methods on. A `Stream` holds a native handle that the native library calls *back into* (read, seek, write, flush). The native library needs a different release function to tear down the callback machinery. + ContextProvider <|-- Context +``` -`Stream` tracks its own state with `_closed` and `_initialized` flags rather than `LifecycleState`, but it supports the same three cleanup paths: context manager, explicit `.close()`, and `__del__` fallback. +`Context` inherits from both `ManagedResource` and `ContextProvider`. `ContextProvider` requires `is_valid` and `execution_context`; `Context` satisfies `is_valid` by inheriting the concrete version from `ManagedResource`, as long as it is listed first: `class Context(ManagedResource, ContextProvider)`. Reversed, method resolution order finds the abstract declaration first and refuses to instantiate `Context` at all. -## Which method to use when? +## Streams -`_create_and_activate`, `_consume_and_swap`, `_consume_no_replacement`, -`_consume_into`, `_release_handle`, and `_wrap_native_handle` each apply to a -different situation when writing a new subclass: +Bytes reach the native library through a `Stream`, which wraps a Python file or memory stream so the native library can read and write it via callbacks. It does not inherit from `ManagedResource`, and uses a separate release function instead of `_free_native_ptr`. + +### Not a `ManagedResource` + +A `Reader` or `Builder` holds a resource Python calls into. A `Stream` holds one native calls back into, for read, seek, write, flush. That difference is why `Stream` gets its own release path. + +`Stream` tracks its state with two flags, `_closed` and `_initialized`, rather than the `LifecycleState` machinery from [Lifecycle states](#lifecycle-states), but supports the same three cleanup paths: context manager, explicit `.close()`, `__del__` fallback. + +### Reentrant callbacks + +A `Stream` registers four ctypes callbacks. Each runs caller-supplied Python, so the same reentrancy hazard applies: a native call driving one cannot hold a lock across it, since the callback may call back into this API on the same thread. + +Each callback checks the stream's own state before touching the underlying Python object, and reports an error rather than reading through it if the stream has been torn down. + +### `Stream` cleanup + +`Stream` holds its own reentrant lock, `_close_lock`, serializing `close()` and `__del__` against each other, same reason `_op_lock` is reentrant: nulling a callback attribute inside the locked region can drop the last reference to an object whose finalizer needs the same lock. + +`Stream` releases the native handle first, guaranteeing no callback can fire again, then drops the callback objects, the reverse of `ManagedResource`'s order, since here callbacks are invoked by native code rather than the reverse. + +`close()` performs both steps. `__del__` performs only the release, leaving the callback attributes in place; nothing leaks, since `__del__` runs while the `Stream` itself is being collected. + +`Stream` never closes the Python object it wraps. The caller that opened a file owns that file. A `Reader` that opened the file itself tracks it separately and closes it during its own cleanup. + +### Reference cycles + +Each ctypes callback closes over its `Stream`. Captured directly, that's a reference cycle, leaving cleanup to the slower cycle collector. The callbacks capture a weak reference instead, resolved fresh each call, so the `Stream`'s reference count can reach zero on the deterministic path. + +## Methods to use with a `ManagedResource` + +Writing a new `ManagedResource` subclass, each situation maps to one call: | Situation | Call this | | --- | --- | @@ -556,14 +418,11 @@ different situation when writing a new subclass: | An FFI call consumes the current handle and returns a replacement for the same object | `_consume_and_swap(ffi_call, error_message)` | | An FFI call consumes the current handle to configure or feed another object, returning only a status code | `_consume_no_replacement(ffi_call, error_message)` | | An FFI call consumes the current handle and returns a *different* object's pointer, for that object to own | `_consume_into(ffi_call, error_message)` | -| A call fails and it is unclear whether native took the handle first (a Python exception before native reported anything, or an empty error slot) | `_release_handle()` | +| A call fails and it is unclear whether native took the handle first | `_release_handle()` | | A Python instance needs to wrap a handle a native call already returned, without creating a new one | `_wrap_native_handle(handle)` (classmethod) | -| Ordinary teardown (`close()`, `__del__`) | Neither: these already route through `_cleanup_resources()` and `_teardown()`. Nothing outside `ManagedResource` itself calls `_teardown()` directly. | +| Ordinary teardown (`close()`, `__del__`) | Neither. These already route through the shared cleanup path. Nothing outside `ManagedResource` itself tears an object down directly. | -`_activate()` and `_swap_handle()` are two low-level primitives this -situation table builds on. - -## Implementing a subclass of `ManagedResource` +## Subclassing To wrap a new native resource, inherit from `ManagedResource` and follow these rules: @@ -597,15 +456,9 @@ class NativeResource(ManagedResource): "Failed to create MyResource: {}") def _release(self): - # 4. Clean up class-specific resources. - # Never let this method raise. Must be idempotent. - # - # Consider defining a simple lifecycle for native resources - # so _release() can check whether they are releasable - # before attempting cleanup. The if-guard below - # verifies the stream exists and has not - # already been released. The try/except is a fallback - # that silences unexpected errors from .close(). + # 3. Clean up class-specific resources. Never raise; be idempotent. + # The if-guard checks the stream exists and is not yet released. + # The try/except silences unexpected errors from .close(). if self._my_stream: try: self._my_stream.close() @@ -615,7 +468,7 @@ class NativeResource(ManagedResource): self._my_stream = None def do_something(self): - # 5. Check state at the start of every public method. + # 4. Check state at the start of every public method. # This raises C2paError if the resource is closed. self._ensure_valid_state() return _lib.c2pa_my_resource_do_something(self._handle) @@ -623,18 +476,10 @@ class NativeResource(ManagedResource): ### Troubleshooting -- An attribute set only in `__init__` is missing on an instance built by `_wrap_native_handle()`, because that path never runs `__init__`. The failure shows up later as an `AttributeError` from whichever method reads the attribute, often `_release()` during cleanup. Attributes belong in `_init_attrs()`, which `__init__` calls. - -- `_init_attrs()` called after an FFI call that can raise leaves `_release()` accessing attributes that do not exist yet when that call fails, crashing with `AttributeError`. It belongs immediately after `super().__init__()`, before anything that can fail. - -- 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. Direct assignment gives up both, and the resulting bugs (an ACTIVE object with a null handle, or a silently discarded pointer) surface far from their cause. - -- A `_release()` that raises has its exception silently swallowed by `_cleanup_resources()`, visible only in the logs. A small lifecycle for managed resources would let `_release()` check whether they need releasing; the actual release call wrapped in try/except is a fallback for unexpected failures. - -- `_release()` can be called more than once (via `close()` then `__del__`, or multiple `close()` calls), so it must handle 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 unknown-ownership failure paths can issue a guarded free without risking a double-free. A manual free is still wrong — the lifecycle owns the pointer and bypassing it defeats the state checks. - -- When 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. With the ABC listed first, Python finds the abstract property before the concrete one and raises `TypeError: Can't instantiate abstract class`. The class with the concrete implementation therefore comes first (e.g. `class Context(ManagedResource, ContextProvider)`, not `class Context(ContextProvider, ManagedResource)`). - -- When two parent classes define the same method or property with different concrete implementations, the MRO silently picks the first one, which can cause subtle bugs where the wrong implementation is used. With shared property names across multiple inheritance, `ClassName.__mro__` or `ClassName.mro()` confirms the expected resolution order. +- An attribute set only in `__init__` is missing on an instance `_wrap_native_handle()` built, since that path skips `__init__`. Put attributes in `_init_attrs()` instead. +- `_init_attrs()` called after an FFI call that can raise leaves `_release()` reading attributes that don't exist yet on failure. Call it right after `super().__init__()`. +- Assigning `self._handle` or `self._lifecycle_state` directly skips `_activate()`'s and `_consume_and_swap()`'s checks, so bugs surface far from their cause. +- A raising `_release()` has its exception swallowed, visible only in logs. +- `_release()` runs more than once (`close()` then `__del__`, or repeated `close()`), so it must handle an already-cleaned-up object. Set attributes to `None` after closing them. +- Free through `ManagedResource`, not `c2pa_free` directly, so lifecycle checks stay in effect. +- Multiple inheritance ordering matters for shared property names; see [Class hierarchy](#class-hierarchy). `ClassName.__mro__` confirms it. diff --git a/pyproject.toml b/pyproject.toml index 8b6464b9..b02c9f14 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta" [project] name = "c2pa-python" -version = "0.37.12" +version = "0.38.0" requires-python = ">=3.10" description = "Python bindings for the C2PA Content Authenticity Initiative (CAI) library" readme = { file = "README.md", content-type = "text/markdown" } diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index 69b0d4eb..eca9e952 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -11,14 +11,17 @@ # specific language governing permissions and limitations under # each license. -# Version: 0.37.12 +# Version: 0.38.0 +import contextlib import ctypes import enum +import functools import json import logging import sys import os +import threading import warnings import weakref from abc import ABC, abstractmethod @@ -233,9 +236,8 @@ class ManagedResource: - Call `_activate(handle)` once the native pointer is created and 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 (the success side of - `_consume_and_swap`). + - Call `_consume_and_swap(ffi_call, message)` when an FFI call consumes + the current handle and returns a replacement. - 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. @@ -253,6 +255,11 @@ class ManagedResource: The native pointer is freed automatically via `_free_native_ptr`. """ + _inflight = 0 + _mut_inflight = 0 + _pending_teardown: Optional[bool] = None + _released = False + def _init_attrs(self): """Set this class's own attributes to their defaults. @@ -264,8 +271,178 @@ def _init_attrs(self): def __init__(self): self._lifecycle_state = LifecycleState.UNINITIALIZED self._handle = None + self._op_lock = threading.RLock() + self._inflight = 0 + self._mut_inflight = 0 + self._pending_teardown = None + self._teardown_lock = threading.Lock() + self._released = False record_owner_pid(self) + def _live_op_lock(self): + """Return this resource's operation lock, for mutual exclusion. + + Reentrant: a finalizer can run at any bytecode boundary, including + inside a region this thread already locked, and a consuming call + tears the handle down from inside the locked region. + + Falls back to a fresh lock when the attribute is missing. + + Never hold this across a native call that drives stream callbacks + (construction, resource_to_stream, the Builder stream methods, + signing). + Those calls release the Global Interpreter Lock (GIL) + and re-enter caller-supplied Python code, which may call back into + this API on another thread. + + Raises in a forked child instead of returning the lock: a child + inherits it in whatever state it had at fork(), and no thread in + the child exists to release it, so acquiring there hangs forever. + The child's copy is as unusable as a closed resource, so it + reports the same error. + """ + if is_foreign_process(self): + raise C2paError(f"{type(self).__name__} is closed") + + lock = getattr(self, '_op_lock', None) + if lock is None: + lock = threading.RLock() + try: + self._op_lock = lock + except Exception: + pass + return lock + + def _live_teardown_lock(self): + """Lock to protect teardowns. + + Held only for plain attribute updates, never across a native call or + an acquisition of the operation lock, so it can be taken either alone + or inside the operation lock without an ordering cycle. + + Falls back to a fresh lock when the attribute is missing. + """ + lock = getattr(self, '_teardown_lock', None) + if lock is None: + lock = threading.Lock() + try: + self._teardown_lock = lock + lock = self._teardown_lock + except Exception: + pass + return lock + + def _ensure_not_borrowed(self): + """Raise if any native call, shared or mutating, is in flight on + this handle, ensuring exclusive mutating calls. + + Raises: + C2paError: If a native call is in flight on this resource. + """ + if self._inflight > 0: + raise C2paError( + f"{type(self).__name__} is in use by another operation") + + def _ensure_no_mutating_call(self): + """Raise if a mutating native call is in flight on this handle. + + Raises: + C2paError: when a mutating native call is in progress. + """ + if self._mut_inflight > 0: + raise C2paError( + f"{type(self).__name__} is running a mutating operation") + + @contextlib.contextmanager + def _guarded_op(self, *, refuse_mut=True, exclusive=False): + """Hold this resource's operation lock its duration, + and mark this thread as inside a native-error section. + + Note: the native section opens first + for the native call and closes last. + + Never hold this across a native call that drives stream callbacks. + Those calls release the Global Interpreter Lock + and re-enter caller-supplied code, which may call back into this API + on another thread. + """ + with _native_section(): + try: + with self._live_op_lock(): + if exclusive: + self._ensure_not_borrowed() + self._mut_inflight += 1 + self._inflight += 1 + try: + yield + finally: + self._mut_inflight -= 1 + self._inflight -= 1 + else: + if refuse_mut: + self._ensure_no_mutating_call() + yield + finally: + self._maybe_flush_pending() + + def _begin_reservation(self, *, mutating, refuse): + """Count a native call as in flight, or raise. + + Only place counters go up. + `refuse` runs under the lock, before counting, + and raises to reject the call. + """ + try: + with self._live_op_lock(): + self._ensure_valid_state() + refuse() + if mutating: + self._mut_inflight += 1 + self._inflight += 1 + except BaseException: + self._maybe_flush_pending() + raise + + def _end_reservation(self, *, mutating): + """Uncount a _begin_reservation() call and run any queued teardown. + The only place the counters go down.""" + with self._live_op_lock(): + if mutating: + self._mut_inflight -= 1 + self._inflight -= 1 + self._maybe_flush_pending() + + @contextlib.contextmanager + def _reserve(self, *, mutating, refuse): + """Hold a reservation across a native call. + The lock is not held across the call itself: + the call may run caller-supplied reentrant callbacks. + A teardown that arrives meanwhile queues until + the last call out. + """ + self._begin_reservation(mutating=mutating, refuse=refuse) + try: + with _native_section(): + yield + finally: + self._end_reservation(mutating=mutating) + + @contextlib.contextmanager + def _native_call(self): + """Reserve the handle for a shared call: + other shared calls may run alongside it, no mutating call may.""" + with self._reserve(mutating=False, + refuse=self._ensure_no_mutating_call): + yield + + @contextlib.contextmanager + def _exclusive_native_call(self): + """Reserve the handle for a mutating call: + no other call, shared or mutating, may run alongside it.""" + with self._reserve(mutating=True, + refuse=self._ensure_not_borrowed): + yield + @staticmethod def _free_native_ptr(ptr): """Free a native pointer by passing it to c2pa_free. @@ -279,13 +456,14 @@ def _free_native_ptr(ptr): -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. + the native lib. """ result = _lib.c2pa_free(ptr) if result != 0: logger.debug( - "c2pa_free returned %s for an untracked pointer ", + "c2pa_free returned %s for an untracked pointer", result) + _write_no_error_marker() return result def _ensure_valid_state(self): @@ -321,16 +499,117 @@ def _safe_release(self): def _teardown(self, free_handle: bool): """Close the object: run _release, optionally free the handle, null it. free_handle=False (consumed) frees nothing, the new owner needs to free. + + The state change runs under the operation lock; _release and the + free run after the lock is released. + Deferred when any gate is blocking: + - this resource's own handle is in flight in a native call + - this thread is inside a native-error section for some call + - someone else holds the operation lock (free intent gets queued) + + The forked-child case is handled before the lock is taken, because + _live_op_lock() raises in a child: this path has to finish rather + than report an error, so it cannot rely on acquiring. """ if is_foreign_process(self): + self._detach_in_child() + return + + if self._released: + return + self._record_pending_intent(free_handle) + + lock = self._live_op_lock() + if not lock.acquire(blocking=False): + self._close_lifecycle() + if _in_native_section(): + _register_for_section_flush(self) + return + + detached = None + try: + if self._released: + # Checks released as it recorded possible free intents. + return + if self._inflight > 0 or _in_native_section(): + self._close_lifecycle() + if _in_native_section(): + _register_for_section_flush(self) + return + + with self._live_teardown_lock(): + pending = self._pending_teardown + if pending is not None: + free_handle = pending and free_handle + self._pending_teardown = None + detached = self._detach_for_teardown(free_handle) + finally: + lock.release() + if detached is not None: + self._finish_teardown(*detached) + + def _record_pending_intent(self, free_handle: bool): + """Queue a teardown intent, leaving the resource usable until + the intent runs. + A queued consume wins over a free, since native already owns a + consumed handle: freeing it again corrupts memory, where a missed + free only leaks. + """ + with self._live_teardown_lock(): + if self._released: + return + pending = self._pending_teardown + if pending is None: + self._pending_teardown = free_handle + else: + self._pending_teardown = pending and free_handle + + def _close_lifecycle(self): + """Close the resource so it can no longer be used.""" + with self._live_teardown_lock(): + self._lifecycle_state = LifecycleState.CLOSED + + def _record_pending_teardown(self, free_handle: bool): + """Record a teardown intent and queue it. + Also closes the resource, and a queued consume wins + over a (new) teardown request. + """ + self._record_pending_intent(free_handle) + self._close_lifecycle() + + def _detach_in_child(self): + """In a forked child: + null this copy's handle and mark it closed, without freeing. + The parent still owns the native pointer. + """ + if hasattr(self, '_handle'): self._handle = None + if hasattr(self, '_lifecycle_state'): self._lifecycle_state = LifecycleState.CLOSED - return - self._lifecycle_state = LifecycleState.CLOSED - self._safe_release() + def _detach_for_teardown(self, free_handle: bool): + """Mark the resource released and closed, and take its handle. + Called under the operation lock. Returns (handle, free_handle) for + _finish_teardown, or None when there is nothing left to do. + """ + if is_foreign_process(self): + self._detach_in_child() + return None + with self._live_teardown_lock(): + if self._released: + return None + self._released = True + self._pending_teardown = None + self._lifecycle_state = LifecycleState.CLOSED handle, self._handle = self._handle, None + return handle, free_handle + + def _finish_teardown(self, handle, free_handle: bool): + """Run _release, then free the detached handle if requested. + Called after the operation lock is released. + """ + self._safe_release() if free_handle and handle: try: ManagedResource._free_native_ptr(handle) @@ -338,13 +617,56 @@ def _teardown(self, free_handle: bool): logger.error("Failed to free native %s resources", type(self).__name__, exc_info=True) + def _has_pending_teardown(self) -> bool: + """Check if a teardown request is waiting for the resource.""" + return self._pending_teardown is not None + + def _flush_pending_pass(self): + """Attempt to run pending teardowns.""" + if self._pending_teardown is None: + return + + with self._live_op_lock(): + if self._pending_teardown is None: + return + if self._inflight > 0: + return + if _in_native_section(): + _register_for_section_flush(self) + return + with self._live_teardown_lock(): + free_handle = self._pending_teardown + self._pending_teardown = None + detached = self._detach_for_teardown(free_handle) + if detached is not None: + self._finish_teardown(*detached) + + def _maybe_flush_pending(self): + """Recheck if a teardown can run after something + that blocked it cleared.""" + if is_foreign_process(self): + return + + self._flush_pending_pass() + if self._has_pending_teardown() and not self._released: + self._flush_pending_pass() + def _release_handle(self): - """Free this handle, then close the object. Used only where ownership is - unknown (a guarded free is a real free if ours, a no-op if not). + """Free this handle and close the object, unless a queued teardown + already owns the free. Used only where ownership is unknown + (a guarded free is a real free if ours, a no-op if not). + Nulling the handle under a queued teardown would leave it nothing + to free. """ - if self._lifecycle_state != LifecycleState.ACTIVE: - self._handle = None - self._lifecycle_state = LifecycleState.CLOSED + with self._live_op_lock(): + owned_elsewhere = self._pending_teardown is not None + if not owned_elsewhere and ( + self._lifecycle_state != LifecycleState.ACTIVE): + self._handle = None + self._lifecycle_state = LifecycleState.CLOSED + owned_elsewhere = True + if owned_elsewhere: + self._maybe_flush_pending() return self._teardown(free_handle=True) @@ -373,22 +695,21 @@ def _activate(self, handle): def _create_and_activate(self, ffi_call, error_message, *, check=lambda r: not r): - """Obtain a fresh native pointer, validate it, and take ownership. - On any failure before ownership transfers, the pointer is freed - and the error re-raised. + """Get a new pointer/handle, validate, take ownership. Args: ffi_call: Zero-arg callable returning a fresh native pointer. error_message: Message for the C2paError raised on failure. - check: Predicate marking a result invalid - (default: a falsy pointer). + check: Lambda determining result invalidity. Raises: - C2paError: If the pointer fails validation; it is freed first. + C2paError: If the pointer fails the validation step. """ - ptr = ffi_call() + ptr = None try: - _check_ffi_operation_result(ptr, error_message, check=check) + with _native_section(): + ptr = ffi_call() + _check_ffi_operation_result(ptr, error_message, check=check) self._activate(ptr) except Exception: if ptr: @@ -396,36 +717,37 @@ def _create_and_activate(self, ffi_call, error_message, *, raise return ptr - def _swap_handle(self, new_handle): - """Replace the handle after an FFI call consumed the old one and - returned a replacement. - A null return from such a call is ambiguous (the callee may have - failed validation before taking ownership, or failed the operation - after), so callers must not call this with a null replacement. - Requires the resource to be active. + # Errors set by native lib, hinting at the cause of the error + # These errors here means the pointer got somehow rejected by the lib, + # so it is still ours to deal with. + _PRE_CONSUME_ERROR_TAGS = ( + "UntrackedPointer:", + "WrongPointerType:", + "PointerInUse:", + "ForeignProcess:", + ) - Args: - new_handle: Non-null native pointer returned by the FFI call + # An error tag starts the message or follows this one wrapper. + _NATIVE_ERROR_WRAPPER = "Other: " - Raises: - C2paError: If the resource is not ACTIVE or new_handle is null - """ - name = type(self).__name__ - if self._lifecycle_state != LifecycleState.ACTIVE: - raise C2paError( - f"{name}: cannot swap the handle of a resource that is not " - f"active ({self._lifecycle_state.name})") - if not new_handle: - raise C2paError(f"{name}: cannot swap in a null handle") + @staticmethod + def _is_pre_consume_rejection(error: str) -> bool: + """True when native rejected the handle before taking ownership. - self._handle = new_handle + Anchored, not a substring search: native quotes caller text verbatim, + so a tag mid-message describes the caller's input. - # Errors set by native lib, hinting at the cause of the error - # These errors here means the pointer got somehow rejected by the lib, - # so it is still ours to deal with. - _PRE_CONSUME_ERROR_TAGS = ("UntrackedPointer:", "WrongPointerType:") + The tags are the errors `untrack` in c2pa-rs + c2pa_c_ffi/src/cimpl/utils.rs returns before it removes the handle's + registry entry. + """ + body = error + if body.startswith(ManagedResource._NATIVE_ERROR_WRAPPER): + body = body[len(ManagedResource._NATIVE_ERROR_WRAPPER):] + return any(body.startswith(tag) + for tag in ManagedResource._PRE_CONSUME_ERROR_TAGS) - def _invoke_consume(self, ffi_call, error_message): + def _invoke_consume(self, ffi_call, error_message, *, reserved=False): """Run an FFI call that consumes this handle, returning its raw result. A marshalling ArgumentError is re-raised untouched (call never reached @@ -439,11 +761,16 @@ def _invoke_consume(self, ffi_call, error_message): result (a replacement pointer, a status code, ...). error_message: Format string with one placeholder, used to wrap a callback exception. + reserved: True when the caller reserved the handle with + _begin_consume(), which frees here rather than through + _release_handle(). Raises: ctypes.ArgumentError: If marshalling failed; handle untouched. C2paError: If the call raised any other exception. """ + # Same thread that makes the call, same thread-local slot. + _write_no_error_marker() try: return ffi_call(self._handle) except ctypes.ArgumentError: @@ -451,35 +778,38 @@ def _invoke_consume(self, ffi_call, error_message): # is untouched and still ours. Re-raise as-is. raise except Exception as e: - self._release_handle() + if reserved: + self._teardown(free_handle=True) + else: + self._release_handle() raise C2paError(error_message.format(e)) from e - def _raise_consume_failure(self, error_message): + def _raise_consume_failure(self, error_message, *, reserved: bool): """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. + pointer-tracking error cannot overwrite it. + The native error slot is sticky and thread-local, + and the native SDK does not clear it before the call. + _invoke_consume marks the slot as carrying no error right before a + consuming call, so a failure that sets no error of its own reads back + as no error rather than as a stale one left by an earlier call. - That ordering is required: - c2pa_free on a handle the registry no longer tracks returns -1 and - overwrites the slot with its own "Other: UntrackedPointer: 0x..." - message. Freeing first would therefore replace the real failure - with another one and, because that substitute carries a pre-consume - tag, invert the retain/consume decision made below. + A caller that reserved the handle with _begin_consume() passes + reserved=True (in-flight mark). Args: error_message: Format string with one placeholder, used when the native layer offers no error of its own. + reserved: True when the caller reserved this handle with + _begin_consume(). 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): + if ManagedResource._is_pre_consume_rejection(error): logger.warning( "%s: native call rejected the handle before taking " "ownership (%s); handle retained", @@ -494,20 +824,70 @@ def _raise_consume_failure(self, error_message): self._teardown(free_handle=False) _raise_typed_c2pa_error(error) - # No error in the slot: ownership is unknown, so free defensively. - self._release_handle() + # No error of its own: ownership is unknown, so free defensively. + # c2pa_free returns -1 for an address native already reclaimed. + if reserved: + self._teardown(free_handle=True) + else: + self._release_handle() raise C2paError(error_message.format("Unknown error")) + def _begin_consume(self): + """Reserve this handle exclusively for a consuming call, or raise. + The resource stays ACTIVE while being consumed. + A teardown (deferred while the consume is in flight) closes it. + + Raises: + C2paError: Unusable resource or native call in progress. + """ + self._begin_reservation(mutating=True, + refuse=self._ensure_not_borrowed) + + def _end_consume(self): + """Release a _begin_consume() reservation, then run any teardown + that arrived while it was held.""" + self._end_reservation(mutating=True) + 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. + """Run an FFI call consuming the handle, reserving it. + A replacement handle swaps in on success + (a returned null value 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 swap(new_ptr): + with self._live_op_lock(): + self._handle = new_ptr + + self._consume_reserved(ffi_call, error_message, + succeeded=bool, on_success=swap) + + def _consume_reserved(self, ffi_call, error_message, *, succeeded, + on_success=None): + """Run a reserved consuming call, then act on success. + + Args: + succeeded: Reads the call's raw result and returns whether it + succeeded. Each entry point has its own convention: a status + code, or a replacement pointer. + on_success: Called with the raw result on success. Default: + mark the handle consumed, closed, not freed. + + Returns: + The call's raw result, for callers that hand it on. + """ + self._begin_consume() + try: + with _native_section(): + result = self._invoke_consume( + ffi_call, error_message, reserved=True) + if succeeded(result): + if on_success is None: + self._teardown(free_handle=False) + else: + on_success(result) + return result + self._raise_consume_failure(error_message, reserved=True) + finally: + self._end_consume() def _consume_no_replacement(self, ffi_call, error_message): """Run an FFI call that consumes this handle on success, when the native @@ -515,11 +895,9 @@ def _consume_no_replacement(self, ffi_call, error_message): 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) + self._consume_reserved( + ffi_call, error_message, + succeeded=lambda status: status == 0) def _consume_into(self, ffi_call, error_message): """Run an FFI call that consumes this handle and returns a *different* @@ -527,11 +905,10 @@ def _consume_into(self, ffi_call, error_message): 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) + # A null pointer is falsy. + return self._consume_reserved( + ffi_call, error_message, + succeeded=lambda pointer: bool(pointer)) @classmethod def _wrap_native_handle(cls, handle): @@ -562,30 +939,24 @@ def _cleanup_resources(self): """Release native resources idempotently.""" try: if is_foreign_process(self): - # A forked child holds a separate copy of this object and the - # parent still owns the real handle and frees it. Mark this - # copy closed and null its handle so the child cannot mistake - # it for usable or free it, but do not free here. - # Mutating this copy does not touch the parent's. - if hasattr(self, '_handle'): - self._handle = None - if hasattr(self, '_lifecycle_state'): - self._lifecycle_state = LifecycleState.CLOSED + self._detach_in_child() return - if ( - hasattr(self, '_lifecycle_state') - and self._lifecycle_state != LifecycleState.CLOSED - ): + if hasattr(self, '_lifecycle_state'): + # Closes here must defer to the teardown checks. self._teardown(free_handle=True) except Exception: pass @property def is_valid(self) -> bool: - """Check if the resource is in a valid (active) state.""" + """Is the resource usable now? + ACTIVE, holding a handle, or a shared borrow, + and no mutating (exclusive) or consuming native call in progress.""" return ( self._lifecycle_state == LifecycleState.ACTIVE and self._handle is not None + and self._mut_inflight == 0 + and not is_foreign_process(self) ) def close(self) -> None: @@ -693,25 +1064,131 @@ class C2paStream(ctypes.Structure): ] +# Address passed to c2pa_free to plant a marker in the native error slot. +# 2 is not an allocatable address. +_NO_ERROR_MARKER_ADDR = 2 + + +@functools.lru_cache(maxsize=1) +def _marker_text_for(pid): + """Learn the marker text once per process ID.""" + return _learn_no_error_marker_text() + + +def _marker_text(): + """Marker text for this process, or None if it could not be learned.""" + return _marker_text_for(os.getpid()) + + +def _write_no_error_marker(): + """A c2pa_free of an address the registry does not track writes + an expected error message learned on first use into the + thread-local error slot and returns -1. + + This marker mechanism exists to distinguish a consuming call that + failed without setting its own error from a stale message left + by an earlier call on the same thread. + + No-op when the marker text could not be learned on first use. + """ + if _marker_text() is None: + return + _lib.c2pa_free(_NO_ERROR_MARKER_ADDR) + + +def _is_no_error_marker(message: str) -> bool: + """True for the marker meaning "no current error of our own".""" + return message == _marker_text() + + def _read_native_error() -> Optional[str]: """Read the last error from the native library, or None if unset. - Peeks: the error stays in the native slot, - until the next error overwrites it. - - With no error set the native side still returns an owned pointer to an - empty string, so the pointer alone does not tell us whether there is an - error. Only a non-empty message counts as one; the empty string still - has to be freed. + The slot is marked as carrying no error before returning, so a + given error is reported once, by the caller that observes it. The native + slot is thread-local and sticky, so a message left in place stays readable + indefinitely and is available to be reported again by a later, + unrelated call that failed without setting an error of its own + (or a missing clear of an error slot). """ error = _lib.c2pa_error() if not error: + # NULL means the message could not be rendered, not that the slot + # is empty, so it still has to be marked. + _write_no_error_marker() return None try: message = ctypes.string_at(error).decode('utf-8') finally: _lib.c2pa_string_free(error) - return message or None + + _write_no_error_marker() + if not message or _is_no_error_marker(message): + return None + return message + + +_native_section_state = threading.local() + + +def _in_native_section() -> bool: + """True while this thread is between an FFI call and reading back the + native error it may have set (see _native_section()).""" + return getattr(_native_section_state, 'depth', 0) > 0 + + +def _register_for_section_flush(resource): + """Record that `resource`'s teardown was deferred only because this + thread's native-error section was open.""" + pending = getattr(_native_section_state, 'pending_resources', None) + if pending is not None: + pending.append(resource) + + +@contextlib.contextmanager +def _native_section(): + """Mark this thread as inside a section where a native call's result is + about to be read back: an error-slot check, or a consuming call's + success/failure classification. + + Reentrant: a nested native call on the same thread nests. + """ + state = _native_section_state + depth = getattr(state, 'depth', 0) + state.depth = depth + 1 + if depth == 0: + state.pending_resources = [] + + def _drain(): + """Flush every deferred resource. Returns the first error raised.""" + pending, state.pending_resources = state.pending_resources, [] + first_error = None + for resource in pending: + try: + resource._maybe_flush_pending() + except BaseException as e: # noqa: BLE001 + if first_error is None: + first_error = e + return first_error + + try: + yield + except BaseException: + state.depth -= 1 + if state.depth == 0: + drain_error = _drain() + if drain_error is not None: + logger.error( + "Deferred teardown failed while unwinding: %s", + drain_error) + raise + else: + state.depth -= 1 + if state.depth == 0: + drain_error = _drain() + if drain_error is not None: + logger.error( + "Deferred teardown failed: %s", drain_error) class C2paSignerInfo(ctypes.Structure): @@ -751,6 +1228,7 @@ def __init__(self, alg, sign_cert, private_key, ta_url): alg = alg_str elif isinstance(alg, str): # String to bytes, as requested by native lib + _check_cstr_arg("alg", alg) alg = alg.encode('utf-8') elif isinstance(alg, bytes): # In bytes already @@ -768,6 +1246,7 @@ def __init__(self, alg, sign_cert, private_key, ta_url): pass elif isinstance(ta_url, str): # String to bytes, as requested by native lib + _check_cstr_arg("ta_url", ta_url) ta_url = ta_url.encode('utf-8') elif isinstance(ta_url, bytes): # In bytes already @@ -1036,6 +1515,37 @@ def _setup_function(func, argtypes, restype=None): ) _setup_function(_lib.c2pa_free, [ctypes.c_void_p], ctypes.c_int) + +def _learn_no_error_marker_text(): + """Plant the marker once and read back the exact text the native lib + produces for it, so equality checks match this build of the lib. + Runs on first use in each process. + + No-op/None if the marker couldn't be learned. + """ + _lib.c2pa_free(_NO_ERROR_MARKER_ADDR) + raw = _lib.c2pa_error() + if not raw: + logger.warning( + "c2pa: could not find out error marker") + return None + try: + text = ctypes.string_at(raw).decode('utf-8') + finally: + _lib.c2pa_string_free(raw) + if not text: + logger.warning( + "c2pa: error-slot marker not set, some errors may be stale") + return None + marker_hex = hex(_NO_ERROR_MARKER_ADDR) + if marker_hex not in text: + logger.warning( + "c2pa: error-slot marker %s unclear, some errors may be stale", + marker_hex) + return None + return text + + _setup_function( _lib.c2pa_context_builder_set_signer, [ctypes.POINTER(C2paContextBuilder), ctypes.POINTER(C2paSigner)], @@ -1249,12 +1759,63 @@ def _convert_to_py_string(value) -> str: # Ignore clean up issues pass except (ctypes.ArgumentError, TypeError, ValueError, OSError): - # Invalid pointer type or value + # Invalid pointer type or value, gracefully handled by native lib. + try: + _lib.c2pa_string_free(value) + except Exception: + pass return "" return py_string +def _check_cstr_arg(name: str, value) -> None: + """Reject a string argument the native layer would refuse. + Checking here keeps the rejection on this side of the boundary, + where the handle is known to be untouched. + + Raises: + C2paError: With same message the native layer would have produced. + """ + if value is None: + raise C2paError(f"NullParameter: {name}") + + embedded_nul = ( + '\x00' in value if isinstance(value, str) else b'\x00' in value) + if embedded_nul: + # ctypes truncates at the first NUL. + raise C2paError(f"NullParameter: {name} contains a null byte") + + +def _check_handle_arg(name: str, handle) -> None: + """Reject a null handle argument. + Note: registry membership is not observable from Python, + so a tracked-but-invalid pointer still reaches native. + + Raises: + C2paError: With same message the native layer would have produced. + """ + if not handle: + raise C2paError(f"NullParameter: {name}") + + +def _check_bytes_arg(name: str, buffer) -> None: + """Reject a byte buffer the native layer would refuse. + + Native rejects a null pointer and any size outside 1..=isize::MAX. + An empty buffer reaches it as size 0. + Checking here keeps the rejection on this side of the boundary, + where the handle is known to be untouched. + + Raises: + C2paError: With same message the native layer would have produced. + """ + if buffer is None: + raise C2paError(f"NullParameter: {name}") + if len(buffer) == 0: + raise C2paError(f"InvalidBufferSize: 0 for '{name}'") + + def _raise_typed_c2pa_error(error_str: str) -> None: """Parse an error string and raise the appropriate typed C2paError. @@ -1452,16 +2013,35 @@ def load_settings(settings: Union[str, dict], format: str = "json") -> None: raise C2paError(f"Failed to serialize settings to JSON: {e}") try: + _check_cstr_arg("settings", settings_str) + _check_cstr_arg("format", format) settings_bytes = settings_str.encode('utf-8') format_bytes = format.encode('utf-8') except (AttributeError, UnicodeEncodeError) as e: raise C2paError(f"Failed to encode settings to UTF-8: {e}") - result = _lib.c2pa_load_settings(settings_bytes, format_bytes) - _check_ffi_operation_result( - result, - "Error loading settings", - check=lambda r: r != 0) + with _native_section(): + result = _lib.c2pa_load_settings(settings_bytes, format_bytes) + _check_ffi_operation_result( + result, + "Error loading settings", + check=lambda r: r != 0) + + +@contextlib.contextmanager +def _context_guard(context): + """Hold a caller-supplied context valid across a native call. + + ContextProvider requires only is_valid and execution_context. + A context that implements _native_call (a ManagedResource) is + guarded through it instead. + """ + native_call = getattr(context, "_native_call", None) + if native_call is None: + yield + return + with native_call(): + yield class ContextProvider(ABC): @@ -1469,20 +2049,26 @@ class ContextProvider(ABC): Subclass to implement a custom context provider. The built-in Context class is the standard implementation. + + A provider that does not derive from ManagedResource is used without + in-flight teardown protection: closing it on another thread while a Reader + or Builder is being constructed from it can free the native context while + that construction is still using it. """ @property @abstractmethod def is_valid(self) -> bool: - """Whether this provider is in a usable state. + """Whether a call using this provider would be accepted right now. - Return True when the underlying native context is active - and its handle has not been freed or consumed. Return - False after the provider has been closed or invalidated. + Return True when the underlying native context is active, holds a + handle, and has no mutating or consuming native call in flight. + Return False after the provider has been closed or invalidated. + The answer is a snapshot and does not keep the handle alive. The ManagedResource base class provides a standard - implementation that checks lifecycle state and handle - presence. + implementation that checks lifecycle state, handle presence and + the mutating in-flight count. """ ... @@ -1543,21 +2129,28 @@ def set(self, path: str, value: str) -> 'Settings': Args: path: Dot-notation path (e.g. "builder.thumbnail.enabled"). - value: The value to set. + value: Value to set, as JSON string. Returns: self, for method chaining. - """ - self._ensure_valid_state() + Raises: + C2paError: If path or value contains a null byte, or native + rejects the path or the parsed value. + """ path_bytes = _to_utf8_bytes(path, "settings path") value_bytes = _to_utf8_bytes(value, "settings value") + _check_cstr_arg("settings path", path_bytes) + _check_cstr_arg("settings value", value_bytes) - _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) + with self._guarded_op(exclusive=True): + self._ensure_valid_state() + + _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 @@ -1574,15 +2167,17 @@ def update( Returns: self, for method chaining. """ - self._ensure_valid_state() - data_bytes = _to_utf8_bytes(data, "settings data") + _check_cstr_arg("settings data", data_bytes) - _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) + with self._guarded_op(exclusive=True): + self._ensure_valid_state() + + _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 @@ -1651,10 +2246,11 @@ 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. + class _NativeContextBuilder(ManagedResource): + """Wrapper so the context builder gets a lifecycle: + a failure inside the `with` block frees via close() + unless a consuming call already did. + Wrapper should be short-lived. """ def __init__(self): @@ -1690,25 +2286,28 @@ def __init__( else: # 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: + with self._NativeContextBuilder() as context_builder: if settings is not None: - _check_ffi_operation_result( - _lib.c2pa_context_builder_set_settings( - nb._handle, settings._c_settings), - "Failed to set settings on Context", - check=lambda r: r != 0) + with context_builder._guarded_op(exclusive=True), \ + settings._native_call(): + _check_ffi_operation_result( + _lib.c2pa_context_builder_set_settings( + context_builder._handle, + settings._c_settings), + "Failed to set settings on Context", + check=lambda r: r != 0) if signer is not None: - signer._ensure_valid_state() - # A rejected signer is retained, not closed and leaked. + # Retain a rejected signer for later teardown. self._signer_callback_cb = signer._callback_cb + _check_handle_arg('builder', context_builder._handle) signer._consume_no_replacement( lambda h: _lib.c2pa_context_builder_set_signer( - nb._handle, h), + context_builder._handle, h), "Failed to set signer on Context: {}") self._has_signer = True - context_ptr = nb._consume_into( + context_ptr = context_builder._consume_into( lambda h: _lib.c2pa_context_builder_build(h), "Failed to build Context: {}") @@ -1813,6 +2412,8 @@ def __init__(self, file_like_stream): self._closed = False self._initialized = False self._stream = None + # Serializes close() and __del__ against a concurrent double-free. + self._close_lock = threading.RLock() # Generate unique stream ID using object ID and counter stream_counter = next(Stream._stream_id_counter) @@ -2034,22 +2635,27 @@ def __del__(self): try: if is_foreign_process(self): return - # Only cleanup if not already closed and we have a valid stream - if hasattr(self, '_closed') and not self._closed: - stream = self._stream - if hasattr(self, '_stream') and stream: - # Use internal cleanup to avoid calling close() which could - # cause issues - try: - _lib.c2pa_release_stream(stream) - except Exception: - # Destructors shouldn't raise exceptions - logger.error("Failed to release Stream") - pass - finally: - self._stream = None + lock = getattr(self, '_close_lock', None) + if lock is not None and not lock.acquire(blocking=False): + return + try: + # Only cleanup if not already closed and we have a valid stream + if hasattr(self, '_closed') and not self._closed: + stream = self._stream + if hasattr(self, '_stream') and stream: self._closed = True self._initialized = False + try: + _lib.c2pa_release_stream(stream) + except Exception: + # Destructors shouldn't raise exceptions + logger.error("Failed to release Stream") + pass + finally: + self._stream = None + finally: + if lock is not None: + lock.release() except Exception: # Destructors must not raise exceptions pass @@ -2061,46 +2667,56 @@ def close(self): even if errors occur during cleanup. Errors during cleanup are logged but not raised to ensure cleanup. Multiple calls to close() are handled gracefully. + Only the Stream's owner closes the stream. """ - if self._closed: - return + # Checked before the lock, as _live_op_lock() and __del__ do: + # a child inherits _close_lock in whatever state it had at fork(), + # and the thread holding it does not exist there to release it. if is_foreign_process(self): self._closed = True self._initialized = False return - try: - # Clean up stream first as it depends on callbacks - # Note: We don't close self._file_like_stream as we don't own it, - # the opener owns it. - stream = self._stream - if stream: - try: - _lib.c2pa_release_stream(stream) - except Exception as e: - logger.error( - Stream._ERROR_MESSAGES['stream_error'].format( - str(e))) - finally: - self._stream = None + # Serializes against __del__ / a concurrent close(). + with self._close_lock: + if self._closed: + return - # Clean up callbacks - for attr in ['_read_cb', '_seek_cb', '_write_cb', '_flush_cb']: - if hasattr(self, attr): + try: + # Clean up stream first as it depends on callbacks + # Note: We don't close self._file_like_stream as we don't + # own it, the opener owns it. + stream = self._stream + if stream: + self._closed = True + self._initialized = False try: - setattr(self, attr, None) + _lib.c2pa_release_stream(stream) except Exception as e: logger.error( - Stream._ERROR_MESSAGES['callback_error'].format( - attr, str(e))) + Stream._ERROR_MESSAGES['stream_error'].format( + str(e))) + finally: + self._stream = None - except Exception as e: - logger.error( - Stream._ERROR_MESSAGES['cleanup_error'].format( - str(e))) - finally: - self._closed = True - self._initialized = False + # Clean up callbacks + for attr in [ + '_read_cb', '_seek_cb', '_write_cb', '_flush_cb']: + if hasattr(self, attr): + try: + setattr(self, attr, None) + except Exception as e: + logger.error( + Stream._ERROR_MESSAGES['callback_error'] + .format(attr, str(e))) + + except Exception as e: + logger.error( + Stream._ERROR_MESSAGES['cleanup_error'].format( + str(e))) + finally: + self._closed = True + self._initialized = False def write_to_target(self, dest_stream): self._file_like_stream.seek(0) @@ -2507,9 +3123,7 @@ def __init__( else: # format_or_path is a format string, stream is a stream object - with Stream(stream) as stream_obj: - self._create_reader( - format_bytes, stream_obj, manifest_data) + self._init_from_stream(stream, format_bytes, manifest_data) @staticmethod def _resolve_format_bytes(format_or_path, stream) -> Optional[bytes]: @@ -2580,6 +3194,24 @@ def _init_from_file(self, path, format_bytes, raise C2paError.Io( Reader._ERROR_MESSAGES['io_error'].format(str(e))) + def _init_from_stream(self, stream, format_bytes, + manifest_data=None): + """Create a reader from a caller-supplied stream object. + The native reader reads through this stream as long as it's alive, + so the wrapper is stored on the instance and released by _release(). + + Args: + stream: A stream-like object owned by the caller + format_bytes: UTF-8 encoded format/MIME type + manifest_data: Optional manifest bytes + """ + try: + self._own_stream = Stream(stream) + self._create_reader(format_bytes, self._own_stream, manifest_data) + except Exception: + self._close_streams() + raise + def _init_from_context(self, context, format_or_path, stream, manifest_data=None): """Initialize Reader from a Context object implementing @@ -2605,20 +3237,27 @@ def _init_from_context(self, context, format_or_path, self._own_stream = Stream(stream) try: - # Adopt before the consuming call: _consume_and_swap needs an - # active resource, and cleanup then owns the pointer either way. - self._create_and_activate( - lambda: _lib.c2pa_reader_from_context( - context.execution_context), - Reader._ERROR_MESSAGES['reader_error']) + # The Context is caller-supplied and may be shared, so its handle + # needs its own in-flight guard across the native call. + with _context_guard(context): + # Adopt before the consuming call: _consume_and_swap needs an + # active resource, and cleanup then owns the pointer either + # way. + self._create_and_activate( + lambda: _lib.c2pa_reader_from_context( + context.execution_context), + Reader._ERROR_MESSAGES['reader_error']) + _check_cstr_arg('format', format_arg) + _check_handle_arg('stream', self._own_stream._stream) if manifest_data is not None: + _check_bytes_arg('manifest_data', manifest_data) manifest_array = ( ctypes.c_ubyte * len(manifest_data)).from_buffer_copy(manifest_data) # Consume current reader, # with manifest data and stream (C FFI pattern), - # to create a new one (switch out) + # to switch it out using _consume_and_swap. self._consume_and_swap( lambda handle: ( _lib.c2pa_reader_with_manifest_data_and_stream( @@ -2649,6 +3288,13 @@ def _init_attrs(self): # Tracks a file we opened ourselves and must close later. self._backing_file = None + # Fragment streams handed to the native reader by with_fragment, + # which it keeps reading from for the rest of its lifecycle. + self._fragment_streams = [] + + # Serializes with_fragment against itself. + self._fragment_lock = threading.RLock() + # Caches for manifest JSON string and parsed data. # These are invalidated when with_fragment() is called. self._manifest_json_str_cache = None @@ -2672,6 +3318,12 @@ def _close_streams(self): logger.warning("Failed to close Reader backing file") finally: self._backing_file = None + for fragment in getattr(self, '_fragment_streams', []): + try: + fragment.close() + except Exception: + logger.warning("Failed to close Reader fragment stream") + self._fragment_streams = [] def _release(self): """Release Reader-specific resources (caches, stream, backing file). @@ -2693,22 +3345,25 @@ def _get_cached_manifest_data(self) -> Optional[dict]: Raises: C2paError: If there was an error getting the JSON """ - if self._manifest_data_cache is None: - if self._manifest_json_str_cache is None: - self._manifest_json_str_cache = self.json() + # Locked so the cache fields can't be read and written + # across concurrent handle swaps. + with self._guarded_op(): + self._ensure_valid_state() + if self._manifest_data_cache is None: + if self._manifest_json_str_cache is None: + self._manifest_json_str_cache = self.json() - try: - self._manifest_data_cache = json.loads( - self._manifest_json_str_cache - ) - except json.JSONDecodeError: - # Reset cache to reattempt read, possibly - self._manifest_data_cache = None - self._manifest_json_str_cache = None - # Failed to parse manifest JSON - return None + try: + self._manifest_data_cache = json.loads( + self._manifest_json_str_cache + ) + except json.JSONDecodeError: + # Next call should retry the read. + self._manifest_data_cache = None + self._manifest_json_str_cache = None + return None - return self._manifest_data_cache + return self._manifest_data_cache def with_fragment(self, format: Optional[str], stream, fragment_stream) -> "Reader": @@ -2731,29 +3386,95 @@ def with_fragment(self, format: Optional[str], stream, C2paError: If there was an error processing the fragment. On failure the native call may already have consumed the underlying object, in which case this Reader is closed and - cannot be retried: create a new one instead of reusing this - instance. + cannot be retried: create a new one. + C2paError: "Reader is already processing a fragment on another + thread" when another thread is inside this method on the same + Reader. The Reader is untouched, and the call can be retried + after the other one returns. One thread feeds fragments to a + Reader, and the caller serializes those calls. + C2paError: "Reader is in use by another operation" when another + native call is in flight on it. + + While this call runs, read methods on other threads raise + C2paError("Reader is running a mutating operation") and is_valid is + False. The Reader is usable again when the call returns successfully. + A thread that calls read methods in a loop without yielding keeps + this call refused; yield between reads. """ - self._ensure_valid_state() - format_arg = _format_ffi_arg(_encode_format(format, "Reader")) - with Stream(stream) as main_obj, Stream(fragment_stream) as frag_obj: - self._consume_and_swap( - lambda handle: _lib.c2pa_reader_with_fragment( - handle, - format_arg, - main_obj._stream, - frag_obj._stream, - ), - Reader._ERROR_MESSAGES['fragment_error']) - - # Invalidate caches: processing a new BMFF fragment updates the native - # reader's state, which can change the manifest data it returns. - # The cached JSON string and parsed dict may now be stale, so clear - # them to force a fresh read from the native layer on next access. - self._manifest_json_str_cache = None - self._manifest_data_cache = None + # A forked child cannot wait on a lock no surviving thread will + # release, so it reports the same error _live_op_lock() does. + if is_foreign_process(self): + raise C2paError(f"{type(self).__name__} is closed") + + # The native call and the ownership transfer are one unit. + # Reentrant so a thread already here can continue. + if not self._fragment_lock.acquire(blocking=False): + raise C2paError( + f"{type(self).__name__} is already processing a fragment " + f"on another thread") + try: + # The native reader keeps reading through both streams. + main_obj = Stream(stream) + frag_obj = None + try: + frag_obj = Stream(fragment_stream) + _check_cstr_arg('format', format_arg) + _check_handle_arg('stream', main_obj._stream) + _check_handle_arg('fragment', frag_obj._stream) + + def with_fragment_call(handle): + # Cleared here because these describe the replaced handle, + # and a reader must never be served them. + self._manifest_json_str_cache = None + self._manifest_data_cache = None + return _lib.c2pa_reader_with_fragment( + handle, + format_arg, + main_obj._stream, + frag_obj._stream, + ) + + self._consume_and_swap( + with_fragment_call, + Reader._ERROR_MESSAGES['fragment_error']) + except Exception: + main_obj.close() + if frag_obj is not None: + frag_obj.close() + raise + + with self._guarded_op(refuse_mut=False): + try: + self._ensure_valid_state() + except Exception: + main_obj.close() + frag_obj.close() + raise + + # Replace the streams this reader owned, closing the previous + # ones (only the current fragment is retained). + previous = self._own_stream + previous_fragments = self._fragment_streams + self._own_stream = main_obj + self._fragment_streams = [frag_obj] + if previous is not None and previous is not main_obj: + try: + previous.close() + except Exception: + logger.warning( + "Failed to close previous Reader stream") + for fragment in previous_fragments: + if fragment is frag_obj: + continue + try: + fragment.close() + except Exception: + logger.warning( + "Failed to close Reader fragment stream") + finally: + self._fragment_lock.release() return self @@ -2767,19 +3488,16 @@ def json(self) -> str: C2paError: If there was an error getting the JSON """ - self._ensure_valid_state() - - # Return cached result if available - if self._manifest_json_str_cache is not None: - return self._manifest_json_str_cache + with self._native_call(): + if self._manifest_json_str_cache is not None: + return self._manifest_json_str_cache - result = _lib.c2pa_reader_json(self._handle) - _check_ffi_operation_result(result, - "Error during manifest parsing in Reader") + result = _lib.c2pa_reader_json(self._handle) + _check_ffi_operation_result( + result, "Error during manifest parsing in Reader") - # Cache the result and return it - self._manifest_json_str_cache = _convert_to_py_string(result) - return self._manifest_json_str_cache + self._manifest_json_str_cache = _convert_to_py_string(result) + return self._manifest_json_str_cache def detailed_json(self) -> str: """Get the detailed JSON representation of the C2PA manifest store. @@ -2797,13 +3515,12 @@ def detailed_json(self) -> str: the Reader has been closed. """ - self._ensure_valid_state() - - result = _lib.c2pa_reader_detailed_json(self._handle) - _check_ffi_operation_result( - result, "Error during detailed manifest parsing in Reader") + with self._native_call(): + result = _lib.c2pa_reader_detailed_json(self._handle) + _check_ffi_operation_result( + result, "Error during detailed manifest parsing in Reader") - return _convert_to_py_string(result) + return _convert_to_py_string(result) def crjson(self) -> str: """Get the manifest store as a crJSON string. @@ -2819,12 +3536,11 @@ def crjson(self) -> str: call returns null. """ - self._ensure_valid_state() - - result = _lib.c2pa_reader_crjson(self._handle) - _check_ffi_operation_result(result, "Error parsing crJSON") + with self._native_call(): + result = _lib.c2pa_reader_crjson(self._handle) + _check_ffi_operation_result(result, "Error parsing crJSON") - return _convert_to_py_string(result) + return _convert_to_py_string(result) def _get_manifest_field(self, extractor): """Extract a field from (cached) manifest data, or None if unavailable. @@ -2928,7 +3644,7 @@ def get_validation_results(self) -> Optional[dict]: return self._get_manifest_field(lambda d: d.get("validation_results")) def resource_to_stream(self, uri: str, stream: Any) -> int: - """Write a resource to a stream. + """Write a resource to a stream (shared borrow). Args: uri: The URI of the resource to write @@ -2940,10 +3656,9 @@ def resource_to_stream(self, uri: str, stream: Any) -> int: Raises: C2paError: If there was an error writing the resource to stream """ - self._ensure_valid_state() - + _check_cstr_arg("uri", uri) uri_str = uri.encode('utf-8') - with Stream(stream) as stream_obj: + with self._native_call(), Stream(stream) as stream_obj: result = _lib.c2pa_reader_resource_to_stream( self._handle, uri_str, stream_obj._stream) @@ -2964,11 +3679,10 @@ def is_embedded(self) -> bool: Raises: C2paError: If there was an error checking the embedded status """ - self._ensure_valid_state() + with self._native_call(): + result = _lib.c2pa_reader_is_embedded(self._handle) - result = _lib.c2pa_reader_is_embedded(self._handle) - - return bool(result) + return bool(result) def get_remote_url(self) -> Optional[str]: """Get the remote URL of the manifest if it was obtained remotely. @@ -2981,17 +3695,14 @@ def get_remote_url(self) -> Optional[str]: Raises: C2paError: If there was an error getting the remote URL """ - self._ensure_valid_state() - - result = _lib.c2pa_reader_remote_url(self._handle) + with self._native_call(): + result = _lib.c2pa_reader_remote_url(self._handle) - if result is None: - # No remote URL set (manifest is embedded) - return None + if result is None: + # No remote URL set (manifest is embedded). + return None - # Convert the C string to Python string - url_str = _convert_to_py_string(result) - return url_str + return _convert_to_py_string(result) class Signer(ManagedResource): @@ -3019,10 +3730,12 @@ def from_info(cls, signer_info: C2paSignerInfo) -> 'Signer': Raises: C2paError: If there was an error creating the signer """ - signer_ptr = _lib.c2pa_signer_from_info(ctypes.byref(signer_info)) + with _native_section(): + signer_ptr = _lib.c2pa_signer_from_info(ctypes.byref(signer_info)) - _check_ffi_operation_result( - signer_ptr, "Failed to create signer from configured signer_info") + _check_ffi_operation_result( + signer_ptr, + "Failed to create signer from configured signer_info") try: return cls(signer_ptr) @@ -3145,16 +3858,17 @@ def wrapped_callback( callback_cb = SignerCallback(wrapped_callback) # Create the signer with the wrapped callback - signer_ptr = _lib.c2pa_signer_create( - None, - callback_cb, - alg, - certs_bytes, - tsa_url_bytes - ) + with _native_section(): + signer_ptr = _lib.c2pa_signer_create( + None, + callback_cb, + alg, + certs_bytes, + tsa_url_bytes + ) - _check_ffi_operation_result(signer_ptr, - "Failed to create signer") + _check_ffi_operation_result(signer_ptr, + "Failed to create signer") try: # Create and return the signer instance with the callback @@ -3209,16 +3923,17 @@ def reserve_size(self) -> int: Raises: C2paError: If there was an error getting the size """ - self._ensure_valid_state() + with self._guarded_op(): + self._ensure_valid_state() - result = _lib.c2pa_signer_reserve_size(self._handle) + result = _lib.c2pa_signer_reserve_size(self._handle) - _check_ffi_operation_result( - result, - "Failed to get reserve size", - check=lambda r: r < 0) + _check_ffi_operation_result( + result, + "Failed to get reserve size", + check=lambda r: r < 0) - return result + return result class Builder(ManagedResource): @@ -3315,11 +4030,11 @@ def from_archive( stream_obj = Stream(stream) try: - handle = _lib.c2pa_builder_from_archive(stream_obj._stream) + with _native_section(): + handle = _lib.c2pa_builder_from_archive(stream_obj._stream) - _check_ffi_operation_result(handle, - "Failed to create builder from archive" - ) + _check_ffi_operation_result( + handle, "Failed to create builder from archive") try: # A builder from an archive here carries no context. @@ -3388,12 +4103,18 @@ def _init_from_context(self, context, json_str): if not context.is_valid: raise C2paError("Context is not valid") - # Adopt before the consuming call: _consume_and_swap needs an - # active resource, and cleanup then owns the pointer either way. - self._create_and_activate( - lambda: _lib.c2pa_builder_from_context(context.execution_context), - Builder._ERROR_MESSAGES['builder_error']) + # The Context is caller-supplied and may be shared, + # so its handle needs its own in-flight guard across + # the native call, especially for state checks. + with _context_guard(context): + # Adopt before the consuming call. + self._create_and_activate( + lambda: _lib.c2pa_builder_from_context( + context.execution_context), + Builder._ERROR_MESSAGES['builder_error']) + _check_cstr_arg('manifest_json', json_str) + # _consume_and_swap reserves the handle. self._consume_and_swap( lambda handle: _lib.c2pa_builder_with_definition( handle, json_str), @@ -3416,8 +4137,9 @@ def set_no_embed(self): into the asset when signing. This is useful when creating cloud or sidecar manifests. """ - self._ensure_valid_state() - _lib.c2pa_builder_set_no_embed(self._handle) + with self._guarded_op(exclusive=True): + self._ensure_valid_state() + _lib.c2pa_builder_set_no_embed(self._handle) def set_remote_url(self, remote_url: str): """Set the remote URL. @@ -3431,15 +4153,17 @@ def set_remote_url(self, remote_url: str): Raises: C2paError: If there was an error setting the remote URL """ - self._ensure_valid_state() - url_bytes = _to_utf8_bytes(remote_url, "remote URL") - result = _lib.c2pa_builder_set_remote_url(self._handle, url_bytes) - _check_ffi_operation_result( - result, - Builder._ERROR_MESSAGES['url_error'], - check=lambda r: r != 0) + with self._guarded_op(exclusive=True): + self._ensure_valid_state() + + result = _lib.c2pa_builder_set_remote_url(self._handle, url_bytes) + + _check_ffi_operation_result( + result, + Builder._ERROR_MESSAGES['url_error'], + check=lambda r: r != 0) def set_intent( self, @@ -3467,18 +4191,19 @@ def set_intent( Raises: C2paError: If there was an error setting the intent """ - self._ensure_valid_state() + with self._guarded_op(exclusive=True): + self._ensure_valid_state() - result = _lib.c2pa_builder_set_intent( - self._handle, - ctypes.c_uint(intent), - ctypes.c_uint(digital_source_type), - ) + result = _lib.c2pa_builder_set_intent( + self._handle, + ctypes.c_uint(intent), + ctypes.c_uint(digital_source_type), + ) - _check_ffi_operation_result( - result, - Builder._ERROR_MESSAGES['intent_error'], - check=lambda r: r != 0) + _check_ffi_operation_result( + result, + Builder._ERROR_MESSAGES['intent_error'], + check=lambda r: r != 0) def add_resource(self, uri: str, stream: Any): """Add a resource to the builder. @@ -3491,10 +4216,8 @@ def add_resource(self, uri: str, stream: Any): Raises: C2paError: If there was an error adding the resource """ - self._ensure_valid_state() - uri_bytes = _to_utf8_bytes(uri, "resource URI") - with Stream(stream) as stream_obj: + with self._exclusive_native_call(), Stream(stream) as stream_obj: result = _lib.c2pa_builder_add_resource( self._handle, uri_bytes, stream_obj._stream) @@ -3555,7 +4278,7 @@ def add_ingredient_from_stream( ingredient_str = _to_utf8_bytes(ingredient_json, "ingredient JSON") format_str = _to_utf8_bytes(format, "ingredient format") - with Stream(source) as source_stream: + with self._exclusive_native_call(), Stream(source) as source_stream: result = ( _lib.c2pa_builder_add_ingredient_from_stream( self._handle, @@ -3582,15 +4305,17 @@ def add_action(self, action_json: Union[str, dict]) -> None: C2paError: If there was an error adding the action C2paError.Encoding: If the action JSON contains invalid UTF-8 chars """ - self._ensure_valid_state() - action_str = _to_utf8_bytes(action_json, "action JSON") - result = _lib.c2pa_builder_add_action(self._handle, action_str) - _check_ffi_operation_result( - result, - Builder._ERROR_MESSAGES['action_error'], - check=lambda r: r != 0) + with self._guarded_op(exclusive=True): + self._ensure_valid_state() + + result = _lib.c2pa_builder_add_action(self._handle, action_str) + + _check_ffi_operation_result( + result, + Builder._ERROR_MESSAGES['action_error'], + check=lambda r: r != 0) def to_archive(self, stream: Any) -> None: """Write an archive of the builder to a stream. @@ -3602,9 +4327,7 @@ def to_archive(self, stream: Any) -> None: Raises: C2paError: If there was an error writing the archive """ - self._ensure_valid_state() - - with Stream(stream) as stream_obj: + with self._exclusive_native_call(), Stream(stream) as stream_obj: result = _lib.c2pa_builder_to_archive( self._handle, stream_obj._stream) @@ -3629,7 +4352,7 @@ def write_ingredient_archive(self, ingredient_id: str, stream: Any) -> None: ingredient_id_str = _to_utf8_bytes(ingredient_id, "ingredient_id") - with Stream(stream) as stream_obj: + with self._exclusive_native_call(), Stream(stream) as stream_obj: result = _lib.c2pa_builder_write_ingredient_archive( self._handle, ingredient_id_str, stream_obj._stream) @@ -3649,9 +4372,7 @@ def add_ingredient_from_archive(self, stream: Any) -> None: Raises: C2paError: If there was an error reading the archive """ - self._ensure_valid_state() - - with Stream(stream) as stream_obj: + with self._exclusive_native_call(), Stream(stream) as stream_obj: result = _lib.c2pa_builder_add_ingredient_from_archive( self._handle, stream_obj._stream) @@ -3676,11 +4397,13 @@ def with_archive(self, stream: Any) -> 'Builder': C2paError: If there was an error loading the archive. On failure the native call may already have consumed the underlying object, in which case this Builder is closed and cannot be - retried: create a new one instead of reusing this instance. + retried: create a new one. + C2paError: If another native call is in flight on this Builder. """ self._ensure_valid_state() with Stream(stream) as stream_obj: + _check_handle_arg('stream', stream_obj._stream) self._consume_and_swap( lambda handle: _lib.c2pa_builder_with_archive( handle, stream_obj._stream), @@ -3727,36 +4450,41 @@ def _sign_internal( _encode_format(format, "Builder", allow_autodetect=False)) manifest_bytes_ptr = ctypes.POINTER(ctypes.c_ubyte)() - try: - if signer is not None: - result = _lib.c2pa_builder_sign( - self._handle, - format_arg, - source_stream._stream, - dest_stream._stream, - signer._handle, - ctypes.byref(manifest_bytes_ptr) - ) - else: - result = _lib.c2pa_builder_sign_context( - self._handle, - format_arg, - source_stream._stream, - dest_stream._stream, - ctypes.byref(manifest_bytes_ptr), - ) - # Sign borrows the Builder without taking ownership. - # Closing here ensures resources clean up, - # and single use/single sign done by a Builder. - self.close() - except Exception as e: - self.close() - raise C2paError(f"Error during signing: {e}") from e + # The Context pins the consumed signer's callback, which + # native invokes during this call + with self._exclusive_native_call(): + with (signer._native_call() if signer is not None + else _context_guard(self._context)): + try: + if signer is not None: + result = _lib.c2pa_builder_sign( + self._handle, + format_arg, + source_stream._stream, + dest_stream._stream, + signer._handle, + ctypes.byref(manifest_bytes_ptr) + ) + else: + result = _lib.c2pa_builder_sign_context( + self._handle, + format_arg, + source_stream._stream, + dest_stream._stream, + ctypes.byref(manifest_bytes_ptr), + ) + except Exception as e: + self.close() + raise C2paError(f"Error during signing: {e}") from e - _check_ffi_operation_result( - result, - "Error during signing", - check=lambda r: r < 0) + try: + _check_ffi_operation_result( + result, + "Error during signing", + check=lambda r: r < 0) + finally: + # Single use for a Builder, once signed, close. + self.close() # Capture the manifest bytes if available manifest_bytes = b"" @@ -3982,25 +4710,30 @@ def format_embeddable(format: str, manifest_bytes: bytes) -> tuple[int, bytes]: Raises: C2paError: If there was an error converting the manifest """ + _check_cstr_arg("format", format) format_str = format.encode('utf-8') manifest_array = (ctypes.c_ubyte * len(manifest_bytes)).from_buffer_copy( manifest_bytes ) result_bytes_ptr = ctypes.POINTER(ctypes.c_ubyte)() - result = _lib.c2pa_format_embeddable( - format_str, - manifest_array, - len(manifest_bytes), - ctypes.byref(result_bytes_ptr) - ) + with _native_section(): + result = _lib.c2pa_format_embeddable( + format_str, + manifest_array, + len(manifest_bytes), + ctypes.byref(result_bytes_ptr) + ) - _check_ffi_operation_result( - result, - "Failed to format embeddable manifest", - check=lambda r: r < 0) + _check_ffi_operation_result( + result, + "Failed to format embeddable manifest", + check=lambda r: r < 0) size = result + if not result_bytes_ptr: + raise C2paError( + "Failed to format embeddable manifest: no data returned") try: result_bytes = ctypes.string_at(result_bytes_ptr, size) except Exception as e: @@ -4114,20 +4847,22 @@ def ed25519_sign(data: bytes, private_key: str) -> bytes: # Encode private key to bytes try: + _check_cstr_arg("private_key", private_key) key_bytes = private_key.encode('utf-8') except UnicodeError as e: raise C2paError.Encoding( f"Invalid UTF-8 characters in private key: {str(e)}") # Perform the signing operation - signature_ptr = _lib.c2pa_ed25519_sign( - data_array, - data_size, - key_bytes - ) + with _native_section(): + signature_ptr = _lib.c2pa_ed25519_sign( + data_array, + data_size, + key_bytes + ) - _check_ffi_operation_result(signature_ptr, - "Failed to sign data with Ed25519") + _check_ffi_operation_result(signature_ptr, + "Failed to sign data with Ed25519") try: # Ed25519 signatures are always 64 bytes diff --git a/tests/perf/README.md b/tests/perf/README.md index 1e2baf41..a9a87a93 100644 --- a/tests/perf/README.md +++ b/tests/perf/README.md @@ -1,9 +1,21 @@ -# Memory profiling framework +# Performance and thread-safety frameworks + +Two suites share this directory and one Docker image: + +| Suite | Question it answers | Entry point | +| --- | --- | --- | +| Memory profiling | Does an operation allocate or leak more than it used to? | `make memory-use-bench` | +| Thread-safety invariants | Do the concurrency guards still hold? | `make threading-bench` | + +The memory suite is documented first; the thread-safety suite has its own section +at the end. + +## Memory profiling Uses [memray](https://github.com/bloomberg/memray) to track peak memory, allocation patterns, and memory leaks across c2pa-python SDK operations. -## Files +### Files | File | Purpose | | --- | --- | @@ -13,7 +25,7 @@ and memory leaks across c2pa-python SDK operations. | `entrypoint.sh` | Container entrypoint. Downloads the Linux native `libc2pa_c.so` at startup into the volume-mounted workspace so it sticks around even through the `-v` mount. | | `reports/` | Generated HTML reports (gitignored). Three files per scenario: `-peak.html` (peak/high-water view), `-leaks.html` (leak view), and `-temporary.html` (temporary-allocations view). | -## Scenarios +### Scenarios Each scenario loops multiple times so leaks accumulate and become visible in the leaks flamegraph and the memory use graph (defaults to 100). Change the count of iterations when running by setting the `MEMRAY_ITERATIONS` variable (the Makefile forwards it into the container): @@ -25,7 +37,7 @@ Most scenarios use the Context API: they build a `Context` once and reuse it acr The `builder_sign_{jpeg,png}_parallel_*` scenarios build one `Context` and share it across 10 threads that sign concurrently, each with its own streams and `Builder`. The name encodes two axes. `split` divides the iteration budget across the threads, so total work matches a single-threaded scenario; `full` runs the full loop on each of the 10 threads, so total work is 10x (use these with `SCENARIO=` rather than the whole suite). `pool` runs the threads through a `ThreadPoolExecutor`; `barrier` starts all 10 at once with a `threading.Barrier`. -## Environments +### Environments Select the target environment with `PERF_ENV` (default: `python-3.12-slim`): @@ -38,7 +50,7 @@ Select the target environment with `PERF_ENV` (default: `python-3.12-slim`): The slim images run a source-built `/usr/local/bin/python` that ships stripped, and Debian's `python3-dbg` targets a different binary (build-id mismatch), so memray cannot resolve the interpreter's native (C) frames there. You will see a "No debug information was found for the Python interpreter" warning, and native traces may lack file names and line numbers. The ubuntu images install `python3-dbg` for the matching apt interpreter, so their native flamegraphs are fully symbolized. Use an `ubuntu-*` `PERF_ENV` when you need resolved native traces. -## Running (via Docker) +### Running (via Docker) ```bash # First run (if there is no baseline.json): establishes baseline.json @@ -67,7 +79,7 @@ The trailing `VAR=value` arguments (e.g. `PERF_ENV=ubuntu-24.04`, `PERF_ARGS=--u Reports are written to `tests/perf/reports/` on the local machine. Three HTML files per scenario, one per suffix (described below). Open any in a browser. After a run, the run also reports if the scenarios were or were not all within baseline threshold (baseline +10% memory use tolerance). -## Running in CI +### Running in CI The `.github/workflows/memory-benchmark.yml` workflow runs the Docker-based benchmarks on a PR, but only when the PR has the `check-memory-benchmark` label. This runs `make memory-use-bench`, so: @@ -77,11 +89,11 @@ The `.github/workflows/memory-benchmark.yml` workflow runs the Docker-based ben The gate only acts as regression test once a `tests/perf/baseline.json` is committed on the branch. Without one, `run_profile.py` treats the run as baseline creation (exits 0, no gating). -## Report views +### Report views Each scenario produces three [memray flamegraphs](https://bloomberg.github.io/memray/flamegraph.html). All three are flamegraphs of the same run. They differ only in which allocations they count. -### `-peak.html`: peak/high-water view +#### `-peak.html`: peak/high-water view What it shows: allocations that were simultaneously alive at the moment the process used the most memory (the high-water mark). @@ -89,7 +101,7 @@ Why it's useful: tells you what drives the largest memory footprint, the working How to read it: the widest frames are the biggest contributors to peak. Walk up a wide column to the top frame to find the call site holding that memory at the high-water instant. -### `-leaks.html`: leak view +#### `-leaks.html`: leak view What it shows: memory that was allocated but never freed before tracking stopped (`memray --leaks`). @@ -97,7 +109,7 @@ Why it's useful: finds memory leaks, meaning memory that grows with work done. I How to read it: a wide frame here is unfreed memory. If its width grows when you raise the iteration count, that top frame is the leaking call site. -### `-temporary.html`: temporary-allocations view +#### `-temporary.html`: temporary-allocations view What it shows: short-lived churn, meaning memory allocated and then freed almost immediately (memray's threshold: freed before more than one other allocation happens). @@ -107,7 +119,7 @@ How to read it: wide frames are the biggest sources of throwaway allocations. Th The temporary view is the heaviest to render: memray holds every allocation and free to decide which are short-lived. On a very large capture (a long run, a high `MEMRAY_ITERATIONS`, or a churn-heavy scenario) the render can run out of memory and fail. The run does not abort in that case; it records what failed and keeps going. See [Troubleshooting](#troubleshooting). -## Running without Docker (if memray is supported and installed locally) +### Running without Docker (if memray is supported and installed locally) ```bash pip install memray @@ -126,7 +138,7 @@ With `--update-baseline`, a single-scenario run only rewrites that scenario's en python -m tests.perf.run_profile --scenario builder_sign_gif --update-baseline ``` -## Configuration +### Configuration With `make memory-use-bench VAR=value` you set the **`make` variable** and the Makefile forwards it as shown in the "Forwarded as" column. Running `run_profile.py` without Docker, you set the **env var** (or pass the CLI arg) directly. @@ -146,7 +158,7 @@ Example to override iteration count: make memory-use-bench MEMRAY_ITERATIONS=1000 ``` -## Reading baseline.json +### Reading baseline.json `baseline.json` is committed to the repo and reports following data for each scenario: @@ -188,7 +200,7 @@ The `_meta` block records which toolchain produced the baseline so the numbers a `total_allocations` is the total number of individual memory allocation calls made. -### Why is leaked_bytes not zero? +#### Why is leaked_bytes not zero? You might expect the baseline to show `leaked_bytes: 0`. In practice it never does. When the c2pa native library (`libc2pa_c.so`) is first loaded, Rust sets up global data structures designed to live for the entire lifetime of the process. They get cleaned up when the process exits, which is after memray stops watching, so memray sees them as "never freed" even though they are not leaking. @@ -198,7 +210,7 @@ The baseline captures this expected static overhead. Future runs compare against The framework runs `gc.collect()` twice after the scenario finishes, while memray is still tracking. Without that sweep, objects sitting in not-yet-collected reference cycles would be counted in `leaked_bytes` and the number would depend on garbage collector timing rather than on actual leaks. With it, `leaked_bytes` means memory that is still allocated even though nothing in Python can reach it: true leaks plus the one-time static overhead described above. -### How to confirm no leak exists? +#### How to confirm no leak exists? Run with a higher iteration count than default (100) and compare: @@ -208,7 +220,7 @@ make memory-use-bench MEMRAY_ITERATIONS=1000 PERF_ARGS=--update-baseline If `leaked_bytes` stays flat compared to a baseline run or in a larger run (more iterations), there is no leak. If it scales with iterations, open `tests/perf/reports/-leaks.html` in a browser to see which function is responsible. -### Reading the "Resident set size over time" graph (why memory looks like it climbs) +#### Reading the "Resident set size over time" graph (why memory looks like it climbs) The "Resident set size over time" plot (chart icon, top-right of the report) draws two lines. "Resident size" (RSS) is every page the OS counts as resident: interpreter and pages the allocator holds but has not returned. "Heap size" is only the live tracked allocations. @@ -216,11 +228,11 @@ On the parallel scenarios the RSS line steps up and stays high. The threads each Judge leaks by the heap line. The heap rises early and then settles or falls, the same shape as the single-threaded baseline. A within-run heap rise is not by itself proof of a leak (the allocator high-water can climb and settle within a bounded run). -### Temporary allocations +#### Temporary allocations `-temporary.html` shows temporary allocations, meaning memory that is allocated and then freed almost immediately (memray's threshold is one allocation: a block is temporary if it is freed before more than one other allocation happens). The memory is returned, so these are not leaks, but they are churn: high allocation and free turnover that costs CPU and can fragment the heap. A scenario doing lots of short-lived work can show heavy temporary allocations while `leaked_bytes` stays flat. -### When to update the baseline +#### When to update the baseline Update `baseline.json` after any intentional change that affects memory use: @@ -230,9 +242,9 @@ make memory-use-bench PERF_ARGS=--update-baseline Commit the updated `baseline.json` alongside the code change, so it becomes the new reference to compare against. -## Troubleshooting +### Troubleshooting -### A flamegraph render fails with `exit -9` +#### A flamegraph render fails with `exit -9` You may see a message like `flamegraph render failed for reader_mp4-...-temporary.html (killed (likely OOM))`. The `-9` is SIGKILL: the operating system's out-of-memory killer terminated the `memray flamegraph` subprocess. The temporary view is the heaviest to render, and on a large capture (a long run, a high `MEMRAY_ITERATIONS`, or a churn-heavy scenario such as `reader_mp4`) it can exhaust available memory. @@ -263,3 +275,208 @@ python3 -m memray flamegraph reports/reader_mp4-python-3.12-slim.bin \ -o reports/reader_mp4-python-3.12-slim-temporary.html \ --temporary-allocations --temporary-allocation-threshold=10 --force ``` + +## Thread-safety invariants + +Checks that the binding's concurrency guards still hold. Shares this directory's +Docker image and fixtures with the memory suite; different question, different +driver. + +### Files + +| File | Purpose | +| --- | --- | +| `thread_scenarios.py` | The scenarios and the `THREAD_SCENARIOS` registry, which pairs each scenario with the outcome it must produce. Imported by `run_thread_profile.py`. | +| `run_thread_profile.py` | Runs each scenario in a subprocess, classifies the result, and fails the run unless every round produced the expected outcome. | +| `reports/--threads.log` | Captured stderr from a failing scenario, holding the all-threads traceback (gitignored). | + +### What this measures, and why it is not a crash test + +The faults being guarded against are use-after-free of C2PA handles and of ctypes +trampolines, reached through the GIL-release window that every `ctypes` call opens. +They do not raise: they corrupt memory, and the crash surfaces somewhere unrelated, +or as a hang, or as wrong output. + +Waiting for that crash is a poor gate. Freeing memory does not unmap it, so reading +through a freed pointer usually succeeds and returns whatever occupies the address +now. Under sustained load the per-round crash probability measured **p = 0.00124**, +needing ~5000 rounds (~4 minutes) to reach 99.8%. Forcing a crash deterministically +also failed: the corruption needs the allocator to have reused the page, and that is +not something a scenario can force. + +So these scenarios force the dangerous interleaving and then assert the guard state +instead. That is deterministic: each invariant in the table below separates +hardened from unhardened code on every round. + +### The forcing primitive + +A stream or signer callback that blocks on a `threading.Event`. A callback runs on +the thread that entered the native call, so blocking inside one holds that native +call open with the GIL released. A teardown issued from another thread then lands +mid-call by construction rather than by luck. + +`ParkingStream` does this for stream callbacks, `parking_sign_callback` for the +signer trampoline. + +### Injection is verified every round + +A scenario whose callback is never reached reports `NOT_PARKED` and fails. Without +that check, a scenario that stopped forcing anything would keep passing while +testing nothing. A `with_fragment` candidate hit exactly this during +development: it returned `NO_URI` on both hardened and unhardened code, because an +MP4 init segment has no thumbnail resource to stream. It was dropped rather than +kept as an always-green test. + +For the same reason, assertions are behavioural and never `hasattr`. Guard symbols +such as `_native_section`, `_inflight` and `_op_lock` do not exist at all on +unhardened code, so asserting their presence would test that code exists, not that +it works. + +### Scenarios + +| Scenario | Asserts | Hardened | Unhardened | +| --- | --- | --- | --- | +| `trampoline_held_during_sign` | the signer trampoline outlives a `Context` closed mid-sign | `HELD` | `DROPPED` | +| `no_free_during_parked_call` | no `c2pa_free` while a call still holds the handle | `freed=0` | `freed=1` | + +`no_free_during_parked_call` is the most direct: it instruments +`ManagedResource._free_native_ptr`, the single funnel every free passes through, and +starts counting once the callback confirms the call is still open. Unhardened code +frees the in-use handle once per round, which is the use-after-free observed rather +than inferred from a crash. + +`trampoline_held_during_sign` covers the worst failure mode. The freed object there +is an ordinary refcounted Python object whose only reference is one attribute on the +`Context`. `_release()` dropping that reference leaves native calling through freed +memory, and a sign that never invoked the signer can still report success. + +### Running (via Docker) + +```bash +# Build the image (shared with the memory suite) +make perf-image + +# Check the harness can detect failures, then run the scenarios +make threading-bench + +# Just the harness self-check +make threading-bench-self-test + +# A single scenario +make threading-bench SCENARIO=no_free_during_parked_call + +# More rounds per scenario +make threading-bench THREAD_ROUNDS=100 + +# Clear failure logs +make clean-threading-reports +``` + +Takes about 40 seconds at the default 20 rounds. + +### Running without Docker + +```bash +PYTHONPATH=src python -m tests.perf.run_thread_profile --list +PYTHONPATH=src python -m tests.perf.run_thread_profile --self-test +PYTHONPATH=src python -m tests.perf.run_thread_profile +``` + +### Configuration + +| Variable | Default | Meaning | +| --- | --- | --- | +| `THREAD_ROUNDS` | `20` | Rounds per scenario. Each is independent, so a partial violation shows as a mixed count. | +| `THREAD_HANG_TIMEOUT` | `120` | Seconds before a stuck scenario is dumped and killed. | +| `PERF_ENV` | `python-3.12-slim` | Image tag, and the suffix on log filenames. | + +There is no baseline file. Each scenario asserts a fixed invariant, so there is +nothing to drift against: the expected value is declared in the registry next to +the scenario. + +### Statuses + +| Status | Meaning | +| --- | --- | +| `pass` | Every round produced the expected outcome. | +| `VIOLATED` | A guard did not hold. The counters name what happened instead. | +| `CRASHED` | Killed by SIGSEGV, SIGABRT or SIGTRAP: native memory corruption. | +| `HUNG` | No progress within `THREAD_HANG_TIMEOUT`, usually a guard that blocked where it should refuse. | +| `FAILED` | Any other non-zero exit: a scenario bug, a missing dependency, a failed artifact download. | + +`CRASHED` and `FAILED` stay separate. An `ImportError` and a failed native-library +download both exit 1, and reporting either as a crash would invent a finding that +does not exist. Only a signalled exit counts as corruption, and a signalled exit is +reported as `128+N` by a container but `-N` by a direct child, so both encodings are +recognised. + +`--self-test` proves all four classes are distinguished, including that an exception +is not reported as a crash. It runs before the scenarios in CI, because a harness +that has never been shown to detect a failure cannot be told apart from one that +cannot detect anything. + +### Reading a failure + +`VIOLATED` prints the counter dict, so a partial violation is visible as such: + +```text +VIOLATED: expected freed=0 x20, got {"freed=0": 17, "freed=1": 3} +``` + +`CRASHED` and `HUNG` write the child's stderr to +`reports/--threads.log` and echo it. `faulthandler` dumps a traceback +for every thread, which is what identifies the blocked lock; its timer is a C +thread, so it fires even when the main thread is parked inside a native call with +the GIL released. + +### Confirming the gate still detects regressions + +These scenarios are only worth their runtime if they fail on unhardened code. Check +that against a pre-hardening revision, in a scratch worktree so the working tree is +never touched: + +```bash +git worktree add --detach /tmp/c2pa-prefix +cp -r tests/perf tests/fixtures /tmp/c2pa-prefix/tests/ +mkdir -p /tmp/c2pa-prefix/src/c2pa/libs +cp src/c2pa/libs/libc2pa_c.* /tmp/c2pa-prefix/src/c2pa/libs/ + +cd /tmp/c2pa-prefix +THREAD_ROUNDS=5 PYTHONPATH=/tmp/c2pa-prefix/src \ + python -m tests.perf.run_thread_profile + +cd - # leave the worktree before removing it +git worktree remove --force /tmp/c2pa-prefix +``` + +All four must report `VIOLATED` with the unhardened value from the scenario table. A +scenario that passes there asserts nothing and should be fixed or removed. + +### Running in CI + +`.github/workflows/threading-benchmark.yml` runs on pull requests labelled +`check-threading-benchmark`, from a collaborator, on `ubuntu-24.04-arm`. It builds +the image, runs the harness self-check, runs the scenarios, and uploads any +`*-threads.log` as the `threading-invariant-logs` artifact. The job carries a +`timeout-minutes` backstop in case a guard blocks the interpreter before the +in-process hang detector is armed. + +### Gotchas + +- The container re-downloads the native library at start-up. `entrypoint.sh` + calls the GitHub API every run and gets `403 rate limit exceeded` after a few dozen + unauthenticated runs. Python then never starts, and the container exits 1 with only + downloader output, which gives no sign that a whole batch of results is void. The + Make targets forward + `GITHUB_TOKEN` for this reason. For a long unauthenticated local loop, bypass the + entrypoint with `docker run --entrypoint python ...`. The library is already in + `src/c2pa/libs/`, so nothing needs to be downloaded. +- `Signer.from_info` has no Python trampoline (`_callback_cb` is a `str`), so only + `Signer.from_callback` exercises trampoline lifetime. +- The signing algorithm enum is `C2paSigningAlg`, not `SigningAlg`. +- `Builder.sign()` is `sign(signer, format, source, dest=None)` or + `sign(format, source, dest=None)`, and returns manifest bytes. Calling + `sign(fmt, source, out)` binds `out` to `source` instead of `dest`, and signs + nothing while raising no error, so scenarios assert on the returned manifest length. +- `Reader.with_fragment(format, stream, fragment_stream)` takes three arguments. +- `_handle` is a ctypes pointer object, not an int. Use `repr()`, never `int()`. diff --git a/tests/perf/baseline.json b/tests/perf/baseline.json index c151efe5..5d7017ee 100644 --- a/tests/perf/baseline.json +++ b/tests/perf/baseline.json @@ -2,299 +2,304 @@ "_meta": { "memray_version": "1.19.3", "python_version": "3.12.13", - "c2pa_native_version": "c2pa-v0.90.0", + "c2pa_native_version": "c2pa-v0.90.16", "iterations": 200, "perf_env": "python-3.12-slim", "arch": "aarch64" }, "reader_jpeg_legacy": { - "peak_bytes": 3851610, - "leaked_bytes": 3351823, - "total_allocations": 1362322 + "peak_bytes": 3912724, + "leaked_bytes": 3414162, + "total_allocations": 1324293 }, "reader_jpeg_with_context": { - "peak_bytes": 3845367, - "leaked_bytes": 3345097, - "total_allocations": 1349879 + "peak_bytes": 3907256, + "leaked_bytes": 3407478, + "total_allocations": 1333897 }, "reader_manifest_data_context": { - "peak_bytes": 7636730, - "leaked_bytes": 3468040, - "total_allocations": 1147359 + "peak_bytes": 7692972, + "leaked_bytes": 3524955, + "total_allocations": 1132827 }, "reader_mp4": { - "peak_bytes": 4222601, - "leaked_bytes": 3345724, - "total_allocations": 4095915 + "peak_bytes": 4272788, + "leaked_bytes": 3406355, + "total_allocations": 4018933 }, "reader_wav": { - "peak_bytes": 4523095, - "leaked_bytes": 3355666, - "total_allocations": 742391 + "peak_bytes": 4573253, + "leaked_bytes": 3416313, + "total_allocations": 773409 }, "builder_sign_jpeg_legacy": { - "peak_bytes": 7785129, - "leaked_bytes": 3468507, - "total_allocations": 1041412 + "peak_bytes": 7844930, + "leaked_bytes": 3530986, + "total_allocations": 1046607 }, "builder_sign_jpeg_with_context": { - "peak_bytes": 7779538, - "leaked_bytes": 3463042, - "total_allocations": 1027485 + "peak_bytes": 7839456, + "leaked_bytes": 3524460, + "total_allocations": 1058202 }, "builder_sign_png_legacy": { - "peak_bytes": 8023081, - "leaked_bytes": 3468300, - "total_allocations": 3883115 + "peak_bytes": 8082891, + "leaked_bytes": 3530892, + "total_allocations": 3888499 }, "builder_sign_png_with_context": { - "peak_bytes": 8017008, - "leaked_bytes": 3462829, - "total_allocations": 3869515 + "peak_bytes": 8077349, + "leaked_bytes": 3524774, + "total_allocations": 3900456 }, "builder_sign_jpeg_parallel_split_pool": { - "peak_bytes": 45854797, - "leaked_bytes": 3840928, - "total_allocations": 1035646 + "peak_bytes": 45936892, + "leaked_bytes": 3893837, + "total_allocations": 1062520 }, "builder_sign_jpeg_parallel_split_barrier": { - "peak_bytes": 45844809, - "leaked_bytes": 3861014, - "total_allocations": 1037741 + "peak_bytes": 45905378, + "leaked_bytes": 3892593, + "total_allocations": 1061149 }, "builder_sign_png_parallel_split_pool": { - "peak_bytes": 46586728, - "leaked_bytes": 3868054, - "total_allocations": 3877696 + "peak_bytes": 46673225, + "leaked_bytes": 3929128, + "total_allocations": 3904507 }, "builder_sign_png_parallel_split_barrier": { - "peak_bytes": 46082548, - "leaked_bytes": 3879161, - "total_allocations": 3879780 + "peak_bytes": 46143013, + "leaked_bytes": 3910964, + "total_allocations": 3903125 }, "builder_sign_gif": { - "peak_bytes": 14635465, - "leaked_bytes": 3461270, - "total_allocations": 17017654 + "peak_bytes": 14696646, + "leaked_bytes": 3524513, + "total_allocations": 17048351 }, "builder_sign_heic": { - "peak_bytes": 4698434, - "leaked_bytes": 3469086, - "total_allocations": 1563419 + "peak_bytes": 4759642, + "leaked_bytes": 3532315, + "total_allocations": 1582361 }, "builder_sign_m4a": { - "peak_bytes": 18833496, - "leaked_bytes": 3469085, - "total_allocations": 5194205 + "peak_bytes": 18895243, + "leaked_bytes": 3532373, + "total_allocations": 5213365 }, "builder_sign_webp": { - "peak_bytes": 8991237, - "leaked_bytes": 3461271, - "total_allocations": 916145 + "peak_bytes": 9052463, + "leaked_bytes": 3524559, + "total_allocations": 950737 }, "builder_sign_avi": { - "peak_bytes": 7130933, - "leaked_bytes": 3461270, - "total_allocations": 89982012 + "peak_bytes": 7192106, + "leaked_bytes": 3524502, + "total_allocations": 90011891 }, "builder_sign_mp4": { - "peak_bytes": 6245379, - "leaked_bytes": 3469085, - "total_allocations": 3788717 + "peak_bytes": 6306630, + "leaked_bytes": 3532325, + "total_allocations": 3805992 }, "builder_sign_tiff": { - "peak_bytes": 13213169, - "leaked_bytes": 3461271, - "total_allocations": 10862700 + "peak_bytes": 13274395, + "leaked_bytes": 3524559, + "total_allocations": 10898003 }, "builder_sign_jpeg_parent_of": { - "peak_bytes": 14265295, - "leaked_bytes": 3461665, - "total_allocations": 2506107 + "peak_bytes": 14324563, + "leaked_bytes": 3525074, + "total_allocations": 2495210 }, "builder_sign_jpeg_component_of": { - "peak_bytes": 14266996, - "leaked_bytes": 3462012, - "total_allocations": 2551180 + "peak_bytes": 14325966, + "leaked_bytes": 3524803, + "total_allocations": 2538825 }, "builder_sign_jpeg_parent_and_component": { - "peak_bytes": 14665241, - "leaked_bytes": 3614613, - "total_allocations": 4523960 + "peak_bytes": 14605703, + "leaked_bytes": 3610907, + "total_allocations": 4464450 }, "builder_sign_jpeg_parent_and_component_mixed_mime": { - "peak_bytes": 14568780, - "leaked_bytes": 3462718, - "total_allocations": 5517180 + "peak_bytes": 14627596, + "leaked_bytes": 3525478, + "total_allocations": 5516963 }, "builder_sign_jpeg_two_components_same_mime": { - "peak_bytes": 14559274, - "leaked_bytes": 3564233, - "total_allocations": 4497379 + "peak_bytes": 14602589, + "leaked_bytes": 3610897, + "total_allocations": 4436718 }, "builder_sign_jpeg_two_components_mixed_mime": { - "peak_bytes": 14564839, - "leaked_bytes": 3461873, - "total_allocations": 5490592 + "peak_bytes": 14624276, + "leaked_bytes": 3525287, + "total_allocations": 5489138 }, "builder_sign_jpeg_archive_roundtrip": { - "peak_bytes": 14297571, - "leaked_bytes": 3481212, - "total_allocations": 3467149 + "peak_bytes": 14356429, + "leaked_bytes": 3545507, + "total_allocations": 3432412 }, "builder_from_archive_roundtrip": { - "peak_bytes": 14297349, - "leaked_bytes": 3480475, - "total_allocations": 3101030 + "peak_bytes": 14353258, + "leaked_bytes": 3542427, + "total_allocations": 3024501 }, "builder_with_archive_swap": { - "peak_bytes": 3681081, - "leaked_bytes": 3350198, - "total_allocations": 704373 + "peak_bytes": 3753525, + "leaked_bytes": 3421374, + "total_allocations": 744039 }, "reader_with_fragment_swap": { - "peak_bytes": 3778159, - "leaked_bytes": 3353205, - "total_allocations": 3787587 + "peak_bytes": 3839453, + "leaked_bytes": 3414104, + "total_allocations": 3806570 }, "with_fragment_pre_consume_rejection": { - "peak_bytes": 3778057, - "leaked_bytes": 3354795, - "total_allocations": 2094004 + "peak_bytes": 3841126, + "leaked_bytes": 3417826, + "total_allocations": 2128201 }, "with_archive_post_consume_failure": { - "peak_bytes": 3350600, - "leaked_bytes": 3308056, - "total_allocations": 175290 + "peak_bytes": 3423615, + "leaked_bytes": 3380332, + "total_allocations": 208578 }, "with_fragment_marshalling_error": { - "peak_bytes": 3708068, - "leaked_bytes": 3352335, - "total_allocations": 2077090 + "peak_bytes": 3767911, + "leaked_bytes": 3413267, + "total_allocations": 2094661 }, "with_fragment_mixed_outcomes": { - "peak_bytes": 3779175, - "leaked_bytes": 3356294, - "total_allocations": 2656787 + "peak_bytes": 3840297, + "leaked_bytes": 3417238, + "total_allocations": 2686269 }, "builder_to_archive_with_ingredient": { - "peak_bytes": 14069232, - "leaked_bytes": 3337316, - "total_allocations": 1830896 + "peak_bytes": 14142488, + "leaked_bytes": 3409388, + "total_allocations": 1790801 }, "builder_sign_jpeg_archive_roundtrip_ingredient_in_archive": { - "peak_bytes": 14287046, - "leaked_bytes": 3481977, - "total_allocations": 5879957 + "peak_bytes": 14345054, + "leaked_bytes": 3543673, + "total_allocations": 5769615 }, "builder_write_ingredient_archive": { - "peak_bytes": 14069289, - "leaked_bytes": 3337377, - "total_allocations": 1805304 + "peak_bytes": 14142437, + "leaked_bytes": 3409341, + "total_allocations": 1767419 }, "builder_sign_jpeg_add_ingredient_from_archive": { - "peak_bytes": 14133742, - "leaked_bytes": 3480831, - "total_allocations": 3415920 + "peak_bytes": 14207929, + "leaked_bytes": 3544945, + "total_allocations": 3383511 }, "builder_ingredient_archive_roundtrip": { - "peak_bytes": 14284443, - "leaked_bytes": 3480809, - "total_allocations": 5132060 + "peak_bytes": 14345163, + "leaked_bytes": 3545508, + "total_allocations": 5061203 }, "builder_sign_jpeg_two_ingredient_archives": { - "peak_bytes": 14134560, - "leaked_bytes": 3481604, - "total_allocations": 4215728 + "peak_bytes": 14208534, + "leaked_bytes": 3545923, + "total_allocations": 4185124 }, "reader_error_no_manifest": { - "peak_bytes": 3564471, - "leaked_bytes": 3323629, - "total_allocations": 276175 + "peak_bytes": 3622191, + "leaked_bytes": 3383889, + "total_allocations": 291735 }, "builder_error_invalid_manifest": { - "peak_bytes": 3352053, - "leaked_bytes": 3297079, - "total_allocations": 113926 + "peak_bytes": 3421406, + "leaked_bytes": 3365544, + "total_allocations": 126199 }, "reader_string_apis": { - "peak_bytes": 3978113, - "leaked_bytes": 3346111, - "total_allocations": 2287335 + "peak_bytes": 4039136, + "leaked_bytes": 3407512, + "total_allocations": 2238705 }, "signer_construction": { - "peak_bytes": 3350893, - "leaked_bytes": 3288137, - "total_allocations": 153245 + "peak_bytes": 3421644, + "leaked_bytes": 3358098, + "total_allocations": 159717 }, "builder_from_context_construction": { - "peak_bytes": 3350600, - "leaked_bytes": 3288582, - "total_allocations": 112688 + "peak_bytes": 3423152, + "leaked_bytes": 3360708, + "total_allocations": 146014 }, "fork_reader_collect": { - "peak_bytes": 3850530, - "leaked_bytes": 3353063, - "total_allocations": 1328122 + "peak_bytes": 3911936, + "leaked_bytes": 3413740, + "total_allocations": 1284294 }, "fork_contended_mutex": { - "peak_bytes": 7679019, - "leaked_bytes": 3482128, - "total_allocations": 67472694 + "peak_bytes": 7700288, + "leaked_bytes": 3510073, + "total_allocations": 66621221 }, "fork_thread_local_orphan": { - "peak_bytes": 3936170, - "leaked_bytes": 3439733, - "total_allocations": 1381055 + "peak_bytes": 4073730, + "leaked_bytes": 3581333, + "total_allocations": 1339630 }, "fork_gc_cycle": { - "peak_bytes": 3850434, - "leaked_bytes": 3353160, - "total_allocations": 1332098 + "peak_bytes": 3913068, + "leaked_bytes": 3414664, + "total_allocations": 1289268 }, "fork_parent_frees_after_fork": { - "peak_bytes": 5447584, - "leaked_bytes": 3350400, - "total_allocations": 24829257 + "peak_bytes": 5602620, + "leaked_bytes": 3423572, + "total_allocations": 23965279 }, "fork_child_closes_then_parent_frees": { - "peak_bytes": 5446620, - "leaked_bytes": 3350407, - "total_allocations": 24829254 + "peak_bytes": 5603711, + "leaked_bytes": 3424393, + "total_allocations": 23965271 }, "fork_child_sys_exit": { - "peak_bytes": 3850546, - "leaked_bytes": 3353234, - "total_allocations": 1335925 + "peak_bytes": 3911952, + "leaked_bytes": 3413956, + "total_allocations": 1301497 }, "fork_stream_cleanup": { - "peak_bytes": 3464063, - "leaked_bytes": 3291969, - "total_allocations": 105340 + "peak_bytes": 3532824, + "leaked_bytes": 3361106, + "total_allocations": 110397 }, "fork_swap_cleanup": { - "peak_bytes": 3681171, - "leaked_bytes": 3350696, - "total_allocations": 714376 + "peak_bytes": 3753679, + "leaked_bytes": 3421936, + "total_allocations": 754042 }, "fork_contended_mutex_swap": { - "peak_bytes": 7302379, - "leaked_bytes": 3475147, - "total_allocations": 35948516 + "peak_bytes": 7360409, + "leaked_bytes": 3525035, + "total_allocations": 37359891 }, "fork_contended_mutex_wrap": { - "peak_bytes": 7288748, - "leaked_bytes": 3463411, - "total_allocations": 34847186 + "peak_bytes": 7140341, + "leaked_bytes": 3522965, + "total_allocations": 34204380 }, "fork_consumed_signer": { - "peak_bytes": 3350894, - "leaked_bytes": 3288906, - "total_allocations": 175055 + "peak_bytes": 3421645, + "leaked_bytes": 3359803, + "total_allocations": 206540 }, "swap_chain_churn": { - "peak_bytes": 3681161, - "leaked_bytes": 3350287, - "total_allocations": 672537 + "peak_bytes": 3753669, + "leaked_bytes": 3421527, + "total_allocations": 679964 + }, + "deferred_teardown_flush_queue": { + "peak_bytes": 4103247, + "leaked_bytes": 3412690, + "total_allocations": 2448668 } } \ No newline at end of file diff --git a/tests/perf/run_thread_profile.py b/tests/perf/run_thread_profile.py new file mode 100644 index 00000000..39d1a162 --- /dev/null +++ b/tests/perf/run_thread_profile.py @@ -0,0 +1,292 @@ +#!/usr/bin/env python3 +# Copyright 2026 Adobe. All rights reserved. +# This file is licensed to you under the Apache License, +# Version 2.0 (http://www.apache.org/licenses/LICENSE-2.0) +# or the MIT license (http://opensource.org/licenses/MIT), +# at your option. + +""" +Thread-safety invariant harness. + +For each scenario in thread_scenarios.THREAD_SCENARIOS this script: +- Runs the scenario in a subprocess with faulthandler armed +- Reads the scenario's outcome counters from the subprocess's last stdout line +- Fails the run unless every round produced the expected outcome + +There is no baseline file. Each scenario asserts a fixed invariant of the +binding, so there is nothing to drift against: the expected value is declared in +the registry beside the scenario. + +The scenarios force a teardown to run while a native call is still open, and then +check the guard state instead of waiting for a use-after-free to fault. A fault +only happens when the allocator has reused the freed page, so crash-based detection +is probabilistic; a guard-state assertion is deterministic. + +A scenario that cannot reach its injection point reports NOT_PARKED and fails, +because a scenario that stops forcing anything would otherwise keep passing +while testing nothing. + +Usage: + python -m tests.perf.run_thread_profile [--scenario NAME] + python -m tests.perf.run_thread_profile --self-test + python -m tests.perf.run_thread_profile --list + +Environment variables: +- THREAD_ROUNDS: rounds each scenario loops (default: 20) +- THREAD_HANG_TIMEOUT: seconds before a stuck scenario is dumped and killed + (default: 120) +""" + +import argparse +import json +import os +import signal +import subprocess +import sys +from pathlib import Path + +from tests.perf.thread_scenarios import ( + THREAD_SCENARIO_NAMES, + THREAD_SCENARIOS, +) + +HERE = Path(__file__).parent +REPORTS_DIR = HERE / "reports" + +ROUNDS = int(os.environ.get("THREAD_ROUNDS", "20")) +HANG_TIMEOUT = int(os.environ.get("THREAD_HANG_TIMEOUT", "120")) +PERF_ENV = os.environ.get("PERF_ENV", "") + +# Signals that mean native memory corruption. A container reports a signalled +# exit as 128+N while a direct child reports -N, so both encodings are accepted. +# SIGTRAP is included because the macOS allocator traps rather than aborting. +_CRASH_SIGNALS = (signal.SIGSEGV, signal.SIGABRT, signal.SIGTRAP) +_CRASH_CODES = frozenset( + [-int(s) for s in _CRASH_SIGNALS] + [128 + int(s) for s in _CRASH_SIGNALS] +) + +# faulthandler prints this banner before dumping every thread on timeout. +_HANG_MARKER = "Timeout (" + +# Status values. A crash and an ordinary exception stay separate: an +# ImportError and a failed artifact download both surface as exit 1, and +# reporting either as a crash would invent a finding that does not exist. +_PASS = "pass" +_VIOLATED = "VIOLATED" +_CRASHED = "CRASHED" +_HUNG = "HUNG" +_FAILED = "FAILED" + + +def _child_script(name: str) -> str: + """Source for the subprocess that runs one scenario. + + faulthandler turns a crash or a hang into a traceback naming every thread, + which is the diagnostic for a guard that blocked instead of refusing. Its + timer thread is a C thread, so it still fires when the main thread is parked + inside a native call with the GIL released. + """ + repo_root = HERE.parent.parent + return f""" +import faulthandler, json, sys +faulthandler.enable() +faulthandler.dump_traceback_later({HANG_TIMEOUT}, exit=True) +sys.path.insert(0, {str(repo_root)!r}) +sys.path.insert(0, {str(repo_root / 'src')!r}) +from tests.perf.thread_scenarios import THREAD_SCENARIOS +counts = THREAD_SCENARIOS[{name!r}][0]({ROUNDS}) +faulthandler.cancel_dump_traceback_later() +print("COUNTS " + json.dumps(counts)) +""" + + +def _parse_counts(stdout: str): + """The counter dict from the child's COUNTS line, or None if absent.""" + for line in reversed(stdout.splitlines()): + if line.startswith("COUNTS "): + try: + return json.loads(line[len("COUNTS "):]) + except json.JSONDecodeError: + return None + return None + + +def _classify(returncode: int, counts, expected: str, stderr: str = ""): + """Map a finished child onto a status and a human-readable detail.""" + if returncode in _CRASH_CODES: + return _CRASHED, f"killed by signal (exit {returncode})" + + if returncode != 0: + # A hang and an ordinary exception both exit 1. Only faulthandler's + # timeout banner tells them apart, so match on it rather than guessing + # from the exit code. + if _HANG_MARKER in stderr: + return _HUNG, f"no progress for {HANG_TIMEOUT}s" + return _FAILED, f"exit {returncode}" + + if not counts: + return _FAILED, "scenario produced no counters" + + if set(counts) == {expected}: + return _PASS, f"{expected} x{counts[expected]}" + + unexpected = {k: v for k, v in counts.items() if k != expected} + return _VIOLATED, f"expected {expected} x{ROUNDS}, got {json.dumps(counts)}" \ + if expected not in counts else \ + f"expected only {expected}, also saw {json.dumps(unexpected)}" + + +def _run_scenario(name: str, expected: str): + """Run one scenario in a subprocess and classify the result.""" + proc = subprocess.run( + [sys.executable, "-c", _child_script(name)], + text=True, + capture_output=True, + env={**os.environ, "PERF_SCENARIO": name}, + ) + counts = _parse_counts(proc.stdout) + status, detail = _classify( + proc.returncode, counts, expected, proc.stderr) + + if status != _PASS and proc.stderr.strip(): + # The all-threads traceback is the diagnostic, so it has to reach both + # the log and the artifact. + print(proc.stderr.rstrip(), file=sys.stderr) + log_name = f"{name}-{PERF_ENV}-threads.log" if PERF_ENV else f"{name}-threads.log" + (REPORTS_DIR / log_name).write_text(proc.stderr, encoding="utf-8") + + return status, detail, counts + + +def _write_github_summary(results: dict) -> None: + """Append a results table to $GITHUB_STEP_SUMMARY when running in CI.""" + summary_path = os.environ.get("GITHUB_STEP_SUMMARY") + if not summary_path or not results: + return + + lines = [ + "## Threading invariants", + "", + f"Rounds: {ROUNDS}" + f"{f' · env: {PERF_ENV}' if PERF_ENV else ''}", + "", + "| scenario | rounds | expected | actual | status |", + "|----------|--------|----------|--------|--------|", + ] + for name, row in results.items(): + actual = json.dumps(row["counts"]) if row["counts"] else "-" + lines.append( + f"| {name} | {ROUNDS} | {THREAD_SCENARIOS[name][1]} " + f"| {actual} | {row['status']} |" + ) + lines.append("") + + with open(summary_path, "a", encoding="utf-8") as fh: + fh.write("\n".join(lines) + "\n") + + +# Synthetic children for --self-test. A detector that has never been shown to +# fire cannot be told apart from one that cannot fire, and the last case is the +# one that matters most: an exception must not be reported as a crash. +_SELF_TESTS = ( + ("segfault", "import faulthandler,ctypes; faulthandler.enable(); " + "ctypes.string_at(0)", _CRASHED), + ("abort", "import os,signal; os.kill(os.getpid(), signal.SIGABRT)", _CRASHED), + ("hang", "import faulthandler,threading; faulthandler.enable(); " + "faulthandler.dump_traceback_later(2, exit=True); " + "threading.Event().wait()", _HUNG), + ("exception", "raise RuntimeError('boom')", _FAILED), +) + + +def _self_test() -> int: + """Prove the classifier reports each failure class correctly.""" + print("=== self-test: classifier ===") + failures = [] + for label, code, want in _SELF_TESTS: + proc = subprocess.run( + [sys.executable, "-c", code], text=True, capture_output=True) + got, detail = _classify( + proc.returncode, _parse_counts(proc.stdout), "n/a", proc.stderr) + ok = got == want + print(f" {label:<10} want={want:<9} got={got:<9} ({detail}) " + f"{'ok' if ok else 'MISCLASSIFIED'}") + if not ok: + failures.append(f"{label}: wanted {want}, got {got}") + + if failures: + print("\nself-test FAILED - the harness cannot see the failures it " + "exists to catch:", file=sys.stderr) + for line in failures: + print(f" {line}", file=sys.stderr) + return 1 + print("self-test passed") + return 0 + + +def main() -> None: + parser = argparse.ArgumentParser( + description="c2pa-python thread-safety invariant harness") + parser.add_argument( + "--scenario", + choices=THREAD_SCENARIO_NAMES, + default=None, + help="Run a single scenario instead of all of them.", + ) + parser.add_argument( + "--list", + action="store_true", + help="List the scenarios with their expected outcomes and exit.", + ) + parser.add_argument( + "--self-test", + action="store_true", + help="Check that the harness classifies crashes, hangs and exceptions " + "correctly, then exit.", + ) + args = parser.parse_args() + + if args.list: + for name in THREAD_SCENARIO_NAMES: + print(f"{name}\texpects {THREAD_SCENARIOS[name][1]}") + return + + if args.self_test: + sys.exit(_self_test()) + + REPORTS_DIR.mkdir(parents=True, exist_ok=True) + scenarios_to_run = (args.scenario,) if args.scenario else THREAD_SCENARIO_NAMES + + # Print the knobs so a CI failure can be reproduced locally from the log. + print(f"rounds={ROUNDS} hang_timeout={HANG_TIMEOUT}s" + f"{f' env={PERF_ENV}' if PERF_ENV else ''}") + + results: dict = {} + failures: list[str] = [] + + total = len(scenarios_to_run) + for idx, name in enumerate(scenarios_to_run, 1): + expected = THREAD_SCENARIOS[name][1] + print(f"\n=== [{idx}/{total}] {name} (expects {expected}) ===", flush=True) + status, detail, counts = _run_scenario(name, expected) + results[name] = {"status": status, "detail": detail, "counts": counts} + print(f" {status}: {detail}", flush=True) + if status != _PASS: + failures.append(f"{name}: {status} - {detail}") + + _write_github_summary(results) + + print("\n=== summary ===") + for name, row in results.items(): + print(f" {row['status']:<9} {name}") + + if failures: + print(f"\n{len(failures)} scenario(s) failed:", file=sys.stderr) + for line in failures: + print(f" {line}", file=sys.stderr) + sys.exit(1) + print(f"\nall {total} invariant(s) hold") + + +if __name__ == "__main__": + main() diff --git a/tests/perf/scenarios.py b/tests/perf/scenarios.py index bdfd2795..9510f60f 100644 --- a/tests/perf/scenarios.py +++ b/tests/perf/scenarios.py @@ -594,8 +594,8 @@ def scenario_reader_with_fragment_pre_consume_rejection( # Fail loudly: without these the scenario still runs when the # ownership logic regresses, and a rejection that stops being # recognised looks identical to a pass. - if not any(tag in str(e) for tag in - c2pa_module.ManagedResource._PRE_CONSUME_ERROR_TAGS): + if not c2pa_module.ManagedResource._is_pre_consume_rejection( + str(e)): raise AssertionError( f"expected a pre-consume rejection, got: {e}") from e if reader._handle is None: @@ -1304,6 +1304,40 @@ def scenario_swap_chain_churn(iterations: int = 100) -> None: context.close() +def scenario_deferred_teardown_flush_queue(iterations: int = 100) -> None: + """Close resources from inside an open native-error section, so their + teardowns defer onto one pending list and are drained together when the + section closes. + + Two resources per iteration rather than one: a single-element queue cannot + show a resource stranded behind its predecessor. + """ + signed_bytes = SIGNED_JPEG.read_bytes() + real_free = c2pa_module.ManagedResource._free_native_ptr + for _ in _iterate(iterations): + first = Reader("image/jpeg", io.BytesIO(signed_bytes)) + second = Reader("image/jpeg", io.BytesIO(signed_bytes)) + + freed = [] + c2pa_module.ManagedResource._free_native_ptr = staticmethod( + lambda ptr: (freed.append(ptr), real_free(ptr))[1]) + try: + with c2pa_module._native_section(): + first.close() + second.close() + # Fail loudly: a free here means the teardown was not deferred. + if freed: + raise AssertionError( + "teardown inside a section freed immediately " + "instead of deferring") + if len(freed) != 2: + raise AssertionError( + f"drain freed {len(freed)} of 2 deferred handles; " + f"the rest leak") + finally: + c2pa_module.ManagedResource._free_native_ptr = real_free + + def scenario_fork_swap_cleanup(iterations: int = 100) -> None: """Fork safety benchmark scenario: the handle a Builder owns at fork time came from with_archive(), which @@ -1410,6 +1444,7 @@ def scenario_fork_stream_cleanup(iterations: int = 100) -> None: "fork_contended_mutex_wrap": scenario_fork_contended_mutex_wrap, "fork_consumed_signer": scenario_fork_consumed_signer, "swap_chain_churn": scenario_swap_chain_churn, + "deferred_teardown_flush_queue": scenario_deferred_teardown_flush_queue, } diff --git a/tests/perf/thread_scenarios.py b/tests/perf/thread_scenarios.py new file mode 100644 index 00000000..5bfa530e --- /dev/null +++ b/tests/perf/thread_scenarios.py @@ -0,0 +1,407 @@ +# Copyright 2026 Adobe. All rights reserved. +# This file is licensed to you under the Apache License, +# Version 2.0 (http://www.apache.org/licenses/LICENSE-2.0) +# or the MIT license (http://opensource.org/licenses/MIT), +# at your option. + +""" +Plain functions (no pytest dependencies) asserting thread-safety invariants. +Each function is called once by run_thread_profile.py and loops internally. + +These scenarios do not wait for a crash. A native use-after-free only faults when +the allocator happens to have reused the freed page, which makes crash detection +probabilistic (measured at p=0.00124 per round of sustained load). Instead each +scenario forces the dangerous interleaving and then asserts the guard state that +the hardening maintains, which is deterministic. + +The forcing primitive is a stream or signer callback that blocks on an Event. A +callback runs on the thread that entered the native call, so blocking inside one +holds that native call open with the GIL released. A teardown issued from another +thread then lands mid-call by construction rather than by luck. + +Every scenario reports the outcome of each round as a counter dict, and reports +NOT_PARKED when its callback never ran. A scenario whose injection stops working +would otherwise keep passing while testing nothing. +""" + +import io +import json +import threading + +from cryptography.hazmat.primitives import hashes, serialization +from cryptography.hazmat.primitives.asymmetric import ec + +from c2pa import ( + Builder, + C2paSigningAlg, + Context, + Reader, + Signer, +) +import c2pa.c2pa as c2pa_module + +from tests.perf.scenarios import ( + FIXTURES_DIR, + MANIFEST_BASE, + SIGNED_JPEG, + SOURCE_JPEG, +) + +# How long a scenario waits for its callback to be entered before giving up and +# reporting NOT_PARKED, and how long it then holds the native call open. +_PARK_TIMEOUT = 30.0 +_RELEASE_TIMEOUT = 40.0 +_JOIN_TIMEOUT = 40.0 + +_TSA_URL = "http://timestamp.digicert.com" + +_CERTS = (FIXTURES_DIR / "es256_certs.pem").read_bytes().decode("utf-8") +_PRIVATE_KEY = (FIXTURES_DIR / "es256_private.key").read_bytes() + +NOT_PARKED = "NOT_PARKED" + + +class ParkingStream(io.RawIOBase): + """Stream that blocks once inside a callback, holding a native call open. + + On the first read (or write) past the start of the data it sets `inside` and + waits on `release`. The callback runs on whichever thread entered the native + call, so while it waits that call is open with the GIL dropped and another + thread's teardown is guaranteed to arrive mid-call. + """ + + def __init__(self, data: bytes, inside, release, *, for_write: bool = False): + self._buffer = io.BytesIO(b"" if for_write else data) + self._inside = inside + self._release = release + self._for_write = for_write + self._parked = False + self._written = 0 + + def readable(self) -> bool: + return not self._for_write + + def writable(self) -> bool: + return self._for_write + + def seekable(self) -> bool: + return True + + def seek(self, offset, whence=0): + if self._for_write: + return self._written + return self._buffer.seek(offset, whence) + + def tell(self): + if self._for_write: + return self._written + return self._buffer.tell() + + def _park_once(self) -> None: + if self._parked: + return + self._parked = True + self._inside.set() + self._release.wait(_RELEASE_TIMEOUT) + + def read(self, size=-1): + # Park only after the first chunk, so the native side is already holding + # the handle rather than still validating arguments. + if self._buffer.tell() > 0: + self._park_once() + return self._buffer.read(size) + + def readinto(self, target): + chunk = self.read(len(target)) + count = len(chunk) + target[:count] = chunk + return count + + def write(self, data): + self._written += len(data) + self._park_once() + return len(data) + + +def parking_sign_callback(inside, release): + """Signer callback that parks while native is calling through its trampoline. + + The trampoline is an ordinary refcounted Python object whose only reference is + an attribute on the Context. Parking here means a Context teardown runs while + native holds nothing but the trampoline's address. + """ + + def callback(data: bytes) -> bytes: + inside.set() + release.wait(_RELEASE_TIMEOUT) + key = serialization.load_pem_private_key(_PRIVATE_KEY, password=None) + assert isinstance(key, ec.EllipticCurvePrivateKey) + return key.sign(data, ec.ECDSA(hashes.SHA256())) + + return callback + + +def _callback_signer(inside, release) -> Signer: + """A Signer whose callback parks. Signer.from_info has no Python trampoline, + so only from_callback exercises the trampoline lifetime.""" + return Signer.from_callback( + callback=parking_sign_callback(inside, release), + alg=C2paSigningAlg.ES256, + certs=_CERTS, + tsa_url=_TSA_URL, + ) + + +def _first_resource_uri(reader: Reader): + """A resource identifier from the reader's manifest, or None. + + resource_to_stream needs one, and it is the shared borrow that + no_free_during_parked_call parks inside. + """ + manifest = json.loads(reader.json()) + for entry in manifest.get("manifests", {}).values(): + thumbnail = entry.get("thumbnail") or {} + identifier = thumbnail.get("identifier") + if identifier: + return identifier + return None + + +def _close_quietly(resource) -> None: + try: + resource.close() + except Exception: + pass + + +class _ParkedResourceCall: + """Runs resource_to_stream on a worker thread and parks inside its callback. + + Used as a context manager: the body runs while the native call is open. On + entry `parked` says whether the callback was reached; when it is False the + body must not assert anything. + """ + + def __init__(self, reader: Reader, uri: str): + self._reader = reader + self._uri = uri + self._inside = threading.Event() + self._release = threading.Event() + self._thread = None + self.parked = False + + def __enter__(self): + stream = ParkingStream(b"", self._inside, self._release, for_write=True) + + def worker(): + try: + self._reader.resource_to_stream(self._uri, stream) + except Exception: + # This call can fail once the resource is torn down. Scenarios + # assert on the guard state the with-block observes, not on + # this exception. + pass + + self._thread = threading.Thread(target=worker, name="parked-resource-call") + self._thread.start() + self.parked = self._inside.wait(_PARK_TIMEOUT) + return self + + def __exit__(self, exc_type, exc, tb): + self._release.set() + if self._thread is not None: + self._thread.join(_JOIN_TIMEOUT) + return False + + @property + def hung(self) -> bool: + return self._thread is not None and self._thread.is_alive() + + +def _tally(rounds: int, one_round): + """Run one_round() `rounds` times and count the outcomes.""" + counts: dict = {} + for _ in range(rounds): + try: + outcome = one_round() + except Exception as err: + # A scenario raising here is a bug in the scenario, not the + # invariant it is checking. + outcome = f"ERROR:{type(err).__name__}" + counts[outcome] = counts.get(outcome, 0) + 1 + return counts + + +def scenario_trampoline_held_during_sign(rounds: int = 20) -> dict: + """The signer trampoline must outlive a Context closed mid-sign. + + Context._release() drops its reference to the trampoline. If that happens + while native is calling through it, native is left calling freed memory, and + a sign that never invoked the signer can still report success. + + HELD: the in-flight guard kept the trampoline alive. + DROPPED: the only reference was dropped mid-call. + """ + source = SOURCE_JPEG.read_bytes() + manifest = {**MANIFEST_BASE, "format": "image/jpeg"} + + def one_round(): + inside = threading.Event() + release = threading.Event() + signer = _callback_signer(inside, release) + context = Context(signer=signer) + result: dict = {} + + def worker(): + try: + result["manifest"] = Builder(manifest, context=context).sign( + "image/jpeg", io.BytesIO(source)) + except Exception as err: + result["error"] = type(err).__name__ + + thread = threading.Thread(target=worker, name="parked-sign") + thread.start() + if not inside.wait(_PARK_TIMEOUT): + release.set() + thread.join(_JOIN_TIMEOUT) + _close_quietly(context) + return NOT_PARKED + + # Native is inside the trampoline right now. + _close_quietly(context) + held = getattr(context, "_signer_callback_cb", None) is not None + + release.set() + thread.join(_JOIN_TIMEOUT) + if thread.is_alive(): + return "HUNG" + return "HELD" if held else "DROPPED" + + return _tally(rounds, one_round) + + +def scenario_no_free_during_parked_call(rounds: int = 20) -> dict: + """A handle must not be freed while a native call still holds it. + + c2pa_free is reached through the single funnel ManagedResource._free_native_ptr. + A round only counts once its callback has parked, which confirms the call is + still open, so counting frees from that point measures the use-after-free + directly rather than waiting for it to fault. + + freed=0: the teardown was deferred until the call returned. + freed=1: the in-use handle was freed mid-call. + """ + signed = SIGNED_JPEG.read_bytes() + + def one_round(): + reader = Reader("image/jpeg", io.BytesIO(signed)) + reader.json() + uri = _first_resource_uri(reader) + if uri is None: + _close_quietly(reader) + return "NO_RESOURCE_URI" + + freed_while_open = [] + watching = {"active": False} + original = c2pa_module.ManagedResource.__dict__["_free_native_ptr"] + underlying = getattr(original, "__func__", original) + + def traced(ptr): + if watching["active"]: + freed_while_open.append(ptr) + return underlying(ptr) + + c2pa_module.ManagedResource._free_native_ptr = staticmethod(traced) + try: + with _ParkedResourceCall(reader, uri) as parked: + if not parked.parked: + return NOT_PARKED + watching["active"] = True + _close_quietly(reader) + watching["active"] = False + count = len(freed_while_open) + return f"freed={count}" + finally: + # Restore even on failure: leaving the trace installed would corrupt + # every later scenario in this process. + c2pa_module.ManagedResource._free_native_ptr = staticmethod(underlying) + + return _tally(rounds, one_round) + + +def scenario_builder_no_free_during_parked_sign(rounds: int = 20) -> dict: + """The Builder handle must not be freed while c2pa_builder_sign_context + still holds it, parked inside the Context signer's callback. + + freed=0: the teardown was deferred until sign returned. + freed=1: the in-use handle was freed mid-call. + """ + source = SOURCE_JPEG.read_bytes() + manifest = {**MANIFEST_BASE, "format": "image/jpeg"} + + def one_round(): + inside = threading.Event() + release = threading.Event() + signer = _callback_signer(inside, release) + context = Context(signer=signer) + builder = Builder(manifest, context=context) + + freed_while_open = [] + watching = {"active": False} + original = c2pa_module.ManagedResource.__dict__["_free_native_ptr"] + underlying = getattr(original, "__func__", original) + + def traced(ptr): + if watching["active"]: + freed_while_open.append(ptr) + return underlying(ptr) + + c2pa_module.ManagedResource._free_native_ptr = staticmethod(traced) + result: dict = {} + + def worker(): + try: + result["manifest"] = builder.sign( + "image/jpeg", io.BytesIO(source)) + except Exception as err: + result["error"] = type(err).__name__ + + thread = threading.Thread(target=worker, name="parked-builder-sign") + thread.start() + try: + if not inside.wait(_PARK_TIMEOUT): + release.set() + thread.join(_JOIN_TIMEOUT) + return NOT_PARKED + + watching["active"] = True + _close_quietly(builder) + watching["active"] = False + count = len(freed_while_open) + + release.set() + thread.join(_JOIN_TIMEOUT) + if thread.is_alive(): + return "HUNG" + return f"freed={count}" + finally: + c2pa_module.ManagedResource._free_native_ptr = staticmethod( + underlying) + _close_quietly(context) + + return _tally(rounds, one_round) + + +# Scenario name -> (function, expected outcome on hardened code). +# The expected value is what the driver asserts; anything else fails the run. +THREAD_SCENARIOS = { + "trampoline_held_during_sign": ( + scenario_trampoline_held_during_sign, "HELD"), + "no_free_during_parked_call": ( + scenario_no_free_during_parked_call, "freed=0"), + "builder_no_free_during_parked_sign": ( + scenario_builder_no_free_during_parked_sign, "freed=0"), +} + +# Derived so the name list cannot drift from the registry. +THREAD_SCENARIO_NAMES = tuple(THREAD_SCENARIOS) diff --git a/tests/test_unit_tests.py b/tests/test_unit_tests.py index bc925aad..dd3c3ec3 100644 --- a/tests/test_unit_tests.py +++ b/tests/test_unit_tests.py @@ -11,6 +11,7 @@ # specific language governing permissions and limitations under # each license. +import ast import gc import inspect import os @@ -28,8 +29,11 @@ from cryptography.hazmat.backends import default_backend import tempfile import shutil +import subprocess +import sys import ctypes import threading +from unittest.mock import patch # Suppress deprecation warnings warnings.simplefilter("ignore", category=DeprecationWarning) @@ -50,6 +54,23 @@ ALTERNATIVE_INGREDIENT_TEST_FILE = os.path.join(FIXTURES_DIR, "cloud.jpg") +def _patch_free(test, fn): + """Route ManagedResource._free_native_ptr to `fn` until `test` ends.""" + patcher = patch.object( + ManagedResource, '_free_native_ptr', staticmethod(fn)) + patcher.start() + test.addCleanup(patcher.stop) + + +def _fail_with_native_error(tag_bytes): + """Build a mock FFI callable that sets a native error and returns None. + """ + def _mock(*args): + c2pa_module._lib.c2pa_error_set_last(tag_bytes) + return None + return _mock + + def load_test_settings_json(): """ Load default (legacy) trust configuration test settings from a @@ -1372,7 +1393,6 @@ def test_sign_and_read_is_not_embedded(self): # Direct the Builder not to embed the manifest into the asset builder.set_no_embed() - with open(temp_file_path, "wb") as temp_file: manifest_data = builder.sign( signer, "image/jpeg", file, temp_file) @@ -6665,6 +6685,18 @@ def test_settings_update_dict(self): self.assertIs(result, settings) settings.close() + def test_settings_update_rejects_nul_in_json_string(self): + settings = Settings.from_dict({ + "builder": {"thumbnail": {"enabled": True}}, + }) + try: + with self.assertRaises(Error): + settings.update( + '{"verify": {"verify_after_sign": true}}\x00' + '{"builder": {"thumbnail": {"enabled": false}}}') + finally: + settings.close() + def test_settings_is_valid_after_close(self): settings = Settings() settings.close() @@ -7919,7 +7951,7 @@ def test_callbacks_return_minus_one_after_stream_collected(self): class TestManagedResourceLifecycle(unittest.TestCase): - """Lifecycle primitives (_activate, _swap_handle, _wrap_native_handle), + """Lifecycle primitives (_activate, _consume_and_swap, _wrap_native_handle), the _owner_pid stamp that governs which process may free a handle, and the ownership hand-offs between Python and the native library. @@ -7972,10 +8004,14 @@ def setUp(self): self.data_dir = FIXTURES_DIR self.freed = [] self._real_free = ManagedResource._free_native_ptr - ManagedResource._free_native_ptr = staticmethod(self.freed.append) + # Registered first so it runs last, after every patch has unwound. + self.addCleanup(self._assert_free_hook_restored) + _patch_free(self, self.freed.append) - def tearDown(self): - ManagedResource._free_native_ptr = self._real_free + def _assert_free_hook_restored(self): + self.assertIs( + ManagedResource._free_native_ptr, self._real_free, + "{} leaked a _free_native_ptr patch".format(self.id())) def _free_counts(self): counts = {} @@ -7985,7 +8021,7 @@ def _free_counts(self): def _use_real_frees(self): """Undo free recorder, so native handles are really freed.""" - ManagedResource._free_native_ptr = self._real_free + _patch_free(self, self._real_free) def _make_signer(self): with open(os.path.join(self.data_dir, "es256_certs.pem"), "rb") as f: @@ -8064,41 +8100,18 @@ def test_activate_does_not_mutate_on_rejection(self): "rejected activation replaced the handle") self.assertEqual(res._lifecycle_state, LifecycleState.ACTIVE) - def test_swap_handle_does_not_free_consumed_handle(self): - res = self._FakeHandleResource() - res._activate(0xAAA1) - - res._swap_handle(0xAAA2) - - # The FFI already owns and frees the old pointer. - self.assertEqual(self.freed, []) - self.assertEqual(res._handle, 0xAAA2) - - res.close() - self.assertEqual(self.freed, [0xAAA2]) - - def test_swap_handle_requires_active_resource(self): + def test_consume_and_swap_requires_active_resource(self): uninitialized = self._FakeHandleResource() - with self.assertRaises(Error) as ctx: - uninitialized._swap_handle(0x1) - self.assertIn("not active", str(ctx.exception)) + with self.assertRaises(Error): + uninitialized._consume_and_swap(lambda h: 0x1, "swap: {}") closed = self._FakeHandleResource() closed._activate(0x2) closed.close() + self.freed.clear() with self.assertRaises(Error): - closed._swap_handle(0x3) - - def test_swap_handle_rejects_null_replacement(self): - res = self._FakeHandleResource() - res._activate(0x7777) - - with self.assertRaises(Error) as ctx: - res._swap_handle(None) - - self.assertIn("null handle", str(ctx.exception)) - self.assertEqual(res._handle, 0x7777) - self.assertEqual(res._lifecycle_state, LifecycleState.ACTIVE) + closed._consume_and_swap(lambda h: 0x3, "swap: {}") + self.assertEqual(self.freed, []) def test_wrap_native_handle_bypasses_init(self): seen = [] @@ -8153,7 +8166,7 @@ def test_every_construction_path_records_owner_pid(self): # A swap keeps the original stamp: # the replacement handle was allocated by the same process # that created the object. - wrapped._swap_handle(0xA3) + wrapped._consume_and_swap(lambda h: 0xA3, "swap: {}") self.assertEqual(wrapped._owner_pid, pid) def test_foreign_child_skips_free_for_wrapped_and_swapped(self): @@ -8163,7 +8176,7 @@ def test_foreign_child_skips_free_for_wrapped_and_swapped(self): swapped = self._FakeHandleResource() swapped._activate(0xC2) - swapped._swap_handle(0xC3) + swapped._consume_and_swap(lambda h: 0xC3, "swap: {}") swapped._owner_pid = os.getpid() + 1 swapped.close() @@ -8189,7 +8202,7 @@ def test_owning_process_frees_wrapped_and_swapped_exactly_once(self): swapped = self._FakeHandleResource() swapped._activate(0xC5) - swapped._swap_handle(0xC6) + swapped._consume_and_swap(lambda h: 0xC6, "swap: {}") swapped.close() # 0xC5 was consumed by the test FFI swap. @@ -8335,12 +8348,9 @@ def test_context_build_failure_consumes_signer(self): # Nothing left to free, so close() must be a no-op. freed = [] - real_free = ManagedResource._free_native_ptr - ManagedResource._free_native_ptr = staticmethod(freed.append) - try: + with patch.object(ManagedResource, '_free_native_ptr', + staticmethod(freed.append)): signer.close() - finally: - ManagedResource._free_native_ptr = real_free self.assertEqual(freed, []) def test_context_with_signer_consumes_it_on_success(self): @@ -8375,12 +8385,11 @@ def test_construction_failure_leaves_nothing_to_free(self): c2pa_module._lib.c2pa_builder_from_json = real_json def test_context_build_null_return_frees_builder(self): - # Set a pre-consume tag in the error slot to mock a pointer rejection. + # Mock a pointer rejection. settings = Settings() - c2pa_module._lib.c2pa_error_set_last( - b"UntrackedPointer: mocked pre-consume rejection") real_build = c2pa_module._lib.c2pa_context_builder_build - c2pa_module._lib.c2pa_context_builder_build = lambda ptr: None + c2pa_module._lib.c2pa_context_builder_build = _fail_with_native_error( + b"UntrackedPointer: mocked pre-consume rejection") try: with self.assertRaises(Error): Context(settings=settings) @@ -8405,22 +8414,28 @@ def test_consume_no_replacement_marks_consumed_on_success(self): 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 + for message in ("UntrackedPointer: rejected", + "PointerInUse: 0x2a", + "Other: ForeignProcess: 0x2a"): + with self.subTest(message=message): + self.freed.clear() + res = self._FakeHandleResource() + res._activate(0xCAFE) + real_read = c2pa_module._read_native_error + c2pa_module._read_native_error = lambda: message + 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]) + # 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() @@ -8439,6 +8454,92 @@ def test_consume_no_replacement_marks_consumed_on_other_error(self): self.assertIsNone(res._handle) self.assertEqual(res._lifecycle_state, LifecycleState.CLOSED) + def test_invoke_consume_success_does_not_consult_error_slot(self): + """A successful consuming call must not read the error slot at all: + only a failure inspects it.""" + res = self._FakeHandleResource() + res._activate(0xCAFE) + + res._consume_no_replacement(lambda h: 0, "set failed: {}") + + self.assertIsNone(c2pa_module._read_native_error()) + + def test_native_section_defers_unrelated_finalizer_free(self): + """A finalizer for a completely unrelated resource firing mid + native-call must not free immediately. + """ + victim = self._FakeHandleResource() + victim._activate(0xCAFE) + bystander = self._FakeHandleResource() + bystander._activate(0xB00B) + + def polluting_free(ptr): + self.freed.append(ptr) + # Freeing and untracked/ pointer writes its own error into the + # same thread-local slot. + c2pa_module._lib.c2pa_error_set_last( + "Other: UntrackedPointer: {:#x}".format(ptr).encode()) + return -1 + _patch_free(self, polluting_free) + + def ffi_call(handle): + nonlocal bystander + del bystander # last reference dropped: __del__ fires right here + return None # the real call failed but set no error of its own + + # A bare section: the consume needs the error section, but not a + # borrow on its own handle. _ensure_not_borrowed + # refuses a consume nested in a _native_call() on the same resource. + with c2pa_module._native_section(): + with self.assertRaises(Error): + victim._consume_no_replacement(ffi_call, "op failed: {}") + + # The bystander's free is deferred to the section close, so it + # runs after the consuming call, before the victim's free. + self.assertEqual(self.freed, [0xB00B, 0xCAFE]) + + def test_nested_native_sections_flush_only_at_outermost_close(self): + """A native-error section opened inside another, already-open one + on the same thread must not flush anything until the outermost + one closes.""" + res = self._FakeHandleResource() + res._activate(0xCAFE) + + outer = c2pa_module._native_section() + outer.__enter__() + try: + inner = c2pa_module._native_section() + inner.__enter__() + try: + res.close() + finally: + inner.__exit__(None, None, None) + # Inner closed, outer is still open: still deferred. + self.assertEqual(self.freed, [], "inner section flushed") + finally: + outer.__exit__(None, None, None) + self.assertEqual(self.freed, [0xCAFE]) + + def test_stale_error_not_misattributed_after_preset_error(self): + """A stale tag left by an earlier, unrelated call on this thread + must not be read as this call's own error.""" + # A stale tag from an earlier, unrelated call. + c2pa_module._lib.c2pa_error_set_last( + b"Other: UntrackedPointer: 0xdeadbeef") + + res = self._FakeHandleResource() + res._activate(0xCAFE) + + # Fails without setting any error of its own. + # The marker written inside _invoke_consume must have cleared + # the stale tag, so this routes to the "no error of our own" branch. + with self.assertRaises(Error): + res._consume_no_replacement(lambda h: -1, "op failed: {}") + + # A misattributed stale tag would have matched + # _PRE_CONSUME_ERROR_TAGS and left the resource ACTIVE. + self.assertEqual(self.freed, [0xCAFE]) + class TestManagedResourceObjects(TestContextAPIs): """Tests native resource handling management when managed manually. @@ -8459,11 +8560,7 @@ def _instrument_frees(self): """Record frees instead of performing them, and restore on teardown. """ freed = [] - real_free = ManagedResource._free_native_ptr - ManagedResource._free_native_ptr = staticmethod(freed.append) - self.addCleanup( - lambda: setattr( - ManagedResource, '_free_native_ptr', real_free)) + _patch_free(self, freed.append) return freed def _free_count(self, freed, handle): @@ -8704,9 +8801,9 @@ def test_builder_with_archive_null_return_marks_consumed(self): # Mimic a non-tag error: native took ownership then failed and dropped # the value itself, so the handle is marked consumed, not freed. - c2pa_module._lib.c2pa_error_set_last(b"Other: mocked test error") real_call = c2pa_module._lib.c2pa_builder_with_archive - c2pa_module._lib.c2pa_builder_with_archive = lambda b, s: None + c2pa_module._lib.c2pa_builder_with_archive = _fail_with_native_error( + b"Other: mocked test error") # Instrument before the failure... freed = self._instrument_frees() @@ -8740,11 +8837,9 @@ def test_reader_with_fragment_null_return_marks_consumed(self): # Mimic a non-tag error: native took ownership then failed and dropped # the value itself, so the handle is marked consumed, not freed. - c2pa_module._lib.c2pa_error_set_last(b"Other: mocked test error") - real_call = c2pa_module._lib.c2pa_reader_with_fragment - c2pa_module._lib.c2pa_reader_with_fragment = ( - lambda r, f, s, frag: None) + c2pa_module._lib.c2pa_reader_with_fragment = _fail_with_native_error( + b"Other: mocked test error") # Instrument before failure so any free would be counted. freed = self._instrument_frees() @@ -8812,11 +8907,11 @@ def _raise(*_args): @staticmethod def _is_pre_consume_rejection(error_message): - """True if this native error means ownership never transferred.""" + """True if this native error means ownership never transferred. + """ if not error_message: return False - return any(tag in error_message - for tag in ManagedResource._PRE_CONSUME_ERROR_TAGS) + return ManagedResource._is_pre_consume_rejection(error_message) def _stale_reader_handle(self): """A freed, untracked pointer, captured before close() nulls it. @@ -8842,31 +8937,6 @@ def _untracked_reader_handle(): return (ctypes.cast(buf, ctypes.POINTER(c2pa_module.C2paReader)), buf) - def test_with_fragment_pre_consume_rejection_keeps_handle(self): - # Rejected before native lib took ownership, - # so nothing was consumed and the handle is still ours. - init_path = os.path.join(FIXTURES_DIR, "dashinit.mp4") - fragment_path = os.path.join(FIXTURES_DIR, "dash1.m4s") - with open(init_path, "rb") as init: - reader = Reader("video/mp4", init) - real_handle = reader._handle - - reader._handle = self._stale_reader_handle() - try: - with open(init_path, "rb") as init, \ - open(fragment_path, "rb") as frag: - with self.assertRaises(Error) as caught: - reader.with_fragment("video/mp4", init, frag) - finally: - reader._handle = real_handle - - self.assertIn("UntrackedPointer", str(caught.exception)) - # Ownership never transferred, so the resource stays usable. - self.assertIsNotNone(reader._handle) - self.assertEqual(reader._lifecycle_state, LifecycleState.ACTIVE) - self.assertTrue(reader.json()) - reader.close() - def test_with_fragment_pre_consume_rejection_does_not_leak(self): # A handle dropped on this path leaks one reader per call. init_path = os.path.join(FIXTURES_DIR, "dashinit.mp4") @@ -8889,6 +8959,84 @@ def test_with_fragment_pre_consume_rejection_does_not_leak(self): self.assertTrue(reader.json()) reader.close() + def test_reader_with_empty_manifest_data_never_calls_native(self): + """End-to-end: the guard is wired into the public path, not just + available as a helper.""" + context = Context() + self.addCleanup(context.close) + with open(os.path.join(FIXTURES_DIR, + DEFAULT_TEST_FILE_NAME), "rb") as image: + image_bytes = image.read() + + freed = self._instrument_frees() + + with self.assertRaises(Error): + Reader("image/jpeg", io.BytesIO(image_bytes), + manifest_data=b"", context=context) + + # The guard raises before the FFI call, so the reader handle is still + # the binding's to free: exactly one free, and no abandoned handle. + self.assertEqual(len(freed), 1) + + def test_check_cstr_arg_rejects_none_and_embedded_nul(self): + """Both cases would reach native as something other than the caller + passed: None as a null pointer, an embedded NUL as a short string.""" + with self.assertRaises(Error): + c2pa_module._check_cstr_arg('format', None) + + with self.assertRaises(Error): + c2pa_module._check_cstr_arg('format', "image/\x00jpeg") + + c2pa_module._check_cstr_arg('format', "image/jpeg") + c2pa_module._check_cstr_arg('format', b"") + + def test_format_embeddable_null_out_pointer_raises_not_crashes(self): + real = c2pa_module._lib.c2pa_format_embeddable + c2pa_module._lib.c2pa_format_embeddable = ( + lambda fmt, data, size, out: 128) + try: + with self.assertRaises(Error): + format_embeddable("image/jpeg", b"junk") + finally: + c2pa_module._lib.c2pa_format_embeddable = real + + def test_check_bytes_arg_rejects_none_and_empty(self): + for bad in (None, b""): + with self.assertRaises(Error): + c2pa_module._check_bytes_arg('manifest_data', bad) + + c2pa_module._check_bytes_arg('manifest_data', b"x") + + def test_check_handle_arg_rejects_null(self): + """A null handle is a NullParameter on both native versions.""" + with self.assertRaises(Error): + c2pa_module._check_handle_arg('stream', None) + + c2pa_module._check_handle_arg( + 'stream', ctypes.cast(1, ctypes.c_void_p)) + + def test_repeated_with_fragment_does_not_accumulate_streams(self): + """Repeated with_fragment Reader calls should not accumulate streams. + """ + 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) + self.addCleanup(reader.close) + + superseded = [] + for _ in range(25): + with open(init_path, "rb") as init, \ + open(fragment_path, "rb") as frag: + reader.with_fragment("video/mp4", init, frag) + self.assertLessEqual(len(reader._fragment_streams), 1) + superseded.append(reader._fragment_streams[-1]) + + # Dropping the reference is not enough: the native stream is only + # released by close(), so every superseded wrapper must be closed. + self.assertTrue(all(s.closed for s in superseded[:-1])) + 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. @@ -8946,10 +9094,9 @@ def test_unknown_failure_drops_handle_without_freeing(self): consumed_handle = reader._handle # Simulate an error being set - c2pa_module._lib.c2pa_error_set_last(b"Other: mocked test error") real_call = c2pa_module._lib.c2pa_reader_with_fragment - c2pa_module._lib.c2pa_reader_with_fragment = ( - lambda r, f, s, frag: None) + c2pa_module._lib.c2pa_reader_with_fragment = _fail_with_native_error( + b"Other: mocked test error") try: with open(init_path, "rb") as init, \ open(fragment_path, "rb") as frag: @@ -8999,9 +9146,8 @@ def test_pre_consume_tags_still_match_the_native_wording(self): message = str(caught.exception) self.assertTrue( self._is_pre_consume_rejection(message), - f"the native rejection wording changed and no longer matches " - f"_PRE_CONSUME_ERROR_TAGS; ownership will be misjudged: " - f"{message!r}") + f"rejection wording does not match _PRE_CONSUME_ERROR_TAGS, " + f"so ownership will be misjudged: {message!r}") reader.close() def test_stale_handle_is_actually_rejected_every_time(self): @@ -9052,7 +9198,7 @@ def test_perf_scenario_bogus_handle_is_rejected(self): self.assertTrue( self._is_pre_consume_rejection(str(caught.exception)), - "the perf scenarios' bogus handle is no longer rejected, so " + "the perf bogus handle was not rejected, so " "with_fragment_pre_consume_rejection measures nothing") # Handle kept, so the reader still works and frees normally. self.assertEqual(reader._lifecycle_state, LifecycleState.ACTIVE) @@ -9060,17 +9206,15 @@ def test_perf_scenario_bogus_handle_is_rejected(self): reader.close() def test_every_null_return_sets_its_own_error(self): - # Reading the slot without clearing it is only sound because every - # null return sets an error. Check each path reports its own. + # Each null-returning path must report the error it set itself, never + # one left behind by an earlier call. init_path = os.path.join(FIXTURES_DIR, "dashinit.mp4") fragment_path = os.path.join(FIXTURES_DIR, "dash1.m4s") - # Leave a recognisable error behind, so anything stale shows up. - try: - Reader("image/jpeg", io.BytesIO(b"not an image")).json() - except Error: - pass - self.assertIn("NotSupported", c2pa_module._read_native_error() or "") + # Set a recognizable error, so anything stale is caught by the + # assertNotIn checks. + c2pa_module._lib.c2pa_error_set_last( + b"NotSupported: planted by the test") # Pre-consume rejection: reports UntrackedPointer, not NotSupported. with open(init_path, "rb") as init: @@ -9140,25 +9284,10 @@ def worker(): self.assertEqual(problems, [], "ownership was misjudged under concurrency") - def test_reading_the_native_error_does_not_empty_the_slot(self): - # c2pa_error() peeks, so nothing Python can call empties the slot. - # _consume_and_swap depends on this. - try: - Reader("image/jpeg", io.BytesIO(b"not an image")).json() - except Error: - pass - - first = c2pa_module._read_native_error() - self.assertTrue(first, "expected a native error to have been set") - - self.assertEqual( - c2pa_module._read_native_error(), first, - "reading emptied the native slot; the comments in " - "_consume_and_swap about a persistent error are now wrong") - def test_read_native_error_returns_none_for_an_empty_message(self): # c2pa_error() returns an owned pointer to "" when no error is set, # never NULL, so the pointer cannot be the "is there an error" test. + c2pa_module._write_no_error_marker() original = c2pa_module._lib.c2pa_error empty = ctypes.create_string_buffer(b"") @@ -9172,22 +9301,30 @@ def test_read_native_error_returns_none_for_an_empty_message(self): finally: c2pa_module._lib.c2pa_error = original - def test_mocked_null_without_error_is_a_known_limitation(self): - # A null with no error of its own is the case that breaks: the slot - # still holds whatever came before. No native path does this, so it - # is pinned here rather than defended in _consume_and_swap. + def test_null_return_with_no_native_error_is_treated_as_consumed(self): + # A null with no error of its own is the case that breaks without + # the marker: + # the slot still held whatever an unrelated, earlier call on this same + # (pooled) thread left behind, and a stale UntrackedPointer/ + # WrongPointerType tag would make this call believe it still owned a + # handle the native side already dropped. init_path = os.path.join(FIXTURES_DIR, "dashinit.mp4") fragment_path = os.path.join(FIXTURES_DIR, "dash1.m4s") + # A stale, unrelated tag left by a prior call on this thread. c2pa_module._lib.c2pa_error_set_last( b"UntrackedPointer: 0xdeadbeef") with open(init_path, "rb") as init: reader = Reader("video/mp4", init) + consumed_handle = reader._handle real_call = c2pa_module._lib.c2pa_reader_with_fragment + # The fake native call sets no error of its own, + # the marker planted by _invoke_consume is left in the slot. c2pa_module._lib.c2pa_reader_with_fragment = ( lambda r, f, s, frag: None) + freed = self._instrument_frees() try: with open(init_path, "rb") as init, \ open(fragment_path, "rb") as frag: @@ -9195,16 +9332,10 @@ def test_mocked_null_without_error_is_a_known_limitation(self): reader.with_fragment("video/mp4", init, frag) finally: c2pa_module._lib.c2pa_reader_with_fragment = real_call - # Nothing clears the slot, so a planted tag would follow other - # tests around and change how their failures are classified. - c2pa_module._lib.c2pa_error_set_last( - b"Other: cleared by test teardown") - # The stale tag wins, so the handle is kept. Safe here (the mock - # consumed nothing), and the reader is still usable. - self.assertIsNotNone(reader._handle) - self.assertEqual(reader._lifecycle_state, LifecycleState.ACTIVE) - reader.close() + # Ownership is unknown, so the handle is freed once. c2pa_free + # returns -1 if native had already taken the value. + self.assertEqual(self._free_count(freed, consumed_handle), 1) # Backfilling a pointer minted by a direct FFI call. Builder.from_archive # is the only production caller of _wrap_native_handle, so these are the @@ -9353,10 +9484,9 @@ def test_consumed_reader_closes_backing_file(self): self.assertFalse(backing_file.closed) # Simulate an error being set - c2pa_module._lib.c2pa_error_set_last(b"Other: mocked test error") real_call = c2pa_module._lib.c2pa_reader_with_fragment - c2pa_module._lib.c2pa_reader_with_fragment = ( - lambda r, f, s, frag: None) + c2pa_module._lib.c2pa_reader_with_fragment = _fail_with_native_error( + b"Other: mocked test error") try: with open(DEFAULT_TEST_FILE, "rb") as main, \ open(DEFAULT_TEST_FILE, "rb") as frag: @@ -9375,9 +9505,9 @@ def test_consumed_builder_releases_context(self): archive = self._make_archive() # Simulate an error being set - c2pa_module._lib.c2pa_error_set_last(b"Other: mocked test error") real_call = c2pa_module._lib.c2pa_builder_with_archive - c2pa_module._lib.c2pa_builder_with_archive = lambda b, s: None + c2pa_module._lib.c2pa_builder_with_archive = _fail_with_native_error( + b"Other: mocked test error") try: with self.assertRaises(Error): builder.with_archive(archive) @@ -9424,10 +9554,9 @@ def test_consumed_reader_clears_caches(self): self.assertIsNotNone(reader._manifest_json_str_cache) # Simulate an error being set - c2pa_module._lib.c2pa_error_set_last(b"Other: mocked test error") real_call = c2pa_module._lib.c2pa_reader_with_fragment - c2pa_module._lib.c2pa_reader_with_fragment = ( - lambda r, f, s, frag: None) + c2pa_module._lib.c2pa_reader_with_fragment = _fail_with_native_error( + b"Other: mocked test error") try: with open(DEFAULT_TEST_FILE, "rb") as main, \ open(DEFAULT_TEST_FILE, "rb") as frag: @@ -9499,7 +9628,6 @@ def _boom(*args): self.assertIs(ctx.exception.__cause__, sentinel, "signing error dropped the original exception") - class TestErrorPlumbing(unittest.TestCase): """Covers the error helpers themselves, which had no direct tests.""" @@ -9527,19 +9655,6 @@ def test_unmapped_tag_falls_back_to_base_error(self): # Base class only: no subclass should claim an unknown tag. self.assertIs(type(ctx.exception), Error) - def test_pre_consume_tag_match_is_substring_not_prefix(self): - """The tags arrive mid-string, so the match must stay a substring one. - - Guards the triage in _raise_consume_failure against being "cleaned up" - into error.startswith(tag), which would match nothing and silently - turn every retained handle into a consumed one. - """ - wire_error = "Other: UntrackedPointer: 0xdeadb000" - tags = ManagedResource._PRE_CONSUME_ERROR_TAGS - - self.assertTrue(any(tag in wire_error for tag in tags)) - self.assertFalse(any(wire_error.startswith(tag) for tag in tags)) - def test_check_ffi_operation_result_raises_with_native_message(self): self._set_native_error("Io: disk exploded") with self.assertRaises(Error) as ctx: @@ -9595,9 +9710,7 @@ def run(): t.join() self.assertEqual(len(captured), 1, "expected a raise on failure") - self.assertIsInstance( - captured[0], Error, - "stream failure must raise C2paError, not bare Exception") + self.assertIsInstance(captured[0], Error, "must be typed, not bare") self.assertNotIn("None", str(captured[0])) finally: c2pa_module._lib.c2pa_create_stream = real @@ -9620,9 +9733,7 @@ def run(): t.start() t.join() - self.assertIsInstance( - captured[0], Error, - "a failed MIME lookup returned data instead of raising") + self.assertIsInstance(captured[0], Error, "failed lookup returned data") def test_supported_mime_types_reports_the_native_message(self): self._set_native_error("Io: mime lookup failed") @@ -9630,6 +9741,174 @@ def test_supported_mime_types_reports_the_native_message(self): c2pa_module._get_supported_mime_types(lambda count: None, None) self.assertIn("mime lookup failed", str(ctx.exception)) + def test_reading_an_error_does_not_leave_it_readable(self): + """An error is reportable once, by the reader that observes it. + """ + self._set_native_error("Io: read me once") + + self.assertEqual( + c2pa_module._read_native_error(), "Io: read me once") + self.assertIsNone(c2pa_module._read_native_error(), "reported twice") + + def test_import_learns_no_marker_before_first_use(self): + result = subprocess.run( + [sys.executable, "-c", + "import c2pa.c2pa as m\n" + "assert m._marker_text_for.cache_info().currsize == 0, " + "'learned at import'\n" + "m._read_native_error()\n" + "assert m._marker_text(), 'never learned on first use'\n"], + capture_output=True, text=True, timeout=120) + self.assertEqual(result.returncode, 0, result.stderr[-2000:]) + + def test_the_no_error_marker_never_reaches_a_caller(self): + """The marker is internal, not a message for users.""" + marker = c2pa_module._marker_text() + + c2pa_module._write_no_error_marker() + self.assertIsNone(c2pa_module._read_native_error()) + + c2pa_module._write_no_error_marker() + with self.assertRaises(Error) as ctx: + c2pa_module._check_ffi_operation_result(None, "fallback: {}") + self.assertNotIn(marker, str(ctx.exception)) + + def test_a_failure_after_a_null_read_does_not_inherit_the_old_message(self): + """The message surviving a NULL read must not become someone's error.""" + c2pa_module._write_no_error_marker() + c2pa_module._lib.c2pa_error_set_last(b"Io: belongs to an earlier call") + + original = c2pa_module._lib.c2pa_error + try: + c2pa_module._lib.c2pa_error = lambda: None + c2pa_module._read_native_error() + finally: + c2pa_module._lib.c2pa_error = original + + with self.assertRaises(Error) as ctx: + c2pa_module._check_ffi_operation_result( + None, "Later unrelated failure: {}") + + self.assertNotIn("belongs to an earlier call", str(ctx.exception)) + + def test_every_real_rejection_wording_is_classified_as_pre_consume(self): + """Every tag arrives bare or behind the "Other: " wrapper.""" + wrapper = c2pa_module.ManagedResource._NATIVE_ERROR_WRAPPER + classify = c2pa_module.ManagedResource._is_pre_consume_rejection + + for tag in c2pa_module.ManagedResource._PRE_CONSUME_ERROR_TAGS: + bare = f"{tag} some detail" + wrapped = f"{wrapper}{tag} some detail" + self.assertTrue(classify(bare), bare) + self.assertTrue(classify(wrapped), wrapped) + + def test_caller_text_quoting_a_tag_is_not_a_rejection(self): + """A tag inside the message body describes the caller's input. + + Native errors quote caller-supplied strings verbatim: a JSON parse + failure repeats the offending value, an Io failure names the path. + Reading one of those as a pre-consume rejection hands the resource back + as usable after native may already own and have dropped its handle. + """ + classify = c2pa_module.ManagedResource._is_pre_consume_rejection + + forged = ( + 'Json: invalid type: string "NullParameter: x", expected a ' + 'sequence at line 1 column 43', + 'Json: invalid type: string "WrongPointerType: y", expected a ' + 'sequence at line 1 column 46', + "Io: cannot open /tmp/UntrackedPointer: 0xdead.jpg", + "Other: manifest text mentions InvalidBufferSize: in passing", + ) + for message in forged: + self.assertFalse( + classify(message), + f"caller text was read as a pointer rejection: {message!r}") + + def test_a_failing_flush_does_not_strand_the_rest_of_the_queue(self): + """One resource raising must not skip the resources queued behind it. + """ + flushed = [] + + class Recorder: + def __init__(self, name, raises=None): + self.name = name + self.raises = raises + + def _maybe_flush_pending(self): + if self.raises is not None: + raise self.raises + flushed.append(self.name) + + first = Recorder("first") + middle = Recorder("middle", raises=KeyboardInterrupt()) + last = Recorder("last") + + with self.assertLogs("c2pa", level="ERROR"): + with c2pa_module._native_section(): + for resource in (first, middle, last): + c2pa_module._register_for_section_flush(resource) + + self.assertEqual(flushed, ["first", "last"]) + + def test_runtime_does_not_call_error_set_last(self): + """The marker mechanism must not depend on c2pa_error_set_last, + so this module loads against native builds that lack it.""" + for fn in (c2pa_module.ManagedResource._invoke_consume, + c2pa_module._read_native_error, + c2pa_module._write_no_error_marker): + self.assertNotIn( + 'c2pa_error_set_last', inspect.getsource(fn)) + + +class TestMarkerOutlivesPointerConsumptionSemantics(unittest.TestCase): + """The marker is needed for reasons independent of pointer ownership. + + The native error slot is sticky and thread-local, so failure paths + that carry no still need to tell an error this call set from an + earlier, unrelated call left behind. + """ + + def setUp(self): + # Leave no message from an earlier test in this thread's slot. + c2pa_module._write_no_error_marker() + + def test_settings_set_failure_reports_its_own_error(self): + settings = Settings() + self.addCleanup(settings.close) + + c2pa_module._lib.c2pa_error_set_last(b"Signature: earlier task") + self.assertEqual( + c2pa_module._read_native_error(), "Signature: earlier task") + + with self.assertRaises(Error) as ctx: + settings.set("builder.thumbnail.enabled", "not-a-json-value") + + self.assertNotIn("earlier task", str(ctx.exception)) + + def test_one_thread_marker_does_not_clear_another_threads_error(self): + """Marking on one thread must leave another thread's pending error + readable: the slot is per thread, and so is the marker.""" + set_on_worker = threading.Event() + marked_on_main = threading.Event() + seen = {} + + def worker(): + c2pa_module._lib.c2pa_error_set_last(b"Io: worker error") + set_on_worker.set() + marked_on_main.wait(5) + seen["worker"] = c2pa_module._read_native_error() + + thread = threading.Thread(target=worker, daemon=True) + thread.start() + set_on_worker.wait(5) + + c2pa_module._write_no_error_marker() + marked_on_main.set() + thread.join(5) + + self.assertEqual(seen.get("worker"), "Io: worker error") + class TestErrorsStillRaiseAfterCleanup(unittest.TestCase): """Each surface that lost a _clear_error_state() call still reports.""" @@ -9652,5 +9931,226 @@ def test_ed25519_sign_with_empty_data_raises(self): c2pa_module.ed25519_sign(b"", "not a key") +class TestConsumeOwnership(unittest.TestCase): + """Ownership of the native handle across the consuming call paths.""" + + def setUp(self): + self.freed = [] + self._real_free = ManagedResource._free_native_ptr + + def counting_free(ptr): + self.freed.append(ptr) + return self._real_free(ptr) + + _patch_free(self, counting_free) + + def test_close_called_during_parallel_call(self): + """Parallel closes handling. + """ + resource = Settings() + spare = Settings() + replacement = spare._handle + # Only test should be able to free. + spare._handle = None + spare._lifecycle_state = LifecycleState.CLOSED + self.freed.clear() + + def close_then_swap(handle): + resource.close() + return replacement + + resource._consume_and_swap(close_then_swap, "swap: {}") + + self.assertIn(replacement, self.freed) + + +class TestContextProviderContract(unittest.TestCase): + """The published ContextProvider contract is is_valid plus + execution_context, and nothing more. + """ + + class _MinimalProvider(ContextProvider): + """Implements exactly what the abstract base class declares.""" + + def __init__(self): + self._inner = Context(Settings()) + + @property + def is_valid(self): + return self._inner.is_valid + + @property + def execution_context(self): + return self._inner.execution_context + + def test_built_in_context_still_gets_in_flight_protection(self): + """The compatibility shim must not silently drop the guard for the + provider that does implement it. + """ + context = Context(Settings()) + with c2pa_module._context_guard(context): + self.assertGreater(context._inflight, 0) + + +class TestLockOrderStaticAnalysis(unittest.TestCase): + """Static analysis over the source: no threads are spawned here. + """ + + def test_no_conflicting_lock_acquisition_order(self): + """No two locks may be nested in opposite orders by different methods. + + Two methods nesting the same pair of locks in opposite order is a + AB/BA deadlock shape: thread 1 holds A and waits for B while + thread 2 holds B and waits for A. + """ + tree = ast.parse(inspect.getsource(c2pa_module)) + + # Every self._X = threading.Lock()/RLock()/Condition() assignment, + # grouped by the class that owns it. + lock_attrs_by_class = {} + for cls in ast.walk(tree): + if not isinstance(cls, ast.ClassDef): + continue + found = set() + for node in ast.walk(cls): + if not (isinstance(node, ast.Assign) + and len(node.targets) == 1): + continue + target = node.targets[0] + if not (isinstance(target, ast.Attribute) + and isinstance(target.value, ast.Name) + and target.value.id == "self"): + continue + value = node.value + if (isinstance(value, ast.Call) + and isinstance(value.func, ast.Attribute) + and value.func.attr in + ("Lock", "RLock", "Condition")): + found.add(target.attr) + if found: + lock_attrs_by_class[cls.name] = found + + def lock_name_for_with(item): + """The lock attribute a `with` item enters, or None.""" + ctx = item.context_expr + # with self._fragment_lock: + if (isinstance(ctx, ast.Attribute) + and isinstance(ctx.value, ast.Name) + and ctx.value.id == "self" + and any(ctx.attr in attrs + for attrs in lock_attrs_by_class.values())): + return ctx.attr + # with self._guarded_op(): returns _op_lock itself, and the + # accessors return the lock they are named for. + lock_by_method = { + "_guarded_op": "_op_lock", + "_live_op_lock": "_op_lock", + "_live_teardown_lock": "_teardown_lock", + } + if (isinstance(ctx, ast.Call) + and isinstance(ctx.func, ast.Attribute) + and ctx.func.attr in lock_by_method + and isinstance(ctx.func.value, ast.Name) + and ctx.func.value.id == "self"): + return lock_by_method[ctx.func.attr] + return None + + def lock_name_for_acquire(node): + """A lock taken with acquire() and released in a finally nests just + as a `with` does, so the scan has to follow it or it silently stops + seeing whole regions. + """ + call = node.value if isinstance(node, ast.Expr) else node + if isinstance(call, ast.UnaryOp) and isinstance(call.op, ast.Not): + call = call.operand + if not (isinstance(call, ast.Call) + and isinstance(call.func, ast.Attribute) + and call.func.attr == "acquire"): + return None + owner = call.func.value + if (isinstance(owner, ast.Attribute) + and isinstance(owner.value, ast.Name) + and owner.value.id == "self" + and any(owner.attr in attrs + for attrs in lock_attrs_by_class.values())): + return owner.attr + return None + + def acquires_in_test(node): + """Lock taken by `if not self._X.acquire(...)`-style guards.""" + if isinstance(node, ast.If): + return lock_name_for_acquire(node.test) + return None + + def orders_in(node, stack, pairs): + """Record (outer, inner) for every nesting this node contains.""" + if isinstance(node, (ast.With, ast.AsyncWith)): + names = [n for n in (lock_name_for_with(i) + for i in node.items) if n] + for name in names: + if stack: + pairs.add((stack[-1], name)) + stack.append(name) + for child in node.body: + orders_in(child, stack, pairs) + for _ in names: + stack.pop() + return + # A statement list can open a lock partway through via acquire(); + # everything after it in that list is nested inside. + for field, value in ast.iter_fields(node): + if not isinstance(value, list): + continue + held = [] + for child in value: + if not isinstance(child, ast.stmt): + continue + name = (lock_name_for_acquire(child) + or acquires_in_test(child)) + if name: + if stack: + pairs.add((stack[-1], name)) + stack.append(name) + held.append(name) + continue + orders_in(child, stack, pairs) + for _ in held: + stack.pop() + for child in ast.iter_child_nodes(node): + if not isinstance(child, ast.stmt): + orders_in(child, stack, pairs) + + pairs_by_method = {} + for cls in ast.walk(tree): + if not isinstance(cls, ast.ClassDef): + continue + for method in cls.body: + if not isinstance(method, (ast.FunctionDef, + ast.AsyncFunctionDef)): + continue + pairs = set() + orders_in(method, [], pairs) + if pairs: + pairs_by_method[(cls.name, method.name)] = pairs + + all_pairs = set().union(*pairs_by_method.values()) \ + if pairs_by_method else set() + conflicts = [] + for (outer, inner) in all_pairs: + # Each conflicting pair appears twice in all_pairs, once per direction. + if outer >= inner or (inner, outer) not in all_pairs: + continue + forward = [k for k, v in pairs_by_method.items() + if (outer, inner) in v] + backward = [k for k, v in pairs_by_method.items() + if (inner, outer) in v] + conflicts.append( + "{} nests {} inside {}, but {} nests {} inside {}".format( + forward[0], inner, outer, backward[0], outer, inner)) + + self.assertGreater(len(pairs_by_method), 0, "scan found no nesting") + self.assertEqual(conflicts, [], ", ".join(sorted(set(conflicts)))) + + if __name__ == '__main__': unittest.main(warnings='ignore') diff --git a/tests/test_unit_tests_threaded.py b/tests/test_unit_tests_threaded.py index d0d0b2c1..2722ea59 100644 --- a/tests/test_unit_tests_threaded.py +++ b/tests/test_unit_tests_threaded.py @@ -11,28 +11,40 @@ # specific language governing permissions and limitations under # each license. +import ast import ctypes import gc import os +import re +import inspect import io import json +import subprocess +import sys +import textwrap import unittest import threading import concurrent.futures import time -import asyncio -import random from unittest.mock import MagicMock, patch - -from c2pa import Builder, C2paError as Error, Reader, C2paSigningAlg as SigningAlg, C2paSignerInfo, Signer, sdk_version # noqa: E501 +from c2pa import Builder, C2paError as Error, Reader, C2paSignerInfo, Signer, sdk_version # noqa: E501 from c2pa import Context, Settings -from c2pa.c2pa import ManagedResource, Stream, LifecycleState +from c2pa.c2pa import ManagedResource, Stream, LifecycleState, _native_section +import c2pa.c2pa as c2pa_module from c2pa.lib import is_foreign_process, record_owner_pid PROJECT_PATH = os.getcwd() FIXTURES_FOLDER = os.path.join(os.path.dirname(__file__), "fixtures") +def _patch_free(test, fn): + """Route ManagedResource._free_native_ptr to `fn` until `test` ends.""" + patcher = patch.object( + ManagedResource, '_free_native_ptr', staticmethod(fn)) + patcher.start() + test.addCleanup(patcher.stop) + + class _ConcreteResource(ManagedResource): """Minimal concrete subclass for testing ManagedResource cleanup.""" @@ -58,6 +70,7 @@ def _make_stream(pid_offset): obj._closed = False obj._initialized = True obj._stream = MagicMock() # non-None stream handle + obj._close_lock = threading.Lock() if pid_offset is not None: obj._owner_pid = os.getpid() + pid_offset return obj @@ -178,6 +191,234 @@ def test_foreign_pid_close_marks_closed(self): self.assertFalse(obj._initialized) +class TestForkedChildDoesNotDeadlock(unittest.TestCase): + """A forked child must never block on a lock the parent held at fork(). + + Locking a resource for the duration of an operation means a child that + forks while some thread holds that lock inherits it locked, with the owner + thread gone. Anything in the child that acquires it waits forever. + + The failure mode is a hang: each operation runs on a worker thread and + is joined with a timeout: a test that called it directly would hang the + runner instead of failing. + """ + + _TIMEOUT = 5.0 + + def _foreign_reader_with_lock_held(self, fragment_lock=False): + """A Reader in the state a forked child inherits: + lock held by another thread, and stamped with a PID other than this process's. + """ + with open(DEFAULT_TEST_FILE, "rb") as asset: + reader = Reader("image/jpeg", asset) + holding = threading.Event() + release = threading.Event() + + def hold_the_lock(): + held = (reader._fragment_lock if fragment_lock + else reader._guarded_op()) + with held: + holding.set() + release.wait(30) + + holder = threading.Thread(target=hold_the_lock, daemon=True) + holder.start() + self.assertTrue(holding.wait(self._TIMEOUT), + "helper thread never acquired the lock") + self.addCleanup(holder.join, self._TIMEOUT) + self.addCleanup(release.set) + + reader._owner_pid = os.getpid() + 1 + return reader + + def _foreign_stream_with_close_lock_held(self): + """A Stream in the state a forked child inherits: _close_lock held by + a thread that does not exist in the child, and a foreign owner PID. + """ + stream = Stream(io.BytesIO(b"payload")) + holding = threading.Event() + release = threading.Event() + + def hold_the_lock(): + with stream._close_lock: + holding.set() + release.wait(30) + + holder = threading.Thread(target=hold_the_lock, daemon=True) + holder.start() + self.assertTrue(holding.wait(self._TIMEOUT), + "helper thread never acquired _close_lock") + # Cleanups run last-registered-first, so this one runs after + # release.set and holder.join. + self.addCleanup(self._reclaim_foreign_stream, stream) + self.addCleanup(holder.join, self._TIMEOUT) + self.addCleanup(release.set) + + stream._owner_pid = os.getpid() + 1 + return stream + + def _reclaim_foreign_stream(self, stream): + """Release a stream the foreign-process path left tracked.""" + stream._owner_pid = os.getpid() + stream._closed = False + stream.close() + + def test_stream_close_completes_with_close_lock_held(self): + """close() must take the foreign-process path without acquiring + _close_lock, which no surviving thread would release.""" + stream = self._foreign_stream_with_close_lock_held() + + outcome = self._run_with_timeout(stream.close) + + self.assertEqual(outcome, "ok") + + def _run_with_timeout(self, operation): + """Run operation on a worker; return 'ok', the exception, or None if it + was still running when the timeout expired.""" + result = {} + + def run(): + try: + operation() + result["outcome"] = "ok" + except BaseException as e: # noqa: BLE001 - asserted on below + result["outcome"] = e + + worker = threading.Thread(target=run, daemon=True) + worker.start() + worker.join(self._TIMEOUT) + return result.get("outcome") + + def test_locked_read_raises_instead_of_blocking(self): + reader = self._foreign_reader_with_lock_held() + outcome = self._run_with_timeout(reader.json) + self.assertIsInstance(outcome, Error) + + def test_native_call_path_raises_instead_of_blocking(self): + reader = self._foreign_reader_with_lock_held() + outcome = self._run_with_timeout( + lambda: reader.resource_to_stream("any-uri", io.BytesIO())) + self.assertIsInstance(outcome, Error) + + def test_fragment_lock_path_raises_instead_of_blocking(self): + reader = self._foreign_reader_with_lock_held(fragment_lock=True) + outcome = self._run_with_timeout( + lambda: reader.with_fragment( + "video/mp4", io.BytesIO(b""), io.BytesIO(b""))) + self.assertIsInstance(outcome, Error) + + def test_close_still_completes(self): + reader = self._foreign_reader_with_lock_held() + self.assertEqual(self._run_with_timeout(reader.close), "ok") + + def test_is_valid_false_for_inherited_object(self): + reader = self._foreign_reader_with_lock_held() + self.assertFalse(reader.is_valid) + + def test_parent_copy_unaffected(self): + """The child closing its copy must leave the parent's usable. + + Runs in a subprocess so that the fork happens in a single-threaded + process. Operations that reach the network, such as reading an asset + with a remote manifest, start background native threads that outlive + the object that triggered them, and forking a multi-threaded process + can lead to issues. + """ + source = textwrap.dedent(""" + import os, sys + from c2pa import Reader + from c2pa.c2pa import LifecycleState + + asset_path = sys.argv[1] + with open(asset_path, "rb") as asset: + reader = Reader("image/jpeg", asset) + before = reader.json() + + pid = os.fork() + if pid == 0: + try: + reader.close() + os._exit(0) + except BaseException: + os._exit(1) + _, status = os.waitpid(pid, 0) + + assert status >> 8 == 0, "child could not close its own copy" + assert reader._lifecycle_state == LifecycleState.ACTIVE + assert reader.json() == before + reader.close() + print("OK") + """) + + result = subprocess.run( + [sys.executable, "-c", source, DEFAULT_TEST_FILE], + capture_output=True, text=True, timeout=120) + + self.assertEqual(result.returncode, 0, result.stderr[-2000:]) + + +class TestReaderWithFragmentConcurrency(unittest.TestCase): + """with_fragment's native call and its stream-ownership transfer + must not interleave with another with_fragment on the same Reader. + """ + + def setUp(self): + self.init_path = os.path.join(FIXTURES_FOLDER, "dashinit.mp4") + self.fragment_path = os.path.join(FIXTURES_FOLDER, "dash1.m4s") + with open(self.init_path, "rb") as f: + self.init_bytes = f.read() + with open(self.fragment_path, "rb") as f: + self.fragment_bytes = f.read() + + def _advance(self, reader): + reader.with_fragment( + "video/mp4", + io.BytesIO(self.init_bytes), + io.BytesIO(self.fragment_bytes)) + + def test_close_during_with_fragment_does_not_double_close_stream(self): + with open(self.init_path, "rb") as init: + reader = Reader("video/mp4", init) + + entered_gap = threading.Event() + release_gap = threading.Event() + + real_consume_and_swap = reader._consume_and_swap + + def gated_consume_and_swap(ffi_call, error_message): + real_consume_and_swap(ffi_call, error_message) + # Pauses in with_fragment's window before it reassigns _own_stream/_fragment_streams. + entered_gap.set() + release_gap.wait(5) + + reader._consume_and_swap = gated_consume_and_swap + + result = {} + + def run_with_fragment(): + try: + with open(self.init_path, "rb") as init, \ + open(self.fragment_path, "rb") as frag: + reader.with_fragment("video/mp4", init, frag) + result["outcome"] = "ok" + except BaseException as e: # noqa: BLE001 - asserted below + result["outcome"] = e + + worker = threading.Thread(target=run_with_fragment, daemon=True) + worker.start() + self.assertTrue(entered_gap.wait(5), "gap never reached") + + # close() must win the race, and with_fragment must not hang, + # crash, or succeed without signalling. + reader.close() + release_gap.set() + worker.join(5) + self.assertIsInstance(result.get("outcome"), Error, "must raise, not hang") + + # with_fragment must not resurrect these fields on a reader close() already tore down. + self.assertEqual( + (reader._own_stream, reader._fragment_streams), (None, [])) + class TestHelpers(unittest.TestCase): def test_record_and_detect_own_pid(self): @@ -325,8 +566,7 @@ def process_file(filename): errors.append(error) except Exception as e: errors.append( - f"Unexpected error processing {filename}: { - str(e)}") + f"Unexpected error processing {filename}: {str(e)}") # If any errors occurred, fail the test with all error messages if errors: @@ -699,12 +939,12 @@ def setUp(self): with open(os.path.join(self.data_dir, "es256_private.key"), "rb") as key_file: self.key = key_file.read() - # Create a local Es256 signer with certs and a timestamp server + # Create a local Es256 signer with certs and no timestamp server. self.signer_info = C2paSignerInfo( alg=b"es256", sign_cert=self.certs, private_key=self.key, - ta_url=b"http://timestamp.digicert.com" + ta_url=None ) self.signer = Signer.from_info(self.signer_info) @@ -860,8 +1100,7 @@ def sign_file(filename, thread_id): active_manifest = manifest_store["manifests"][manifest_store["active_manifest"]] # Verify the correct manifest was used - expected_claim_generator = f"python_test_{ - 2 if thread_id % 2 == 0 else 1}/0.0.1" + expected_claim_generator = f"python_test_{2 if thread_id % 2 == 0 else 1}/0.0.1" self.assertEqual( active_manifest["claim_generator"], expected_claim_generator) @@ -879,8 +1118,7 @@ def sign_file(filename, thread_id): except Error.NotSupported: return None except Exception as e: - return f"Failed to sign { - filename} in thread {thread_id}: {str(e)}" + return f"Failed to sign {filename} in thread {thread_id}: {str(e)}" # Create a thread pool with 6 workers with concurrent.futures.ThreadPoolExecutor(max_workers=6) as executor: @@ -904,129 +1142,12 @@ def sign_file(filename, thread_id): if error: errors.append(error) except Exception as e: - errors.append(f"Unexpected error processing { - filename} in thread {thread_id}: {str(e)}") + errors.append(f"Unexpected error processing {filename} in thread {thread_id}: {str(e)}") # If any errors occurred, fail the test with all error messages if errors: self.fail("\n".join(errors)) - def test_sign_all_files_async(self): - """Test signing all files using asyncio with a pool of workers""" - signing_dir = os.path.join(self.data_dir, "files-for-signing-tests") - reading_dir = os.path.join(self.data_dir, "files-for-reading-tests") - - # Map of file extensions to MIME types - mime_types = { - '.jpg': 'image/jpeg', - '.jpeg': 'image/jpeg', - '.png': 'image/png', - '.gif': 'image/gif', - '.webp': 'image/webp', - '.heic': 'image/heic', - '.heif': 'image/heif', - '.avif': 'image/avif', - '.tif': 'image/tiff', - '.tiff': 'image/tiff', - '.mp4': 'video/mp4', - '.avi': 'video/x-msvideo', - '.mp3': 'audio/mpeg', - '.m4a': 'audio/mp4', - '.wav': 'audio/wav' - } - - # Skip files that are known to be invalid or unsupported - skip_files = { - 'sample3.invalid.wav', # Invalid file - } - - async def async_sign_file(filename, thread_id): - """Async version of file signing operation""" - if filename in skip_files: - return None - - file_path = os.path.join(signing_dir, filename) - if not os.path.isfile(file_path): - return None - - # Get file extension and corresponding MIME type - _, ext = os.path.splitext(filename) - ext = ext.lower() - if ext not in mime_types: - return None - - mime_type = mime_types[ext] - - try: - with open(file_path, "rb") as file: - # Choose manifest based on thread number - manifest_def = self.manifestDefinition_2 if thread_id % 2 == 0 else self.manifestDefinition_1 - expected_author = "Tester Two" if thread_id % 2 == 0 else "Tester One" - - builder = Builder(manifest_def) - output = io.BytesIO(bytearray()) - builder.sign(self.signer, mime_type, file, output) - output.seek(0) - - # Verify the signed file - reader = Reader(mime_type, output) - json_data = reader.json() - manifest_store = json.loads(json_data) - active_manifest = manifest_store["manifests"][manifest_store["active_manifest"]] - - # Verify the correct manifest was used - expected_claim_generator = f"python_test_{ - 2 if thread_id % 2 == 0 else 1}/0.0.1" - self.assertEqual( - active_manifest["claim_generator"], - expected_claim_generator) - - # Verify the author is correct - assertions = active_manifest["assertions"] - for assertion in assertions: - if assertion["label"] == "com.unit.test": - author_name = assertion["data"]["author"][0]["name"] - self.assertEqual(author_name, expected_author) - break - - output.close() - return None # Success case - except Error.NotSupported: - return None - except Exception as e: - return f"Failed to sign { - filename} in thread {thread_id}: {str(e)}" - - async def run_async_tests(): - # Get all files from both directories - all_files = [] - for directory in [signing_dir, reading_dir]: - all_files.extend(os.listdir(directory)) - - # Create tasks for all files - tasks = [] - for i, filename in enumerate(all_files): - task = asyncio.create_task(async_sign_file(filename, i)) - tasks.append(task) - - # Wait for all tasks to complete and collect results - results = await asyncio.gather(*tasks, return_exceptions=True) - - # Process results - errors = [] - for result in results: - if isinstance(result, Exception): - errors.append(str(result)) - elif result: # Non-None result indicates an error - errors.append(result) - - # If any errors occurred, fail the test with all error messages - if errors: - self.fail("\n".join(errors)) - - # Run the async tests - asyncio.run(run_async_tests()) - def test_parallel_manifest_writing(self): """Test writing different manifests to two files in parallel and verify no data mixing occurs""" output1 = io.BytesIO(bytearray()) @@ -1059,8 +1180,7 @@ def write_manifest(manifest_def, output_stream, thread_id): if assertion["label"] == "com.unit.test": author_name = assertion["data"]["author"][0]["name"] self.assertEqual( - author_name, f"Tester { - 'One' if thread_id == 1 else 'Two'}") + author_name, f"Tester {'One' if thread_id == 1 else 'Two'}") break return active_manifest @@ -1203,8 +1323,7 @@ def sign_file(filename, thread_id): if thread_id % 3 == 0: expected_claim_generator = "python_test/0.0.1" else: - expected_claim_generator = f"python_test_{ - expected_thread}/0.0.1" + expected_claim_generator = f"python_test_{expected_thread}/0.0.1" self.assertEqual( active_manifest["claim_generator"], @@ -1223,8 +1342,7 @@ def sign_file(filename, thread_id): except Error.NotSupported: return None except Exception as e: - return f"Failed to sign { - filename} in thread {thread_id}: {str(e)}" + return f"Failed to sign {filename} in thread {thread_id}: {str(e)}" # Create a thread pool with 3 workers with concurrent.futures.ThreadPoolExecutor(max_workers=3) as executor: @@ -1248,8 +1366,7 @@ def sign_file(filename, thread_id): if error: errors.append(error) except Exception as e: - errors.append(f"Unexpected error processing { - filename} in thread {thread_id}: {str(e)}") + errors.append(f"Unexpected error processing {filename} in thread {thread_id}: {str(e)}") # Verify thread interleaving # Check that we don't have long sequences of the same thread @@ -1262,8 +1379,7 @@ def sign_file(filename, thread_id): if thread_execution_order[i][1] == current_thread: current_sequence += 1 if current_sequence > max_same_thread_sequence: - self.fail(f"Thread {current_thread} executed { - current_sequence} times in sequence, indicating poor interleaving") + self.fail(f"Thread {current_thread} executed {current_sequence} times in sequence, indicating poor interleaving") else: current_sequence = 1 current_thread = thread_execution_order[i][1] @@ -1807,183 +1923,6 @@ def sign_file(output_stream, manifest_def, thread_id): output1.close() output2.close() - def test_concurrent_read_after_write_async(self): - """Test reading from a file after writing is complete using asyncio""" - output = io.BytesIO(bytearray()) - write_complete = asyncio.Event() - write_errors = [] - read_errors = [] - write_success = False - - async def write_manifest(): - nonlocal write_success - try: - with open(self.test_path, "rb") as file: - builder = Builder(self.manifestDefinition_1) - builder.sign(self.signer, "image/jpeg", file, output) - output.seek(0) - write_success = True - write_complete.set() - except Exception as e: - write_errors.append(f"Write error: {str(e)}") - write_complete.set() - - async def read_manifest(): - try: - # Wait for write to complete before reading - await write_complete.wait() - - # Verify write was successful - if not write_success: - raise Exception( - "Write operation did not complete successfully") - - # Verify output is not empty - output_size = len(output.getvalue()) - self.assertGreater( - output_size, 0, "Output should not be empty after write") - - # Read after write is complete - output.seek(0) - reader = Reader("image/jpeg", output) - json_data = reader.json() - manifest_store = json.loads(json_data) - - # Verify manifest store structure - self.assertIn( - "manifests", - manifest_store, - "Manifest store should contain 'manifests'") - self.assertIn( - "active_manifest", - manifest_store, - "Manifest store should contain 'active_manifest'") - - active_manifest = manifest_store["manifests"][manifest_store["active_manifest"]] - - # Verify final manifest - self.assertEqual( - active_manifest["claim_generator"], - "python_test_1/0.0.1") - self.assertEqual( - active_manifest["title"], - "Python Test Image 1") - - # Verify the author is correct - assertions = active_manifest["assertions"] - author_found = False - for assertion in assertions: - if assertion["label"] == "com.unit.test": - author_name = assertion["data"]["author"][0]["name"] - self.assertEqual(author_name, "Tester One") - author_found = True - break - self.assertTrue(author_found, - "Author assertion not found in manifest") - - except Exception as e: - read_errors.append(f"Read error: {str(e)}") - - async def run_async_tests(): - # Create and run write task first - write_task = asyncio.create_task(write_manifest()) - await write_task # Wait for write to complete - - # Only start read task after write is complete - read_task = asyncio.create_task(read_manifest()) - await read_task # Wait for read to complete - - # Run the async tests - asyncio.run(run_async_tests()) - - # Clean up - output.close() - - # Check for errors - if write_errors: - self.fail("\n".join(write_errors)) - if read_errors: - self.fail("\n".join(read_errors)) - - def test_resource_contention_read_parallel_async(self): - """Test multiple async tasks reading the same file concurrently""" - output = io.BytesIO(bytearray()) - read_errors = [] - reader_count = 5 # Number of concurrent readers - active_readers = 0 - readers_lock = asyncio.Lock() # Lock for reader count - stream_lock = asyncio.Lock() # Lock for stream access - # Barrier to synchronize task starts - start_barrier = asyncio.Barrier(reader_count) - - # First write some data to read - with open(self.test_path, "rb") as file: - builder = Builder(self.manifestDefinition_1) - builder.sign(self.signer, "image/jpeg", file, output) - output.seek(0) - - async def read_manifest(reader_id): - nonlocal active_readers - try: - async with readers_lock: - active_readers += 1 - - # Wait for all tasks to be ready - await start_barrier.wait() - - # Read the manifest - async with stream_lock: # Ensure exclusive access to stream - output.seek(0) # Reset stream position before read - reader = Reader("image/jpeg", output) - json_data = reader.json() - manifest_store = json.loads(json_data) - active_manifest = manifest_store["manifests"][manifest_store["active_manifest"]] - - # Verify manifest data - self.assertEqual( - active_manifest["claim_generator"], - "python_test_1/0.0.1") - self.assertEqual( - active_manifest["title"], - "Python Test Image 1") - - # Verify the author is correct - assertions = active_manifest["assertions"] - for assertion in assertions: - if assertion["label"] == "com.unit.test": - author_name = assertion["data"]["author"][0]["name"] - self.assertEqual(author_name, "Tester One") - break - - except Exception as e: - read_errors.append(f"Reader {reader_id} error: {str(e)}") - finally: - async with readers_lock: - active_readers -= 1 - - async def run_async_tests(): - # Create all tasks first - tasks = [] - for i in range(reader_count): - task = asyncio.create_task(read_manifest(i)) - tasks.append(task) - - # Wait for all tasks to complete - await asyncio.gather(*tasks) - - # Run the async tests - asyncio.run(run_async_tests()) - - # Clean up - output.close() - - # Check for errors - if read_errors: - self.fail("\n".join(read_errors)) - - # Verify all readers completed - self.assertEqual(active_readers, 0, "Not all readers completed") - def test_builder_sign_with_multiple_ingredient_random_many_threads(self): """Test Builder class operations with 12 threads, each adding 3 specific ingredients and signing a file.""" # Number of threads to use in the test @@ -2229,73 +2168,6 @@ def sign_file(filename, thread_id): if errors: self.fail("\n".join(errors)) - def test_sign_all_files_async(self): - """Test signing all files using asyncio with Context""" - signing_dir = os.path.join(self.data_dir, "files-for-signing-tests") - reading_dir = os.path.join(self.data_dir, "files-for-reading-tests") - mime_types = { - '.jpg': 'image/jpeg', '.jpeg': 'image/jpeg', '.png': 'image/png', - '.gif': 'image/gif', '.webp': 'image/webp', '.heic': 'image/heic', - '.heif': 'image/heif', '.avif': 'image/avif', '.tif': 'image/tiff', - '.tiff': 'image/tiff', '.mp4': 'video/mp4', '.avi': 'video/x-msvideo', - '.mp3': 'audio/mpeg', '.m4a': 'audio/mp4', '.wav': 'audio/wav' - } - skip_files = {'sample3.invalid.wav'} - - async def async_sign_file(filename, thread_id): - if filename in skip_files: - return None - file_path = os.path.join(signing_dir, filename) - if not os.path.isfile(file_path): - return None - _, ext = os.path.splitext(filename) - ext = ext.lower() - if ext not in mime_types: - return None - mime_type = mime_types[ext] - try: - with open(file_path, "rb") as file: - manifest_def = self.manifestDefinition_2 if thread_id % 2 == 0 else self.manifestDefinition_1 - expected_author = "Tester Two" if thread_id % 2 == 0 else "Tester One" - ctx = Context() - builder = Builder(manifest_def, ctx) - output = io.BytesIO(bytearray()) - builder.sign(self.signer, mime_type, file, output) - output.seek(0) - read_ctx = Context() - reader = Reader(mime_type, output, context=read_ctx) - json_data = reader.json() - manifest_store = json.loads(json_data) - active_manifest = manifest_store["manifests"][manifest_store["active_manifest"]] - expected_claim_generator = f"python_test_{2 if thread_id % 2 == 0 else 1}/0.0.1" - self.assertEqual(active_manifest["claim_generator"], expected_claim_generator) - for assertion in active_manifest["assertions"]: - if assertion["label"] == "com.unit.test": - self.assertEqual(assertion["data"]["author"][0]["name"], expected_author) - break - output.close() - return None - except Error.NotSupported: - return None - except Exception as e: - return f"Failed to sign {filename} in thread {thread_id}: {str(e)}" - - async def run_async_tests(): - all_files = [] - for directory in [signing_dir, reading_dir]: - all_files.extend(os.listdir(directory)) - tasks = [asyncio.create_task(async_sign_file(f, i)) for i, f in enumerate(all_files)] - results = await asyncio.gather(*tasks, return_exceptions=True) - errors = [] - for result in results: - if isinstance(result, Exception): - errors.append(str(result)) - elif result: - errors.append(result) - if errors: - self.fail("\n".join(errors)) - asyncio.run(run_async_tests()) - def test_parallel_manifest_writing(self): """Test writing different manifests in parallel using context APIs""" output1 = io.BytesIO(bytearray()) @@ -2716,116 +2588,6 @@ def sign_file(output_stream, manifest_def, thread_id): output1.close() output2.close() - def test_concurrent_read_after_write_async(self): - """Test read after write using asyncio with context APIs""" - output = io.BytesIO(bytearray()) - write_complete = asyncio.Event() - write_errors = [] - read_errors = [] - write_success = False - - async def write_manifest(): - nonlocal write_success - try: - ctx = Context() - with open(self.test_path, "rb") as file: - builder = Builder(self.manifestDefinition_1, ctx) - builder.sign(self.signer, "image/jpeg", file, output) - output.seek(0) - write_success = True - write_complete.set() - except Exception as e: - write_errors.append(f"Write error: {str(e)}") - write_complete.set() - - async def read_manifest(): - try: - await write_complete.wait() - if not write_success: - raise Exception("Write operation did not complete successfully") - self.assertGreater(len(output.getvalue()), 0) - output.seek(0) - read_ctx = Context() - reader = Reader("image/jpeg", output, context=read_ctx) - json_data = reader.json() - manifest_store = json.loads(json_data) - self.assertIn("manifests", manifest_store) - self.assertIn("active_manifest", manifest_store) - active_manifest = manifest_store["manifests"][manifest_store["active_manifest"]] - self.assertEqual(active_manifest["claim_generator"], "python_test_1/0.0.1") - self.assertEqual(active_manifest["title"], "Python Test Image 1") - author_found = False - for assertion in active_manifest["assertions"]: - if assertion["label"] == "com.unit.test": - self.assertEqual(assertion["data"]["author"][0]["name"], "Tester One") - author_found = True - break - self.assertTrue(author_found) - except Exception as e: - read_errors.append(f"Read error: {str(e)}") - - async def run_async_tests(): - write_task = asyncio.create_task(write_manifest()) - await write_task - read_task = asyncio.create_task(read_manifest()) - await read_task - asyncio.run(run_async_tests()) - output.close() - if write_errors: - self.fail("\n".join(write_errors)) - if read_errors: - self.fail("\n".join(read_errors)) - - def test_resource_contention_read_parallel_async(self): - """Test multiple async tasks reading the same file with context APIs""" - output = io.BytesIO(bytearray()) - read_errors = [] - reader_count = 5 - active_readers = 0 - readers_lock = asyncio.Lock() - stream_lock = asyncio.Lock() - start_barrier = asyncio.Barrier(reader_count) - - ctx = Context() - with open(self.test_path, "rb") as file: - builder = Builder(self.manifestDefinition_1, ctx) - builder.sign(self.signer, "image/jpeg", file, output) - output.seek(0) - - async def read_manifest(reader_id): - nonlocal active_readers - try: - async with readers_lock: - active_readers += 1 - await start_barrier.wait() - async with stream_lock: - output.seek(0) - read_ctx = Context() - reader = Reader("image/jpeg", output, context=read_ctx) - json_data = reader.json() - manifest_store = json.loads(json_data) - active_manifest = manifest_store["manifests"][manifest_store["active_manifest"]] - self.assertEqual(active_manifest["claim_generator"], "python_test_1/0.0.1") - self.assertEqual(active_manifest["title"], "Python Test Image 1") - for assertion in active_manifest["assertions"]: - if assertion["label"] == "com.unit.test": - self.assertEqual(assertion["data"]["author"][0]["name"], "Tester One") - break - except Exception as e: - read_errors.append(f"Reader {reader_id} error: {str(e)}") - finally: - async with readers_lock: - active_readers -= 1 - - async def run_async_tests(): - tasks = [asyncio.create_task(read_manifest(i)) for i in range(reader_count)] - await asyncio.gather(*tasks) - asyncio.run(run_async_tests()) - output.close() - if read_errors: - self.fail("\n".join(read_errors)) - self.assertEqual(active_readers, 0) - def test_builder_sign_with_multiple_ingredient_random_many_threads(self): """Test Builder with 12 threads adding ingredients and signing using context APIs""" TOTAL_THREADS_USED = 12 @@ -2917,19 +2679,189 @@ def thread_work(thread_id): self.assertNotEqual(current_manifest["active_manifest"], thread_manifest_data[other_thread_id]["active_manifest"]) -class TestManagedResourceCrossThread(unittest.TestCase): - """Tests cross-thread resources handling, especially closing/releasind. +class TestWithFragmentReentrancy(unittest.TestCase): + """with_fragment drives caller-supplied stream callbacks, so it must not + hold a lock a callback-spawned thread would wait on. + """ + + def test_reentrant_call_is_refused_rather_than_blocked(self): + init_path = os.path.join(FIXTURES_FOLDER, "dashinit.mp4") + fragment_path = os.path.join(FIXTURES_FOLDER, "dash1.m4s") + with open(init_path, "rb") as handle: + init_bytes = handle.read() + with open(fragment_path, "rb") as handle: + fragment_bytes = handle.read() + + reader = Reader("video/mp4", io.BytesIO(init_bytes)) + state = {"fired": False, "result": None, "hung": None} + + class ReentrantStream(io.BytesIO): + """Re-enters the API from another thread, from inside a callback, + and waits for it: the shape that deadlocks a lock held across the + native call. + """ + + def _reenter_once(self): + if state["fired"]: + return + state["fired"] = True + + def second_call(): + try: + reader.with_fragment( + "video/mp4", + io.BytesIO(init_bytes), + io.BytesIO(fragment_bytes)) + state["result"] = "completed" + except Error as e: + state["result"] = e + + thread = threading.Thread(target=second_call, daemon=True) + thread.start() + thread.join(10) + state["hung"] = thread.is_alive() + + def read(self, size=-1): + self._reenter_once() + return super().read(size) + + def seek(self, offset, whence=0): + self._reenter_once() + return super().seek(offset, whence) + + reader.with_fragment("video/mp4", + ReentrantStream(init_bytes), + io.BytesIO(fragment_bytes)) + + self.assertFalse(state["hung"], "re-entrant call blocked") + self.assertIsInstance(state["result"], Error, "must refuse, not interleave") + +class TestStreamCloseReentrancy(unittest.TestCase): + """close() clears the callback references inside _close_lock, which can run + a finalizer at that bytecode boundary, and __del__ takes the same lock. """ + def test_close_can_be_reentered_on_the_same_thread(self): + stream = Stream(io.BytesIO(b"payload")) + finished = threading.Event() + + def hold_then_reenter(): + with stream._close_lock: + # A finalizer running here re-takes the lock this thread holds. + stream.close() + finished.set() + + worker = threading.Thread(target=hold_then_reenter, daemon=True) + worker.start() + + self.assertTrue(finished.wait(10), "re-entrant close() blocked") + + +class TestConsumeReservationWindow(unittest.TestCase): + """The consume reservation must outlast ownership classification. + + The reservation is a mutating in-flight mark. The lifecycle state stays + ACTIVE. _read_native_error() is a native call that releases the GIL, so a + reservation released before the error is classified would let another + thread use a handle native may already own. + """ + + def test_no_thread_sees_a_consumed_handle_as_valid(self): + resource = Settings() + + reading = threading.Event() + may_finish = threading.Event() + seen_valid = [] + + real_read = c2pa_module._read_native_error + + def gated_read(): + # Stand in for the GIL release inside the real native call. + reading.set() + may_finish.wait(10) + # No pre-consume tag: native took ownership and then failed. + return "Other: operation failed after taking ownership" + + def observer(): + if not reading.wait(10): + return + # The consuming call is mid-classification at this point. + seen_valid.append(resource.is_valid) + may_finish.set() + + watcher = threading.Thread(target=observer, daemon=True) + watcher.start() + + c2pa_module._read_native_error = gated_read + try: + with self.assertRaises(Error): + resource._consume_no_replacement(lambda h: 1, "consume: {}") + finally: + c2pa_module._read_native_error = real_read + may_finish.set() + watcher.join(10) + + self.assertFalse(seen_valid[0], "consumed handle seen valid") + + +class TestLocking(unittest.TestCase): + """Tests for the locks that guard native resources: + - the per-object operation lock that serializes native calls against teardown, + - the fragment lock, + - cross-thread creation/closing/releasing. + + Every join here is bounded: + A deadlock must fail the test when timing out, not hang the suite. + """ + + JOIN_TIMEOUT = 30 + + @classmethod + def setUpClass(cls): + cls.data_dir = FIXTURES_FOLDER + with open(DEFAULT_TEST_FILE, 'rb') as handle: + cls.image_bytes = handle.read() + with open(os.path.join(FIXTURES_FOLDER, + "es256_certs.pem"), 'rb') as handle: + cls.certs = handle.read() + with open(os.path.join(FIXTURES_FOLDER, + "es256_private.key"), 'rb') as handle: + cls.private_key = handle.read() + def setUp(self): # Flush pending finalizers through the real free first. gc.collect() self.freed = [] self._real_free = ManagedResource._free_native_ptr - ManagedResource._free_native_ptr = staticmethod(self.freed.append) + # Registered first so it runs last, after every patch has unwound. + self.addCleanup(self._assert_free_hook_restored) + _patch_free(self, self.freed.append) - def tearDown(self): - ManagedResource._free_native_ptr = self._real_free + def _assert_free_hook_restored(self): + self.assertIs( + ManagedResource._free_native_ptr, self._real_free, + "{} leaked a _free_native_ptr patch".format(self.id())) + + def _join_all(self, threads, what): + for thread in threads: + thread.join(self.JOIN_TIMEOUT) + stuck = [t for t in threads if t.is_alive()] + self.assertEqual( + stuck, [], + "{} did not finish within {}s: deadlock".format( + what, self.JOIN_TIMEOUT)) + + def _run_isolated(self, body, timeout=180): + """Run body in a subprocess and return it, + so that crashes can be caught and do not crash the suite itself. + """ + source = textwrap.dedent(body) + return subprocess.run( + [sys.executable, "-c", source], + cwd=os.path.dirname(os.path.dirname(os.path.abspath(__file__))), + capture_output=True, + timeout=timeout, + ) def _free_counts(self): counts = {} @@ -2939,7 +2871,6 @@ def _free_counts(self): def test_cross_thread_create_and_close_frees_exactly_once(self): count = 300 - pid = os.getpid() def create(index): res = _ConcreteResource() @@ -2951,8 +2882,6 @@ def create(index): # Created on worker threads, closed on the main thread. for res in created: - self.assertEqual(res._owner_pid, pid) - self.assertFalse(is_foreign_process(res)) res.close() with concurrent.futures.ThreadPoolExecutor(max_workers=8) as pool: @@ -2989,13 +2918,117 @@ def make_and_drop(index): counts = {handle: count for handle, count in self._free_counts().items() if 0x30000 <= handle < 0x30000 + 200} - self.assertEqual(len(counts), 200, - "dropped resources were not all freed") - self.assertEqual(set(counts.values()), {1}, - "a dropped resource was freed more than once") + self.assertEqual(counts, dict.fromkeys(range(0x30000, 0x30000 + 200), 1)) + + def test_close_queued_inside_a_lock_hold_is_not_orphaned(self): + resource = _ConcreteResource() + resource._activate(0x60001) + + real_lock = resource._op_lock + closed = threading.Event() + join_timeout = self.JOIN_TIMEOUT + + class GatedLock: + def acquire(self, blocking=True, timeout=-1): + if timeout == -1: + return real_lock.acquire(blocking) + return real_lock.acquire(blocking, timeout) + + def release(self): + return real_lock.release() + + def __enter__(self): + real_lock.acquire() + return self + + def __exit__(self, exc_type, exc_val, exc_tb): + if not closed.is_set(): + worker = threading.Thread( + target=lambda: (resource.close(), closed.set()), + daemon=True) + worker.start() + worker.join(join_timeout) + real_lock.release() + return False + + resource._op_lock = GatedLock() + try: + with resource._guarded_op(): + pass + finally: + resource._op_lock = real_lock + + self.assertTrue(closed.is_set(), "close() was never injected") + self.assertEqual(self._free_counts().get(0x60001), 1, "orphaned") + + def test_closed_reader_never_serves_cached_manifest(self): + reader = Reader("image/jpeg", io.BytesIO(self.image_bytes)) + reader.get_active_manifest() + + parked = threading.Event() + go = threading.Event() + real_release = reader._release + + def gated_release(): + parked.set() + go.wait(self.JOIN_TIMEOUT) + real_release() + + reader._release = gated_release + closer = threading.Thread(target=reader.close, daemon=True) + closer.start() + self.assertTrue(parked.wait(self.JOIN_TIMEOUT)) + + served = [] + + def read(): + try: + served.append(reader.get_active_manifest()) + except Error as e: + served.append(e) + + reader_thread = threading.Thread(target=read, daemon=True) + reader_thread.start() + reader_thread.join(1) + go.set() + self._join_all([closer, reader_thread], "close and read") + + self.assertIsInstance(served[0], Error, "closed Reader served cache") + + def test_release_may_wait_on_a_thread_that_takes_the_op_lock(self): + class Polled(_ConcreteResource): + def __init__(self): + super().__init__() + self.go = threading.Event() + self.worker = None + + def _shutdown_path(self): + self.go.wait(TestLocking.JOIN_TIMEOUT) + try: + with self._guarded_op(): + pass + except Error: + pass + + def _release(self): + self.go.set() + self.worker.join() + + resource = Polled() + resource._activate(0x52000) + resource.worker = threading.Thread( + target=resource._shutdown_path, daemon=True) + resource.worker.start() + + closer = threading.Thread(target=resource.close, daemon=True) + closer.start() + closer.join(5) + if closer.is_alive(): + resource.worker = threading.Thread(target=lambda: None) + self.assertFalse(closer.is_alive(), "close() blocked") def test_settings_relayed_across_threads_stays_usable(self): - ManagedResource._free_native_ptr = self._real_free + _patch_free(self, self._real_free) manifest = { "claim_generator": "threaded_stamp_test", @@ -3003,7 +3036,6 @@ def test_settings_relayed_across_threads_stays_usable(self): "assertions": [], } settings = Settings() - pid = os.getpid() results = [] errors = [] @@ -3011,8 +3043,7 @@ def build_context_and_builder(): try: context = Context(settings=settings) builder = Builder(manifest, context=context) - results.append(( - builder._owner_pid, context._owner_pid, builder.is_valid)) + results.append(builder.is_valid) builder.close() context.close() except Exception as exc: @@ -3026,14 +3057,1184 @@ def build_context_and_builder(): settings.close() + self.assertEqual(results, [True] * 8, errors) + + def test_json_racing_finalizer_does_not_crash(self): + """Readers used on one thread while others are collected. + """ + result = self._run_isolated(""" + import sys, io, gc, random, threading, time + sys.path.insert(0, "src") + from c2pa import Reader + + data = open("tests/fixtures/C.jpg", "rb").read() + stop = threading.Event() + pool, lock = [], threading.Lock() + + def worker(): + while not stop.is_set(): + choice = random.random() + try: + if choice < 0.40: + reader = Reader("image/jpeg", io.BytesIO(data)) + with lock: + pool.append(reader) + elif choice < 0.75: + with lock: + snapshot = list(pool) + if snapshot: + reader = random.choice(snapshot) + reader._manifest_json_str_cache = None + reader.json() + elif choice < 0.90: + with lock: + reader = pool.pop(0) if pool else None + if reader: + reader.close() + else: + with lock: + if len(pool) > 20: + del pool[0:5] + gc.collect() + except Exception: + pass + + threads = [threading.Thread(target=worker) for _ in range(12)] + for thread in threads: + thread.start() + deadline = time.time() + 10 + while time.time() < deadline: + time.sleep(0.05) + stop.set() + for thread in threads: + thread.join(30) + """) + self.assertEqual(result.returncode, 0, result.stderr.decode()[-800:]) + + def test_stream_callback_blocking_on_other_thread_does_not_deadlock(self): + """A stream callback that blocks on another thread + which touches the same object. + + A lock held across construction deadlocks here, whether it is global + or per-object. + """ + data = self.image_bytes + target = Reader("image/jpeg", io.BytesIO(data)) + errors = [] + + class BlockingStream(io.BytesIO): + def readinto(self, buffer): + def use(): + try: + target._manifest_json_str_cache = None + target.json() + except Exception: + pass + + helper = threading.Thread(target=use) + helper.start() + helper.join(10) + if helper.is_alive(): + errors.append("helper stuck inside stream callback") + return super().readinto(buffer) + + def body(): + try: + for _ in range(5): + Reader("image/jpeg", BlockingStream(data)) + except Exception as exc: + errors.append(repr(exc)) + + thread = threading.Thread(target=body) + thread.start() + self._join_all([thread], "callback blocking on another thread") self.assertEqual(errors, []) - self.assertEqual(len(results), 8) - for builder_pid, context_pid, valid in results: - self.assertEqual(builder_pid, pid) - self.assertEqual(context_pid, pid) - self.assertTrue(valid) - self.assertEqual(settings._owner_pid, pid) + target.close() + def test_no_nested_op_locks(self): + """No code path may hold two resources' operation locks at once. + With only one lock ever held, no cycle can form here. + """ + data = self.image_bytes + held = threading.local() + violations = [] + real_lock = ManagedResource._guarded_op + real_live_op_lock = ManagedResource._live_op_lock + + def make_tracking(real): + def tracking(resource, **kw): + lock = real(resource, **kw) + depth = getattr(held, 'stack', None) + if depth is None: + depth = held.stack = [] + + class Tracked: + def __enter__(self): + others = [r for r in depth if r is not resource] + if others: + violations.append( + "{} while holding {}".format( + type(resource).__name__, + [type(o).__name__ for o in others])) + depth.append(resource) + return lock.__enter__() + + def __exit__(self, *exc): + depth.pop() + return lock.__exit__(*exc) + + return Tracked() + return tracking + + ManagedResource._guarded_op = make_tracking(real_lock) + ManagedResource._live_op_lock = make_tracking(real_live_op_lock) + try: + reader = Reader("image/jpeg", io.BytesIO(data)) + reader.json() + reader.detailed_json() + reader.is_embedded() + reader.get_remote_url() + reader.close() + finally: + ManagedResource._guarded_op = real_lock + ManagedResource._live_op_lock = real_live_op_lock + + self.assertEqual(violations, [], "nested op locks") + + def test_native_section_deferred_free_is_thread_local(self): + """Two threads each with their own open native-error section: one + thread's section closing must not flush a free deferred inside + the other thread's still-open section. + """ + freed = self._counted_free() + resource = _ConcreteResource() + resource._activate(0x1001) + + thread_ready = threading.Event() + release_thread = threading.Event() + + def worker(): + with _native_section(): + resource.close() + thread_ready.set() + release_thread.wait(self.JOIN_TIMEOUT) + # Flush happens here, on the worker thread, once its own + # section closes. + + thread = threading.Thread(target=worker) + thread.start() + try: + self.assertTrue(thread_ready.wait(self.JOIN_TIMEOUT)) + + # A section opened and closed entirely on this (main) thread, + # while the worker's section is still open on its own thread. + with _native_section(): + pass + + self.assertEqual(freed, [], "other thread's section flushed") + finally: + release_thread.set() + self._join_all([thread], "native-section worker") + + self.assertEqual(freed, [0x1001], "own section never flushed") + + def _counted_free(self): + """Patch _free_native_ptr to count frees; returns the list.""" + freed = [] + real = ManagedResource._free_native_ptr + + def counting(ptr): + freed.append(ptr) + return real(ptr) + + _patch_free(self, counting) + return freed + + def _thumbnail_uri(self, reader): + manifests = json.loads(reader.json()).get("manifests", {}) + for manifest in manifests.values(): + thumbnail = manifest.get("thumbnail") + if thumbnail and thumbnail.get("identifier"): + return thumbnail["identifier"] + self.skipTest("fixture has no thumbnail resource to stream") + + def test_close_inside_callback_defers_free(self): + """A close() from inside a stream callback must not free the handle + the native call is still using.""" + freed = self._counted_free() + reader = Reader("image/jpeg", io.BytesIO(self.image_bytes)) + uri = self._thumbnail_uri(reader) + during = [] + + class Closer(io.BytesIO): + def write(self, buffer): + reader.close() + during.append(len(freed)) + return super().write(buffer) + + try: + reader.resource_to_stream(uri, Closer()) + except Error: + pass + + self.assertEqual(during, [0], "handle was freed mid-call") + self.assertEqual(len(freed), 1) + + def test_with_fragment_closes_main_stream_when_second_stream_fails(self): + """Streams in with_fragment on failure must not get into a broken state""" + opened = [] + real_init = Stream.__init__ + + def tracking_init(wrapper, source): + if opened: + raise ValueError("fragment stream could not be built") + real_init(wrapper, source) + opened.append(wrapper) + + reader = Reader("image/jpeg", io.BytesIO(self.image_bytes)) + self.addCleanup(reader.close) + + with patch.object(Stream, '__init__', tracking_init): + with self.assertRaises(ValueError): + reader.with_fragment( + "video/mp4", + io.BytesIO(self.image_bytes), + io.BytesIO(self.image_bytes)) + + self.assertTrue(opened[0].closed, "main stream left open") + + def test_use_after_deferred_close_is_rejected(self): + """Deferring must not leave the resource usable: + the free is pending, so the handle is about to go away.""" + reader = Reader("image/jpeg", io.BytesIO(self.image_bytes)) + uri = self._thumbnail_uri(reader) + states = [] + + class Closer(io.BytesIO): + def write(self, buffer): + reader.close() + states.append(reader._lifecycle_state) + try: + reader.json() + states.append("json succeeded") + except Error: + states.append("json rejected") + return super().write(buffer) + + try: + reader.resource_to_stream(uri, Closer()) + except Error: + pass + + self.assertEqual(states[1], "json rejected") + + def test_release_raising_during_deferred_teardown_does_not_leak(self): + """The deferred free survives a failing _release: + the handle must still be freed.""" + freed = self._counted_free() + reader = Reader("image/jpeg", io.BytesIO(self.image_bytes)) + uri = self._thumbnail_uri(reader) + + def boom(self): + raise RuntimeError("release failure") + + class Closer(io.BytesIO): + def write(self, buffer): + reader.close() + return super().write(buffer) + + with patch.object(Reader, '_release', boom): + try: + reader.resource_to_stream(uri, Closer()) + except Error: + pass + + self.assertEqual(len(freed), 1, "handle leaked") + + def test_concurrent_closes_during_callback_free_once(self): + """Many threads closing while one native call is in flight + must produce exactly one free (avoid double-frees, + or freeing something the object wouldn't own).""" + freed = self._counted_free() + reader = Reader("image/jpeg", io.BytesIO(self.image_bytes)) + uri = self._thumbnail_uri(reader) + started = threading.Event() + closers = [] + during = [] + + class Slow(io.BytesIO): + def write(self, buffer): + started.set() + time.sleep(0.3) + during.append(len(freed)) + return super().write(buffer) + + def closer(): + started.wait(self.JOIN_TIMEOUT) + reader.close() + + for _ in range(8): + thread = threading.Thread(target=closer) + closers.append(thread) + thread.start() + try: + reader.resource_to_stream(uri, Slow()) + except Error: + pass + self._join_all(closers, "concurrent closers") + + self.assertEqual(during[:1], [0], "handle was freed mid-call") + self.assertEqual(len(freed), 1) + + def _borrow_resource(self): + """An ACTIVE resource with no native handle behind it.""" + res = _ConcreteResource() + res._lifecycle_state = LifecycleState.ACTIVE + res._handle = ctypes.c_void_p(1) + return res + + def _park(self, res, enter): + """Hold `enter(res)` open on another thread. + Returns (thread, release).""" + inside = threading.Event() + release = threading.Event() + + def holder(): + with enter(res): + inside.set() + release.wait(self.JOIN_TIMEOUT) + + thread = threading.Thread(target=holder, daemon=True) + thread.start() + self.assertTrue(inside.wait(self.JOIN_TIMEOUT), + "holder never entered its native call") + return thread, release + + def test_is_valid_false_during_exclusive_guarded_op(self): + res = self._borrow_resource() + thread, release = self._park( + res, lambda r: r._guarded_op(exclusive=True)) + try: + self.assertFalse(res.is_valid) + finally: + release.set() + self._join_all([thread], "exclusive guarded holder") + self.assertTrue(res.is_valid) + + def test_reservation_matrix(self): + """Readers share, writers exclude everyone. + + In flight: the two reservation kinds (a _guarded_op holds the lock, + so a second caller blocks on it rather than being refused). + """ + def consume(r): + r._consume_no_replacement(lambda h: 0, "unused: {}") + + def entered(cm): + def run(r): + with cm(r): + pass + return run + + in_flight = { + "shared": lambda r: r._native_call(), + "exclusive": lambda r: r._exclusive_native_call(), + } + attempts = { + "shared": entered(lambda r: r._native_call()), + "exclusive": entered(lambda r: r._exclusive_native_call()), + "guarded": entered(lambda r: r._guarded_op()), + "guarded_exclusive": entered( + lambda r: r._guarded_op(exclusive=True)), + "consume": consume, + } + admitted = {("shared", "shared"), ("shared", "guarded")} + + wrong = [] + for held_name, held in in_flight.items(): + for new_name, attempt in attempts.items(): + res = self._borrow_resource() + thread, release = self._park(res, held) + try: + try: + attempt(res) + got = True + except Error: + got = False + finally: + release.set() + self._join_all([thread], "holder") + want = (held_name, new_name) in admitted + if got != want: + wrong.append("{} in flight, {} {}".format( + held_name, new_name, + "admitted" if got else "refused")) + + self.assertEqual(wrong, []) + + def test_settings_update_refused_during_settings_borrow(self): + """Settings.update/set take &mut. + Context construction borrows the same Settings as &. + The mutation must be refused. + """ + settings = Settings() + lib = c2pa_module._lib + real_set_settings = lib.c2pa_context_builder_set_settings + parked = threading.Event() + release = threading.Event() + + def gated_set_settings(builder, handle): + parked.set() + release.wait(self.JOIN_TIMEOUT) + return real_set_settings(builder, handle) + + built = [] + lib.c2pa_context_builder_set_settings = gated_set_settings + thread = threading.Thread( + target=lambda: built.append(Context(settings=settings)), + daemon=True) + try: + thread.start() + parked.wait(self.JOIN_TIMEOUT) + with self.assertRaises(Error): + settings.update({"builder": {"thumbnail": {"enabled": False}}}) + with self.assertRaises(Error): + settings.set("builder.thumbnail.enabled", "false") + finally: + release.set() + self._join_all([thread], "Context construction") + lib.c2pa_context_builder_set_settings = real_set_settings + for context in built: + context.close() + settings.close() + + def test_deferred_consume_is_not_upgraded_to_free(self): + """A deferred consuming teardown must not be overwritten by a later + free intent arriving while the same call is still in flight. + + Scenario: a Signer shared across concurrent signs: sign borrows + the handle (holding the in-flight guard) while Context.__init__ + consumes it. + """ + freed = self._counted_free() + reader = Reader("image/jpeg", io.BytesIO(self.image_bytes)) + + with reader._native_call(): + # The consuming call: native took ownership, so nothing here frees. + reader._teardown(free_handle=False) + + # A free intent arriving behind it, past a stale state check. + reader._teardown(free_handle=True) + + self.assertEqual(freed, [], "freed a consumed handle") + + def test_concurrent_close_runs_release_once(self): + """Two racing close() calls on one instance must run _release() + exactly once. + + The native free is already single (the handle is nulled after the + first teardown), so a free-counting test cannot see this: it is + What must not run twice is _release(), the Python-side + stream/cache cleanup a subclass overrides. _teardown() has to be + idempotent under its own lock. + + Gate _teardown so the first close() pauses on entry, before taking + the lock; the second then runs a full teardown (release + free + + mark closed); the first resumes and must find the resource already + released and do nothing. + """ + join_timeout = self.JOIN_TIMEOUT + orig_teardown = ManagedResource._teardown + + reader = Reader("image/jpeg", io.BytesIO(self.image_bytes)) + release_calls = [] + orig_release = reader._release + + def counting_release(_orig=orig_release, _calls=release_calls): + _calls.append(1) + _orig() + + reader._release = counting_release + + call_count = {"n": 0} + count_lock = threading.Lock() + first_arrived = threading.Event() + release_first = threading.Event() + + def gated_teardown(self, free_handle, _target=reader, + _timeout=join_timeout): + if self is _target: + with count_lock: + call_count["n"] += 1 + is_first = call_count["n"] == 1 + if is_first: + first_arrived.set() + release_first.wait(_timeout) + return orig_teardown(self, free_handle) + + with patch.object(ManagedResource, '_teardown', gated_teardown): + t1 = threading.Thread(target=reader.close) + t1.start() + self.assertTrue(first_arrived.wait(join_timeout)) + + t2 = threading.Thread(target=reader.close) + t2.start() + t2.join(join_timeout) + + release_first.set() + self._join_all([t1], "paused close() resuming") + + self.assertEqual(len(release_calls), 1, "_release() not idempotent") + + def test_sign_raising_native_call_closes_builder(self): + freed = self._counted_free() + signer = Signer.from_info(C2paSignerInfo( + alg=b"es256", sign_cert=self.certs, + private_key=self.private_key, ta_url=None)) + self.addCleanup(signer.close) + builder = Builder({"claim_generator": "python_test", + "format": "image/jpeg", "assertions": []}) + real_sign = c2pa_module._lib.c2pa_builder_sign + + def raising(*args): + raise RuntimeError("native sign raised") + + c2pa_module._lib.c2pa_builder_sign = raising + try: + with self.assertRaises(Error): + builder.sign(signer, "image/jpeg", + io.BytesIO(self.image_bytes), io.BytesIO()) + finally: + c2pa_module._lib.c2pa_builder_sign = real_sign + self.assertEqual(len([f for f in freed if f]), 1) + + def test_every_callback_running_method_is_guarded(self): + """Every method that hands a Stream to the native lib must be guarded, + except the construction paths. + """ + source = inspect.getsource(sys.modules[Reader.__module__]) + lines = source.split("\n") + class_a = { + ("Reader", "_create_reader"), + ("Reader", "_init_from_context"), + ("Builder", "from_archive"), + } + stream_use = re.compile( + r"(_stream|stream_obj|source_stream|dest_stream|main_obj" + r"|frag_obj)\._stream") + + bodies = {} + current_class = current_method = None + start = None + for index, line in enumerate(lines): + if re.match(r"^class ", line): + current_class = line.split("(")[0].replace( + "class ", "").strip(":") + if re.match(r"^def ", line): + current_class = None + match = re.match(r"^ def (\w+)", line) + if match: + if current_class and current_method and start is not None: + bodies[(current_class, current_method)] = "\n".join( + lines[start:index]) + current_method = match.group(1) + start = index + if current_class and current_method and start is not None: + bodies[(current_class, current_method)] = "\n".join(lines[start:]) + + unguarded = [] + checked = 0 + for key, body in bodies.items(): + if not stream_use.search(body): + continue + checked += 1 + if key in class_a: + continue + if ("_native_call()" not in body + and "_exclusive_native_call()" not in body + and "_consume_and_swap(" not in body): + unguarded.append("{}.{}".format(*key)) + + self.assertGreater(checked, 0, "coverage scan found no methods") + self.assertEqual(unguarded, []) + + def test_every_borrowed_handle_is_guarded(self): + """When a method hands a second object's handle to the native library, + that object needs its own _native_call() guard. + + This can happen in callbacks, where you can't express whose handle + is the one needing attention. + """ + module = sys.modules[Reader.__module__] + tree = ast.parse(inspect.getsource(module)) + + # Attributes that carry a native handle out of an object. + handle_attrs = {"_handle", "execution_context"} + + def guarded_names(node): + """Names X guarded at this node, by either form: + `with X._native_call():`, or `with _context_guard(X):` for a + caller-supplied ContextProvider, which enters X._native_call() + when X offers it. + """ + found = set() + calls = [] + for item in getattr(node, "items", []): + expr = item.context_expr + if isinstance(expr, ast.IfExp): + calls.extend([expr.body, expr.orelse]) + else: + calls.append(expr) + for call in calls: + if not isinstance(call, ast.Call): + continue + if (isinstance(call.func, ast.Attribute) + and call.func.attr == "_native_call" + and isinstance(call.func.value, ast.Name)): + found.add(call.func.value.id) + elif (isinstance(call.func, ast.Name) + and call.func.id == "_context_guard" + and call.args + and isinstance(call.args[0], ast.Name)): + found.add(call.args[0].id) + return found + + def borrowed_in_call(call): + """Names X whose handle this _lib.* call receives, X not self.""" + if not (isinstance(call.func, ast.Attribute) + and isinstance(call.func.value, ast.Name) + and call.func.value.id == "_lib"): + return set() + names = set() + for arg in ast.walk(call): + if (isinstance(arg, ast.Attribute) + and arg.attr in handle_attrs + and isinstance(arg.value, ast.Name) + and arg.value.id != "self"): + names.add(arg.value.id) + return names + + def locally_owned(method): + """A resource created inside the method never escapes to another + thread, so nothing can close it mid-call and it needs no guard. + """ + owned = set() + for node in ast.walk(method): + # `with self._NativeContextBuilder() as context_builder:` / `x = Foo()` + if isinstance(node, (ast.With, ast.AsyncWith)): + for item in node.items: + if (isinstance(item.context_expr, ast.Call) + and isinstance(item.optional_vars, ast.Name)): + owned.add(item.optional_vars.id) + elif isinstance(node, ast.Assign): + if isinstance(node.value, ast.Call): + for target in node.targets: + if isinstance(target, ast.Name): + owned.add(target.id) + return owned + + unguarded = [] + checked = 0 + + for cls in ast.walk(tree): + if not isinstance(cls, ast.ClassDef): + continue + for method in cls.body: + if not isinstance(method, (ast.FunctionDef, + ast.AsyncFunctionDef)): + continue + owned = locally_owned(method) + + # Walk the body tracking which guards are open, so a borrowed + # handle is only accepted when its own guard encloses the use. + def visit(node, active): + nonlocal checked + if isinstance(node, (ast.With, ast.AsyncWith)): + active = active | guarded_names(node) + if isinstance(node, ast.Call): + for name in borrowed_in_call(node) - owned: + checked += 1 + if name not in active: + unguarded.append( + "{}.{} passes {}._handle to native " + "without {}._native_call()".format( + cls.name, method.name, name, name)) + for child in ast.iter_child_nodes(node): + visit(child, active) + + visit(method, frozenset()) + + self.assertGreater(checked, 0, "scan found nothing") + self.assertEqual(unguarded, [], "unguarded: " + ", ".join(unguarded)) + + # FFI functions that take their receiver (first argument) as &mut. + MUTATING_FFI = frozenset({ + "c2pa_settings_set_value", + "c2pa_settings_update_from_string", + "c2pa_context_builder_set_settings", + "c2pa_builder_set_intent", + "c2pa_builder_set_no_embed", + "c2pa_builder_set_remote_url", + "c2pa_builder_add_action", + "c2pa_builder_add_resource", + "c2pa_builder_add_ingredient_from_stream", + "c2pa_builder_add_ingredient_from_archive", + "c2pa_builder_sign", + "c2pa_builder_sign_context", + }) + + def test_every_mutating_ffi_call_is_exclusive(self): + """A native call taking its receiver as &mut must run under an + exclusive guard on that receiver. + """ + tree = ast.parse(inspect.getsource(sys.modules[Reader.__module__])) + + def exclusive_names(node): + found = set() + for item in getattr(node, "items", []): + call = item.context_expr + if not (isinstance(call, ast.Call) + and isinstance(call.func, ast.Attribute) + and isinstance(call.func.value, ast.Name)): + continue + attr = call.func.attr + exclusive_kw = any( + kw.arg == "exclusive" + and isinstance(kw.value, ast.Constant) + and kw.value.value is True + for kw in call.keywords) + if (attr == "_exclusive_native_call" + or (attr == "_guarded_op" and exclusive_kw)): + found.add(call.func.value.id) + return found + + def receiver(call): + if call.args: + first = call.args[0] + if (isinstance(first, ast.Attribute) + and first.attr == "_handle" + and isinstance(first.value, ast.Name)): + return first.value.id + return None + + seen = set() + unguarded = [] + + def visit(node, active, where): + if isinstance(node, (ast.With, ast.AsyncWith)): + active = active | exclusive_names(node) + if (isinstance(node, ast.Call) + and isinstance(node.func, ast.Attribute) + and isinstance(node.func.value, ast.Name) + and node.func.value.id == "_lib" + and node.func.attr in self.MUTATING_FFI): + seen.add(node.func.attr) + name = receiver(node) + if name is None or name not in active: + unguarded.append("{} calls {} without an exclusive " + "guard on {}".format( + where, node.func.attr, name)) + for child in ast.iter_child_nodes(node): + visit(child, active, where) + + for cls in ast.walk(tree): + if isinstance(cls, ast.ClassDef): + for method in cls.body: + if isinstance(method, (ast.FunctionDef, + ast.AsyncFunctionDef)): + visit(method, frozenset(), + "{}.{}".format(cls.name, method.name)) + + # Positive control: verify call seen. + self.assertEqual(seen, set(self.MUTATING_FFI)) + self.assertEqual(unguarded, [], ", ".join(unguarded)) + + def test_no_native_call_under_a_shared_guarded_op(self): + """A Reader's direct native calls run under a reservation, so they do + not serialize on one Reader. The manifest-field getters still share + the op lock through _get_cached_manifest_data. + """ + tree = ast.parse(inspect.getsource(sys.modules[Reader.__module__])) + + def is_shared_guarded_op(node): + for item in node.items: + call = item.context_expr + if (isinstance(call, ast.Call) + and isinstance(call.func, ast.Attribute) + and call.func.attr == "_guarded_op" + and not any(kw.arg == "exclusive" + for kw in call.keywords)): + return True + return False + + found = set() + + def visit(node, shared, where): + if isinstance(node, ast.With) and is_shared_guarded_op(node): + shared = True + if (shared and isinstance(node, ast.Call) + and isinstance(node.func, ast.Attribute) + and isinstance(node.func.value, ast.Name) + and node.func.value.id == "_lib"): + found.add((where, node.func.attr)) + for child in ast.iter_child_nodes(node): + visit(child, shared, where) + + for cls in ast.walk(tree): + if isinstance(cls, ast.ClassDef): + for method in cls.body: + if isinstance(method, ast.FunctionDef): + visit(method, False, + "{}.{}".format(cls.name, method.name)) + + self.assertEqual( + sorted(f for f in found if f[0].startswith("Reader.")), []) + + def _callback_signer_source(self): + """Shared subprocess preamble: an ES256 callback signer.""" + return """ + import io, os, sys, threading, time + from c2pa import (Builder, Context, Signer, + C2paSigningAlg as SigningAlg) + from cryptography.hazmat.primitives import hashes, serialization + from cryptography.hazmat.primitives.asymmetric import ec + + data_dir = sys.argv[1] + certs = open(os.path.join(data_dir, + "es256_certs.pem"), "rb").read().decode() + key_path = os.path.join(data_dir, "es256_private.key") + key = open(key_path, "rb").read() + img = open(os.path.join(data_dir, "C.jpg"), "rb").read() + manifest = {"claim_generator_info": + [{"name": "test", "version": "0.1"}], + "assertions": []} + private_key = serialization.load_pem_private_key( + key, password=None) + + def sign_callback(data): + return private_key.sign(data, ec.ECDSA(hashes.SHA256())) + + def make_context(): + return Context(signer=Signer.from_callback( + sign_callback, SigningAlg.ES256, certs, + "http://timestamp.digicert.com")) +""" + + def test_context_close_during_context_sign_does_not_crash(self): + """Closing a Context must not free the signer callback mid-sign. + + Context.__init__ pins the consumed signer's ctypes callback so it + outlives the Signer object, and Context._release() drops that pin. + Without an in-flight guard on the Context, a close() on another thread + runs _release() while c2pa_builder_sign_context is calling through the + trampoline, and the process dies with SIGSEGV. + + Runs in a subprocess: the failure mode is a segfault, which would take + the test runner down with it otherwise. + """ + source = textwrap.dedent(self._callback_signer_source() + """ + for trial in range(60): + ctx = make_context() + entered = threading.Event() + + def worker(): + try: + builder = Builder(dict(manifest), context=ctx) + entered.set() + builder.sign("image/jpeg", io.BytesIO(img), + io.BytesIO()) + builder.close() + except Exception: + # A closed context may be rejected; + # only a crash is a failure here. + entered.set() + + t = threading.Thread(target=worker) + t.start() + entered.wait(5) + time.sleep(0.002) + ctx.close() + t.join(20) + print("OK") + """) + + result = subprocess.run( + [sys.executable, "-c", source, self.data_dir], + capture_output=True, text=True, timeout=300) + + self.assertEqual(result.returncode, 0, result.stderr[-2000:]) + self.assertIn("OK", result.stdout) + + def test_section_drain_error_does_not_mask_the_body_error(self): + """The body's exception is what the caller asked for, so it wins.""" + + class FlushRaises: + _pending_teardown = True + + def _maybe_flush_pending(self): + raise RuntimeError("flush failed") + + class BodyError(Exception): + pass + + with self.assertLogs('c2pa', level='ERROR') as logs: + with self.assertRaises(BodyError): + with _native_section(): + c2pa_module._register_for_section_flush(FlushRaises()) + raise BodyError("the error the caller cares about") + + self.assertTrue(any("flush failed" in line for line in logs.output)) + + def test_context_sign_after_close_raises_rather_than_skipping_signer(self): + """Signing through a closed Context must raise, not silently succeed. + + Context._release() has already dropped the pinned callback, so the + native side signs without ever invoking it: the call returns a + manifest of the same size while the caller's signing callback runs + zero times. Refusing the call is what makes that visible. + + Runs in a subprocess because the callback signer needs the + cryptography package, which this module does not otherwise import. + """ + source = textwrap.dedent(self._callback_signer_source() + """ + calls = [] + + def counting_callback(data): + calls.append(1) + return private_key.sign(data, ec.ECDSA(hashes.SHA256())) + + signer = Signer.from_callback( + counting_callback, SigningAlg.ES256, certs, + "http://timestamp.digicert.com") + ctx = Context(signer=signer) + builder = Builder(dict(manifest), context=ctx) + ctx.close() + + try: + builder.sign("image/jpeg", io.BytesIO(img), io.BytesIO()) + print("SIGNED_WITH_CALLS", len(calls)) + except Exception as exc: + print("RAISED", type(exc).__name__, len(calls)) + """) + + result = subprocess.run( + [sys.executable, "-c", source, self.data_dir], + capture_output=True, text=True, timeout=300) + + self.assertIn("RAISED", result.stdout, result.stderr[-2000:]) + + def test_close_during_concurrent_sign_does_not_crash(self): + """A Signer shared across threads must not be freed mid-sign. + + Builder.sign borrows the signer's handle for the duration of the + native call. Without a guard on the signer itself, a close() on + another thread frees that handle while c2pa_builder_sign is using + it, and the process dies with SIGSEGV instead of raising. + + Rotates a shared signer while other threads sign with it. Runs in a + subprocess: the failure mode is a segfault, which would take the + test runner down with it otherwise. + """ + source = textwrap.dedent(""" + import io, os, sys, threading + from c2pa import (Builder, Signer, C2paSignerInfo, + C2paSigningAlg as SigningAlg) + + data_dir = sys.argv[1] + certs = open(os.path.join(data_dir, "es256_certs.pem"), "rb").read() + key = open(os.path.join(data_dir, "es256_private.key"), "rb").read() + img = open(os.path.join(data_dir, "C.jpg"), "rb").read() + manifest = {"claim_generator_info": + [{"name": "test", "version": "0.1"}], + "assertions": []} + + def make(): + return Signer.from_info(C2paSignerInfo( + SigningAlg.ES256, certs, key, None)) + + box = {"signer": make(), "stop": False} + + def rotate(): + while not box["stop"]: + old = box["signer"] + try: + box["signer"] = make() + old.close() + except Exception: + pass + + def sign(): + for _ in range(120): + if box["stop"]: + return + try: + b = Builder(manifest) + b.sign(box["signer"], "image/jpeg", + io.BytesIO(img), io.BytesIO()) + b.close() + except Exception: + # A closed signer may be rejected; + # only a crash is a failure here. + pass + + rot = threading.Thread(target=rotate, daemon=True) + rot.start() + threads = [threading.Thread(target=sign) for _ in range(8)] + for t in threads: + t.start() + for t in threads: + t.join() + box["stop"] = True + rot.join(timeout=5) + print("OK") + """) + + result = subprocess.run( + [sys.executable, "-c", source, self.data_dir], + capture_output=True, text=True, timeout=300) + + self.assertEqual(result.returncode, 0, result.stderr[-2000:]) + self.assertIn("OK", result.stdout) + + +class TestSwapConsumeExclusion(unittest.TestCase): + """with_archive, with_fragment must be rejected during other in-flight calls. + """ + + _MANIFEST = { + "claim_generator": "c2pa_python_test", + "claim_generator_info": [{ + "name": "c2pa_python_test", + "version": "0.1.0", + }], + "format": "image/jpeg", + "title": "Python Test", + "ingredients": [], + "assertions": [], + } + + def _archive_bytes(self): + builder = Builder(self._MANIFEST) + try: + archive = io.BytesIO() + builder.to_archive(archive) + archive.seek(0) + return archive + finally: + builder.close() + + def test_refused_sign_leaves_builder_usable(self): + with open(os.path.join(FIXTURES_FOLDER, "es256_certs.pem"), "rb") as f: + certs = f.read() + with open(os.path.join(FIXTURES_FOLDER, "es256_private.key"), "rb") as f: + key = f.read() + with open(os.path.join(FIXTURES_FOLDER, "A.jpg"), "rb") as f: + image = f.read() + signer = Signer.from_info(C2paSignerInfo(b"es256", certs, key, None)) + self.addCleanup(signer.close) + manifest = dict(self._MANIFEST, assertions=[{ + "label": "c2pa.actions", + "data": {"actions": [{ + "action": "c2pa.created", + "digitalSourceType": "http://cv.iptc.org/newscodes/" + "digitalsourcetype/digitalCreation", + }]}, + }]) + builder = Builder(manifest) + self.addCleanup(builder.close) + inside = threading.Event() + release = threading.Event() + + class SlowStream(io.BytesIO): + def readinto(self, buffer): + inside.set() + release.wait(10) + return super().readinto(buffer) + + worker = threading.Thread( + target=lambda: builder.add_resource("thumb", SlowStream(image)), + daemon=True) + worker.start() + try: + inside.wait(10) + with self.assertRaises(Error): + builder.sign(signer, "image/jpeg", + io.BytesIO(image), io.BytesIO()) + finally: + release.set() + worker.join(10) + output = io.BytesIO() + builder.sign(signer, "image/jpeg", io.BytesIO(image), output) + self.assertGreater(len(output.getvalue()), 0) + + def test_with_fragment_rejected_when_native_in_progress(self): + init_path = os.path.join(FIXTURES_FOLDER, "dashinit.mp4") + fragment_path = os.path.join(FIXTURES_FOLDER, "dash1.m4s") + with open(init_path, "rb") as f: + init_bytes = f.read() + with open(fragment_path, "rb") as f: + fragment_bytes = f.read() + + reader = Reader("video/mp4", io.BytesIO(init_bytes)) + try: + with reader._native_call(): + with self.assertRaises(Error) as raised: + reader.with_fragment( + "video/mp4", + io.BytesIO(init_bytes), + io.BytesIO(fragment_bytes)) + self.assertIn("in use", str(raised.exception)) + + # The refusal must leave the reader untouched: the swap still + # works once the borrow is gone. + reader.with_fragment( + "video/mp4", + io.BytesIO(init_bytes), + io.BytesIO(fragment_bytes)) + reader.json() + finally: + reader.close() + + def test_close_during_with_archive_defers_and_frees(self): + archive_bytes = self._archive_bytes().getvalue() + builder = Builder(self._MANIFEST) + + inside = threading.Event() + release = threading.Event() + + class BlockingArchive(io.BytesIO): + def read(self, *args): + inside.set() + release.wait(10) + return super().read(*args) + + def seek(self, *args): + inside.set() + release.wait(10) + return super().seek(*args) + + outcome = {} + + def consume(): + try: + builder.with_archive(BlockingArchive(archive_bytes)) + outcome["result"] = "ok" + except Exception as e: # noqa: BLE001 - asserted below + outcome["result"] = e + + worker = threading.Thread(target=consume, daemon=True) + worker.start() + try: + self.assertTrue(inside.wait(10), "callback never reached") + # Defers: the swap is counted in flight. + builder.close() + finally: + release.set() + worker.join(10) + + # The deferred teardown freed the replacement handle: closed for + # good, nothing left to free, exactly one release. + self.assertTrue(builder._released) if __name__ == '__main__': unittest.main()