[PATCH] D46816: [llvm-rc] Read the Planes/BitCount fields from BITMAPINFOHEADER for icons

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added inline comments.


================
Comment at: tools/llvm-rc/ResourceFileWriter.cpp:929
+      NewHeader.Cursor.Width = OldHeader.Icon.Width;
+      // Each cursor in fact stores two bitmaps, one under another.
+      // Height provided in cursor definition describes the height of the
----------------
amccarth wrote:
> Most icons also store two bitmaps (the mask and the "color").  I wonder why this adjustment is needed only for cursors.
No idea about the "why" here...


================
Comment at: tools/llvm-rc/ResourceFileWriter.cpp:945-946
     RETURN_IF_ERROR(Reader.readObject(BMPHeader));
-    NewHeader.Planes = BMPHeader->Planes;
-    NewHeader.BitCount = BMPHeader->BitCount;
-
-    // Two WORDs were written at the beginning of the resource (hotspot
-    // location). This is reflected in Size field.
-    NewHeader.Size = OldHeader.Size + 2 * sizeof(uint16_t);
+    constexpr size_t BITMAPINFOHEADER_size = 0x28;
+    if (BMPHeader->Size == BITMAPINFOHEADER_size) {
+      NewHeader.Planes = BMPHeader->Planes;
----------------
zturner wrote:
> How about just `sizeof(BitmapInfoHeader)`?
Sure, that probably works.


================
Comment at: tools/llvm-rc/ResourceFileWriter.cpp:950
+    } else {
+      // A PNG .ico file.
+      // https://blogs.msdn.microsoft.com/oldnewthing/20101022-00/?p=12473
----------------
amccarth wrote:
> Is there a more-specific test to make sure this is a PNG icon and nut just a corrupted file?  For example, BMPHeader->biCompression should have a special value for PNG.
For png, this actually isn't a bmpheader at all, but the png header. I could check if the size field (the first field of the struct) matches the png magic interpreted as ulittle32_t.


Repository:
  rL LLVM

https://reviews.llvm.org/D46816