Skip to content

ICO: gate AND-mask handling on the embedded BMP's real bit depth, not ICONDIRENTRY#3057

Closed
greymoth-jp wants to merge 1 commit into
image-rs:mainfrom
greymoth-jp:greymoth-jp/fix/ico-and-mask-real-bitdepth
Closed

ICO: gate AND-mask handling on the embedded BMP's real bit depth, not ICONDIRENTRY#3057
greymoth-jp wants to merge 1 commit into
image-rs:mainfrom
greymoth-jp:greymoth-jp/fix/ico-and-mask-real-bitdepth

Conversation

@greymoth-jp

Copy link
Copy Markdown
Contributor

Follow-up to #3056. After that merged, @RunDevelopment kindly invited more ICO edge cases ("if you found more edge cases that aren't handled properly, feel free to let us know, either by opening an issue or providing a fix"), so here is one.

The bug

In src/codecs/ico/decoder.rs, the decision to ignore the AND mask for true-color icons is keyed on the ICONDIRENTRY bits-per-pixel field (selected_entry.bits_per_pixel):

// 32bpp BMPs already have a native alpha channel, so the AND mask is ignored.
// For lower bit depths, read and apply the AND mask.
if self.selected_entry.bits_per_pixel < 32 {
    // ... apply AND mask, zeroing alpha where masked ...
}

That field is unreliable:

  • For ICO, wBitCount may be 0 ("unspecified"), in which case the decoder guesses a bit depth from bColorCount (see read_entry). A genuine 32bpp icon authored with wBitCount = 0 therefore resolves to a low guessed depth.
  • For CUR, that same field is the hotspot Y coordinate, not a bit depth at all.

The authoritative bit depth is the embedded BMP's real biBitCount, which is already parsed into BmpDecoder's private bit_count but had no accessor.

Consequence: a true-32bpp icon whose ICONDIRENTRY bpp resolves to <32 wrongly takes the low-bit-depth branch and applies the AND mask to a true-color image, zeroing its native alpha.

The fix

  • Add a pub(crate) fn source_bit_count(&self) -> u16 accessor on BmpDecoder exposing the parsed biBitCount (mirrors the existing #[cfg(feature = "ico")] reader() accessor).
  • Gate the mask-ignore decision on source_bit_count instead of selected_entry.bits_per_pixel.
  • Apply the same change to the sibling lenient "truncated mask for 32bpp" acceptance branch, since it encodes the exact same "image has native alpha → ignore the AND mask" decision.

Test

Added bmp_32bpp_unspecified_entry_bpp_and_mask_ignored. It takes the existing bmp-32bpp-conflicting-and-mask.ico fixture (a genuine 32bpp icon whose AND mask, if applied, would zero alpha), rewrites its ICONDIRENTRY wBitCount to 0 ("unspecified" — still a valid 32bpp icon), and asserts the native alpha (128) is preserved. This test fails on main (alpha comes back 0) and passes with the fix.

Verified locally:

  • cargo test --no-default-features --features ico — all ICO tests pass, including the existing bmp_32bpp_and_mask_ignored and truncated_mask_32bpp_lenient (both fixtures have real biBitCount == 32, so behavior is unchanged for them).
  • cargo test --no-default-features --features ico BMP tests — all pass.
  • clippy and rustfmt --check clean on both edited files; --features bmp (without ico) still builds (accessor is cfg-gated).

Disclaimer: this change was prepared with AI assistance; I reviewed the diff, re-verified the bug, and ran the tests myself before opening this.

…RENTRY

The decision to ignore the AND mask for true-color icons was keyed on the
ICONDIRENTRY bits-per-pixel field, which is unreliable: for ICO it may be 0
("unspecified", then guessed from bColorCount), and for CUR that field holds
the hotspot Y coordinate. The authoritative value is the embedded BMP's real
biBitCount.

As a result, a genuine 32bpp icon whose ICONDIRENTRY bpp resolves to <32 took
the low-bit-depth branch and applied the AND mask to a true-color image,
zeroing its native alpha.

Add a pub(crate) `BmpDecoder::source_bit_count()` accessor exposing the parsed
biBitCount and gate the mask-ignore decision (and the sibling lenient
truncated-mask acceptance) on it instead of `selected_entry.bits_per_pixel`.

Adds a regression test: the existing 32bpp conflicting-AND-mask fixture with its
ICONDIRENTRY wBitCount rewritten to "unspecified" (0) must still preserve its
native alpha.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@RunDevelopment

Copy link
Copy Markdown
Member

Thank you, but... In general, before searching for bugs and making fixes, it's a good idea to look at existing issues and PRs. 😅

See: #3047. Funnily enough, the two fixes are near identical. The main differences are that the AI liked to write (probably needlessly) verbose comments and that the added tests are different. And the main difference for the added tests are that this PR uses an icon entry bit count of 0 while #3047 uses 8. (Note that I picked 8 to try and trick the decoder. A decoder might trust plausible values (1, 4, 8, 16, 24, 32) even when it shouldn't.)

So we are unfortunately now in the situation where I can't meaningfully review this PR. I'd basically review my own code and any changes I want to request would only change your PR into my PR. (Because I like my own PR better, of course :) )

So.... @197g, which one do you want to pick? This PR and #3047 are basically the same. Really the one thing that needs changing here is the bit count in the test (any plausible value <32 will do).

Disclaimer: this change was prepared with AI assistance; I reviewed the diff, re-verified the bug, and ran the tests myself before opening this.

Very good disclaimer 👍

@greymoth-jp

Copy link
Copy Markdown
Contributor Author

You're right, this overlaps #3047 and I should have checked the open PRs first. Closing it. Thanks for the catch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants