Merge pull request #2764 from telecos/feature/lenient-validation

Allow lenient validation in BMP decoder

Author: 197g

src/codecs/bmp/decoder.rs

@@ -11,6 +11,18 @@ use crate::error::{
 };
 use crate::{ImageDecoder, ImageFormat};
 
+/// Controls how strictly the BMP decoder adheres to the specification.
+#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)]
+pub(crate) enum BmpSpec {
+    /// Strictly follow the BMP specification.
+    /// Rejects files that violate spec constraints (e.g., RLE with top-down).
+    Strict,
+    /// Allow some non-conformant files that violate some spec constraints
+    /// but still can be decoded at best effort.
+    #[default]
+    Lenient,
+}
+
 const BITMAPCOREHEADER_SIZE: u32 = 12;
 const BITMAPINFOHEADER_SIZE: u32 = 40;
 const BITMAPV2HEADER_SIZE: u32 = 52;
@@ -116,12 +128,12 @@ struct ParsedCoreHeader {
 
 impl ParsedCoreHeader {
     /// Parse BITMAPCOREHEADER fields from an 8-byte buffer.
-    fn parse(buffer: &[u8; 8]) -> ImageResult<Self> {
+    fn parse(buffer: &[u8; 8], spec_strictness: BmpSpec) -> ImageResult<Self> {
         let width = i32::from(u16::from_le_bytes(buffer[0..2].try_into().unwrap()));
         let height = i32::from(u16::from_le_bytes(buffer[2..4].try_into().unwrap()));
 
         let planes = u16::from_le_bytes(buffer[4..6].try_into().unwrap());
-        if planes != 1 {
+        if spec_strictness == BmpSpec::Strict && planes != 1 {
             return Err(DecoderError::MoreThanOnePlane.into());
         }
 
@@ -157,7 +169,7 @@ struct ParsedInfoHeader {
 
 impl ParsedInfoHeader {
     /// Parse BITMAPINFOHEADER fields from a 36-byte buffer.
-    fn parse(buffer: &[u8; 36]) -> ImageResult<Self> {
+    fn parse(buffer: &[u8; 36], spec_strictness: BmpSpec) -> ImageResult<Self> {
         let width = i32::from_le_bytes(buffer[0..4].try_into().unwrap());
         let mut height = i32::from_le_bytes(buffer[4..8].try_into().unwrap());
 
@@ -181,15 +193,21 @@ impl ParsedInfoHeader {
         };
 
         let planes = u16::from_le_bytes(buffer[8..10].try_into().unwrap());
-        if planes != 1 {
+        if spec_strictness == BmpSpec::Strict && planes != 1 {
             return Err(DecoderError::MoreThanOnePlane.into());
         }
 
         let bit_count = u16::from_le_bytes(buffer[10..12].try_into().unwrap());
         let compression = u32::from_le_bytes(buffer[12..16].try_into().unwrap());
 
-        // Top-down DIBs cannot be compressed
-        if top_down && compression != BI_RGB && compression != BI_BITFIELDS {
+        // Top-down DIBs cannot be compressed (per BMP specification).
+        // In lenient mode, we allow this for compatibility with other decoders.
+        if spec_strictness == BmpSpec::Strict
+            && top_down
+            && compression != BI_RGB
+            && compression != BI_BITFIELDS
+            && compression != BI_ALPHABITFIELDS
+        {
             return Err(DecoderError::ImageTypeInvalidForTopDown(compression).into());
         }
 
@@ -780,6 +798,7 @@ impl Bitfield {
     fn read(&self, data: u32) -> u8 {
         let data = data >> self.shift;
         match self.len {
+            0 => 0,
             1 => ((data & 0b1) * 0xff) as u8,
             2 => ((data & 0b11) * 0x55) as u8,
             3 => LOOKUP_TABLE_3_BIT_TO_8_BIT[(data & 0b00_0111) as usize],
@@ -808,14 +827,19 @@ impl Bitfields {
         b_mask: u32,
         a_mask: u32,
         max_len: u32,
+        spec_strictness: BmpSpec,
     ) -> ImageResult<Bitfields> {
         let bitfields = Bitfields {
             r: Bitfield::from_mask(r_mask, max_len)?,
             g: Bitfield::from_mask(g_mask, max_len)?,
             b: Bitfield::from_mask(b_mask, max_len)?,
             a: Bitfield::from_mask(a_mask, max_len)?,
         };
-        if bitfields.r.len == 0 || bitfields.g.len == 0 || bitfields.b.len == 0 {
+        // In strict mode, all RGB channels must have non-zero masks.
+        // In lenient mode, allow zero masks (the channel will read as 0).
+        if spec_strictness == BmpSpec::Strict
+            && (bitfields.r.len == 0 || bitfields.g.len == 0 || bitfields.b.len == 0)
+        {
             return Err(DecoderError::BitfieldMaskMissing(max_len).into());
         }
         Ok(bitfields)
@@ -901,6 +925,7 @@ pub struct BmpDecoder<R> {
     palette: Option<Vec<[u8; 3]>>,
     bitfields: Option<Bitfields>,
     icc_profile: Option<Vec<u8>>,
+    spec_strictness: BmpSpec,
 
     /// Current decoder state for resumable decoding.
     state: DecoderState,
@@ -935,7 +960,7 @@ impl<R: BufRead + Seek> BmpDecoder<R> {
             palette: None,
             bitfields: None,
             icc_profile: None,
-
+            spec_strictness: BmpSpec::default(),
             state: DecoderState::default(),
         }
     }
@@ -1120,7 +1145,7 @@ impl<R: BufRead + Seek> BmpDecoder<R> {
         let mut buffer = [0u8; 8];
         self.reader.read_exact(&mut buffer)?;
 
-        let parsed = ParsedCoreHeader::parse(&buffer)?;
+        let parsed = ParsedCoreHeader::parse(&buffer, self.spec_strictness)?;
 
         self.width = parsed.width;
         self.height = parsed.height;
@@ -1140,7 +1165,8 @@ impl<R: BufRead + Seek> BmpDecoder<R> {
         // Info header (after size field): 36 bytes minimum
         let mut buffer = [0u8; 36];
         self.reader.read_exact(&mut buffer)?;
-        let parsed = ParsedInfoHeader::parse(&buffer)?;
+
+        let parsed = ParsedInfoHeader::parse(&buffer, self.spec_strictness)?;
 
         self.width = parsed.width;
         self.height = parsed.height;
@@ -1193,6 +1219,7 @@ impl<R: BufRead + Seek> BmpDecoder<R> {
                     parsed.b_mask,
                     parsed.a_mask,
                     max_len,
+                    self.spec_strictness,
                 )?)
             }
             _ => None,
@@ -1431,14 +1458,17 @@ impl<R: BufRead + Seek> BmpDecoder<R> {
         match self.colors_used {
             0 => Ok(1 << self.bit_count),
             _ => {
-                if self.colors_used > 1 << self.bit_count {
+                if self.spec_strictness == BmpSpec::Strict && self.colors_used > 1 << self.bit_count
+                {
                     return Err(DecoderError::PaletteSizeExceeded {
                         colors_used: self.colors_used,
                         bit_count: self.bit_count,
                     }
                     .into());
                 }
-                Ok(self.colors_used as usize)
+                // In lenient mode, clamp to max palette size for the bit depth
+                let max_size = 1usize << self.bit_count;
+                Ok((self.colors_used as usize).min(max_size))
             }
         }
     }
@@ -2563,4 +2593,62 @@ mod test {
             assert_eq!(buf, ref_buf, "{path}: decoded data mismatch");
         }
     }
+
+    /// Test that BMP files with known spec violations are accepted by the
+    /// decoder (which defaults to lenient mode), and that strict mode still
+    /// detects the violations internally.
+    ///
+    /// These files come from the Chromium BMP test suite ("bad/" category):
+    /// - `rletopdown`: RLE compression with top-down orientation (spec forbids this)
+    /// - `badplanes`: planes field != 1 (spec requires exactly 1)
+    /// - `badpalettesize`: colors_used exceeds max for the bit depth
+    /// - `pal8oversizepal`: 8-bit palette with colors_used=300 (max is 256)
+    /// - `rgb16-880`: 16-bit bitfields with 8-8-0 channel widths (blue mask is zero)
+    #[test]
+    fn test_strict_vs_lenient_spec_validation() {
+        let questionable_files = [
+            (
+                "tests/images/bmp/images/lenient/rletopdown.bmp",
+                "rletopdown: RLE with top-down should be rejected in strict mode",
+            ),
+            (
+                "tests/images/bmp/images/lenient/badplanes.bmp",
+                "badplanes: planes != 1 should be rejected in strict mode",
+            ),
+            (
+                "tests/images/bmp/images/lenient/badpalettesize.bmp",
+                "badpalettesize: palette size exceeding bit depth should be rejected in strict mode",
+            ),
+            (
+                "tests/images/bmp/images/lenient/pal8oversizepal.bmp",
+                "pal8oversizepal: colors_used=300 exceeds max 256 for 8-bit",
+            ),
+            (
+                "tests/images/bmp/images/lenient/rgb16-880.bmp",
+                "rgb16-880: zero blue mask should be rejected in strict mode",
+            ),
+        ];
+
+        for (path, description) in &questionable_files {
+            let data = std::fs::read(path)
+                .unwrap_or_else(|e| panic!("{description}: failed to read {path}: {e}"));
+
+            // Default (lenient) mode: these files should be accepted
+            let decoder = BmpDecoder::new(Cursor::new(&data)).unwrap_or_else(|e| {
+                panic!("{description}: decoding failed: {e:?}");
+            });
+            let mut buf = vec![0u8; decoder.total_bytes() as usize];
+            decoder.read_image(buf.as_mut_slice()).unwrap_or_else(|e| {
+                panic!("{description}: read_image failed: {e:?}");
+            });
+
+            // Strict mode (internal): these files should be rejected
+            let mut strict_decoder = BmpDecoder::new_resumable(Cursor::new(&data));
+            strict_decoder.spec_strictness = BmpSpec::Strict;
+            assert!(
+                strict_decoder.read_metadata().is_err(),
+                "{description}: expected error in strict mode, but got Ok"
+            );
+        }
+    }
 }

tests/images/bmp/images/lenient/badpalettesize.bmp

@@ -94,7 +94,8 @@ fn bad_bmps() {
         // Manually reading the file so we can use load() instead of open()
         // We have to use load() so we can override the format
         let im_file = BufReader::new(File::open(path).unwrap());
-        let im = image::load(im_file, image::ImageFormat::Bmp);
+        let im: Result<image::DynamicImage, image::ImageError> =
+            image::load(im_file, image::ImageFormat::Bmp);
         assert!(im.is_err());
     }
 }