BrotliDecompress panics if output writer stops accepting data

[ctz]

Reproducer:

use std::io::Cursor;

fn main() {
    // this is a valid compression of [0u8; 2048];
    let compression = [27, 255, 7, 0, 36, 0, 194, 177, 64, 114, 7];
    // output buffer doesn't have enough space
    let mut output_buffer = [0u8; 2047];

    brotli::BrotliDecompress(
        &mut Cursor::new(compression),
        &mut Cursor::new(&mut output_buffer[..]),
    )
    .unwrap_err();
}

This panics for me with:

thread 'main' panicked at /home/jbp/.cargo/registry/src/index.crates.io-6f17d22bba15001f/brotli-decompressor-4.0.0/src/lib.rs:284:13:
assertion `left == right` failed
  left: true
 right: false
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

I think there's a confusion there about what io::Write::write is allowed to return -- Ok(0) is valid, and in this case should be be promoted to an error.

[danielrh]

Is there an analogous bug in the compressor, or did fixing the decompressor also resolve this issue?

[ctz]

Apologies, I put this issue in the wrong repo. Thanks for taking the decompressor fix and making a release.

But for the sake of completeness, here's a test with a similar case on the compressor:

diff --git a/src/enc/test.rs b/src/enc/test.rs
index fff4856..2c56f89 100644
--- a/src/enc/test.rs
+++ b/src/enc/test.rs
@@ -549,6 +549,27 @@ fn test_roundtrip_empty() {
     assert_eq!(output_offset, 0);
     assert_eq!(compressed_offset, compressed.len());
 }
+
+#[cfg(feature="std")]
+#[test]
+fn test_compress_into_short_buffer() {
+    use std::io::{Cursor, Write};
+
+    // this plaintext should compress to 11 bytes
+    let plaintext = [0u8; 2048];
+
+    // but we only provide space for 10
+    let mut output_buffer = [0u8; 10];
+    let mut output_cursor = Cursor::new(&mut output_buffer[..]);
+
+    let mut w = crate::CompressorWriter::new(&mut output_cursor,
+                                         4096, 4, 22);
+    w.write(&plaintext).unwrap_err();
+    w.into_inner();
+
+    println!("{output_buffer:?}");
+}
+
 /*

This test unfortunately does not terminate, as it is making no progress in

rust-brotli/src/enc/writer.rs

Lines 131 to 142 in 37d403b

	pub fn write_all<ErrType, W: CustomWrite<ErrType>>(
	writer: &mut W,
	mut buf: &[u8],
	) -> Result<(), ErrType> {
	while !buf.is_empty() {
	match writer.write(buf) {
	Ok(bytes_written) => buf = &buf[bytes_written..],
	Err(e) => return Err(e),
	}
	}
	Ok(())
	}

And fixing that seems tricky without a breaking change. Ideally a parameter would be added to this function for the ErrType value to return on seeing Ok(0). For std callers that would be std::io::ErrorKind::WriteZero.into().

In our use case we compress into a Cursor over a Vec, so won't be affected by this. However happy to put together a PR that:

• documents the shortcoming of pub fn write_all, marks it deprecated, but leaves it as-is
• introduces pub fn write_all_until_eof (naming up for grabs)
• moves the internal callers of write_all to write_all_until_eof
• adds this test

[danielrh]

I would really appreciate that PR...

this might warrant a breaking change in the future, but maybe we could do a non-breaking release first with your PR and then add that WriteZero capability into the next major version on its heels?

[danielrh]

I think the recent commit has improved the situation--but you're right it's a breaking change--moved version to 7.0.0