From 402de2985d9c999a2d2bdcfd4262360de1cd4aaf Mon Sep 17 00:00:00 2001
From: Lucas CHOLLET <lucas.chollet@free.fr>
Date: Sat, 6 Jan 2024 19:24:41 -0500
Subject: [PATCH] LibGfx/ICO: Do not try to decode a mask if we already reached
 EOF

When using the BMP encoding, ICO images are expected to contain a 1-bit
mask for transparency. Regardless an alpha channel is already included
in the image, the mask is always required. As stated here[1], the
mask is used to provide shadow around the image.

Unfortunately, it seems that some encoder do not include that second
transparency mask. So let's read that mask only if some data is still
remaining after decoding the image.

The test case has been generated by truncating the 64 last bytes
(originally dedicated to the mask) from the `serenity.ico` file and
changing the declared size of the image in the ICO header. The size
value is stored at the offset 0x0E in the file and I changed the value
from 0x0468 to 0x0428.

[1]: https://devblogs.microsoft.com/oldnewthing/20101021-00/?p=12483
---
 Tests/LibGfx/TestImageDecoder.cpp                |  11 +++++++++++
 .../test-inputs/ico/malformed_maskless.ico       | Bin 0 -> 1086 bytes
 .../Libraries/LibGfx/ImageFormats/BMPLoader.cpp  |   4 ++--
 3 files changed, 13 insertions(+), 2 deletions(-)
 create mode 100644 Tests/LibGfx/test-inputs/ico/malformed_maskless.ico

diff --git a/Tests/LibGfx/TestImageDecoder.cpp b/Tests/LibGfx/TestImageDecoder.cpp
index 55de6c1292..444f98f0e4 100644
--- a/Tests/LibGfx/TestImageDecoder.cpp
+++ b/Tests/LibGfx/TestImageDecoder.cpp
@@ -177,6 +177,17 @@ TEST_CASE(test_bmp_embedded_in_ico)
     EXPECT_EQ(frame.image->get_pixel(7, 4), Gfx::Color(161, 0, 0));
 }
 
+TEST_CASE(test_malformed_maskless_ico)
+{
+    auto file = TRY_OR_FAIL(Core::MappedFile::map(TEST_INPUT("ico/malformed_maskless.ico"sv)));
+    EXPECT(Gfx::ICOImageDecoderPlugin::sniff(file->bytes()));
+    auto plugin_decoder = TRY_OR_FAIL(Gfx::ICOImageDecoderPlugin::create(file->bytes()));
+
+    auto frame = TRY_OR_FAIL(expect_single_frame_of_size(*plugin_decoder, { 16, 16 }));
+    EXPECT_EQ(frame.image->get_pixel(0, 0), Gfx::Color::NamedColor::Transparent);
+    EXPECT_EQ(frame.image->get_pixel(7, 4), Gfx::Color(161, 0, 0));
+}
+
 TEST_CASE(test_ilbm)
 {
     auto file = MUST(Core::MappedFile::map(TEST_INPUT("ilbm/gradient.iff"sv)));
diff --git a/Tests/LibGfx/test-inputs/ico/malformed_maskless.ico b/Tests/LibGfx/test-inputs/ico/malformed_maskless.ico
new file mode 100644
index 0000000000000000000000000000000000000000..3736d606c1fd4de33c483d273ffd505fd5ce437a
GIT binary patch
literal 1086
zcmZQzU}Ruq5D);-3Je-73=Con3=A3!3=9Gc3=9ek5OD^wK*E0p1`sAy^X~r)3=RJo
z7#2ct?|%k{?Em;oVPj*1%7gTR^nuiaXb^_6K{NvcLM_5XkQfL<?9BcTVT05ovloKJ
zA#w=42o}h0RQ1@*L#Rh*f%K!h7sLl)kXv9hx;_{mq@Q&ALH+?@1_t8vgVcgB%vJ;q
z(hqYZhz4O~cf<Gy9jGjje$w3!G6Uou5XSTavUw0TL>)*M0;7t-+zFyV7~}^KhNwqX
m3lT#nL28gOx;!y_n0{i^qN{=Fh0*Bp`1l|_=<?XaKxzQejIL4u

literal 0
HcmV?d00001

diff --git a/Userland/Libraries/LibGfx/ImageFormats/BMPLoader.cpp b/Userland/Libraries/LibGfx/ImageFormats/BMPLoader.cpp
index f5b8e0bda1..eca50ef10f 100644
--- a/Userland/Libraries/LibGfx/ImageFormats/BMPLoader.cpp
+++ b/Userland/Libraries/LibGfx/ImageFormats/BMPLoader.cpp
@@ -1429,7 +1429,7 @@ static ErrorOr<void> decode_bmp_pixel_data(BMPLoadingContext& context)
             TRY(process_row(row));
         }
 
-        if (context.is_included_in_ico) {
+        if (context.is_included_in_ico && !streamer.at_end()) {
             for (u32 row = 0; row < height; ++row) {
                 TRY(process_mask_row(row));
             }
@@ -1440,7 +1440,7 @@ static ErrorOr<void> decode_bmp_pixel_data(BMPLoadingContext& context)
             TRY(process_row(row));
         }
 
-        if (context.is_included_in_ico) {
+        if (context.is_included_in_ico && !streamer.at_end()) {
             for (i32 row = height - 1; row >= 0; --row) {
                 TRY(process_mask_row(row));
             }