From ca293514082885f61a71d89cdaf2a8331306cc6e Mon Sep 17 00:00:00 2001 From: Tania Mathern Date: Thu, 16 Jul 2026 11:42:30 -0700 Subject: [PATCH 01/20] fix: Open up API to swap out pointers --- src/c2pa/c2pa.py | 43 ++++++++++++++++++++++++++--------------- tests/perf/scenarios.py | 36 ++++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 16 deletions(-) diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index 8b252bdc..b6e0523e 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -281,6 +281,28 @@ def _mark_consumed(self): self._handle = None self._lifecycle_state = LifecycleState.CLOSED + def _activate(self, handle, **extra_attrs): + """Attach a native handle (and any extra instance attrs) to self and + mark it ACTIVE. + + Caller must guarantee `handle` is non-null and that ownership is + being transferred here (exactly one activation per handle). + """ + for attr, value in extra_attrs.items(): + setattr(self, attr, value) + self._handle = handle + self._lifecycle_state = LifecycleState.ACTIVE + + @classmethod + def _wrap_native_handle(cls, handle, **extra_attrs): + """Build a brand-new instance around an already-valid, already-owned + native handle, bypassing __init__ entirely. + """ + obj = object.__new__(cls) + ManagedResource.__init__(obj) + obj._activate(handle, **extra_attrs) + return obj + def _cleanup_resources(self): """Release native resources idempotently.""" try: @@ -2876,13 +2898,10 @@ def __init__(self, signer_ptr: ctypes.POINTER(C2paSigner)): """ super().__init__() - self._callback_cb = None - if not signer_ptr: raise C2paError("Invalid signer pointer: pointer is null") - self._handle = signer_ptr - self._lifecycle_state = LifecycleState.ACTIVE + self._activate(signer_ptr, _callback_cb=None) def _release(self): """Release Signer-specific resources (callback reference).""" @@ -3005,25 +3024,17 @@ def from_archive( C2paError: If there was an error creating the builder from archive """ - # Handle builder._handle lifecycle somewhat manually - builder = object.__new__(cls) - ManagedResource.__init__(builder) - builder._context = None - builder._has_context_signer = False - stream_obj = Stream(stream) try: - builder._handle = ( - _lib.c2pa_builder_from_archive(stream_obj._stream) - ) + handle = _lib.c2pa_builder_from_archive(stream_obj._stream) - _check_ffi_operation_result(builder._handle, + _check_ffi_operation_result(handle, "Failed to create builder from archive" ) - builder._lifecycle_state = LifecycleState.ACTIVE - return builder + return cls._wrap_native_handle( + handle, _context=None, _has_context_signer=False) finally: stream_obj.close() diff --git a/tests/perf/scenarios.py b/tests/perf/scenarios.py index d2baae19..46865f4b 100644 --- a/tests/perf/scenarios.py +++ b/tests/perf/scenarios.py @@ -477,6 +477,28 @@ def scenario_builder_sign_jpeg_archive_roundtrip(iterations: int = 100) -> None: builder.sign("image/jpeg", io.BytesIO(source_bytes), io.BytesIO()) +def scenario_builder_from_archive_roundtrip(iterations: int = 100) -> None: + """Loop Builder.from_archive() itself (context-less alternate constructor), + then sign. Regression guard for the classmethod's native-handle wrapping. + """ + signer = _make_signer() + source_bytes = SOURCE_JPEG.read_bytes() + ingredient_bytes = SIGNED_JPEG.read_bytes() + archive_bytes = io.BytesIO() + Builder(MANIFEST_BASE).to_archive(archive_bytes) + archive_bytes = archive_bytes.getvalue() + for _ in _iterate(iterations): + # from_archive() yields a context-less Builder, so sign() needs an + # explicit signer (no Context to pull one from). + builder = Builder.from_archive(io.BytesIO(archive_bytes)) + with io.BytesIO(ingredient_bytes) as ing: + builder.add_ingredient( + {"relationship": "parentOf", "instance_id": _PARENT_ID}, + "image/jpeg", ing, + ) + builder.sign(signer, "image/jpeg", io.BytesIO(source_bytes), io.BytesIO()) + + # Archive scenarios: builder as working store (to_archive/with_archive) and # per-ingredient archives (write_ingredient_archive/add_ingredient_from_archive). @@ -678,6 +700,18 @@ def scenario_reader_string_apis(iterations: int = 100) -> None: reader.close() +def scenario_signer_construction(iterations: int = 100) -> None: + """Loop Signer.from_info()/__init__ construction and teardown. + + Every other scenario calls _make_signer() once outside its loop, so + repeated Signer construction/destruction has no coverage elsewhere. + Regression guard for Signer.__init__'s native-handle activation. + """ + for _ in _iterate(iterations): + signer = _make_signer() + signer.close() + + # jpeg + png context variants, paired with the `_legacy` scenarios above for # side-by-side comparison. @@ -916,6 +950,7 @@ def scenario_fork_stream_cleanup(iterations: int = 100) -> None: "builder_sign_jpeg_two_components_same_mime": scenario_builder_sign_jpeg_two_components_same_mime, "builder_sign_jpeg_two_components_mixed_mime": scenario_builder_sign_jpeg_two_components_mixed_mime, "builder_sign_jpeg_archive_roundtrip": scenario_builder_sign_jpeg_archive_roundtrip, + "builder_from_archive_roundtrip": scenario_builder_from_archive_roundtrip, "builder_to_archive_with_ingredient": scenario_builder_to_archive_with_ingredient, "builder_sign_jpeg_archive_roundtrip_ingredient_in_archive": scenario_builder_sign_jpeg_archive_roundtrip_ingredient_in_archive, "builder_write_ingredient_archive": scenario_builder_write_ingredient_archive, @@ -925,6 +960,7 @@ def scenario_fork_stream_cleanup(iterations: int = 100) -> None: "reader_error_no_manifest": scenario_reader_error_no_manifest, "builder_error_invalid_manifest": scenario_builder_error_invalid_manifest, "reader_string_apis": scenario_reader_string_apis, + "signer_construction": scenario_signer_construction, "fork_reader_collect": scenario_fork_reader_collect, "fork_contended_mutex": scenario_fork_contended_mutex, "fork_thread_local_orphan": scenario_fork_thread_local_orphan, From af97d7f76fddc9957685a6c9549f9812e62bfdb7 Mon Sep 17 00:00:00 2001 From: tmathern <60901087+tmathern@users.noreply.github.com> Date: Thu, 16 Jul 2026 16:45:36 -0700 Subject: [PATCH 02/20] fix: Hardening native handles --- src/c2pa/c2pa.py | 161 +++++++++++++++++------ tests/perf/baseline.json | 20 +++ tests/perf/scenarios.py | 47 +++++++ tests/test_unit_tests.py | 272 ++++++++++++++++++++++++++++++++++++++- 4 files changed, 461 insertions(+), 39 deletions(-) diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index 0bceabf3..83af9431 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -230,8 +230,13 @@ class ManagedResource: for native resources (e.g. pointers). Subclasses must: - - Set `self._handle` to the native pointer after creation. - - Set `self._lifecycle_state = LifecycleState.ACTIVE` once initialized. + - 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. + - Call `_mark_consumed()` when an FFI call took ownership of the handle + without returning a replacement. - Override `_release()` to free class-specific resources (streams, caches, callbacks, etc.), called before the native pointer is freed. @@ -287,18 +292,84 @@ def _activate(self, handle, **extra_attrs): """Attach a native handle (and any extra instance attrs) to self and mark it ACTIVE. - Caller must guarantee `handle` is non-null and that ownership is - being transferred here (exactly one activation per handle). + Ownership of `handle` transfers here: this object frees it on close. + Only an UNINITIALIZED resource can be activated, so a handle can never + be activated twice (which would leak the one being replaced) and a + CLOSED resource can never be resurrected (which would free its handle + a second time). + + Any attribute the subclass's `_release()` reads must be passed in + `extra_attrs` when __init__ did not already set it, otherwise + `_release()` raises during cleanup. + + Args: + handle: Non-null native pointer to take ownership of + **extra_attrs: Instance attributes to set before activating + + Raises: + C2paError: If the handle is null or the resource is not + UNINITIALIZED """ + name = type(self).__name__ + # Guards run before any mutation: a rejected activation must leave + # the object exactly as it was. + if not handle: + raise C2paError(f"{name}: cannot activate a null handle") + if self._lifecycle_state != LifecycleState.UNINITIALIZED: + raise C2paError( + f"{name}: already activated " + f"({self._lifecycle_state.name})") + for attr, value in extra_attrs.items(): setattr(self, attr, value) self._handle = handle self._lifecycle_state = LifecycleState.ACTIVE + def _swap_handle(self, new_handle): + """Replace the handle after an FFI call consumed the old one and + returned a replacement (the consume-and-return pattern, e.g. + c2pa_builder_with_archive / c2pa_reader_with_fragment). + + The old pointer is already owned and freed by the callee, so it is + deliberately NOT freed here; doing so would be a double-free. + + 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. Treat + that case as consumed via `_mark_consumed()` instead: risking a leak + on a path Python cannot reach beats freeing a pointer whose address + may have been recycled. + + Args: + new_handle: Non-null native pointer returned by the FFI call + + 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") + + self._handle = new_handle + @classmethod def _wrap_native_handle(cls, handle, **extra_attrs): """Build a brand-new instance around an already-valid, already-owned native handle, bypassing __init__ entirely. + + Because __init__ is bypassed, every attribute the subclass's + `_release()` reads must be passed in `extra_attrs`. + + Args: + handle: Non-null native pointer to take ownership of + **extra_attrs: Instance attributes to set before activating + + Raises: + C2paError: If the handle is null """ obj = object.__new__(cls) ManagedResource.__init__(obj) @@ -315,7 +386,17 @@ def _cleanup_resources(self): and self._lifecycle_state != LifecycleState.CLOSED ): self._lifecycle_state = LifecycleState.CLOSED - self._release() + # A failing _release() must not skip the free below: + # that would strand the native handle on an object already + # marked CLOSED, making it unreachable and unfreeable. + try: + self._release() + except Exception: + logger.error( + "Failed to release %s resources", + type(self).__name__, + exc_info=True, + ) if hasattr(self, '_handle') and self._handle: try: ManagedResource._free_native_ptr(self._handle) @@ -1297,8 +1378,7 @@ def __init__(self): ManagedResource._free_native_ptr(ptr) raise - self._handle = ptr - self._lifecycle_state = LifecycleState.ACTIVE + self._activate(ptr) @classmethod def from_json(cls, json_str: str) -> 'Settings': @@ -1464,7 +1544,7 @@ def __init__( _check_ffi_operation_result( ptr, "Failed to create Context" ) - self._handle = ptr + self._activate(ptr) else: # Use ContextBuilder for settings/signer builder_ptr = _lib.c2pa_context_builder_new() @@ -1484,33 +1564,33 @@ def __init__( if signer is not None: signer._ensure_valid_state() + # c2pa_context_builder_set_signer takes ownership of the + # signer pointer immediately (Box::from_raw), on its error + # path as well as on success. The Signer is therefore + # consumed below on any result; leaving it owning a freed + # pointer would make any later use of it a use-after-free. + self._signer_callback_cb = signer._callback_cb result = ( _lib.c2pa_context_builder_set_signer( builder_ptr, signer._handle, ) ) + signer._mark_consumed() if result != 0: _parse_operation_result_for_error(None) + self._has_signer = True # Build consumes builder_ptr ptr = ( _lib.c2pa_context_builder_build(builder_ptr) ) builder_ptr = None - self._handle = ptr _check_ffi_operation_result( ptr, "Failed to build Context" ) - # Build succeeded, consume the Signer. - # Keep its callback ref alive on this Context, - # then mark it so it won't double-free the - # pointer the Context now owns. - if signer is not None: - self._signer_callback_cb = signer._callback_cb - signer._mark_consumed() - self._has_signer = True + self._activate(ptr) except Exception: # Free builder if build was not reached if builder_ptr is not None: @@ -1520,8 +1600,6 @@ def __init__( pass raise - self._lifecycle_state = LifecycleState.ACTIVE - def _release(self): """Release Context-specific resources.""" self._signer_callback_cb = None @@ -2224,11 +2302,11 @@ def __init__( with Stream(stream) as stream_obj: self._create_reader( format_bytes, stream_obj, manifest_data) - self._lifecycle_state = LifecycleState.ACTIVE def _create_reader(self, format_bytes, stream_obj, manifest_data=None): - """Create a Reader from a Stream. + """Create a native reader from a Stream and activate this Reader + around it. Args: format_bytes: UTF-8 encoded format/MIME type @@ -2236,7 +2314,7 @@ def _create_reader(self, format_bytes, stream_obj, manifest_data: Optional manifest bytes """ if manifest_data is None: - self._handle = _lib.c2pa_reader_from_stream( + ptr = _lib.c2pa_reader_from_stream( format_bytes, stream_obj._stream) else: if not isinstance(manifest_data, bytes): @@ -2244,7 +2322,7 @@ def _create_reader(self, format_bytes, stream_obj, manifest_array = ( ctypes.c_ubyte * len(manifest_data)).from_buffer_copy(manifest_data) - self._handle = ( + ptr = ( _lib.c2pa_reader_from_manifest_data_and_stream( format_bytes, stream_obj._stream, @@ -2254,9 +2332,11 @@ def _create_reader(self, format_bytes, stream_obj, ) _check_ffi_operation_result( - self._handle, + ptr, Reader._ERROR_MESSAGES['reader_error'].format("Unknown error")) + self._activate(ptr) + def _init_from_file(self, path, format_bytes, manifest_data=None): """Open a file and create a reader from it. @@ -2270,7 +2350,6 @@ def _init_from_file(self, path, format_bytes, self._backing_file = open(path, 'rb') self._own_stream = Stream(self._backing_file) self._create_reader(format_bytes, self._own_stream, manifest_data) - self._lifecycle_state = LifecycleState.ACTIVE except C2paError: self._close_streams() raise @@ -2352,18 +2431,17 @@ def _init_from_context(self, context, format_or_path, self._own_stream._stream, ) - # reader_ptr has been consumed by the FFI call. + # reader_ptr has been consumed by the FFI call (freed by it even + # on failure), so there is nothing to free on the error path. reader_ptr = None - self._handle = new_ptr - _check_ffi_operation_result(new_ptr, Reader._ERROR_MESSAGES[ 'reader_error' ].format("Unknown error") ) - self._lifecycle_state = LifecycleState.ACTIVE + self._activate(new_ptr) except Exception: self._close_streams() raise @@ -2449,13 +2527,15 @@ def with_fragment(self, format: str, stream, frag_obj._stream, ) + # c2pa_reader_with_fragment consumed the old handle. A null return + # leaves no replacement to take ownership of. if not new_ptr: self._mark_consumed() _check_ffi_operation_result(new_ptr, Reader._ERROR_MESSAGES[ 'fragment_error' ].format("Unknown error")) - self._handle = new_ptr + self._swap_handle(new_ptr) # Invalidate caches: processing a new BMFF fragment updates the native # reader's state, which can change the manifest data it returns. @@ -3083,13 +3163,13 @@ def __init__( if context is not None: self._init_from_context(context, json_str) else: - self._handle = _lib.c2pa_builder_from_json(json_str) + ptr = _lib.c2pa_builder_from_json(json_str) _check_ffi_operation_result( - self._handle, + ptr, Builder._ERROR_MESSAGES['builder_error'].format("Unknown error")) - self._lifecycle_state = LifecycleState.ACTIVE + self._activate(ptr) def _init_from_context(self, context, json_str): """Initialize Builder from a ContextProvider. @@ -3114,10 +3194,10 @@ def _init_from_context(self, context, json_str): ManagedResource._free_native_ptr(builder_ptr) raise - # Consume-and-return: builder_ptr is consumed, - # new_ptr is the valid pointer going forward + # Consume-and-return: builder_ptr is consumed (freed by the FFI even + # on failure), new_ptr is the valid pointer going forward. Nothing to + # free here on the error path. new_ptr = _lib.c2pa_builder_with_definition(builder_ptr, json_str) - self._handle = new_ptr _check_ffi_operation_result(new_ptr, Builder._ERROR_MESSAGES[ @@ -3125,6 +3205,8 @@ def _init_from_context(self, context, json_str): ].format("Unknown error") ) + self._activate(new_ptr) + def set_no_embed(self): """Set the no-embed flag. @@ -3404,10 +3486,13 @@ def with_archive(self, stream: Any) -> 'Builder': raise C2paError( f"Error loading archive: {e}" ) - # Old handle consumed by FFI - self._handle = new_ptr + # c2pa_builder_with_archive consumed the old handle. A null return + # leaves no replacement to take ownership of. + if not new_ptr: + self._mark_consumed() _check_ffi_operation_result( new_ptr, "Failed to load archive into builder") + self._swap_handle(new_ptr) return self diff --git a/tests/perf/baseline.json b/tests/perf/baseline.json index 7af9ffb5..d4827c25 100644 --- a/tests/perf/baseline.json +++ b/tests/perf/baseline.json @@ -221,5 +221,25 @@ "peak_bytes": 3402893, "leaked_bytes": 3230663, "total_allocations": 93141 + }, + "builder_from_archive_roundtrip": { + "peak_bytes": 14258579, + "leaked_bytes": 3436798, + "total_allocations": 1593617 + }, + "signer_construction": { + "peak_bytes": 3307608, + "leaked_bytes": 3243306, + "total_allocations": 117601 + }, + "builder_with_archive_swap": { + "peak_bytes": 3635924, + "leaked_bytes": 3305070, + "total_allocations": 395447 + }, + "reader_with_fragment_swap": { + "peak_bytes": 3733349, + "leaked_bytes": 3306787, + "total_allocations": 1936244 } } \ No newline at end of file diff --git a/tests/perf/scenarios.py b/tests/perf/scenarios.py index 096efa3e..b913aba7 100644 --- a/tests/perf/scenarios.py +++ b/tests/perf/scenarios.py @@ -36,6 +36,8 @@ CLOUD_JPEG = FIXTURES_DIR / "cloud.jpg" SOURCE_JPEG = FIXTURES_DIR / "A.jpg" SIGNING_PNG = SIGNING_FIXTURES_DIR / "sample1.png" +DASH_INIT_MP4 = FIXTURES_DIR / "dashinit.mp4" +DASH_FRAGMENT = FIXTURES_DIR / "dash1.m4s" _DST_COMPOSITE = "http://cv.iptc.org/newscodes/digitalsourcetype/compositeWithTrainedAlgorithmicMedia" @@ -477,6 +479,49 @@ def scenario_builder_sign_jpeg_archive_roundtrip(iterations: int = 100) -> None: builder.sign("image/jpeg", io.BytesIO(source_bytes), io.BytesIO()) +def scenario_builder_with_archive_swap(iterations: int = 100) -> None: + """Loop Builder.with_archive(), the consume-and-return FFI path. + + c2pa_builder_with_archive consumes the old native handle and returns a + replacement, so the Python side swaps the pointer without freeing the + consumed one. Freeing it would be a double-free, and failing to adopt the + replacement would leak. The other builder scenarios never swap a live + handle, so neither mistake would show up there. + """ + context = Context(signer=_make_signer()) + archive = io.BytesIO() + Builder(MANIFEST_BASE).to_archive(archive) + archive_bytes = archive.getvalue() + for _ in _iterate(iterations): + builder = Builder(MANIFEST_BASE, context=context) + builder.with_archive(io.BytesIO(archive_bytes)) + builder.close() + + +def scenario_reader_with_fragment_swap(iterations: int = 100) -> None: + """Loop Reader.with_fragment(), the other consume-and-return FFI path. + + Same ownership hand-off as with_archive: c2pa_reader_with_fragment eats + the old reader handle and returns a new one. + """ + init_bytes = DASH_INIT_MP4.read_bytes() + fragment_bytes = DASH_FRAGMENT.read_bytes() + for _ in _iterate(iterations): + reader = Reader("video/mp4", io.BytesIO(init_bytes)) + try: + reader.with_fragment( + "video/mp4", + io.BytesIO(init_bytes), + io.BytesIO(fragment_bytes), + ) + except C2paError: + # A failed call consumed the old handle just as a successful one + # would, so the scenario measures both outcomes. + pass + finally: + reader.close() + + def scenario_builder_from_archive_roundtrip(iterations: int = 100) -> None: """Loop Builder.from_archive() itself (context-less alternate constructor), then sign. Regression guard for the classmethod's native-handle wrapping. @@ -978,6 +1023,8 @@ def scenario_fork_stream_cleanup(iterations: int = 100) -> None: "builder_sign_jpeg_two_components_mixed_mime": scenario_builder_sign_jpeg_two_components_mixed_mime, "builder_sign_jpeg_archive_roundtrip": scenario_builder_sign_jpeg_archive_roundtrip, "builder_from_archive_roundtrip": scenario_builder_from_archive_roundtrip, + "builder_with_archive_swap": scenario_builder_with_archive_swap, + "reader_with_fragment_swap": scenario_reader_with_fragment_swap, "builder_to_archive_with_ingredient": scenario_builder_to_archive_with_ingredient, "builder_sign_jpeg_archive_roundtrip_ingredient_in_archive": scenario_builder_sign_jpeg_archive_roundtrip_ingredient_in_archive, "builder_write_ingredient_archive": scenario_builder_write_ingredient_archive, diff --git a/tests/test_unit_tests.py b/tests/test_unit_tests.py index e0b3289c..67bdc2cc 100644 --- a/tests/test_unit_tests.py +++ b/tests/test_unit_tests.py @@ -30,7 +30,8 @@ from c2pa import Builder, C2paError as Error, Reader, C2paSigningAlg as SigningAlg, C2paSignerInfo, Signer, sdk_version, C2paBuilderIntent, C2paDigitalSourceType from c2pa import Settings, Context, ContextBuilder, ContextProvider -from c2pa.c2pa import Stream, LifecycleState, load_settings, create_signer, create_signer_from_info, ed25519_sign, format_embeddable +from c2pa.c2pa import Stream, LifecycleState, ManagedResource, load_settings, create_signer, create_signer_from_info, ed25519_sign, format_embeddable +import c2pa.c2pa as c2pa_module PROJECT_PATH = os.getcwd() @@ -6924,5 +6925,274 @@ def test_callbacks_return_minus_one_after_stream_collected(self): self.assertEqual(flush_cb(None), -1) +class _FakeHandleResource(ManagedResource): + """ManagedResource with a fake integer handle, for lifecycle tests that + must not touch the native library.""" + + +class _CallbackHoldingResource(ManagedResource): + """Mimics Signer: its _release() reads an attribute that must have been + supplied by __init__ or by _activate's extra_attrs.""" + + def _release(self): + if self._callback_cb: + self._callback_cb = None + + +class TestManagedResourceLifecycle(unittest.TestCase): + """Lifecycle primitives: _activate, _swap_handle, _wrap_native_handle. + + These use fake integer handles and record calls to _free_native_ptr + instead of freeing anything, so a mistake here cannot crash the process. + """ + + def setUp(self): + self.freed = [] + self._real_free = ManagedResource._free_native_ptr + ManagedResource._free_native_ptr = staticmethod(self.freed.append) + + def tearDown(self): + ManagedResource._free_native_ptr = self._real_free + + # _cleanup_resources: a failing _release() must not strand the handle + + def test_release_failure_still_frees_handle(self): + res = _CallbackHoldingResource() + # _callback_cb is never set, so _release() raises AttributeError. + res._activate(0xBBBB) + + res.close() + + self.assertEqual(self.freed, [0xBBBB], + "handle leaked when _release() raised") + self.assertIsNone(res._handle) + + def test_release_failure_is_logged(self): + res = _CallbackHoldingResource() + res._activate(0xBBBB) + + with self.assertLogs('c2pa', level='ERROR') as captured: + res.close() + + self.assertTrue( + any('Failed to release' in line for line in captured.output), + f"_release() failure was not logged: {captured.output}") + + # _activate guards + + def test_activate_rejects_null_handle(self): + res = _FakeHandleResource() + + with self.assertRaises(Error) as ctx: + res._activate(None) + + self.assertIn("null handle", str(ctx.exception)) + self.assertEqual(res._lifecycle_state, LifecycleState.UNINITIALIZED) + + def test_activate_rejects_double_activation(self): + res = _FakeHandleResource() + res._activate(0x1111) + + with self.assertRaises(Error) as ctx: + res._activate(0x2222) + + self.assertIn("already activated", str(ctx.exception)) + # The first handle is still owned, and is freed exactly once. + self.assertEqual(res._handle, 0x1111) + res.close() + self.assertEqual(self.freed, [0x1111]) + + def test_activate_rejects_reactivation_after_close(self): + res = _FakeHandleResource() + res._activate(0x3333) + res.close() + self.freed.clear() + + with self.assertRaises(Error): + res._activate(0x4444) + + # Staying CLOSED is what prevents a second free of the handle. + self.assertEqual(res._lifecycle_state, LifecycleState.CLOSED) + self.assertIsNone(res._handle) + self.assertEqual(self.freed, []) + + def test_activate_does_not_mutate_on_rejection(self): + res = _FakeHandleResource() + res._activate(0x5555) + + with self.assertRaises(Error): + res._activate(0x6666, _extra='should not be set') + + self.assertFalse(hasattr(res, '_extra'), + "rejected activation still set extra_attrs") + + # _swap_handle + + def test_swap_handle_does_not_free_consumed_handle(self): + res = _FakeHandleResource() + res._activate(0xAAA1) + + res._swap_handle(0xAAA2) + + # The FFI already owns and frees the old pointer, so freeing it here + # would be a double-free. + self.assertEqual(self.freed, []) + self.assertEqual(res._handle, 0xAAA2) + + res.close() + self.assertEqual(self.freed, [0xAAA2]) + + def test_swap_handle_requires_active_resource(self): + uninitialized = _FakeHandleResource() + with self.assertRaises(Error) as ctx: + uninitialized._swap_handle(0x1) + self.assertIn("not active", str(ctx.exception)) + + closed = _FakeHandleResource() + closed._activate(0x2) + closed.close() + with self.assertRaises(Error): + closed._swap_handle(0x3) + + def test_swap_handle_rejects_null_replacement(self): + res = _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) + + # _wrap_native_handle + + def test_wrap_native_handle_sets_extra_attrs_and_bypasses_init(self): + seen = [] + + class Probe(ManagedResource): + def __init__(self): + raise AssertionError("__init__ must be bypassed") + + def _release(self): + seen.append(self._tag) + + obj = Probe._wrap_native_handle(0xC0DE, _tag='from extra_attrs') + + self.assertEqual(obj._tag, 'from extra_attrs') + self.assertEqual(obj._lifecycle_state, LifecycleState.ACTIVE) + self.assertTrue(obj.is_valid) + # ManagedResource.__init__ still ran, so fork-safety is intact. + self.assertTrue(hasattr(obj, '_owner_pid')) + + obj.close() + self.assertEqual(seen, ['from extra_attrs'], + "_release() could not see the extra attrs") + + def test_wrap_native_handle_rejects_null(self): + with self.assertRaises(Error): + _FakeHandleResource._wrap_native_handle(None) + + def test_close_after_wrap_is_idempotent(self): + obj = _FakeHandleResource._wrap_native_handle(0xD00D) + + obj.close() + obj.close() + + self.assertEqual(self.freed, [0xD00D], "handle freed more than once") + + +class TestNativeHandleOwnership(unittest.TestCase): + """Ownership hand-offs between Python and the native library, driven by + fault injection at the FFI boundary.""" + + def setUp(self): + self.data_dir = os.path.join(os.path.dirname(__file__), "fixtures") + + def _make_signer(self): + with open(os.path.join(self.data_dir, "es256_certs.pem"), "rb") as f: + certs = f.read() + with open(os.path.join(self.data_dir, "es256_private.key"), "rb") as f: + key = f.read() + return Signer.from_info(C2paSignerInfo( + b"es256", certs, key, b"http://timestamp.digicert.com")) + + def test_signer_init_rejects_null_pointer(self): + with self.assertRaises(Error): + Signer(None) + + def test_builder_from_archive_wraps_handle(self): + archive = io.BytesIO() + Builder({"claim_generator": "test", "format": "image/jpeg"}).to_archive( + archive) + + builder = Builder.from_archive(io.BytesIO(archive.getvalue())) + + self.assertTrue(builder.is_valid) + self.assertIsNone(builder._context) + self.assertFalse(builder._has_context_signer) + builder.close() + self.assertEqual(builder._lifecycle_state, LifecycleState.CLOSED) + + def test_context_build_failure_consumes_signer(self): + # c2pa_context_builder_set_signer takes ownership of the signer + # pointer immediately, so a later build failure must still leave the + # Signer consumed. Otherwise it holds a pointer the native side has + # already freed. + signer = self._make_signer() + real_build = c2pa_module._lib.c2pa_context_builder_build + c2pa_module._lib.c2pa_context_builder_build = lambda ptr: None + try: + with self.assertRaises(Error): + Context(signer=signer) + finally: + c2pa_module._lib.c2pa_context_builder_build = real_build + + self.assertIsNone(signer._handle, + "Signer still holds a pointer the native side freed") + self.assertEqual(signer._lifecycle_state, LifecycleState.CLOSED) + + # 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: + signer.close() + finally: + ManagedResource._free_native_ptr = real_free + self.assertEqual(freed, []) + + def test_context_with_signer_consumes_it_on_success(self): + signer = self._make_signer() + + context = Context(signer=signer) + + self.assertTrue(context.is_valid) + self.assertIsNone(signer._handle) + self.assertEqual(signer._lifecycle_state, LifecycleState.CLOSED) + self.assertTrue(context.has_signer) + context.close() + + def test_construction_failure_leaves_nothing_to_free(self): + # A failed FFI construction must not leave a half-constructed object + # holding a handle. Activation happens after the check, so _handle is + # never set. + real_new = c2pa_module._lib.c2pa_context_new + c2pa_module._lib.c2pa_context_new = lambda: None + try: + with self.assertRaises(Error): + Context() + finally: + c2pa_module._lib.c2pa_context_new = real_new + + real_json = c2pa_module._lib.c2pa_builder_from_json + c2pa_module._lib.c2pa_builder_from_json = lambda j: None + try: + with self.assertRaises(Error): + Builder({"claim_generator": "test"}) + finally: + c2pa_module._lib.c2pa_builder_from_json = real_json + + if __name__ == '__main__': unittest.main(warnings='ignore') From 3c045878b12616acc063e1237157bb69265f7da4 Mon Sep 17 00:00:00 2001 From: tmathern <60901087+tmathern@users.noreply.github.com> Date: Thu, 16 Jul 2026 17:11:53 -0700 Subject: [PATCH 03/20] fix: COmments --- src/c2pa/c2pa.py | 33 +++++++++++++-------------------- 1 file changed, 13 insertions(+), 20 deletions(-) diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index 83af9431..f68e3a48 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -289,14 +289,12 @@ def _mark_consumed(self): self._lifecycle_state = LifecycleState.CLOSED def _activate(self, handle, **extra_attrs): - """Attach a native handle (and any extra instance attrs) to self and - mark it ACTIVE. + """Attach a native handle (and any extra instance attrs) to self + and mark it active. Attaching activates it. Ownership of `handle` transfers here: this object frees it on close. - Only an UNINITIALIZED resource can be activated, so a handle can never - be activated twice (which would leak the one being replaced) and a - CLOSED resource can never be resurrected (which would free its handle - a second time). + Only an uninitialized resource can be activated, so a handle can never + be activated twice and a closed resource can never be reopened. Any attribute the subclass's `_release()` reads must be passed in `extra_attrs` when __init__ did not already set it, otherwise @@ -307,12 +305,11 @@ def _activate(self, handle, **extra_attrs): **extra_attrs: Instance attributes to set before activating Raises: - C2paError: If the handle is null or the resource is not - UNINITIALIZED + C2paError: If the handle is null + or the resource is not uninitialized """ name = type(self).__name__ - # Guards run before any mutation: a rejected activation must leave - # the object exactly as it was. + # A rejected activation must leave the object as it was. if not handle: raise C2paError(f"{name}: cannot activate a null handle") if self._lifecycle_state != LifecycleState.UNINITIALIZED: @@ -327,18 +324,14 @@ def _activate(self, handle, **extra_attrs): def _swap_handle(self, new_handle): """Replace the handle after an FFI call consumed the old one and - returned a replacement (the consume-and-return pattern, e.g. - c2pa_builder_with_archive / c2pa_reader_with_fragment). + returned a replacement. - The old pointer is already owned and freed by the callee, so it is - deliberately NOT freed here; doing so would be a double-free. + The old pointer is already owned and freed by the callee, + so it is not freed here. 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. Treat - that case as consumed via `_mark_consumed()` instead: risking a leak - on a path Python cannot reach beats freeing a pointer whose address - may have been recycled. + after), so callers must not call this with a null replacement. Args: new_handle: Non-null native pointer returned by the FFI call @@ -358,8 +351,8 @@ def _swap_handle(self, new_handle): @classmethod def _wrap_native_handle(cls, handle, **extra_attrs): - """Build a brand-new instance around an already-valid, already-owned - native handle, bypassing __init__ entirely. + """Build a brand-new instance around an already-valid, + already-owned native handle, bypassing __init__ entirely. Because __init__ is bypassed, every attribute the subclass's `_release()` reads must be passed in `extra_attrs`. From 9d7b2aca05781aed977dc263266d2c01f11a535f Mon Sep 17 00:00:00 2001 From: tmathern <60901087+tmathern@users.noreply.github.com> Date: Thu, 16 Jul 2026 18:32:34 -0700 Subject: [PATCH 04/20] fix: Update PID stamping --- src/c2pa/c2pa.py | 31 ++- tests/perf/baseline.json | 25 ++ tests/perf/scenarios.py | 156 ++++++++++++ tests/test_unit_tests.py | 403 +++++++++++++++++++++++++++++- tests/test_unit_tests_threaded.py | 156 ++++++++++++ 5 files changed, 765 insertions(+), 6 deletions(-) diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index f68e3a48..9ef2f4d3 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -224,6 +224,12 @@ class LifecycleState(enum.IntEnum): CLOSED = 2 +# Attributes that make up the lifecycle state itself, which _activate sets +# from its own arguments and must never accept from a caller's extra_attrs. +_RESERVED_ACTIVATION_ATTRS = frozenset( + {'_handle', '_lifecycle_state', '_owner_pid'}) + + class ManagedResource: """Base class for objects that hold a native (C FFI) resource. This is an internal base class that provides lifecycle management @@ -300,13 +306,19 @@ def _activate(self, handle, **extra_attrs): `extra_attrs` when __init__ did not already set it, otherwise `_release()` raises during cleanup. + `extra_attrs` cannot carry the lifecycle attributes themselves. + `_owner_pid` in particular records the process that created the + handle, and overwriting it would let a forked child free a pointer + its parent still owns. + Args: handle: Non-null native pointer to take ownership of **extra_attrs: Instance attributes to set before activating Raises: - C2paError: If the handle is null - or the resource is not uninitialized + C2paError: If the handle is null, + the resource is not uninitialized, + or extra_attrs names a lifecycle attribute """ name = type(self).__name__ # A rejected activation must leave the object as it was. @@ -316,6 +328,11 @@ def _activate(self, handle, **extra_attrs): raise C2paError( f"{name}: already activated " f"({self._lifecycle_state.name})") + reserved = _RESERVED_ACTIVATION_ATTRS.intersection(extra_attrs) + if reserved: + raise C2paError( + f"{name}: cannot set lifecycle attributes via extra_attrs: " + f"{', '.join(sorted(reserved))}") for attr, value in extra_attrs.items(): setattr(self, attr, value) @@ -356,15 +373,19 @@ def _wrap_native_handle(cls, handle, **extra_attrs): Because __init__ is bypassed, every attribute the subclass's `_release()` reads must be passed in `extra_attrs`. + The lifecycle attributes are still off limits there, + and the instance is stamped with the creating process either way. Args: handle: Non-null native pointer to take ownership of **extra_attrs: Instance attributes to set before activating Raises: - C2paError: If the handle is null + C2paError: If the handle is null, or extra_attrs names a + lifecycle attribute """ obj = object.__new__(cls) + # Stamps _owner_pid, which the fork guard relies on. ManagedResource.__init__(obj) obj._activate(handle, **extra_attrs) return obj @@ -2298,8 +2319,8 @@ def __init__( def _create_reader(self, format_bytes, stream_obj, manifest_data=None): - """Create a native reader from a Stream and activate this Reader - around it. + """Create a native reader from a Stream + and activate this Reader around it. Args: format_bytes: UTF-8 encoded format/MIME type diff --git a/tests/perf/baseline.json b/tests/perf/baseline.json index d4827c25..934a6390 100644 --- a/tests/perf/baseline.json +++ b/tests/perf/baseline.json @@ -241,5 +241,30 @@ "peak_bytes": 3733349, "leaked_bytes": 3306787, "total_allocations": 1936244 + }, + "fork_swap_cleanup": { + "peak_bytes": 3639043, + "leaked_bytes": 3308397, + "total_allocations": 401032 + }, + "swap_chain_churn": { + "peak_bytes": 3650097, + "leaked_bytes": 3319300, + "total_allocations": 379064 + }, + "fork_consumed_signer": { + "peak_bytes": 3321464, + "leaked_bytes": 3258207, + "total_allocations": 130030 + }, + "fork_contended_mutex_swap": { + "peak_bytes": 7281970, + "leaked_bytes": 3443799, + "total_allocations": 18799685 + }, + "fork_contended_mutex_wrap": { + "peak_bytes": 7268578, + "leaked_bytes": 3442991, + "total_allocations": 17791158 } } \ No newline at end of file diff --git a/tests/perf/scenarios.py b/tests/perf/scenarios.py index b913aba7..b4b7e46c 100644 --- a/tests/perf/scenarios.py +++ b/tests/perf/scenarios.py @@ -979,6 +979,157 @@ def _child(): context.close() +def _fork_contended_over(make_object, iterations): + """Fork over an object built by make_object() while 8 threads churn + Readers, so the registry Mutex is likely held at the instant of fork(). + + The child closes the inherited object. Without the PID guard that close + calls into the native library and can block forever on a mutex left + locked by a thread that fork() did not clone, which _fork_wait's alarm + reports as a timeout. The parent closes afterwards for the real free. + """ + if not hasattr(os, "fork"): + return + stop = threading.Event() + + def _worker(): + while not stop.is_set(): + with open(SIGNED_JPEG, "rb") as f: + r = Reader("image/jpeg", f) + r.close() + + threads = [threading.Thread(target=_worker, daemon=True) + for _ in range(8)] + for t in threads: + t.start() + try: + for _ in _iterate(iterations): + for _ in range(5): + obj = make_object() + + def _child(o=obj): + o.close() + gc.collect() + + _fork_wait(_child) + obj.close() + finally: + stop.set() + for t in threads: + t.join(timeout=5) + + +def scenario_fork_contended_mutex_swap(iterations: int = 100) -> None: + """Fork safety benchmark scenario: + fork over a Builder whose handle came from with_archive(), under the same + thread contention as fork_contended_mutex. That scenario only ever forks + over handles that came straight from a constructor, so a swapped-in + handle losing its stamp would go unnoticed there. + """ + if not hasattr(os, "fork"): + return + context = Context(signer=_make_signer()) + archive = io.BytesIO() + Builder(MANIFEST_BASE).to_archive(archive) + archive_bytes = archive.getvalue() + + def _make(): + builder = Builder(MANIFEST_BASE, context=context) + builder.with_archive(io.BytesIO(archive_bytes)) + return builder + + _fork_contended_over(_make, iterations) + + +def scenario_fork_contended_mutex_wrap(iterations: int = 100) -> None: + """Fork safety benchmark scenario: + fork over a Builder built by from_archive(), under thread contention. + from_archive is the only path that bypasses __init__, so it is the one + most likely to be missing the PID stamp the child's close() depends on. + """ + if not hasattr(os, "fork"): + return + archive = io.BytesIO() + Builder(MANIFEST_BASE).to_archive(archive) + archive_bytes = archive.getvalue() + + _fork_contended_over( + lambda: Builder.from_archive(io.BytesIO(archive_bytes)), iterations) + + +def scenario_fork_consumed_signer(iterations: int = 100) -> None: + """Fork safety benchmark scenario: + the parent builds a Context that consumed a Signer, then forks. The child + closes both. The consumed Signer holds no handle, so it must be inert in + either process, and the Context must be skipped by the PID guard. + """ + if not hasattr(os, "fork"): + return + for _ in _iterate(iterations): + signer = _make_signer() + context = Context(signer=signer) + + def _child(c=context, s=signer): + s.close() + c.close() + gc.collect() + + _fork_wait(_child) + signer.close() + context.close() + + +def scenario_swap_chain_churn(iterations: int = 100) -> None: + """Loop with_archive() repeatedly on one Builder, so a chain of handles + is consumed and replaced on a single live object. Every other scenario + swaps a given object at most once. + + This one is a crash and allocation-churn guard rather than a leak gate. + Only one Builder is closed however many times the loop runs, so a + close-path leak here is O(1) and invisible against the interpreter's + allocation floor. What a broken swap does instead is fail loudly: keeping + the consumed pointer makes the next call raise UntrackedPointer from the + native registry, and freeing it makes the free itself fail. total_allocations + still tracks the churn. + """ + context = Context(signer=_make_signer()) + archive = io.BytesIO() + Builder(MANIFEST_BASE).to_archive(archive) + archive_bytes = archive.getvalue() + builder = Builder(MANIFEST_BASE, context=context) + for _ in _iterate(iterations): + builder.with_archive(io.BytesIO(archive_bytes)) + builder.close() + context.close() + + +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 + consumed the original and returned a replacement. The child must skip the + free on the swapped-in handle just as it would on an original one, and the + parent must still free it exactly once afterwards. The other fork + scenarios only ever fork over handles that came straight from a + constructor. + """ + if not hasattr(os, "fork"): + return + context = Context(signer=_make_signer()) + archive = io.BytesIO() + Builder(MANIFEST_BASE).to_archive(archive) + archive_bytes = archive.getvalue() + for _ in _iterate(iterations): + builder = Builder(MANIFEST_BASE, context=context) + builder.with_archive(io.BytesIO(archive_bytes)) + + def _child(b=builder): + b.close() + gc.collect() + + _fork_wait(_child) + builder.close() + + def scenario_fork_stream_cleanup(iterations: int = 100) -> None: """Fork safety benchmark scenario: Stream wraps a BytesIO with ctypes callbacks stored as instance attributes. @@ -1042,6 +1193,11 @@ def scenario_fork_stream_cleanup(iterations: int = 100) -> None: "fork_parent_frees_after_fork": scenario_fork_parent_frees_after_fork, "fork_child_sys_exit": scenario_fork_child_sys_exit, "fork_stream_cleanup": scenario_fork_stream_cleanup, + "fork_swap_cleanup": scenario_fork_swap_cleanup, + "fork_contended_mutex_swap": scenario_fork_contended_mutex_swap, + "fork_contended_mutex_wrap": scenario_fork_contended_mutex_wrap, + "fork_consumed_signer": scenario_fork_consumed_signer, + "swap_chain_churn": scenario_swap_chain_churn, } diff --git a/tests/test_unit_tests.py b/tests/test_unit_tests.py index 67bdc2cc..78154cca 100644 --- a/tests/test_unit_tests.py +++ b/tests/test_unit_tests.py @@ -6939,8 +6939,20 @@ def _release(self): self._callback_cb = None +class _ReleaseRecordingResource(ManagedResource): + """Records _release() calls for test asserts.""" + + def __init__(self): + super().__init__() + self.release_calls = 0 + + def _release(self): + self.release_calls += 1 + + class TestManagedResourceLifecycle(unittest.TestCase): - """Lifecycle primitives: _activate, _swap_handle, _wrap_native_handle. + """Lifecycle primitives (_activate, _swap_handle, _wrap_native_handle) + and the _owner_pid stamp that governs which process may free a handle. These use fake integer handles and record calls to _free_native_ptr instead of freeing anything, so a mistake here cannot crash the process. @@ -6954,6 +6966,12 @@ def setUp(self): def tearDown(self): ManagedResource._free_native_ptr = self._real_free + def _free_counts(self): + counts = {} + for handle in self.freed: + counts[handle] = counts.get(handle, 0) + 1 + return counts + # _cleanup_resources: a failing _release() must not strand the handle def test_release_failure_still_frees_handle(self): @@ -7101,6 +7119,389 @@ def test_close_after_wrap_is_idempotent(self): self.assertEqual(self.freed, [0xD00D], "handle freed more than once") + def test_every_construction_path_records_owner_pid(self): + pid = os.getpid() + + plain = _FakeHandleResource() + self.assertEqual(plain._owner_pid, pid) + + activated = _FakeHandleResource() + activated._activate(0xA1) + self.assertEqual(activated._owner_pid, pid) + + wrapped = _FakeHandleResource._wrap_native_handle(0xA2) + self.assertEqual(wrapped._owner_pid, pid) + + # A swap keeps the original stamp: + # the replacement handle was allocated by the same process + # that created the object. + wrapped._swap_handle(0xA3) + self.assertEqual(wrapped._owner_pid, pid) + + def test_activate_rejects_reserved_extra_attrs(self): + for attr, value in ( + ('_owner_pid', os.getpid() + 1), + ('_handle', 0xDEAD), + ('_lifecycle_state', LifecycleState.CLOSED), + ): + with self.subTest(attr=attr): + res = _FakeHandleResource() + stamp = res._owner_pid + + with self.assertRaises(Error) as ctx: + res._activate(0xB1, **{attr: value}) + + self.assertIn(attr, str(ctx.exception)) + self.assertEqual(res._owner_pid, stamp) + self.assertEqual( + res._lifecycle_state, LifecycleState.UNINITIALIZED) + self.assertIsNone(res._handle) + + def test_wrap_native_handle_rejects_reserved_extra_attrs(self): + with self.assertRaises(Error): + _FakeHandleResource._wrap_native_handle( + 0xB2, _owner_pid=os.getpid() + 1) + + def test_foreign_child_skips_free_for_wrapped_and_swapped(self): + wrapped = _FakeHandleResource._wrap_native_handle(0xC1) + wrapped._owner_pid = os.getpid() + 1 + wrapped.close() + + swapped = _FakeHandleResource() + swapped._activate(0xC2) + swapped._swap_handle(0xC3) + swapped._owner_pid = os.getpid() + 1 + swapped.close() + + self.assertEqual(self.freed, [], + "forked child freed a pointer its parent still owns") + # The parent still owns these handles, + # so the child must not mark the objects dead either. + self.assertEqual(wrapped._lifecycle_state, LifecycleState.ACTIVE) + self.assertEqual(swapped._handle, 0xC3) + + def test_owning_process_frees_wrapped_and_swapped_exactly_once(self): + wrapped = _FakeHandleResource._wrap_native_handle(0xC4) + wrapped.close() + wrapped.close() + + swapped = _FakeHandleResource() + swapped._activate(0xC5) + swapped._swap_handle(0xC6) + swapped.close() + + # 0xC5 was consumed by the (simulated) FFI swap, + # so only the replacement is ours to free. + self.assertEqual(self._free_counts(), {0xC4: 1, 0xC6: 1}) + + def test_foreign_child_skips_release(self): + foreign = _ReleaseRecordingResource() + foreign._activate(0xD1) + foreign._owner_pid = os.getpid() + 1 + foreign.close() + self.assertEqual(foreign.release_calls, 0) + + owned = _ReleaseRecordingResource() + owned._activate(0xD2) + owned.close() + self.assertEqual(owned.release_calls, 1) + + def test_consumed_resource_frees_nothing_in_either_process(self): + owned = _FakeHandleResource() + owned._activate(0xE1) + owned._mark_consumed() + owned.close() + + foreign = _FakeHandleResource() + foreign._activate(0xE2) + foreign._mark_consumed() + foreign._owner_pid = os.getpid() + 1 + foreign.close() + + self.assertEqual(self.freed, []) + + +class TestManagedResourceObjects(TestContextAPIs): + """Tests native resource handling management when managed manually. + """ + + 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)) + return freed + + def _make_archive(self, manifest=None): + archive = io.BytesIO() + builder = Builder(manifest or self.test_manifest) + try: + builder.to_archive(archive) + finally: + builder.close() + archive.seek(0) + return archive + + # Activation: every public constructor stamps the creating process + + def test_settings_activation_paths(self): + pid = os.getpid() + for label, factory in ( + ("Settings()", lambda: Settings()), + ("from_json", lambda: Settings.from_json('{"version_major": 1}')), + ("from_dict", lambda: Settings.from_dict({"version_major": 1})), + ): + with self.subTest(path=label): + settings = factory() + try: + self.assertTrue(settings.is_valid) + self.assertEqual(settings._owner_pid, pid) + finally: + settings.close() + + def test_context_activation_paths(self): + pid = os.getpid() + settings = Settings.from_dict({"version_major": 1}) + try: + for label, factory in ( + # No settings and no signer takes the c2pa_context_new path. + ("Context()", lambda: Context()), + # Anything else goes through the ContextBuilder path. + ("Context(settings)", lambda: Context(settings)), + ("from_dict", lambda: Context.from_dict({"version_major": 1})), + ("builder()", + lambda: Context.builder().with_settings(settings).build()), + ): + with self.subTest(path=label): + context = factory() + try: + self.assertTrue(context.is_valid) + self.assertEqual(context._owner_pid, pid) + finally: + context.close() + finally: + settings.close() + + def test_reader_activation_paths(self): + pid = os.getpid() + context = Context() + try: + with open(DEFAULT_TEST_FILE, "rb") as f: + from_stream = Reader("image/jpeg", f) + self.addCleanup(from_stream.close) + self.assertEqual(from_stream._owner_pid, pid) + + from_path = Reader(DEFAULT_TEST_FILE) + self.addCleanup(from_path.close) + self.assertEqual(from_path._owner_pid, pid) + + with open(DEFAULT_TEST_FILE, "rb") as f: + with_context = Reader(DEFAULT_TEST_FILE, context=context) + self.addCleanup(with_context.close) + self.assertEqual(with_context._owner_pid, pid) + + with open(DEFAULT_TEST_FILE, "rb") as f: + created = Reader.try_create("image/jpeg", f) + self.addCleanup(created.close) + self.assertEqual(created._owner_pid, pid) + finally: + context.close() + + def test_builder_activation_paths(self): + pid = os.getpid() + context = Context() + try: + plain = Builder(self.test_manifest) + self.addCleanup(plain.close) + self.assertEqual(plain._owner_pid, pid) + + from_json = Builder.from_json(self.test_manifest) + self.addCleanup(from_json.close) + self.assertEqual(from_json._owner_pid, pid) + + with_context = Builder(self.test_manifest, context=context) + self.addCleanup(with_context.close) + self.assertEqual(with_context._owner_pid, pid) + + # from_archive is the only caller of _wrap_native_handle, + # which bypasses __init__ entirely + wrapped = Builder.from_archive(self._make_archive()) + self.addCleanup(wrapped.close) + self.assertEqual(wrapped._owner_pid, pid) + self.assertIsNone(wrapped._context) + self.assertFalse(wrapped._has_context_signer) + finally: + context.close() + + def test_signer_activation_paths(self): + signer = self._ctx_make_signer() + self.addCleanup(signer.close) + self.assertTrue(signer.is_valid) + self.assertEqual(signer._owner_pid, os.getpid()) + + callback_signer = self._ctx_make_callback_signer() + self.addCleanup(callback_signer.close) + self.assertEqual(callback_signer._owner_pid, os.getpid()) + + # Swapping: the two public consume-and-return APIs + + def test_builder_with_archive_swaps_the_handle(self): + context = Context() + self.addCleanup(context.close) + builder = Builder(self.test_manifest, context=context) + original_handle = builder._handle + original_stamp = builder._owner_pid + + result = builder.with_archive(self._make_archive()) + + self.assertIs(result, builder, "with_archive should return self") + self.assertNotEqual(builder._handle, original_handle, + "the native handle was not replaced") + self.assertEqual(builder._lifecycle_state, LifecycleState.ACTIVE) + # The replacement came from this process, so the stamp still applies. + self.assertEqual(builder._owner_pid, original_stamp) + self.assertEqual(builder._owner_pid, os.getpid()) + builder.close() + + def test_reader_with_fragment_swaps_the_handle(self): + init_path = os.path.join(FIXTURES_DIR, "dashinit.mp4") + fragment_path = os.path.join(FIXTURES_DIR, "dash1.m4s") + context = Context() + self.addCleanup(context.close) + + with open(init_path, "rb") as init: + reader = Reader("video/mp4", init, context=context) + self.addCleanup(reader.close) + original_handle = reader._handle + + # The Reader consumed the first handle, so the init stream is reopened. + with open(init_path, "rb") as init, open(fragment_path, "rb") as frag: + result = reader.with_fragment("video/mp4", init, frag) + + self.assertIs(result, reader, "with_fragment should return self") + self.assertNotEqual(reader._handle, original_handle, + "the native handle was not replaced") + self.assertEqual(reader._lifecycle_state, LifecycleState.ACTIVE) + self.assertEqual(reader._owner_pid, os.getpid()) + + def test_swapped_builder_is_freed_exactly_once(self): + context = Context() + self.addCleanup(context.close) + builder = Builder(self.test_manifest, context=context) + original_handle = builder._handle + # The helper closes a temporary Builder, + # whose free would otherwise be counted here. + archive = self._make_archive() + + # Instrument across the swap so a free of the consumed pointer is recorded. + freed = self._instrument_frees() + builder.with_archive(archive) + swapped_handle = builder._handle + + self.assertEqual(freed, [], "the swap freed the consumed pointer") + + builder.close() + builder.close() + + # Only the replacement is ours to free: the original was consumed by + # the FFI call that returned it. + self.assertEqual(freed, [swapped_handle]) + self.assertNotIn(original_handle, freed) + + def test_repeated_swaps_on_one_builder(self): + # Each with_archive consumes the handle the previous one returned, so + # a chain of swaps is where a wrong swap surfaces: keeping the + # consumed pointer makes the next call raise UntrackedPointer from + # the native registry. + context = Context() + self.addCleanup(context.close) + builder = Builder(self.test_manifest, context=context) + self.addCleanup(builder.close) + + seen = [builder._handle] + for _ in range(5): + builder.with_archive(self._make_archive()) + self.assertEqual(builder._lifecycle_state, LifecycleState.ACTIVE) + seen.append(builder._handle) + + self.assertTrue(builder.is_valid) + self.assertEqual(builder._owner_pid, os.getpid()) + + def test_context_consumes_signer_but_not_settings(self): + settings = Settings.from_dict({"version_major": 1}) + signer = self._ctx_make_signer() + + context = Context(settings=settings, signer=signer) + self.addCleanup(context.close) + + # The signer pointer moved to the native context builder. + self.assertIsNone(signer._handle) + self.assertEqual(signer._lifecycle_state, LifecycleState.CLOSED) + self.assertTrue(context.has_signer) + + # Settings are copied, not consumed, so the caller still owns them. + self.assertTrue(settings.is_valid) + self.assertEqual(settings._owner_pid, os.getpid()) + settings.close() + + def test_consumed_signer_close_frees_nothing(self): + signer = self._ctx_make_signer() + context = Context(signer=signer) + self.addCleanup(context.close) + + freed = self._instrument_frees() + signer.close() + + self.assertEqual(freed, [], + "closing a consumed Signer freed a pointer the " + "context now owns") + + def test_builder_with_archive_null_return_consumes_self(self): + builder = Builder(self.test_manifest) + real_call = c2pa_module._lib.c2pa_builder_with_archive + c2pa_module._lib.c2pa_builder_with_archive = lambda b, s: None + try: + with self.assertRaises(Error): + builder.with_archive(self._make_archive()) + finally: + c2pa_module._lib.c2pa_builder_with_archive = real_call + + # The FFI consumed the old handle and returned no replacement, + # so there is nothing left for this object to own... + self.assertIsNone(builder._handle) + self.assertEqual(builder._lifecycle_state, LifecycleState.CLOSED) + + freed = self._instrument_frees() + builder.close() + self.assertEqual(freed, []) + + def test_reader_with_fragment_null_return_consumes_self(self): + init_path = os.path.join(FIXTURES_DIR, "dashinit.mp4") + fragment_path = os.path.join(FIXTURES_DIR, "dash1.m4s") + with open(init_path, "rb") as init: + reader = Reader("video/mp4", init) + + real_call = c2pa_module._lib.c2pa_reader_with_fragment + c2pa_module._lib.c2pa_reader_with_fragment = ( + lambda r, f, s, frag: None) + try: + with open(init_path, "rb") as init, \ + open(fragment_path, "rb") as frag: + with self.assertRaises(Error): + reader.with_fragment("video/mp4", init, frag) + finally: + c2pa_module._lib.c2pa_reader_with_fragment = real_call + + self.assertIsNone(reader._handle) + self.assertEqual(reader._lifecycle_state, LifecycleState.CLOSED) + + freed = self._instrument_frees() + reader.close() + self.assertEqual(freed, []) + class TestNativeHandleOwnership(unittest.TestCase): """Ownership hand-offs between Python and the native library, driven by diff --git a/tests/test_unit_tests_threaded.py b/tests/test_unit_tests_threaded.py index efb7ba27..44609adc 100644 --- a/tests/test_unit_tests_threaded.py +++ b/tests/test_unit_tests_threaded.py @@ -2911,5 +2911,161 @@ 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. + """ + + def setUp(self): + self.freed = [] + self._real_free = ManagedResource._free_native_ptr + ManagedResource._free_native_ptr = staticmethod(self.freed.append) + + def tearDown(self): + ManagedResource._free_native_ptr = self._real_free + + def _free_counts(self): + counts = {} + for handle in self.freed: + counts[handle] = counts.get(handle, 0) + 1 + return counts + + def test_cross_thread_create_and_close_frees_exactly_once(self): + count = 300 + pid = os.getpid() + + def create(index): + res = _ConcreteResource() + res._activate(0x10000 + index) + return res + + with concurrent.futures.ThreadPoolExecutor(max_workers=8) as pool: + created = list(pool.map(create, range(count))) + + # 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: + made_on_main = [] + for index in range(count): + res = _ConcreteResource() + res._activate(0x20000 + index) + made_on_main.append(res) + # Created on the main thread, closed on worker threads. + list(pool.map(lambda r: r.close(), made_on_main)) + + expected = {0x10000 + i: 1 for i in range(count)} + expected.update({0x20000 + i: 1 for i in range(count)}) + # Restrict to this test's handles: resources dropped by other tests in + # the class can be collected at any point and land in self.freed. + counts = {handle: value + for handle, value in self._free_counts().items() + if handle in expected} + self.assertEqual(counts, expected) + + def test_third_thread_gc_of_dropped_reference_frees_exactly_once(self): + import gc + + def make_and_drop(index): + res = _ConcreteResource() + res._activate(0x30000 + index) + # Reference dies here; __del__ may run on this thread or later. + return index + + with concurrent.futures.ThreadPoolExecutor(max_workers=4) as pool: + list(pool.map(make_and_drop, range(200))) + + gc.collect() + + # Count only this test's handles: resources dropped by other tests in + # the class can be collected at any point and land in self.freed. + 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") + + +class TestSettingsAsContextAcrossThreads(unittest.TestCase): + """Tests Settings handed between threads and reused as the basis + for Contexts, using real native handles. + """ + + def setUp(self): + self.manifest = { + "claim_generator": "threaded_stamp_test", + "format": "image/jpeg", + "assertions": [], + } + + def test_settings_relayed_across_threads_stays_usable(self): + settings = Settings() + pid = os.getpid() + results = [] + errors = [] + + def build_context_and_builder(): + try: + context = Context(settings=settings) + builder = Builder(self.manifest, context=context) + results.append(( + builder._owner_pid, context._owner_pid, builder.is_valid)) + builder.close() + context.close() + except Exception as exc: + errors.append(exc) + + # Sequential hand-off: each thread owns the Settings for its turn. + for _ in range(8): + thread = threading.Thread(target=build_context_and_builder) + thread.start() + thread.join() + + settings.close() + + 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) + + def test_context_created_on_one_thread_closed_on_another(self): + created = [] + errors = [] + + def create(): + try: + created.append(Context()) + except Exception as exc: + errors.append(exc) + + def close_all(): + try: + for context in created: + context.close() + except Exception as exc: + errors.append(exc) + + maker = threading.Thread(target=create) + maker.start() + maker.join() + + closer = threading.Thread(target=close_all) + closer.start() + closer.join() + + self.assertEqual(errors, []) + self.assertEqual(len(created), 1) + for context in created: + self.assertEqual(context._lifecycle_state, LifecycleState.CLOSED) + self.assertIsNone(context._handle) + + if __name__ == '__main__': unittest.main() From e926b5f05d8d661f4f4b59e45e33264db908fd42 Mon Sep 17 00:00:00 2001 From: tmathern <60901087+tmathern@users.noreply.github.com> Date: Thu, 16 Jul 2026 22:03:46 -0700 Subject: [PATCH 05/20] fix: Make isntances swappable --- tests/test_unit_tests.py | 284 +++++++++++++++++++-------------------- 1 file changed, 138 insertions(+), 146 deletions(-) diff --git a/tests/test_unit_tests.py b/tests/test_unit_tests.py index 78154cca..fd7b3669 100644 --- a/tests/test_unit_tests.py +++ b/tests/test_unit_tests.py @@ -6925,40 +6925,39 @@ def test_callbacks_return_minus_one_after_stream_collected(self): self.assertEqual(flush_cb(None), -1) -class _FakeHandleResource(ManagedResource): - """ManagedResource with a fake integer handle, for lifecycle tests that - must not touch the native library.""" - - -class _CallbackHoldingResource(ManagedResource): - """Mimics Signer: its _release() reads an attribute that must have been - supplied by __init__ or by _activate's extra_attrs.""" - - def _release(self): - if self._callback_cb: - self._callback_cb = None +class TestManagedResourceLifecycle(unittest.TestCase): + """Lifecycle primitives (_activate, _swap_handle, _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. + setUp records frees instead of performing them, so a miscount reads as a + leak or a double-free rather than a crash. Tests holding real handles + call _use_real_frees() first. + """ -class _ReleaseRecordingResource(ManagedResource): - """Records _release() calls for test asserts.""" + class _FakeHandleResource(ManagedResource): + """Concrete subclass with no resources of its own.""" - def __init__(self): - super().__init__() - self.release_calls = 0 + class _CallbackHoldingResource(ManagedResource): + """Mimics Signer: its _release() reads an attribute that must have + been supplied by __init__ or by _activate's extra_attrs.""" - def _release(self): - self.release_calls += 1 + def _release(self): + if self._callback_cb: + self._callback_cb = None + class _ReleaseRecordingResource(ManagedResource): + """Records _release() calls for test asserts.""" -class TestManagedResourceLifecycle(unittest.TestCase): - """Lifecycle primitives (_activate, _swap_handle, _wrap_native_handle) - and the _owner_pid stamp that governs which process may free a handle. + def __init__(self): + super().__init__() + self.release_calls = 0 - These use fake integer handles and record calls to _free_native_ptr - instead of freeing anything, so a mistake here cannot crash the process. - """ + def _release(self): + self.release_calls += 1 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) @@ -6972,10 +6971,22 @@ def _free_counts(self): counts[handle] = counts.get(handle, 0) + 1 return counts + def _use_real_frees(self): + """Undo setUp's recorder, so native handles are really freed.""" + ManagedResource._free_native_ptr = self._real_free + + def _make_signer(self): + with open(os.path.join(self.data_dir, "es256_certs.pem"), "rb") as f: + certs = f.read() + with open(os.path.join(self.data_dir, "es256_private.key"), "rb") as f: + key = f.read() + return Signer.from_info(C2paSignerInfo( + b"es256", certs, key, b"http://timestamp.digicert.com")) + # _cleanup_resources: a failing _release() must not strand the handle def test_release_failure_still_frees_handle(self): - res = _CallbackHoldingResource() + res = self._CallbackHoldingResource() # _callback_cb is never set, so _release() raises AttributeError. res._activate(0xBBBB) @@ -6986,7 +6997,7 @@ def test_release_failure_still_frees_handle(self): self.assertIsNone(res._handle) def test_release_failure_is_logged(self): - res = _CallbackHoldingResource() + res = self._CallbackHoldingResource() res._activate(0xBBBB) with self.assertLogs('c2pa', level='ERROR') as captured: @@ -6999,7 +7010,7 @@ def test_release_failure_is_logged(self): # _activate guards def test_activate_rejects_null_handle(self): - res = _FakeHandleResource() + res = self._FakeHandleResource() with self.assertRaises(Error) as ctx: res._activate(None) @@ -7008,7 +7019,7 @@ def test_activate_rejects_null_handle(self): self.assertEqual(res._lifecycle_state, LifecycleState.UNINITIALIZED) def test_activate_rejects_double_activation(self): - res = _FakeHandleResource() + res = self._FakeHandleResource() res._activate(0x1111) with self.assertRaises(Error) as ctx: @@ -7021,7 +7032,7 @@ def test_activate_rejects_double_activation(self): self.assertEqual(self.freed, [0x1111]) def test_activate_rejects_reactivation_after_close(self): - res = _FakeHandleResource() + res = self._FakeHandleResource() res._activate(0x3333) res.close() self.freed.clear() @@ -7035,7 +7046,7 @@ def test_activate_rejects_reactivation_after_close(self): self.assertEqual(self.freed, []) def test_activate_does_not_mutate_on_rejection(self): - res = _FakeHandleResource() + res = self._FakeHandleResource() res._activate(0x5555) with self.assertRaises(Error): @@ -7047,7 +7058,7 @@ def test_activate_does_not_mutate_on_rejection(self): # _swap_handle def test_swap_handle_does_not_free_consumed_handle(self): - res = _FakeHandleResource() + res = self._FakeHandleResource() res._activate(0xAAA1) res._swap_handle(0xAAA2) @@ -7061,19 +7072,19 @@ def test_swap_handle_does_not_free_consumed_handle(self): self.assertEqual(self.freed, [0xAAA2]) def test_swap_handle_requires_active_resource(self): - uninitialized = _FakeHandleResource() + uninitialized = self._FakeHandleResource() with self.assertRaises(Error) as ctx: uninitialized._swap_handle(0x1) self.assertIn("not active", str(ctx.exception)) - closed = _FakeHandleResource() + closed = self._FakeHandleResource() closed._activate(0x2) closed.close() with self.assertRaises(Error): closed._swap_handle(0x3) def test_swap_handle_rejects_null_replacement(self): - res = _FakeHandleResource() + res = self._FakeHandleResource() res._activate(0x7777) with self.assertRaises(Error) as ctx: @@ -7109,10 +7120,10 @@ def _release(self): def test_wrap_native_handle_rejects_null(self): with self.assertRaises(Error): - _FakeHandleResource._wrap_native_handle(None) + self._FakeHandleResource._wrap_native_handle(None) def test_close_after_wrap_is_idempotent(self): - obj = _FakeHandleResource._wrap_native_handle(0xD00D) + obj = self._FakeHandleResource._wrap_native_handle(0xD00D) obj.close() obj.close() @@ -7122,14 +7133,14 @@ def test_close_after_wrap_is_idempotent(self): def test_every_construction_path_records_owner_pid(self): pid = os.getpid() - plain = _FakeHandleResource() + plain = self._FakeHandleResource() self.assertEqual(plain._owner_pid, pid) - activated = _FakeHandleResource() + activated = self._FakeHandleResource() activated._activate(0xA1) self.assertEqual(activated._owner_pid, pid) - wrapped = _FakeHandleResource._wrap_native_handle(0xA2) + wrapped = self._FakeHandleResource._wrap_native_handle(0xA2) self.assertEqual(wrapped._owner_pid, pid) # A swap keeps the original stamp: @@ -7145,7 +7156,7 @@ def test_activate_rejects_reserved_extra_attrs(self): ('_lifecycle_state', LifecycleState.CLOSED), ): with self.subTest(attr=attr): - res = _FakeHandleResource() + res = self._FakeHandleResource() stamp = res._owner_pid with self.assertRaises(Error) as ctx: @@ -7159,15 +7170,15 @@ def test_activate_rejects_reserved_extra_attrs(self): def test_wrap_native_handle_rejects_reserved_extra_attrs(self): with self.assertRaises(Error): - _FakeHandleResource._wrap_native_handle( + self._FakeHandleResource._wrap_native_handle( 0xB2, _owner_pid=os.getpid() + 1) def test_foreign_child_skips_free_for_wrapped_and_swapped(self): - wrapped = _FakeHandleResource._wrap_native_handle(0xC1) + wrapped = self._FakeHandleResource._wrap_native_handle(0xC1) wrapped._owner_pid = os.getpid() + 1 wrapped.close() - swapped = _FakeHandleResource() + swapped = self._FakeHandleResource() swapped._activate(0xC2) swapped._swap_handle(0xC3) swapped._owner_pid = os.getpid() + 1 @@ -7181,11 +7192,11 @@ def test_foreign_child_skips_free_for_wrapped_and_swapped(self): self.assertEqual(swapped._handle, 0xC3) def test_owning_process_frees_wrapped_and_swapped_exactly_once(self): - wrapped = _FakeHandleResource._wrap_native_handle(0xC4) + wrapped = self._FakeHandleResource._wrap_native_handle(0xC4) wrapped.close() wrapped.close() - swapped = _FakeHandleResource() + swapped = self._FakeHandleResource() swapped._activate(0xC5) swapped._swap_handle(0xC6) swapped.close() @@ -7195,24 +7206,24 @@ def test_owning_process_frees_wrapped_and_swapped_exactly_once(self): self.assertEqual(self._free_counts(), {0xC4: 1, 0xC6: 1}) def test_foreign_child_skips_release(self): - foreign = _ReleaseRecordingResource() + foreign = self._ReleaseRecordingResource() foreign._activate(0xD1) foreign._owner_pid = os.getpid() + 1 foreign.close() self.assertEqual(foreign.release_calls, 0) - owned = _ReleaseRecordingResource() + owned = self._ReleaseRecordingResource() owned._activate(0xD2) owned.close() self.assertEqual(owned.release_calls, 1) def test_consumed_resource_frees_nothing_in_either_process(self): - owned = _FakeHandleResource() + owned = self._FakeHandleResource() owned._activate(0xE1) owned._mark_consumed() owned.close() - foreign = _FakeHandleResource() + foreign = self._FakeHandleResource() foreign._activate(0xE2) foreign._mark_consumed() foreign._owner_pid = os.getpid() + 1 @@ -7220,6 +7231,83 @@ def test_consumed_resource_frees_nothing_in_either_process(self): self.assertEqual(self.freed, []) + def test_signer_init_rejects_null_pointer(self): + with self.assertRaises(Error): + Signer(None) + + def test_builder_from_archive_wraps_handle(self): + self._use_real_frees() + archive = io.BytesIO() + Builder({"claim_generator": "test", "format": "image/jpeg"}).to_archive( + archive) + + builder = Builder.from_archive(io.BytesIO(archive.getvalue())) + + self.assertTrue(builder.is_valid) + self.assertIsNone(builder._context) + self.assertFalse(builder._has_context_signer) + builder.close() + self.assertEqual(builder._lifecycle_state, LifecycleState.CLOSED) + + def test_context_build_failure_consumes_signer(self): + # c2pa_context_builder_set_signer takes ownership of the signer + # pointer immediately, so a later build failure must still leave the + # Signer consumed. Otherwise it holds a pointer the native side has + # already freed. + self._use_real_frees() + signer = self._make_signer() + real_build = c2pa_module._lib.c2pa_context_builder_build + c2pa_module._lib.c2pa_context_builder_build = lambda ptr: None + try: + with self.assertRaises(Error): + Context(signer=signer) + finally: + c2pa_module._lib.c2pa_context_builder_build = real_build + + self.assertIsNone(signer._handle, + "Signer still holds a pointer the native side freed") + self.assertEqual(signer._lifecycle_state, LifecycleState.CLOSED) + + # 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: + signer.close() + finally: + ManagedResource._free_native_ptr = real_free + self.assertEqual(freed, []) + + def test_context_with_signer_consumes_it_on_success(self): + self._use_real_frees() + signer = self._make_signer() + + context = Context(signer=signer) + + self.assertTrue(context.is_valid) + self.assertIsNone(signer._handle) + self.assertEqual(signer._lifecycle_state, LifecycleState.CLOSED) + self.assertTrue(context.has_signer) + context.close() + + def test_construction_failure_leaves_nothing_to_free(self): + # Activation happens after the null check, so a failed construction + # leaves no handle on the object for __del__ to find. + real_new = c2pa_module._lib.c2pa_context_new + c2pa_module._lib.c2pa_context_new = lambda: None + try: + with self.assertRaises(Error): + Context() + finally: + c2pa_module._lib.c2pa_context_new = real_new + + real_json = c2pa_module._lib.c2pa_builder_from_json + c2pa_module._lib.c2pa_builder_from_json = lambda j: None + try: + with self.assertRaises(Error): + Builder({"claim_generator": "test"}) + finally: + c2pa_module._lib.c2pa_builder_from_json = real_json class TestManagedResourceObjects(TestContextAPIs): """Tests native resource handling management when managed manually. @@ -7245,8 +7333,6 @@ def _make_archive(self, manifest=None): archive.seek(0) return archive - # Activation: every public constructor stamps the creating process - def test_settings_activation_paths(self): pid = os.getpid() for label, factory in ( @@ -7346,8 +7432,6 @@ def test_signer_activation_paths(self): self.addCleanup(callback_signer.close) self.assertEqual(callback_signer._owner_pid, os.getpid()) - # Swapping: the two public consume-and-return APIs - def test_builder_with_archive_swaps_the_handle(self): context = Context() self.addCleanup(context.close) @@ -7503,97 +7587,5 @@ def test_reader_with_fragment_null_return_consumes_self(self): self.assertEqual(freed, []) -class TestNativeHandleOwnership(unittest.TestCase): - """Ownership hand-offs between Python and the native library, driven by - fault injection at the FFI boundary.""" - - def setUp(self): - self.data_dir = os.path.join(os.path.dirname(__file__), "fixtures") - - def _make_signer(self): - with open(os.path.join(self.data_dir, "es256_certs.pem"), "rb") as f: - certs = f.read() - with open(os.path.join(self.data_dir, "es256_private.key"), "rb") as f: - key = f.read() - return Signer.from_info(C2paSignerInfo( - b"es256", certs, key, b"http://timestamp.digicert.com")) - - def test_signer_init_rejects_null_pointer(self): - with self.assertRaises(Error): - Signer(None) - - def test_builder_from_archive_wraps_handle(self): - archive = io.BytesIO() - Builder({"claim_generator": "test", "format": "image/jpeg"}).to_archive( - archive) - - builder = Builder.from_archive(io.BytesIO(archive.getvalue())) - - self.assertTrue(builder.is_valid) - self.assertIsNone(builder._context) - self.assertFalse(builder._has_context_signer) - builder.close() - self.assertEqual(builder._lifecycle_state, LifecycleState.CLOSED) - - def test_context_build_failure_consumes_signer(self): - # c2pa_context_builder_set_signer takes ownership of the signer - # pointer immediately, so a later build failure must still leave the - # Signer consumed. Otherwise it holds a pointer the native side has - # already freed. - signer = self._make_signer() - real_build = c2pa_module._lib.c2pa_context_builder_build - c2pa_module._lib.c2pa_context_builder_build = lambda ptr: None - try: - with self.assertRaises(Error): - Context(signer=signer) - finally: - c2pa_module._lib.c2pa_context_builder_build = real_build - - self.assertIsNone(signer._handle, - "Signer still holds a pointer the native side freed") - self.assertEqual(signer._lifecycle_state, LifecycleState.CLOSED) - - # 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: - signer.close() - finally: - ManagedResource._free_native_ptr = real_free - self.assertEqual(freed, []) - - def test_context_with_signer_consumes_it_on_success(self): - signer = self._make_signer() - - context = Context(signer=signer) - - self.assertTrue(context.is_valid) - self.assertIsNone(signer._handle) - self.assertEqual(signer._lifecycle_state, LifecycleState.CLOSED) - self.assertTrue(context.has_signer) - context.close() - - def test_construction_failure_leaves_nothing_to_free(self): - # A failed FFI construction must not leave a half-constructed object - # holding a handle. Activation happens after the check, so _handle is - # never set. - real_new = c2pa_module._lib.c2pa_context_new - c2pa_module._lib.c2pa_context_new = lambda: None - try: - with self.assertRaises(Error): - Context() - finally: - c2pa_module._lib.c2pa_context_new = real_new - - real_json = c2pa_module._lib.c2pa_builder_from_json - c2pa_module._lib.c2pa_builder_from_json = lambda j: None - try: - with self.assertRaises(Error): - Builder({"claim_generator": "test"}) - finally: - c2pa_module._lib.c2pa_builder_from_json = real_json - - if __name__ == '__main__': unittest.main(warnings='ignore') From 222b9a8b31454fd763c81f0a8345ad33456e8261 Mon Sep 17 00:00:00 2001 From: tmathern <60901087+tmathern@users.noreply.github.com> Date: Thu, 16 Jul 2026 22:31:23 -0700 Subject: [PATCH 06/20] fix: Update tests --- src/c2pa/c2pa.py | 81 ++++++++++++------ tests/test_unit_tests.py | 178 +++++++++++++++++++++++++++++++++++++-- 2 files changed, 226 insertions(+), 33 deletions(-) diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index 9ef2f4d3..dec8a8bc 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -237,7 +237,7 @@ class ManagedResource: Subclasses must: - Call `_activate(handle)` once the native pointer is created and - validated, which takes ownership of it and marks the resource ACTIVE. + 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. @@ -246,10 +246,22 @@ class ManagedResource: - Override `_release()` to free class-specific resources (streams, caches, callbacks, etc.), called before the native pointer is freed. + - Override `_init_attrs()` to set the class's own attributes to their + defaults, and call it from `__init__`. `_wrap_native_handle` calls it + too, so an instance built around an existing handle is never missing + the attributes the rest of the class reads. The native pointer is freed automatically via `_free_native_ptr`. """ + def _init_attrs(self): + """Set this class's own attributes to their defaults. + + Called by __init__ and by _wrap_native_handle, which bypasses + __init__. Keeping the defaults here means an instance built around an + existing handle is never missing what the rest of the class reads. + """ + def __init__(self): self._lifecycle_state = LifecycleState.UNINITIALIZED self._handle = None @@ -371,14 +383,17 @@ def _wrap_native_handle(cls, handle, **extra_attrs): """Build a brand-new instance around an already-valid, already-owned native handle, bypassing __init__ entirely. - Because __init__ is bypassed, every attribute the subclass's - `_release()` reads must be passed in `extra_attrs`. - The lifecycle attributes are still off limits there, - and the instance is stamped with the creating process either way. + __init__ is bypassed, so `_init_attrs()` supplies the class's own + defaults; `extra_attrs` is only for overriding them. The lifecycle + attributes are off limits there, and the instance is stamped with the + creating process either way. + + Ownership of `handle` only transfers once this returns. If it raises, + the caller still owns the pointer and must free it. Args: handle: Non-null native pointer to take ownership of - **extra_attrs: Instance attributes to set before activating + **extra_attrs: Instance attributes overriding the defaults Raises: C2paError: If the handle is null, or extra_attrs names a @@ -387,6 +402,7 @@ def _wrap_native_handle(cls, handle, **extra_attrs): obj = object.__new__(cls) # Stamps _owner_pid, which the fork guard relies on. ManagedResource.__init__(obj) + obj._init_attrs() obj._activate(handle, **extra_attrs) return obj @@ -1549,8 +1565,7 @@ def __init__( C2paError: If creation fails """ super().__init__() - self._has_signer = False - self._signer_callback_cb = None + self._init_attrs() if settings is None and signer is None: # Simple default context @@ -1614,6 +1629,10 @@ def __init__( pass raise + def _init_attrs(self): + self._has_signer = False + self._signer_callback_cb = None + def _release(self): """Release Context-specific resources.""" self._signer_callback_cb = None @@ -2262,20 +2281,7 @@ def __init__( contain invalid UTF-8 characters """ super().__init__() - - self._own_stream = None - - # This is used to keep track of a file - # we may have opened ourselves, and that we need to close later - self._backing_file = None - - # Caches for manifest JSON string and parsed data. - # These are invalidated when with_fragment() is called, because each - # new BMFF fragment can refine or update the manifest content as the - # reader progressively builds its understanding of the fragmented stream. - # They are also cleared on close() to release memory. - self._manifest_json_str_cache = None - self._manifest_data_cache = None + self._init_attrs() self._context = context @@ -2460,6 +2466,22 @@ def _init_from_context(self, context, format_or_path, self._close_streams() raise + def _init_attrs(self): + self._own_stream = None + + # Tracks a file we opened ourselves and must close later. + self._backing_file = None + + # Caches for manifest JSON string and parsed data. + # These are invalidated when with_fragment() is called, because each + # new BMFF fragment can refine or update the manifest content as the + # reader progressively builds its understanding of the fragmented + # stream. They are also cleared on close() to release memory. + self._manifest_json_str_cache = None + self._manifest_data_cache = None + + self._context = None + def _close_streams(self): """Close owned stream and backing file if present.""" if getattr(self, '_own_stream', None): @@ -3129,8 +3151,14 @@ def from_archive( "Failed to create builder from archive" ) - return cls._wrap_native_handle( - handle, _context=None, _has_context_signer=False) + try: + # A builder from an archive carries no context, which is what + # _init_attrs() already defaults to. + return cls._wrap_native_handle(handle) + except Exception: + # No instance took ownership, so the handle is still ours. + ManagedResource._free_native_ptr(handle) + raise finally: stream_obj.close() @@ -3164,6 +3192,7 @@ def __init__( C2paError.Json: If the manifest JSON cannot be serialized """ super().__init__() + self._init_attrs() self._context = context self._has_context_signer = ( @@ -3221,6 +3250,10 @@ def _init_from_context(self, context, json_str): self._activate(new_ptr) + def _init_attrs(self): + self._context = None + self._has_context_signer = False + def set_no_embed(self): """Set the no-embed flag. diff --git a/tests/test_unit_tests.py b/tests/test_unit_tests.py index fd7b3669..180447b0 100644 --- a/tests/test_unit_tests.py +++ b/tests/test_unit_tests.py @@ -11,9 +11,12 @@ # specific language governing permissions and limitations under # each license. +import gc +import inspect import os import io import json +import re import unittest import ctypes import warnings @@ -7309,12 +7312,29 @@ def test_construction_failure_leaves_nothing_to_free(self): finally: c2pa_module._lib.c2pa_builder_from_json = real_json +def _ptr_addr(ptr): + """Address a ctypes pointer points at, or None for a null pointer. + + ctypes pointers compare by identity, not by value: two pointer objects + for the same address are unequal. Compare addresses instead. + """ + if not ptr: + return None + return ctypes.cast(ptr, ctypes.c_void_p).value + + class TestManagedResourceObjects(TestContextAPIs): """Tests native resource handling management when managed manually. """ def _instrument_frees(self): - """Record frees instead of performing them, and restore on teardown.""" + """Record frees instead of performing them, and restore on teardown. + + This patches the base class, so every ManagedResource freed while the + patch is installed lands in the list, including objects the garbage + collector reclaims mid-test. Ask _free_count() about one handle rather + than asserting on the length of the list. + """ freed = [] real_free = ManagedResource._free_native_ptr ManagedResource._free_native_ptr = staticmethod(freed.append) @@ -7323,6 +7343,12 @@ def _instrument_frees(self): ManagedResource, '_free_native_ptr', real_free)) return freed + def _free_count(self, freed, handle): + """How many times `handle` was freed, ignoring unrelated frees.""" + target = _ptr_addr(handle) + self.assertIsNotNone(target, "cannot count frees of a null handle") + return sum(1 for ptr in freed if _ptr_addr(ptr) == target) + def _make_archive(self, manifest=None): archive = io.BytesIO() builder = Builder(manifest or self.test_manifest) @@ -7476,8 +7502,6 @@ def test_swapped_builder_is_freed_exactly_once(self): self.addCleanup(context.close) builder = Builder(self.test_manifest, context=context) original_handle = builder._handle - # The helper closes a temporary Builder, - # whose free would otherwise be counted here. archive = self._make_archive() # Instrument across the swap so a free of the consumed pointer is recorded. @@ -7485,15 +7509,16 @@ def test_swapped_builder_is_freed_exactly_once(self): builder.with_archive(archive) swapped_handle = builder._handle - self.assertEqual(freed, [], "the swap freed the consumed pointer") + self.assertEqual(self._free_count(freed, original_handle), 0, + "the swap freed the consumed pointer") builder.close() builder.close() # Only the replacement is ours to free: the original was consumed by # the FFI call that returned it. - self.assertEqual(freed, [swapped_handle]) - self.assertNotIn(original_handle, freed) + self.assertEqual(self._free_count(freed, swapped_handle), 1) + self.assertEqual(self._free_count(freed, original_handle), 0) def test_repeated_swaps_on_one_builder(self): # Each with_archive consumes the handle the previous one returned, so @@ -7533,18 +7558,22 @@ def test_context_consumes_signer_but_not_settings(self): def test_consumed_signer_close_frees_nothing(self): signer = self._ctx_make_signer() + # Captured before the context consumes it: close() nulls the handle, + # so afterwards there is no pointer left to identify the free by. + signer_handle = signer._handle context = Context(signer=signer) self.addCleanup(context.close) freed = self._instrument_frees() signer.close() - self.assertEqual(freed, [], + self.assertEqual(self._free_count(freed, signer_handle), 0, "closing a consumed Signer freed a pointer the " "context now owns") def test_builder_with_archive_null_return_consumes_self(self): builder = Builder(self.test_manifest) + consumed_handle = builder._handle real_call = c2pa_module._lib.c2pa_builder_with_archive c2pa_module._lib.c2pa_builder_with_archive = lambda b, s: None try: @@ -7560,13 +7589,15 @@ def test_builder_with_archive_null_return_consumes_self(self): freed = self._instrument_frees() builder.close() - self.assertEqual(freed, []) + self.assertEqual(self._free_count(freed, consumed_handle), 0, + "close() freed a handle the FFI already consumed") def test_reader_with_fragment_null_return_consumes_self(self): init_path = os.path.join(FIXTURES_DIR, "dashinit.mp4") fragment_path = os.path.join(FIXTURES_DIR, "dash1.m4s") with open(init_path, "rb") as init: reader = Reader("video/mp4", init) + consumed_handle = reader._handle real_call = c2pa_module._lib.c2pa_reader_with_fragment c2pa_module._lib.c2pa_reader_with_fragment = ( @@ -7584,7 +7615,136 @@ def test_reader_with_fragment_null_return_consumes_self(self): freed = self._instrument_frees() reader.close() - self.assertEqual(freed, []) + self.assertEqual(self._free_count(freed, consumed_handle), 0, + "close() freed a handle the FFI already consumed") + + # 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 + # only tests that drive the primitive as the generic entry point it is. + + def _raw_builder_handle(self): + manifest = json.dumps( + {"claim_generator": "raw_ffi_test", "format": "image/jpeg"} + ).encode("utf-8") + handle = c2pa_module._lib.c2pa_builder_from_json(manifest) + self.assertTrue(handle, "the FFI did not return a builder pointer") + return handle + + def test_wrap_raw_ffi_builder_pointer(self): + builder = Builder._wrap_native_handle( + self._raw_builder_handle(), + _context=None, _has_context_signer=False) + + self.assertTrue(builder.is_valid) + self.assertEqual(builder._lifecycle_state, LifecycleState.ACTIVE) + self.assertEqual(builder._owner_pid, os.getpid()) + + archive = io.BytesIO() + builder.to_archive(archive) + self.assertTrue(archive.getvalue()) + + builder.close() + self.assertEqual(builder._lifecycle_state, LifecycleState.CLOSED) + self.assertIsNone(builder._handle) + + def test_wrap_raw_ffi_settings_pointer(self): + handle = c2pa_module._lib.c2pa_settings_new() + self.assertTrue(handle) + + settings = Settings._wrap_native_handle(handle) + try: + self.assertTrue(settings.is_valid) + self.assertEqual(settings._owner_pid, os.getpid()) + settings.set("version_major", "1") + finally: + settings.close() + + def test_wrap_raw_ffi_context_pointer(self): + handle = c2pa_module._lib.c2pa_context_new() + self.assertTrue(handle) + + context = Context._wrap_native_handle( + handle, _has_signer=False, _signer_callback_cb=None) + try: + self.assertTrue(context.is_valid) + self.assertEqual(context._owner_pid, os.getpid()) + # Handing the wrapped pointer back to the FFI proves it is live. + builder = Builder(self.test_manifest, context=context) + self.assertTrue(builder.is_valid) + builder.close() + finally: + context.close() + + def test_wrapped_raw_pointer_freed_exactly_once(self): + handle = self._raw_builder_handle() + freed = self._instrument_frees() + + builder = Builder._wrap_native_handle( + handle, _context=None, _has_context_signer=False) + builder.close() + builder.close() + del builder + gc.collect() + + self.assertEqual(self._free_count(freed, handle), 1, + "wrapped handle not freed exactly once") + + def test_wrap_supplies_defaults_without_extra_attrs(self): + # _init_attrs() runs on the wrap path, so a caller that passes no + # extra_attrs still gets an instance the rest of the class can read. + builder = Builder._wrap_native_handle(self._raw_builder_handle()) + self.addCleanup(builder.close) + + self.assertIsNone(builder._context) + self.assertFalse(builder._has_context_signer) + + def test_wrap_extra_attrs_override_defaults(self): + context = Context() + self.addCleanup(context.close) + + builder = Builder._wrap_native_handle( + self._raw_builder_handle(), _context=context) + self.addCleanup(builder.close) + + self.assertIs(builder._context, context) + # Untouched by the override, so still the default. + self.assertFalse(builder._has_context_signer) + + def test_init_attrs_covers_what_init_sets(self): + # Anything __init__ sets but _init_attrs() misses is absent on a + # wrapped instance, which is the trap _init_attrs() exists to close. + for cls in (Builder, Context, Reader): + with self.subTest(cls=cls.__name__): + defaulted = set(re.findall( + r"self\.(_[a-z][a-z0-9_]*)\s*=", + inspect.getsource(cls._init_attrs))) + assigned = set(re.findall( + r"self\.(_[a-z][a-z0-9_]*)\s*=", + inspect.getsource(cls.__init__))) + self.assertEqual( + assigned - defaulted, set(), + f"{cls.__name__}.__init__ sets attributes that " + f"_init_attrs() does not default") + + def test_from_archive_frees_handle_when_wrap_fails(self): + # The wrap raising means no Python object took ownership, so + # from_archive still holds the handle and has to free it. + archive = self._make_archive() # closes a Builder; keep it off the count + freed = self._instrument_frees() + real_wrap = Builder._wrap_native_handle + + def _boom(*args, **kwargs): + raise Error("wrap failed") + + Builder._wrap_native_handle = _boom + try: + with self.assertRaises(Error): + Builder.from_archive(archive) + finally: + Builder._wrap_native_handle = real_wrap + + self.assertEqual(len(freed), 1, + "from_archive leaked the handle when the wrap failed") if __name__ == '__main__': From 3f93e7e85c0c84e3d7801b5012607faf02cac913 Mon Sep 17 00:00:00 2001 From: tmathern <60901087+tmathern@users.noreply.github.com> Date: Thu, 16 Jul 2026 22:41:58 -0700 Subject: [PATCH 07/20] fix: Improve pointers handling --- src/c2pa/c2pa.py | 26 +++++++++++++---- tests/test_unit_tests.py | 60 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 79 insertions(+), 7 deletions(-) diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index dec8a8bc..6de29389 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -314,9 +314,8 @@ def _activate(self, handle, **extra_attrs): Only an uninitialized resource can be activated, so a handle can never be activated twice and a closed resource can never be reopened. - Any attribute the subclass's `_release()` reads must be passed in - `extra_attrs` when __init__ did not already set it, otherwise - `_release()` raises during cleanup. + `extra_attrs` overrides the defaults `_init_attrs()` already supplied, + so it is only for values that differ from them. `extra_attrs` cannot carry the lifecycle attributes themselves. `_owner_pid` in particular records the process that created the @@ -1630,6 +1629,7 @@ def __init__( raise def _init_attrs(self): + super()._init_attrs() self._has_signer = False self._signer_callback_cb = None @@ -2467,6 +2467,7 @@ def _init_from_context(self, context, format_or_path, raise def _init_attrs(self): + super()._init_attrs() self._own_stream = None # Tracks a file we opened ourselves and must close later. @@ -2484,14 +2485,14 @@ def _init_attrs(self): def _close_streams(self): """Close owned stream and backing file if present.""" - if getattr(self, '_own_stream', None): + if self._own_stream: try: self._own_stream.close() except Exception: logger.error("Failed to close Reader stream") finally: self._own_stream = None - if getattr(self, '_backing_file', None): + if self._backing_file: try: self._backing_file.close() except Exception: @@ -3015,11 +3016,18 @@ def __init__(self, signer_ptr: ctypes.POINTER(C2paSigner)): C2paError: If the signer pointer is invalid """ super().__init__() + self._init_attrs() if not signer_ptr: raise C2paError("Invalid signer pointer: pointer is null") - self._activate(signer_ptr, _callback_cb=None) + self._activate(signer_ptr) + + def _init_attrs(self): + super()._init_attrs() + # from_callback() replaces this with the real callback, which has to + # outlive the signer that calls it. + self._callback_cb = None def _release(self): """Release Signer-specific resources (callback reference).""" @@ -3251,9 +3259,15 @@ def _init_from_context(self, context, json_str): self._activate(new_ptr) def _init_attrs(self): + super()._init_attrs() self._context = None self._has_context_signer = False + def _release(self): + """Release the Builder's reference to its Context.""" + # The Context is not ours to close, only to stop pinning. + self._context = None + def set_no_embed(self): """Set the no-embed flag. diff --git a/tests/test_unit_tests.py b/tests/test_unit_tests.py index 180447b0..53156af0 100644 --- a/tests/test_unit_tests.py +++ b/tests/test_unit_tests.py @@ -7713,7 +7713,7 @@ def test_wrap_extra_attrs_override_defaults(self): def test_init_attrs_covers_what_init_sets(self): # Anything __init__ sets but _init_attrs() misses is absent on a # wrapped instance, which is the trap _init_attrs() exists to close. - for cls in (Builder, Context, Reader): + for cls in (Builder, Context, Reader, Signer): with self.subTest(cls=cls.__name__): defaulted = set(re.findall( r"self\.(_[a-z][a-z0-9_]*)\s*=", @@ -7726,6 +7726,64 @@ def test_init_attrs_covers_what_init_sets(self): f"{cls.__name__}.__init__ sets attributes that " f"_init_attrs() does not default") + def test_init_attrs_overrides_chain_to_super(self): + # A subclass of these would silently lose the parent's defaults if + # the chain were broken. + for cls in (Builder, Context, Reader, Signer): + with self.subTest(cls=cls.__name__): + self.assertIn( + "super()._init_attrs()", + inspect.getsource(cls._init_attrs), + f"{cls.__name__}._init_attrs() does not chain to super()") + + def test_wrap_raw_ffi_signer_pointer(self): + # Signer._release() reads _callback_cb, so a wrap that skipped the + # defaults would fail during cleanup rather than at the wrap. + freed = self._instrument_frees() + + signer = Signer._wrap_native_handle(0xABCD) + self.assertIsNone(signer._callback_cb) + self.assertEqual(signer._owner_pid, os.getpid()) + + # Cleanup swallows a failing _release(), so the error log is the only + # way to see one. + with self.assertNoLogs("c2pa", level="ERROR"): + signer.close() + + self.assertEqual(freed, [0xABCD]) + + def test_signer_release_clears_callback(self): + signer = self._ctx_make_callback_signer() + self.assertIsNotNone(signer._callback_cb) + + signer.close() + + self.assertIsNone(signer._callback_cb) + + def test_builder_release_clears_context(self): + context = Context() + self.addCleanup(context.close) + builder = Builder(self.test_manifest, context=context) + + builder.close() + + self.assertIsNone(builder._context, + "closed Builder still pins its Context") + # The Builder does not own the Context, so it must not close it. + self.assertTrue(context.is_valid) + + def test_reader_close_closes_backing_file(self): + # _close_streams reads the attrs _init_attrs() defaults, so this is + # the regression guard for reading them directly. + reader = Reader(DEFAULT_TEST_FILE) + backing_file = reader._backing_file + self.assertIsNotNone(backing_file) + + reader.close() + + self.assertTrue(backing_file.closed, "Reader left its file open") + self.assertIsNone(reader._backing_file) + def test_from_archive_frees_handle_when_wrap_fails(self): # The wrap raising means no Python object took ownership, so # from_archive still holds the handle and has to free it. From 97fe07e784709a3c1859b7c23e664bb1ee4671ec Mon Sep 17 00:00:00 2001 From: tmathern <60901087+tmathern@users.noreply.github.com> Date: Thu, 16 Jul 2026 22:57:57 -0700 Subject: [PATCH 08/20] fix: Improve pointers handling 2 --- src/c2pa/c2pa.py | 39 +++++++++--- tests/test_unit_tests.py | 128 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 159 insertions(+), 8 deletions(-) diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index 6de29389..da33b949 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -299,9 +299,26 @@ def _release(self): def _mark_consumed(self): """Mark as consumed by an FFI call that took ownership - of native resources e.g. pointers. This means we should not - call clean-up here anymore, and leave it to the new owner. + of native resources e.g. pointers. The new owner frees the handle, + but this class's own resources are still ours to let go of, and + marking the resource closed means _cleanup_resources will not do it + later. """ + if is_foreign_process(self): + self._handle = None + self._lifecycle_state = LifecycleState.CLOSED + return + + # Callers raise straight after consuming, so a failing _release() + # here would mask the error they are reporting. + try: + self._release() + except Exception: + logger.error( + "Failed to release %s resources", + type(self).__name__, + exc_info=True, + ) self._handle = None self._lifecycle_state = LifecycleState.CLOSED @@ -1986,6 +2003,11 @@ def close(self): if self._closed: return if is_foreign_process(self): + # Unlike ManagedResource, which leaves a child's copy active + # because the parent still owns the pointer, a Stream's callbacks + # are bound to the parent's objects. + # The child's copy can never be used, so mark it closed + # and let the parent free it. self._closed = True self._initialized = False return @@ -2501,7 +2523,13 @@ def _close_streams(self): self._backing_file = None def _release(self): - """Release Reader-specific resources (stream, backing file).""" + """Release Reader-specific resources (caches, stream, backing file). + + Every teardown path runs this, including _mark_consumed(), which + close() never gets a chance to follow. + """ + self._manifest_json_str_cache = None + self._manifest_data_cache = None self._close_streams() def _get_cached_manifest_data(self) -> Optional[dict]: @@ -2583,11 +2611,6 @@ def with_fragment(self, format: str, stream, return self - def close(self): - """Release the reader resources.""" - self._manifest_json_str_cache = None - self._manifest_data_cache = None - super().close() def json(self) -> str: """Get the manifest store as a JSON string. diff --git a/tests/test_unit_tests.py b/tests/test_unit_tests.py index 53156af0..fadc7bb7 100644 --- a/tests/test_unit_tests.py +++ b/tests/test_unit_tests.py @@ -7234,6 +7234,41 @@ def test_consumed_resource_frees_nothing_in_either_process(self): self.assertEqual(self.freed, []) + # Consuming a handle hands the native pointer to a new owner, + # but the Python-side resources are still ours to let go of. + + def test_mark_consumed_releases_python_resources(self): + res = self._ReleaseRecordingResource() + res._activate(0xF1) + + res._mark_consumed() + + self.assertEqual(res.release_calls, 1) + self.assertEqual(res._lifecycle_state, LifecycleState.CLOSED) + self.assertIsNone(res._handle) + self.assertEqual(self.freed, []) + + def test_mark_consumed_swallows_failing_release(self): + res = self._CallbackHoldingResource() + res._activate(0xF2) + + with self.assertLogs("c2pa", level="ERROR"): + res._mark_consumed() + + self.assertEqual(res._lifecycle_state, LifecycleState.CLOSED) + self.assertIsNone(res._handle) + + def test_mark_consumed_in_foreign_process_skips_release(self): + res = self._ReleaseRecordingResource() + res._activate(0xF3) + res._owner_pid = os.getpid() + 1 + + res._mark_consumed() + + self.assertEqual(res.release_calls, 0) + self.assertEqual(res._lifecycle_state, LifecycleState.CLOSED) + self.assertIsNone(res._handle) + def test_signer_init_rejects_null_pointer(self): with self.assertRaises(Error): Signer(None) @@ -7772,10 +7807,64 @@ def test_builder_release_clears_context(self): # The Builder does not own the Context, so it must not close it. self.assertTrue(context.is_valid) + def test_consumed_reader_closes_backing_file(self): + # A failed with_fragment consumes the reader. + # # Reader(path) opened the backing file itself, + # so nothing else will ever close it. + reader = Reader(DEFAULT_TEST_FILE) + backing_file = reader._backing_file + self.assertFalse(backing_file.closed) + + real_call = c2pa_module._lib.c2pa_reader_with_fragment + c2pa_module._lib.c2pa_reader_with_fragment = ( + lambda r, f, s, frag: None) + try: + with open(DEFAULT_TEST_FILE, "rb") as main, \ + open(DEFAULT_TEST_FILE, "rb") as frag: + with self.assertRaises(Error): + reader.with_fragment("image/jpeg", main, frag) + finally: + c2pa_module._lib.c2pa_reader_with_fragment = real_call + + self.assertTrue(backing_file.closed, + "consumed Reader leaked its backing file") + + def test_consumed_builder_releases_context(self): + context = Context() + self.addCleanup(context.close) + builder = Builder(self.test_manifest, context=context) + archive = self._make_archive() + + real_call = c2pa_module._lib.c2pa_builder_with_archive + c2pa_module._lib.c2pa_builder_with_archive = lambda b, s: None + try: + with self.assertRaises(Error): + builder.with_archive(archive) + finally: + c2pa_module._lib.c2pa_builder_with_archive = real_call + + self.assertIsNone(builder._context, + "consumed Builder still pins its Context") + self.assertTrue(context.is_valid) + + def test_context_takes_callback_before_consuming_signer(self): + # Consuming the signer releases its callback reference, + # so the Context has to take it first or the callback dies with the signer. + signer = self._ctx_make_callback_signer() + callback = signer._callback_cb + self.assertIsNotNone(callback) + + context = Context(signer=signer) + self.addCleanup(context.close) + + self.assertIs(context._signer_callback_cb, callback) + self.assertIsNone(signer._callback_cb) + def test_reader_close_closes_backing_file(self): # _close_streams reads the attrs _init_attrs() defaults, so this is # the regression guard for reading them directly. reader = Reader(DEFAULT_TEST_FILE) + reader.json() backing_file = reader._backing_file self.assertIsNotNone(backing_file) @@ -7783,6 +7872,45 @@ def test_reader_close_closes_backing_file(self): self.assertTrue(backing_file.closed, "Reader left its file open") self.assertIsNone(reader._backing_file) + self.assertIsNone(reader._manifest_json_str_cache) + self.assertIsNone(reader._manifest_data_cache) + + def test_consumed_reader_clears_caches(self): + # Consuming marks the reader closed, so close() will not run later. + # Anything cleanup owes the object has to happen at consume time. + reader = Reader(DEFAULT_TEST_FILE) + reader.json() + self.assertIsNotNone(reader._manifest_json_str_cache) + + real_call = c2pa_module._lib.c2pa_reader_with_fragment + c2pa_module._lib.c2pa_reader_with_fragment = ( + lambda r, f, s, frag: None) + try: + with open(DEFAULT_TEST_FILE, "rb") as main, \ + open(DEFAULT_TEST_FILE, "rb") as frag: + with self.assertRaises(Error): + reader.with_fragment("image/jpeg", main, frag) + finally: + c2pa_module._lib.c2pa_reader_with_fragment = real_call + + self.assertIsNone(reader._manifest_json_str_cache, + "consumed Reader kept its manifest cache") + self.assertIsNone(reader._manifest_data_cache) + + def test_reader_del_clears_caches(self): + # __del__ goes through _cleanup_resources, not close(), so cache + # clearing has to live somewhere both paths reach. + reader = Reader(DEFAULT_TEST_FILE) + reader.json() + self.assertIsNotNone(reader._manifest_json_str_cache) + + # __del__ runs _cleanup_resources directly, so drive that rather than + # dropping the reference: the assertions need the object afterwards. + reader._cleanup_resources() + + self.assertIsNone(reader._manifest_json_str_cache, + "cleanup left the manifest cache alive") + self.assertIsNone(reader._manifest_data_cache) def test_from_archive_frees_handle_when_wrap_fails(self): # The wrap raising means no Python object took ownership, so From d95a9d7f7f5c594482b2701577acea0f86c0a288 Mon Sep 17 00:00:00 2001 From: tmathern <60901087+tmathern@users.noreply.github.com> Date: Thu, 16 Jul 2026 23:17:21 -0700 Subject: [PATCH 09/20] fix: The refactor --- src/c2pa/c2pa.py | 42 ++++------------------ tests/test_unit_tests.py | 77 +++++++++++----------------------------- 2 files changed, 28 insertions(+), 91 deletions(-) diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index da33b949..26a6913f 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -224,12 +224,6 @@ class LifecycleState(enum.IntEnum): CLOSED = 2 -# Attributes that make up the lifecycle state itself, which _activate sets -# from its own arguments and must never accept from a caller's extra_attrs. -_RESERVED_ACTIVATION_ATTRS = frozenset( - {'_handle', '_lifecycle_state', '_owner_pid'}) - - class ManagedResource: """Base class for objects that hold a native (C FFI) resource. This is an internal base class that provides lifecycle management @@ -323,30 +317,19 @@ def _mark_consumed(self): self._handle = None self._lifecycle_state = LifecycleState.CLOSED - def _activate(self, handle, **extra_attrs): - """Attach a native handle (and any extra instance attrs) to self - and mark it active. Attaching activates it. + def _activate(self, handle): + """Attach a native handle to self and mark it active. Ownership of `handle` transfers here: this object frees it on close. Only an uninitialized resource can be activated, so a handle can never be activated twice and a closed resource can never be reopened. - `extra_attrs` overrides the defaults `_init_attrs()` already supplied, - so it is only for values that differ from them. - - `extra_attrs` cannot carry the lifecycle attributes themselves. - `_owner_pid` in particular records the process that created the - handle, and overwriting it would let a forked child free a pointer - its parent still owns. - Args: handle: Non-null native pointer to take ownership of - **extra_attrs: Instance attributes to set before activating Raises: C2paError: If the handle is null, - the resource is not uninitialized, - or extra_attrs names a lifecycle attribute + or the resource is not uninitialized """ name = type(self).__name__ # A rejected activation must leave the object as it was. @@ -356,14 +339,7 @@ def _activate(self, handle, **extra_attrs): raise C2paError( f"{name}: already activated " f"({self._lifecycle_state.name})") - reserved = _RESERVED_ACTIVATION_ATTRS.intersection(extra_attrs) - if reserved: - raise C2paError( - f"{name}: cannot set lifecycle attributes via extra_attrs: " - f"{', '.join(sorted(reserved))}") - for attr, value in extra_attrs.items(): - setattr(self, attr, value) self._handle = handle self._lifecycle_state = LifecycleState.ACTIVE @@ -395,31 +371,27 @@ def _swap_handle(self, new_handle): self._handle = new_handle @classmethod - def _wrap_native_handle(cls, handle, **extra_attrs): + def _wrap_native_handle(cls, handle): """Build a brand-new instance around an already-valid, already-owned native handle, bypassing __init__ entirely. __init__ is bypassed, so `_init_attrs()` supplies the class's own - defaults; `extra_attrs` is only for overriding them. The lifecycle - attributes are off limits there, and the instance is stamped with the - creating process either way. + defaults and the instance is stamped with the creating process. Ownership of `handle` only transfers once this returns. If it raises, the caller still owns the pointer and must free it. Args: handle: Non-null native pointer to take ownership of - **extra_attrs: Instance attributes overriding the defaults Raises: - C2paError: If the handle is null, or extra_attrs names a - lifecycle attribute + C2paError: If the handle is null """ obj = object.__new__(cls) # Stamps _owner_pid, which the fork guard relies on. ManagedResource.__init__(obj) obj._init_attrs() - obj._activate(handle, **extra_attrs) + obj._activate(handle) return obj def _cleanup_resources(self): diff --git a/tests/test_unit_tests.py b/tests/test_unit_tests.py index fadc7bb7..4824e377 100644 --- a/tests/test_unit_tests.py +++ b/tests/test_unit_tests.py @@ -6942,8 +6942,8 @@ class _FakeHandleResource(ManagedResource): """Concrete subclass with no resources of its own.""" class _CallbackHoldingResource(ManagedResource): - """Mimics Signer: its _release() reads an attribute that must have - been supplied by __init__ or by _activate's extra_attrs.""" + """Mimics Signer: its _release() reads an attribute that _init_attrs() + is responsible for defaulting.""" def _release(self): if self._callback_cb: @@ -7053,10 +7053,11 @@ def test_activate_does_not_mutate_on_rejection(self): res._activate(0x5555) with self.assertRaises(Error): - res._activate(0x6666, _extra='should not be set') + res._activate(0x6666) - self.assertFalse(hasattr(res, '_extra'), - "rejected activation still set extra_attrs") + self.assertEqual(res._handle, 0x5555, + "rejected activation replaced the handle") + self.assertEqual(res._lifecycle_state, LifecycleState.ACTIVE) # _swap_handle @@ -7099,27 +7100,31 @@ def test_swap_handle_rejects_null_replacement(self): # _wrap_native_handle - def test_wrap_native_handle_sets_extra_attrs_and_bypasses_init(self): + def test_wrap_native_handle_bypasses_init(self): seen = [] class Probe(ManagedResource): def __init__(self): raise AssertionError("__init__ must be bypassed") + def _init_attrs(self): + super()._init_attrs() + self._tag = 'from _init_attrs' + def _release(self): seen.append(self._tag) - obj = Probe._wrap_native_handle(0xC0DE, _tag='from extra_attrs') + obj = Probe._wrap_native_handle(0xC0DE) - self.assertEqual(obj._tag, 'from extra_attrs') + self.assertEqual(obj._tag, 'from _init_attrs') self.assertEqual(obj._lifecycle_state, LifecycleState.ACTIVE) self.assertTrue(obj.is_valid) # ManagedResource.__init__ still ran, so fork-safety is intact. self.assertTrue(hasattr(obj, '_owner_pid')) obj.close() - self.assertEqual(seen, ['from extra_attrs'], - "_release() could not see the extra attrs") + self.assertEqual(seen, ['from _init_attrs'], + "_release() could not see the class's own attrs") def test_wrap_native_handle_rejects_null(self): with self.assertRaises(Error): @@ -7152,30 +7157,6 @@ def test_every_construction_path_records_owner_pid(self): wrapped._swap_handle(0xA3) self.assertEqual(wrapped._owner_pid, pid) - def test_activate_rejects_reserved_extra_attrs(self): - for attr, value in ( - ('_owner_pid', os.getpid() + 1), - ('_handle', 0xDEAD), - ('_lifecycle_state', LifecycleState.CLOSED), - ): - with self.subTest(attr=attr): - res = self._FakeHandleResource() - stamp = res._owner_pid - - with self.assertRaises(Error) as ctx: - res._activate(0xB1, **{attr: value}) - - self.assertIn(attr, str(ctx.exception)) - self.assertEqual(res._owner_pid, stamp) - self.assertEqual( - res._lifecycle_state, LifecycleState.UNINITIALIZED) - self.assertIsNone(res._handle) - - def test_wrap_native_handle_rejects_reserved_extra_attrs(self): - with self.assertRaises(Error): - self._FakeHandleResource._wrap_native_handle( - 0xB2, _owner_pid=os.getpid() + 1) - def test_foreign_child_skips_free_for_wrapped_and_swapped(self): wrapped = self._FakeHandleResource._wrap_native_handle(0xC1) wrapped._owner_pid = os.getpid() + 1 @@ -7666,9 +7647,7 @@ def _raw_builder_handle(self): return handle def test_wrap_raw_ffi_builder_pointer(self): - builder = Builder._wrap_native_handle( - self._raw_builder_handle(), - _context=None, _has_context_signer=False) + builder = Builder._wrap_native_handle(self._raw_builder_handle()) self.assertTrue(builder.is_valid) self.assertEqual(builder._lifecycle_state, LifecycleState.ACTIVE) @@ -7698,8 +7677,7 @@ def test_wrap_raw_ffi_context_pointer(self): handle = c2pa_module._lib.c2pa_context_new() self.assertTrue(handle) - context = Context._wrap_native_handle( - handle, _has_signer=False, _signer_callback_cb=None) + context = Context._wrap_native_handle(handle) try: self.assertTrue(context.is_valid) self.assertEqual(context._owner_pid, os.getpid()) @@ -7714,8 +7692,7 @@ def test_wrapped_raw_pointer_freed_exactly_once(self): handle = self._raw_builder_handle() freed = self._instrument_frees() - builder = Builder._wrap_native_handle( - handle, _context=None, _has_context_signer=False) + builder = Builder._wrap_native_handle(handle) builder.close() builder.close() del builder @@ -7724,27 +7701,15 @@ def test_wrapped_raw_pointer_freed_exactly_once(self): self.assertEqual(self._free_count(freed, handle), 1, "wrapped handle not freed exactly once") - def test_wrap_supplies_defaults_without_extra_attrs(self): - # _init_attrs() runs on the wrap path, so a caller that passes no - # extra_attrs still gets an instance the rest of the class can read. + def test_wrap_supplies_defaults(self): + # _init_attrs() runs on the wrap path, so an instance built around a + # raw handle still has everything the rest of the class reads. builder = Builder._wrap_native_handle(self._raw_builder_handle()) self.addCleanup(builder.close) self.assertIsNone(builder._context) self.assertFalse(builder._has_context_signer) - def test_wrap_extra_attrs_override_defaults(self): - context = Context() - self.addCleanup(context.close) - - builder = Builder._wrap_native_handle( - self._raw_builder_handle(), _context=context) - self.addCleanup(builder.close) - - self.assertIs(builder._context, context) - # Untouched by the override, so still the default. - self.assertFalse(builder._has_context_signer) - def test_init_attrs_covers_what_init_sets(self): # Anything __init__ sets but _init_attrs() misses is absent on a # wrapped instance, which is the trap _init_attrs() exists to close. From 0309a921b3df9d76eb7e7bfd97f051031a675bc3 Mon Sep 17 00:00:00 2001 From: tmathern <60901087+tmathern@users.noreply.github.com> Date: Fri, 17 Jul 2026 19:19:45 -0700 Subject: [PATCH 10/20] fix: Improve pointers handling 4 --- pr294-finishing-plan.md | 121 +++++++++++++++++++++++++++++++++++++++ src/c2pa/c2pa.py | 49 ++++++++++------ tests/test_unit_tests.py | 56 +++++++++++++++++- 3 files changed, 207 insertions(+), 19 deletions(-) create mode 100644 pr294-finishing-plan.md diff --git a/pr294-finishing-plan.md b/pr294-finishing-plan.md new file mode 100644 index 00000000..3579c47c --- /dev/null +++ b/pr294-finishing-plan.md @@ -0,0 +1,121 @@ +# PR #294 — Finishing plan: native-handle lifecycle extension surface + +**Goal:** take #294 from WIP to mergeable and reviewer-proof. The code is +mechanically sound; the work left is *committing to the extension contract* and +writing it down where it can be enforced. No functional rewrite required. + +--- + +## Decision to record first: the extension model + +Downstream libraries should be able to wrap their own FFI pointers in the +managed lifecycle, **without** the project promising a stable public API. + +Chosen: **single-underscore, subclass-facing (protected).** Keep the names as +they are (`_activate`, `_swap_handle`, `_mark_consumed`, `_wrap_native_handle`, +`_init_attrs`). + +Why this level and not the alternatives — state this in the PR so it isn't +re-litigated in review: + +- **Not public (no underscore).** A bare name is an implicit stability promise. + The seam is still evolving; we don't want to guarantee it. +- **Not dunder (`__name`).** Double underscore triggers per-class name + mangling, which breaks subclassing — the exact capability this PR exists to + enable. It's the one "more private" option that defeats the purpose. +- **Single underscore is the idiom for "reachable, unsupported, subclass- + friendly."** Python privacy is convention, not enforcement: a downstream lib + *can* call these, and that's intended. The underscore communicates "no + stability guarantee," nothing more. + +Consequence that drives the rest of the plan: because the underscore does not +enforce anything, **the docstrings are the contract.** They must carry the +invariants as if the methods were public, because for the people who reach them +they effectively are. + +--- + +## Work items + +### 1. Make the docstrings the contract (primary work) +Each exposed primitive states its ownership transfer explicitly: +- `_activate(handle)` — takes ownership; frees on close; only from + `UNINITIALIZED`; rejects null and double-activation. +- `_swap_handle(new_handle)` — **the caller guarantees the callee already + consumed and freed the old pointer.** Requires `ACTIVE`; rejects null. +- `_mark_consumed()` — the native pointer is now owned elsewhere; this releases + only *Python-side* resources and marks `CLOSED` without freeing the pointer. +- `_wrap_native_handle(handle)` — ownership transfers **only on successful + return**; if it raises, the caller still owns the pointer and must free it. + +### 2. Nail the `_wrap_native_handle` initialization invariant +It bypasses `__init__` entirely and runs only `_init_attrs()`. Add one explicit +line to the docstring: + +> Everything an instance needs besides the native handle must be set in +> `_init_attrs()`, not `__init__` — `_wrap_native_handle` never runs `__init__`. + +Live proof this matters: `Reader.__init__` sets `self._context = context` +*after* `_init_attrs()`, so a Reader built via `_wrap_native_handle` would get +`_context = None`. Fine for `Builder.from_archive` (no context by design), but a +footgun for any external extender who puts setup in `__init__`. + +### 3. Legibility on the consume-and-return null path +In `with_fragment` and `with_archive`, the null branch relies on +`_check_ffi_operation_result` raising *before* control reaches `_swap_handle`. +It is safe today, but reads as "mark consumed, then a check that happens to +raise." Make the intent explicit: + +```python +new_ptr = _lib.c2pa_..._with_...(self._handle, ...) +if not new_ptr: + # callee consumed the old handle and returned nothing to own + self._mark_consumed() + _check_ffi_operation_result(new_ptr, "...") # raises here +self._swap_handle(new_ptr) +``` + +Moving the check inside the `if not new_ptr:` block makes the control flow say +what it means. No behavior change. + +### 4. `super().__init__()` audit (cheap) +Every `_activate` caller depends on `self._lifecycle_state` already existing +(set by `ManagedResource.__init__`). Context / Reader / Builder / Signer visibly +call `super().__init__()`. Confirm `Settings.__init__` does too. If the existing +tests pass, it already does — this is a five-second eyeball, not a suspected bug. + +### 5. Tests: cover the *external-extender* path +The added lifecycle / integration / cross-thread tests cover the built-in types. +Add the case the PR actually unlocks: +- A minimal subclass that owns a raw handle and reaches the lifecycle via + `_wrap_native_handle` + `_init_attrs`, asserting the instance is fully built + (no missing attributes) and frees exactly once. +- The same wrapped instance under the fork guard: a foreign-process teardown + skips the native free (owner-PID stamped through the wrap path). + +--- + +## Explicit non-goals (pre-empt scope-creep review comments) +- **Not** renaming anything to public. The access level is the decision, not an + oversight. +- **Not** adding fork guards to *operation* paths (`with_fragment` etc.). The + PID guard's contract is teardown-safety in a forked child, not operation- + safety. Operating on a handle in a forked child is caller error and out of + scope. + +--- + +## Review-defense notes (the "why", pre-answered) + +- **Why underscore, not public?** See the extension-model decision above: + reachable but unsupported is exactly the intent. +- **Why does `_mark_consumed()` now run `_release()`?** Previously the consume + paths leaked Python-side resources (callback refs, streams, caches) until GC. + Releasing on consume is a fix, not a regression. It cannot double-release: + `_cleanup_resources` skips when the state is already `CLOSED`. +- **Why is the signer callback copied onto the Context before the signer is + consumed?** The native signer holds a pointer to the Python callback. + `_mark_consumed() -> _release()` clears the signer's callback ref, so the + Context must capture its own reference *first*, or the first invocation of the + consumed signer's callback would be a use-after-free. The ordering is the + whole reason it's safe; the inline comment explaining it must stay. diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index 54e3ac38..277082b6 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -292,11 +292,15 @@ def _release(self): """ def _mark_consumed(self): - """Mark as consumed by an FFI call that took ownership - of native resources e.g. pointers. The new owner frees the handle, - but this class's own resources are still ours to let go of, and - marking the resource closed means _cleanup_resources will not do it - later. + """Mark as consumed by an FFI call that took ownership of the native + handle. + + Ownership contract: the native pointer is now owned elsewhere, so this + does not free it. It releases only Python-side resources (via + `_release()`: callback refs, streams, caches) and marks the resource + closed. Marking it closed stops `_cleanup_resources` from freeing the + now foreign-owned pointer later; releasing here means those Python + resources are not stranded until garbage collection. """ if is_foreign_process(self): self._handle = None @@ -347,13 +351,17 @@ def _swap_handle(self, new_handle): """Replace the handle after an FFI call consumed the old one and returned a replacement. - The old pointer is already owned and freed by the callee, - so it is not freed here. + Ownership contract: the caller guarantees the callee has already + consumed and freed the old pointer. This method never frees the old + pointer; it only rebinds `self._handle` to the replacement. Calling it + when the old pointer was not consumed leaks that pointer. 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. + Args: new_handle: Non-null native pointer returned by the FFI call @@ -378,8 +386,13 @@ def _wrap_native_handle(cls, handle): __init__ is bypassed, so `_init_attrs()` supplies the class's own defaults and the instance is stamped with the creating process. - Ownership of `handle` only transfers once this returns. If it raises, - the caller still owns the pointer and must free it. + Everything an instance needs besides the native handle must be set in + `_init_attrs()`, not `__init__` — `_wrap_native_handle` never runs + `__init__`. An attribute a subclass sets only in `__init__` will be + missing (or left at its `_init_attrs` default) on a wrapped instance. + + Ownership of `handle` transfers only on successful return. If this + raises, the caller still owns the pointer and must free it. Args: handle: Non-null native pointer to take ownership of @@ -2565,13 +2578,14 @@ def with_fragment(self, format: str, stream, ) # c2pa_reader_with_fragment consumed the old handle. A null return - # leaves no replacement to take ownership of. + # leaves no replacement to take ownership of: mark consumed, then + # raise. _swap_handle is only reached when new_ptr is non-null. if not new_ptr: self._mark_consumed() - _check_ffi_operation_result(new_ptr, - Reader._ERROR_MESSAGES[ - 'fragment_error' - ].format("Unknown error")) + _check_ffi_operation_result(new_ptr, + Reader._ERROR_MESSAGES[ + 'fragment_error' + ].format("Unknown error")) self._swap_handle(new_ptr) # Invalidate caches: processing a new BMFF fragment updates the native @@ -3543,11 +3557,12 @@ def with_archive(self, stream: Any) -> 'Builder': f"Error loading archive: {e}" ) # c2pa_builder_with_archive consumed the old handle. A null return - # leaves no replacement to take ownership of. + # leaves no replacement to take ownership of: mark consumed, then + # raise. _swap_handle is only reached when new_ptr is non-null. if not new_ptr: self._mark_consumed() - _check_ffi_operation_result( - new_ptr, "Failed to load archive into builder") + _check_ffi_operation_result( + new_ptr, "Failed to load archive into builder") self._swap_handle(new_ptr) return self diff --git a/tests/test_unit_tests.py b/tests/test_unit_tests.py index 43bbcb05..fdea7041 100644 --- a/tests/test_unit_tests.py +++ b/tests/test_unit_tests.py @@ -6960,6 +6960,24 @@ def __init__(self): def _release(self): self.release_calls += 1 + class _ExtenderResource(ManagedResource): + """A downstream extender that owns a raw handle and wraps it via + _wrap_native_handle. It carries several attributes of its own, all + defaulted in _init_attrs (not __init__), and _release reads them, so a + missing attribute would surface as an AttributeError on teardown. + """ + + def _init_attrs(self): + super()._init_attrs() + self.label = "extender" + self.buffer = [] + self.released = False + + def _release(self): + # Reads attributes _init_attrs is responsible for defaulting. + self.buffer.append(self.label) + self.released = True + def setUp(self): self.data_dir = FIXTURES_DIR self.freed = [] @@ -7251,6 +7269,40 @@ def test_mark_consumed_in_foreign_process_skips_release(self): self.assertEqual(res._lifecycle_state, LifecycleState.CLOSED) self.assertIsNone(res._handle) + def test_extender_wraps_handle_fully_built(self): + obj = self._ExtenderResource._wrap_native_handle(0xE0) + + # Every attribute _init_attrs defaults is present, + # even thoug __init__ never ran. + self.assertEqual(obj.label, "extender") + self.assertEqual(obj.buffer, []) + self.assertFalse(obj.released) + self.assertTrue(obj.is_valid) + self.assertEqual(obj._owner_pid, os.getpid()) + + # _release reads those attributes, so a missing one would raise here. + obj.close() + obj.close() + + self.assertTrue(obj.released) + self.assertEqual(obj.buffer, ["extender"]) + self.assertEqual(self.freed, [0xE0], "wrapped handle freed once") + + def test_extender_foreign_teardown_skips_native_free(self): + obj = self._ExtenderResource._wrap_native_handle(0xE1) + # Stamp a foreign owner: + # teardown runs in a process that did not create the handle, + # so it must not free the pointer or release. + obj._owner_pid = os.getpid() + 1 + + obj.close() + + self.assertEqual(self.freed, [], + "forked child freed a handle its parent still owns") + self.assertFalse(obj.released, "foreign teardown ran _release") + self.assertEqual(obj._lifecycle_state, LifecycleState.ACTIVE) + self.assertEqual(obj._handle, 0xE1) + def test_signer_init_rejects_null_pointer(self): with self.assertRaises(Error): Signer(None) @@ -7272,8 +7324,8 @@ def test_builder_from_archive_wraps_handle(self): def test_context_build_failure_consumes_signer(self): # c2pa_context_builder_set_signer takes ownership of the signer # pointer immediately, so a later build failure must still leave the - # Signer consumed. Otherwise it holds a pointer the native side has - # already freed. + # Signer consumed. + # Otherwise it holds a pointer the native side has already freed. self._use_real_frees() signer = self._make_signer() real_build = c2pa_module._lib.c2pa_context_builder_build From d6ebd6fade918ca91eedb5297139abe442d704ab Mon Sep 17 00:00:00 2001 From: tmathern <60901087+tmathern@users.noreply.github.com> Date: Fri, 17 Jul 2026 19:47:17 -0700 Subject: [PATCH 11/20] fix: Improve pointers handling 5 --- examples/read.py | 2 +- examples/sign.py | 2 +- examples/sign_info.py | 4 +- src/c2pa/c2pa.py | 63 ++++++++++++++++++----------- tests/perf/scenarios.py | 33 ++++++++++++++++ tests/test_unit_tests.py | 66 +++++++++++++++++++++++++++---- tests/test_unit_tests_threaded.py | 13 ++++-- 7 files changed, 146 insertions(+), 37 deletions(-) diff --git a/examples/read.py b/examples/read.py index e4b718a9..b2ef9fc6 100644 --- a/examples/read.py +++ b/examples/read.py @@ -36,7 +36,7 @@ def read_c2pa_data(media_path: str): # All objects using this context will have trust configured. with c2pa.Context(settings) as context: with c2pa.Reader(media_path, context=context) as reader: - print(reader.detailed_json()) + print(reader.crjson()) except Exception as e: print(f"Error reading C2PA data from {media_path}: {e}") sys.exit(1) diff --git a/examples/sign.py b/examples/sign.py index e6c14859..09133b35 100644 --- a/examples/sign.py +++ b/examples/sign.py @@ -110,6 +110,6 @@ def callback_signer_es256(data: bytes) -> bytes: # The validation state will depend on loaded trust settings. # Without loaded trust settings, # the manifest validation_state will be "Invalid". - print(reader.json()) + print(reader.crjson()) print("\nExample completed successfully!") diff --git a/examples/sign_info.py b/examples/sign_info.py index 6b256647..3735ad0b 100644 --- a/examples/sign_info.py +++ b/examples/sign_info.py @@ -40,7 +40,7 @@ print("\nReading existing C2PA metadata:") with open(fixtures_dir + "C.jpg", "rb") as file: with c2pa.Reader("image/jpeg", file) as reader: - print(reader.json()) + print(reader.crjson()) # Create a signer from certificate and key files with open(fixtures_dir + "es256_certs.pem", "rb") as cert_file: @@ -103,7 +103,7 @@ # The validation state will depend on loaded trust settings. # Without loaded trust settings, # the manifest validation_state will be "Invalid". - print(reader.json()) + print(reader.crjson()) print("\nExample completed successfully!") diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index 277082b6..b0a24206 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -291,6 +291,23 @@ def _release(self): The default implementation does nothing. """ + def _safe_release(self): + """Run _release(), logging (never raising) if it fails. + + Shared by _mark_consumed and _cleanup_resources so the try/except/log + wrapper lives in one place. Each caller keeps its own lifecycle-state + transition, because the two paths deliberately order the CLOSED flip + differently relative to this call (see each call site). + """ + try: + self._release() + except Exception: + logger.error( + "Failed to release %s resources", + type(self).__name__, + exc_info=True, + ) + def _mark_consumed(self): """Mark as consumed by an FFI call that took ownership of the native handle. @@ -309,14 +326,7 @@ def _mark_consumed(self): # Callers raise straight after consuming, so a failing _release() # here would mask the error they are reporting. - try: - self._release() - except Exception: - logger.error( - "Failed to release %s resources", - type(self).__name__, - exc_info=True, - ) + self._safe_release() self._handle = None self._lifecycle_state = LifecycleState.CLOSED @@ -411,6 +421,15 @@ 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 return if ( hasattr(self, '_lifecycle_state') @@ -420,14 +439,7 @@ def _cleanup_resources(self): # A failing _release() must not skip the free below: # that would strand the native handle on an object already # marked CLOSED, making it unreachable and unfreeable. - try: - self._release() - except Exception: - logger.error( - "Failed to release %s resources", - type(self).__name__, - exc_info=True, - ) + self._safe_release() if hasattr(self, '_handle') and self._handle: try: ManagedResource._free_native_ptr(self._handle) @@ -2570,12 +2582,19 @@ def with_fragment(self, format: str, stream, ) with Stream(stream) as main_obj, Stream(fragment_stream) as frag_obj: - new_ptr = _lib.c2pa_reader_with_fragment( - self._handle, - format_bytes, - main_obj._stream, - frag_obj._stream, - ) + try: + new_ptr = _lib.c2pa_reader_with_fragment( + self._handle, + format_bytes, + main_obj._stream, + frag_obj._stream, + ) + except Exception as e: + # The callee consumes the old handle before it can fail, so + # treat it as consumed and let go of resources. + self._mark_consumed() + raise C2paError( + Reader._ERROR_MESSAGES['fragment_error'].format(e)) # c2pa_reader_with_fragment consumed the old handle. A null return # leaves no replacement to take ownership of: mark consumed, then diff --git a/tests/perf/scenarios.py b/tests/perf/scenarios.py index b4b7e46c..25c688a0 100644 --- a/tests/perf/scenarios.py +++ b/tests/perf/scenarios.py @@ -957,6 +957,37 @@ def scenario_fork_parent_frees_after_fork(iterations: int = 100) -> None: r.close() +def scenario_fork_child_closes_then_parent_frees(iterations: int = 100) -> None: + """Fork safety benchmark scenario: + 20 Readers created, fork, the CHILD closes all 20 inherited Readers (and + runs GC so any __del__ fires) before exiting, then the parent closes its + own 20 copies. Exercises the child-side path where _cleanup_resources marks + the child's copy CLOSED and nulls the handle while skipping the native free + — the branch scenario_fork_parent_frees_after_fork never hits (its child + does nothing). Two invariants: the child must exit cleanly (no deadlock via + the 5 s alarm, no crash from a child-side double-free), and the parent must + still free all 20 (leaked_bytes stays at baseline — the child's state + mutation does not suppress the parent's frees, since the copies are + independent post-fork). + """ + if not hasattr(os, "fork"): + return + for _ in _iterate(iterations): + readers = [] + for _ in range(20): + with open(SIGNED_JPEG, "rb") as f: + readers.append(Reader("image/jpeg", f)) + + def _child(): + for r in readers: + r.close() # foreign teardown: mark closed, skip native free + gc.collect() + + _fork_wait(_child) + for r in readers: + r.close() # parent's own copies: real free + + def scenario_fork_child_sys_exit(iterations: int = 100) -> None: """Fork safety benchmark scenario: Child calls sys.exit(0), full Python shutdown: atexit, finalizers, GC. @@ -1191,6 +1222,8 @@ def scenario_fork_stream_cleanup(iterations: int = 100) -> None: "fork_thread_local_orphan": scenario_fork_thread_local_orphan, "fork_gc_cycle": scenario_fork_gc_cycle, "fork_parent_frees_after_fork": scenario_fork_parent_frees_after_fork, + "fork_child_closes_then_parent_frees": + scenario_fork_child_closes_then_parent_frees, "fork_child_sys_exit": scenario_fork_child_sys_exit, "fork_stream_cleanup": scenario_fork_stream_cleanup, "fork_swap_cleanup": scenario_fork_swap_cleanup, diff --git a/tests/test_unit_tests.py b/tests/test_unit_tests.py index fdea7041..466d5a81 100644 --- a/tests/test_unit_tests.py +++ b/tests/test_unit_tests.py @@ -7189,10 +7189,19 @@ def test_foreign_child_skips_free_for_wrapped_and_swapped(self): self.assertEqual(self.freed, [], "forked child freed a pointer its parent still owns") - # The parent still owns these handles, - # so the child must not mark the objects dead either. - self.assertEqual(wrapped._lifecycle_state, LifecycleState.ACTIVE) - self.assertEqual(swapped._handle, 0xC3) + # The child must not free a pointer the parent still owns. + # The child does mark its own copies closed and nulls their handles, + # which is safe (the parent holds a separate copy) and + # stops the child from reusing a parent-owned handle. + self.assertEqual(wrapped._lifecycle_state, LifecycleState.CLOSED) + self.assertIsNone(wrapped._handle) + self.assertEqual(swapped._lifecycle_state, LifecycleState.CLOSED) + self.assertIsNone(swapped._handle) + + # A second foreign teardown is a no-op: still nothing freed. + wrapped.close() + swapped.close() + self.assertEqual(self.freed, []) def test_owning_process_frees_wrapped_and_swapped_exactly_once(self): wrapped = self._FakeHandleResource._wrap_native_handle(0xC4) @@ -7292,7 +7301,8 @@ def test_extender_foreign_teardown_skips_native_free(self): obj = self._ExtenderResource._wrap_native_handle(0xE1) # Stamp a foreign owner: # teardown runs in a process that did not create the handle, - # so it must not free the pointer or release. + # so it must not free the pointer or run _release (which could touch + # native streams and deadlock after a multithreaded fork). obj._owner_pid = os.getpid() + 1 obj.close() @@ -7300,8 +7310,18 @@ def test_extender_foreign_teardown_skips_native_free(self): self.assertEqual(self.freed, [], "forked child freed a handle its parent still owns") self.assertFalse(obj.released, "foreign teardown ran _release") - self.assertEqual(obj._lifecycle_state, LifecycleState.ACTIVE) - self.assertEqual(obj._handle, 0xE1) + # The child marks its own copy closed and nulls the handle: safe (the + # parent holds a separate copy) and it stops the child reusing a + # parent-owned handle. + self.assertEqual(obj._lifecycle_state, LifecycleState.CLOSED) + self.assertIsNone(obj._handle) + + # A second foreign teardown stays a no-op, and any operation on the + # now-closed child copy fails loudly instead of reaching native code. + obj.close() + self.assertEqual(self.freed, []) + with self.assertRaises(Error): + obj._ensure_valid_state() def test_signer_init_rejects_null_pointer(self): with self.assertRaises(Error): @@ -7687,6 +7707,38 @@ def test_reader_with_fragment_null_return_consumes_self(self): self.assertEqual(self._free_count(freed, consumed_handle), 0, "close() freed a handle the FFI already consumed") + def test_reader_with_fragment_ffi_raise_consumes_self(self): + # If the ctypes call itself raises (not a null return), the callee has + # already consumed the old handle, so with_fragment must mark self + # consumed rather than leave a dangling pointer that close() would + # double-free. + init_path = os.path.join(FIXTURES_DIR, "dashinit.mp4") + fragment_path = os.path.join(FIXTURES_DIR, "dash1.m4s") + with open(init_path, "rb") as init: + reader = Reader("video/mp4", init) + consumed_handle = reader._handle + + def _raise(*_args): + raise RuntimeError("boom") + + real_call = c2pa_module._lib.c2pa_reader_with_fragment + c2pa_module._lib.c2pa_reader_with_fragment = _raise + try: + with open(init_path, "rb") as init, \ + open(fragment_path, "rb") as frag: + with self.assertRaises(Error): + reader.with_fragment("video/mp4", init, frag) + finally: + c2pa_module._lib.c2pa_reader_with_fragment = real_call + + self.assertIsNone(reader._handle) + self.assertEqual(reader._lifecycle_state, LifecycleState.CLOSED) + + freed = self._instrument_frees() + reader.close() + self.assertEqual(self._free_count(freed, consumed_handle), 0, + "close() freed a handle the FFI already consumed") + # 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 # only tests that drive the primitive as the generic entry point it is. diff --git a/tests/test_unit_tests_threaded.py b/tests/test_unit_tests_threaded.py index 44609adc..540dfc82 100644 --- a/tests/test_unit_tests_threaded.py +++ b/tests/test_unit_tests_threaded.py @@ -96,12 +96,17 @@ def test_no_stamp_calls_free(self): obj._cleanup_resources() mock_free.assert_called_once() - def test_foreign_pid_leaves_state_unchanged(self): - """Guard returns early; lifecycle state stays ACTIVE (not CLOSED).""" + def test_foreign_pid_marks_closed_without_free(self): + """A foreign child skips the native free but marks its own copy closed + and nulls the handle, so the child cannot reuse a parent-owned handle. + The parent holds a separate copy and frees it independently. + """ obj = _make_resource(pid_offset=1) - with patch('c2pa.c2pa._lib'): + with patch.object(ManagedResource, '_free_native_ptr') as mock_free: obj._cleanup_resources() - self.assertEqual(obj._lifecycle_state, LifecycleState.ACTIVE) + mock_free.assert_not_called() + self.assertEqual(obj._lifecycle_state, LifecycleState.CLOSED) + self.assertIsNone(obj._handle) def test_double_cleanup_is_idempotent(self): """Second call is a no-op after successful first cleanup.""" From fad717bf80502312c6017b9843ab8a4bd390aeb3 Mon Sep 17 00:00:00 2001 From: tmathern <60901087+tmathern@users.noreply.github.com> Date: Fri, 17 Jul 2026 20:23:59 -0700 Subject: [PATCH 12/20] fix: Rebaseline --- src/c2pa/c2pa.py | 2 +- tests/perf/baseline.json | 345 ++++++++++++++++++++------------------- 2 files changed, 176 insertions(+), 171 deletions(-) diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index b0a24206..fb7f4cf5 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -2591,7 +2591,7 @@ def with_fragment(self, format: str, stream, ) except Exception as e: # The callee consumes the old handle before it can fail, so - # treat it as consumed and let go of resources. + # treated as consumed to let go of resources. self._mark_consumed() raise C2paError( Reader._ERROR_MESSAGES['fragment_error'].format(e)) diff --git a/tests/perf/baseline.json b/tests/perf/baseline.json index 934a6390..a6a75177 100644 --- a/tests/perf/baseline.json +++ b/tests/perf/baseline.json @@ -2,269 +2,274 @@ "_meta": { "memray_version": "1.19.3", "python_version": "3.12.13", - "c2pa_native_version": "c2pa-v0.89.0", - "iterations": 100, + "c2pa_native_version": "c2pa-v0.90.0", + "iterations": 200, "perf_env": "python-3.12-slim", "arch": "aarch64" }, "reader_jpeg_legacy": { - "peak_bytes": 3791301, - "leaked_bytes": 3292672, - "total_allocations": 722823 + "peak_bytes": 3830666, + "leaked_bytes": 3331494, + "total_allocations": 1365464 }, "reader_jpeg_with_context": { - "peak_bytes": 3785583, - "leaked_bytes": 3284873, - "total_allocations": 717272 + "peak_bytes": 3824947, + "leaked_bytes": 3324219, + "total_allocations": 1354423 }, "reader_manifest_data_context": { - "peak_bytes": 7576945, - "leaked_bytes": 3407648, - "total_allocations": 619356 + "peak_bytes": 7616236, + "leaked_bytes": 3448019, + "total_allocations": 1152354 }, "reader_mp4": { - "peak_bytes": 4159582, - "leaked_bytes": 3283805, - "total_allocations": 2089508 + "peak_bytes": 4200644, + "leaked_bytes": 3323805, + "total_allocations": 4100459 }, "reader_wav": { - "peak_bytes": 4464615, - "leaked_bytes": 3293702, - "total_allocations": 413484 + "peak_bytes": 4501138, + "leaked_bytes": 3333747, + "total_allocations": 746935 }, "builder_sign_jpeg_legacy": { - "peak_bytes": 7724599, - "leaked_bytes": 3408513, - "total_allocations": 560691 + "peak_bytes": 8108426, + "leaked_bytes": 3485229, + "total_allocations": 1115105 }, "builder_sign_jpeg_with_context": { - "peak_bytes": 7716613, - "leaked_bytes": 3400514, - "total_allocations": 554788 + "peak_bytes": 8108408, + "leaked_bytes": 3477526, + "total_allocations": 1103189 }, "builder_sign_png_legacy": { - "peak_bytes": 7961139, - "leaked_bytes": 3406984, - "total_allocations": 1981843 + "peak_bytes": 8002675, + "leaked_bytes": 3447638, + "total_allocations": 3887535 }, "builder_sign_png_with_context": { - "peak_bytes": 7955799, - "leaked_bytes": 3401929, - "total_allocations": 1975867 + "peak_bytes": 7994768, + "leaked_bytes": 3440151, + "total_allocations": 3875483 }, "builder_sign_jpeg_parallel_split_pool": { - "peak_bytes": 44002804, - "leaked_bytes": 3801899, - "total_allocations": 566497 + "peak_bytes": 44042681, + "leaked_bytes": 3841702, + "total_allocations": 1045285 }, "builder_sign_jpeg_parallel_split_barrier": { - "peak_bytes": 45784452, - "leaked_bytes": 3800550, - "total_allocations": 565361 + "peak_bytes": 45824424, + "leaked_bytes": 3840908, + "total_allocations": 1043917 }, "builder_sign_png_parallel_split_pool": { - "peak_bytes": 46054163, - "leaked_bytes": 3801867, - "total_allocations": 1987868 + "peak_bytes": 46094336, + "leaked_bytes": 3860177, + "total_allocations": 3887276 }, "builder_sign_png_parallel_split_barrier": { - "peak_bytes": 44206900, - "leaked_bytes": 3800490, - "total_allocations": 1986526 + "peak_bytes": 46062355, + "leaked_bytes": 3876678, + "total_allocations": 3885976 }, "builder_sign_gif": { - "peak_bytes": 14574556, - "leaked_bytes": 3400735, - "total_allocations": 8550061 + "peak_bytes": 14614794, + "leaked_bytes": 3439807, + "total_allocations": 17023655 }, "builder_sign_heic": { - "peak_bytes": 4644811, - "leaked_bytes": 3407275, - "total_allocations": 831824 + "peak_bytes": 4677761, + "leaked_bytes": 3447623, + "total_allocations": 1569619 }, "builder_sign_m4a": { - "peak_bytes": 18885052, - "leaked_bytes": 3407378, - "total_allocations": 2647270 + "peak_bytes": 18812724, + "leaked_bytes": 3448162, + "total_allocations": 5200482 }, "builder_sign_webp": { - "peak_bytes": 8928848, - "leaked_bytes": 3399564, - "total_allocations": 499272 + "peak_bytes": 8970587, + "leaked_bytes": 3440333, + "total_allocations": 922037 }, "builder_sign_avi": { - "peak_bytes": 7068483, - "leaked_bytes": 3399340, - "total_allocations": 45032279 + "peak_bytes": 7110163, + "leaked_bytes": 3440347, + "total_allocations": 89988101 }, "builder_sign_mp4": { - "peak_bytes": 6198764, - "leaked_bytes": 3407319, - "total_allocations": 1944326 + "peak_bytes": 6224729, + "leaked_bytes": 3448147, + "total_allocations": 3794640 }, "builder_sign_tiff": { - "peak_bytes": 13151779, - "leaked_bytes": 3400355, - "total_allocations": 5472611 + "peak_bytes": 13192399, + "leaked_bytes": 3440348, + "total_allocations": 10868711 }, "builder_sign_jpeg_parent_of": { - "peak_bytes": 14204315, - "leaked_bytes": 3401481, - "total_allocations": 1292637 + "peak_bytes": 14244190, + "leaked_bytes": 3439412, + "total_allocations": 2514770 }, "builder_sign_jpeg_component_of": { - "peak_bytes": 14204923, - "leaked_bytes": 3400730, - "total_allocations": 1315125 + "peak_bytes": 14246389, + "leaked_bytes": 3440789, + "total_allocations": 2559666 }, "builder_sign_jpeg_parent_and_component": { - "peak_bytes": 14588185, - "leaked_bytes": 3549911, - "total_allocations": 2299453 + "peak_bytes": 14597099, + "leaked_bytes": 3546490, + "total_allocations": 4534820 }, "builder_sign_jpeg_parent_and_component_mixed_mime": { - "peak_bytes": 14506586, - "leaked_bytes": 3401364, - "total_allocations": 2796028 + "peak_bytes": 14545928, + "leaked_bytes": 3440216, + "total_allocations": 5528067 }, "builder_sign_jpeg_two_components_same_mime": { - "peak_bytes": 14601941, - "leaked_bytes": 3558116, - "total_allocations": 2289245 + "peak_bytes": 14539155, + "leaked_bytes": 3544510, + "total_allocations": 4508197 }, "builder_sign_jpeg_two_components_mixed_mime": { - "peak_bytes": 14503695, - "leaked_bytes": 3401276, - "total_allocations": 2785702 + "peak_bytes": 14544717, + "leaked_bytes": 3441158, + "total_allocations": 5501353 }, "builder_sign_jpeg_archive_roundtrip": { - "peak_bytes": 14236247, - "leaked_bytes": 3420354, - "total_allocations": 1777705 + "peak_bytes": 14277459, + "leaked_bytes": 3460332, + "total_allocations": 3480521 + }, + "builder_from_archive_roundtrip": { + "peak_bytes": 14277245, + "leaked_bytes": 3460181, + "total_allocations": 3108863 + }, + "builder_with_archive_swap": { + "peak_bytes": 3660981, + "leaked_bytes": 3330239, + "total_allocations": 708348 + }, + "reader_with_fragment_swap": { + "peak_bytes": 3758666, + "leaked_bytes": 3332314, + "total_allocations": 3792727 }, "builder_to_archive_with_ingredient": { - "peak_bytes": 14010137, - "leaked_bytes": 3278101, - "total_allocations": 957452 + "peak_bytes": 14049708, + "leaked_bytes": 3318104, + "total_allocations": 1836413 }, "builder_sign_jpeg_archive_roundtrip_ingredient_in_archive": { - "peak_bytes": 14225579, - "leaked_bytes": 3420822, - "total_allocations": 2981495 + "peak_bytes": 14264719, + "leaked_bytes": 3459571, + "total_allocations": 5893306 }, "builder_write_ingredient_archive": { - "peak_bytes": 14010131, - "leaked_bytes": 3278099, - "total_allocations": 944654 + "peak_bytes": 14049702, + "leaked_bytes": 3318102, + "total_allocations": 1810851 }, "builder_sign_jpeg_add_ingredient_from_archive": { - "peak_bytes": 14075511, - "leaked_bytes": 3421125, - "total_allocations": 1753642 + "peak_bytes": 14113307, + "leaked_bytes": 3459596, + "total_allocations": 3424465 }, "builder_ingredient_archive_roundtrip": { - "peak_bytes": 14222976, - "leaked_bytes": 3419885, - "total_allocations": 2609017 + "peak_bytes": 14264391, + "leaked_bytes": 3460351, + "total_allocations": 5146608 }, "builder_sign_jpeg_two_ingredient_archives": { - "peak_bytes": 14075190, - "leaked_bytes": 3421103, - "total_allocations": 2161232 + "peak_bytes": 14114874, + "leaked_bytes": 3461104, + "total_allocations": 4226879 }, "reader_error_no_manifest": { - "peak_bytes": 3504260, - "leaked_bytes": 3263283, - "total_allocations": 178873 + "peak_bytes": 3544227, + "leaked_bytes": 3302551, + "total_allocations": 278514 }, "builder_error_invalid_manifest": { - "peak_bytes": 3292723, - "leaked_bytes": 3235293, - "total_allocations": 96622 + "peak_bytes": 3331458, + "leaked_bytes": 3276705, + "total_allocations": 114863 }, "reader_string_apis": { - "peak_bytes": 3915891, - "leaked_bytes": 3284213, - "total_allocations": 1185492 + "peak_bytes": 3957555, + "leaked_bytes": 3324853, + "total_allocations": 2296696 + }, + "signer_construction": { + "peak_bytes": 3331349, + "leaked_bytes": 3268750, + "total_allocations": 155984 }, "fork_reader_collect": { - "peak_bytes": 3789318, - "leaked_bytes": 3291267, - "total_allocations": 705123 + "peak_bytes": 3830817, + "leaked_bytes": 3333241, + "total_allocations": 1330058 }, "fork_contended_mutex": { - "peak_bytes": 7617864, - "leaked_bytes": 3421711, - "total_allocations": 33591148 + "peak_bytes": 7659277, + "leaked_bytes": 3461442, + "total_allocations": 67635759 }, "fork_thread_local_orphan": { - "peak_bytes": 3876269, - "leaked_bytes": 3379616, - "total_allocations": 731511 + "peak_bytes": 3916484, + "leaked_bytes": 3419968, + "total_allocations": 1383197 }, "fork_gc_cycle": { - "peak_bytes": 3790340, - "leaked_bytes": 3291400, - "total_allocations": 706802 + "peak_bytes": 3830375, + "leaked_bytes": 3332761, + "total_allocations": 1334044 }, "fork_parent_frees_after_fork": { - "peak_bytes": 5989167, - "leaked_bytes": 3289951, - "total_allocations": 12461253 + "peak_bytes": 5427183, + "leaked_bytes": 3329834, + "total_allocations": 24873000 + }, + "fork_child_closes_then_parent_frees": { + "peak_bytes": 5427152, + "leaked_bytes": 3329841, + "total_allocations": 24872992 }, "fork_child_sys_exit": { - "peak_bytes": 3789334, - "leaked_bytes": 3291456, - "total_allocations": 708625 + "peak_bytes": 3831271, + "leaked_bytes": 3332312, + "total_allocations": 1338865 }, "fork_stream_cleanup": { - "peak_bytes": 3402893, - "leaked_bytes": 3230663, - "total_allocations": 93141 - }, - "builder_from_archive_roundtrip": { - "peak_bytes": 14258579, - "leaked_bytes": 3436798, - "total_allocations": 1593617 - }, - "signer_construction": { - "peak_bytes": 3307608, - "leaked_bytes": 3243306, - "total_allocations": 117601 - }, - "builder_with_archive_swap": { - "peak_bytes": 3635924, - "leaked_bytes": 3305070, - "total_allocations": 395447 - }, - "reader_with_fragment_swap": { - "peak_bytes": 3733349, - "leaked_bytes": 3306787, - "total_allocations": 1936244 + "peak_bytes": 3444268, + "leaked_bytes": 3272486, + "total_allocations": 106279 }, "fork_swap_cleanup": { - "peak_bytes": 3639043, - "leaked_bytes": 3308397, - "total_allocations": 401032 - }, - "swap_chain_churn": { - "peak_bytes": 3650097, - "leaked_bytes": 3319300, - "total_allocations": 379064 - }, - "fork_consumed_signer": { - "peak_bytes": 3321464, - "leaked_bytes": 3258207, - "total_allocations": 130030 + "peak_bytes": 3661376, + "leaked_bytes": 3330987, + "total_allocations": 718355 }, "fork_contended_mutex_swap": { - "peak_bytes": 7281970, - "leaked_bytes": 3443799, - "total_allocations": 18799685 + "peak_bytes": 7276901, + "leaked_bytes": 3443869, + "total_allocations": 37514894 }, "fork_contended_mutex_wrap": { - "peak_bytes": 7268578, - "leaked_bytes": 3442991, - "total_allocations": 17791158 + "peak_bytes": 7262615, + "leaked_bytes": 3474333, + "total_allocations": 35888503 + }, + "fork_consumed_signer": { + "peak_bytes": 3331615, + "leaked_bytes": 3269742, + "total_allocations": 179994 + }, + "swap_chain_churn": { + "peak_bytes": 3661193, + "leaked_bytes": 3330367, + "total_allocations": 673717 } } \ No newline at end of file From 075fb21c75920bb447efb39e13fdd31cd31d1979 Mon Sep 17 00:00:00 2001 From: tmathern <60901087+tmathern@users.noreply.github.com> Date: Sun, 19 Jul 2026 08:15:59 -0700 Subject: [PATCH 13/20] fix: Clean up debug --- pr294-finishing-plan.md | 121 ---------------------------------------- 1 file changed, 121 deletions(-) delete mode 100644 pr294-finishing-plan.md diff --git a/pr294-finishing-plan.md b/pr294-finishing-plan.md deleted file mode 100644 index 3579c47c..00000000 --- a/pr294-finishing-plan.md +++ /dev/null @@ -1,121 +0,0 @@ -# PR #294 — Finishing plan: native-handle lifecycle extension surface - -**Goal:** take #294 from WIP to mergeable and reviewer-proof. The code is -mechanically sound; the work left is *committing to the extension contract* and -writing it down where it can be enforced. No functional rewrite required. - ---- - -## Decision to record first: the extension model - -Downstream libraries should be able to wrap their own FFI pointers in the -managed lifecycle, **without** the project promising a stable public API. - -Chosen: **single-underscore, subclass-facing (protected).** Keep the names as -they are (`_activate`, `_swap_handle`, `_mark_consumed`, `_wrap_native_handle`, -`_init_attrs`). - -Why this level and not the alternatives — state this in the PR so it isn't -re-litigated in review: - -- **Not public (no underscore).** A bare name is an implicit stability promise. - The seam is still evolving; we don't want to guarantee it. -- **Not dunder (`__name`).** Double underscore triggers per-class name - mangling, which breaks subclassing — the exact capability this PR exists to - enable. It's the one "more private" option that defeats the purpose. -- **Single underscore is the idiom for "reachable, unsupported, subclass- - friendly."** Python privacy is convention, not enforcement: a downstream lib - *can* call these, and that's intended. The underscore communicates "no - stability guarantee," nothing more. - -Consequence that drives the rest of the plan: because the underscore does not -enforce anything, **the docstrings are the contract.** They must carry the -invariants as if the methods were public, because for the people who reach them -they effectively are. - ---- - -## Work items - -### 1. Make the docstrings the contract (primary work) -Each exposed primitive states its ownership transfer explicitly: -- `_activate(handle)` — takes ownership; frees on close; only from - `UNINITIALIZED`; rejects null and double-activation. -- `_swap_handle(new_handle)` — **the caller guarantees the callee already - consumed and freed the old pointer.** Requires `ACTIVE`; rejects null. -- `_mark_consumed()` — the native pointer is now owned elsewhere; this releases - only *Python-side* resources and marks `CLOSED` without freeing the pointer. -- `_wrap_native_handle(handle)` — ownership transfers **only on successful - return**; if it raises, the caller still owns the pointer and must free it. - -### 2. Nail the `_wrap_native_handle` initialization invariant -It bypasses `__init__` entirely and runs only `_init_attrs()`. Add one explicit -line to the docstring: - -> Everything an instance needs besides the native handle must be set in -> `_init_attrs()`, not `__init__` — `_wrap_native_handle` never runs `__init__`. - -Live proof this matters: `Reader.__init__` sets `self._context = context` -*after* `_init_attrs()`, so a Reader built via `_wrap_native_handle` would get -`_context = None`. Fine for `Builder.from_archive` (no context by design), but a -footgun for any external extender who puts setup in `__init__`. - -### 3. Legibility on the consume-and-return null path -In `with_fragment` and `with_archive`, the null branch relies on -`_check_ffi_operation_result` raising *before* control reaches `_swap_handle`. -It is safe today, but reads as "mark consumed, then a check that happens to -raise." Make the intent explicit: - -```python -new_ptr = _lib.c2pa_..._with_...(self._handle, ...) -if not new_ptr: - # callee consumed the old handle and returned nothing to own - self._mark_consumed() - _check_ffi_operation_result(new_ptr, "...") # raises here -self._swap_handle(new_ptr) -``` - -Moving the check inside the `if not new_ptr:` block makes the control flow say -what it means. No behavior change. - -### 4. `super().__init__()` audit (cheap) -Every `_activate` caller depends on `self._lifecycle_state` already existing -(set by `ManagedResource.__init__`). Context / Reader / Builder / Signer visibly -call `super().__init__()`. Confirm `Settings.__init__` does too. If the existing -tests pass, it already does — this is a five-second eyeball, not a suspected bug. - -### 5. Tests: cover the *external-extender* path -The added lifecycle / integration / cross-thread tests cover the built-in types. -Add the case the PR actually unlocks: -- A minimal subclass that owns a raw handle and reaches the lifecycle via - `_wrap_native_handle` + `_init_attrs`, asserting the instance is fully built - (no missing attributes) and frees exactly once. -- The same wrapped instance under the fork guard: a foreign-process teardown - skips the native free (owner-PID stamped through the wrap path). - ---- - -## Explicit non-goals (pre-empt scope-creep review comments) -- **Not** renaming anything to public. The access level is the decision, not an - oversight. -- **Not** adding fork guards to *operation* paths (`with_fragment` etc.). The - PID guard's contract is teardown-safety in a forked child, not operation- - safety. Operating on a handle in a forked child is caller error and out of - scope. - ---- - -## Review-defense notes (the "why", pre-answered) - -- **Why underscore, not public?** See the extension-model decision above: - reachable but unsupported is exactly the intent. -- **Why does `_mark_consumed()` now run `_release()`?** Previously the consume - paths leaked Python-side resources (callback refs, streams, caches) until GC. - Releasing on consume is a fix, not a regression. It cannot double-release: - `_cleanup_resources` skips when the state is already `CLOSED`. -- **Why is the signer callback copied onto the Context before the signer is - consumed?** The native signer holds a pointer to the Python callback. - `_mark_consumed() -> _release()` clears the signer's callback ref, so the - Context must capture its own reference *first*, or the first invocation of the - consumed signer's callback would be a use-after-free. The ordering is the - whole reason it's safe; the inline comment explaining it must stay. From 2a78a766d6cf2d41d11a7d438fddcd60b1f440f6 Mon Sep 17 00:00:00 2001 From: tmathern <60901087+tmathern@users.noreply.github.com> Date: Sun, 19 Jul 2026 20:17:11 -0700 Subject: [PATCH 14/20] fix: Refactor --- src/c2pa/c2pa.py | 305 ++++++++++-------- tests/perf/scenarios.py | 212 +++++++++++++ tests/test_unit_tests.py | 652 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 1037 insertions(+), 132 deletions(-) diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index fb7f4cf5..ee06d265 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -259,7 +259,6 @@ def _init_attrs(self): def __init__(self): self._lifecycle_state = LifecycleState.UNINITIALIZED self._handle = None - _clear_error_state() record_owner_pid(self) @staticmethod @@ -282,7 +281,6 @@ def _ensure_valid_state(self): if not self._handle: raise C2paError( f"{name} has an invalid internal state (active but no handle)") - _clear_error_state() def _release(self): """Override to free class-specific resources (streams, caches, etc.). @@ -293,11 +291,6 @@ def _release(self): def _safe_release(self): """Run _release(), logging (never raising) if it fails. - - Shared by _mark_consumed and _cleanup_resources so the try/except/log - wrapper lives in one place. Each caller keeps its own lifecycle-state - transition, because the two paths deliberately order the CLOSED flip - differently relative to this call (see each call site). """ try: self._release() @@ -388,6 +381,70 @@ def _swap_handle(self, new_handle): self._handle = new_handle + # 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:") + + def _consume_and_swap(self, ffi_call, error_message): + """Run an FFI call that consumes this handle and returns a replacement. + + The native lib takes ownership partway through the call, + so a null return can be ambiguous. + The native error tells the cases apart: + - a pointer rejection (`_PRE_CONSUME_ERROR_TAGS`) precedes the + transfer, so the handle is still ours and is kept; + - any other error means it was taken, then the operation failed; + - a null with no error cannot be placed, so the handle is dropped + anyway: leaking is recoverable, double-freeing is not. No native + path does this, so it only appears under mocks. + + The error is read without clearing it first. + The slot is sticky: c2pa_error() peeks and nothing empties it. + + Args: + ffi_call: Callable taking the current handle, returning the + replacement or null. + error_message: Format string with one placeholder, used when the + native layer offers no error of its own. + + Raises: + C2paError: If the call fails, typed by native error when one. + """ + try: + new_ptr = ffi_call(self._handle) + except ctypes.ArgumentError: + # Marshalling failed, so the call never reached the native side + # and the handle is untouched. + raise + except BaseException as e: + self._mark_consumed() + raise C2paError(error_message.format(e)) from e + + if new_ptr: + self._swap_handle(new_ptr) + return + + error = _read_native_error() + if error: + if any(tag in error + for tag in ManagedResource._PRE_CONSUME_ERROR_TAGS): + # Rejected before ownership transferred, so the handle is + # still ours: keep it and let normal cleanup free it. + logger.warning( + "%s: native call rejected the handle before taking " + "ownership (%s); handle retained", + type(self).__name__, + error) + _raise_typed_c2pa_error(error) + + # Ownership transferred and then the operation failed. + self._mark_consumed() + _raise_typed_c2pa_error(error) + + self._mark_consumed() + raise C2paError(error_message.format("Unknown error")) + @classmethod def _wrap_native_handle(cls, handle): """Build a brand-new instance around an already-valid, @@ -566,18 +623,25 @@ class C2paStream(ctypes.Structure): ] -def _clear_error_state(): - """Clear any existing error state from the C library. +def _read_native_error() -> Optional[str]: + """Read the last error from the native library, or None if unset. - This function should be called at the beginning of object initialization - and before any operations that could potentially raise an error, - to ensure that stale error states from previous operations don't interfere - with new objects being created, or independent function calls. + 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. """ error = _lib.c2pa_error() - if error: - # Free the error to clear the state + if not error: + return None + try: + message = ctypes.string_at(error).decode('utf-8') + finally: _lib.c2pa_string_free(error) + return message or None class C2paSignerInfo(ctypes.Structure): @@ -600,7 +664,6 @@ def __init__(self, alg, sign_cert, private_key, ta_url): private_key: The private key as a string ta_url: The timestamp authority URL as bytes """ - _clear_error_state() if sign_cert is None: raise ValueError("sign_cert must be set") @@ -1010,6 +1073,24 @@ class _C2paVerify(C2paError): pass +class _C2paUntrackedPointer(C2paError): + """Exception raised when the native layer does not recognize a pointer. + + Raised when a consume-and-return call rejects the handle it was given + before taking ownership of it, so the caller still owns that handle. + """ + pass + + +class _C2paWrongPointerType(C2paError): + """Exception raised when a pointer is tracked under a different type. + + Like _C2paUntrackedPointer, this is rejected before ownership transfer, + so the caller still owns the handle it passed in. + """ + pass + + # Attach exception subclasses to C2paError for backward compatibility # Preserves behavior for exception catching like except C2paError.ManifestNotFound, # also reduces imports (think of it as an alias of sorts) @@ -1028,6 +1109,8 @@ class _C2paVerify(C2paError): C2paError.ResourceNotFound = _C2paResourceNotFound C2paError.Signature = _C2paSignature C2paError.Verify = _C2paVerify +C2paError.UntrackedPointer = _C2paUntrackedPointer +C2paError.WrongPointerType = _C2paWrongPointerType class _StringContainer: @@ -1152,6 +1235,10 @@ def _raise_typed_c2pa_error(error_str: str) -> None: raise C2paError.Signature(error_str) elif error_type == "Verify": raise C2paError.Verify(error_str) + elif error_type == "UntrackedPointer": + raise C2paError.UntrackedPointer(error_str) + elif error_type == "WrongPointerType": + raise C2paError.WrongPointerType(error_str) # If no recognized error type, raise base C2paError raise C2paError(error_str) @@ -1179,10 +1266,8 @@ def _parse_operation_result_for_error( """ if not result: # pragma: no cover if check_error: - error = _lib.c2pa_error() - if error: - error_str = ctypes.string_at(error).decode('utf-8') - _lib.c2pa_string_free(error) + error_str = _read_native_error() + if error_str: _raise_typed_c2pa_error(error_str) return None @@ -1314,7 +1399,6 @@ def load_settings(settings: Union[str, dict], format: str = "json") -> None: DeprecationWarning, stacklevel=2, ) - _clear_error_state() # Convert to JSON string as necessary try: @@ -1416,7 +1500,7 @@ def __init__(self): ptr = _lib.c2pa_settings_new() try: _check_ffi_operation_result(ptr, "Failed to create Settings") - except Exception: + except BaseException: if ptr: ManagedResource._free_native_ptr(ptr) raise @@ -1608,15 +1692,18 @@ def __init__( signer._ensure_valid_state() # c2pa_context_builder_set_signer takes ownership of the # signer pointer immediately (Box::from_raw), on its error - # path as well as on success. The Signer is therefore - # consumed below on any result; leaving it owning a freed - # pointer would make any later use of it a use-after-free. + # path as well as on success. self._signer_callback_cb = signer._callback_cb - result = ( - _lib.c2pa_context_builder_set_signer( - builder_ptr, signer._handle, + try: + result = ( + _lib.c2pa_context_builder_set_signer( + builder_ptr, signer._handle, + ) ) - ) + except ctypes.ArgumentError: + # Marshalling failed, so the call never reached the + # native side and the signer was never taken. + raise signer._mark_consumed() if result != 0: _parse_operation_result_for_error(None) @@ -1633,7 +1720,7 @@ def __init__( ) self._activate(ptr) - except Exception: + except BaseException: # Free builder if build was not reached if builder_ptr is not None: try: @@ -1935,7 +2022,6 @@ def flush_callback(ctx): self._flush_cb = FlushCallback(flush_callback) # Create the stream - _clear_error_state() self._stream = _lib.c2pa_create_stream( None, self._read_cb, @@ -1944,8 +2030,9 @@ def flush_callback(ctx): self._flush_cb ) if not self._stream: - error = _parse_operation_result_for_error(_lib.c2pa_error()) - raise Exception("Failed to create stream: {}".format(error)) + error = _read_native_error() + raise C2paError( + "Failed to create stream: {}".format(error or "Unknown error")) self._initialized = True record_owner_pid(self) @@ -2078,15 +2165,14 @@ def _get_supported_mime_types(ffi_func, cache): if cache is not None: return list(cache), cache - _clear_error_state() count = ctypes.c_size_t() arr = ffi_func(ctypes.byref(count)) if not arr: - error = _parse_operation_result_for_error(_lib.c2pa_error()) - if error: - raise C2paError(f"Failed to get supported MIME types: {error}") - return [], cache + error = _read_native_error() + raise C2paError( + "Failed to get supported MIME types: " + f"{error or 'Unknown error'}") if count.value <= 0: try: @@ -2442,11 +2528,16 @@ def _init_from_context(self, context, format_or_path, 'reader_error' ].format("Unknown error") ) - except Exception: + except BaseException: if reader_ptr: ManagedResource._free_native_ptr(reader_ptr) raise + # Adopt the handle before the consuming call: _consume_and_swap + # needs an ACTIVE resource, and from here on normal cleanup owns + # the pointer whichever way the call goes. + self._activate(reader_ptr) + if manifest_data is not None: manifest_array = ( ctypes.c_ubyte * @@ -2454,34 +2545,26 @@ def _init_from_context(self, context, format_or_path, # Consume current reader, # with manifest data and stream (C FFI pattern), # to create a new one (switch out) - new_ptr = ( - _lib.c2pa_reader_with_manifest_data_and_stream( - reader_ptr, - format_bytes, - self._own_stream._stream, - manifest_array, - len(manifest_data), - ) - ) + self._consume_and_swap( + lambda handle: ( + _lib.c2pa_reader_with_manifest_data_and_stream( + handle, + format_bytes, + self._own_stream._stream, + manifest_array, + len(manifest_data), + ) + ), + Reader._ERROR_MESSAGES['reader_error']) else: # Consume reader with stream - new_ptr = _lib.c2pa_reader_with_stream( - reader_ptr, format_bytes, - self._own_stream._stream, - ) - - # reader_ptr has been consumed by the FFI call (freed by it even - # on failure), so there is nothing to free on the error path. - reader_ptr = None - - _check_ffi_operation_result(new_ptr, - Reader._ERROR_MESSAGES[ - 'reader_error' - ].format("Unknown error") - ) - - self._activate(new_ptr) - except Exception: + self._consume_and_swap( + lambda handle: _lib.c2pa_reader_with_stream( + handle, format_bytes, + self._own_stream._stream, + ), + Reader._ERROR_MESSAGES['reader_error']) + except BaseException: self._close_streams() raise @@ -2582,30 +2665,14 @@ def with_fragment(self, format: str, stream, ) with Stream(stream) as main_obj, Stream(fragment_stream) as frag_obj: - try: - new_ptr = _lib.c2pa_reader_with_fragment( - self._handle, + self._consume_and_swap( + lambda handle: _lib.c2pa_reader_with_fragment( + handle, format_bytes, main_obj._stream, frag_obj._stream, - ) - except Exception as e: - # The callee consumes the old handle before it can fail, so - # treated as consumed to let go of resources. - self._mark_consumed() - raise C2paError( - Reader._ERROR_MESSAGES['fragment_error'].format(e)) - - # c2pa_reader_with_fragment consumed the old handle. A null return - # leaves no replacement to take ownership of: mark consumed, then - # raise. _swap_handle is only reached when new_ptr is non-null. - if not new_ptr: - self._mark_consumed() - _check_ffi_operation_result(new_ptr, - Reader._ERROR_MESSAGES[ - 'fragment_error' - ].format("Unknown error")) - self._swap_handle(new_ptr) + ), + 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. @@ -2616,7 +2683,6 @@ def with_fragment(self, format: str, stream, return self - def json(self) -> str: """Get the manifest store as a JSON string. @@ -2883,10 +2949,6 @@ def from_info(cls, signer_info: C2paSignerInfo) -> 'Signer': Raises: C2paError: If there was an error creating the signer """ - # Native libs plumbing: - # Clear any stale error state from previous operations - _clear_error_state() - signer_ptr = _lib.c2pa_signer_from_info(ctypes.byref(signer_info)) _check_ffi_operation_result( @@ -3004,10 +3066,6 @@ def wrapped_callback( cls._ERROR_MESSAGES['encoding_error'].format( str(e))) - # Native libs plumbing: - # Clear any stale error state from previous operations - _clear_error_state() - # Create the callback object using the callback function callback_cb = SignerCallback(wrapped_callback) @@ -3102,6 +3160,7 @@ class Builder(ManagedResource): 'archive_read_error': "Error loading ingredient from archive: {}", 'action_error': "Error adding action: {}", 'archive_error': "Error writing archive: {}", + 'archive_load_error': "Failed to load archive into builder: {}", 'sign_error': "Error during signing: {}", 'encoding_error': "Invalid UTF-8 characters in manifest: {}", 'json_error': "Failed to serialize manifest JSON: {}" @@ -3188,10 +3247,9 @@ def from_archive( ) try: - # A builder from an archive carries no context, which is what - # _init_attrs() already defaults to. + # A builder from an archive here carries no context. return cls._wrap_native_handle(handle) - except Exception: + except BaseException: # No instance took ownership, so the handle is still ours. ManagedResource._free_native_ptr(handle) raise @@ -3268,23 +3326,20 @@ def _init_from_context(self, context, json_str): 'builder_error' ].format("Unknown error") ) - except Exception: + except BaseException: if builder_ptr: ManagedResource._free_native_ptr(builder_ptr) raise - # Consume-and-return: builder_ptr is consumed (freed by the FFI even - # on failure), new_ptr is the valid pointer going forward. Nothing to - # free here on the error path. - new_ptr = _lib.c2pa_builder_with_definition(builder_ptr, json_str) + # Adopt the handle before the consuming call: _consume_and_swap needs + # an ACTIVE resource, and from here on normal cleanup owns the pointer + # whichever way the call goes. + self._activate(builder_ptr) - _check_ffi_operation_result(new_ptr, - Builder._ERROR_MESSAGES[ - 'builder_error' - ].format("Unknown error") - ) - - self._activate(new_ptr) + self._consume_and_swap( + lambda handle: _lib.c2pa_builder_with_definition( + handle, json_str), + Builder._ERROR_MESSAGES['builder_error']) def _init_attrs(self): super()._init_attrs() @@ -3567,22 +3622,10 @@ def with_archive(self, stream: Any) -> 'Builder': self._ensure_valid_state() with Stream(stream) as stream_obj: - try: - new_ptr = _lib.c2pa_builder_with_archive( - self._handle, stream_obj._stream) - except Exception as e: - self._mark_consumed() - raise C2paError( - f"Error loading archive: {e}" - ) - # c2pa_builder_with_archive consumed the old handle. A null return - # leaves no replacement to take ownership of: mark consumed, then - # raise. _swap_handle is only reached when new_ptr is non-null. - if not new_ptr: - self._mark_consumed() - _check_ffi_operation_result( - new_ptr, "Failed to load archive into builder") - self._swap_handle(new_ptr) + self._consume_and_swap( + lambda handle: _lib.c2pa_builder_with_archive( + handle, stream_obj._stream), + Builder._ERROR_MESSAGES['archive_load_error']) return self @@ -3645,9 +3688,11 @@ def _sign_internal( # Closing here ensures resources clean up, # and single use/single sign done by a Builder. self.close() - except Exception as e: + except BaseException as e: + # BaseException, not Exception: a KeyboardInterrupt mid-sign must + # still close the Builder rather than leaving its handle live. self.close() - raise C2paError(f"Error during signing: {e}") + raise C2paError(f"Error during signing: {e}") from e _check_ffi_operation_result( result, @@ -3863,8 +3908,6 @@ def format_embeddable(format: str, manifest_bytes: bytes) -> tuple[int, bytes]: Raises: C2paError: If there was an error converting the manifest """ - _clear_error_state() - format_str = format.encode('utf-8') manifest_array = (ctypes.c_ubyte * len(manifest_bytes)).from_buffer_copy( manifest_bytes @@ -3980,8 +4023,6 @@ def ed25519_sign(data: bytes, private_key: str) -> bytes: C2paError: If there was an error signing the data C2paError.Encoding: If the private key contains invalid UTF-8 chars """ - _clear_error_state() - if not data: raise C2paError("Data to sign cannot be empty") diff --git a/tests/perf/scenarios.py b/tests/perf/scenarios.py index 25c688a0..6166f6f6 100644 --- a/tests/perf/scenarios.py +++ b/tests/perf/scenarios.py @@ -9,6 +9,7 @@ Each function is called N times by run_profile.py. """ +import ctypes import gc import io import json @@ -27,6 +28,7 @@ Signer, Stream, ) +import c2pa.c2pa as c2pa_module FIXTURES_DIR = Path(__file__).parent.parent / "fixtures" READING_FIXTURES_DIR = FIXTURES_DIR / "files-for-reading-tests" @@ -544,6 +546,148 @@ def scenario_builder_from_archive_roundtrip(iterations: int = 100) -> None: builder.sign(signer, "image/jpeg", io.BytesIO(source_bytes), io.BytesIO()) +# Consume-and-return failure paths. +# +# These calls take ownership partway through their body, so a null return is +# ambiguous and getting it wrong leaks one handle per call. The success paths +# are covered above; these loop the failure paths, where the leak would be. + +def _untracked_reader_handle(): + """A pointer the native registry does not know about. + + A never-allocated buffer, so it is rejected like a stale handle without + allocating a real Reader per call, which would swamp the measurement. + Freed handles are unusable here: recycled addresses become tracked again. + """ + buf = ctypes.create_string_buffer(64) + return ctypes.cast(buf, ctypes.POINTER(c2pa_module.C2paReader)), buf + + +def scenario_reader_with_fragment_pre_consume_rejection( + iterations: int = 100) -> None: + """Loop the rejection that precedes the ownership transfer. + + The handle is still ours, so treating this as consumed drops a pointer + the registry still holds and leaked_bytes climbs with iterations. + """ + init_bytes = DASH_INIT_MP4.read_bytes() + fragment_bytes = DASH_FRAGMENT.read_bytes() + for _ in _iterate(iterations): + reader = Reader("video/mp4", io.BytesIO(init_bytes)) + real_handle = reader._handle + # Keep the buffer alive: the cast pointer does not own it, and an + # early collection would hand the FFI a dangling address. + bogus, _buf = _untracked_reader_handle() + reader._handle = bogus + try: + reader.with_fragment("video/mp4", io.BytesIO(init_bytes), + io.BytesIO(fragment_bytes)) + raise AssertionError("pre-consume rejection did not raise") + except C2paError as e: + # 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): + raise AssertionError( + f"expected a pre-consume rejection, got: {e}") from e + if reader._handle is None: + raise AssertionError( + "handle was dropped on a pre-consume rejection; the " + "native side never took ownership, so this leaks") from e + finally: + # Restore before close() so the real handle is freed exactly once. + reader._handle = real_handle + reader.close() + + +def scenario_builder_with_archive_post_consume_failure( + iterations: int = 100) -> None: + """Loop a failure after the ownership transfer. + + The control: if the fix over-corrected into retaining handles the native + side already dropped, this scenario double-frees or leaks. + """ + for _ in _iterate(iterations): + builder = Builder(MANIFEST_BASE) + try: + builder.with_archive(io.BytesIO(b"not a valid archive")) + raise AssertionError("post-consume failure did not raise") + except C2paError: + pass + finally: + builder.close() + + +def scenario_with_fragment_marshalling_error(iterations: int = 100) -> None: + """Loop a failure that never reaches native code. + + Nothing was consumed, so the reader must stay usable. The old blanket + except marked it consumed here, leaking on what is only a type error. + """ + init_bytes = DASH_INIT_MP4.read_bytes() + for _ in _iterate(iterations): + reader = Reader("video/mp4", io.BytesIO(init_bytes)) + try: + reader.with_fragment("video/mp4", object(), object()) + raise AssertionError("marshalling error did not raise") + except (C2paError, TypeError, ctypes.ArgumentError): + pass + finally: + # Must still be usable: nothing was handed over. + reader.json() + reader.close() + + +def scenario_with_fragment_mixed_outcomes(iterations: int = 100) -> None: + """Interleave success, pre-consume rejection and post-consume failure. + + Each path leaves a different state behind, so running them in sequence + catches a stale error being read as the current call's. + """ + init_bytes = DASH_INIT_MP4.read_bytes() + fragment_bytes = DASH_FRAGMENT.read_bytes() + for i in _iterate(iterations): + phase = i % 3 + reader = Reader("video/mp4", io.BytesIO(init_bytes)) + try: + if phase == 0: + reader.with_fragment("video/mp4", io.BytesIO(init_bytes), + io.BytesIO(fragment_bytes)) + elif phase == 1: + real_handle = reader._handle + bogus, _buf = _untracked_reader_handle() + reader._handle = bogus + try: + reader.with_fragment("video/mp4", io.BytesIO(init_bytes), + io.BytesIO(fragment_bytes)) + raise AssertionError( + "pre-consume rejection did not raise") + except C2paError as e: + # A stale error from the phase before must not be read as + # this call's; the rejection would stop being recognised. + if not any( + tag in str(e) for tag in c2pa_module + .ManagedResource._PRE_CONSUME_ERROR_TAGS): + raise AssertionError( + f"expected a pre-consume rejection, got: {e}" + ) from e + if reader._handle is None: + raise AssertionError( + "handle dropped on a pre-consume rejection" + ) from e + finally: + reader._handle = real_handle + else: + try: + reader.with_fragment("video/mp4", io.BytesIO(b"garbage"), + io.BytesIO(b"garbage")) + except C2paError: + pass + finally: + reader.close() + + # Archive scenarios: builder as working store (to_archive/with_archive) and # per-ingredient archives (write_ingredient_archive/add_ingredient_from_archive). @@ -745,6 +889,64 @@ def scenario_reader_string_apis(iterations: int = 100) -> None: reader.close() +def scenario_builder_from_context_construction(iterations: int = 100) -> None: + """Loop Builder(context=...) construction, the consume-and-swap path. + + c2pa_builder_from_context hands back a handle that + c2pa_builder_with_definition then consumes and replaces. The Builder + adopts the first handle before that call so _consume_and_swap can own the + swap, which means a mis-sequenced swap leaks the replacement or frees the + consumed pointer twice. The other context builder scenarios sign a full + asset per iteration, so a one-handle regression here would sit under their + noise. This one only constructs and closes. + + scenario_reader_manifest_data_context is the Reader-side equivalent. + """ + context = Context() + for _ in _iterate(iterations): + builder = Builder(MANIFEST_BASE, context=context) + builder.close() + + +def scenario_sign_interrupted(iterations: int = 100) -> None: + """Loop a sign that is interrupted partway through the FFI call. + + _sign_internal catches BaseException so the Builder is closed even when + the interrupt is not an Exception subclass. Narrowing that back to + Exception leaks the whole Builder per iteration, since close() is skipped + and the handle outlives the object. + """ + source_bytes = SOURCE_JPEG.read_bytes() + signer = _make_signer() + real_sign = c2pa_module._lib.c2pa_builder_sign + + def _interrupt(*args): + raise KeyboardInterrupt + + c2pa_module._lib.c2pa_builder_sign = _interrupt + try: + for _ in _iterate(iterations): + builder = Builder(MANIFEST_BASE) + try: + builder.sign(signer, "image/jpeg", + io.BytesIO(source_bytes), io.BytesIO()) + except C2paError: + # The wrapper re-raises the interrupt as C2paError, having + # closed the Builder first. Anything else means the handler + # stopped catching it. + pass + else: + raise AssertionError("interrupted sign did not raise") + if (builder._lifecycle_state + is not c2pa_module.LifecycleState.CLOSED): + raise AssertionError( + "interrupted sign left the Builder open; its handle " + "leaks once per iteration") + finally: + c2pa_module._lib.c2pa_builder_sign = real_sign + signer.close() + + def scenario_signer_construction(iterations: int = 100) -> None: """Loop Signer.from_info()/__init__ construction and teardown. @@ -1207,6 +1409,13 @@ def scenario_fork_stream_cleanup(iterations: int = 100) -> None: "builder_from_archive_roundtrip": scenario_builder_from_archive_roundtrip, "builder_with_archive_swap": scenario_builder_with_archive_swap, "reader_with_fragment_swap": scenario_reader_with_fragment_swap, + "with_fragment_pre_consume_rejection": + scenario_reader_with_fragment_pre_consume_rejection, + "with_archive_post_consume_failure": + scenario_builder_with_archive_post_consume_failure, + "with_fragment_marshalling_error": + scenario_with_fragment_marshalling_error, + "with_fragment_mixed_outcomes": scenario_with_fragment_mixed_outcomes, "builder_to_archive_with_ingredient": scenario_builder_to_archive_with_ingredient, "builder_sign_jpeg_archive_roundtrip_ingredient_in_archive": scenario_builder_sign_jpeg_archive_roundtrip_ingredient_in_archive, "builder_write_ingredient_archive": scenario_builder_write_ingredient_archive, @@ -1217,6 +1426,9 @@ def scenario_fork_stream_cleanup(iterations: int = 100) -> None: "builder_error_invalid_manifest": scenario_builder_error_invalid_manifest, "reader_string_apis": scenario_reader_string_apis, "signer_construction": scenario_signer_construction, + "builder_from_context_construction": + scenario_builder_from_context_construction, + "sign_interrupted": scenario_sign_interrupted, "fork_reader_collect": scenario_fork_reader_collect, "fork_contended_mutex": scenario_fork_contended_mutex, "fork_thread_local_orphan": scenario_fork_thread_local_orphan, diff --git a/tests/test_unit_tests.py b/tests/test_unit_tests.py index 466d5a81..0056789a 100644 --- a/tests/test_unit_tests.py +++ b/tests/test_unit_tests.py @@ -7688,6 +7688,10 @@ def test_reader_with_fragment_null_return_consumes_self(self): reader = Reader("video/mp4", init) consumed_handle = reader._handle + # The mock sets no error, which no native path does. Plant an + # operation-style one so a stale tag from another test is not read. + c2pa_module._lib.c2pa_error_set_last(b"Other: mocked null return") + real_call = c2pa_module._lib.c2pa_reader_with_fragment c2pa_module._lib.c2pa_reader_with_fragment = ( lambda r, f, s, frag: None) @@ -7739,6 +7743,408 @@ def _raise(*_args): self.assertEqual(self._free_count(freed, consumed_handle), 0, "close() freed a handle the FFI already consumed") + # Consume-and-return ownership: the native call takes the handle partway + # through its body, so a null return does not say on its own whether the + # handle was consumed. These pin the classification down. + + @staticmethod + def _is_pre_consume_rejection(error_message): + """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) + + def _stale_reader_handle(self): + """A freed, untracked pointer, captured before close() nulls it. + + Take a fresh one per call: recycled addresses become tracked again. + """ + init_path = os.path.join(FIXTURES_DIR, "dashinit.mp4") + with open(init_path, "rb") as init: + victim = Reader("video/mp4", init) + stale = ctypes.cast(victim._handle, + ctypes.POINTER(c2pa_module.C2paReader)) + victim.close() + return stale + + @staticmethod + def _untracked_reader_handle(): + """A pointer the native registry never handed out. + + Rejected like a stale handle, but not a freed address, so it cannot + be recycled and start passing the registry lookup. + """ + buf = ctypes.create_string_buffer(64) + return (ctypes.cast(buf, ctypes.POINTER(c2pa_module.C2paReader)), + buf) + + def test_with_fragment_pre_consume_rejection_keeps_handle(self): + # Rejected before Box::from_raw, so nothing was consumed and the + # handle is still ours. Dropping it here would leak it. + 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") + fragment_path = os.path.join(FIXTURES_DIR, "dash1.m4s") + + for _ in range(25): + 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): + reader.with_fragment("video/mp4", init, frag) + finally: + reader._handle = real_handle + # Still owns a working handle every time round, so nothing + # leaked: a dropped handle would leave the reader closed. + self.assertEqual(reader._lifecycle_state, LifecycleState.ACTIVE) + self.assertTrue(reader.json()) + reader.close() + + def test_with_archive_post_consume_failure_consumes_handle(self): + # Ownership taken, then the operation failed: the handle is gone, + # so close() must not free it again. + builder = Builder(json.dumps( + {"claim_generator_info": [{"name": "test", "version": "0.1"}], + "assertions": []})) + consumed_handle = builder._handle + + with self.assertRaises(Error): + builder.with_archive(io.BytesIO(b"not a valid archive")) + + self.assertIsNone(builder._handle) + self.assertEqual(builder._lifecycle_state, LifecycleState.CLOSED) + + freed = self._instrument_frees() + builder.close() + self.assertEqual(self._free_count(freed, consumed_handle), 0, + "close() freed a handle the FFI already consumed") + + def test_with_fragment_marshalling_error_keeps_handle(self): + # Never reaches native code, so nothing was consumed. The old + # blanket except marked it consumed here and leaked the handle. + init_path = os.path.join(FIXTURES_DIR, "dashinit.mp4") + with open(init_path, "rb") as init: + reader = Reader("video/mp4", init) + real_handle = reader._handle + + def _bad_marshalling(*_args): + raise ctypes.ArgumentError("wrong argument type") + + real_call = c2pa_module._lib.c2pa_reader_with_fragment + c2pa_module._lib.c2pa_reader_with_fragment = _bad_marshalling + try: + with open(init_path, "rb") as init, \ + open(init_path, "rb") as frag: + with self.assertRaises(ctypes.ArgumentError): + reader.with_fragment("video/mp4", init, frag) + finally: + c2pa_module._lib.c2pa_reader_with_fragment = real_call + + self.assertIs(reader._handle, real_handle) + self.assertEqual(reader._lifecycle_state, LifecycleState.ACTIVE) + self.assertTrue(reader.json(), + "a marshalling failure never reached the FFI, so " + "the reader must still be usable") + + reader.close() + + def test_unknown_failure_drops_handle_without_freeing(self): + # Ownership unknowable, so the handle is let go rather than freed. + # Needs a mock: every real null return sets an error. + init_path = os.path.join(FIXTURES_DIR, "dashinit.mp4") + fragment_path = os.path.join(FIXTURES_DIR, "dash1.m4s") + with open(init_path, "rb") as init: + reader = Reader("video/mp4", init) + + consumed_handle = reader._handle + # Not a pre-consume tag, so the mocked failure reads as unplaceable. + c2pa_module._lib.c2pa_error_set_last(b"Other: mocked null return") + real_call = c2pa_module._lib.c2pa_reader_with_fragment + c2pa_module._lib.c2pa_reader_with_fragment = ( + lambda r, f, s, frag: None) + try: + with open(init_path, "rb") as init, \ + open(fragment_path, "rb") as frag: + with self.assertRaises(Error): + reader.with_fragment("video/mp4", init, frag) + finally: + c2pa_module._lib.c2pa_reader_with_fragment = real_call + + # Unplaceable, so the handle is let go rather than freed twice. + self.assertIsNone(reader._handle) + self.assertEqual(reader._lifecycle_state, LifecycleState.CLOSED) + + freed = self._instrument_frees() + reader.close() + self.assertEqual(self._free_count(freed, consumed_handle), 0, + "close() freed a handle of unknown ownership") + + def test_pre_consume_rejection_is_typed(self): + # Rejections arrive wrapped as "Other: UntrackedPointer: 0x...". + self.assertTrue(self._is_pre_consume_rejection( + "Other: UntrackedPointer: 0x600001234567")) + self.assertTrue(self._is_pre_consume_rejection( + "Other: WrongPointerType: 0x600001234567")) + self.assertFalse(self._is_pre_consume_rejection( + "Verify: invalid JUMBF header")) + self.assertFalse(self._is_pre_consume_rejection(None)) + self.assertFalse(self._is_pre_consume_rejection("")) + + def test_pre_consume_tags_still_match_the_native_wording(self): + # Classification keys on error text (the numeric code is not + # exported), so a native rename would silently misjudge ownership. + 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 + + 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}") + reader.close() + + def test_stale_handle_is_actually_rejected_every_time(self): + # A handle that stopped being rejected would quietly measure the + # success path. Uses the never-allocated buffer: freed addresses get + # recycled and start passing the registry lookup. + init_path = os.path.join(FIXTURES_DIR, "dashinit.mp4") + fragment_path = os.path.join(FIXTURES_DIR, "dash1.m4s") + + for _ in range(25): + with open(init_path, "rb") as init: + reader = Reader("video/mp4", init) + real_handle = reader._handle + bogus, _buf = self._untracked_reader_handle() + reader._handle = bogus + 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) + self.assertTrue( + self._is_pre_consume_rejection( + str(caught.exception)), + "the bogus handle was not rejected, so this stopped " + "exercising the pre-consume path") + finally: + reader._handle = real_handle + reader.close() + + def test_perf_scenario_bogus_handle_is_rejected(self): + # The perf scenarios use a plain buffer so looping does not swamp + # the measurement. It still has to produce a real rejection. + 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 + + bogus, _buf = self._untracked_reader_handle() + reader._handle = bogus + 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.assertTrue( + self._is_pre_consume_rejection(str(caught.exception)), + "the perf scenarios' bogus handle is no longer 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) + self.assertTrue(reader.json()) + 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. + 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 "") + + # Pre-consume rejection: reports UntrackedPointer, not NotSupported. + 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)) + self.assertNotIn("NotSupported", str(caught.exception)) + reader.close() + + # Post-consume failure: reports the operation error, not the + # UntrackedPointer left by the step above. + builder = Builder(json.dumps( + {"claim_generator_info": [{"name": "test", "version": "0.1"}], + "assertions": []})) + with self.assertRaises(Error) as caught: + builder.with_archive(io.BytesIO(b"not a valid archive")) + self.assertNotIn("UntrackedPointer", str(caught.exception)) + builder.close() + + def test_pre_consume_classification_holds_across_threads(self): + # The error slot is thread-local, so concurrent calls do not mask + # each other's errors and misjudge ownership. + init_path = os.path.join(FIXTURES_DIR, "dashinit.mp4") + fragment_path = os.path.join(FIXTURES_DIR, "dash1.m4s") + init_bytes = open(init_path, "rb").read() + frag_bytes = open(fragment_path, "rb").read() + problems = [] + + def worker(): + for _ in range(10): + reader = Reader("video/mp4", io.BytesIO(init_bytes)) + real_handle = reader._handle + # A never-allocated buffer, not a freed pointer: with several + # threads churning the allocator, a freed address gets reused + # and starts passing the registry lookup, which would end the + # iteration in a real consume instead of a rejection. + bogus, _buf = self._untracked_reader_handle() + reader._handle = bogus + try: + reader.with_fragment("video/mp4", + io.BytesIO(init_bytes), + io.BytesIO(frag_bytes)) + problems.append("rejection did not raise") + except Error as e: + if not self._is_pre_consume_rejection(str(e)): + problems.append(f"misclassified: {str(e)[:60]}") + finally: + reader._handle = real_handle + if reader._lifecycle_state != LifecycleState.ACTIVE: + problems.append("handle dropped on a rejection") + reader.close() + + threads = [threading.Thread(target=worker) for _ in range(6)] + for t in threads: + t.start() + for t in threads: + t.join() + + 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. + original = c2pa_module._lib.c2pa_error + empty = ctypes.create_string_buffer(b"") + + try: + c2pa_module._lib.c2pa_error = lambda: ctypes.cast( + empty, ctypes.c_void_p).value + self.assertIsNone( + c2pa_module._read_native_error(), + "an empty native message must read as None, otherwise " + "callers' 'if error:' checks are only accidentally right") + 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. + init_path = os.path.join(FIXTURES_DIR, "dashinit.mp4") + fragment_path = os.path.join(FIXTURES_DIR, "dash1.m4s") + + c2pa_module._lib.c2pa_error_set_last( + b"UntrackedPointer: 0xdeadbeef") + + with open(init_path, "rb") as init: + reader = Reader("video/mp4", init) + + real_call = c2pa_module._lib.c2pa_reader_with_fragment + c2pa_module._lib.c2pa_reader_with_fragment = ( + lambda r, f, s, frag: None) + try: + with open(init_path, "rb") as init, \ + open(fragment_path, "rb") as frag: + with self.assertRaises(Error): + 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() + # 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 # only tests that drive the primitive as the generic entry point it is. @@ -8002,6 +8408,252 @@ def _boom(*args, **kwargs): self.assertEqual(len(freed), 1, "from_archive leaked the handle when the wrap failed") + # Interrupt paths. + # + # These handlers catch BaseException rather than Exception, so a + # KeyboardInterrupt arriving mid-call still runs the cleanup. Catching + # Exception would let the interrupt escape past the free/close. + + def test_interrupted_context_build_frees_builder_ptr(self): + # An interrupt between c2pa_context_builder_new and the build call + # leaves builder_ptr owned by nobody but this frame. + freed = self._instrument_frees() + real_build = c2pa_module._lib.c2pa_context_builder_build + + def _interrupt(ptr): + raise KeyboardInterrupt + + c2pa_module._lib.c2pa_context_builder_build = _interrupt + try: + with self.assertRaises(KeyboardInterrupt): + Context(signer=self._ctx_make_signer()) + finally: + c2pa_module._lib.c2pa_context_builder_build = real_build + + self.assertEqual(len(freed), 1, + "interrupted Context build leaked the context " + "builder pointer") + + def test_interrupted_sign_closes_builder(self): + # sign() borrows the Builder and closes it afterwards. An interrupt + # must not skip that close, or the handle outlives the object. + builder = Builder(self.test_manifest) + signer = self._ctx_make_signer() + self.addCleanup(signer.close) + with open(os.path.join(FIXTURES_DIR, "C.jpg"), "rb") as f: + source_bytes = f.read() + + real_sign = c2pa_module._lib.c2pa_builder_sign + + def _interrupt(*args): + raise KeyboardInterrupt + + c2pa_module._lib.c2pa_builder_sign = _interrupt + try: + # With `except Exception` the KeyboardInterrupt escapes unhandled + # and self.close() never runs; the BaseException handler catches + # it and re-raises it wrapped, having closed the Builder first. + with self.assertRaises(Error): + builder.sign(signer, "image/jpeg", + io.BytesIO(source_bytes), io.BytesIO()) + finally: + c2pa_module._lib.c2pa_builder_sign = real_sign + + self.assertEqual(builder._lifecycle_state, LifecycleState.CLOSED, + "interrupted sign left the Builder open") + self.assertIsNone(builder._handle) + + def test_sign_failure_chains_the_original_exception(self): + # The wrapper re-raises as C2paError; losing __cause__ hides which + # call actually failed. + builder = Builder(self.test_manifest) + signer = self._ctx_make_signer() + self.addCleanup(signer.close) + + sentinel = RuntimeError("native call blew up") + real_sign = c2pa_module._lib.c2pa_builder_sign + + def _boom(*args): + raise sentinel + + c2pa_module._lib.c2pa_builder_sign = _boom + try: + with self.assertRaises(Error) as ctx: + builder.sign(signer, "image/jpeg", + io.BytesIO(b"x"), io.BytesIO()) + finally: + c2pa_module._lib.c2pa_builder_sign = real_sign + + self.assertIs(ctx.exception.__cause__, sentinel, + "signing error dropped the original exception") + + def test_marshalling_error_on_set_signer_leaves_signer_owned(self): + # ctypes.ArgumentError means the call never reached the native side, + # so the signer was never taken and must keep owning its handle. + # + # This passes with or without the explicit ArgumentError re-raise at + # the call site, because a bare re-raise matches what happens with no + # handler at all. It pins the invariant against a future edit + # that adds work between the FFI call and _mark_consumed(), which would + # otherwise start marking an untaken signer as consumed. + signer = self._ctx_make_signer() + self.addCleanup(signer.close) + real_set = c2pa_module._lib.c2pa_context_builder_set_signer + + def _bad_marshal(builder_ptr, signer_ptr): + raise ctypes.ArgumentError("bad argument type") + + c2pa_module._lib.c2pa_context_builder_set_signer = _bad_marshal + try: + with self.assertRaises(ctypes.ArgumentError): + Context(signer=signer) + finally: + c2pa_module._lib.c2pa_context_builder_set_signer = real_set + + self.assertIsNotNone( + signer._handle, + "a signer the native side never took was marked consumed") + self.assertEqual(signer._lifecycle_state, LifecycleState.ACTIVE) + + +class TestErrorPlumbing(unittest.TestCase): + """Covers the error helpers themselves, which had no direct tests.""" + + def _set_native_error(self, text): + c2pa_module._lib.c2pa_error_set_last(text.encode('utf-8')) + + def test_every_error_tag_maps_to_its_typed_subclass(self): + # The wire text is "Tag: message"; each tag gets its own subclass so + # callers can catch precisely. + tags = [ + "Assertion", "AssertionNotFound", "Decoding", "Encoding", + "FileNotFound", "Io", "Json", "Manifest", "ManifestNotFound", + "NotSupported", "Other", "RemoteManifest", "ResourceNotFound", + "Signature", "Verify", "UntrackedPointer", "WrongPointerType", + ] + for tag in tags: + with self.subTest(tag=tag): + expected = getattr(Error, tag) + with self.assertRaises(expected): + c2pa_module._raise_typed_c2pa_error(f"{tag}: detail") + + def test_unmapped_tag_falls_back_to_base_error(self): + with self.assertRaises(Error) as ctx: + c2pa_module._raise_typed_c2pa_error("Nonsense: detail") + # Base class only: no subclass should claim an unknown tag. + self.assertIs(type(ctx.exception), Error) + + def test_check_ffi_operation_result_raises_with_native_message(self): + self._set_native_error("Io: disk exploded") + with self.assertRaises(Error) as ctx: + c2pa_module._check_ffi_operation_result(None, "fallback text") + self.assertIn("disk exploded", str(ctx.exception)) + + def test_check_ffi_operation_result_uses_fallback_when_slot_empty(self): + # The slot is sticky and thread-local, so a fresh thread is the only + # way to observe it unset. + captured = [] + + def run(): + try: + c2pa_module._check_ffi_operation_result(None, "fallback text") + except BaseException as e: # noqa: BLE001 - reported below + captured.append(e) + + t = threading.Thread(target=run) + t.start() + t.join() + + self.assertEqual(len(captured), 1, "expected a raise on failure") + self.assertIsInstance(captured[0], Error) + self.assertIn("fallback text", str(captured[0])) + + def test_check_ffi_operation_result_passes_success_through(self): + self.assertEqual( + c2pa_module._check_ffi_operation_result(42, "unused"), 42) + + def test_stream_creation_failure_reports_a_real_message(self): + # Regression: used to raise bare Exception("...: None"), because + # _parse_operation_result_for_error never returns a message. + real = c2pa_module._lib.c2pa_create_stream + c2pa_module._lib.c2pa_create_stream = lambda *a: None + try: + # With an error set, the message must carry it. + self._set_native_error("Io: stream refused") + with self.assertRaises(Error) as ctx: + c2pa_module.Stream(io.BytesIO(b"x")) + self.assertIn("stream refused", str(ctx.exception)) + + # With the slot unset, the old code raised bare Exception. + captured = [] + + def run(): + try: + c2pa_module.Stream(io.BytesIO(b"x")) + except BaseException as e: # noqa: BLE001 - reported below + captured.append(e) + + t = threading.Thread(target=run) + t.start() + 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.assertNotIn("None", str(captured[0])) + finally: + c2pa_module._lib.c2pa_create_stream = real + + def test_supported_mime_types_raises_instead_of_returning_empty(self): + # Regression: `if error:` was always False, so a native failure + # returned an empty list instead of raising. Only visible with the + # slot unset, hence the fresh thread. + captured = [] + + def run(): + try: + captured.append( + c2pa_module._get_supported_mime_types( + lambda count: None, None)) + except BaseException as e: # noqa: BLE001 - reported below + captured.append(e) + + t = threading.Thread(target=run) + t.start() + t.join() + + self.assertIsInstance( + captured[0], Error, + "a failed MIME lookup returned data instead of raising") + + def test_supported_mime_types_reports_the_native_message(self): + self._set_native_error("Io: mime lookup failed") + with self.assertRaises(Error) as ctx: + c2pa_module._get_supported_mime_types(lambda count: None, None) + self.assertIn("mime lookup failed", str(ctx.exception)) + + +class TestErrorsStillRaiseAfterCleanup(unittest.TestCase): + """Each surface that lost a _clear_error_state() call still reports.""" + + def test_reader_on_garbage_raises(self): + with self.assertRaises(Error): + Reader("image/jpeg", io.BytesIO(b"not an image")).json() + + def test_signer_from_info_with_bad_certs_raises(self): + with self.assertRaises(Error): + c2pa_module.create_signer_from_info(C2paSignerInfo( + alg=b"es256", + sign_cert=b"not a certificate", + private_key=b"not a key", + ta_url=b"", + )) + + def test_ed25519_sign_with_empty_data_raises(self): + with self.assertRaises(Error): + c2pa_module.ed25519_sign(b"", "not a key") + if __name__ == '__main__': unittest.main(warnings='ignore') From c24ede6d0b7dc12a52416b216e27a7391a6902a6 Mon Sep 17 00:00:00 2001 From: tmathern <60901087+tmathern@users.noreply.github.com> Date: Sun, 19 Jul 2026 20:25:21 -0700 Subject: [PATCH 15/20] fix: Refactor --- src/c2pa/c2pa.py | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index ee06d265..7484af7d 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -493,9 +493,6 @@ def _cleanup_resources(self): and self._lifecycle_state != LifecycleState.CLOSED ): self._lifecycle_state = LifecycleState.CLOSED - # A failing _release() must not skip the free below: - # that would strand the native handle on an object already - # marked CLOSED, making it unreachable and unfreeable. self._safe_release() if hasattr(self, '_handle') and self._handle: try: @@ -1691,8 +1688,7 @@ def __init__( if signer is not None: signer._ensure_valid_state() # c2pa_context_builder_set_signer takes ownership of the - # signer pointer immediately (Box::from_raw), on its error - # path as well as on success. + # signer pointer , on its error path as well as on success. self._signer_callback_cb = signer._callback_cb try: result = ( @@ -2087,11 +2083,6 @@ def close(self): if self._closed: return if is_foreign_process(self): - # Unlike ManagedResource, which leaves a child's copy active - # because the parent still owns the pointer, a Stream's callbacks - # are bound to the parent's objects. - # The child's copy can never be used, so mark it closed - # and let the parent free it. self._closed = True self._initialized = False return From 359ce9862fcf3dee33ec84aacc1717d34c35352e Mon Sep 17 00:00:00 2001 From: tmathern <60901087+tmathern@users.noreply.github.com> Date: Sun, 19 Jul 2026 20:35:21 -0700 Subject: [PATCH 16/20] fix: Refactor 3 --- src/c2pa/c2pa.py | 51 +++++++----------------- tests/perf/scenarios.py | 40 ------------------- tests/test_unit_tests.py | 83 ---------------------------------------- 3 files changed, 14 insertions(+), 160 deletions(-) diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index 7484af7d..1b020ee2 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -302,16 +302,11 @@ def _safe_release(self): ) def _mark_consumed(self): - """Mark as consumed by an FFI call that took ownership of the native - handle. - - Ownership contract: the native pointer is now owned elsewhere, so this - does not free it. It releases only Python-side resources (via - `_release()`: callback refs, streams, caches) and marks the resource - closed. Marking it closed stops `_cleanup_resources` from freeing the - now foreign-owned pointer later; releasing here means those Python - resources are not stranded until garbage collection. + """Mark as consumed by an FFI call that took ownership + of native resources e.g. pointers. This means we should not + call clean-up here anymore, and leave it to the new owner. """ + if is_foreign_process(self): self._handle = None self._lifecycle_state = LifecycleState.CLOSED @@ -326,10 +321,7 @@ def _mark_consumed(self): def _activate(self, handle): """Attach a native handle to self and mark it active. - - Ownership of `handle` transfers here: this object frees it on close. - Only an uninitialized resource can be activated, so a handle can never - be activated twice and a closed resource can never be reopened. + Ownership of `handle` transfers here. Args: handle: Non-null native pointer to take ownership of @@ -353,16 +345,9 @@ def _activate(self, handle): def _swap_handle(self, new_handle): """Replace the handle after an FFI call consumed the old one and returned a replacement. - - Ownership contract: the caller guarantees the callee has already - consumed and freed the old pointer. This method never frees the old - pointer; it only rebinds `self._handle` to the replacement. Calling it - when the old pointer was not consumed leaks that pointer. - 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. Args: @@ -389,15 +374,11 @@ def _swap_handle(self, new_handle): def _consume_and_swap(self, ffi_call, error_message): """Run an FFI call that consumes this handle and returns a replacement. - The native lib takes ownership partway through the call, - so a null return can be ambiguous. - The native error tells the cases apart: + The native lib may take ownership partway through the call. + The native error tells if ownership was transferred: - a pointer rejection (`_PRE_CONSUME_ERROR_TAGS`) precedes the transfer, so the handle is still ours and is kept; - - any other error means it was taken, then the operation failed; - - a null with no error cannot be placed, so the handle is dropped - anyway: leaking is recoverable, double-freeing is not. No native - path does this, so it only appears under mocks. + - any other error means it was taken, then the operation failed. The error is read without clearing it first. The slot is sticky: c2pa_error() peeks and nothing empties it. @@ -429,8 +410,8 @@ def _consume_and_swap(self, ffi_call, error_message): if error: if any(tag in error for tag in ManagedResource._PRE_CONSUME_ERROR_TAGS): - # Rejected before ownership transferred, so the handle is - # still ours: keep it and let normal cleanup free it. + # Rejected before ownership transferred, + # so the handle is still ours. logger.warning( "%s: native call rejected the handle before taking " "ownership (%s); handle retained", @@ -438,7 +419,7 @@ def _consume_and_swap(self, ffi_call, error_message): error) _raise_typed_c2pa_error(error) - # Ownership transferred and then the operation failed. + # Ownership transferred and then an operation failed. self._mark_consumed() _raise_typed_c2pa_error(error) @@ -448,14 +429,11 @@ def _consume_and_swap(self, ffi_call, error_message): @classmethod def _wrap_native_handle(cls, handle): """Build a brand-new instance around an already-valid, - already-owned native handle, bypassing __init__ entirely. - - __init__ is bypassed, so `_init_attrs()` supplies the class's own - defaults and the instance is stamped with the creating process. + already-owned native handle (bypassing __init__). Everything an instance needs besides the native handle must be set in - `_init_attrs()`, not `__init__` — `_wrap_native_handle` never runs - `__init__`. An attribute a subclass sets only in `__init__` will be + `_init_attrs()`. `_wrap_native_handle` never runs `__init__`. + An attribute a subclass sets only in `__init__` will be missing (or left at its `_init_attrs` default) on a wrapped instance. Ownership of `handle` transfers only on successful return. If this @@ -468,7 +446,6 @@ def _wrap_native_handle(cls, handle): C2paError: If the handle is null """ obj = object.__new__(cls) - # Stamps _owner_pid, which the fork guard relies on. ManagedResource.__init__(obj) obj._init_attrs() obj._activate(handle) diff --git a/tests/perf/scenarios.py b/tests/perf/scenarios.py index 6166f6f6..23300aed 100644 --- a/tests/perf/scenarios.py +++ b/tests/perf/scenarios.py @@ -908,45 +908,6 @@ def scenario_builder_from_context_construction(iterations: int = 100) -> None: builder.close() -def scenario_sign_interrupted(iterations: int = 100) -> None: - """Loop a sign that is interrupted partway through the FFI call. - - _sign_internal catches BaseException so the Builder is closed even when - the interrupt is not an Exception subclass. Narrowing that back to - Exception leaks the whole Builder per iteration, since close() is skipped - and the handle outlives the object. - """ - source_bytes = SOURCE_JPEG.read_bytes() - signer = _make_signer() - real_sign = c2pa_module._lib.c2pa_builder_sign - - def _interrupt(*args): - raise KeyboardInterrupt - - c2pa_module._lib.c2pa_builder_sign = _interrupt - try: - for _ in _iterate(iterations): - builder = Builder(MANIFEST_BASE) - try: - builder.sign(signer, "image/jpeg", - io.BytesIO(source_bytes), io.BytesIO()) - except C2paError: - # The wrapper re-raises the interrupt as C2paError, having - # closed the Builder first. Anything else means the handler - # stopped catching it. - pass - else: - raise AssertionError("interrupted sign did not raise") - if (builder._lifecycle_state - is not c2pa_module.LifecycleState.CLOSED): - raise AssertionError( - "interrupted sign left the Builder open; its handle " - "leaks once per iteration") - finally: - c2pa_module._lib.c2pa_builder_sign = real_sign - signer.close() - - def scenario_signer_construction(iterations: int = 100) -> None: """Loop Signer.from_info()/__init__ construction and teardown. @@ -1428,7 +1389,6 @@ def scenario_fork_stream_cleanup(iterations: int = 100) -> None: "signer_construction": scenario_signer_construction, "builder_from_context_construction": scenario_builder_from_context_construction, - "sign_interrupted": scenario_sign_interrupted, "fork_reader_collect": scenario_fork_reader_collect, "fork_contended_mutex": scenario_fork_contended_mutex, "fork_thread_local_orphan": scenario_fork_thread_local_orphan, diff --git a/tests/test_unit_tests.py b/tests/test_unit_tests.py index 0056789a..59a68ee8 100644 --- a/tests/test_unit_tests.py +++ b/tests/test_unit_tests.py @@ -8408,61 +8408,6 @@ def _boom(*args, **kwargs): self.assertEqual(len(freed), 1, "from_archive leaked the handle when the wrap failed") - # Interrupt paths. - # - # These handlers catch BaseException rather than Exception, so a - # KeyboardInterrupt arriving mid-call still runs the cleanup. Catching - # Exception would let the interrupt escape past the free/close. - - def test_interrupted_context_build_frees_builder_ptr(self): - # An interrupt between c2pa_context_builder_new and the build call - # leaves builder_ptr owned by nobody but this frame. - freed = self._instrument_frees() - real_build = c2pa_module._lib.c2pa_context_builder_build - - def _interrupt(ptr): - raise KeyboardInterrupt - - c2pa_module._lib.c2pa_context_builder_build = _interrupt - try: - with self.assertRaises(KeyboardInterrupt): - Context(signer=self._ctx_make_signer()) - finally: - c2pa_module._lib.c2pa_context_builder_build = real_build - - self.assertEqual(len(freed), 1, - "interrupted Context build leaked the context " - "builder pointer") - - def test_interrupted_sign_closes_builder(self): - # sign() borrows the Builder and closes it afterwards. An interrupt - # must not skip that close, or the handle outlives the object. - builder = Builder(self.test_manifest) - signer = self._ctx_make_signer() - self.addCleanup(signer.close) - with open(os.path.join(FIXTURES_DIR, "C.jpg"), "rb") as f: - source_bytes = f.read() - - real_sign = c2pa_module._lib.c2pa_builder_sign - - def _interrupt(*args): - raise KeyboardInterrupt - - c2pa_module._lib.c2pa_builder_sign = _interrupt - try: - # With `except Exception` the KeyboardInterrupt escapes unhandled - # and self.close() never runs; the BaseException handler catches - # it and re-raises it wrapped, having closed the Builder first. - with self.assertRaises(Error): - builder.sign(signer, "image/jpeg", - io.BytesIO(source_bytes), io.BytesIO()) - finally: - c2pa_module._lib.c2pa_builder_sign = real_sign - - self.assertEqual(builder._lifecycle_state, LifecycleState.CLOSED, - "interrupted sign left the Builder open") - self.assertIsNone(builder._handle) - def test_sign_failure_chains_the_original_exception(self): # The wrapper re-raises as C2paError; losing __cause__ hides which # call actually failed. @@ -8487,34 +8432,6 @@ def _boom(*args): self.assertIs(ctx.exception.__cause__, sentinel, "signing error dropped the original exception") - def test_marshalling_error_on_set_signer_leaves_signer_owned(self): - # ctypes.ArgumentError means the call never reached the native side, - # so the signer was never taken and must keep owning its handle. - # - # This passes with or without the explicit ArgumentError re-raise at - # the call site, because a bare re-raise matches what happens with no - # handler at all. It pins the invariant against a future edit - # that adds work between the FFI call and _mark_consumed(), which would - # otherwise start marking an untaken signer as consumed. - signer = self._ctx_make_signer() - self.addCleanup(signer.close) - real_set = c2pa_module._lib.c2pa_context_builder_set_signer - - def _bad_marshal(builder_ptr, signer_ptr): - raise ctypes.ArgumentError("bad argument type") - - c2pa_module._lib.c2pa_context_builder_set_signer = _bad_marshal - try: - with self.assertRaises(ctypes.ArgumentError): - Context(signer=signer) - finally: - c2pa_module._lib.c2pa_context_builder_set_signer = real_set - - self.assertIsNotNone( - signer._handle, - "a signer the native side never took was marked consumed") - self.assertEqual(signer._lifecycle_state, LifecycleState.ACTIVE) - class TestErrorPlumbing(unittest.TestCase): """Covers the error helpers themselves, which had no direct tests.""" From 85b6bfd973cf11eb8ff53e83f58ba7eb2509c659 Mon Sep 17 00:00:00 2001 From: tmathern <60901087+tmathern@users.noreply.github.com> Date: Sun, 19 Jul 2026 21:21:17 -0700 Subject: [PATCH 17/20] fix: Refactor once more --- src/c2pa/c2pa.py | 9 +---- tests/test_unit_tests.py | 61 ++++++++++++------------------- tests/test_unit_tests_threaded.py | 50 +++---------------------- 3 files changed, 31 insertions(+), 89 deletions(-) diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index 1b020ee2..d631dbee 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -2502,7 +2502,7 @@ def _init_from_context(self, context, format_or_path, raise # Adopt the handle before the consuming call: _consume_and_swap - # needs an ACTIVE resource, and from here on normal cleanup owns + # needs an active resource, and from here on normal cleanup owns # the pointer whichever way the call goes. self._activate(reader_ptr) @@ -2572,9 +2572,6 @@ def _close_streams(self): def _release(self): """Release Reader-specific resources (caches, stream, backing file). - - Every teardown path runs this, including _mark_consumed(), which - close() never gets a chance to follow. """ self._manifest_json_str_cache = None self._manifest_data_cache = None @@ -3300,7 +3297,7 @@ def _init_from_context(self, context, json_str): raise # Adopt the handle before the consuming call: _consume_and_swap needs - # an ACTIVE resource, and from here on normal cleanup owns the pointer + # an active resource, and from here on normal cleanup owns the pointer # whichever way the call goes. self._activate(builder_ptr) @@ -3657,8 +3654,6 @@ def _sign_internal( # and single use/single sign done by a Builder. self.close() except BaseException as e: - # BaseException, not Exception: a KeyboardInterrupt mid-sign must - # still close the Builder rather than leaving its handle live. self.close() raise C2paError(f"Error during signing: {e}") from e diff --git a/tests/test_unit_tests.py b/tests/test_unit_tests.py index 59a68ee8..f440d115 100644 --- a/tests/test_unit_tests.py +++ b/tests/test_unit_tests.py @@ -6935,8 +6935,8 @@ class TestManagedResourceLifecycle(unittest.TestCase): the ownership hand-offs between Python and the native library. setUp records frees instead of performing them, so a miscount reads as a - leak or a double-free rather than a crash. Tests holding real handles - call _use_real_frees() first. + leak or a double-free rather than a crash. + Tests holding real handles call _use_real_frees() first. """ class _FakeHandleResource(ManagedResource): @@ -6961,7 +6961,7 @@ def _release(self): self.release_calls += 1 class _ExtenderResource(ManagedResource): - """A downstream extender that owns a raw handle and wraps it via + """Am extender that owns a raw handle and wraps it via _wrap_native_handle. It carries several attributes of its own, all defaulted in _init_attrs (not __init__), and _release reads them, so a missing attribute would surface as an AttributeError on teardown. @@ -6994,7 +6994,7 @@ def _free_counts(self): return counts def _use_real_frees(self): - """Undo setUp's recorder, so native handles are really freed.""" + """Undo free recorder, so native handles are really freed.""" ManagedResource._free_native_ptr = self._real_free def _make_signer(self): @@ -7005,8 +7005,6 @@ def _make_signer(self): return Signer.from_info(C2paSignerInfo( b"es256", certs, key, b"http://timestamp.digicert.com")) - # _cleanup_resources: a failing _release() must not strand the handle - def test_release_failure_still_frees_handle(self): res = self._CallbackHoldingResource() # _callback_cb is never set, so _release() raises AttributeError. @@ -7029,8 +7027,6 @@ def test_release_failure_is_logged(self): any('Failed to release' in line for line in captured.output), f"_release() failure was not logged: {captured.output}") - # _activate guards - def test_activate_rejects_null_handle(self): res = self._FakeHandleResource() @@ -7078,16 +7074,14 @@ def test_activate_does_not_mutate_on_rejection(self): "rejected activation replaced the handle") self.assertEqual(res._lifecycle_state, LifecycleState.ACTIVE) - # _swap_handle - 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, so freeing it here - # would be a double-free. + # The FFI already owns and frees the old pointer, + # so freeing it here would be a double-free. self.assertEqual(self.freed, []) self.assertEqual(res._handle, 0xAAA2) @@ -7117,8 +7111,6 @@ def test_swap_handle_rejects_null_replacement(self): self.assertEqual(res._handle, 0x7777) self.assertEqual(res._lifecycle_state, LifecycleState.ACTIVE) - # _wrap_native_handle - def test_wrap_native_handle_bypasses_init(self): seen = [] @@ -7138,7 +7130,6 @@ def _release(self): self.assertEqual(obj._tag, 'from _init_attrs') self.assertEqual(obj._lifecycle_state, LifecycleState.ACTIVE) self.assertTrue(obj.is_valid) - # ManagedResource.__init__ still ran, so fork-safety is intact. self.assertTrue(hasattr(obj, '_owner_pid')) obj.close() @@ -7191,8 +7182,7 @@ def test_foreign_child_skips_free_for_wrapped_and_swapped(self): "forked child freed a pointer its parent still owns") # The child must not free a pointer the parent still owns. # The child does mark its own copies closed and nulls their handles, - # which is safe (the parent holds a separate copy) and - # stops the child from reusing a parent-owned handle. + # which stops the child from reusing a parent-owned handle. self.assertEqual(wrapped._lifecycle_state, LifecycleState.CLOSED) self.assertIsNone(wrapped._handle) self.assertEqual(swapped._lifecycle_state, LifecycleState.CLOSED) @@ -7244,8 +7234,7 @@ def test_consumed_resource_frees_nothing_in_either_process(self): self.assertEqual(self.freed, []) # Consuming a handle hands the native pointer to a new owner, - # but the Python-side resources are still ours to let go of. - + # but the Python-side resources are still ours and we need to free. def test_mark_consumed_releases_python_resources(self): res = self._ReleaseRecordingResource() res._activate(0xF1) @@ -7282,7 +7271,7 @@ def test_extender_wraps_handle_fully_built(self): obj = self._ExtenderResource._wrap_native_handle(0xE0) # Every attribute _init_attrs defaults is present, - # even thoug __init__ never ran. + # even though __init__ never ran. self.assertEqual(obj.label, "extender") self.assertEqual(obj.buffer, []) self.assertFalse(obj.released) @@ -7301,8 +7290,7 @@ def test_extender_foreign_teardown_skips_native_free(self): obj = self._ExtenderResource._wrap_native_handle(0xE1) # Stamp a foreign owner: # teardown runs in a process that did not create the handle, - # so it must not free the pointer or run _release (which could touch - # native streams and deadlock after a multithreaded fork). + # so it must not free the pointer or run _release. obj._owner_pid = os.getpid() + 1 obj.close() @@ -7342,10 +7330,6 @@ def test_builder_from_archive_wraps_handle(self): self.assertEqual(builder._lifecycle_state, LifecycleState.CLOSED) def test_context_build_failure_consumes_signer(self): - # c2pa_context_builder_set_signer takes ownership of the signer - # pointer immediately, so a later build failure must still leave the - # Signer consumed. - # Otherwise it holds a pointer the native side has already freed. self._use_real_frees() signer = self._make_signer() real_build = c2pa_module._lib.c2pa_context_builder_build @@ -7401,21 +7385,22 @@ def test_construction_failure_leaves_nothing_to_free(self): finally: c2pa_module._lib.c2pa_builder_from_json = real_json -def _ptr_addr(ptr): - """Address a ctypes pointer points at, or None for a null pointer. - - ctypes pointers compare by identity, not by value: two pointer objects - for the same address are unequal. Compare addresses instead. - """ - if not ptr: - return None - return ctypes.cast(ptr, ctypes.c_void_p).value - class TestManagedResourceObjects(TestContextAPIs): """Tests native resource handling management when managed manually. """ + @staticmethod + def _ptr_addr(ptr): + """Address a ctypes pointer points at, or None for a null pointer. + + ctypes pointers compare by identity, not by value: two pointer objects + for the same address are unequal. Compare addresses instead. + """ + if not ptr: + return None + return ctypes.cast(ptr, ctypes.c_void_p).value + def _instrument_frees(self): """Record frees instead of performing them, and restore on teardown. @@ -7434,9 +7419,9 @@ def _instrument_frees(self): def _free_count(self, freed, handle): """How many times `handle` was freed, ignoring unrelated frees.""" - target = _ptr_addr(handle) + target = self._ptr_addr(handle) self.assertIsNotNone(target, "cannot count frees of a null handle") - return sum(1 for ptr in freed if _ptr_addr(ptr) == target) + return sum(1 for ptr in freed if self._ptr_addr(ptr) == target) def _make_archive(self, manifest=None): archive = io.BytesIO() diff --git a/tests/test_unit_tests_threaded.py b/tests/test_unit_tests_threaded.py index 540dfc82..e7d49b16 100644 --- a/tests/test_unit_tests_threaded.py +++ b/tests/test_unit_tests_threaded.py @@ -2984,8 +2984,7 @@ def make_and_drop(index): gc.collect() - # Count only this test's handles: resources dropped by other tests in - # the class can be collected at any point and land in self.freed. + # Count only this test's handles counts = {handle: count for handle, count in self._free_counts().items() if 0x30000 <= handle < 0x30000 + 200} @@ -2994,20 +2993,14 @@ def make_and_drop(index): self.assertEqual(set(counts.values()), {1}, "a dropped resource was freed more than once") + def test_settings_relayed_across_threads_stays_usable(self): + ManagedResource._free_native_ptr = self._real_free -class TestSettingsAsContextAcrossThreads(unittest.TestCase): - """Tests Settings handed between threads and reused as the basis - for Contexts, using real native handles. - """ - - def setUp(self): - self.manifest = { + manifest = { "claim_generator": "threaded_stamp_test", "format": "image/jpeg", "assertions": [], } - - def test_settings_relayed_across_threads_stays_usable(self): settings = Settings() pid = os.getpid() results = [] @@ -3016,7 +3009,7 @@ def test_settings_relayed_across_threads_stays_usable(self): def build_context_and_builder(): try: context = Context(settings=settings) - builder = Builder(self.manifest, context=context) + builder = Builder(manifest, context=context) results.append(( builder._owner_pid, context._owner_pid, builder.is_valid)) builder.close() @@ -3024,7 +3017,7 @@ def build_context_and_builder(): except Exception as exc: errors.append(exc) - # Sequential hand-off: each thread owns the Settings for its turn. + # Each thread owns the Settings for its turn. for _ in range(8): thread = threading.Thread(target=build_context_and_builder) thread.start() @@ -3040,37 +3033,6 @@ def build_context_and_builder(): self.assertTrue(valid) self.assertEqual(settings._owner_pid, pid) - def test_context_created_on_one_thread_closed_on_another(self): - created = [] - errors = [] - - def create(): - try: - created.append(Context()) - except Exception as exc: - errors.append(exc) - - def close_all(): - try: - for context in created: - context.close() - except Exception as exc: - errors.append(exc) - - maker = threading.Thread(target=create) - maker.start() - maker.join() - - closer = threading.Thread(target=close_all) - closer.start() - closer.join() - - self.assertEqual(errors, []) - self.assertEqual(len(created), 1) - for context in created: - self.assertEqual(context._lifecycle_state, LifecycleState.CLOSED) - self.assertIsNone(context._handle) - if __name__ == '__main__': unittest.main() From 2725dce369fde4902c8878f6a8e72b04d2b5ada9 Mon Sep 17 00:00:00 2001 From: tmathern <60901087+tmathern@users.noreply.github.com> Date: Sun, 19 Jul 2026 21:53:21 -0700 Subject: [PATCH 18/20] fix: Baseline for new scenarios --- tests/perf/baseline.json | 343 +++++++++++++++++++++------------------ 1 file changed, 184 insertions(+), 159 deletions(-) diff --git a/tests/perf/baseline.json b/tests/perf/baseline.json index a6a75177..c151efe5 100644 --- a/tests/perf/baseline.json +++ b/tests/perf/baseline.json @@ -8,268 +8,293 @@ "arch": "aarch64" }, "reader_jpeg_legacy": { - "peak_bytes": 3830666, - "leaked_bytes": 3331494, - "total_allocations": 1365464 + "peak_bytes": 3851610, + "leaked_bytes": 3351823, + "total_allocations": 1362322 }, "reader_jpeg_with_context": { - "peak_bytes": 3824947, - "leaked_bytes": 3324219, - "total_allocations": 1354423 + "peak_bytes": 3845367, + "leaked_bytes": 3345097, + "total_allocations": 1349879 }, "reader_manifest_data_context": { - "peak_bytes": 7616236, - "leaked_bytes": 3448019, - "total_allocations": 1152354 + "peak_bytes": 7636730, + "leaked_bytes": 3468040, + "total_allocations": 1147359 }, "reader_mp4": { - "peak_bytes": 4200644, - "leaked_bytes": 3323805, - "total_allocations": 4100459 + "peak_bytes": 4222601, + "leaked_bytes": 3345724, + "total_allocations": 4095915 }, "reader_wav": { - "peak_bytes": 4501138, - "leaked_bytes": 3333747, - "total_allocations": 746935 + "peak_bytes": 4523095, + "leaked_bytes": 3355666, + "total_allocations": 742391 }, "builder_sign_jpeg_legacy": { - "peak_bytes": 8108426, - "leaked_bytes": 3485229, - "total_allocations": 1115105 + "peak_bytes": 7785129, + "leaked_bytes": 3468507, + "total_allocations": 1041412 }, "builder_sign_jpeg_with_context": { - "peak_bytes": 8108408, - "leaked_bytes": 3477526, - "total_allocations": 1103189 + "peak_bytes": 7779538, + "leaked_bytes": 3463042, + "total_allocations": 1027485 }, "builder_sign_png_legacy": { - "peak_bytes": 8002675, - "leaked_bytes": 3447638, - "total_allocations": 3887535 + "peak_bytes": 8023081, + "leaked_bytes": 3468300, + "total_allocations": 3883115 }, "builder_sign_png_with_context": { - "peak_bytes": 7994768, - "leaked_bytes": 3440151, - "total_allocations": 3875483 + "peak_bytes": 8017008, + "leaked_bytes": 3462829, + "total_allocations": 3869515 }, "builder_sign_jpeg_parallel_split_pool": { - "peak_bytes": 44042681, - "leaked_bytes": 3841702, - "total_allocations": 1045285 + "peak_bytes": 45854797, + "leaked_bytes": 3840928, + "total_allocations": 1035646 }, "builder_sign_jpeg_parallel_split_barrier": { - "peak_bytes": 45824424, - "leaked_bytes": 3840908, - "total_allocations": 1043917 + "peak_bytes": 45844809, + "leaked_bytes": 3861014, + "total_allocations": 1037741 }, "builder_sign_png_parallel_split_pool": { - "peak_bytes": 46094336, - "leaked_bytes": 3860177, - "total_allocations": 3887276 + "peak_bytes": 46586728, + "leaked_bytes": 3868054, + "total_allocations": 3877696 }, "builder_sign_png_parallel_split_barrier": { - "peak_bytes": 46062355, - "leaked_bytes": 3876678, - "total_allocations": 3885976 + "peak_bytes": 46082548, + "leaked_bytes": 3879161, + "total_allocations": 3879780 }, "builder_sign_gif": { - "peak_bytes": 14614794, - "leaked_bytes": 3439807, - "total_allocations": 17023655 + "peak_bytes": 14635465, + "leaked_bytes": 3461270, + "total_allocations": 17017654 }, "builder_sign_heic": { - "peak_bytes": 4677761, - "leaked_bytes": 3447623, - "total_allocations": 1569619 + "peak_bytes": 4698434, + "leaked_bytes": 3469086, + "total_allocations": 1563419 }, "builder_sign_m4a": { - "peak_bytes": 18812724, - "leaked_bytes": 3448162, - "total_allocations": 5200482 + "peak_bytes": 18833496, + "leaked_bytes": 3469085, + "total_allocations": 5194205 }, "builder_sign_webp": { - "peak_bytes": 8970587, - "leaked_bytes": 3440333, - "total_allocations": 922037 + "peak_bytes": 8991237, + "leaked_bytes": 3461271, + "total_allocations": 916145 }, "builder_sign_avi": { - "peak_bytes": 7110163, - "leaked_bytes": 3440347, - "total_allocations": 89988101 + "peak_bytes": 7130933, + "leaked_bytes": 3461270, + "total_allocations": 89982012 }, "builder_sign_mp4": { - "peak_bytes": 6224729, - "leaked_bytes": 3448147, - "total_allocations": 3794640 + "peak_bytes": 6245379, + "leaked_bytes": 3469085, + "total_allocations": 3788717 }, "builder_sign_tiff": { - "peak_bytes": 13192399, - "leaked_bytes": 3440348, - "total_allocations": 10868711 + "peak_bytes": 13213169, + "leaked_bytes": 3461271, + "total_allocations": 10862700 }, "builder_sign_jpeg_parent_of": { - "peak_bytes": 14244190, - "leaked_bytes": 3439412, - "total_allocations": 2514770 + "peak_bytes": 14265295, + "leaked_bytes": 3461665, + "total_allocations": 2506107 }, "builder_sign_jpeg_component_of": { - "peak_bytes": 14246389, - "leaked_bytes": 3440789, - "total_allocations": 2559666 + "peak_bytes": 14266996, + "leaked_bytes": 3462012, + "total_allocations": 2551180 }, "builder_sign_jpeg_parent_and_component": { - "peak_bytes": 14597099, - "leaked_bytes": 3546490, - "total_allocations": 4534820 + "peak_bytes": 14665241, + "leaked_bytes": 3614613, + "total_allocations": 4523960 }, "builder_sign_jpeg_parent_and_component_mixed_mime": { - "peak_bytes": 14545928, - "leaked_bytes": 3440216, - "total_allocations": 5528067 + "peak_bytes": 14568780, + "leaked_bytes": 3462718, + "total_allocations": 5517180 }, "builder_sign_jpeg_two_components_same_mime": { - "peak_bytes": 14539155, - "leaked_bytes": 3544510, - "total_allocations": 4508197 + "peak_bytes": 14559274, + "leaked_bytes": 3564233, + "total_allocations": 4497379 }, "builder_sign_jpeg_two_components_mixed_mime": { - "peak_bytes": 14544717, - "leaked_bytes": 3441158, - "total_allocations": 5501353 + "peak_bytes": 14564839, + "leaked_bytes": 3461873, + "total_allocations": 5490592 }, "builder_sign_jpeg_archive_roundtrip": { - "peak_bytes": 14277459, - "leaked_bytes": 3460332, - "total_allocations": 3480521 + "peak_bytes": 14297571, + "leaked_bytes": 3481212, + "total_allocations": 3467149 }, "builder_from_archive_roundtrip": { - "peak_bytes": 14277245, - "leaked_bytes": 3460181, - "total_allocations": 3108863 + "peak_bytes": 14297349, + "leaked_bytes": 3480475, + "total_allocations": 3101030 }, "builder_with_archive_swap": { - "peak_bytes": 3660981, - "leaked_bytes": 3330239, - "total_allocations": 708348 + "peak_bytes": 3681081, + "leaked_bytes": 3350198, + "total_allocations": 704373 }, "reader_with_fragment_swap": { - "peak_bytes": 3758666, - "leaked_bytes": 3332314, - "total_allocations": 3792727 + "peak_bytes": 3778159, + "leaked_bytes": 3353205, + "total_allocations": 3787587 + }, + "with_fragment_pre_consume_rejection": { + "peak_bytes": 3778057, + "leaked_bytes": 3354795, + "total_allocations": 2094004 + }, + "with_archive_post_consume_failure": { + "peak_bytes": 3350600, + "leaked_bytes": 3308056, + "total_allocations": 175290 + }, + "with_fragment_marshalling_error": { + "peak_bytes": 3708068, + "leaked_bytes": 3352335, + "total_allocations": 2077090 + }, + "with_fragment_mixed_outcomes": { + "peak_bytes": 3779175, + "leaked_bytes": 3356294, + "total_allocations": 2656787 }, "builder_to_archive_with_ingredient": { - "peak_bytes": 14049708, - "leaked_bytes": 3318104, - "total_allocations": 1836413 + "peak_bytes": 14069232, + "leaked_bytes": 3337316, + "total_allocations": 1830896 }, "builder_sign_jpeg_archive_roundtrip_ingredient_in_archive": { - "peak_bytes": 14264719, - "leaked_bytes": 3459571, - "total_allocations": 5893306 + "peak_bytes": 14287046, + "leaked_bytes": 3481977, + "total_allocations": 5879957 }, "builder_write_ingredient_archive": { - "peak_bytes": 14049702, - "leaked_bytes": 3318102, - "total_allocations": 1810851 + "peak_bytes": 14069289, + "leaked_bytes": 3337377, + "total_allocations": 1805304 }, "builder_sign_jpeg_add_ingredient_from_archive": { - "peak_bytes": 14113307, - "leaked_bytes": 3459596, - "total_allocations": 3424465 + "peak_bytes": 14133742, + "leaked_bytes": 3480831, + "total_allocations": 3415920 }, "builder_ingredient_archive_roundtrip": { - "peak_bytes": 14264391, - "leaked_bytes": 3460351, - "total_allocations": 5146608 + "peak_bytes": 14284443, + "leaked_bytes": 3480809, + "total_allocations": 5132060 }, "builder_sign_jpeg_two_ingredient_archives": { - "peak_bytes": 14114874, - "leaked_bytes": 3461104, - "total_allocations": 4226879 + "peak_bytes": 14134560, + "leaked_bytes": 3481604, + "total_allocations": 4215728 }, "reader_error_no_manifest": { - "peak_bytes": 3544227, - "leaked_bytes": 3302551, - "total_allocations": 278514 + "peak_bytes": 3564471, + "leaked_bytes": 3323629, + "total_allocations": 276175 }, "builder_error_invalid_manifest": { - "peak_bytes": 3331458, - "leaked_bytes": 3276705, - "total_allocations": 114863 + "peak_bytes": 3352053, + "leaked_bytes": 3297079, + "total_allocations": 113926 }, "reader_string_apis": { - "peak_bytes": 3957555, - "leaked_bytes": 3324853, - "total_allocations": 2296696 + "peak_bytes": 3978113, + "leaked_bytes": 3346111, + "total_allocations": 2287335 }, "signer_construction": { - "peak_bytes": 3331349, - "leaked_bytes": 3268750, - "total_allocations": 155984 + "peak_bytes": 3350893, + "leaked_bytes": 3288137, + "total_allocations": 153245 + }, + "builder_from_context_construction": { + "peak_bytes": 3350600, + "leaked_bytes": 3288582, + "total_allocations": 112688 }, "fork_reader_collect": { - "peak_bytes": 3830817, - "leaked_bytes": 3333241, - "total_allocations": 1330058 + "peak_bytes": 3850530, + "leaked_bytes": 3353063, + "total_allocations": 1328122 }, "fork_contended_mutex": { - "peak_bytes": 7659277, - "leaked_bytes": 3461442, - "total_allocations": 67635759 + "peak_bytes": 7679019, + "leaked_bytes": 3482128, + "total_allocations": 67472694 }, "fork_thread_local_orphan": { - "peak_bytes": 3916484, - "leaked_bytes": 3419968, - "total_allocations": 1383197 + "peak_bytes": 3936170, + "leaked_bytes": 3439733, + "total_allocations": 1381055 }, "fork_gc_cycle": { - "peak_bytes": 3830375, - "leaked_bytes": 3332761, - "total_allocations": 1334044 + "peak_bytes": 3850434, + "leaked_bytes": 3353160, + "total_allocations": 1332098 }, "fork_parent_frees_after_fork": { - "peak_bytes": 5427183, - "leaked_bytes": 3329834, - "total_allocations": 24873000 + "peak_bytes": 5447584, + "leaked_bytes": 3350400, + "total_allocations": 24829257 }, "fork_child_closes_then_parent_frees": { - "peak_bytes": 5427152, - "leaked_bytes": 3329841, - "total_allocations": 24872992 + "peak_bytes": 5446620, + "leaked_bytes": 3350407, + "total_allocations": 24829254 }, "fork_child_sys_exit": { - "peak_bytes": 3831271, - "leaked_bytes": 3332312, - "total_allocations": 1338865 + "peak_bytes": 3850546, + "leaked_bytes": 3353234, + "total_allocations": 1335925 }, "fork_stream_cleanup": { - "peak_bytes": 3444268, - "leaked_bytes": 3272486, - "total_allocations": 106279 + "peak_bytes": 3464063, + "leaked_bytes": 3291969, + "total_allocations": 105340 }, "fork_swap_cleanup": { - "peak_bytes": 3661376, - "leaked_bytes": 3330987, - "total_allocations": 718355 + "peak_bytes": 3681171, + "leaked_bytes": 3350696, + "total_allocations": 714376 }, "fork_contended_mutex_swap": { - "peak_bytes": 7276901, - "leaked_bytes": 3443869, - "total_allocations": 37514894 + "peak_bytes": 7302379, + "leaked_bytes": 3475147, + "total_allocations": 35948516 }, "fork_contended_mutex_wrap": { - "peak_bytes": 7262615, - "leaked_bytes": 3474333, - "total_allocations": 35888503 + "peak_bytes": 7288748, + "leaked_bytes": 3463411, + "total_allocations": 34847186 }, "fork_consumed_signer": { - "peak_bytes": 3331615, - "leaked_bytes": 3269742, - "total_allocations": 179994 + "peak_bytes": 3350894, + "leaked_bytes": 3288906, + "total_allocations": 175055 }, "swap_chain_churn": { - "peak_bytes": 3661193, - "leaked_bytes": 3330367, - "total_allocations": 673717 + "peak_bytes": 3681161, + "leaked_bytes": 3350287, + "total_allocations": 672537 } } \ No newline at end of file From 7c946b985597012bbd6d1140880e7a754e26ca0a Mon Sep 17 00:00:00 2001 From: tmathern <60901087+tmathern@users.noreply.github.com> Date: Sun, 19 Jul 2026 22:01:26 -0700 Subject: [PATCH 19/20] fix: Rename vars for clarity --- src/c2pa/c2pa.py | 49 ++++++++++++++++++++++++------------------------ 1 file changed, 24 insertions(+), 25 deletions(-) diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index d631dbee..52c2a2f3 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -1471,15 +1471,16 @@ def __init__(self): """Create new Settings with default values.""" super().__init__() - ptr = _lib.c2pa_settings_new() + settings_ptr = _lib.c2pa_settings_new() try: - _check_ffi_operation_result(ptr, "Failed to create Settings") + _check_ffi_operation_result( + settings_ptr, "Failed to create Settings") except BaseException: - if ptr: - ManagedResource._free_native_ptr(ptr) + if settings_ptr: + ManagedResource._free_native_ptr(settings_ptr) raise - self._activate(ptr) + self._activate(settings_ptr) @classmethod def from_json(cls, json_str: str) -> 'Settings': @@ -1640,11 +1641,11 @@ def __init__( if settings is None and signer is None: # Simple default context - ptr = _lib.c2pa_context_new() + context_ptr = _lib.c2pa_context_new() _check_ffi_operation_result( - ptr, "Failed to create Context" + context_ptr, "Failed to create Context" ) - self._activate(ptr) + self._activate(context_ptr) else: # Use ContextBuilder for settings/signer builder_ptr = _lib.c2pa_context_builder_new() @@ -1683,16 +1684,16 @@ def __init__( self._has_signer = True # Build consumes builder_ptr - ptr = ( + context_ptr = ( _lib.c2pa_context_builder_build(builder_ptr) ) builder_ptr = None _check_ffi_operation_result( - ptr, "Failed to build Context" + context_ptr, "Failed to build Context" ) - self._activate(ptr) + self._activate(context_ptr) except BaseException: # Free builder if build was not reached if builder_ptr is not None: @@ -2407,7 +2408,7 @@ def _create_reader(self, format_bytes, stream_obj, manifest_data: Optional manifest bytes """ if manifest_data is None: - ptr = _lib.c2pa_reader_from_stream( + reader_ptr = _lib.c2pa_reader_from_stream( format_bytes, stream_obj._stream) else: if not isinstance(manifest_data, bytes): @@ -2415,7 +2416,7 @@ def _create_reader(self, format_bytes, stream_obj, manifest_array = ( ctypes.c_ubyte * len(manifest_data)).from_buffer_copy(manifest_data) - ptr = ( + reader_ptr = ( _lib.c2pa_reader_from_manifest_data_and_stream( format_bytes, stream_obj._stream, @@ -2425,10 +2426,10 @@ def _create_reader(self, format_bytes, stream_obj, ) _check_ffi_operation_result( - ptr, + reader_ptr, Reader._ERROR_MESSAGES['reader_error'].format("Unknown error")) - self._activate(ptr) + self._activate(reader_ptr) def _init_from_file(self, path, format_bytes, manifest_data=None): @@ -2544,10 +2545,7 @@ def _init_attrs(self): self._backing_file = None # Caches for manifest JSON string and parsed data. - # These are invalidated when with_fragment() is called, because each - # new BMFF fragment can refine or update the manifest content as the - # reader progressively builds its understanding of the fragmented - # stream. They are also cleared on close() to release memory. + # These are invalidated when with_fragment() is called. self._manifest_json_str_cache = None self._manifest_data_cache = None @@ -2573,6 +2571,7 @@ def _close_streams(self): def _release(self): """Release Reader-specific resources (caches, stream, backing file). """ + self._manifest_json_str_cache = None self._manifest_data_cache = None self._close_streams() @@ -3265,13 +3264,13 @@ def __init__( if context is not None: self._init_from_context(context, json_str) else: - ptr = _lib.c2pa_builder_from_json(json_str) + builder_ptr = _lib.c2pa_builder_from_json(json_str) _check_ffi_operation_result( - ptr, + builder_ptr, Builder._ERROR_MESSAGES['builder_error'].format("Unknown error")) - self._activate(ptr) + self._activate(builder_ptr) def _init_from_context(self, context, json_str): """Initialize Builder from a ContextProvider. @@ -3296,9 +3295,9 @@ def _init_from_context(self, context, json_str): ManagedResource._free_native_ptr(builder_ptr) raise - # Adopt the handle before the consuming call: _consume_and_swap needs - # an active resource, and from here on normal cleanup owns the pointer - # whichever way the call goes. + # Adopt the handle before the consuming call: + # _consume_and_swap needs an active resource, + # and from here on normal cleanup owns the pointer. self._activate(builder_ptr) self._consume_and_swap( From f342997a176e00f8eb9bf358be3f1fa0854bc61d Mon Sep 17 00:00:00 2001 From: tmathern <60901087+tmathern@users.noreply.github.com> Date: Sun, 19 Jul 2026 22:26:45 -0700 Subject: [PATCH 20/20] fix: Refactor once more 2 --- src/c2pa/c2pa.py | 68 ++++++++++++++++++------------------------------ 1 file changed, 25 insertions(+), 43 deletions(-) diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index 52c2a2f3..02447e67 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -1261,7 +1261,11 @@ def _check_ffi_operation_result( Args: result: The return value from the FFI call - fallback_msg: Error message if the native library has no error details + fallback_msg: Error message if the native library has no error details. + An error message template ending in `: {}` may be passed + unformatted. An "Unknown error" fallback is filled in here, since + reaching this point means the native layer offered nothing better. + Plain messages with no placeholder are used as-is. check: Predicate that returns True when the result indicates failure. Defaults to `not r` (for pointer-returning calls). Use `lambda r: r != 0` for status-code-returning calls. @@ -1277,7 +1281,7 @@ def _check_ffi_operation_result( error = _parse_operation_result_for_error(_lib.c2pa_error()) if error: raise C2paError(error) - raise C2paError(fallback_msg) + raise C2paError(fallback_msg.format("Unknown error")) return result @@ -1780,15 +1784,6 @@ class Stream: 'stream_error': "Error cleaning up stream: {}", 'callback_error': "Error cleaning up callback {}: {}", 'cleanup_error': "Error during cleanup: {}", - 'read': "Stream is closed or not initialized during read operation", - 'memory_error': "Memory error during stream operation: {}", - 'read_error': "Error during read operation: {}", - 'seek': "Stream is closed or not initialized during seek operation", - 'seek_error': "Error during seek operation: {}", - 'write': "Stream is closed or not initialized during write operation", - 'write_error': "Error during write operation: {}", - 'flush': "Stream is closed or not initialized during flush operation", - 'flush_error': "Error during flush operation: {}" } def __init__(self, file_like_stream): @@ -2218,16 +2213,12 @@ class Reader(ManagedResource): # Class-level error messages to avoid multiple creation _ERROR_MESSAGES = { - 'unsupported': "Unsupported format", 'io_error': "IO error: {}", 'manifest_error': "Invalid manifest data: must be bytes", 'reader_error': "Failed to create reader: {}", 'cleanup_error': "Error during cleanup: {}", 'stream_error': "Error cleaning up stream: {}", - 'file_error': "Error cleaning up file: {}", - 'reader_cleanup_error': "Error cleaning up reader: {}", 'encoding_error': "Invalid UTF-8 characters in input: {}", - 'closed_error': "Reader is closed", 'fragment_error': "Failed to process fragment: {}" } @@ -2427,7 +2418,7 @@ def _create_reader(self, format_bytes, stream_obj, _check_ffi_operation_result( reader_ptr, - Reader._ERROR_MESSAGES['reader_error'].format("Unknown error")) + Reader._ERROR_MESSAGES['reader_error']) self._activate(reader_ptr) @@ -2495,7 +2486,7 @@ def _init_from_context(self, context, format_or_path, _check_ffi_operation_result(reader_ptr, Reader._ERROR_MESSAGES[ 'reader_error' - ].format("Unknown error") + ] ) except BaseException: if reader_ptr: @@ -2575,6 +2566,8 @@ def _release(self): self._manifest_json_str_cache = None self._manifest_data_cache = None self._close_streams() + # The Context is not ours to close, only to stop pinning. + self._context = None def _get_cached_manifest_data(self) -> Optional[dict]: """Get the cached manifest data, fetching and parsing if not cached. @@ -2889,12 +2882,8 @@ class Signer(ManagedResource): # Class-level error messages to avoid multiple creation _ERROR_MESSAGES = { - 'closed_error': "Signer is closed", 'cleanup_error': "Error during cleanup: {}", - 'signer_cleanup': "Error cleaning up signer: {}", 'callback_error': "Error in signer callback: {}", - 'info_error': "Error creating signer from info: {}", - 'invalid_data': "Invalid data for signing: {}", 'invalid_certs': "Invalid certificate data: {}", 'invalid_tsa': "Invalid TSA URL: {}", 'encoding_error': "Invalid UTF-8 characters in input: {}" @@ -3115,19 +3104,14 @@ class Builder(ManagedResource): _ERROR_MESSAGES = { 'builder_error': "Failed to create builder: {}", 'cleanup_error': "Error during cleanup: {}", - 'builder_cleanup': "Error cleaning up builder: {}", - 'closed_error': "Builder is closed", - 'manifest_error': "Invalid manifest data: must be string or dict", 'url_error': "Error setting remote URL: {}", + 'intent_error': "Error setting intent for Builder: {}", 'resource_error': "Error adding resource: {}", 'ingredient_error': "Error adding ingredient: {}", 'archive_read_error': "Error loading ingredient from archive: {}", 'action_error': "Error adding action: {}", 'archive_error': "Error writing archive: {}", 'archive_load_error': "Failed to load archive into builder: {}", - 'sign_error': "Error during signing: {}", - 'encoding_error': "Invalid UTF-8 characters in manifest: {}", - 'json_error': "Failed to serialize manifest JSON: {}" } @classmethod @@ -3268,7 +3252,7 @@ def __init__( _check_ffi_operation_result( builder_ptr, - Builder._ERROR_MESSAGES['builder_error'].format("Unknown error")) + Builder._ERROR_MESSAGES['builder_error']) self._activate(builder_ptr) @@ -3288,7 +3272,7 @@ def _init_from_context(self, context, json_str): _check_ffi_operation_result(builder_ptr, Builder._ERROR_MESSAGES[ 'builder_error' - ].format("Unknown error") + ] ) except BaseException: if builder_ptr: @@ -3344,7 +3328,7 @@ def set_remote_url(self, remote_url: str): _check_ffi_operation_result( result, - Builder._ERROR_MESSAGES['url_error'].format("Unknown error"), + Builder._ERROR_MESSAGES['url_error'], check=lambda r: r != 0) def set_intent( @@ -3383,7 +3367,7 @@ def set_intent( _check_ffi_operation_result( result, - "Error setting intent for Builder: Unknown error", + Builder._ERROR_MESSAGES['intent_error'], check=lambda r: r != 0) def add_resource(self, uri: str, stream: Any): @@ -3406,7 +3390,7 @@ def add_resource(self, uri: str, stream: Any): _check_ffi_operation_result( result, - Builder._ERROR_MESSAGES['resource_error'].format("Unknown error"), + Builder._ERROR_MESSAGES['resource_error'], check=lambda r: r != 0) def add_ingredient( @@ -3473,7 +3457,7 @@ def add_ingredient_from_stream( _check_ffi_operation_result( result, - Builder._ERROR_MESSAGES['ingredient_error'].format("Unknown error"), + Builder._ERROR_MESSAGES['ingredient_error'], check=lambda r: r != 0) def add_action(self, action_json: Union[str, dict]) -> None: @@ -3495,7 +3479,7 @@ def add_action(self, action_json: Union[str, dict]) -> None: _check_ffi_operation_result( result, - Builder._ERROR_MESSAGES['action_error'].format("Unknown error"), + Builder._ERROR_MESSAGES['action_error'], check=lambda r: r != 0) def to_archive(self, stream: Any) -> None: @@ -3516,7 +3500,7 @@ def to_archive(self, stream: Any) -> None: _check_ffi_operation_result( result, - Builder._ERROR_MESSAGES["archive_error"].format("Unknown error"), + Builder._ERROR_MESSAGES["archive_error"], check=lambda r: r != 0) def write_ingredient_archive(self, ingredient_id: str, stream: Any) -> None: @@ -3539,10 +3523,9 @@ def write_ingredient_archive(self, ingredient_id: str, stream: Any) -> None: result = _lib.c2pa_builder_write_ingredient_archive( self._handle, ingredient_id_str, stream_obj._stream) - _check_ffi_operation_result(result, - Builder._ERROR_MESSAGES["archive_error"].format( - "Unknown error" - ), + _check_ffi_operation_result( + result, + Builder._ERROR_MESSAGES["archive_error"], check=lambda r: r != 0) def add_ingredient_from_archive(self, stream: Any) -> None: @@ -3562,10 +3545,9 @@ def add_ingredient_from_archive(self, stream: Any) -> None: result = _lib.c2pa_builder_add_ingredient_from_archive( self._handle, stream_obj._stream) - _check_ffi_operation_result(result, - Builder._ERROR_MESSAGES["archive_read_error"].format( - "Unknown error" - ), + _check_ffi_operation_result( + result, + Builder._ERROR_MESSAGES["archive_read_error"], check=lambda r: r != 0) def with_archive(self, stream: Any) -> 'Builder':