Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions crashes/context-close-drops-signer-callback-mid-sign/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
The process dies with SIGSEGV (exit code 139, no Python exception) when a
`Context` built from a callback signer is closed on one thread while another
thread runs a context-sign (`Builder(manifest, context=ctx)` followed by
`builder.sign(format, source, dest)`) through it.

40-120 trials:

| Variant | Result |
|---|---|
| Close during concurrent context-sign, callback signer | SIGSEGV, reproducible |
| Same race, `Context._release` patched to keep the callback reference alive | 80/80 clean |
| Same race, context's native free suppressed (release still runs) | still SIGSEGV |
| Same race, info signer (`Signer.from_info`, no Python callback) | 120/120 clean |
| Single-threaded close-then-sign | clean (errors, no crash) |
| Dropping the last `ctx` reference mid-sign (finalizer close) | 80/80 clean |

```
python3 crashes/context-close-drops-signer-callback-mid-sign/repro.py
```

Exit code 139 within a few trials. The script: build a `Context` from
`Signer.from_callback(...)`, start a thread running a context-sign, sleep
~2 ms after the sign begins, call `ctx.close()` from the main thread, join,
repeat.
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
Fatal Python error: Segmentation fault

Current thread 0x000000016e3ab000 (most recent call first):
File "/Users/taniamathern/Desktop/code/c2pa-python/src/c2pa/c2pa.py", line 4032 in _sign_internal
File "/Users/taniamathern/Desktop/code/c2pa-python/src/c2pa/c2pa.py", line 4110 in _sign_common
File "/Users/taniamathern/Desktop/code/c2pa-python/src/c2pa/c2pa.py", line 4186 in sign
File "/private/tmp/claude-501/-Users-taniamathern-Desktop-code-c2pa-python/a1e731b8-8f71-4e3d-a858-5b5d2dfe2dfb/scratchpad/crash/min.py", line 24 in w
File "/Library/Frameworks/Python.framework/Versions/3.13/lib/python3.13/threading.py", line 994 in run
File "/Library/Frameworks/Python.framework/Versions/3.13/lib/python3.13/threading.py", line 1043 in _bootstrap_inner
File "/Library/Frameworks/Python.framework/Versions/3.13/lib/python3.13/threading.py", line 1014 in _bootstrap

Thread 0x00000001efdc1d80 (most recent call first):
File "/Library/Frameworks/Python.framework/Versions/3.13/lib/python3.13/threading.py", line 1094 in join
File "/private/tmp/claude-501/-Users-taniamathern-Desktop-code-c2pa-python/a1e731b8-8f71-4e3d-a858-5b5d2dfe2dfb/scratchpad/crash/min.py", line 31 in <module>

Extension modules: _cffi_backend (total: 1)
60 changes: 60 additions & 0 deletions crashes/context-close-drops-signer-callback-mid-sign/repro.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
"""SIGSEGV reproduction: Context.close() racing a context-sign that uses a
callback signer. Run from the repository root:

python3 crashes/context-close-drops-signer-callback-mid-sign/repro.py

Expected: the process dies with SIGSEGV (exit 139) within a few trials.
The crash needs the `cryptography` package for the ES256 callback.
"""
import sys, io, os, threading, time, faulthandler

sys.path.insert(0, "src")
faulthandler.enable()

from c2pa import Builder, Signer, Context, C2paSigningAlg as Alg
from cryptography.hazmat.primitives import hashes, serialization
from cryptography.hazmat.primitives.asymmetric import ec

FIXTURES = "tests/fixtures"
certs = open(os.path.join(FIXTURES, "es256_certs.pem"), "rb").read().decode()
key_bytes = open(os.path.join(FIXTURES, "es256_private.key"), "rb").read()
image = open(os.path.join(FIXTURES, "C.jpg"), "rb").read()
MANIFEST = {"claim_generator_info": [{"name": "repro", "version": "0.1"}],
"assertions": []}

private_key = serialization.load_pem_private_key(key_bytes, password=None)


def sign_callback(data: bytes) -> bytes:
return private_key.sign(data, ec.ECDSA(hashes.SHA256()))


def make_context() -> Context:
signer = Signer.from_callback(sign_callback, Alg.ES256, certs,
"http://timestamp.digicert.com")
return Context(signer=signer) # consumes the signer


for trial in range(80):
ctx = make_context()
entered = threading.Event()

def worker():
try:
builder = Builder(dict(MANIFEST), context=ctx)
entered.set()
builder.sign("image/jpeg", io.BytesIO(image), io.BytesIO())
builder.close()
except Exception:
entered.set()

t = threading.Thread(target=worker)
t.start()
entered.wait(5)
time.sleep(0.002) # let the sign enter the native call
ctx.close() # drops _signer_callback_cb mid-invocation
t.join(20)
if trial % 20 == 0:
print("trial", trial, "still alive")

print("survived 80 trials (crash did not reproduce this run)")
18 changes: 16 additions & 2 deletions docs/native-resources-management.md
Original file line number Diff line number Diff line change
Expand Up @@ -124,9 +124,23 @@ A lock (Python's `threading.Lock`) can be acquired once, and a second `acquire()

`_op_lock` is an `RLock` rather than a plain `Lock` for two reasons specific to this code. First, a finalizer (`__del__`) can run at any bytecode boundary — including one in the middle of a method that has already acquired the lock on this same thread — so `__del__` calling back into locked code must not deadlock against itself. Second, a consuming call tears the handle down from inside the locked region it is already holding: `_teardown()` is called while `_op_lock` is held, and it needs to acquire the same lock again rather than re-entering as a different, blocked acquisition. `_lock()` returns it, except in a forked child: there it raises `C2paError` immediately rather than blocking, because the thread that might hold the lock at fork time does not exist in the child to release it, and waiting on it would hang forever (see [Fork safety](#fork-safety)).

The lock is never held across a native call that drives a stream callback: construction, `resource_to_stream`, the Builder stream methods, and signing all release the GIL and call back into caller-supplied Python, which may itself call into this API on another thread. Holding `_op_lock` there would deadlock against that reentry. Those calls go through `_native_call()` instead: a context manager that increments `_inflight` under the lock, yields to run the native call unlocked, then decrements `_inflight` on the way out. If `_teardown()` runs while a call is in flight, it records the requested `free_handle` value in `_pending_teardown` and marks the resource `CLOSED` immediately, so no other caller can start using it, but defers the actual free. The last `_native_call()` to exit picks up `_pending_teardown` and runs `_teardown()` for real.
The lock is never held across a native call that drives a stream callback: construction, `resource_to_stream`, the Builder stream methods, and signing all release the Global Interpreter Lock (GIL) and call back into caller-supplied Python, which may itself call into this API on another thread. Holding `_op_lock` there would deadlock against that reentry. Those calls go through `_native_call()` instead: a context manager that increments `_inflight` under the lock, yields to run the native call unlocked, then decrements `_inflight` on the way out. If `_teardown()` runs while a call is in flight, it records the requested `free_handle` value in `_pending_teardown` and marks the resource `CLOSED` immediately, so no other caller can start using it, but defers the actual free. The last `_native_call()` to exit picks up `_pending_teardown` and runs `_teardown()` for real.

`Context.__init__` wraps the signer hand-off in `signer._native_call()`, so a `signer.close()` on another thread cannot free the handle between the state check and the consuming call. `Builder._sign_internal` wraps the sign call in `self._native_call()` and, when an explicit `Signer` is passed, nests `signer._native_call()` inside it in that fixed order, so two concurrent `sign()` calls sharing one `Signer` cannot deadlock by acquiring the two locks in opposite orders. The Builder's `close()` after signing runs outside its own `_native_call()` block, so a teardown deferred during the call still executes once the call returns.
`Context.__init__` does not wrap the signer hand-off in `signer._native_call()`: the consuming call marks the signer `CLOSED` under its own lock before calling native, which is what stops a `signer.close()` on another thread from freeing the handle mid-transfer (see [Borrowing versus consuming](#borrowing-versus-consuming)). `Builder._sign_internal` wraps the sign call in `self._native_call()` and, when an explicit `Signer` is passed, nests `signer._native_call()` inside it in that fixed order, so two concurrent `sign()` calls sharing one `Signer` cannot deadlock by acquiring the two locks in opposite orders. The Builder's `close()` after signing runs outside its own `_native_call()` block, so a teardown deferred during the call still executes once the call returns. When the Builder was created from a `Context` and signs through its context signer, `self._context._native_call()` is nested in that same position instead, for the reason described in [Context lifetime during a context-sign](#context-lifetime-during-a-context-sign).

### Borrowing versus consuming

Deferring a teardown protects a consuming call against a racing `close()`. It does not protect a borrowing call against a racing consume, which is a different risk with a different mitigation.

A borrowing call passes the handle to native and gets it back unchanged. A consuming call hands ownership over, and the native side frees the pointer during the call. A borrowing call validates the pointer once on entry, then holds it for the duration of the operation. The pointer registry is never consulted again. So a consume starting midway through a borrow frees memory the borrowing call is still reading, and the usual `-1` rejection never happens because validation already succeeded.

`ManagedResource` therefore refuses the consume rather than allowing it to start. `_begin_consume()` runs `_ensure_not_borrowed()`, which rejects the call when `_inflight` is nonzero, and then marks the resource `CLOSED` before releasing `_op_lock`.

The check catches a borrow already in flight. The `CLOSED` mark catches one arriving afterwards: `_native_call()` calls `_ensure_valid_state()` under the same lock, so a borrow that starts later is refused instead of reaching a pointer about to be freed. The lock cannot simply be held across the native call, because those calls run caller-supplied stream callbacks that re-enter this API.

The mark is provisional. `_abort_consume()` restores the previous state when the native call turns out not to have taken the handle, which keeps the retained branch of the [ownership-taken triage](#why-an-ownership-taken-failure-does-not-free) handing back a usable object.

`_consume_and_swap()` is excluded. `_swap_handle()` requires the resource to stay `ACTIVE` and the object remains usable with its replacement pointer, so there is no `CLOSED` mark to make and no check. Its callers (`Reader.with_fragment`, `Builder.with_archive`) pass streams whose callbacks re-enter this API, so they hold their own `_native_call()`, and they act on resources the caller is required to serialize.

## Guarantees provided by ManagedResource

Expand Down
Loading
Loading