feat: Adds thread safe Settings and Context support to c_ffi_api #1783

gpeacock · 2026-01-27T18:23:30Z

New C2paSettings and C2paContext along with related Reader and Builder updates.

…rt (still WIP)

…and update stream testing code.

New C FFI Functions Added: c2pa_settings_new() - Creates a new settings object c2pa_settings_update_from_string() - Updates settings from JSON/TOML string c2pa_settings_set_value() - Sets individual config values using dot notation (with JSON parsing) c2pa_context_new() - Creates a new context object c2pa_context_set_settings() - Applies settings to a context c2pa_free() - Universal free function for all C2PA objects There are also new set apis in Rust corresponding to Contex-with apis. This is requirement for bindings

codspeed-hq · 2026-01-27T18:31:11Z

Merging this PR will not alter performance

✅ 16 untouched benchmarks
⏩ 2 skipped benchmarks¹

_{Comparing gpeacock/context_ffi (d28e863) with main (80a74d5)}

2 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

codecov · 2026-01-27T18:38:49Z

Codecov Report

❌ Patch coverage is 56.27119% with 258 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.48%. Comparing base (054ca77) to head (d28e863).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
c2pa_c_ffi/src/c_api.rs	53.47%	134 Missing ⚠️
c2pa_c_ffi/src/cimpl/utils.rs	61.78%	60 Missing ⚠️
sdk/src/context.rs	31.25%	22 Missing ⚠️
c2pa_c_ffi/src/c2pa_stream.rs	61.70%	18 Missing ⚠️
c2pa_c_ffi/src/error.rs	50.00%	15 Missing ⚠️
c2pa_c_ffi/src/cimpl/cimpl_error.rs	75.00%	9 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1783      +/-   ##
==========================================
- Coverage   75.66%   75.48%   -0.19%     
==========================================
  Files         168      170       +2     
  Lines       38893    39242     +349     
==========================================
+ Hits        29428    29620     +192     
- Misses       9465     9622     +157

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

tmathern · 2026-01-27T18:31:58Z

c2pa_c_ffi/src/c_api.rs

 #[no_mangle]
 pub unsafe extern "C" fn c2pa_error() -> *mut c_char {
-    to_c_string(Error::last_message().unwrap_or_default())
+    to_c_string(Error::last_message())


No default anymore?

c2pa_c_ffi/src/c_api.rs

tmathern · 2026-01-27T18:35:28Z

c2pa_c_ffi/src/c_api.rs

+// TODO: Reader::from_context takes ownership of Context, but Context doesn't implement Clone
+// and we can't take ownership from a *const pointer. Need to decide on the correct approach:
+// 1. Change signature to *mut and actually take ownership from C
+// 2. Use from_shared_context instead


Isn't that what would make the most sense? Since I assume context may be passed around potentially in this layer?

tmathern · 2026-01-27T18:48:11Z

c2pa_c_ffi/src/error.rs

 #[derive(Error, Debug)]
 /// Defines all possible errors that can occur in this library
-pub enum Error {
+pub enum C2paError {


Type name change for namespacing?

Pretty much, neither of these are directly exported, but this helps keep us from clashing with other Error instances.

tmathern · 2026-01-27T19:18:48Z

c2pa_c_ffi/src/c_api.rs

+///
+/// 0 on success, negative value on error.
+#[no_mangle]
+pub unsafe extern "C" fn c2pa_settings_set_value(


That is nice, but agreed a settings builder pattern would be nicer! (Albeit the params & ref handling can be tricky, as I am discovering too).

…le making later access immutable. New C FFI Functions (in c2pa_c_ffi/src/c_api.rs): Context Builder Pattern: c2pa_context_builder_new() - Creates a new mutable context builder c2pa_context_builder_set_settings() - Configures the builder with settings c2pa_context_builder_build() - Builds an immutable Arc<Context> from the builder c2pa_context_new() - Convenience function for creating a default immutable context Reader Functions: c2pa_reader_from_context() - Creates a Reader from a shared context (context can be reused) c2pa_reader_with_stream() - Configures an existing reader with a stream (consumes the reader) Supporting Infrastructure: c2pa_free() - Universal free function for all tracked pointers (was already added earlier) c2pa_settings_new() - Creates a new settings object c2pa_settings_update_from_string() - Updates settings from JSON/TOML c2pa_settings_set_value() - Sets individual config values using dot notation Type Aliases: C2paContext = Arc<Context> - Immutable, shareable context C2paContextBuilder = Context - Mutable builder for construction

tmathern · 2026-01-28T22:38:09Z

c2pa_c_ffi/src/c_api.rs

-/// Any thumbnails or other binary resources will be written to data_dir if provided.
+/// # Safety
+///
+/// This function is safe to call. The returned pointer must be freed


Suggested change

/// This function is safe to call. The returned pointer must be freed

/// The returned pointer must be freed

c2pa_c_ffi/src/cimpl/macros.rs

tmathern · 2026-01-28T22:42:14Z

c2pa_c_ffi/src/cimpl/macros.rs

+
+// Internal routine to convert a *const c_char to a rust String or return a -1 int error.
+#[macro_export]
+macro_rules! cstr_or_return_int {


Same as cstr_or_return_neg?

c2pa_c_ffi/src/cimpl/macros.rs

scouten-adobe

Consider this a "request changes" status, but not using that status in case I'm not available to re-review.

scouten-adobe · 2026-01-29T23:45:21Z

c2pa_c_ffi/src/cimpl/macros.rs

+
+// Unused - kept for potential future use
+#[allow(unused_macros)]
+macro_rules! some_or_return_other_null {


Naming request: some_or_return_other makes sense when you're providing the "other" value.

For macros such as this where the macro name specifies the None case, can I suggest naming as some_or_return_null (omitting the other part)?

Applies to several macros in this file.

scouten-adobe · 2026-01-29T23:47:26Z

c2pa_c_ffi/src/cimpl/utils.rs

+    /// Track a pointer with its type and cleanup function
+    fn track(&self, ptr: usize, type_id: TypeId, cleanup: CleanupFn) {
+        if ptr != 0 {
+            self.tracked.lock().unwrap().insert(ptr, (type_id, cleanup));


Please add a comment here explaining why the unwrap() should be considered safe.

(Or add a panic handler? TO turn panics into errors for that FFI layer?)

scouten-adobe · 2026-01-29T23:54:46Z

c2pa_c_ffi/src/cimpl/utils.rs

+    fn drop(&mut self) {
+        let tracked = self.tracked.lock().unwrap();
+        if !tracked.is_empty() {
+            eprintln!(


tmathern

(Please disregard thread-safety related comment if thread safety is not a topic here).

tmathern · 2026-01-30T06:34:04Z

c2pa_c_ffi/src/cimpl/macros.rs

+            // Caller must ensure ptr is valid for reading and points to a
+            // null-terminated string within MAX_CSTRING_LEN bytes.
+            let bytes = unsafe {
+                std::slice::from_raw_parts(ptr as *const u8, $crate::macros::MAX_CSTRING_LEN)


Isn't that always exactly the size MAX_CSTRING_LEN, even if our string is less, like if the expected size let's say is MAX_CSTRING_LEN/2? Then aren't we over-allocating? Is it fine or undefined behavior then?

tmathern · 2026-01-30T06:39:30Z

c2pa_c_ffi/src/cimpl/utils.rs

+        self.validate(ptr as usize, TypeId::of::<T>())?;
+
+        // Temporarily swap out the value (no clone needed!)
+        let value = std::ptr::replace(ptr, T::default());


Do we want thread safety? We may need a guard/lock here so another thread couldn't be in this code with a pointer in parallel

tmathern · 2026-01-30T06:40:14Z

c2pa_c_ffi/src/cimpl/utils.rs

+        let mutex = &*ptr;
+
+        // Lock and swap - all atomic under the lock
+        let mut guard = mutex.lock().unwrap();


Poisoned mutex would panic, should we return an error instead?

tmathern · 2026-01-30T06:43:20Z

c2pa_c_ffi/src/cimpl/utils.rs

+/// ```ignore
+/// let ptr = track_box(Box::into_raw(Box::new(value)));
+/// ```
+pub fn track_box<T: 'static>(ptr: *mut T) -> *mut T {


Suggested change

pub fn track_box<T: 'static>(ptr: *mut T) -> *mut T {

pub fn track_box<T: 'static + Send>(ptr: *mut T) -> *mut T {

Do we want thread safety?
This captures the usize, which I think is Send. If T is !Send, because the closure using usize is Send, we could end up still running it. So we could end up freeing a !Send on another thread. Are we even worried about this case?

tmathern · 2026-01-30T06:44:52Z

c2pa_c_ffi/src/cimpl/utils.rs

+/// This function performs pointer arithmetic with `ptr.add(size)` which requires
+/// that the pointer and size are valid for the memory region being checked.
+pub unsafe fn is_safe_buffer_size(size: usize, ptr: *const c_uchar) -> bool {
+    // Combined checks for early return - improves branch prediction


Suggested change

// Combined checks for early return - improves branch prediction

// Combined checks for early return - improves branch prediction

// 0-length buffer is considered unsafe, since used by C FFI layer

tmathern · 2026-01-30T06:46:21Z

c2pa_c_ffi/src/cimpl/utils.rs

+/// The returned pointer must be freed exactly once by calling `free_c_bytes`
+pub fn to_c_bytes(bytes: Vec<u8>) -> *const c_uchar {
+    let len = bytes.len();
+    let ptr = Box::into_raw(bytes.into_boxed_slice()) as *const c_uchar;


In theory, bytes could be 0 sized. Since we deal with the C level here, this may create dangling pointers then (0-sized allocation). In another place we said o-sized things are not allowed, we should stay consistent.

tmathern · 2026-01-30T06:46:57Z

c2pa_c_ffi/src/cimpl/utils.rs

+    /// Track a pointer with its type and cleanup function
+    fn track(&self, ptr: usize, type_id: TypeId, cleanup: CleanupFn) {
+        if ptr != 0 {
+            self.tracked.lock().unwrap().insert(ptr, (type_id, cleanup));


(Or add a panic handler? TO turn panics into errors for that FFI layer?)

tmathern

(Can we leak in errors paths, since we expect caller error handling/invalidation? That is a recurring question for FFI layers... how to handle the memory ine rror paths, since it has technically all become invalid... due to the error...).

tmathern · 2026-01-30T06:50:28Z

c2pa_c_ffi/src/c_api.rs

-    check_or_return_int!(dest);
-    check_or_return_int!(signer_ptr);
-    check_or_return_int!(manifest_bytes_ptr);
+    ptr_or_return_int!(builder_ptr);


Do we need to check if these dereferenced pointers are some we track, or we allow any?

tmathern · 2026-01-30T06:51:22Z

c2pa_c_ffi/src/c_api.rs

    tsa_url: *const c_char,
 ) -> *mut C2paSigner {
-    let certs = from_cstr_or_return_null!(certs);
+    let certs = cstr_or_return_null!(certs);


Isn't tracking missing here somewhere? I'd expect tracking starts here for a signer pointer, possibly?

tmathern · 2026-01-30T06:52:40Z

c2pa_c_ffi/src/c_api.rs

+) -> *mut C2paBuilder {
+    // Take ownership of the builder (consumes it)
+    let builder = Box::from_raw(builder);
+    let manifest_json = cstr_or_return_null!(manifest_json);


I think if we return/error early before the box_tracked call, we leak the builder pointer

gpeacock added 4 commits January 23, 2026 17:35

updates to use cimpl macro system and adds Context and Settings suppo…

9a7002e

…rt (still WIP)

Add tracking for c2pa_stream, check last_error from read/write calls …

51ba3a4

…and update stream testing code.

Merge branch 'main' into gpeacock/context_ffi

6871923

gpeacock marked this pull request as draft January 27, 2026 18:23

tmathern reviewed Jan 27, 2026

View reviewed changes

gpeacock added 3 commits January 27, 2026 12:17

add more reader and builder with apis

0e79bf5

Merge branch 'main' into gpeacock/context_ffi

d28e863

tmathern reviewed Jan 28, 2026

View reviewed changes

scouten-adobe reviewed Jan 30, 2026

View reviewed changes

tmathern reviewed Jan 30, 2026

View reviewed changes

	/// This function is safe to call. The returned pointer must be freed
	/// The returned pointer must be freed

	pub fn track_box<T: 'static>(ptr: mut T) -> mut T {
	pub fn track_box<T: 'static + Send>(ptr: mut T) -> mut T {

	// Combined checks for early return - improves branch prediction
	// Combined checks for early return - improves branch prediction
	// 0-length buffer is considered unsafe, since used by C FFI layer

feat: Adds thread safe Settings and Context support to c_ffi_api #1783

Are you sure you want to change the base?

feat: Adds thread safe Settings and Context support to c_ffi_api #1783

Uh oh!

Conversation

gpeacock commented Jan 27, 2026

Uh oh!

codspeed-hq bot commented Jan 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging this PR will not alter performance

Footnotes

Uh oh!

codecov bot commented Jan 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmathern Jan 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

scouten-adobe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmathern left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmathern left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

codspeed-hq bot commented Jan 27, 2026 •

edited

Loading

codecov bot commented Jan 27, 2026 •

edited

Loading

tmathern Jan 27, 2026 •

edited

Loading