From 4150af81af371fade6448214fad51df808533d9b Mon Sep 17 00:00:00 2001 From: tmathern <60901087+tmathern@users.noreply.github.com> Date: Mon, 27 Jul 2026 21:14:01 -0700 Subject: [PATCH 1/2] fix: Add tests for error handling --- src/c2pa_builder.cpp | 22 ++++++++++++--------- src/c2pa_context.cpp | 12 ++++++----- src/c2pa_internal.hpp | 16 +++++++++++++++ src/c2pa_reader.cpp | 26 +++++++++++++----------- tests/builder.test.cpp | 45 ++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 96 insertions(+), 25 deletions(-) diff --git a/src/c2pa_builder.cpp b/src/c2pa_builder.cpp index 8c02213..9fc02e1 100644 --- a/src/c2pa_builder.cpp +++ b/src/c2pa_builder.cpp @@ -52,11 +52,12 @@ namespace c2pa // Apply the manifest definition to the Builder. // Note: c2pa_builder_with_definition consumes the builder pointer on success - // and on operation failure. + // and on operation failure, but not when argument validation rejects the call. C2paBuilder* updated = c2pa_builder_with_definition(builder, manifest_json.c_str()); - builder = nullptr; if (updated == nullptr) { - throw C2paException(); + auto error = detail::error_from_failed_call(builder); + builder = nullptr; + throw error; } builder = updated; } @@ -116,10 +117,11 @@ namespace c2pa throw C2paException(); } + // base is a local, so no destructor reclaims it if this throws: a + // constructor that throws never gets ~Builder(). Free it here on failure. builder = c2pa_builder_with_archive(base, c_archive.c_stream); - base = nullptr; if (builder == nullptr) { - throw C2paException(); + throw detail::error_from_failed_call(base); } } @@ -152,8 +154,9 @@ namespace c2pa // c2pa_builder_with_definition consumes the builder pointer, // so the original pointer is invalid after the call. C2paBuilder* updated = c2pa_builder_with_definition(builder, manifest_json.c_str()); - builder = nullptr; if (updated == nullptr) { + (void)detail::error_from_failed_call(builder); + builder = nullptr; throw C2paException("Failed to set builder definition"); } builder = updated; @@ -344,11 +347,12 @@ namespace c2pa { CppIStream c_archive(archive); - // c2pa_builder_with_archive consumes the builder pointer and returns a new one + // c2pa_builder_with_archive consumes the builder pointer, and returns a new one. C2paBuilder* updated = c2pa_builder_with_archive(builder, c_archive.c_stream); - builder = nullptr; if (updated == nullptr) { - throw C2paException(); + auto error = detail::error_from_failed_call(builder); + builder = nullptr; + throw error; } builder = updated; return *this; diff --git a/src/c2pa_context.cpp b/src/c2pa_context.cpp index c763520..cf7e345 100644 --- a/src/c2pa_context.cpp +++ b/src/c2pa_context.cpp @@ -183,9 +183,8 @@ namespace c2pa if (!raw) { throw C2paException("Signer is not valid"); } - if (c2pa_context_builder_set_signer(context_builder, raw) != 0) { - throw C2paException(); + throw detail::error_from_failed_call(raw); } return *this; } @@ -203,7 +202,7 @@ namespace c2pa throw C2paException("Failed to create HTTP resolver"); } if (c2pa_context_builder_set_http_resolver(context_builder, resolver) != 0) { - throw C2paException(); + throw detail::error_from_failed_call(resolver); } return *this; } @@ -275,10 +274,13 @@ namespace c2pa throw C2paException("ContextBuilder is invalid (moved from)"); } - // The C API consumes the context builder on build (success or failure). + // The C API consumes the context builder on build. C2paContext* ctx = c2pa_context_builder_build(context_builder); - context_builder = nullptr; if (!ctx) { + (void)detail::error_from_failed_call(context_builder); + context_builder = nullptr; + // The native builder is gone, so nothing can invoke the callback now. + pending_callback_.reset(); throw C2paException("Failed to build context"); } diff --git a/src/c2pa_internal.hpp b/src/c2pa_internal.hpp index 35dc18f..d2ab6db 100644 --- a/src/c2pa_internal.hpp +++ b/src/c2pa_internal.hpp @@ -39,6 +39,22 @@ inline bool error_indicates_manifest_not_found(const char* message) noexcept { return message != nullptr && std::strstr(message, "ManifestNotFound") != nullptr; } +/// @brief Finish with a handle whose consuming C API call failed: free it if the +/// library left it to us, and return the error to throw. +/// @param handle The handle passed to the failed call. Invalid once this returns, +/// whichever side ended up freeing it, so the caller must drop its copy. +/// @return The exception to throw, carrying the library's error message. Meant to be +/// used directly: `throw detail::error_from_failed_call(handle);`. +/// @details The exception is constructed before the free because a failing +/// c2pa_free() overwrites the thread-local last error, +/// which would replace the real failure message. +template +[[nodiscard]] inline C2paException error_from_failed_call(Handle* handle) { + C2paException error; // Capture the real error first. + c2pa_free(handle); // Free only if still tracked, otherwise a no-op. + return error; +} + /// @brief Converts a C array of C strings to a std::vector of std::string. /// @param mime_types Pointer to an array of C strings (const char*). /// @param count Number of elements in the array. diff --git a/src/c2pa_reader.cpp b/src/c2pa_reader.cpp index 7e9ef2f..bf1577c 100644 --- a/src/c2pa_reader.cpp +++ b/src/c2pa_reader.cpp @@ -58,11 +58,12 @@ namespace c2pa } // Update reader with stream. - // Note: c2pa_reader_with_stream consumes the reader pointer. + // Note: c2pa_reader_with_stream consumes the reader pointer on success. C2paReader* updated = c2pa_reader_with_stream(c2pa_reader, resolved.c_str(), cpp_stream->c_stream); - c2pa_reader = nullptr; if (updated == nullptr) { - throw C2paException(); + auto error = detail::error_from_failed_call(c2pa_reader); + c2pa_reader = nullptr; + throw error; } c2pa_reader = updated; } @@ -94,11 +95,12 @@ namespace c2pa throw C2paException("Failed to create reader from context"); } - // Note: c2pa_reader_with_stream consumes the reader pointer. + // Note: c2pa_reader_with_stream consumes the reader pointer on success. C2paReader* updated = c2pa_reader_with_stream(c2pa_reader, extension.c_str(), cpp_stream->c_stream); - c2pa_reader = nullptr; if (updated == nullptr) { - throw C2paException(); + auto error = detail::error_from_failed_call(c2pa_reader); + c2pa_reader = nullptr; + throw error; } c2pa_reader = updated; } @@ -126,16 +128,17 @@ namespace c2pa throw C2paException("Failed to create reader from context"); } - // c2pa_reader_with_manifest_data_and_stream always consumes c2pa_reader. + // c2pa_reader_with_manifest_data_and_stream consumes c2pa_reader on success. C2paReader* updated = c2pa_reader_with_manifest_data_and_stream( c2pa_reader, resolved.c_str(), cpp_stream->c_stream, manifest_jumbf.data(), manifest_jumbf.size()); - c2pa_reader = nullptr; if (updated == nullptr) { - throw C2paException(); + auto error = detail::error_from_failed_call(c2pa_reader); + c2pa_reader = nullptr; + throw error; } c2pa_reader = updated; @@ -209,9 +212,10 @@ namespace c2pa resolved.c_str(), main_wrapper.c_stream, fragment_wrapper.c_stream); - c2pa_reader = nullptr; if (updated == nullptr) { - throw C2paException(); + auto error = detail::error_from_failed_call(c2pa_reader); + c2pa_reader = nullptr; + throw error; } c2pa_reader = updated; diff --git a/tests/builder.test.cpp b/tests/builder.test.cpp index 8db2bb1..800ab93 100644 --- a/tests/builder.test.cpp +++ b/tests/builder.test.cpp @@ -7597,6 +7597,51 @@ TEST_F(BuilderTest, SignTrimsPaddedFormat) { EXPECT_NO_THROW({ builder.sign(" \t image/jpeg \n ", source, dest, signer); }); } +TEST_F(BuilderTest, WithDefinitionReclaimsHandleWhenValidationFails) { + auto builder = make_builder(); + C2paBuilder* original = builder.c2pa_builder(); + ASSERT_NE(original, nullptr); + + // Over MAX_CSTRING_LEN (1MB), so should be rejected. + const std::string oversized = "{\"x\":\"" + std::string(1100000, 'a') + "\"}"; + EXPECT_THROW(builder.with_definition(oversized), c2pa::C2paException); + + // The handle must already be freed, a second free must report "not tracked". + EXPECT_EQ(c2pa_free(original), -1) + << "native builder was still tracked after a failed with_definition: leaked"; +} + +TEST_F(BuilderTest, FailedConsumingCallKeepsTheLibraryErrorMessage) { + // A failed consuming call reclaims the handle, and c2pa_free() on a handle the + // library already took overwrites the thread-local error with its own + // "Other: UntrackedPointer: 0x...". + auto context = std::make_shared(); + + // Provoked failure: the JSON passes argument validation, then fails to parse. + try { + c2pa::Builder builder(context, "{ this is not valid json "); + FAIL() << "expected malformed JSON to throw"; + } catch (const c2pa::C2paException& e) { + const std::string message = e.what(); + EXPECT_NE(message.find("Json:"), std::string::npos) + << "lost the library's parse error, got: " << message; + EXPECT_EQ(message.find("UntrackedPointer"), std::string::npos) + << "error slot was dirtied by the reclaiming free, got: " << message; + } + + // Validation failure: rejected before the library takes the handle, + // so the reclaiming free succeeds and sets no error of its own. + const std::string oversized = "{\"x\":\"" + std::string(1100000, 'a') + "\"}"; + try { + c2pa::Builder builder(context, oversized); + FAIL() << "expected an over-long manifest to throw"; + } catch (const c2pa::C2paException& e) { + const std::string message = e.what(); + EXPECT_NE(message.find("StringTooLong"), std::string::npos) + << "lost the library's length error, got: " << message; + } +} + TEST_F(BuilderTest, SignWithUnsupportedFormatThrows) { auto signer = c2pa_test::create_test_signer(); auto builder = make_builder(); From d39d5cdc42ef21cafacd1e7b762dd3ce77375612 Mon Sep 17 00:00:00 2001 From: tmathern <60901087+tmathern@users.noreply.github.com> Date: Mon, 27 Jul 2026 21:39:24 -0700 Subject: [PATCH 2/2] fix: Memory handling --- src/c2pa_builder.cpp | 14 +++++--------- src/c2pa_context.cpp | 1 - src/c2pa_internal.hpp | 43 +++++++++++++++++++++++++++++++++++------- src/c2pa_reader.cpp | 16 ++++------------ tests/builder.test.cpp | 14 -------------- 5 files changed, 45 insertions(+), 43 deletions(-) diff --git a/src/c2pa_builder.cpp b/src/c2pa_builder.cpp index 9fc02e1..43a18ae 100644 --- a/src/c2pa_builder.cpp +++ b/src/c2pa_builder.cpp @@ -51,13 +51,12 @@ namespace c2pa init_from_context(context); // Apply the manifest definition to the Builder. - // Note: c2pa_builder_with_definition consumes the builder pointer on success - // and on operation failure, but not when argument validation rejects the call. + // Note: c2pa_builder_with_definition consumes the builder pointer on success. + // On failure the handle is only ours again when the library rejected it + // before taking ownership, which error_from_failed_call works out. C2paBuilder* updated = c2pa_builder_with_definition(builder, manifest_json.c_str()); if (updated == nullptr) { - auto error = detail::error_from_failed_call(builder); - builder = nullptr; - throw error; + throw detail::error_from_failed_call(builder); } builder = updated; } @@ -156,7 +155,6 @@ namespace c2pa C2paBuilder* updated = c2pa_builder_with_definition(builder, manifest_json.c_str()); if (updated == nullptr) { (void)detail::error_from_failed_call(builder); - builder = nullptr; throw C2paException("Failed to set builder definition"); } builder = updated; @@ -350,9 +348,7 @@ namespace c2pa // c2pa_builder_with_archive consumes the builder pointer, and returns a new one. C2paBuilder* updated = c2pa_builder_with_archive(builder, c_archive.c_stream); if (updated == nullptr) { - auto error = detail::error_from_failed_call(builder); - builder = nullptr; - throw error; + throw detail::error_from_failed_call(builder); } builder = updated; return *this; diff --git a/src/c2pa_context.cpp b/src/c2pa_context.cpp index cf7e345..cf2d9ce 100644 --- a/src/c2pa_context.cpp +++ b/src/c2pa_context.cpp @@ -278,7 +278,6 @@ namespace c2pa C2paContext* ctx = c2pa_context_builder_build(context_builder); if (!ctx) { (void)detail::error_from_failed_call(context_builder); - context_builder = nullptr; // The native builder is gone, so nothing can invoke the callback now. pending_callback_.reset(); throw C2paException("Failed to build context"); diff --git a/src/c2pa_internal.hpp b/src/c2pa_internal.hpp index d2ab6db..79bfeb5 100644 --- a/src/c2pa_internal.hpp +++ b/src/c2pa_internal.hpp @@ -39,19 +39,48 @@ inline bool error_indicates_manifest_not_found(const char* message) noexcept { return message != nullptr && std::strstr(message, "ManifestNotFound") != nullptr; } -/// @brief Finish with a handle whose consuming C API call failed: free it if the -/// library left it to us, and return the error to throw. -/// @param handle The handle passed to the failed call. Invalid once this returns, -/// whichever side ended up freeing it, so the caller must drop its copy. +/// @brief Error prefixes the library reports when it rejects a handle +/// before taking ownership, leaving the handle ours to free. +/// @details Any other error means the library took the handle and dropped it itself. +/// Both entries say the library never took the handle, so it is still ours +/// and we remain responsible for releasing it. +inline constexpr const char *kPreConsumeErrorTags[] = { + "UntrackedPointer:", // handle not in the registry + "WrongPointerType:", // handle tracked under a different type +}; + +/// @brief True when @p message says the library rejected the handle before +/// taking ownership. +/// @param message The library's error text, or nullptr when it reported none. +/// @return False for nullptr: with no error, assume the handle was consumed. +inline bool is_pre_consume_error(const char *message) noexcept { + if (message == nullptr) { + return false; + } + for (const char *tag : kPreConsumeErrorTags) { + if (std::strstr(message, tag) != nullptr) { + return true; + } + } + return false; +} + +/// @brief Finish with a handle whose consuming C API call failed: release it if +/// the library left it to us, and return the error to throw. +/// @param handle The handle passed to the failed call, set to null before this +/// returns. Invalid either way afterwards, whichever side released it. /// @return The exception to throw, carrying the library's error message. Meant to be /// used directly: `throw detail::error_from_failed_call(handle);`. /// @details The exception is constructed before the free because a failing /// c2pa_free() overwrites the thread-local last error, /// which would replace the real failure message. template -[[nodiscard]] inline C2paException error_from_failed_call(Handle* handle) { - C2paException error; // Capture the real error first. - c2pa_free(handle); // Free only if still tracked, otherwise a no-op. +[[nodiscard]] inline C2paException error_from_failed_call(Handle*& handle) { + C2paException error; + if (is_pre_consume_error(error.what())) { + c2pa_free(handle); + } + handle = nullptr; return error; } diff --git a/src/c2pa_reader.cpp b/src/c2pa_reader.cpp index bf1577c..374e5e6 100644 --- a/src/c2pa_reader.cpp +++ b/src/c2pa_reader.cpp @@ -61,9 +61,7 @@ namespace c2pa // Note: c2pa_reader_with_stream consumes the reader pointer on success. C2paReader* updated = c2pa_reader_with_stream(c2pa_reader, resolved.c_str(), cpp_stream->c_stream); if (updated == nullptr) { - auto error = detail::error_from_failed_call(c2pa_reader); - c2pa_reader = nullptr; - throw error; + throw detail::error_from_failed_call(c2pa_reader); } c2pa_reader = updated; } @@ -98,9 +96,7 @@ namespace c2pa // Note: c2pa_reader_with_stream consumes the reader pointer on success. C2paReader* updated = c2pa_reader_with_stream(c2pa_reader, extension.c_str(), cpp_stream->c_stream); if (updated == nullptr) { - auto error = detail::error_from_failed_call(c2pa_reader); - c2pa_reader = nullptr; - throw error; + throw detail::error_from_failed_call(c2pa_reader); } c2pa_reader = updated; } @@ -136,9 +132,7 @@ namespace c2pa manifest_jumbf.data(), manifest_jumbf.size()); if (updated == nullptr) { - auto error = detail::error_from_failed_call(c2pa_reader); - c2pa_reader = nullptr; - throw error; + throw detail::error_from_failed_call(c2pa_reader); } c2pa_reader = updated; @@ -213,9 +207,7 @@ namespace c2pa main_wrapper.c_stream, fragment_wrapper.c_stream); if (updated == nullptr) { - auto error = detail::error_from_failed_call(c2pa_reader); - c2pa_reader = nullptr; - throw error; + throw detail::error_from_failed_call(c2pa_reader); } c2pa_reader = updated; diff --git a/tests/builder.test.cpp b/tests/builder.test.cpp index 800ab93..14a3f78 100644 --- a/tests/builder.test.cpp +++ b/tests/builder.test.cpp @@ -7597,20 +7597,6 @@ TEST_F(BuilderTest, SignTrimsPaddedFormat) { EXPECT_NO_THROW({ builder.sign(" \t image/jpeg \n ", source, dest, signer); }); } -TEST_F(BuilderTest, WithDefinitionReclaimsHandleWhenValidationFails) { - auto builder = make_builder(); - C2paBuilder* original = builder.c2pa_builder(); - ASSERT_NE(original, nullptr); - - // Over MAX_CSTRING_LEN (1MB), so should be rejected. - const std::string oversized = "{\"x\":\"" + std::string(1100000, 'a') + "\"}"; - EXPECT_THROW(builder.with_definition(oversized), c2pa::C2paException); - - // The handle must already be freed, a second free must report "not tracked". - EXPECT_EQ(c2pa_free(original), -1) - << "native builder was still tracked after a failed with_definition: leaked"; -} - TEST_F(BuilderTest, FailedConsumingCallKeepsTheLibraryErrorMessage) { // A failed consuming call reclaims the handle, and c2pa_free() on a handle the // library already took overwrites the thread-local error with its own