ZlibDecoder accepts invalid content and does not error out

[robisonsantos]

I am writing an application that needs to read a zlib stream of bytes and it should only succeed once the whole stream of valid bytes is received. Every time a new byte is read from the stream, I append to a vector of bytes an call something like this:

        let mut z = ZlibDecoder::new(&bytes[..]);
        let mut s = String::new();
        z.read_to_string(&mut s)?;

I was expecting the read_to_string call to keep failing until the whole valid stream is received, but ZlibDecoder seems to always succeed when the bytes are in some valid range.

Example:

    #[test]
    pub fn test_invalid_input() {
        let bytes:Vec<u8> = vec![77];
        let mut z = ZlibDecoder::new(&bytes[..]);
        let mut s = String::new();
        let result = z.read_to_string(&mut s);
        assert!(result.is_err());
    }

    #[test]
    pub fn test_invalid_input() {
        let bytes:Vec<u8> = vec![1,1,1,1];
        let mut z = ZlibDecoder::new(&bytes[..]);
        let mut s = String::new();
        let result = z.read_to_string(&mut s);
        assert!(result.is_err());
    }

The first test fails, while the second succeed. I was expecting both tests to fail since both inputs are not valid zlib data.

[alexcrichton]

Can you clarify where ZlibDecoder is coming from and how you're depending on this crate? I added those tests to this repository and both passed.

[robisonsantos]

I am using

[dependencies]
flate2 = { version = "1.0.17", features = ["zlib"], default-features = false }

[robisonsantos]

Sorry, GitHub on mobile is sh**

I am using this

[dependencies]
flate2 = { version = "1.0.17", features = ["zlib"], default-features = false }

As described in the docs.

[alexcrichton]

Ah ok it looks like this is related to the zlib feature, the miniz feature or the rust_backend port to Rust doesn't fail these tests.

I believe this patch at least fixes the read half, but the write half is more complicated which I'd have to dig in more to fix. I'm also not entirely sure if this is the right fix, I never was certain about handling the "buf error" status.

diff --git a/src/zio.rs b/src/zio.rs
index 5028188..4a04565 100644
--- a/src/zio.rs
+++ b/src/zio.rs
@@ -144,7 +144,29 @@ where
             // return that 0 bytes of data have been read then it will
             // be interpreted as EOF.
             Ok(Status::Ok) | Ok(Status::BufError) if read == 0 && !eof && dst.len() > 0 => continue,
-            Ok(Status::Ok) | Ok(Status::BufError) | Ok(Status::StreamEnd) => return Ok(read),
+
+            // Otherwise if we got OK or we're at the stream end, then report
+            // how much was read.
+            Ok(Status::Ok) | Ok(Status::StreamEnd) => return Ok(read),
+
+            // If a buffer error is flagged then this could be either normal or
+            // abnormal. If any data was read it simply means we need some more
+            // space to make progress, so report that data was read so the
+            // application can free up some space.
+            //
+            // If nothing was read, however, and we'll never be getting any
+            // more data into the input buffer, then that means this is a
+            // corrupst stream.
+            Ok(Status::BufError) => {
+                return if read == 0 && eof && dst.len() > 0 {
+                    Err(io::Error::new(
+                        io::ErrorKind::InvalidInput,
+                        "corrupt deflate stream",
+                    ))
+                } else {
+                    Ok(read)
+                };
+            }
 
             Err(..) => {
                 return Err(io::Error::new(
diff --git a/src/zlib/mod.rs b/src/zlib/mod.rs
index 1813a4e..83dc203 100644
--- a/src/zlib/mod.rs
+++ b/src/zlib/mod.rs
@@ -156,4 +156,20 @@ mod tests {
             v == w.finish().unwrap().finish().unwrap()
         }
     }
+
+    #[test]
+    fn invalid_is_invalid() {
+        let mut s = Vec::new();
+        let result = read::ZlibDecoder::new(&[77][..]).read_to_end(&mut s);
+        assert!(result.is_err());
+
+        let mut s = Vec::new();
+        let result = read::ZlibDecoder::new(&[1, 1, 1][..]).read_to_end(&mut s);
+        assert!(result.is_err());
+
+        let mut z = write::ZlibDecoder::new(Vec::new());
+        assert!(z.write_all(&[77]).is_err() || z.finish().is_err());
+        let mut z = write::ZlibDecoder::new(Vec::new());
+        assert!(z.write_all(&[1, 1, 1]).is_err() || z.finish().is_err());
+    }
 }

[robisonsantos]

I am trying to port this application form ruby to rust (for performance gains): https://github.com/robisonsantos/packfile_reader If you need some inspiration, this is what I expect to get from the library: https://github.com/robisonsantos/packfile_reader/blob/main/lib/packfile_reader/packfile_entry.rb#L52-L76 I also tested "compress" rust crate, but it appears it "succeed" inflating the stream too early too, about 4 bytes too early. I will try using the rust-backend version of flaten2 and see what I get from it.

…

On Wed, Dec 16, 2020 at 7:51 AM Alex Crichton ***@***.***> wrote: Ah ok it looks like this is related to the zlib feature, the miniz feature or the rust_backend port to Rust doesn't fail these tests. I believe this patch at least fixes the read half, but the write half is more complicated which I'd have to dig in more to fix. I'm also not entirely sure if this is the right fix, I never was certain about handling the "buf error" status. diff --git a/src/zio.rs b/src/zio.rs index 5028188..4a04565 100644--- a/src/zio.rs+++ b/src/zio.rs@@ -144,7 +144,29 @@ where // return that 0 bytes of data have been read then it will // be interpreted as EOF. Ok(Status::Ok) | Ok(Status::BufError) if read == 0 && !eof && dst.len() > 0 => continue,- Ok(Status::Ok) | Ok(Status::BufError) | Ok(Status::StreamEnd) => return Ok(read),++ // Otherwise if we got OK or we're at the stream end, then report+ // how much was read.+ Ok(Status::Ok) | Ok(Status::StreamEnd) => return Ok(read),++ // If a buffer error is flagged then this could be either normal or+ // abnormal. If any data was read it simply means we need some more+ // space to make progress, so report that data was read so the+ // application can free up some space.+ //+ // If nothing was read, however, and we'll never be getting any+ // more data into the input buffer, then that means this is a+ // corrupst stream.+ Ok(Status::BufError) => {+ return if read == 0 && eof && dst.len() > 0 {+ Err(io::Error::new(+ io::ErrorKind::InvalidInput,+ "corrupt deflate stream",+ ))+ } else {+ Ok(read)+ };+ } Err(..) => { return Err(io::Error::new(diff --git a/src/zlib/mod.rs b/src/zlib/mod.rs index 1813a4e..83dc203 100644--- a/src/zlib/mod.rs+++ b/src/zlib/mod.rs@@ -156,4 +156,20 @@ mod tests { v == w.finish().unwrap().finish().unwrap() } }++ #[test]+ fn invalid_is_invalid() {+ let mut s = Vec::new();+ let result = read::ZlibDecoder::new(&[77][..]).read_to_end(&mut s);+ assert!(result.is_err());++ let mut s = Vec::new();+ let result = read::ZlibDecoder::new(&[1, 1, 1][..]).read_to_end(&mut s);+ assert!(result.is_err());++ let mut z = write::ZlibDecoder::new(Vec::new());+ assert!(z.write_all(&[77]).is_err() || z.finish().is_err());+ let mut z = write::ZlibDecoder::new(Vec::new());+ assert!(z.write_all(&[1, 1, 1]).is_err() || z.finish().is_err());+ } } — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#258 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAA66LNZOVHKDMAKWCCSN63SVDJPJANCNFSM4U3Q7UCA> .

-- Robison W R Santos