Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove single use of unsafe. #67

Merged
merged 1 commit into from
Mar 25, 2022
Merged

Remove single use of unsafe. #67

merged 1 commit into from
Mar 25, 2022

Conversation

mleonhard
Copy link
Contributor

@mleonhard mleonhard commented Mar 24, 2022

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:

fn read_cstring<R>(mut reader: R) -> io::Result<CString>

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
Copy link
Owner

sile commented Mar 25, 2022

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.

@sile sile merged commit d6ec2d2 into sile:master Mar 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants