From 2e2cae26c6995c0b3f0e1d3f2ed09858aeb38ed3 Mon Sep 17 00:00:00 2001 From: Nico Weber Date: Mon, 4 Mar 2024 18:07:43 -0500 Subject: [PATCH] LibGfx+Fallout: Make ImageDecoder return ErrorOr ...from try_create_for_raw_bytes(). If a plugin returns `true` from sniff but then fails when calling its `create()` method, we now no longer swallow that error. Allows `image` (and other places in the system) to print a more actionable error if early image headers are invalid. (We now no longer try to find another plugin that can also handle the image.) Fixes a regression from #20063 / #19893 -- before then, we didn't do fallible work this early. --- .../Applications/FileManager/PropertiesWindow.cpp | 2 +- Userland/Applications/ImageViewer/ViewWidget.cpp | 2 +- Userland/Applications/Maps/MapWidget.cpp | 5 +++-- Userland/Libraries/LibGUI/ImageWidget.cpp | 2 +- Userland/Libraries/LibGfx/Bitmap.cpp | 2 +- .../Libraries/LibGfx/ImageFormats/ImageDecoder.cpp | 14 ++++++-------- .../Libraries/LibGfx/ImageFormats/ImageDecoder.h | 2 +- .../Services/ImageDecoder/ConnectionFromClient.cpp | 5 +++-- Userland/Services/SpiceAgent/SpiceAgent.cpp | 2 +- Userland/Utilities/file.cpp | 2 +- Userland/Utilities/icc.cpp | 2 +- Userland/Utilities/image.cpp | 4 ++-- 12 files changed, 22 insertions(+), 22 deletions(-) diff --git a/Userland/Applications/FileManager/PropertiesWindow.cpp b/Userland/Applications/FileManager/PropertiesWindow.cpp index 730ef96084..b39cd3e61f 100644 --- a/Userland/Applications/FileManager/PropertiesWindow.cpp +++ b/Userland/Applications/FileManager/PropertiesWindow.cpp @@ -403,7 +403,7 @@ ErrorOr PropertiesWindow::create_font_tab(GUI::TabWidget& tab_widget, Nonn ErrorOr PropertiesWindow::create_image_tab(GUI::TabWidget& tab_widget, NonnullOwnPtr mapped_file, StringView mime_type) { - auto image_decoder = Gfx::ImageDecoder::try_create_for_raw_bytes(mapped_file->bytes(), mime_type); + auto image_decoder = TRY(Gfx::ImageDecoder::try_create_for_raw_bytes(mapped_file->bytes(), mime_type)); if (!image_decoder) return {}; diff --git a/Userland/Applications/ImageViewer/ViewWidget.cpp b/Userland/Applications/ImageViewer/ViewWidget.cpp index c65a23518a..a699cacf6d 100644 --- a/Userland/Applications/ImageViewer/ViewWidget.cpp +++ b/Userland/Applications/ImageViewer/ViewWidget.cpp @@ -224,7 +224,7 @@ ErrorOr ViewWidget::try_open_file(String const& path, Core::File& file) Vector frames; // Note: Doing this check only requires reading the header of images // (so if the image is not vector graphics it can be still be decoded OOP). - if (auto decoder = Gfx::ImageDecoder::try_create_for_raw_bytes(file_data); decoder && decoder->natural_frame_format() == Gfx::NaturalFrameFormat::Vector) { + if (auto decoder = TRY(Gfx::ImageDecoder::try_create_for_raw_bytes(file_data)); decoder && decoder->natural_frame_format() == Gfx::NaturalFrameFormat::Vector) { // Use in-process decoding for vector graphics. is_animated = decoder->is_animated(); loop_count = decoder->loop_count(); diff --git a/Userland/Applications/Maps/MapWidget.cpp b/Userland/Applications/Maps/MapWidget.cpp index 70b8105db7..6a6ce5bfea 100644 --- a/Userland/Applications/Maps/MapWidget.cpp +++ b/Userland/Applications/Maps/MapWidget.cpp @@ -341,11 +341,12 @@ void MapWidget::process_tile_queue() m_first_image_loaded = true; // Decode loaded PNG image data - auto decoder = Gfx::ImageDecoder::try_create_for_raw_bytes(payload, "image/png"); - if (!decoder || (decoder->frame_count() == 0)) { + auto decoder_or_err = Gfx::ImageDecoder::try_create_for_raw_bytes(payload, "image/png"); + if (decoder_or_err.is_error() || !decoder_or_err.value() || (decoder_or_err.value()->frame_count() == 0)) { dbgln("Maps: Can't decode image: {}", url); return; } + auto decoder = decoder_or_err.release_value(); m_tiles.set(tile_key, decoder->frame(0).release_value_but_fixme_should_propagate_errors().image); // FIXME: only update the part of the screen that this tile covers diff --git a/Userland/Libraries/LibGUI/ImageWidget.cpp b/Userland/Libraries/LibGUI/ImageWidget.cpp index d262a9b981..e3a3ce2bcc 100644 --- a/Userland/Libraries/LibGUI/ImageWidget.cpp +++ b/Userland/Libraries/LibGUI/ImageWidget.cpp @@ -79,7 +79,7 @@ void ImageWidget::load_from_file(StringView path) auto& mapped_file = *file_or_error.value(); auto mime_type = Core::guess_mime_type_based_on_filename(path); - m_image_decoder = Gfx::ImageDecoder::try_create_for_raw_bytes(mapped_file.bytes(), mime_type); + m_image_decoder = MUST(Gfx::ImageDecoder::try_create_for_raw_bytes(mapped_file.bytes(), mime_type)); VERIFY(m_image_decoder); auto frame = m_image_decoder->frame(0).release_value_but_fixme_should_propagate_errors(); diff --git a/Userland/Libraries/LibGfx/Bitmap.cpp b/Userland/Libraries/LibGfx/Bitmap.cpp index b4648224ba..8064a89955 100644 --- a/Userland/Libraries/LibGfx/Bitmap.cpp +++ b/Userland/Libraries/LibGfx/Bitmap.cpp @@ -147,7 +147,7 @@ ErrorOr> Bitmap::load_from_file(NonnullOwnPtr ErrorOr> Bitmap::load_from_bytes(ReadonlyBytes bytes, Optional ideal_size, Optional mine_type) { - if (auto decoder = ImageDecoder::try_create_for_raw_bytes(bytes, mine_type)) { + if (auto decoder = TRY(ImageDecoder::try_create_for_raw_bytes(bytes, mine_type))) { auto frame = TRY(decoder->frame(0, ideal_size)); if (auto& bitmap = frame.image) return bitmap.release_nonnull(); diff --git a/Userland/Libraries/LibGfx/ImageFormats/ImageDecoder.cpp b/Userland/Libraries/LibGfx/ImageFormats/ImageDecoder.cpp index 56d054bfde..5f703f8d14 100644 --- a/Userland/Libraries/LibGfx/ImageFormats/ImageDecoder.cpp +++ b/Userland/Libraries/LibGfx/ImageFormats/ImageDecoder.cpp @@ -26,7 +26,7 @@ namespace Gfx { -static OwnPtr probe_and_sniff_for_appropriate_plugin(ReadonlyBytes bytes) +static ErrorOr> probe_and_sniff_for_appropriate_plugin(ReadonlyBytes bytes) { struct ImagePluginInitializer { bool (*sniff)(ReadonlyBytes) = nullptr; @@ -56,11 +56,9 @@ static OwnPtr probe_and_sniff_for_appropriate_plugin(Readonl auto sniff_result = plugin.sniff(bytes); if (!sniff_result) continue; - auto plugin_decoder = plugin.create(bytes); - if (!plugin_decoder.is_error()) - return plugin_decoder.release_value(); + return TRY(plugin.create(bytes)); } - return {}; + return OwnPtr {}; } static OwnPtr probe_and_sniff_for_appropriate_plugin_with_known_mime_type(StringView mime_type, ReadonlyBytes bytes) @@ -88,9 +86,9 @@ static OwnPtr probe_and_sniff_for_appropriate_plugin_with_kn return {}; } -RefPtr ImageDecoder::try_create_for_raw_bytes(ReadonlyBytes bytes, Optional mime_type) +ErrorOr> ImageDecoder::try_create_for_raw_bytes(ReadonlyBytes bytes, Optional mime_type) { - if (OwnPtr plugin = probe_and_sniff_for_appropriate_plugin(bytes); plugin) + if (auto plugin = TRY(probe_and_sniff_for_appropriate_plugin(bytes)); plugin) return adopt_ref_if_nonnull(new (nothrow) ImageDecoder(plugin.release_nonnull())); if (mime_type.has_value()) { @@ -98,7 +96,7 @@ RefPtr ImageDecoder::try_create_for_raw_bytes(ReadonlyBytes bytes, return adopt_ref_if_nonnull(new (nothrow) ImageDecoder(plugin.release_nonnull())); } - return {}; + return RefPtr {}; } ImageDecoder::ImageDecoder(NonnullOwnPtr plugin) diff --git a/Userland/Libraries/LibGfx/ImageFormats/ImageDecoder.h b/Userland/Libraries/LibGfx/ImageFormats/ImageDecoder.h index 31c816f0e6..9c7fe47672 100644 --- a/Userland/Libraries/LibGfx/ImageFormats/ImageDecoder.h +++ b/Userland/Libraries/LibGfx/ImageFormats/ImageDecoder.h @@ -98,7 +98,7 @@ protected: class ImageDecoder : public RefCounted { public: - static RefPtr try_create_for_raw_bytes(ReadonlyBytes, Optional mime_type = {}); + static ErrorOr> try_create_for_raw_bytes(ReadonlyBytes, Optional mime_type = {}); ~ImageDecoder() = default; IntSize size() const { return m_plugin->size(); } diff --git a/Userland/Services/ImageDecoder/ConnectionFromClient.cpp b/Userland/Services/ImageDecoder/ConnectionFromClient.cpp index acb5d7f0b0..e92c022085 100644 --- a/Userland/Services/ImageDecoder/ConnectionFromClient.cpp +++ b/Userland/Services/ImageDecoder/ConnectionFromClient.cpp @@ -43,11 +43,12 @@ static void decode_image_to_details(Core::AnonymousBuffer const& encoded_buffer, VERIFY(bitmaps.size() == 0); VERIFY(durations.size() == 0); - auto decoder = Gfx::ImageDecoder::try_create_for_raw_bytes(ReadonlyBytes { encoded_buffer.data(), encoded_buffer.size() }, known_mime_type); - if (!decoder) { + auto decoder_or_err = Gfx::ImageDecoder::try_create_for_raw_bytes(ReadonlyBytes { encoded_buffer.data(), encoded_buffer.size() }, known_mime_type); + if (decoder_or_err.is_error() || !decoder_or_err.value()) { dbgln_if(IMAGE_DECODER_DEBUG, "Could not find suitable image decoder plugin for data"); return; } + auto decoder = decoder_or_err.release_value(); if (!decoder->frame_count()) { dbgln_if(IMAGE_DECODER_DEBUG, "Could not decode image from encoded data"); diff --git a/Userland/Services/SpiceAgent/SpiceAgent.cpp b/Userland/Services/SpiceAgent/SpiceAgent.cpp index bdd4ab630a..77fb2b5ed3 100644 --- a/Userland/Services/SpiceAgent/SpiceAgent.cpp +++ b/Userland/Services/SpiceAgent/SpiceAgent.cpp @@ -256,7 +256,7 @@ ErrorOr SpiceAgent::did_receive_clipboard_message(ClipboardMessage& messag auto mime_type = TRY(clipboard_data_type_to_mime_type(message.data_type())); // FIXME: It should be trivial to make `try_create_for_raw_bytes` take a `StringView` instead of a direct `ByteString`. - auto decoder = Gfx::ImageDecoder::try_create_for_raw_bytes(message.contents(), mime_type.to_byte_string()); + auto decoder = TRY(Gfx::ImageDecoder::try_create_for_raw_bytes(message.contents(), mime_type.to_byte_string())); if (!decoder || (decoder->frame_count() == 0)) { return Error::from_string_literal("Failed to find a suitable decoder for a pasted image!"); } diff --git a/Userland/Utilities/file.cpp b/Userland/Utilities/file.cpp index d33be7bde8..db54a5a6d8 100644 --- a/Userland/Utilities/file.cpp +++ b/Userland/Utilities/file.cpp @@ -33,7 +33,7 @@ static ErrorOr> image_details(StringView description, StringVie { auto mapped_file = TRY(Core::MappedFile::map(path)); auto mime_type = Core::guess_mime_type_based_on_filename(path); - auto image_decoder = Gfx::ImageDecoder::try_create_for_raw_bytes(mapped_file->bytes(), mime_type); + auto image_decoder = TRY(Gfx::ImageDecoder::try_create_for_raw_bytes(mapped_file->bytes(), mime_type)); if (!image_decoder) return OptionalNone {}; diff --git a/Userland/Utilities/icc.cpp b/Userland/Utilities/icc.cpp index 61ae09d575..0c6316e29a 100644 --- a/Userland/Utilities/icc.cpp +++ b/Userland/Utilities/icc.cpp @@ -276,7 +276,7 @@ ErrorOr serenity_main(Main::Arguments arguments) } auto file = TRY(Core::MappedFile::map(path)); - auto decoder = Gfx::ImageDecoder::try_create_for_raw_bytes(file->bytes()); + auto decoder = TRY(Gfx::ImageDecoder::try_create_for_raw_bytes(file->bytes())); if (decoder) { if (auto embedded_icc_bytes = TRY(decoder->icc_data()); embedded_icc_bytes.has_value()) { icc_bytes = *embedded_icc_bytes; diff --git a/Userland/Utilities/image.cpp b/Userland/Utilities/image.cpp index 0a15abcc72..66f1c93cb0 100644 --- a/Userland/Utilities/image.cpp +++ b/Userland/Utilities/image.cpp @@ -227,9 +227,9 @@ ErrorOr serenity_main(Main::Arguments arguments) Options options = TRY(parse_options(arguments)); auto file = TRY(Core::MappedFile::map(options.in_path)); - auto decoder = Gfx::ImageDecoder::try_create_for_raw_bytes(file->bytes()); + auto decoder = TRY(Gfx::ImageDecoder::try_create_for_raw_bytes(file->bytes())); if (!decoder) - return Error::from_string_view("Failed to decode input file"sv); + return Error::from_string_view("Could not find decoder for input file"sv); LoadedImage image = TRY(load_image(*decoder, options.frame_index));