From 8f5454b31531b84b1ce6fe91c237458fa5b0e0c0 Mon Sep 17 00:00:00 2001 From: tmathern <60901087+tmathern@users.noreply.github.com> Date: Wed, 10 Jun 2026 15:38:38 -0700 Subject: [PATCH 1/3] fix: Initial gc investigation --- MEMORY_FINDINGS.md | 130 ++++++++++++++++++++++++++++ src/c2pa/c2pa.py | 65 +++++++++----- tests/perf/README.md | 2 + tests/perf/baseline.json | 173 +++++++++++++++++++++----------------- tests/perf/run_profile.py | 5 ++ tests/perf/scenarios.py | 53 +++++++++++- 6 files changed, 326 insertions(+), 102 deletions(-) create mode 100644 MEMORY_FINDINGS.md diff --git a/MEMORY_FINDINGS.md b/MEMORY_FINDINGS.md new file mode 100644 index 00000000..4ac02eff --- /dev/null +++ b/MEMORY_FINDINGS.md @@ -0,0 +1,130 @@ +# Memory investigation findings + +Date: 2026-06-10. Environment: macOS arm64, Python 3.12, memray 1.19.3, +native library c2pa-v0.86.1 (local build). Methodology and harness live in +`tests/perf/` (see its README). + +## Summary + +Nine memray scenarios were written to cover previously unmeasured paths: +callback signers, sign-failure error paths, abandoned (never-closed) +readers, `ManifestNotFound` / invalid-manifest error loops, the uncached +Reader string APIs, thumbnail/resource transfer in both directions, and +per-iteration Context/Signer churn. After the investigation, only the three +that guard the fixed code paths were kept in `tests/perf/scenarios.py` +(`reader_error_no_manifest`, `builder_error_invalid_manifest`, +`reader_string_apis`); the others measured clean and added suite runtime +without guarding anything. + +**No memory leak was found in c2pa-rs.** Native memory was measured flat +across all scenarios: `leaked_bytes` deltas between 100 and 300 iterations +were within noise (±4 KiB, i.e. zero per-iteration slope) on every scenario, +including both thumbnail/resource paths. + +**One real issue was found and fixed in the Python bindings** (not a native +leak): several hot ctypes patterns created reference-cycle garbage on every +operation, which only the cycle collector could reclaim. This showed up as +live-memory growth of ~366 B per Reader iteration between garbage-collector +runs, plus avoidable gc pressure (3-4 cycle objects per operation). Details +below. + +## Thumbnail / resource handling verdict + +Both directions were profiled explicitly because they were suspected: + +- `reader_string_apis` — per iteration: `detailed_json()`, `crjson()`, + `get_remote_url()`, and `resource_to_stream()` extracting the active + manifest's JPEG thumbnail (31,608 bytes per extraction) from + `tests/fixtures/C.jpg`. Measured: leaked 2.10 MiB at N=100 vs 2.10 MiB at + N=300 (delta −975 B). No growing allocation site above 4 KiB. +- `builder_add_resource_thumbnail` — per iteration: `Builder.add_resource` + of a ~90 KB JPEG thumbnail followed by a context sign. Measured: leaked + 2.30 MiB at both N=100 and N=300 (delta −2.2 KiB). No growing site. + +The constant ~2-4 MiB `leaked_bytes` floor in every scenario is the +documented one-time static overhead of loading the native library (see +"Why is leaked_bytes not zero?" in `tests/perf/README.md`), not a leak: it +does not scale with iterations. + +Conclusion: the native `c2pa_reader_resource_to_stream`, +`c2pa_builder_add_resource`, manifest parse and sign paths free everything +they allocate. Nothing to report to c2pa-rs. + +## The issue that was real: ctypes reference cycles in the bindings + +Symptom (measured before the fix): + +- `gc.collect()` after 100 Reader iterations found 344 unreachable objects; + after 50 Builder signs, 208 — even when `close()` was called correctly. +- memray's high-watermark snapshot for `reader_jpeg_with_context` grew + 366 B/iteration (2,694 KiB at N=100 → 2,765 KiB at N=300): cycle garbage + accumulating between collector runs counts as live memory. +- Garbage was ctypes-internal: `PyCArrayType` classes, `LP_C2paReader` + pointer objects, and their type dicts/descriptors. + +Root causes, isolated by measuring each pattern in a bare loop: + +| Pattern | Cycle objects per call | +| --- | --- | +| `ctypes.cast(value, c_char_p)` (string returns, error strings) | 2 | +| `ctypes.cast(ffi_ptr, c_void_p)` (every native free) | 2 | +| `(ctypes.c_char * length)` built per stream-read call | ~0.6 | +| `ctypes.string_at(...)` / direct pointer pass | 0 | + +Fixes applied in `src/c2pa/c2pa.py`: + +1. `_convert_to_py_string` and the error path in + `_parse_operation_result_for_error` now read native strings with + `ctypes.string_at` instead of `ctypes.cast(..., c_char_p)`. +2. `ManagedResource._free_native_ptr` passes the pointer directly to + `c2pa_free` (whose argtype is already `c_void_p`) instead of casting. +3. The per-chunk stream read path wraps the native buffer in a writable + memoryview (`PyMemoryView_FromMemory`) instead of building a + `(c_char * length)` array type — no class creation at all, for any chunk + size. The view is `release()`d in a `finally` right after `readinto`, so + a stream object that stashes the buffer gets a `ValueError` on later + access instead of writing into freed native memory (the old ctypes-array + approach had no such guard), and the reported read count is clamped to + the buffer length. The remaining array-type sites (manifest byte arrays, + signing payloads) were left as plain inline `(c_ubyte * n)` creations: + they run once per operation, lengths there are data-dependent and rarely + repeat, so caching would not hit and the one cyclic class per operation + is negligible next to the operation itself. + +Verified after the fix: + +- `gc.collect()` finds **0** unreachable objects after 100 Reader + iterations (with or without `close()`) and after 50 Builder signs. +- High-watermark growth: −13 B/iteration (flat) for the Reader control. +- Peak RSS without memray: flat at 33.7 MB for N=100/300/600. +- Full unit suite: 234 passed. + +## Other paths checked and clean + +| Scenario | leaked @100 → @300 | Verdict | +| --- | --- | --- | +| signer_from_callback_churn | 4.25 MiB → 4.25 MiB | clean | +| signer_callback_sign_error | 3.48 MiB → 3.48 MiB | clean (error strings freed) | +| stream_abandon_no_close | 2.10 MiB → 2.10 MiB | clean (gc + `__del__` releases native stream) | +| reader_error_no_manifest | 2.08 MiB → 2.08 MiB | clean (partial-init cleanup works) | +| builder_error_invalid_manifest | 2.05 MiB → 2.04 MiB | clean | +| context_churn | 2.30 MiB → 2.30 MiB | clean (`c2pa_context_free` + consumed signer) | +| signer_from_info_churn | 2.04 MiB → 2.04 MiB | clean (`c2pa_signer_free`) | + +One measurement note: `memray`'s `metadata.peak_memory` shows a ~3-5 KiB +per-iteration upward slope even after the fix, while the sum of its +high-watermark allocation records, process RSS, and `leaked_bytes` are all +flat. That residual slope is profiler accounting overhead (it scales with +the number of allocation records), not application memory; use the +high-watermark records or RSS when judging peak behavior across different +iteration counts. + +## Also found during review (not measured as leaking, fixed by design) + +`Signer.from_callback` could leak the native signer pointer if signer +creation failed after `c2pa_signer_create` returned non-null. In practice +this path is unreachable with bad input — the native library defers +certificate validation to signing time (confirmed: garbage PEM creates a +signer successfully; the failure surfaces during `Builder.sign` as +`C2paError.Signature`, covered by the `signer_callback_sign_error` +scenario, which measures clean). diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index fc62632e..d721a2d5 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -241,8 +241,11 @@ def __init__(self): @staticmethod def _free_native_ptr(ptr): - """Free a native pointer by casting it to c_void_p and calling c2pa_free.""" - _lib.c2pa_free(ctypes.cast(ptr, ctypes.c_void_p)) + """Free a native pointer by passing it to c2pa_free. + c2pa_free's argtype is c_void_p, so ctypes converts any pointer instance. + (ctypes.cast(ptr, c_void_p) leaves reference cycles behind on) + """ + _lib.c2pa_free(ptr) def _ensure_valid_state(self): """Raise if the resource is closed or uninitialized.""" @@ -881,33 +884,43 @@ def __init__(self): self._data_dir_str = "" +# Wrap a raw (address, length) native region in a writable memoryview +_PyMemoryView_FromMemory = ctypes.pythonapi.PyMemoryView_FromMemory +_PyMemoryView_FromMemory.restype = ctypes.py_object +_PyMemoryView_FromMemory.argtypes = ( + ctypes.c_void_p, ctypes.c_ssize_t, ctypes.c_int) +_PyBUF_WRITE = 0x200 + + +def _writable_memoryview(address, length): + return _PyMemoryView_FromMemory(address, length, _PyBUF_WRITE) + + def _convert_to_py_string(value) -> str: if value is None: return "" py_string = "" - # Validate pointer before casting and freeing + # Validate pointer before reading and freeing if not isinstance(value, (int, ctypes.c_void_p)) or value == 0: return "" try: - ptr = ctypes.cast(value, ctypes.c_char_p) + raw = ctypes.string_at(value) - # Only if we got a valid pointer with valid content - if ptr and ptr.value is not None: + try: + py_string = raw.decode('utf-8', errors='strict') + except Exception: + py_string = "" + finally: + # Only free if we have a valid pointer try: - py_string = ptr.value.decode('utf-8', errors='strict') + _lib.c2pa_string_free(value) except Exception: - py_string = "" - finally: - # Only free if we have a valid pointer - try: - _lib.c2pa_string_free(value) - except Exception: - # Ignore clean up issues - pass - except (ctypes.ArgumentError, TypeError, ValueError): + # Ignore clean up issues + pass + except (ctypes.ArgumentError, TypeError, ValueError, OSError): # Invalid pointer type or value return "" @@ -995,8 +1008,7 @@ def _parse_operation_result_for_error( if check_error: error = _lib.c2pa_error() if error: - error_str = ctypes.cast( - error, ctypes.c_char_p).value.decode('utf-8') + error_str = ctypes.string_at(error).decode('utf-8') _lib.c2pa_string_free(error) _raise_typed_c2pa_error(error_str) return None @@ -1617,10 +1629,19 @@ def read_callback(ctx, data, length): readinto = getattr(stream, "readinto", None) if readinto is not None: # Most streams have readinto - buf = (ctypes.c_char * length).from_address( - ctypes.addressof(data.contents)) - n = readinto(buf) - return n if n else 0 + buf = _writable_memoryview( + ctypes.addressof(data.contents), length) + try: + n = readinto(buf) + finally: + # Invalidate the view: + # The native buffer is only valid for + # the duration of the callback... + buf.release() + if not n: + return 0 + # Never report more than the buffer can hold + return min(n, length) # Fallback for streams without readinto. buffer = stream.read(length) diff --git a/tests/perf/README.md b/tests/perf/README.md index 1f2ec022..cccc60de 100644 --- a/tests/perf/README.md +++ b/tests/perf/README.md @@ -186,6 +186,8 @@ A memory leak grows proportionally with work done. If you sign 50 images and get The baseline captures this expected static overhead. Future runs compare against it: if `leaked_bytes` grows beyond the baseline by more than 10%, the run fails. +The framework runs `gc.collect()` twice after the scenario finishes, while memray is still tracking. Without that sweep, objects sitting in not-yet-collected reference cycles would be counted in `leaked_bytes` and the number would depend on garbage collector timing rather than on actual leaks. With it, `leaked_bytes` means memory that is still allocated even though nothing in Python can reach it: true leaks plus the one-time static overhead described above. + ### How to confirm no leak exists? Run with a higher iteration count than default (100) and compare: diff --git a/tests/perf/baseline.json b/tests/perf/baseline.json index 302d648a..3e436967 100644 --- a/tests/perf/baseline.json +++ b/tests/perf/baseline.json @@ -2,139 +2,154 @@ "_meta": { "memray_version": "1.19.3", "python_version": "3.12.13", - "c2pa_native_version": "c2pa-v0.85.1", + "c2pa_native_version": "c2pa-v0.86.1", "iterations": 100, "perf_env": "python-3.12-slim", "arch": "aarch64" }, "reader_jpeg_legacy": { - "peak_bytes": 3814421, - "leaked_bytes": 3266116, - "total_allocations": 698899 + "peak_bytes": 3730321, + "leaked_bytes": 3236992, + "total_allocations": 717596 }, "reader_jpeg_with_context": { - "peak_bytes": 3822953, - "leaked_bytes": 3257471, - "total_allocations": 692953 + "peak_bytes": 3724412, + "leaked_bytes": 3229219, + "total_allocations": 711543 }, "reader_mp4": { - "peak_bytes": 4876441, - "leaked_bytes": 3257485, - "total_allocations": 2112991 + "peak_bytes": 4099225, + "leaked_bytes": 3228160, + "total_allocations": 2084373 }, "reader_wav": { - "peak_bytes": 5520266, - "leaked_bytes": 3267427, - "total_allocations": 400371 + "peak_bytes": 4399719, + "leaked_bytes": 3238102, + "total_allocations": 408059 }, "builder_sign_jpeg_legacy": { - "peak_bytes": 7695310, - "leaked_bytes": 3383623, - "total_allocations": 522425 + "peak_bytes": 7663441, + "leaked_bytes": 3352658, + "total_allocations": 555922 }, "builder_sign_jpeg_with_context": { - "peak_bytes": 7688236, - "leaked_bytes": 3376293, - "total_allocations": 516851 + "peak_bytes": 7656560, + "leaked_bytes": 3345863, + "total_allocations": 550105 }, "builder_sign_png_legacy": { - "peak_bytes": 7932767, - "leaked_bytes": 3383648, - "total_allocations": 1694629 + "peak_bytes": 7900956, + "leaked_bytes": 3351994, + "total_allocations": 1978914 }, "builder_sign_png_with_context": { - "peak_bytes": 7925490, - "leaked_bytes": 3376452, - "total_allocations": 1688908 + "peak_bytes": 7893973, + "leaked_bytes": 3345450, + "total_allocations": 1973003 }, "builder_sign_jpeg_parallel_split_pool": { - "peak_bytes": 45764159, - "leaked_bytes": 3818113, - "total_allocations": 528785 + "peak_bytes": 45726143, + "leaked_bytes": 3714796, + "total_allocations": 557891 }, "builder_sign_jpeg_parallel_split_barrier": { - "peak_bytes": 46225287, - "leaked_bytes": 3809216, - "total_allocations": 527412 + "peak_bytes": 45817488, + "leaked_bytes": 3780629, + "total_allocations": 627768 }, "builder_sign_png_parallel_split_pool": { - "peak_bytes": 46002549, - "leaked_bytes": 3817801, - "total_allocations": 1700731 + "peak_bytes": 40563556, + "leaked_bytes": 3746819, + "total_allocations": 1984928 }, "builder_sign_png_parallel_split_barrier": { - "peak_bytes": 45970433, - "leaked_bytes": 3812044, - "total_allocations": 1699396 + "peak_bytes": 45964496, + "leaked_bytes": 3745249, + "total_allocations": 1983686 }, "builder_sign_gif": { - "peak_bytes": 14544515, - "leaked_bytes": 3375865, - "total_allocations": 7183237 + "peak_bytes": 14514114, + "leaked_bytes": 3345545, + "total_allocations": 8547131 }, "builder_sign_heic": { - "peak_bytes": 4608484, - "leaked_bytes": 3376030, - "total_allocations": 771079 + "peak_bytes": 7717414, + "leaked_bytes": 3381771, + "total_allocations": 877927 }, "builder_sign_m4a": { - "peak_bytes": 18849082, - "leaked_bytes": 3376431, - "total_allocations": 2273497 + "peak_bytes": 18817771, + "leaked_bytes": 3345503, + "total_allocations": 2627261 }, "builder_sign_webp": { - "peak_bytes": 8900701, - "leaked_bytes": 3376432, - "total_allocations": 487683 + "peak_bytes": 8869451, + "leaked_bytes": 3345563, + "total_allocations": 496534 }, "builder_sign_avi": { - "peak_bytes": 7040387, - "leaked_bytes": 3376267, - "total_allocations": 40315553 + "peak_bytes": 7009007, + "leaked_bytes": 3345266, + "total_allocations": 45029611 }, "builder_sign_mp4": { - "peak_bytes": 6162851, - "leaked_bytes": 3376431, - "total_allocations": 1809672 + "peak_bytes": 6131977, + "leaked_bytes": 3345600, + "total_allocations": 1923444 }, "builder_sign_tiff": { - "peak_bytes": 13124728, - "leaked_bytes": 3376268, - "total_allocations": 5139967 + "peak_bytes": 13091168, + "leaked_bytes": 3345348, + "total_allocations": 5469122 }, "builder_sign_jpeg_parent_of": { - "peak_bytes": 14173992, - "leaked_bytes": 3377656, - "total_allocations": 1209933 + "peak_bytes": 14143351, + "leaked_bytes": 3345698, + "total_allocations": 1285766 }, "builder_sign_jpeg_component_of": { - "peak_bytes": 14175518, - "leaked_bytes": 3377891, - "total_allocations": 1232336 + "peak_bytes": 14144869, + "leaked_bytes": 3345779, + "total_allocations": 1308244 }, "builder_sign_jpeg_parent_and_component": { - "peak_bytes": 14530406, - "leaked_bytes": 3474418, - "total_allocations": 2160934 + "peak_bytes": 14434957, + "leaked_bytes": 3450621, + "total_allocations": 2289962 }, "builder_sign_jpeg_parent_and_component_mixed_mime": { - "peak_bytes": 14476171, - "leaked_bytes": 3378735, - "total_allocations": 2451587 + "peak_bytes": 14445750, + "leaked_bytes": 3345959, + "total_allocations": 2787986 }, "builder_sign_jpeg_two_components_same_mime": { - "peak_bytes": 14519270, - "leaked_bytes": 3473673, - "total_allocations": 2150782 + "peak_bytes": 14432257, + "leaked_bytes": 3442393, + "total_allocations": 2279745 }, "builder_sign_jpeg_two_components_mixed_mime": { - "peak_bytes": 14473127, - "leaked_bytes": 3377445, - "total_allocations": 2441195 + "peak_bytes": 14443122, + "leaked_bytes": 3346165, + "total_allocations": 2777653 }, "builder_sign_jpeg_archive_roundtrip": { - "peak_bytes": 14226832, - "leaked_bytes": 3426491, - "total_allocations": 1680290 + "peak_bytes": 14175766, + "leaked_bytes": 3365189, + "total_allocations": 1767740 + }, + "reader_error_no_manifest": { + "peak_bytes": 3443163, + "leaked_bytes": 3207515, + "total_allocations": 173242 + }, + "builder_error_invalid_manifest": { + "peak_bytes": 3243627, + "leaked_bytes": 3186717, + "total_allocations": 95461 + }, + "reader_string_apis": { + "peak_bytes": 3857136, + "leaked_bytes": 3229581, + "total_allocations": 1178409 } } \ No newline at end of file diff --git a/tests/perf/run_profile.py b/tests/perf/run_profile.py index 31593967..362e3c9a 100644 --- a/tests/perf/run_profile.py +++ b/tests/perf/run_profile.py @@ -58,6 +58,11 @@ def _run_scenario_under_memray(name: str, bin_path: Path) -> None: sys.path.insert(0, "{repo_root / 'src'}") from tests.perf.scenarios import SCENARIOS SCENARIOS["{name}"]({ITERATIONS}) +# Collect cycle garbage before tracking ends so leaked_bytes means "still +# allocated though unreachable" (true leaks + one-time statics). +import gc +gc.collect() +gc.collect() """ cmd = [ sys.executable, "-m", "memray", "run", diff --git a/tests/perf/scenarios.py b/tests/perf/scenarios.py index 0432aa20..fcb03e93 100644 --- a/tests/perf/scenarios.py +++ b/tests/perf/scenarios.py @@ -10,12 +10,20 @@ """ import io +import json import os import sys import threading from concurrent.futures import ThreadPoolExecutor from pathlib import Path -from c2pa import Builder, C2paSignerInfo, Context, Reader, Signer +from c2pa import ( + Builder, + C2paError, + C2paSignerInfo, + Context, + Reader, + Signer, +) FIXTURES_DIR = Path(__file__).parent.parent / "fixtures" READING_FIXTURES_DIR = FIXTURES_DIR / "files-for-reading-tests" @@ -463,6 +471,46 @@ def scenario_builder_sign_jpeg_archive_roundtrip(iterations: int = 100) -> None: builder.sign("image/jpeg", io.BytesIO(source_bytes), io.BytesIO()) +def scenario_reader_error_no_manifest(iterations: int = 100) -> None: + """Reader on an unsigned asset: ManifestNotFound partial-init cleanup.""" + source_bytes = SOURCE_JPEG.read_bytes() # A.jpg carries no manifest + for _ in _iterate(iterations): + try: + Reader("image/jpeg", io.BytesIO(source_bytes)).json() + except C2paError: + pass + + +def scenario_builder_error_invalid_manifest(iterations: int = 100) -> None: + """Builder with malformed manifest JSON: error string + partial init.""" + for _ in _iterate(iterations): + try: + Builder('{"not valid json') + except C2paError: + pass + + +def scenario_reader_string_apis(iterations: int = 100) -> None: + """Uncached string returns: detailed_json/crjson/remote_url/resource_to_stream.""" + source_bytes = SIGNED_JPEG.read_bytes() + context = Context() + # Resolve a real resource URI once, outside the measured loop. + probe = Reader("image/jpeg", io.BytesIO(source_bytes), + manifest_data=None, context=context) + manifests = json.loads(probe.json()) + active = manifests["manifests"][manifests["active_manifest"]] + thumb_uri = active["thumbnail"]["identifier"] + probe.close() + for _ in _iterate(iterations): + reader = Reader("image/jpeg", io.BytesIO(source_bytes), + manifest_data=None, context=context) + reader.detailed_json() + reader.crjson() + reader.get_remote_url() + reader.resource_to_stream(thumb_uri, io.BytesIO()) + reader.close() + + # jpeg + png context variants, paired with the `_legacy` scenarios above for # side-by-side comparison. @@ -524,6 +572,9 @@ def scenario_builder_sign_png_parallel_split_barrier(iterations: int = 100) -> N "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, + "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, } From ef12faedcf971086056c479d5a88eb1a29c55d79 Mon Sep 17 00:00:00 2001 From: tmathern <60901087+tmathern@users.noreply.github.com> Date: Wed, 10 Jun 2026 15:51:08 -0700 Subject: [PATCH 2/3] fix: Initial gc investigation 2 --- MEMORY_FINDINGS.md | 18 +++++++++++++++++- src/c2pa/c2pa.py | 18 +++++++++++++----- tests/perf/run_profile.py | 4 ++-- tests/perf/scenarios.py | 6 +++--- 4 files changed, 35 insertions(+), 11 deletions(-) diff --git a/MEMORY_FINDINGS.md b/MEMORY_FINDINGS.md index 4ac02eff..4056e78d 100644 --- a/MEMORY_FINDINGS.md +++ b/MEMORY_FINDINGS.md @@ -36,7 +36,10 @@ Both directions were profiled explicitly because they were suspected: `get_remote_url()`, and `resource_to_stream()` extracting the active manifest's JPEG thumbnail (31,608 bytes per extraction) from `tests/fixtures/C.jpg`. Measured: leaked 2.10 MiB at N=100 vs 2.10 MiB at - N=300 (delta −975 B). No growing allocation site above 4 KiB. + N=300 (delta −975 B). No growing allocation site above 4 KiB. (Note: + `get_remote_url()` on these embedded-manifest fixtures returns `None` + before any string conversion, so within this scenario it covers the NULL + branch only.) - `builder_add_resource_thumbnail` — per iteration: `Builder.add_resource` of a ~90 KB JPEG thumbnail followed by a context sign. Measured: leaked 2.30 MiB at both N=100 and N=300 (delta −2.2 KiB). No growing site. @@ -119,6 +122,19 @@ the number of allocation records), not application memory; use the high-watermark records or RSS when judging peak behavior across different iteration counts. +## Addendum: regression caught in adversarial re-review (2026-06-10) + +The first version of the `string_at` rewrite of `_convert_to_py_string` had a +NULL-handling regression: the `value == 0` guard does not catch +`ctypes.c_void_p(0)` (ctypes instances never compare equal to ints), and +`ctypes.string_at(NULL)` crashes the process where the old +`ctypes.cast(...).value` path returned `None`. Unreachable through current +callers (all pass `int | None` from `c_void_p`-restype functions, and both +are guarded), but the function's own type check admits `c_void_p` instances, +so the latent crash was real. Fixed by normalizing to a raw address first +(`value.value` for `c_void_p`) and bailing out on falsy addresses; verified +against `None`, `0`, `c_void_p(0)`, `c_void_p(None)`, and non-pointer types. + ## Also found during review (not measured as leaking, fixed by design) `Signer.from_callback` could leak the native signer pointer if signer diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index d721a2d5..7b7747de 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -242,8 +242,10 @@ def __init__(self): @staticmethod def _free_native_ptr(ptr): """Free a native pointer by passing it to c2pa_free. - c2pa_free's argtype is c_void_p, so ctypes converts any pointer instance. - (ctypes.cast(ptr, c_void_p) leaves reference cycles behind on) + + c2pa_free's argtype is c_void_p, so ctypes converts any pointer + instance directly. (ctypes.cast(ptr, c_void_p) would do the same + conversion but leaves a reference cycle behind on every call.) """ _lib.c2pa_free(ptr) @@ -902,12 +904,18 @@ def _convert_to_py_string(value) -> str: py_string = "" - # Validate pointer before reading and freeing - if not isinstance(value, (int, ctypes.c_void_p)) or value == 0: + # Validate and normalize pointer before reading and freeing. + if isinstance(value, ctypes.c_void_p): + address = value.value + elif isinstance(value, int): + address = value + else: + return "" + if not address: return "" try: - raw = ctypes.string_at(value) + raw = ctypes.string_at(address) try: py_string = raw.decode('utf-8', errors='strict') diff --git a/tests/perf/run_profile.py b/tests/perf/run_profile.py index 362e3c9a..9b8d4651 100644 --- a/tests/perf/run_profile.py +++ b/tests/perf/run_profile.py @@ -58,8 +58,8 @@ def _run_scenario_under_memray(name: str, bin_path: Path) -> None: sys.path.insert(0, "{repo_root / 'src'}") from tests.perf.scenarios import SCENARIOS SCENARIOS["{name}"]({ITERATIONS}) -# Collect cycle garbage before tracking ends so leaked_bytes means "still -# allocated though unreachable" (true leaks + one-time statics). +# Collect cycle garbage before tracking ends so leaked_bytes means +# "still allocated but unreachable" (true leaks + one-time statics). import gc gc.collect() gc.collect() diff --git a/tests/perf/scenarios.py b/tests/perf/scenarios.py index fcb03e93..e2945294 100644 --- a/tests/perf/scenarios.py +++ b/tests/perf/scenarios.py @@ -460,7 +460,7 @@ def scenario_builder_sign_jpeg_archive_roundtrip(iterations: int = 100) -> None: archive = io.BytesIO() Builder(MANIFEST_BASE).to_archive(archive) archive.seek(0) - # from_archive() yields a context-less Builder; to keep the Context + # from_archive() yields a context-less Builder. To keep the Context # (and its signer), build with the context first, then load the archive. builder = Builder(MANIFEST_BASE, context=context).with_archive(archive) with io.BytesIO(ingredient_bytes) as ing: @@ -472,7 +472,7 @@ def scenario_builder_sign_jpeg_archive_roundtrip(iterations: int = 100) -> None: def scenario_reader_error_no_manifest(iterations: int = 100) -> None: - """Reader on an unsigned asset: ManifestNotFound partial-init cleanup.""" + """Reader on an unsigned asset: partial-init cleanup.""" source_bytes = SOURCE_JPEG.read_bytes() # A.jpg carries no manifest for _ in _iterate(iterations): try: @@ -482,7 +482,7 @@ def scenario_reader_error_no_manifest(iterations: int = 100) -> None: def scenario_builder_error_invalid_manifest(iterations: int = 100) -> None: - """Builder with malformed manifest JSON: error string + partial init.""" + """Error case: Builder with malformed manifest JSON.""" for _ in _iterate(iterations): try: Builder('{"not valid json') From f605566825941e3180d7c1061b3d43aafb6e35e9 Mon Sep 17 00:00:00 2001 From: tmathern <60901087+tmathern@users.noreply.github.com> Date: Wed, 10 Jun 2026 15:59:47 -0700 Subject: [PATCH 3/3] fix: Debug cleanup --- MEMORY_FINDINGS.md | 146 --------------------------------------------- 1 file changed, 146 deletions(-) delete mode 100644 MEMORY_FINDINGS.md diff --git a/MEMORY_FINDINGS.md b/MEMORY_FINDINGS.md deleted file mode 100644 index 4056e78d..00000000 --- a/MEMORY_FINDINGS.md +++ /dev/null @@ -1,146 +0,0 @@ -# Memory investigation findings - -Date: 2026-06-10. Environment: macOS arm64, Python 3.12, memray 1.19.3, -native library c2pa-v0.86.1 (local build). Methodology and harness live in -`tests/perf/` (see its README). - -## Summary - -Nine memray scenarios were written to cover previously unmeasured paths: -callback signers, sign-failure error paths, abandoned (never-closed) -readers, `ManifestNotFound` / invalid-manifest error loops, the uncached -Reader string APIs, thumbnail/resource transfer in both directions, and -per-iteration Context/Signer churn. After the investigation, only the three -that guard the fixed code paths were kept in `tests/perf/scenarios.py` -(`reader_error_no_manifest`, `builder_error_invalid_manifest`, -`reader_string_apis`); the others measured clean and added suite runtime -without guarding anything. - -**No memory leak was found in c2pa-rs.** Native memory was measured flat -across all scenarios: `leaked_bytes` deltas between 100 and 300 iterations -were within noise (±4 KiB, i.e. zero per-iteration slope) on every scenario, -including both thumbnail/resource paths. - -**One real issue was found and fixed in the Python bindings** (not a native -leak): several hot ctypes patterns created reference-cycle garbage on every -operation, which only the cycle collector could reclaim. This showed up as -live-memory growth of ~366 B per Reader iteration between garbage-collector -runs, plus avoidable gc pressure (3-4 cycle objects per operation). Details -below. - -## Thumbnail / resource handling verdict - -Both directions were profiled explicitly because they were suspected: - -- `reader_string_apis` — per iteration: `detailed_json()`, `crjson()`, - `get_remote_url()`, and `resource_to_stream()` extracting the active - manifest's JPEG thumbnail (31,608 bytes per extraction) from - `tests/fixtures/C.jpg`. Measured: leaked 2.10 MiB at N=100 vs 2.10 MiB at - N=300 (delta −975 B). No growing allocation site above 4 KiB. (Note: - `get_remote_url()` on these embedded-manifest fixtures returns `None` - before any string conversion, so within this scenario it covers the NULL - branch only.) -- `builder_add_resource_thumbnail` — per iteration: `Builder.add_resource` - of a ~90 KB JPEG thumbnail followed by a context sign. Measured: leaked - 2.30 MiB at both N=100 and N=300 (delta −2.2 KiB). No growing site. - -The constant ~2-4 MiB `leaked_bytes` floor in every scenario is the -documented one-time static overhead of loading the native library (see -"Why is leaked_bytes not zero?" in `tests/perf/README.md`), not a leak: it -does not scale with iterations. - -Conclusion: the native `c2pa_reader_resource_to_stream`, -`c2pa_builder_add_resource`, manifest parse and sign paths free everything -they allocate. Nothing to report to c2pa-rs. - -## The issue that was real: ctypes reference cycles in the bindings - -Symptom (measured before the fix): - -- `gc.collect()` after 100 Reader iterations found 344 unreachable objects; - after 50 Builder signs, 208 — even when `close()` was called correctly. -- memray's high-watermark snapshot for `reader_jpeg_with_context` grew - 366 B/iteration (2,694 KiB at N=100 → 2,765 KiB at N=300): cycle garbage - accumulating between collector runs counts as live memory. -- Garbage was ctypes-internal: `PyCArrayType` classes, `LP_C2paReader` - pointer objects, and their type dicts/descriptors. - -Root causes, isolated by measuring each pattern in a bare loop: - -| Pattern | Cycle objects per call | -| --- | --- | -| `ctypes.cast(value, c_char_p)` (string returns, error strings) | 2 | -| `ctypes.cast(ffi_ptr, c_void_p)` (every native free) | 2 | -| `(ctypes.c_char * length)` built per stream-read call | ~0.6 | -| `ctypes.string_at(...)` / direct pointer pass | 0 | - -Fixes applied in `src/c2pa/c2pa.py`: - -1. `_convert_to_py_string` and the error path in - `_parse_operation_result_for_error` now read native strings with - `ctypes.string_at` instead of `ctypes.cast(..., c_char_p)`. -2. `ManagedResource._free_native_ptr` passes the pointer directly to - `c2pa_free` (whose argtype is already `c_void_p`) instead of casting. -3. The per-chunk stream read path wraps the native buffer in a writable - memoryview (`PyMemoryView_FromMemory`) instead of building a - `(c_char * length)` array type — no class creation at all, for any chunk - size. The view is `release()`d in a `finally` right after `readinto`, so - a stream object that stashes the buffer gets a `ValueError` on later - access instead of writing into freed native memory (the old ctypes-array - approach had no such guard), and the reported read count is clamped to - the buffer length. The remaining array-type sites (manifest byte arrays, - signing payloads) were left as plain inline `(c_ubyte * n)` creations: - they run once per operation, lengths there are data-dependent and rarely - repeat, so caching would not hit and the one cyclic class per operation - is negligible next to the operation itself. - -Verified after the fix: - -- `gc.collect()` finds **0** unreachable objects after 100 Reader - iterations (with or without `close()`) and after 50 Builder signs. -- High-watermark growth: −13 B/iteration (flat) for the Reader control. -- Peak RSS without memray: flat at 33.7 MB for N=100/300/600. -- Full unit suite: 234 passed. - -## Other paths checked and clean - -| Scenario | leaked @100 → @300 | Verdict | -| --- | --- | --- | -| signer_from_callback_churn | 4.25 MiB → 4.25 MiB | clean | -| signer_callback_sign_error | 3.48 MiB → 3.48 MiB | clean (error strings freed) | -| stream_abandon_no_close | 2.10 MiB → 2.10 MiB | clean (gc + `__del__` releases native stream) | -| reader_error_no_manifest | 2.08 MiB → 2.08 MiB | clean (partial-init cleanup works) | -| builder_error_invalid_manifest | 2.05 MiB → 2.04 MiB | clean | -| context_churn | 2.30 MiB → 2.30 MiB | clean (`c2pa_context_free` + consumed signer) | -| signer_from_info_churn | 2.04 MiB → 2.04 MiB | clean (`c2pa_signer_free`) | - -One measurement note: `memray`'s `metadata.peak_memory` shows a ~3-5 KiB -per-iteration upward slope even after the fix, while the sum of its -high-watermark allocation records, process RSS, and `leaked_bytes` are all -flat. That residual slope is profiler accounting overhead (it scales with -the number of allocation records), not application memory; use the -high-watermark records or RSS when judging peak behavior across different -iteration counts. - -## Addendum: regression caught in adversarial re-review (2026-06-10) - -The first version of the `string_at` rewrite of `_convert_to_py_string` had a -NULL-handling regression: the `value == 0` guard does not catch -`ctypes.c_void_p(0)` (ctypes instances never compare equal to ints), and -`ctypes.string_at(NULL)` crashes the process where the old -`ctypes.cast(...).value` path returned `None`. Unreachable through current -callers (all pass `int | None` from `c_void_p`-restype functions, and both -are guarded), but the function's own type check admits `c_void_p` instances, -so the latent crash was real. Fixed by normalizing to a raw address first -(`value.value` for `c_void_p`) and bailing out on falsy addresses; verified -against `None`, `0`, `c_void_p(0)`, `c_void_p(None)`, and non-pointer types. - -## Also found during review (not measured as leaking, fixed by design) - -`Signer.from_callback` could leak the native signer pointer if signer -creation failed after `c2pa_signer_create` returned non-null. In practice -this path is unreachable with bad input — the native library defers -certificate validation to signing time (confirmed: garbage PEM creates a -signer successfully; the failure surfaces during `Builder.sign` as -`C2paError.Signature`, covered by the `signer_callback_sign_error` -scenario, which measures clean).