collective
Browse / github.com/storytold/pdfcraft / patch
fix(xfa, create): read a BMP's pixels from after its headers when its offset points into them
BMP images whose header has bfOffBits = 0 are drawn shifted and wrapped around PdfCraft 0.14.0, macOS 26.5.2 (Apple silicon). Same code on `main` (1dd2737). ## What happens A BMP whose file header has `bfOffBits = 0` is read from the start of the file instead of from after its headers. The picture comes out shifted sideways and wrapped around, with a thin vertical seam. Acrobat draws the same picture intact. This happens everywhere PdfCraft reads BMPs (see below). Some BMP writers leave bfOffBits at 0. I hit it on the logo of a dynamic XFA form from the Romanian tax authority (ANAF), made with LiveCycle Designer ES 10.0. There every row starts 1078 pixels early, out o
- Status
- open
- Score
- 0.00
- Adopters
- 0 other operators
- Author
- KenM76 (imported from pull request #4943, open)
- Base
1dd2737b47a9c1c9562f0af563053d1bf510e4bf- Size
- +139 −3 in 6 files
- Review
- pending
- Shared
- 2026-10-28
- Id
44bf94784e1eac4400d885da
How it went
No other operator has reported using it yet.
Diff
raw · Cargo.lock crates/create/Cargo.toml crates/create/src/lib.rs crates/create/src/tests.rs crates/xfa/src/image.rs crates/xfa/src/tests.rs
@@ -4187,6 +4187,7 @@ dependencies = [ "pdfcraft-cos", "pdfcraft-fonts", "pdfcraft-model",+ "pdfcraft-xfa", "png 0.18.1", "thiserror 2.0.21", "tiff",@@ -12,6 +12,8 @@ workspace = true pdfcraft-cos = { workspace = true } pdfcraft-model = { workspace = true } pdfcraft-fonts = { workspace = true }+# For `image::bmp_with_pixel_offset`, shared with XFA pictures.+pdfcraft-xfa = { workspace = true } png = "0.18" tiff = "0.11" image = { version = "0.25", default-features = false, features = ["bmp", "gif", "jpeg"] }@@ -562,7 +562,7 @@ fn embed(name: &str, bytes: &[u8], wanted: usize) -> Result<Vec<Embedded>, Creat } else if is_tiff(bytes) { tiff_pages(name, bytes, wanted) } else if bytes.starts_with(b"BM") {- Ok(vec![decoded(name, bytes, image::ImageFormat::Bmp)?])+ Ok(vec![decoded(name, &pdfcraft_xfa::image::bmp_with_pixel_offset(bytes), image::ImageFormat::Bmp)?]) } else if is_gif(bytes) { Ok(vec![decoded(name, bytes, image::ImageFormat::Gif)?]) } else if bytes.starts_with(JP2_SIGNATURE) || bytes.starts_with(&[0xFF, 0x4F, 0xFF, 0x51]) {@@ -206,6 +206,26 @@ fn bmp_gif_and_multi_page_tiff_images() { assert!(matches!(from_images(&[("x.webp".into(), b"RIFF0000WEBP".to_vec())]), Err(CreateError::Image(..)))); } +/// #4937: a BMP whose header gives the pixel data's offset as 0 is placed as the same image as+/// the intact file, not shifted by the header bytes.+#[test]+fn a_bmp_with_a_zero_pixel_offset_is_placed_like_the_intact_file() {+ use image::ImageEncoder;+ let mut bmp = Vec::new();+ let pixels: Vec<u8> = (0..18).map(|i| i * 13).collect();+ image::codecs::bmp::BmpEncoder::new(&mut bmp).write_image(&pixels, 3, 2, image::ExtendedColorType::Rgb8).unwrap();+ let mut zero = bmp.clone();+ zero[10..14].copy_from_slice(&0u32.to_le_bytes());+ let samples = |data: Vec<u8>| {+ let doc = reopen(&from_images(&[("a.bmp".into(), data)]).unwrap());+ let res = doc.resolve(pages(&doc)[0].get(b"Resources").unwrap()).as_dict().cloned().unwrap();+ let xo = doc.resolve(res.get(b"XObject").unwrap()).as_dict().cloned().unwrap();+ let Object::Stream(s) = &*doc.resolve(xo.get(b"Im0").unwrap()) else { panic!("not an image") };+ s.decoded().unwrap()+ };+ assert_eq!(samples(zero), samples(bmp));+}+ /// #665 lifted the tiff crate's 256 MiB decode limit; a few bytes claiming a 60000x60000 page /// (3.6 GB of gray pixels) must still be refused, not allocated. #[test]@@ -41,6 +41,62 @@ fn is_bmp(data: &[u8]) -> bool { && data.get(14..18).is_some_and(|b| matches!(u32::from_le_bytes([b[0], b[1], b[2], b[3]]), 12 | 16 | 40 | 52 | 56 | 64 | 108 | 124)) } +/// `data` with a BMP's pixel-data offset (`bfOffBits`) corrected when it points into the file or+/// info header or at its end with masks or a colour table still to come, as some writers leave it at 0: the rows then start after the headers, the colour+/// masks and the colour table. The `image` decoder seeks to the declared offset unchecked, so+/// such a picture would be read from the wrong place, shifted and wrapped. Anything else,+/// including a header too short to measure, is returned as it is.+pub fn bmp_with_pixel_offset(data: &[u8]) -> std::borrow::Cow<'_, [u8]> {+ match bmp_pixel_offset_repair(data) {+ Some(offset) => {+ let mut fixed = data.to_vec();+ if let Some(field) = fixed.get_mut(10..14) {+ field.copy_from_slice(&offset.to_le_bytes());+ }+ std::borrow::Cow::Owned(fixed)+ }+ None => std::borrow::Cow::Borrowed(data),+ }+}++/// Where a BMP's rows start when its declared offset lies inside its headers, else `None`.+fn bmp_pixel_offset_repair(data: &[u8]) -> Option<u32> {+ let le16 = |at: usize| data.get(at..at + 2).map(|b| u32::from(u16::from_le_bytes([b[0], b[1]])));+ let le32 = |at: usize| data.get(at..at + 4).map(|b| u32::from_le_bytes([b[0], b[1], b[2], b[3]]));+ if !is_bmp(data) {+ return None;+ }+ let (declared, info_size) = (le32(10)?, le32(14)?);+ let headers_end = info_size.checked_add(14)?;+ // At the end of the headers is right only when no masks or colour table follow.+ if declared > headers_end {+ return None;+ }+ let (bpp, masks, used, entry) = if info_size == 12 {+ // BITMAPCOREHEADER: 16-bit fields, no colour count, 3-byte table entries.+ (le16(24)?, 0, 0, 3)+ } else if info_size < 40 {+ // OS/2's 16-byte header ends before the colour count; leave it to the decoder.+ return None;+ } else {+ // BI_BITFIELDS and BI_ALPHABITFIELDS put their masks after a 40-byte header; the larger+ // headers hold them inside.+ let masks = match (info_size, le32(30)?) {+ (40, 3) => 12,+ (40, 6) => 16,+ _ => 0,+ };+ (le16(28)?, masks, le32(46)?, 4)+ };+ let entries = match (used, bpp) {+ (0, 1..=8) => 1 << bpp,+ (0, _) => 0,+ (n, _) => n,+ };+ let offset = entries.checked_mul(entry)?.checked_add(masks)?.checked_add(headers_end)?;+ (offset != declared && usize::try_from(offset).ok()? <= data.len()).then_some(offset)+}+ /// The pixel size, from the header alone; `None` when it can't be read or the image is bigger /// than can be decoded (so layout gives such a picture its box, not a page-high strip). pub fn size(data: &[u8]) -> Option<(u32, u32)> {@@ -70,7 +126,7 @@ fn header_size(data: &[u8]) -> Option<(u32, u32)> { (w > 0 && h > 0).then_some((w, h)) } // The decoders read only the headers (TIFF: the first IFD) until asked for pixels.- Kind::Bmp => reader(data, image::ImageFormat::Bmp).into_dimensions().ok().filter(|&(w, h)| w > 0 && h > 0),+ Kind::Bmp => reader(&bmp_with_pixel_offset(data), image::ImageFormat::Bmp).into_dimensions().ok().filter(|&(w, h)| w > 0 && h > 0), Kind::Tiff => { use image::{ImageDecoder, metadata::Orientation}; let mut decoder = reader(data, image::ImageFormat::Tiff).into_decoder().ok()?;@@ -174,7 +230,8 @@ pub fn decode(data: &[u8]) -> Result<Decoded, String> { Kind::Bmp => image::ImageFormat::Bmp, _ => image::ImageFormat::Tiff, };- let img = if k == Kind::Tiff { decode_tiff(data) } else { reader(data, format).decode() }+ let data = if k == Kind::Bmp { bmp_with_pixel_offset(data) } else { std::borrow::Cow::Borrowed(data) };+ let img = if k == Kind::Tiff { decode_tiff(&data) } else { reader(&data, format).decode() } .map_err(|e| if matches!(e, image::ImageError::Limits(_)) { too_big() } else { bad(e.to_string()) })?; let rgba = img.into_rgba8(); let (width, height) = rgba.dimensions();@@ -1794,6 +1794,62 @@ fn pictures_are_embedded_once_across_relayouts_and_bad_data_pictures_are_reporte assert!(find_image(&t.root.children).is_some_and(|n| n > 1_200_000)); } +/// #4937: LiveCycle wrote BMPs whose header gives the pixel data's offset as 0. Read literally,+/// the pixels start at the file header, so the image is drawn shifted and wrapped around.+#[test]+fn a_bmp_whose_pixel_offset_points_into_its_headers_is_read_from_after_them() {+ use crate::image::{bmp_with_pixel_offset, decode, size};+ for palette in [false, true] {+ let intact = tiny_bmp(palette);+ assert!(matches!(bmp_with_pixel_offset(&intact), std::borrow::Cow::Borrowed(_)), "an intact file is not copied");+ for bad in [0u32, 14, 53] {+ let mut bmp = intact.clone();+ bmp[10..14].copy_from_slice(&bad.to_le_bytes());+ let d = decode(&bmp).unwrap();+ assert_eq!(d.samples, vec![255, 0, 0, 0, 0, 255], "offset {bad}, palette: {palette}");+ assert_eq!(size(&bmp), Some((2, 1)));+ assert_eq!(bmp_with_pixel_offset(&bmp).as_ref(), &intact[..], "offset {bad}, palette: {palette}");+ }+ }+ // Too short to hold the pixels after the headers: left alone, not grown.+ let mut cut = tiny_bmp(true);+ cut[10..14].copy_from_slice(&0u32.to_le_bytes());+ cut.truncate(60);+ assert_eq!(bmp_with_pixel_offset(&cut).as_ref(), &cut[..]);+ // A truncated header, and bytes that are not a BMP: left alone.+ assert_eq!(bmp_with_pixel_offset(b"BM ").as_ref(), b"BM ");+ assert_eq!(bmp_with_pixel_offset(TINY_GIF).as_ref(), TINY_GIF);+ // BI_BITFIELDS in a 40-byte header: three masks follow the header.+ let mut fields = tiny_bmp(false);+ fields[30..34].copy_from_slice(&3u32.to_le_bytes());+ fields[10..14].copy_from_slice(&0u32.to_le_bytes());+ fields.splice(54..54, [0u8; 12]);+ assert_eq!(bmp_with_pixel_offset(&fields).get(10..14), Some(&66u32.to_le_bytes()[..]));+ // Pointing at the end of the 40-byte header, before the masks.+ fields[10..14].copy_from_slice(&54u32.to_le_bytes());+ assert_eq!(bmp_with_pixel_offset(&fields).get(10..14), Some(&66u32.to_le_bytes()[..]));+ // A colour count no file could hold: left alone.+ let mut huge = tiny_bmp(true);+ huge[46..50].copy_from_slice(&u32::MAX.to_le_bytes());+ huge[10..14].copy_from_slice(&0u32.to_le_bytes());+ assert_eq!(bmp_with_pixel_offset(&huge).as_ref(), &huge[..]);+ // A 12-byte core header: 16-bit fields and 3-byte palette entries, `1 << bpp` of them.+ let mut core = b"BM".to_vec();+ core.extend_from_slice(&[0; 8]);+ core.extend_from_slice(&0u32.to_le_bytes());+ core.extend_from_slice(&12u32.to_le_bytes());+ for v in [2u16, 1, 1, 1] {+ core.extend_from_slice(&v.to_le_bytes());+ }+ core.extend_from_slice(&[0, 0, 255, 255, 0, 0, 0b0100_0000, 0, 0, 0]);+ assert_eq!(bmp_with_pixel_offset(&core).get(10..14), Some(&32u32.to_le_bytes()[..]));+ let mut at_table = core.clone();+ at_table[10..14].copy_from_slice(&26u32.to_le_bytes());+ assert_eq!(bmp_with_pixel_offset(&at_table).get(10..14), Some(&32u32.to_le_bytes()[..]), "before the colour table");+ let d = decode(&bmp_with_pixel_offset(&core)).unwrap();+ assert_eq!(d.samples, vec![255, 0, 0, 0, 0, 255], "core header");+}+ /// Designer forms often carry BMP and TIFF pictures, declared as `image/bmp` and `image/tif` /// (the Government of Canada's IMM 5257 and IMM 5710 have both). They were left out. #[test]