sile / libflate

A Rust implementation of DEFLATE algorithm and related formats (ZLIB, GZIP)
https://docs.rs/libflate
MIT License
178 stars 35 forks source link

Remove single use of `unsafe`. #67

Closed mleonhard closed 2 years ago

mleonhard commented 2 years ago

Hi Takeru, Thanks for making the libflate library and maintaining it for 5.5 years.

I am building a modular HTTP server library. The library has forbid(unsafe_code). I carefully choose the dependencies of the library. I try to minimize the amount of unsafe code in dependencies.

I would like to use libflate in my library. I noticed that libflate contains a single use of unsafe. It calls unsafe std::ffi::CString::from_vec_unchecked to convert filenames and comments from Vec<u8> to CString:

https://github.com/sile/libflate/blob/9bf47f849023c515e711652d848beb535c0c4361/src/gzip.rs#L436

fn read_cstring<R>(mut reader: R) -> io::Result<CString>
where
    R: io::Read,
{
    let mut buf = Vec::new();
    loop {
        let mut cbuf = [0; 1];
        reader.read_exact(&mut cbuf)?;
        if cbuf[0] == 0 {
            return Ok(unsafe { CString::from_vec_unchecked(buf) });
        }
        buf.push(cbuf[0]);
    }
}

Can we please change it to use safe std::ffi::CString::new?

The new method iterates over the bytes and checks that there is no 0. Since the code just iterated over the bytes, they will be in the processor's cache. Therefore, I expect that the performance difference between using from_vec_unchecked and using new is unmeasurably small.

The compiler's guarantee that libflate cannot generate undefined behavior is extremely valuable to me. I hope you will agree to this change.

Sincerely, Michael

sile commented 2 years ago

Thank you for your PR. This change looks nice 👍

I am building a modular HTTP server library. ... I try to minimize the amount of unsafe code in dependencies.

It sounds interesting and I'm very glad to hear that you consider using inflate in your project.