From 95a647b7c6ee650f0b0b5c4c25e56eab46757a11 Mon Sep 17 00:00:00 2001 From: Tania Mathern Date: Tue, 26 Aug 2025 11:05:53 -0700 Subject: [PATCH 01/19] fix: Improve string conversion --- src/c2pa/c2pa.py | 32 ++++++++++++++++++++++---------- 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index 0edc61bb..9f2959e5 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -532,19 +532,31 @@ def _convert_to_py_string(value) -> str: return "" py_string = "" - ptr = ctypes.cast(value, ctypes.c_char_p) - # Only if we got a valid pointer - if ptr and ptr.value is not None: - try: - py_string = ptr.value.decode('utf-8', errors='replace') - except Exception: - py_string = "" + # Validate pointer before casting and freeing + if not isinstance(value, (int, ctypes.c_void_p)) or value == 0: + return "" + + try: + ptr = ctypes.cast(value, ctypes.c_char_p) - # Free the Rust-allocated memory - _lib.c2pa_string_free(value) + # Only if we got a valid pointer with valid content + if ptr and ptr.value is not None: + try: + py_string = ptr.value.decode('utf-8', errors='replace') + except Exception: + py_string = "" + finally: + # Only free if we have a valid pointer + try: + _lib.c2pa_string_free(value) + except Exception: + # Log error but don't crash + pass + except (ctypes.ArgumentError, TypeError, ValueError): + # Invalid pointer type or value + return "" - # In case of invalid pointer, no free (avoids double-free) return py_string From 5441599c38adae06140868dbaba322d498105b06 Mon Sep 17 00:00:00 2001 From: Tania Mathern Date: Tue, 26 Aug 2025 11:17:06 -0700 Subject: [PATCH 02/19] fix: Improve error handling --- src/c2pa/c2pa.py | 31 +++++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index 9f2959e5..36e440f7 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -2166,6 +2166,17 @@ def _sign_internal( if not self._builder: raise C2paError(Builder._ERROR_MESSAGES['closed_error']) + # Validate signer pointer before use + if not signer or not hasattr(signer, '_signer') or not signer._signer: + raise C2paError("Invalid or closed signer") + + # Validate stream pointers before use + if not source_stream or not hasattr(source_stream, '_stream') or not source_stream._stream: + raise C2paError("Invalid source stream") + + if not dest_stream or not hasattr(dest_stream, '_stream') or not dest_stream._stream: + raise C2paError("Invalid destination stream") + if format not in Builder.get_supported_mime_types(): raise C2paError.NotSupported( f"Builder does not support {format}") @@ -2174,14 +2185,18 @@ def _sign_internal( manifest_bytes_ptr = ctypes.POINTER(ctypes.c_ubyte)() # c2pa_builder_sign uses streams - result = _lib.c2pa_builder_sign( - self._builder, - format_str, - source_stream._stream, - dest_stream._stream, - signer._signer, - ctypes.byref(manifest_bytes_ptr) - ) + try: + result = _lib.c2pa_builder_sign( + self._builder, + format_str, + source_stream._stream, + dest_stream._stream, + signer._signer, + ctypes.byref(manifest_bytes_ptr) + ) + except Exception as e: + # Handle errors during the C function call + raise C2paError(f"Error calling c2pa_builder_sign: {str(e)}") if result < 0: error = _parse_operation_result_for_error(_lib.c2pa_error()) From 8a78df68edf7ed057c6071e49a377d345ce5eada Mon Sep 17 00:00:00 2001 From: Tania Mathern Date: Tue, 26 Aug 2025 11:19:16 -0700 Subject: [PATCH 03/19] fix: More checks --- src/c2pa/c2pa.py | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index 36e440f7..a8781e53 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -2170,13 +2170,6 @@ def _sign_internal( if not signer or not hasattr(signer, '_signer') or not signer._signer: raise C2paError("Invalid or closed signer") - # Validate stream pointers before use - if not source_stream or not hasattr(source_stream, '_stream') or not source_stream._stream: - raise C2paError("Invalid source stream") - - if not dest_stream or not hasattr(dest_stream, '_stream') or not dest_stream._stream: - raise C2paError("Invalid destination stream") - if format not in Builder.get_supported_mime_types(): raise C2paError.NotSupported( f"Builder does not support {format}") @@ -2208,17 +2201,12 @@ def _sign_internal( manifest_bytes = b"" if manifest_bytes_ptr and result > 0: try: - # Convert the C pointer to Python bytes temp_buffer = (ctypes.c_ubyte * result)() ctypes.memmove(temp_buffer, manifest_bytes_ptr, result) manifest_bytes = bytes(temp_buffer) except Exception: - # If there's any error accessing the memory, just return - # empty bytes manifest_bytes = b"" finally: - # Always free the C-allocated memory, - # even if we failed to copy manifest bytes try: _lib.c2pa_manifest_bytes_free(manifest_bytes_ptr) except Exception: From 3d4c02a2e252bd7ffdfdad802ea863b19c948c28 Mon Sep 17 00:00:00 2001 From: Tania Mathern Date: Tue, 26 Aug 2025 11:26:08 -0700 Subject: [PATCH 04/19] fix: Some more checks --- src/c2pa/c2pa.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index a8781e53..c13c15c7 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -242,6 +242,12 @@ def __init__(self, alg, sign_cert, private_key, ta_url): private_key: The private key as a string ta_url: The timestamp authority URL as bytes """ + + if sign_cert is None: + raise ValueError("sign_cert must be set") + if private_key is None: + raise ValueError("private_key must be set") + # Handle alg parameter: can be C2paSigningAlg enum # or string (or bytes), convert as needed if isinstance(alg, C2paSigningAlg): From cd206012407a79769bfdb00ee35565fe00d62e45 Mon Sep 17 00:00:00 2001 From: Tania Mathern Date: Tue, 26 Aug 2025 11:28:50 -0700 Subject: [PATCH 05/19] fix: Comment --- src/c2pa/c2pa.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index c13c15c7..4ae8fcf1 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -557,7 +557,7 @@ def _convert_to_py_string(value) -> str: try: _lib.c2pa_string_free(value) except Exception: - # Log error but don't crash + # Ignore clean up issues pass except (ctypes.ArgumentError, TypeError, ValueError): # Invalid pointer type or value From 6c6eb9d7368ef2570d025fe9ff880a80ef64f978 Mon Sep 17 00:00:00 2001 From: Tania Mathern Date: Tue, 26 Aug 2025 11:35:14 -0700 Subject: [PATCH 06/19] fix: Format --- src/c2pa/c2pa.py | 2 +- src/c2pa/lib.py | 33 +++++++++++++++++++-------------- 2 files changed, 20 insertions(+), 15 deletions(-) diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index 4ae8fcf1..ebb7b0f6 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -2296,7 +2296,7 @@ def sign_file(self, try: # Open source file and destination file, then use the sign method with open(source_path, 'rb') as source_file, \ - open(dest_path, 'w+b') as dest_file: + open(dest_path, 'w+b') as dest_file: return self.sign(signer, mime_type, source_file, dest_file) except Exception as e: raise C2paError(f"Error signing file: {str(e)}") from e diff --git a/src/c2pa/lib.py b/src/c2pa/lib.py index af324b93..3a6acd2f 100644 --- a/src/c2pa/lib.py +++ b/src/c2pa/lib.py @@ -16,7 +16,8 @@ # Debug flag for library loading DEBUG_LIBRARY_LOADING = False -# Create a module-specific logger with NullHandler to avoid interfering with global configuration +# Create a module-specific logger with NullHandler to avoid interfering +# with global configuration logger = logging.getLogger("c2pa") logger.addHandler(logging.NullHandler()) @@ -45,7 +46,9 @@ def get_platform_identifier() -> str: elif system == "windows": return "x86_64-pc-windows-msvc" elif system == "linux": - if _get_architecture() in [CPUArchitecture.ARM64.value, CPUArchitecture.AARCH64.value]: + if _get_architecture() in [ + CPUArchitecture.ARM64.value, + CPUArchitecture.AARCH64.value]: return "aarch64-unknown-linux-gnu" return "x86_64-unknown-linux-gnu" else: @@ -107,8 +110,8 @@ def _load_single_library(lib_name: str, The loaded library or None if loading failed """ if DEBUG_LIBRARY_LOADING: # pragma: no cover - logger.info( - f"Searching for library '{lib_name}' in paths: {[str(p) for p in search_paths]}") + logger.info(f"Searching for library '{lib_name}' in paths: { + [str(p) for p in search_paths]}") current_arch = _get_architecture() if DEBUG_LIBRARY_LOADING: # pragma: no cover logger.info(f"Current architecture: {current_arch}") @@ -125,8 +128,8 @@ def _load_single_library(lib_name: str, except Exception as e: error_msg = str(e) if "incompatible architecture" in error_msg: - logger.error( - f"Architecture mismatch: Library at {lib_path} is not compatible with current architecture {current_arch}") + logger.error(f"Architecture mismatch: Library at { + lib_path} is not compatible with current architecture {current_arch}") logger.error(f"Error details: {error_msg}") else: logger.error( @@ -224,8 +227,8 @@ def dynamically_load_library( if lib: return lib else: - logger.error( - f"Could not find library {env_lib_name} in any of the search paths") + logger.error(f"Could not find library { + env_lib_name} in any of the search paths") # Continue with normal loading if environment variable library # name fails except Exception as e: @@ -241,18 +244,20 @@ def dynamically_load_library( if not lib: platform_id = get_platform_identifier() current_arch = _get_architecture() - logger.error( - f"Could not find {lib_name} in any of the search paths: {[str(p) for p in possible_paths]}") + logger.error(f"Could not find {lib_name} in any of the search paths: { + [str(p) for p in possible_paths]}") logger.error( f"Platform: {platform_id}, Architecture: {current_arch}") - raise RuntimeError( - f"Could not find {lib_name} in any of the search paths (Platform: {platform_id}, Architecture: {current_arch})") + raise RuntimeError(f"Could not find {lib_name} in any of the search paths (Platform: { + platform_id}, Architecture: {current_arch})") return lib # Default path (no library name provided in the environment) c2pa_lib = _load_single_library(c2pa_lib_name, possible_paths) if not c2pa_lib: - logger.error(f"Could not find {c2pa_lib_name} in any of the search paths: {[str(p) for p in possible_paths]}") - raise RuntimeError(f"Could not find {c2pa_lib_name} in any of the search paths") + logger.error(f"Could not find {c2pa_lib_name} in any of the search paths: { + [str(p) for p in possible_paths]}") + raise RuntimeError( + f"Could not find {c2pa_lib_name} in any of the search paths") return c2pa_lib From 84ca45eb0ef03eb3fbd8c4d993c9ad60fe5a9e8a Mon Sep 17 00:00:00 2001 From: Tania Mathern Date: Tue, 26 Aug 2025 13:35:58 -0700 Subject: [PATCH 07/19] fix: Docs --- src/c2pa/c2pa.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index ebb7b0f6..13c296e5 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -622,7 +622,7 @@ def sdk_version() -> str: Returns the underlying c2pa-rs/c2pa-c-ffi version string """ vstr = version() - # Example: "c2pa-c/0.49.5 c2pa-rs/0.49.5" + # Example: "c2pa-c-ffi/0.59.1 c2pa-rs/0.59.1" for part in vstr.split(): if part.startswith("c2pa-rs/"): return part.split("/", 1)[1] From 4ee40c7f01adfb4ce03abb5d27982c37e0f3b48d Mon Sep 17 00:00:00 2001 From: Tania Mathern Date: Tue, 26 Aug 2025 13:49:04 -0700 Subject: [PATCH 08/19] fix: Improve error handling --- src/c2pa/c2pa.py | 117 +++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 104 insertions(+), 13 deletions(-) diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index 13c296e5..03bc9ec3 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -1194,21 +1194,67 @@ class Reader: @classmethod def get_supported_mime_types(cls) -> list[str]: + """Get the list of supported MIME types for the Reader. + + This method retrieves supported MIME types from the native library + with proper pointer validation and error handling. + + Returns: + List of supported MIME type strings + + Raises: + C2paError: If there was an error retrieving the MIME types + """ if cls._supported_mime_types_cache is not None: return cls._supported_mime_types_cache count = ctypes.c_size_t() arr = _lib.c2pa_reader_supported_mime_types(ctypes.byref(count)) + # Validate the returned array pointer + if not arr: + # If no array returned, check for errors + error = _parse_operation_result_for_error(_lib.c2pa_error()) + if error: + raise C2paError(f"Failed to get supported MIME types: {error}") + # Return empty list if no error but no array + return [] + + # Validate count value + if count.value <= 0: + # Free the array even if count is invalid + try: + _lib.c2pa_free_string_array(arr, count.value) + except Exception: + pass + return [] + try: - # CDecode values to place them in Python managed memory - result = [arr[i].decode("utf-8") for i in range(count.value)] + result = [] + for i in range(count.value): + try: + # Validate each array element before accessing + if arr[i] is None: + continue + + mime_type = arr[i].decode("utf-8", errors='replace') + if mime_type: + result.append(mime_type) + except Exception: + # Ignore cleanup errors + continue finally: - # Release native memory, as per API contract - # c2pa_reader_supported_mime_types must call c2pa_free_string_array - _lib.c2pa_free_string_array(arr, count.value) + # Always free the native memory, even if string extraction fails + try: + _lib.c2pa_free_string_array(arr, count.value) + except Exception: + # Ignore cleanup errors + pass + + # Cache the result + if result: + cls._supported_mime_types_cache = result - cls._supported_mime_types_cache = result return cls._supported_mime_types_cache def __init__(self, @@ -1798,22 +1844,67 @@ class Builder: @classmethod def get_supported_mime_types(cls) -> list[str]: + """Get the list of supported MIME types for the Builder. + + This method retrieves supported MIME types from the native library + with proper pointer validation and error handling. + + Returns: + List of supported MIME type strings + + Raises: + C2paError: If there was an error retrieving the MIME types + """ if cls._supported_mime_types_cache is not None: return cls._supported_mime_types_cache count = ctypes.c_size_t() arr = _lib.c2pa_builder_supported_mime_types(ctypes.byref(count)) + # Validate the returned array pointer + if not arr: + # If no array returned, check for errors + error = _parse_operation_result_for_error(_lib.c2pa_error()) + if error: + raise C2paError(f"Failed to get supported MIME types: {error}") + # Return empty list if no error but no array + return [] + + # Validate count value + if count.value <= 0: + # Free the array even if count is invalid + try: + _lib.c2pa_free_string_array(arr, count.value) + except Exception: + pass + return [] + try: - # CDecode values to place them in Python managed memory - result = [arr[i].decode("utf-8") for i in range(count.value)] + result = [] + for i in range(count.value): + try: + # Validate each array element before accessing + if arr[i] is None: + continue + + mime_type = arr[i].decode("utf-8", errors='replace') + if mime_type: + result.append(mime_type) + except Exception: + # Ignore decoding failures + continue finally: - # Release native memory, as per API contract - # c2pa_builder_supported_mime_types must call - # c2pa_free_string_array - _lib.c2pa_free_string_array(arr, count.value) + # Always free the native memory, even if string extraction fails + try: + _lib.c2pa_free_string_array(arr, count.value) + except Exception: + # Ignore cleanup errors + pass + + # Cache the result + if result: + cls._supported_mime_types_cache = result - cls._supported_mime_types_cache = result return cls._supported_mime_types_cache def __init__(self, manifest_json: Any): From bec60709db7834fd61fd57880d8f87882db0915b Mon Sep 17 00:00:00 2001 From: Tania Mathern Date: Tue, 26 Aug 2025 14:09:41 -0700 Subject: [PATCH 09/19] fix: Improve state handling --- src/c2pa/c2pa.py | 98 +++++++++++++++++++++++++++++++++++------------- 1 file changed, 71 insertions(+), 27 deletions(-) diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index 03bc9ec3..71d5a32c 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -1102,9 +1102,27 @@ def __exit__(self, exc_type, exc_val, exc_tb): self.close() def __del__(self): - """Ensure resources are cleaned up if close() wasn't called.""" - if hasattr(self, '_closed'): - self.close() + """Ensure resources are cleaned up if close() wasn't called. + + This destructor only cleans up if the object hasn't been explicitly closed. + """ + try: + # Only cleanup if not already closed and we have a valid stream + if hasattr(self, '_closed') and not self._closed: + if hasattr(self, '_stream') and self._stream: + # Use internal cleanup to avoid calling close() which could cause issues + try: + _lib.c2pa_release_stream(self._stream) + except Exception: + # Destructors shouldn't raise exceptions, just log silently + pass + finally: + self._stream = None + self._closed = True + self._initialized = False + except Exception: + # Destructors must not raise exceptions + pass def close(self): """Release the stream resources. @@ -1918,6 +1936,8 @@ def __init__(self, manifest_json: Any): C2paError.Encoding: If manifest JSON contains invalid UTF-8 chars C2paError.Json: If the manifest JSON cannot be serialized """ + # Initialize state immediately to prevent race conditions + self._closed = False self._builder = None if not isinstance(manifest_json, str): @@ -1981,6 +2001,8 @@ def from_archive(cls, stream: Any) -> 'Builder': builder._builder = _lib.c2pa_builder_from_archive(stream_obj._stream) if not builder._builder: + # Clean up the stream object if builder creation fails + stream_obj.close() error = _parse_operation_result_for_error(_lib.c2pa_error()) if error: raise C2paError(error) @@ -1989,9 +2011,12 @@ def from_archive(cls, stream: Any) -> 'Builder': return builder def __del__(self): - """Ensure resources are cleaned up if close() wasn't called.""" - if hasattr(self, '_closed'): - self.close() + """Ensure resources are cleaned up if close() wasn't called. + + This destructor safely handles cleanup without causing double frees. + It only cleans up if the object hasn't been explicitly closed. + """ + self._cleanup_resources() def close(self): """Release the builder resources. @@ -2001,25 +2026,14 @@ def close(self): Errors during cleanup are logged but not raised to ensure cleanup. Multiple calls to close() are handled gracefully. """ - # Track if we've already cleaned up - if not hasattr(self, '_closed'): - self._closed = False - if self._closed: return try: - # Clean up builder - if hasattr(self, '_builder') and self._builder: - try: - _lib.c2pa_builder_free(self._builder) - except Exception as e: - print( - Builder._ERROR_MESSAGES['builder_cleanup'].format( - str(e)), file=sys.stderr) - finally: - self._builder = None + # Use the internal cleanup method + self._cleanup_resources() except Exception as e: + # Log any unexpected errors during close print( Builder._ERROR_MESSAGES['cleanup_error'].format( str(e)), file=sys.stderr) @@ -2039,9 +2053,7 @@ def set_no_embed(self): into the asset when signing. This is useful when creating cloud or sidecar manifests. """ - if not self._builder: - raise C2paError(Builder._ERROR_MESSAGES['closed_error']) - + self._ensure_valid_state() _lib.c2pa_builder_set_no_embed(self._builder) def set_remote_url(self, remote_url: str): @@ -2056,8 +2068,7 @@ def set_remote_url(self, remote_url: str): Raises: C2paError: If there was an error setting the remote URL """ - if not self._builder: - raise C2paError(Builder._ERROR_MESSAGES['closed_error']) + self._ensure_valid_state() url_str = remote_url.encode('utf-8') result = _lib.c2pa_builder_set_remote_url(self._builder, url_str) @@ -2080,8 +2091,7 @@ def add_resource(self, uri: str, stream: Any): Raises: C2paError: If there was an error adding the resource """ - if not self._builder: - raise C2paError(Builder._ERROR_MESSAGES['closed_error']) + self._ensure_valid_state() uri_str = uri.encode('utf-8') with Stream(stream) as stream_obj: @@ -2392,6 +2402,40 @@ def sign_file(self, except Exception as e: raise C2paError(f"Error signing file: {str(e)}") from e + def _ensure_valid_state(self): + """Ensure the builder is in a valid state for operations. + + Raises: + C2paError: If the builder is closed or invalid + """ + if self._closed: + raise C2paError(Builder._ERROR_MESSAGES['closed_error']) + if not self._builder: + raise C2paError(Builder._ERROR_MESSAGES['closed_error']) + + def _cleanup_resources(self): + """Internal cleanup method that safely releases native resources. + + This method handles the actual cleanup logic and can be called + from both close() and __del__ without causing double frees. + """ + try: + # Only cleanup if not already closed and we have a valid builder + if hasattr(self, '_closed') and not self._closed: + if hasattr(self, '_builder') and self._builder and self._builder != 0: + try: + _lib.c2pa_builder_free(self._builder) + except Exception: + # Log cleanup errors but don't raise exceptions + pass + finally: + # Always clear the pointer and mark as closed + self._builder = None + self._closed = True + except Exception: + # Ensure we don't raise exceptions during cleanup + pass + def format_embeddable(format: str, manifest_bytes: bytes) -> tuple[int, bytes]: """Convert a binary C2PA manifest into an embeddable version. From 85f0dee1977d1cc70785f0919909c6f44e861405 Mon Sep 17 00:00:00 2001 From: Tania Mathern Date: Tue, 26 Aug 2025 14:28:56 -0700 Subject: [PATCH 10/19] fix: Refactor --- src/c2pa/c2pa.py | 138 ++++++++++++++++++++++++++++------------------- 1 file changed, 82 insertions(+), 56 deletions(-) diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index 71d5a32c..49faefaf 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -1293,9 +1293,14 @@ def __init__(self, contain invalid UTF-8 characters """ + self._closed = False self._reader = None 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 + if stream is None: # If we don't get a stream as param: # Create a stream from the file path in format_or_path @@ -1342,13 +1347,13 @@ def __init__(self, ) # Store the file to close it later - self._file_like_stream = file + self._backing_file = file except Exception as e: if self._own_stream: self._own_stream.close() - if hasattr(self, '_file_like_stream'): - self._file_like_stream.close() + if self._backing_file: + self._backing_file.close() raise C2paError.Io( Reader._ERROR_MESSAGES['io_error'].format( str(e))) @@ -1402,12 +1407,13 @@ def __init__(self, ) ) - self._file_like_stream = file + # File stream we opened and own + self._backing_file = file except Exception as e: if self._own_stream: self._own_stream.close() - if hasattr(self, '_file_like_stream'): - self._file_like_stream.close() + if self._backing_file: + self._backing_file.close() raise C2paError.Io( Reader._ERROR_MESSAGES['io_error'].format( str(e))) @@ -1460,60 +1466,83 @@ def __enter__(self): def __exit__(self, exc_type, exc_val, exc_tb): self.close() + def __del__(self): + """Ensure resources are cleaned up if close() wasn't called. + + This destructor handles cleanup without causing double frees. + It only cleans up if the object hasn't been explicitly closed. + """ + self._cleanup_resources() + + def _ensure_valid_state(self): + """Ensure the reader is in a valid state for operations. + + Raises: + C2paError: If the reader is closed or invalid + """ + if self._closed or not self._reader: + raise C2paError("Reader is closed") + + def _cleanup_resources(self): + """Internal cleanup method that releases native resources. + + This method handles the actual cleanup logic and can be called + from both close() and __del__ without causing double frees. + """ + try: + # Only cleanup if not already closed and we have a valid reader + if hasattr(self, '_closed') and not self._closed: + # Clean up reader + if hasattr(self, '_reader') and self._reader: + try: + _lib.c2pa_reader_free(self._reader) + except Exception: + # Cleanup failure doesn't raise exceptions + pass + finally: + self._reader = None + + # Clean up stream + if hasattr(self, '_own_stream') and self._own_stream: + try: + self._own_stream.close() + except Exception: + # Cleanup failure doesn't raise exceptions + pass + finally: + self._own_stream = None + + # Clean up backing file + if self._backing_file: + try: + self._backing_file.close() + except Exception: + # Cleanup failure doesn't raise exceptions + pass + finally: + self._backing_file = None + + self._closed = True + except Exception: + # Ensure we don't raise exceptions during cleanup + pass + def close(self): - """Release the reader resources. + """Release the reader resources safely. This method ensures all resources are properly cleaned up, even if errors occur during cleanup. Errors during cleanup are logged but not raised to ensure cleanup. Multiple calls to close() are handled gracefully. """ - - # Track if we've already cleaned up - if not hasattr(self, '_closed'): - self._closed = False - if self._closed: - return + return # Already closed, safe to return try: - # Clean up reader - if hasattr(self, '_reader') and self._reader: - try: - _lib.c2pa_reader_free(self._reader) - except Exception as e: - print( - Reader._ERROR_MESSAGES['reader_cleanup_error'].format( - str(e)), file=sys.stderr) - finally: - self._reader = None - - # Clean up stream - if hasattr(self, '_own_stream') and self._own_stream: - try: - self._own_stream.close() - except Exception as e: - print( - Reader._ERROR_MESSAGES['stream_error'].format( - str(e)), file=sys.stderr) - finally: - self._own_stream = None - - # Clean up file - if hasattr(self, '_file_like_stream'): - try: - self._file_like_stream.close() - except Exception as e: - print( - Reader._ERROR_MESSAGES['file_error'].format( - str(e)), file=sys.stderr) - finally: - self._file_like_stream = None - - # Clear any stored strings - if hasattr(self, '_strings'): - self._strings.clear() + # Use the internal cleanup method + self._cleanup_resources() except Exception as e: + # Log any unexpected errors during close print( Reader._ERROR_MESSAGES['cleanup_error'].format( str(e)), file=sys.stderr) @@ -1529,9 +1558,7 @@ def json(self) -> str: Raises: C2paError: If there was an error getting the JSON """ - - if not self._reader: - raise C2paError("Reader is closed") + self._ensure_valid_state() result = _lib.c2pa_reader_json(self._reader) if result is None: @@ -1555,13 +1582,12 @@ def resource_to_stream(self, uri: str, stream: Any) -> int: Raises: C2paError: If there was an error writing the resource to stream """ - if not self._reader: - raise C2paError("Reader is closed") + self._ensure_valid_state() - self._uri_str = uri.encode('utf-8') + uri_str = uri.encode('utf-8') with Stream(stream) as stream_obj: result = _lib.c2pa_reader_resource_to_stream( - self._reader, self._uri_str, stream_obj._stream) + self._reader, uri_str, stream_obj._stream) if result < 0: error = _parse_operation_result_for_error(_lib.c2pa_error()) From 6294acdf980a3b49e31733324979bd6fa9d455d5 Mon Sep 17 00:00:00 2001 From: Tania Mathern Date: Tue, 26 Aug 2025 14:33:14 -0700 Subject: [PATCH 11/19] fix: Simplify reader --- src/c2pa/c2pa.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index 49faefaf..0c6cf8ff 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -1317,7 +1317,7 @@ def __init__(self, f"Reader does not support {mime_type}") try: - self._mime_type_str = mime_type.encode('utf-8') + mime_type_str = mime_type.encode('utf-8') except UnicodeError as e: raise C2paError.Encoding( Reader._ERROR_MESSAGES['encoding_error'].format( @@ -1329,7 +1329,7 @@ def __init__(self, self._own_stream = Stream(file) self._reader = _lib.c2pa_reader_from_stream( - self._mime_type_str, + mime_type_str, self._own_stream._stream ) @@ -1371,11 +1371,11 @@ def __init__(self, self._own_stream = Stream(file) format_str = str(format_or_path) - self._format_str = format_str.encode('utf-8') + format_bytes = format_str.encode('utf-8') if manifest_data is None: self._reader = _lib.c2pa_reader_from_stream( - self._format_str, self._own_stream._stream) + format_bytes, self._own_stream._stream) else: if not isinstance(manifest_data, bytes): raise TypeError( @@ -1387,7 +1387,7 @@ def __init__(self, manifest_data) self._reader = ( _lib.c2pa_reader_from_manifest_data_and_stream( - self._format_str, + format_bytes, self._own_stream._stream, manifest_array, len(manifest_data), @@ -1425,12 +1425,12 @@ def __init__(self, f"Reader does not support {format_str}") # Use the provided stream - self._format_str = format_str.encode('utf-8') + format_bytes = format_str.encode('utf-8') with Stream(stream) as stream_obj: if manifest_data is None: self._reader = _lib.c2pa_reader_from_stream( - self._format_str, stream_obj._stream) + format_bytes, stream_obj._stream) else: if not isinstance(manifest_data, bytes): raise TypeError( @@ -1442,7 +1442,7 @@ def __init__(self, manifest_data) self._reader = ( _lib.c2pa_reader_from_manifest_data_and_stream( - self._format_str, + format_bytes, stream_obj._stream, manifest_array, len(manifest_data) From aff9d33d52956d5f2303a8112a2bad7ca259d979 Mon Sep 17 00:00:00 2001 From: Tania Mathern Date: Tue, 26 Aug 2025 14:33:57 -0700 Subject: [PATCH 12/19] fix: Format --- src/c2pa/c2pa.py | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index 0c6cf8ff..51669ec7 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -1102,19 +1102,23 @@ def __exit__(self, exc_type, exc_val, exc_tb): self.close() def __del__(self): - """Ensure resources are cleaned up if close() wasn't called. + """Ensure resources are cleaned up if close() + wasn't called. - This destructor only cleans up if the object hasn't been explicitly closed. + This destructor only cleans up if the object + hasn't been explicitly closed. """ try: # Only cleanup if not already closed and we have a valid stream if hasattr(self, '_closed') and not self._closed: if hasattr(self, '_stream') and self._stream: - # Use internal cleanup to avoid calling close() which could cause issues + # Use internal cleanup to avoid calling close() which could + # cause issues try: _lib.c2pa_release_stream(self._stream) except Exception: - # Destructors shouldn't raise exceptions, just log silently + # Destructors shouldn't raise exceptions, just log + # silently pass finally: self._stream = None @@ -2448,7 +2452,9 @@ def _cleanup_resources(self): try: # Only cleanup if not already closed and we have a valid builder if hasattr(self, '_closed') and not self._closed: - if hasattr(self, '_builder') and self._builder and self._builder != 0: + if hasattr( + self, + '_builder') and self._builder and self._builder != 0: try: _lib.c2pa_builder_free(self._builder) except Exception: From 8687af97df5e9b8ec7063c59161b453f215f8a98 Mon Sep 17 00:00:00 2001 From: Tania Mathern Date: Tue, 26 Aug 2025 14:37:02 -0700 Subject: [PATCH 13/19] fix: Format 2 --- Makefile | 2 +- src/c2pa/lib.py | 18 ++++++------------ 2 files changed, 7 insertions(+), 13 deletions(-) diff --git a/Makefile b/Makefile index 4c501458..02105bff 100644 --- a/Makefile +++ b/Makefile @@ -98,7 +98,7 @@ check-format: # Formats Python source code using autopep8 with aggressive settings format: - autopep8 --aggressive --aggressive --in-place src/c2pa/*.py + autopep8 --aggressive --aggressive --in-place src/c2pa/c2pa.py # Downloads the required native artifacts for the specified version download-native-artifacts: diff --git a/src/c2pa/lib.py b/src/c2pa/lib.py index 3a6acd2f..1b676f19 100644 --- a/src/c2pa/lib.py +++ b/src/c2pa/lib.py @@ -110,8 +110,7 @@ def _load_single_library(lib_name: str, The loaded library or None if loading failed """ if DEBUG_LIBRARY_LOADING: # pragma: no cover - logger.info(f"Searching for library '{lib_name}' in paths: { - [str(p) for p in search_paths]}") + logger.info(f"Searching for library '{lib_name}' in paths: {[str(p) for p in search_paths]}") current_arch = _get_architecture() if DEBUG_LIBRARY_LOADING: # pragma: no cover logger.info(f"Current architecture: {current_arch}") @@ -128,8 +127,7 @@ def _load_single_library(lib_name: str, except Exception as e: error_msg = str(e) if "incompatible architecture" in error_msg: - logger.error(f"Architecture mismatch: Library at { - lib_path} is not compatible with current architecture {current_arch}") + logger.error(f"Architecture mismatch: Library at {lib_path} is not compatible with current architecture {current_arch}") logger.error(f"Error details: {error_msg}") else: logger.error( @@ -227,8 +225,7 @@ def dynamically_load_library( if lib: return lib else: - logger.error(f"Could not find library { - env_lib_name} in any of the search paths") + logger.error(f"Could not find library {env_lib_name} in any of the search paths") # Continue with normal loading if environment variable library # name fails except Exception as e: @@ -244,19 +241,16 @@ def dynamically_load_library( if not lib: platform_id = get_platform_identifier() current_arch = _get_architecture() - logger.error(f"Could not find {lib_name} in any of the search paths: { - [str(p) for p in possible_paths]}") + logger.error(f"Could not find {lib_name} in any of the search paths: {[str(p) for p in possible_paths]}") logger.error( f"Platform: {platform_id}, Architecture: {current_arch}") - raise RuntimeError(f"Could not find {lib_name} in any of the search paths (Platform: { - platform_id}, Architecture: {current_arch})") + raise RuntimeError(f"Could not find {lib_name} in any of the search paths (Platform: {platform_id}, Architecture: {current_arch})") return lib # Default path (no library name provided in the environment) c2pa_lib = _load_single_library(c2pa_lib_name, possible_paths) if not c2pa_lib: - logger.error(f"Could not find {c2pa_lib_name} in any of the search paths: { - [str(p) for p in possible_paths]}") + logger.error(f"Could not find {c2pa_lib_name} in any of the search paths: {[str(p) for p in possible_paths]}") raise RuntimeError( f"Could not find {c2pa_lib_name} in any of the search paths") From 732d45654963617916ba16a8ead2a2ba407fcf9d Mon Sep 17 00:00:00 2001 From: Tania Mathern Date: Tue, 26 Aug 2025 14:37:49 -0700 Subject: [PATCH 14/19] fix: Format 3 --- src/c2pa/lib.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/c2pa/lib.py b/src/c2pa/lib.py index 1b676f19..f71b4dfe 100644 --- a/src/c2pa/lib.py +++ b/src/c2pa/lib.py @@ -251,7 +251,6 @@ def dynamically_load_library( c2pa_lib = _load_single_library(c2pa_lib_name, possible_paths) if not c2pa_lib: logger.error(f"Could not find {c2pa_lib_name} in any of the search paths: {[str(p) for p in possible_paths]}") - raise RuntimeError( - f"Could not find {c2pa_lib_name} in any of the search paths") + raise RuntimeError(f"Could not find {c2pa_lib_name} in any of the search paths") return c2pa_lib From 2f8394965d04b30cc5a7b5ac4fb4c95f9c5b3184 Mon Sep 17 00:00:00 2001 From: Tania Mathern Date: Tue, 26 Aug 2025 14:58:51 -0700 Subject: [PATCH 15/19] fix: Improve Signer cleanup --- src/c2pa/c2pa.py | 58 ++++++++++++++++++++++++++++++++++-------------- 1 file changed, 41 insertions(+), 17 deletions(-) diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index 51669ec7..e4b8803b 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -1102,8 +1102,7 @@ def __exit__(self, exc_type, exc_val, exc_tb): self.close() def __del__(self): - """Ensure resources are cleaned up if close() - wasn't called. + """Ensure resources are cleaned up if close()wasn't called. This destructor only cleans up if the object hasn't been explicitly closed. @@ -1217,9 +1216,7 @@ class Reader: @classmethod def get_supported_mime_types(cls) -> list[str]: """Get the list of supported MIME types for the Reader. - - This method retrieves supported MIME types from the native library - with proper pointer validation and error handling. + This method retrieves supported MIME types from the native library. Returns: List of supported MIME type strings @@ -1808,6 +1805,41 @@ def __exit__(self, exc_type, exc_val, exc_tb): """Context manager exit.""" self.close() + def _cleanup_resources(self): + """Internal cleanup method that safely releases native resources. + + This method handles the actual cleanup logic and can be called + from both close() and __del__ without causing double frees. + """ + try: + # Only cleanup if not already closed and we have a valid signer + if hasattr(self, '_closed') and not self._closed: + if hasattr(self, '_signer') and self._signer: + try: + _lib.c2pa_signer_free(self._signer) + except Exception: + # Cleanup failure doesn't raise exceptions + pass + finally: + self._signer = None + + # Clear callback reference to prevent cycles + if hasattr(self, '_callback_cb'): + self._callback_cb = None + + self._closed = True + except Exception: + # Ensure we don't raise exceptions during cleanup + pass + + def __del__(self): + """Ensure resources are cleaned up if close() wasn't called. + + This destructor safely handles cleanup without causing double frees. + It only cleans up if the object hasn't been explicitly closed. + """ + self._cleanup_resources() + def close(self): """Release the signer resources. @@ -1820,16 +1852,10 @@ def close(self): return try: - if self._signer: - try: - _lib.c2pa_signer_free(self._signer) - except Exception as e: - print( - Signer._ERROR_MESSAGES['signer_cleanup'].format( - str(e)), file=sys.stderr) - finally: - self._signer = None + # Use the internal cleanup method + self._cleanup_resources() except Exception as e: + # Log any unexpected errors during close print( Signer._ERROR_MESSAGES['cleanup_error'].format( str(e)), file=sys.stderr) @@ -1893,9 +1919,7 @@ class Builder: @classmethod def get_supported_mime_types(cls) -> list[str]: """Get the list of supported MIME types for the Builder. - - This method retrieves supported MIME types from the native library - with proper pointer validation and error handling. + This method retrieves supported MIME types from the native library. Returns: List of supported MIME type strings From 5026a0d5f2ade34f0e335e3f47af8e8de1e99a16 Mon Sep 17 00:00:00 2001 From: Tania Mathern Date: Tue, 26 Aug 2025 15:03:28 -0700 Subject: [PATCH 16/19] fix: Use logger instead of print --- src/c2pa/c2pa.py | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index e4b8803b..8a0cdbea 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -7,7 +7,7 @@ from pathlib import Path from typing import Optional, Union, Callable, Any, overload import io -from .lib import dynamically_load_library +from .lib import dynamically_load_library, logger import mimetypes # Define required function names @@ -1147,9 +1147,9 @@ def close(self): try: _lib.c2pa_release_stream(self._stream) except Exception as e: - print( + logger.error( Stream._ERROR_MESSAGES['stream_error'].format( - str(e)), file=sys.stderr) + str(e))) finally: self._stream = None @@ -1159,14 +1159,14 @@ def close(self): try: setattr(self, attr, None) except Exception as e: - print( + logger.error( Stream._ERROR_MESSAGES['callback_error'].format( - attr, str(e)), file=sys.stderr) + attr, str(e))) except Exception as e: - print( + logger.error( Stream._ERROR_MESSAGES['cleanup_error'].format( - str(e)), file=sys.stderr) + str(e))) finally: self._closed = True self._initialized = False @@ -1544,9 +1544,9 @@ def close(self): self._cleanup_resources() except Exception as e: # Log any unexpected errors during close - print( + logger.error( Reader._ERROR_MESSAGES['cleanup_error'].format( - str(e)), file=sys.stderr) + str(e))) finally: self._closed = True @@ -1748,9 +1748,9 @@ def wrapped_callback( # Native code expects the signed len to be returned, we oblige return actual_len except Exception as e: - print( + logger.error( cls._ERROR_MESSAGES['callback_error'].format( - str(e)), file=sys.stderr) + str(e))) # Error: exception raised, invalid so return -1, # native code will handle the error when seeing -1 return -1 @@ -1856,9 +1856,9 @@ def close(self): self._cleanup_resources() except Exception as e: # Log any unexpected errors during close - print( + logger.error( Signer._ERROR_MESSAGES['cleanup_error'].format( - str(e)), file=sys.stderr) + str(e))) finally: self._closed = True @@ -2088,9 +2088,9 @@ def close(self): self._cleanup_resources() except Exception as e: # Log any unexpected errors during close - print( + logger.error( Builder._ERROR_MESSAGES['cleanup_error'].format( - str(e)), file=sys.stderr) + str(e))) finally: self._closed = True From 62e2d0fbe566aa54ec02aae4a34e66190c0080b8 Mon Sep 17 00:00:00 2001 From: Tania Mathern Date: Tue, 26 Aug 2025 15:21:25 -0700 Subject: [PATCH 17/19] fix: Improve logging --- src/c2pa/c2pa.py | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index 8a0cdbea..132a1a6f 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -865,6 +865,7 @@ def sign_file( try: os.remove(dest_path) except OSError: + logger.warning("Failed to remove destination file") pass # Ignore cleanup errors # Re-raise the error @@ -1116,8 +1117,8 @@ def __del__(self): try: _lib.c2pa_release_stream(self._stream) except Exception: - # Destructors shouldn't raise exceptions, just log - # silently + # Destructors shouldn't raise exceptions + logger.warning("Failed to release Stream") pass finally: self._stream = None @@ -1499,6 +1500,9 @@ def _cleanup_resources(self): _lib.c2pa_reader_free(self._reader) except Exception: # Cleanup failure doesn't raise exceptions + logger.warning( + "Failed to free native Reader resources" + ) pass finally: self._reader = None @@ -1509,16 +1513,18 @@ def _cleanup_resources(self): self._own_stream.close() except Exception: # Cleanup failure doesn't raise exceptions + logger.warning("Failed to close Reader stream") pass finally: self._own_stream = None - # Clean up backing file + # Clean up backing file (if needed) if self._backing_file: try: self._backing_file.close() except Exception: # Cleanup failure doesn't raise exceptions + logger.warning("Failed to close Reader backing file") pass finally: self._backing_file = None @@ -1819,6 +1825,9 @@ def _cleanup_resources(self): _lib.c2pa_signer_free(self._signer) except Exception: # Cleanup failure doesn't raise exceptions + logger.warning( + "Failed to free C2PA Signer during cleanup" + ) pass finally: self._signer = None @@ -2372,6 +2381,9 @@ def _sign_internal( _lib.c2pa_manifest_bytes_free(manifest_bytes_ptr) except Exception: # Ignore errors during cleanup + logger.warning( + "Failed to release native manifest bytes memory" + ) pass return manifest_bytes @@ -2483,6 +2495,9 @@ def _cleanup_resources(self): _lib.c2pa_builder_free(self._builder) except Exception: # Log cleanup errors but don't raise exceptions + logger.warning( + "Failed to release native Builder resources" + ) pass finally: # Always clear the pointer and mark as closed From 0b623e370a3df0537ff95810fcbdebf5c280d204 Mon Sep 17 00:00:00 2001 From: Tania Mathern Date: Tue, 26 Aug 2025 15:33:03 -0700 Subject: [PATCH 18/19] fix: Fix logger strucutre --- src/c2pa/c2pa.py | 7 ++++++- src/c2pa/lib.py | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/c2pa/c2pa.py b/src/c2pa/c2pa.py index 132a1a6f..9388d182 100644 --- a/src/c2pa/c2pa.py +++ b/src/c2pa/c2pa.py @@ -4,12 +4,17 @@ import sys import os import warnings +import logging from pathlib import Path from typing import Optional, Union, Callable, Any, overload import io -from .lib import dynamically_load_library, logger +from .lib import dynamically_load_library import mimetypes +# Create a module-specific logger +logger = logging.getLogger("c2pa") +logger.addHandler(logging.NullHandler()) + # Define required function names _REQUIRED_FUNCTIONS = [ 'c2pa_version', diff --git a/src/c2pa/lib.py b/src/c2pa/lib.py index f71b4dfe..35a716b6 100644 --- a/src/c2pa/lib.py +++ b/src/c2pa/lib.py @@ -18,7 +18,7 @@ # Create a module-specific logger with NullHandler to avoid interfering # with global configuration -logger = logging.getLogger("c2pa") +logger = logging.getLogger("c2pa.loader") logger.addHandler(logging.NullHandler()) From 2b9507434e1d1dd7317cf1630fd82feff6587750 Mon Sep 17 00:00:00 2001 From: Tania Mathern Date: Fri, 29 Aug 2025 07:23:37 -0700 Subject: [PATCH 19/19] fix: Bump version number to rpepare release --- pyproject.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index 00965e12..e9b098d3 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta" [project] name = "c2pa-python" -version = "0.17.0" +version = "0.17.1" requires-python = ">=3.10" description = "Python bindings for the C2PA Content Authenticity Initiative (CAI) library" readme = { file = "README.md", content-type = "text/markdown" }