Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 47 additions & 4 deletions src/codecs/bmp/decoder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1096,6 +1096,7 @@ pub struct BmpDecoder<R> {
top_down: bool,
no_file_header: bool,
add_alpha_channel: bool,
fallback_size: Option<(u16, u16)>,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I suppose ico and bmp are tied together pretty strongly so it makes sense to see this. Just wondering if it already makes sense to have a struct of context input to the BMP that would be filled by ico instead of a non-descript u16 tuple. Your judgment call on whether that extra struct is already more mental overhead or not, a comment on the field suggesting this when future extensions pop up would also do.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, I was thinking of this mostly as a general modifier to how BMPs are parsed. Similar to no_file_header. I don't like the strong coupling between bmp and ico, so I tried to make something that isn't useful for ico alone. Not sure if I succeeded though...

So I'd hold the extra struct for now. If we add a second option that is exclusively used by ICO, then sure.

image_type: ImageType,

bit_count: u16,
Expand Down Expand Up @@ -1123,6 +1124,7 @@ impl<R: BufRead + Seek> BmpDecoder<R> {
top_down: false,
no_file_header: false,
add_alpha_channel: false,
fallback_size: None,
image_type: ImageType::Palette,

bit_count: 0,
Expand Down Expand Up @@ -1218,9 +1220,14 @@ impl<R: BufRead + Seek> BmpDecoder<R> {
}

#[cfg(feature = "ico")]
pub(crate) fn new_with_ico_format(reader: R) -> ImageResult<BmpDecoder<R>> {
pub(crate) fn new_with_ico_format(
reader: R,
spec: SpecCompliance,
ico_size: (u16, u16),
) -> ImageResult<BmpDecoder<R>> {
let mut decoder = Self::new_decoder(reader);
decoder.read_metadata_in_ico_format()?;
decoder.spec_strictness = spec;
decoder.read_metadata_in_ico_format(ico_size)?;
Ok(decoder)
}

Expand Down Expand Up @@ -1389,7 +1396,15 @@ impl<R: BufRead + Seek> BmpDecoder<R> {
let mut buffer = [0u8; 36];
self.reader.read_exact(&mut buffer)?;

let parsed = ParsedInfoHeader::parse(&buffer, self.spec_strictness)?;
let mut parsed = ParsedInfoHeader::parse(&buffer, self.spec_strictness)?;

// we may have a fallback size if it's missing in the header (e.g. ICO decoding)
if let Some(fallback) = self.fallback_size {
if parsed.width == 0 || parsed.height == 0 {
parsed.width = i32::from(fallback.0);
parsed.height = i32::from(fallback.1);
}
}

self.width = parsed.width;
self.height = parsed.height;
Expand Down Expand Up @@ -1721,14 +1736,42 @@ impl<R: BufRead + Seek> BmpDecoder<R> {

#[cfg(feature = "ico")]
#[doc(hidden)]
pub fn read_metadata_in_ico_format(&mut self) -> ImageResult<()> {
pub fn read_metadata_in_ico_format(&mut self, ico_size: (u16, u16)) -> ImageResult<()> {
self.no_file_header = true;
self.add_alpha_channel = true;

// provide a fallback for invalid headers in lenient mode
if self.spec_strictness == SpecCompliance::Lenient {
self.fallback_size = Some((ico_size.0, ico_size.1 * 2));
}

self.read_metadata()?;

if self.spec_strictness == SpecCompliance::Strict && self.height % 2 == 1 {
return Err(ImageError::Decoding(DecodingError::new(
ImageFormat::Ico.into(),
"Invalid ICO BMP height: biHeight in BITMAPINFOHEADER must be even".to_owned(),
)));
}

// 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;

if self.width == 0 || self.height == 0 {
if self.spec_strictness == SpecCompliance::Strict {
return Err(ImageError::Decoding(DecodingError::new(
ImageFormat::Ico.into(),
"Invalid ICO BMP size: size cannot be zero".to_owned(),
)));
} else {
// In lenient mode, use the size specified in the ICON entry as a fallback.
// This behavior is consistent with Windows.
self.width = ico_size.0 as i32;
self.height = ico_size.1 as i32;
}
}

Ok(())
}

Expand Down
26 changes: 24 additions & 2 deletions src/codecs/ico/decoder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ impl<R: BufRead + Seek> IcoDecoder<R> {
let reader_offset = r.stream_position()?;
let entries = read_entries(&mut r, spec)?;
let entry = best_entry(entries)?;
let decoder = entry.decoder(r, reader_offset)?;
let decoder = entry.decoder(r, reader_offset, spec)?;

Ok(IcoDecoder {
selected_entry: entry,
Expand Down Expand Up @@ -267,14 +267,21 @@ impl DirEntry {
&self,
mut r: R,
reader_offset: u64,
spec: SpecCompliance,
) -> ImageResult<InnerDecoder<R>> {
let is_png = self.is_png(&mut r, reader_offset)?;
self.seek_to_start(&mut r, reader_offset)?;

if is_png {
Ok(Png(Box::new(PngDecoder::new(r))))
} else {
Ok(Bmp(BmpDecoder::new_with_ico_format(r)?))
Ok(Bmp(BmpDecoder::new_with_ico_format(
r,
spec,
// Certain invalid BMPs have their biWidth and/or biHeight set to 0.
// We pass in the dimensions from the ICON entry as a fallback in such cases.
(self.real_width(), self.real_height()),
)?))
}
}
}
Expand Down Expand Up @@ -644,4 +651,19 @@ mod test {
IcoDecoder::with_spec_compliance(std::io::Cursor::new(&data), SpecCompliance::Strict);
assert!(decoder_strict.is_err());
}

#[test]
fn size_fallback_on_lenient() {
let data = std::fs::read("tests/images/ico/images/bmp-biHeight=1.ico").unwrap();

let mut decoder =
IcoDecoder::with_spec_compliance(std::io::Cursor::new(&data), SpecCompliance::Lenient)
.unwrap();
let layout = decoder.prepare_image().unwrap().layout;
assert_eq!(layout.dimensions(), (1, 1));

let decoder_strict =
IcoDecoder::with_spec_compliance(std::io::Cursor::new(&data), SpecCompliance::Strict);
assert!(decoder_strict.is_err());
}
}
Binary file not shown.
Binary file added tests/images/ico/images/bmp-biHeight=1.ico
Binary file not shown.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added tests/reference/ico/images/bmp-biHeight=1.ico.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading