This is an archive of the discontinued LLVM Phabricator instance.

[llvm-rc] Read the Planes/BitCount fields from BITMAPINFOHEADER for icons
ClosedPublic

Authored by mstorsjo on May 13 2018, 11:59 PM.

Details

Summary

Previously these fields were only read from this header for cursors, while Planes was hardcoded to 1 for icons (with a comment that it was unknown why this was needed) and BitCount was left at the value read originally in the RESDIRENTRY.

This fixes the single byte that was differing for the icon/cursor test compared to rc.exe.

This is based on research/testing by Nico Weber.

Diff Detail

Event Timeline

mstorsjo created this revision.May 13 2018, 11:59 PM
amccarth added inline comments.May 14 2018, 8:51 AM
tools/llvm-rc/ResourceFileWriter.cpp
932

Most icons also store two bitmaps (the mask and the "color"). I wonder why this adjustment is needed only for cursors.

953

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.

zturner added inline comments.May 14 2018, 11:14 AM
tools/llvm-rc/ResourceFileWriter.cpp
948–958

How about just sizeof(BitmapInfoHeader)?

mstorsjo added inline comments.May 14 2018, 11:51 AM
tools/llvm-rc/ResourceFileWriter.cpp
932

No idea about the "why" here...

948–958

Sure, that probably works.

953

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.

thakis accepted this revision.May 14 2018, 12:48 PM

lgtm, but please credit where at least parts of this are from :-)

This revision is now accepted and ready to land.May 14 2018, 12:48 PM
mstorsjo updated this revision to Diff 146684.May 14 2018, 2:10 PM
mstorsjo edited the summary of this revision. (Show Details)

Changed to check specifically for the PNG magic header, giving credit where it's due in the commit message :-)

amccarth added inline comments.May 14 2018, 2:39 PM
tools/llvm-rc/ResourceFileWriter.cpp
953

There are three possibilities: BMP, PNG, or the wrong type of file. The original change tested to see if it was a valid BMP and assumed PNG otherwise, discounting the possibility that it could be neither. This just flips that around.

If we're going to ignore the possibility of a corrupt/incorrect input, then the original code was clearer, since it was actually testing the size.

mstorsjo updated this revision to Diff 146694.May 14 2018, 2:44 PM

Back to the original form of the check, but using sizeof(BitmapInfoHeader) instead of a separate constant.

amccarth accepted this revision.May 14 2018, 2:48 PM
This revision was automatically updated to reflect the committed changes.