From ca293514082885f61a71d89cdaf2a8331306cc6e Mon Sep 17 00:00:00 2001 From: Tania Mathern Date: Thu, 16 Jul 2026 11:42:30 -0700 Subject: [PATCH 1/9] 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 2/9] 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 3/9] 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 4/9] 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 5/9] 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 6/9] 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 7/9] 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 8/9] 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 9/9] 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.