diff --git a/src/c2pa_builder.cpp b/src/c2pa_builder.cpp index 38ac1cb..4148f43 100644 --- a/src/c2pa_builder.cpp +++ b/src/c2pa_builder.cpp @@ -116,11 +116,18 @@ namespace c2pa throw C2paException(); } - builder = c2pa_builder_with_archive(base, c_archive.c_stream); - base = nullptr; - if (builder == nullptr) { - throw C2paException(); + // On success this consumes base, on error base is retained. + // On error it can be retained and we need to free explicitly + // (c2pa_free handles verifying free is possible). + C2paBuilder* result = c2pa_builder_with_archive(base, c_archive.c_stream); + if (result == nullptr) { + C2paException ex; + c2pa_free(base); + base = nullptr; + throw ex; } + builder = result; + base = nullptr; } Builder::~Builder() @@ -332,11 +339,15 @@ namespace c2pa { CppIStream c_archive(archive); - // c2pa_builder_with_archive consumes the builder pointer and returns a new one + // On success this consumes the builder and returns a new one. + // On error it can be retained and we need to free explicitly + // (c2pa_free handles verifying free is possible). C2paBuilder* updated = c2pa_builder_with_archive(builder, c_archive.c_stream); - builder = nullptr; if (updated == nullptr) { - throw C2paException(); + C2paException ex; + c2pa_free(builder); + builder = nullptr; + throw ex; } builder = updated; return *this; diff --git a/src/c2pa_reader.cpp b/src/c2pa_reader.cpp index 0564fc6..e19c324 100644 --- a/src/c2pa_reader.cpp +++ b/src/c2pa_reader.cpp @@ -55,11 +55,15 @@ namespace c2pa } // Update reader with stream. - // Note: c2pa_reader_with_stream consumes the reader pointer. + // On success c2pa_reader_with_stream consumes the reader. + // On error it can be retained and we need to free explicitly + // (c2pa_free handles verifying free is possible). C2paReader* updated = c2pa_reader_with_stream(c2pa_reader, format.c_str(), cpp_stream->c_stream); - c2pa_reader = nullptr; if (updated == nullptr) { - throw C2paException(); + C2paException ex; // owns the error message before we free + c2pa_free(c2pa_reader); + c2pa_reader = nullptr; + throw ex; } c2pa_reader = updated; } @@ -89,11 +93,15 @@ namespace c2pa throw C2paException("Failed to create reader from context"); } - // Note: c2pa_reader_with_stream consumes the reader pointer. + // On success c2pa_reader_with_stream consumes the reader. + // On error it can be retained and we need to free explicitly + // (c2pa_free handles verifying free is possible). C2paReader* updated = c2pa_reader_with_stream(c2pa_reader, extension.c_str(), cpp_stream->c_stream); - c2pa_reader = nullptr; if (updated == nullptr) { - throw C2paException(); + C2paException ex; + c2pa_free(c2pa_reader); + c2pa_reader = nullptr; + throw ex; } c2pa_reader = updated; } @@ -118,16 +126,20 @@ namespace c2pa throw C2paException("Failed to create reader from context"); } - // c2pa_reader_with_manifest_data_and_stream always consumes c2pa_reader. + // On success this consumes c2pa_reader. + // On error it can be retained and we need to free explicitly + // (c2pa_free handles verifying free is possible). C2paReader* updated = c2pa_reader_with_manifest_data_and_stream( c2pa_reader, format.c_str(), cpp_stream->c_stream, manifest_jumbf.data(), manifest_jumbf.size()); - c2pa_reader = nullptr; if (updated == nullptr) { - throw C2paException(); + C2paException ex; + c2pa_free(c2pa_reader); + c2pa_reader = nullptr; + throw ex; } c2pa_reader = updated; @@ -187,16 +199,19 @@ namespace c2pa CppIStream main_wrapper(stream); CppIStream fragment_wrapper(fragment); - // c2pa_reader_with_fragment consumes the existing reader and returns a new one. - // *this is returned for chaining so reading can go through all segments. + // On success c2pa_reader_with_fragment consumes the existing reader and returns a new one. + // On error it can be retained and we need to free explicitly + // (c2pa_free handles verifying free is possible). C2paReader* updated = c2pa_reader_with_fragment( c2pa_reader, format.c_str(), main_wrapper.c_stream, fragment_wrapper.c_stream); - c2pa_reader = nullptr; if (updated == nullptr) { - throw C2paException(); + C2paException ex; + c2pa_free(c2pa_reader); + c2pa_reader = nullptr; + throw ex; } c2pa_reader = updated; diff --git a/tests/builder.test.cpp b/tests/builder.test.cpp index 9301c62..0cf4065 100644 --- a/tests/builder.test.cpp +++ b/tests/builder.test.cpp @@ -7278,3 +7278,27 @@ TEST_F(BuilderTest, ArchiveToFstreamBackedCppOStream) { EXPECT_GT(std::filesystem::file_size(archive_path), 0u); } + +// A malformed archive makes c2pa_builder_with_archive fail. +// The wrapper must throw and free the retained builder. +TEST(BuilderErrorHandling, FromArchiveMalformedThrows) +{ + std::stringstream bad_archive(std::ios::in | std::ios::out | std::ios::binary); + bad_archive << "not a valid c2pa archive"; + bad_archive.seekg(0); + + EXPECT_THROW(c2pa::Builder::from_archive(bad_archive), c2pa::C2paException); +} + +TEST(BuilderErrorHandling, WithArchiveMalformedThrows) +{ + auto manifest = c2pa_test::read_text_file(c2pa_test::get_fixture_path("training.json")); + auto context = c2pa::Context(); + auto builder = c2pa::Builder(context, manifest); + + std::stringstream bad_archive(std::ios::in | std::ios::out | std::ios::binary); + bad_archive << "not a valid c2pa archive"; + bad_archive.seekg(0); + + EXPECT_THROW(builder.with_archive(bad_archive), c2pa::C2paException); +}