From 7be7ea5e2036fff0bf2f8b50f4ebc762b253fe55 Mon Sep 17 00:00:00 2001 From: Amir Farhane Date: Sun, 14 Jun 2026 18:16:15 +0200 Subject: [PATCH 1/2] test(ico): add regression test for zero-height BMP-in-ICO panic A BMP-backed ICO entry whose embedded BITMAPINFOHEADER height is odd (e.g. 1) is halved to 0 by the ICO double-height convention, after the decoder's overflow/zero validation has already run. Decoding such input must return an error, not panic. This test currently fails (panics). Co-Authored-By: Claude Opus 4.8 --- src/codecs/ico/decoder.rs | 42 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/src/codecs/ico/decoder.rs b/src/codecs/ico/decoder.rs index a738632d82..d041559042 100644 --- a/src/codecs/ico/decoder.rs +++ b/src/codecs/ico/decoder.rs @@ -644,4 +644,46 @@ mod test { IcoDecoder::with_spec_compliance(std::io::Cursor::new(&data), SpecCompliance::Strict); assert!(decoder_strict.is_err()); } + + // A BMP-backed ICO entry whose embedded BITMAPINFOHEADER height is odd (here 1) + // is halved to 0 by the ICO double-height convention. Decoding must return an + // error, not panic in the BMP pixel reader. Regression test for the + // `(self.height - 1).try_into().unwrap()` panic. + #[test] + fn ico_bmp_odd_embedded_height_does_not_panic() { + let mut v: Vec = Vec::new(); + // ICONDIR: reserved=0, type=1 (icon), count=1 + v.extend_from_slice(&[0, 0, 1, 0, 1, 0]); + // ICONDIRENTRY (16 bytes) + v.extend_from_slice(&[1, 1, 0, 0]); // width, height, color_count, reserved + v.extend_from_slice(&1u16.to_le_bytes()); // color planes + v.extend_from_slice(&32u16.to_le_bytes()); // bpp + let len_pos = v.len(); + v.extend_from_slice(&0u32.to_le_bytes()); // image_length (patched below) + v.extend_from_slice(&22u32.to_le_bytes()); // image_offset = 6 + 16 + let img_start = v.len(); + // BITMAPINFOHEADER (40 bytes), no BITMAPFILEHEADER (ICO convention) + v.extend_from_slice(&40u32.to_le_bytes()); // header size + v.extend_from_slice(&1i32.to_le_bytes()); // width = 1 + v.extend_from_slice(&1i32.to_le_bytes()); // height = 1 (odd -> halves to 0) + v.extend_from_slice(&1u16.to_le_bytes()); // planes + v.extend_from_slice(&32u16.to_le_bytes()); // bit_count = 32 (full-byte path) + v.extend_from_slice(&0u32.to_le_bytes()); // compression = BI_RGB + v.extend_from_slice(&[0u8; 20]); // size_image, ppm x/y, colors_used, important + v.extend_from_slice(&[0u8; 64]); // pixel/mask bytes + let img_len = (v.len() - img_start) as u32; + v[len_pos..len_pos + 4].copy_from_slice(&img_len.to_le_bytes()); + + // Default (lenient) mode — the path used by `image::load_from_memory`. + // The decode must fail with an error rather than panic; the error may + // surface at any stage (construction is fail-fast here). + let decode = |bytes: &[u8]| -> ImageResult<()> { + let mut decoder = IcoDecoder::new(std::io::Cursor::new(bytes))?; + let total = decoder.prepare_image()?.total_bytes(); + let mut buf = vec![0u8; usize::try_from(total).unwrap()]; + decoder.read_image(&mut buf)?; + Ok(()) + }; + assert!(decode(&v).is_err()); + } } From 0af05c532d82b0ba424d43df2263d4f56d8e80af Mon Sep 17 00:00:00 2001 From: Amir Farhane Date: Sun, 14 Jun 2026 18:16:15 +0200 Subject: [PATCH 2/2] fix(bmp): reject zero height for ICO-embedded BMP `read_metadata_in_ico_format` halves the BMP height to undo the ICO double-height convention. This halving runs *after* `read_metadata`'s dimension checks, so an entry with an odd doubled-height (e.g. 1) leaves `self.height == 0`. The full-byte and 16-bit pixel readers then evaluate `(self.height - 1).try_into::().unwrap()`, which panics with `TryFromIntError` on a zero height (reachable via the default lenient `image::load_from_memory` path with a ~120-byte crafted ICO). Reject a zero height after halving, mirroring the existing `width < 0` and `height == i32::MIN` validation in the same parser. Co-Authored-By: Claude Opus 4.8 --- src/codecs/bmp/decoder.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/codecs/bmp/decoder.rs b/src/codecs/bmp/decoder.rs index 7cefbebae5..2b829d268b 100644 --- a/src/codecs/bmp/decoder.rs +++ b/src/codecs/bmp/decoder.rs @@ -1729,6 +1729,15 @@ impl BmpDecoder { // The height field in an ICO file is doubled to account for the AND mask // (whether or not an AND mask is actually present). self.height /= 2; + + // The halving above happens after `read_metadata`'s overflow/zero checks, + // so a malformed entry with an odd doubled-height (e.g. 1) leaves height at + // 0 here. A zero height would later panic in the pixel readers, which do + // `(self.height - 1).try_into::().unwrap()`. Reject it as invalid. + if self.height == 0 { + return Err(DecoderError::InvalidHeight.into()); + } + Ok(()) }