fix(bmp): reject zero height for ICO-embedded BMP (panic on crafted input)

[Ace-0-A]

A BMP-backed ICO whose embedded BITMAPINFOHEADER height is odd (e.g. 1) panics
the decoder instead of returning an error. Reachable on the default (lenient)
image::load_from_memory path with a ~120-byte input.

Cause

read_metadata_in_ico_format halves the BMP height to undo the ICO double-height
convention (src/codecs/bmp/decoder.rs:1731). This halving runs after
read_metadata's dimension validation, so an entry with an odd doubled-height
leaves self.height == 0. The full-byte (24/32-bit) and 16-bit pixel readers then
evaluate (self.height - 1).try_into::<u32>().unwrap(), which panics with
TryFromIntError on a zero height.

A standalone BMP with height == 0 is already rejected by check_for_overflow;
only the ICO halving can produce a post-validation zero.

Fix

Reject a zero height immediately after halving, reusing the existing
DecoderError::InvalidHeight and mirroring the width < 0 / height == i32::MIN
checks already in the same parser. No public API change.

A regression test is added first (it panics without the fix); the fix commit makes
it pass. The existing BMP/ICO unit suites and the tests/bad/ corpus stay green.

Minimal reproducer

// default (lenient) decode of a 1x1 32bpp BMP-in-ICO whose embedded
// BITMAPINFOHEADER height is 1 -> halved to 0 -> panic at decoder.rs:2054
let mut v = vec![0,0,1,0,1,0, 1,1,0,0, 1,0, 32,0];      // ICONDIR + entry head
v.extend_from_slice(&104u32.to_le_bytes());              // image_length
v.extend_from_slice(&22u32.to_le_bytes());               // image_offset
v.extend_from_slice(&40u32.to_le_bytes());               // BITMAPINFOHEADER size
v.extend_from_slice(&1i32.to_le_bytes());                // width = 1
v.extend_from_slice(&1i32.to_le_bytes());                // height = 1 (odd)
v.extend_from_slice(&[1,0, 32,0]);                       // planes, bpp=32
v.extend_from_slice(&[0u8; 80]);                         // compression + pixels
let _ = image::load_from_memory_with_format(&v, image::ImageFormat::Ico); // panics on main

[197g]

The fix needs to target the implementation according to specification, not avoid a fault. A height of 0 is a bit odd to be sure though. In the Ico direntry it can not be encoded in the first place, any such entry is mapped to 256 (see real_width). It's new that in permissive mode we use the BITMAPINFOHEADER instead and it should be handled appropriately. That would mean as a height of 0 though not an error (this is lenient mode after all!).

Imagemagick identifies the image as ICO 1x1 1x1+0+0 8-bit sRGB 118B and successfully converts it. It's not really a good reference here though.. From what I can test it uses the width from the BITMAPINFOHEADER but the height from the dir entry. That's really odd.

[RunDevelopment]

I played around with your test image and created a few variations to see how Windows Explorer interprets it. I found that Windows behaves as follows: If biWidth or biHeight / 2 (truncating div) is zero in BITMAPINFOHEADER, it will ignore both fields and use the width and height in the ICON entry as the dimensions of the XOR and AND masks. I think this is a fallback to support invalid ICOs.

So I think the fix for ICO would be to implement this fallback in lenient mode and return an error in strict mode. Strict mode should also validate that biHeight is even.

Also, biBitCount in BITMAPINFOHEADER is redundant in the same way biWidth and biHeight are. So Windows might have a fallback for that too. Further testing is necessary.

That said, BMP decoding is still broken. (self.height - 1).try_into::<u32>().unwrap() trivially panics as identified by @Ace-0-A, And not just for zero. self.height is an i32. AFAICT, some code paths do indeed set this field to a negative number, so assuming self.height > 0 is probably wrong.

(Also, the BMP decoder has 33 .unwrap()s... Maybe we should disallow unwraps for codecs à la clippy::unwrap_used.)

[197g]

Windows behavior sounds sensible to me, too. Thanks for testing it out on that primary target. Agreed with verifying biHeight.is_multiple_of(2).

Regarding the actual panic, that's an artifact of outlining. We calculate that even if the inner loop (over rows) is supposed to be empty. It's only used to test which row is last which does not happen anyways when there are no rows to iterate on. It could just choose any value in this case instead, i.e. saturating_sub(1) should be perfect (with a comment referencing this problem).

[RunDevelopment]

Regarding the panic, while reading through the BMP decoder for #3042 I figured out what the cause is. Basically, the BMP decoder already verifies that width and height are in the interval 1..=u16::MAX. The only path where height could be zero was the ICO path because of height /= 2.

I'm planning to make this undocumented assumption explicit by using NonZeroU16 for width and height internally in another PR.

And yes, the BMP decoder rejects all empty images. It's just super non-obvious from the code and the error it returns is halariously "Image dimensions (0x0 w/4 channels) are too large."