bitmaps: Make missing color palette bytes a hard error instead of a w…

…arning Bitmaps with missing color palette bytes are treated as invalid by both the Windows and Firefox bitmap parsers, and therefore emulating the Windows behavior of padding out the missing bytes doesn't make much sense. This simplifies the handling of this edge case and removes the need for a (to-be-configurable) maximum number of missing palette bytes.
squeek502 · Jun 11, 2024 · e902331 · e902331
1 parent d51c18b
commit e902331
Show file tree

Hide file tree

Showing 4 changed files with 34 additions and 66 deletions.
diff --git a/docs/divergences.md b/docs/divergences.md
@@ -81,14 +81,13 @@ nav_order: 2
   + By always erroring on sizes that can't possibly hold a real image, `resinator` avoids both of these behaviors that always lead to invalid `.res` files.
 - `resinator` will allow bitmap `ICON`/`CURSOR` images with versions other than `Windows NT, 3.1x (BITMAPINFOHEADER)` (e.g. `BITMAPV5HEADER`, unknown versions, etc), and will emit a warning
   + The Win32 RC compiler is strict about the DIB bitmap version it allows, and errors on anything that's not `Windows NT, 3.1x (BITMAPINFOHEADER)`. This needs to be tested, but my hunch is that this limitation is artificial, and that if newer bitmap formats were included in a `.res` that they would be able to be loaded at runtime just fine.
-- `resinator` will avoid a miscompilation when padding missing bytes in the color table of `BITMAP` resources; `resinator` will always fill the missing bytes with zeroes, and will emit a warning
-  + The Win32 RC compiler will fill missing color palette bytes with zeroes if the size of the color palette is specified to be larger than it actually is in the file, but it will read into the pixel data and use that until it reaches EOF. This is almost certainly not what is intended, as pixel data and color table data do not have the same format so it'll essentially be filling the missing bytes with random data.
+- `resinator` will error if there are any missing bytes in the color table of `BITMAP` resources
+  + The Win32 RC compiler will fill missing color palette bytes with zeroes if the size of the color palette is specified to be larger than it actually is in the file, and it will read into the pixel data and use that until it reaches EOF (at which point it will pad with zeroes). Such bitmaps are invalid anyway, so there's no benefit to this behavior.
+  + The Win32 RC compiler has no limit of the amount of missing padding bytes it will add, and will either give an out-of-memory error or just unconditionally write huge amounts of zeroes to the color palette (e.g. if the bit depth of the `.bmp` is 32 and the number of colors used is set to `200,000,000`, then `num_colors * 4` or `800,000,000` bytes will be written to the color palette). Such large amounts of colors are extremely likely be indicative of a malformed bitmap, as bit depths 16, 24, and 32 only use color tables for optimization purposes (["The bmiColors color table is used for optimizing colors used on palette-based devices, and must contain the number of entries specified by the biClrUsed member of the BITMAPINFOHEADER."](https://learn.microsoft.com/en-us/previous-versions//dd183376(v=vs.85))).
+  + Note: Enforcing this avoids malformed bitmaps potentially inducing really large `.res` files
 - `resinator` will error on `BITMAP` resources that contain more colors than their bit depth allows (2^n, e.g. bit depth 4 can have a maximum of 16 colors, bit depth 8 can have a maximum of 256 colors, etc)
   + The Win32 RC compiler does not enforce this, but such bitmaps are definitely malformed. Even for bit depths >= 16 where '[the] bmiColors color table is used for optimizing colors used on palette-based devices', it still wouldn't make sense for the number of colors in the palette to exceed the possible number of colors used by the pixel data.
-  + Note: Enforcing this limit avoids malformed bitmaps potentially inducing really large `.res` files
-- `resinator` will error on `BITMAP` resources with a color palette that contains too many missing bytes. The particular limit is configurable (note: this being configurable is still TODO) and the default is 4096.
-  + The Win32 RC compiler does no checking of this, and will either give an out-of-memory error or just unconditionally write huge amounts of zeroes to the color palette (e.g. if the bit depth of the `.bmp` is 32 and the number of colors used is set to `200,000,000`, then `num_colors * 4` or `800,000,000` bytes will be written to the color palette). Such large amounts of colors are extremely likely be indicative of a malformed bitmap, as bit depths 16, 24, and 32 only use color tables for optimization purposes (["The bmiColors color table is used for optimizing colors used on palette-based devices, and must contain the number of entries specified by the biClrUsed member of the BITMAPINFOHEADER."](https://learn.microsoft.com/en-us/previous-versions//dd183376(v=vs.85))).
-  + Note: Enforcing a limit here avoids malformed bitmaps potentially inducing really large `.res` files
+  + TODO: revisit this; [bmpsuite](https://entropymine.com/jason/bmpsuite/bmpsuite/html/bmpsuite.html) says that such bitmaps 'may be invalid', see `q/pal8oversizepal.bmp`
 - `resinator` will always error if the code page specified in a `#pragma code_page` directive would overflow a `u32`.
   + The Win32 RC compiler has different behavior depending on whether or not the value after wrapping on overflow ends up being a known code page ID or not:
     - If the overflowed `u32` wraps and becomes a known code page ID, then it will error/warn with "Codepage not valid:  ignored" (depending on the `/w` option)

diff --git a/src/compile.zig b/src/compile.zig
@@ -856,47 +856,32 @@ pub const Compiler = struct {
                     } else if (bitmap_info.getActualPaletteByteLen() < bitmap_info.getExpectedPaletteByteLen()) {
                         const num_padding_bytes = bitmap_info.getExpectedPaletteByteLen() - bitmap_info.getActualPaletteByteLen();
 
-                        // TODO: Make this configurable (command line option)
-                        const max_missing_bytes = 4096;
-                        if (num_padding_bytes > max_missing_bytes) {
-                            var numbers_as_bytes: [16]u8 = undefined;
-                            std.mem.writeInt(u64, numbers_as_bytes[0..8], num_padding_bytes, native_endian);
-                            std.mem.writeInt(u64, numbers_as_bytes[8..16], max_missing_bytes, native_endian);
-                            const values_string_index = try self.diagnostics.putString(&numbers_as_bytes);
-                            try self.addErrorDetails(.{
-                                .err = .bmp_too_many_missing_palette_bytes,
-                                .token = filename_token,
-                                .extra = .{ .number = values_string_index },
-                            });
-                            return self.addErrorDetailsAndFail(.{
-                                .err = .bmp_too_many_missing_palette_bytes,
-                                .type = .note,
-                                .print_source_line = false,
-                                .token = filename_token,
-                            });
-                        }
-
                         var number_as_bytes: [8]u8 = undefined;
                         std.mem.writeInt(u64, &number_as_bytes, num_padding_bytes, native_endian);
                         const value_string_index = try self.diagnostics.putString(&number_as_bytes);
                         try self.addErrorDetails(.{
                             .err = .bmp_missing_palette_bytes,
-                            .type = .warning,
+                            .type = .err,
                             .token = filename_token,
                             .extra = .{ .number = value_string_index },
                         });
                         const pixel_data_len = bitmap_info.getPixelDataLen(file_size);
+                        // TODO: This is a hack, but we know we have already added
+                        //       at least one entry to the diagnostics strings, so we can
+                        //       get away with using 0 to mean 'no string' here.
+                        var miscompiled_bytes_string_index: u32 = 0;
                         if (pixel_data_len > 0) {
                             const miscompiled_bytes = @min(pixel_data_len, num_padding_bytes);
                             std.mem.writeInt(u64, &number_as_bytes, miscompiled_bytes, native_endian);
-                            const miscompiled_bytes_string_index = try self.diagnostics.putString(&number_as_bytes);
-                            try self.addErrorDetails(.{
-                                .err = .rc_would_miscompile_bmp_palette_padding,
-                                .type = .warning,
-                                .token = filename_token,
-                                .extra = .{ .number = miscompiled_bytes_string_index },
-                            });
+                            miscompiled_bytes_string_index = try self.diagnostics.putString(&number_as_bytes);
                         }
+                        return self.addErrorDetailsAndFail(.{
+                            .err = .rc_would_miscompile_bmp_palette_padding,
+                            .type = .note,
+                            .print_source_line = false,
+                            .token = filename_token,
+                            .extra = .{ .number = miscompiled_bytes_string_index },
+                        });
                     }
 
                     // TODO: It might be possible that the calculation done in this function
@@ -915,12 +900,6 @@ pub const Compiler = struct {
                     }
                     if (bitmap_info.getExpectedPaletteByteLen() > 0) {
                         try writeResourceDataNoPadding(writer, file_reader, @intCast(bitmap_info.getActualPaletteByteLen()));
-                        // We know that the number of missing palette bytes is <= 4096
-                        // (see `bmp_too_many_missing_palette_bytes` error case above)
-                        const padding_bytes: usize = @intCast(bitmap_info.getMissingPaletteByteLen());
-                        if (padding_bytes > 0) {
-                            try writer.writeByteNTimes(0, padding_bytes);
-                        }
                     }
                     try file.seekTo(bitmap_info.pixel_data_offset);
                     const pixel_bytes: u32 = @intCast(file_size - bitmap_info.pixel_data_offset);

diff --git a/src/errors.zig b/src/errors.zig
@@ -357,11 +357,6 @@ pub const ErrorDetails = struct {
         /// `number` is populated and contains a string index for which the string contains
         /// the bytes of a `u64` (native endian). The `u64` contains the number of miscompiled bytes.
         rc_would_miscompile_bmp_palette_padding,
-        /// `number` is populated and contains a string index for which the string contains
-        /// the bytes of two `u64`s (native endian). The first contains the number of missing
-        /// palette bytes and the second contains the max number of missing palette bytes.
-        /// If type is `.note`, then `extra` is `none`.
-        bmp_too_many_missing_palette_bytes,
         resource_header_size_exceeds_max,
         resource_data_size_exceeds_max,
         control_extra_data_size_exceeds_max,
@@ -667,23 +662,15 @@ pub const ErrorDetails = struct {
             .bmp_missing_palette_bytes => {
                 const bytes = strings[self.extra.number];
                 const missing_bytes = std.mem.readInt(u64, bytes[0..8], native_endian);
-                try writer.print("bitmap has {d} missing color palette bytes which will be padded with zeroes", .{missing_bytes});
+                try writer.print("bitmap has {d} missing color palette bytes", .{missing_bytes});
             },
             .rc_would_miscompile_bmp_palette_padding => {
-                const bytes = strings[self.extra.number];
-                const miscompiled_bytes = std.mem.readInt(u64, bytes[0..8], native_endian);
-                try writer.print("the missing color palette bytes would be miscompiled by the Win32 RC compiler (the added padding bytes would include {d} bytes of the pixel data)", .{miscompiled_bytes});
-            },
-            .bmp_too_many_missing_palette_bytes => switch (self.type) {
-                .err, .warning => {
+                try writer.writeAll("the Win32 RC compiler would erroneously pad out the missing bytes");
+                if (self.extra.number != 0) {
                     const bytes = strings[self.extra.number];
-                    const missing_bytes = std.mem.readInt(u64, bytes[0..8], native_endian);
-                    const max_missing_bytes = std.mem.readInt(u64, bytes[8..16], native_endian);
-                    try writer.print("bitmap has {} missing color palette bytes which exceeds the maximum of {}", .{ missing_bytes, max_missing_bytes });
-                },
-                // TODO: command line option
-                .note => try writer.writeAll("the maximum number of missing color palette bytes is configurable via <<TODO command line option>>"),
-                .hint => return,
+                    const miscompiled_bytes = std.mem.readInt(u64, bytes[0..8], native_endian);
+                    try writer.print(" (and the added padding bytes would include {d} bytes of the pixel data)", .{miscompiled_bytes});
+                }
             },
             .resource_header_size_exceeds_max => {
                 try writer.print("resource's header length exceeds maximum of {} bytes", .{std.math.maxInt(u32)});

diff --git a/test/compile.zig b/test/compile.zig
@@ -383,19 +383,22 @@ test "basic bitmap" {
     //       data into the padding bytes of the color palette.
     try testCompileErrorDetailsWithDir(
         &.{
-            .{ .type = .warning, .str = "bitmap has 16 missing color palette bytes which will be padded with zeroes" },
-            .{ .type = .warning, .str = "the missing color palette bytes would be miscompiled by the Win32 RC compiler (the added padding bytes would include 6 bytes of the pixel data)" },
+            .{ .type = .err, .str = "bitmap has 16 missing color palette bytes" },
+            .{ .type = .note, .str = "the Win32 RC compiler would erroneously pad out the missing bytes (and the added padding bytes would include 6 bytes of the pixel data)" },
         },
         "1 BITMAP test_missing_palette_bytes.bmp",
-        "\x00\x00\x00\x00 \x00\x00\x00\xff\xff\x00\x00\xff\xff\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00>\x00\x00\x00 \x00\x00\x00\xff\xff\x02\x00\xff\xff\x01\x00\x00\x00\x00\x000\x00\t\x04\x00\x00\x00\x00\x00\x00\x00\x00(\x00\x00\x00\x01\x00\x00\x00\x01\x00\x00\x00\x01\x00\x10\x00\x00\x00\x00\x00\x06\x00\x00\x00\x12\x0b\x00\x00\x12\x0b\x00\x00\x04\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\xff\x7f\x00\x00\x00\x00\x00\x00",
+        null,
         tmp_dir.dir,
     );
 
     try tmp_dir.dir.writeFile(.{ .sub_path = "test_win2.0_missing_palette_bytes.bmp", .data = "BMJX\x02\x00\x00\x00\x00\x00G\x00\x00\x00\x0c\x00\x00\x00\x80\x02\xe0\x01\x01\x00\x04\x00\x00\x00\x00\x80\x00\x00\x00\x80\x00\x80\x80\x00\x00\x00\x80\x80\x00\x80\x00\x80\x80\x80\x80\x80\xcc\xcc\xcc\xff\x00\x00\x00\xff\x00\xff\xff\x00\x00\x00\xff\xff\x00\xff\x00\xff\xff" });
     try testCompileErrorDetailsWithDir(
-        &.{.{ .type = .warning, .str = "bitmap has 3 missing color palette bytes which will be padded with zeroes" }},
+        &.{
+            .{ .type = .err, .str = "bitmap has 3 missing color palette bytes" },
+            .{ .type = .note, .str = "the Win32 RC compiler would erroneously pad out the missing bytes" },
+        },
         "1 BITMAP test_win2.0_missing_palette_bytes.bmp",
-        "\x00\x00\x00\x00 \x00\x00\x00\xff\xff\x00\x00\xff\xff\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00<\x00\x00\x00 \x00\x00\x00\xff\xff\x02\x00\xff\xff\x01\x00\x00\x00\x00\x000\x00\t\x04\x00\x00\x00\x00\x00\x00\x00\x00\x0c\x00\x00\x00\x80\x02\xe0\x01\x01\x00\x04\x00\x00\x00\x00\x80\x00\x00\x00\x80\x00\x80\x80\x00\x00\x00\x80\x80\x00\x80\x00\x80\x80\x80\x80\x80\xcc\xcc\xcc\xff\x00\x00\x00\xff\x00\xff\xff\x00\x00\x00\xff\xff\x00\xff\x00\xff\xff\x00\x00\x00",
+        null,
         tmp_dir.dir,
     );
 
@@ -410,8 +413,8 @@ test "basic bitmap" {
     try tmp_dir.dir.writeFile(.{ .sub_path = "test_too_many_missing_palette_bytes.bmp", .data = "BM<\x00\x00\x00\x00\x00\x00\x006\x00\x00\x00(\x00\x00\x00\x01\x00\x00\x00\x01\x00\x00\x00\x01\x00\x20\x00\x00\x00\x00\x00\x06\x00\x00\x00\x12\x0b\x00\x00\x12\x0b\x00\x00\xff\xff\xff\xff\x00\x00\x00\x00\xff\x7f\x00\x00\x00\x00" });
     try testCompileErrorDetailsWithDir(
         &.{
-            .{ .type = .err, .str = "bitmap has 17179869180 missing color palette bytes which exceeds the maximum of 4096" },
-            .{ .type = .note, .str = "the maximum number of missing color palette bytes is configurable via <<TODO command line option>>" },
+            .{ .type = .err, .str = "bitmap has 17179869180 missing color palette bytes" },
+            .{ .type = .note, .str = "the Win32 RC compiler would erroneously pad out the missing bytes (and the added padding bytes would include 6 bytes of the pixel data)" },
         },
         "1 BITMAP test_too_many_missing_palette_bytes.bmp",
         null,