diff --git a/src/c2pa_builder.cpp b/src/c2pa_builder.cpp index 8c02213..43a18ae 100644 --- a/src/c2pa_builder.cpp +++ b/src/c2pa_builder.cpp @@ -51,12 +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. + // 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()); - builder = nullptr; if (updated == nullptr) { - throw C2paException(); + throw detail::error_from_failed_call(builder); } builder = updated; } @@ -116,10 +116,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 +153,8 @@ 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); throw C2paException("Failed to set builder definition"); } builder = updated; @@ -344,11 +345,10 @@ 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(); + throw detail::error_from_failed_call(builder); } builder = updated; return *this; diff --git a/src/c2pa_context.cpp b/src/c2pa_context.cpp index c763520..cf2d9ce 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,12 @@ 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); + // 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..79bfeb5 100644 --- a/src/c2pa_internal.hpp +++ b/src/c2pa_internal.hpp @@ -39,6 +39,51 @@ inline bool error_indicates_manifest_not_found(const char* message) noexcept { return message != nullptr && std::strstr(message, "ManifestNotFound") != nullptr; } +/// @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; + if (is_pre_consume_error(error.what())) { + c2pa_free(handle); + } + handle = nullptr; + 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..374e5e6 100644 --- a/src/c2pa_reader.cpp +++ b/src/c2pa_reader.cpp @@ -58,11 +58,10 @@ 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(); + throw detail::error_from_failed_call(c2pa_reader); } c2pa_reader = updated; } @@ -94,11 +93,10 @@ 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(); + throw detail::error_from_failed_call(c2pa_reader); } c2pa_reader = updated; } @@ -126,16 +124,15 @@ 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(); + throw detail::error_from_failed_call(c2pa_reader); } c2pa_reader = updated; @@ -209,9 +206,8 @@ namespace c2pa resolved.c_str(), main_wrapper.c_stream, fragment_wrapper.c_stream); - c2pa_reader = nullptr; if (updated == nullptr) { - throw C2paException(); + throw detail::error_from_failed_call(c2pa_reader); } c2pa_reader = updated; diff --git a/tests/builder.test.cpp b/tests/builder.test.cpp index 8db2bb1..14a3f78 100644 --- a/tests/builder.test.cpp +++ b/tests/builder.test.cpp @@ -7597,6 +7597,37 @@ TEST_F(BuilderTest, SignTrimsPaddedFormat) { EXPECT_NO_THROW({ builder.sign(" \t image/jpeg \n ", source, dest, signer); }); } +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();