diff --git a/CMakeLists.txt b/CMakeLists.txt index 6132bb9..2b8d22d 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -14,7 +14,7 @@ cmake_minimum_required(VERSION 3.27) # This is the current version of this C++ project -project(c2pa-c VERSION 0.23.2) +project(c2pa-c VERSION 0.23.3) # Set the version of the c2pa_rs library used set(C2PA_VERSION "0.80.0") diff --git a/docs/context-settings.md b/docs/context-settings.md index bf6e767..a10fb2c 100644 --- a/docs/context-settings.md +++ b/docs/context-settings.md @@ -165,6 +165,7 @@ auto context = c2pa::Context::ContextBuilder() | `with_signer(signer)` | Store a `Signer` in the context (consumed; used by `Builder::sign` with no explicit signer) | | `with_progress_callback(callback)` | Register a progress/cancel callback (see [Progress callbacks and cancellation](#progress-callbacks-and-cancellation)) | | `create_context()` | Build and return the `Context` (consumes the builder) | +| `release()` | Release the raw `C2paContextBuilder*` together with its progress-callback heap owner (see also [Releasing to a custom IContextProvider](#releasing-to-a-custom-icontextprovider)) | ## Progress callbacks and cancellation @@ -205,6 +206,9 @@ bool callback(c2pa::ProgressPhase phase, uint32_t step, uint32_t total); **Do not throw** from the progress callback. Exceptions cannot cross the C/Rust boundary safely; if your callback throws, the wrapper catches it and the operation is aborted as a cancellation (you do not get your exception back at the call site). Use `return false`, `context.cancel()`, or application-side state instead. +> [!IMPORTANT] +> A callback passed to `with_progress_callback` is copied onto the heap and its address is handed to the underlying library. The built-in `Context` owns that heap block and frees it only after the native context is freed. The free order is correct by construction when the callback flows through `create_context()` into a `shared_ptr`. When the native handle is transferred out of the builder via `release()`, the same contract must be maintained by the caller. See [Releasing to a custom IContextProvider](#releasing-to-a-custom-icontextprovider). + ### Cancelling from another thread You may call `Context::cancel()` from another thread while the same `Context` remains valid and is not being destroyed or moved concurrently with that call. The SDK returns a `C2paException` with an `OperationCancelled` error at the next progress checkpoint: @@ -978,6 +982,20 @@ The `shared_ptr` overloads accept any `shared_ptr`, so custom External libraries can implement `IContextProvider` to supply their own context objects. The interface requires a valid `C2paContext*` pointer and an `is_valid()` check. Wrap your implementation in a `shared_ptr` when passing to `Reader` or `Builder`. +> [!IMPORTANT] +> **Progress-callback lifetime in custom providers.** If the native `C2paContext*` was built with a progress callback, the underlying library stores a raw pointer into a heap-owned `ProgressCallbackFunc`. A provider implementation must keep that callback alive for at least as long as the native context. When destroying the provider, the `C2paContext*` must be freed first and the callback storage released after. + +#### Releasing to a custom IContextProvider + +`Context::ContextBuilder::release()` hands off both the native `C2paContextBuilder*` and the heap-owned progress callback (if any) to the caller. It is the entry point when application code needs to build the native context directly or adopt it into a custom `IContextProvider` implementation. + +`ReleasedBuilder` exposes two fields: + +- `builder`: the raw `C2paContextBuilder*`. It must be passed to `c2pa_context_builder_build` exactly once. +- `callback_owner`: `std::unique_ptr`; `nullptr` when no progress callback was set. When non-null, it must remain alive at least as long as the native context built from `builder`. If it is destroyed earlier, the library will dereference freed memory on the next progress tick. + +When the built-in `Context` is sufficient, `create_context()` is preferred. It performs the same handoff internally and keeps the native handle and callback owner bound together. + ### Builder::sign overloads `Builder::sign` has two kinds of overloads: those that take an explicit `Signer` argument, and those that use the signer stored in the `Builder`'s `Context`. diff --git a/include/c2pa.hpp b/include/c2pa.hpp index 46767a9..0a0d5a5 100644 --- a/include/c2pa.hpp +++ b/include/c2pa.hpp @@ -115,9 +115,19 @@ namespace c2pa /// @brief Interface for types that can provide C2PA context functionality. /// @details This interface can be implemented by external libraries to provide /// custom context implementations (e.g. AdobeContext wrappers). - /// Reader and Builder take the context by reference and use it only at - /// construction; the underlying implementation copies context state - /// into the reader/builder, so the context does not need to outlive them. + /// Reader and Builder accept a shared_ptr and extend + /// the provider's lifetime for as long as the Reader/Builder exists. + /// + /// @par Progress callback lifetime (post-0322d67) + /// If the native C2paContext* held by the provider was configured with a progress + /// callback, the provider MUST keep the heap-owned ProgressCallbackFunc alive at + /// least as long as the native context. The native side stores only a raw pointer + /// into that heap block and will call it until the context is freed. Destroy + /// order inside the provider: free the native C2paContext* first (stops further + /// callback invocations), then release the callback storage. The built-in + /// Context class enforces this via member declaration order; external providers + /// that adopt a native context built via ContextBuilder::release() must do the + /// same. /// /// @par Move semantics /// Move construction and move assignment are defaulted. After move, the moved-from @@ -404,16 +414,41 @@ namespace c2pa /// @note This consumes the builder. After calling this, is_valid() returns false. [[nodiscard]] Context create_context(); - /// @brief Release ownership of the underlying C2paContextBuilder pointer. - /// After this call, the ContextBuilder no longer owns the pointer - /// and is_valid() returns false. The caller is responsible for managing - /// the lifetime of the returned pointer. - /// @return Pointer to the C2paContextBuilder object, or nullptr if moved from. - C2paContextBuilder* release() noexcept; + /// @brief Result of ContextBuilder::release(): the raw native builder handle + /// paired with the heap-owned progress callback (if any). + /// @details The caller takes joint ownership. If `callback_owner` is non-null, + /// the native side stores a raw pointer into it and will invoke the + /// callback until the native context built from `builder` is freed. + /// Therefore `callback_owner` must remain alive at least until the + /// native context returned by c2pa_context_builder_build(builder) is + /// freed via c2pa_free(). Release order: native context first, then + /// `callback_owner` goes out of scope. + struct ReleasedBuilder { + C2paContextBuilder* builder; + std::unique_ptr callback_owner; + }; + + /// @brief Release ownership of the underlying C2paContextBuilder handle and + /// its progress-callback heap block (if any) to the caller. + /// @details After this call is_valid() returns false. The previous + /// C2paContextBuilder* overload was unsound when a progress callback + /// was pending: the C++ builder destructor would free the heap + /// ProgressCallbackFunc while the native side still held its pointer, + /// yielding a use-after-free on the first progress tick. + /// Callers must keep ReleasedBuilder::callback_owner alive for as + /// long as the native context built from ReleasedBuilder::builder + /// exists. + /// @return ReleasedBuilder holding both the native builder and (optionally) + /// the owning unique_ptr for the progress callback. Fields are null + /// when moved-from. + [[nodiscard]] ReleasedBuilder release() noexcept; private: - C2paContextBuilder* context_builder; + // pending_callback_ is declared before context_builder + // so destruction (reverse of declaration) frees context_builder first, while + // the callback heap block is still live, then releases pending_callback_. std::unique_ptr pending_callback_; + C2paContextBuilder* context_builder; }; // Direct construction @@ -464,6 +499,13 @@ namespace c2pa /// @throws C2paException if ctx is nullptr. explicit Context(C2paContext* ctx); + /// @brief "Adopt" a native context together with its heap-owned progress callback. + /// @param ctx Native context pointer. Context takes ownership. + /// @param callback Heap-owned progress callback whose raw pointer the native + /// context may hold. May be null. + Context(C2paContext* ctx, + std::unique_ptr callback) noexcept; + /// @brief Request cancellation of any in-progress operation on this context. /// /// @details Safe to call from another thread while this Context remains valid @@ -476,11 +518,15 @@ namespace c2pa void cancel() noexcept; private: + // Member order matters: `context` is declared before `callback_owner_` so + // destruction (reverse of declaration) frees the heap callback block after + // the native context has been freed. C2paContext* context; - /// Heap-owned ProgressCallbackFunc; non-null only when set via - /// ContextBuilder::with_progress_callback(). Deleted in the destructor. - void* callback_owner_ = nullptr; + /// Heap-owned ProgressCallbackFunc, non-null only when the Context was built + /// with a progress callback via ContextBuilder::with_progress_callback(). + /// Destroyed after `context`. + std::unique_ptr callback_owner_; }; /// @brief Get the version of the C2PA library. diff --git a/src/c2pa_context.cpp b/src/c2pa_context.cpp index 616bae9..bcecb7c 100644 --- a/src/c2pa_context.cpp +++ b/src/c2pa_context.cpp @@ -40,22 +40,32 @@ namespace c2pa Context::Context(Context&& other) noexcept : context(std::exchange(other.context, nullptr)), - callback_owner_(std::exchange(other.callback_owner_, nullptr)) { + callback_owner_(std::move(other.callback_owner_)) { } Context& Context::operator=(Context&& other) noexcept { if (this != &other) { - c2pa_free(context); - delete static_cast(callback_owner_); + // Free the native context first so it stops invoking the callback, + // then release the heap block the callback lived in (move). + if (context) { + c2pa_free(context); + } context = std::exchange(other.context, nullptr); - callback_owner_ = std::exchange(other.callback_owner_, nullptr); + callback_owner_ = std::move(other.callback_owner_); } return *this; } Context::~Context() noexcept { - c2pa_free(context); - delete static_cast(callback_owner_); + // Free native context first, then let callback_owner_ destruct. + if (context) { + c2pa_free(context); + } + } + + Context::Context(C2paContext* ctx, + std::unique_ptr callback) noexcept + : context(ctx), callback_owner_(std::move(callback)) { } void Context::cancel() noexcept { @@ -91,13 +101,18 @@ namespace c2pa } Context::ContextBuilder::ContextBuilder(ContextBuilder&& other) noexcept - : context_builder(std::exchange(other.context_builder, nullptr)), - pending_callback_(std::move(other.pending_callback_)) { + : pending_callback_(std::move(other.pending_callback_)), + context_builder(std::exchange(other.context_builder, nullptr)) { } Context::ContextBuilder& Context::ContextBuilder::operator=(ContextBuilder&& other) noexcept { if (this != &other) { - c2pa_free(context_builder); + // Free the native builder before moving in the new pending_callback_ so + // the old native builder cannot touch its (about-to-be-replaced) callback + // block after we've dropped the old one. + if (context_builder) { + c2pa_free(context_builder); + } context_builder = std::exchange(other.context_builder, nullptr); pending_callback_ = std::move(other.pending_callback_); } @@ -105,7 +120,12 @@ namespace c2pa } Context::ContextBuilder::~ContextBuilder() noexcept { - c2pa_free(context_builder); + // Member destruction order is reverse of declaration: + // context_builder freed first (stops FFI from calling the callback), + // then pending_callback_ is released. + if (context_builder) { + c2pa_free(context_builder); + } } bool Context::ContextBuilder::is_valid() const noexcept { @@ -225,21 +245,27 @@ namespace c2pa if (!is_valid()) { throw C2paException("ContextBuilder is invalid (moved from)"); } - // Heap-allocate the std::function so we can pass a stable pointer to the C API. - // The resulting Context takes ownership of this allocation. - pending_callback_ = std::make_unique(std::move(callback)); + + // Two-stage swap: create the new heap block, register it with the FFI, and + // only after drop any previous pending_callback_. + auto fresh = std::make_unique(std::move(callback)); if (c2pa_context_builder_set_progress_callback( context_builder, - pending_callback_.get(), + fresh.get(), progress_callback_trampoline) != 0) { - pending_callback_.reset(); throw C2paException(); } + pending_callback_ = std::move(fresh); return *this; } - C2paContextBuilder* Context::ContextBuilder::release() noexcept { - return std::exchange(context_builder, nullptr); + Context::ContextBuilder::ReleasedBuilder Context::ContextBuilder::release() noexcept { + // Transfer both the native builder handle and the callback heap block to + // the caller atomically. + return ReleasedBuilder{ + std::exchange(context_builder, nullptr), + std::move(pending_callback_), + }; } Context Context::ContextBuilder::create_context() { @@ -247,19 +273,13 @@ namespace c2pa throw C2paException("ContextBuilder is invalid (moved from)"); } - // The C API consumes the context builder on build + // The C API consumes the context builder on build (success or failure). C2paContext* ctx = c2pa_context_builder_build(context_builder); + context_builder = nullptr; if (!ctx) { throw C2paException("Failed to build context"); } - // Builder is consumed by the C API - context_builder = nullptr; - - Context result(ctx); - // Transfer progress callback heap ownership to the Context so it is freed - // when the Context is destroyed (the C side holds a raw pointer to it). - result.callback_owner_ = pending_callback_.release(); - return result; + return Context(ctx, std::move(pending_callback_)); } } // namespace c2pa diff --git a/tests/context.test.cpp b/tests/context.test.cpp index 8a943bf..6f0d226 100644 --- a/tests/context.test.cpp +++ b/tests/context.test.cpp @@ -758,3 +758,119 @@ TEST_F(ContextTest, SharedPtrContext_FromAsset) { EXPECT_TRUE(reader.has_value()); EXPECT_EQ(ctx.use_count(), 2); // ctx + reader } + +// ContextBuilder::release() must carry the progress-callback owner +// alongside the raw native builder handle. +TEST_F(ContextTest, ContextBuilderReleaseCarriesCallbackOwner) { + std::atomic call_count{0}; + + // Minimal external IContextProvider that adopts the ReleasedBuilder + // pieces and preserves the required destruction order. + class AdoptingProvider : public c2pa::IContextProvider { + public: + AdoptingProvider(c2pa::Context::ContextBuilder::ReleasedBuilder rb) { + if (!rb.builder) { + throw c2pa::C2paException("AdoptingProvider: null builder"); + } + ctx_ = c2pa_context_builder_build(rb.builder); + if (!ctx_) { + throw c2pa::C2paException("AdoptingProvider: build failed"); + } + callback_owner_ = std::move(rb.callback_owner); + } + ~AdoptingProvider() noexcept override { + if (ctx_) { + c2pa_free(ctx_); + } + } + AdoptingProvider(const AdoptingProvider&) = delete; + AdoptingProvider& operator=(const AdoptingProvider&) = delete; + + C2paContext* c_context() const noexcept override { return ctx_; } + bool is_valid() const noexcept override { return ctx_ != nullptr; } + + private: + C2paContext* ctx_ = nullptr; + std::unique_ptr callback_owner_; + }; + + auto rb = c2pa::Context::ContextBuilder() + .with_progress_callback([&](c2pa::ProgressPhase, uint32_t, uint32_t) { + ++call_count; + return true; + }) + .release(); + ASSERT_NE(rb.builder, nullptr); + ASSERT_NE(rb.callback_owner, nullptr); + + auto provider = std::make_shared(std::move(rb)); + ASSERT_TRUE(provider->is_valid()); + + EXPECT_NO_THROW(sign_with_progress_context(provider, get_temp_path("release_adopting.jpg"))); + EXPECT_GT(call_count.load(), 0); +} + +// A ContextBuilder that has a progress callback set but is never +// turned into a Context must destruct cleanly. +TEST_F(ContextTest, ContextBuilderDroppedAfterCallbackNoUAF) { + std::atomic call_count{0}; + { + c2pa::Context::ContextBuilder b; + b.with_progress_callback([&](c2pa::ProgressPhase, uint32_t, uint32_t) { + ++call_count; + return true; + }); + // Intentionally no create_context(), let b destruct here. + } + + SUCCEED(); +} + +// Ccreate_context() must transfer callback ownership to the new +// Context atomically. +TEST_F(ContextTest, ContextBuilderCreateContextAtomicHandoff) { + std::atomic call_count{0}; + + auto context = std::make_shared(c2pa::Context::ContextBuilder() + .with_progress_callback([&](c2pa::ProgressPhase, uint32_t, uint32_t) { + ++call_count; + return true; + }) + .create_context()); + + ASSERT_TRUE(context->is_valid()); + EXPECT_NO_THROW(sign_with_progress_context(context, get_temp_path("atomic_handoff.jpg"))); + EXPECT_GT(call_count.load(), 0); +} + +// Context move-assignment must free the destination's native context +// before releasing its old callback storage, and must leave the survivor +// with a valid callback bound to its new context. +TEST_F(ContextTest, ContextMoveAssignWithCallbackPreservesInvocation) { + std::atomic lhs{0}; + std::atomic rhs{0}; + + c2pa::Context a = c2pa::Context::ContextBuilder() + .with_progress_callback([&](c2pa::ProgressPhase, uint32_t, uint32_t) { + ++lhs; + return true; + }) + .create_context(); + + c2pa::Context b = c2pa::Context::ContextBuilder() + .with_progress_callback([&](c2pa::ProgressPhase, uint32_t, uint32_t) { + ++rhs; + return true; + }) + .create_context(); + + a = std::move(b); + EXPECT_FALSE(b.is_valid()); + ASSERT_TRUE(a.is_valid()); + + auto sp = std::make_shared(std::move(a)); + EXPECT_NO_THROW(sign_with_progress_context(sp, get_temp_path("move_assign_survivor.jpg"))); + + EXPECT_EQ(lhs.load(), 0); + EXPECT_GT(rhs.load(), 0); +}