Merge pull request #1192 from dylanmtaylor/codex/effectcraft-shared-format-cutover

fix: bound GIF allocations and clarify WebP canvas limits

Cargo.lock

@@ -1622,6 +1622,7 @@ dependencies = [
 name = "craft-gif"
 version = "0.1.0"
 dependencies = [
+ "gif",
  "image",
 ]
 

crates/gif/Cargo.toml

@@ -19,4 +19,5 @@ image = { version = "0.25", default-features = false, features = ["gif"], option
 
 [dev-dependencies]
 # Writes the animated test files the scan is checked against; not a product dependency.
+gif = "0.14"
 image = { version = "0.25", default-features = false, features = ["gif"] }

crates/gif/src/frames.rs

@@ -22,13 +22,17 @@ pub struct FrameLimits {
     pub max_bytes: u64,
     /// Largest canvas (logical screen) in bytes at 4 bytes a pixel. Default 512 MiB.
     pub max_canvas_bytes: u64,
+    /// Combined decoder working-allocation budget. The decoder reserves its persistent canvas
+    /// before the frame buffer, so both count against this limit. `None` uses twice
+    /// `max_canvas_bytes`, matching the decoder's working-buffer allowance. Default `None`.
+    pub max_decode_bytes: Option<u64>,
     /// Merge a frame identical to the one before into it (its delay added). Default off.
     pub merge_identical: bool,
 }
 
 impl Default for FrameLimits {
     fn default() -> Self {
-        FrameLimits { max_frames: 100_000, max_bytes: 1 << 30, max_canvas_bytes: 512 << 20, merge_identical: false }
+        FrameLimits { max_frames: 100_000, max_bytes: 1 << 30, max_canvas_bytes: 512 << 20, max_decode_bytes: None, merge_identical: false }
     }
 }
 
@@ -93,6 +97,10 @@ pub fn read_frames(bytes: &[u8], limits: &FrameLimits) -> Result<Animation, Fram
     if canvas > limits.max_canvas_bytes {
         return Err(FrameError::LimitExceeded(format!("a {width}x{height} canvas is over {} bytes", limits.max_canvas_bytes)));
     }
+    let max_decode_bytes = limits.max_decode_bytes.unwrap_or_else(|| limits.max_canvas_bytes.saturating_mul(2));
+    if canvas > max_decode_bytes {
+        return Err(FrameError::LimitExceeded(format!("a {width}x{height} canvas is over the {max_decode_bytes} byte decode-allocation limit")));
+    }
     // The decoder sets up the whole logical screen before the first frame: the file must back it.
     if !crate::plausible(info.bytes_to_back(4, 1 << 28), bytes.len(), 1 << 14) {
         return Err(FrameError::LimitExceeded(format!("a {} byte file cannot hold a {width}x{height} picture", bytes.len())));
@@ -100,7 +108,7 @@ pub fn read_frames(bytes: &[u8], limits: &FrameLimits) -> Result<Animation, Fram
     let malformed = |e: image::ImageError| FrameError::Malformed(e.to_string());
     let mut decoder = GifDecoder::new(Cursor::new(bytes)).map_err(malformed)?;
     let mut il = image::Limits::default();
-    il.max_alloc = Some(limits.max_canvas_bytes.saturating_mul(2));
+    il.max_alloc = Some(max_decode_bytes);
     decoder.set_limits(il).map_err(malformed)?;
     let (width, height) = decoder.dimensions();
     collect(width, height, decoder.into_frames(), limits)
@@ -114,7 +122,14 @@ fn collect(width: u32, height: u32, frames: image::Frames<'_>, limits: &FrameLim
     for frame in frames {
         let frame = match frame {
             Ok(f) => f,
-            Err(e) if out.is_empty() => return Err(FrameError::Malformed(e.to_string())),
+            Err(e) if out.is_empty() => {
+                let message = e.to_string();
+                return Err(if matches!(e, image::ImageError::Limits(_)) {
+                    FrameError::LimitExceeded(message)
+                } else {
+                    FrameError::Malformed(message)
+                });
+            }
             Err(e) => {
                 stop = Stop::Damaged(e.to_string());
                 break;
@@ -150,6 +165,7 @@ fn collect(width: u32, height: u32, frames: image::Frames<'_>, limits: &FrameLim
 mod tests {
     #![allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)]
     use super::*;
+    use std::borrow::Cow;
 
     /// An animated GIF of solid 3 × 2 frames, written by `image`: (red level, delay in hundredths).
     fn gif(frames: &[(u8, u32)]) -> Vec<u8> {
@@ -176,6 +192,39 @@ mod tests {
         assert!(a.frames.iter().all(|f| f.rgba.len() == 24));
     }
 
+    #[test]
+    fn partial_frames_composite_and_background_disposal_is_applied() {
+        let palette = [0, 0, 0, 255, 0, 0, 0, 0, 255, 0, 255, 0];
+        let (mut bytes, first, second, third) = (Vec::new(), [1], [2], [0]);
+        {
+            let mut encoder = gif::Encoder::new(&mut bytes, 2, 1, &palette).unwrap();
+            let a = gif::Frame { width: 1, height: 1, buffer: Cow::Borrowed(&first), transparent: Some(0), delay: 2, ..gif::Frame::default() };
+            encoder.write_frame(&a).unwrap();
+
+            let b = gif::Frame {
+                left: 1,
+                width: 1,
+                height: 1,
+                buffer: Cow::Borrowed(&second),
+                transparent: Some(0),
+                dispose: gif::DisposalMethod::Background,
+                delay: 3,
+                ..gif::Frame::default()
+            };
+            encoder.write_frame(&b).unwrap();
+
+            let c = gif::Frame { width: 1, height: 1, buffer: Cow::Borrowed(&third), transparent: Some(0), delay: 4, ..gif::Frame::default() };
+            encoder.write_frame(&c).unwrap();
+        }
+
+        let a = read_frames(&bytes, &FrameLimits::default()).unwrap();
+        assert_eq!((a.width, a.height, a.stop), (2, 1, Stop::End));
+        assert_eq!(a.frames.iter().map(|f| f.delay_ms).collect::<Vec<_>>(), [20, 30, 40]);
+        assert_eq!(a.frames[0].rgba, [255, 0, 0, 255, 0, 0, 0, 0]);
+        assert_eq!(a.frames[1].rgba, [255, 0, 0, 255, 0, 0, 255, 255]);
+        assert_eq!(a.frames[2].rgba, [255, 0, 0, 255, 0, 0, 0, 0]);
+    }
+
     #[test]
     fn identical_frames_merge_when_asked() {
         let bytes = gif(&[(10, 5), (10, 7), (200, 1), (200, 1), (10, 2)]);
@@ -195,6 +244,22 @@ mod tests {
         assert_eq!((a.frames.len(), a.stop), (2, Stop::ByteLimit));
         assert!(matches!(read_frames(&bytes, &FrameLimits { max_canvas_bytes: 23, ..FrameLimits::default() }), Err(FrameError::LimitExceeded(_))));
         assert!(read_frames(&bytes, &FrameLimits { max_canvas_bytes: 24, ..FrameLimits::default() }).is_ok());
+        // Canvas and frame buffer each need 24 bytes; decoder allocations share this budget.
+        assert!(matches!(
+            read_frames(&bytes, &FrameLimits { max_decode_bytes: Some(24), ..FrameLimits::default() }),
+            Err(FrameError::LimitExceeded(_))
+        ));
+        assert!(read_frames(&bytes, &FrameLimits { max_decode_bytes: Some(1), ..FrameLimits::default() }).is_err());
+    }
+
+    #[test]
+    fn decoder_allocation_limit_is_not_reported_as_malformed_input() {
+        let mut bytes = gif(&[(1, 1)]);
+        // The logical screen is 1×1, while the frame data still decodes to 3×2. The canvas
+        // passes the eight-byte decode cap, but the decoder's frame allocation does not.
+        bytes[6..10].copy_from_slice(&[1, 0, 1, 0]);
+        let limits = FrameLimits { max_canvas_bytes: 4, max_decode_bytes: Some(8), ..FrameLimits::default() };
+        assert!(matches!(read_frames(&bytes, &limits), Err(FrameError::LimitExceeded(_))));
     }
 
     #[test]

crates/webp/src/frames.rs

@@ -24,7 +24,9 @@ pub struct FrameLimits {
     pub max_frames: usize,
     /// Most bytes of kept pictures (4 bytes a pixel each). Default 1 GiB.
     pub max_bytes: u64,
-    /// Largest canvas (logical screen) in bytes at 4 bytes a pixel. Default 512 MiB.
+    /// Largest canvas (logical screen) in bytes at 4 bytes a pixel. Default 512 MiB. This is a
+    /// pre-decode canvas check; image 0.25.10's WebP decoder does not enforce its working-buffer
+    /// allocations through the `ImageDecoder::set_limits` hook.
     pub max_canvas_bytes: u64,
     /// Merge a frame identical to the one before into it (its delay added). Default off.
     pub merge_identical: bool,
@@ -98,10 +100,7 @@ pub fn read_frames(bytes: &[u8], limits: &FrameLimits) -> Result<Animation, Fram
         return Err(FrameError::LimitExceeded(format!("a {}x{} canvas is over {} bytes", info.width, info.height, limits.max_canvas_bytes)));
     }
     let malformed = |e: image::ImageError| FrameError::Malformed(e.to_string());
-    let mut decoder = WebPDecoder::new(Cursor::new(bytes)).map_err(malformed)?;
-    let mut il = image::Limits::default();
-    il.max_alloc = Some(limits.max_canvas_bytes.saturating_mul(2));
-    decoder.set_limits(il).map_err(malformed)?;
+    let decoder = WebPDecoder::new(Cursor::new(bytes)).map_err(malformed)?;
     let (width, height) = decoder.dimensions();
     if !decoder.has_animation() {
         let mut rgba = vec![0u8; usize::try_from(canvas).map_err(|_| FrameError::LimitExceeded("canvas".into()))?];