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

Document that CString::new sheds unused capacity from the buffer it's given #60558

Closed
Minoru opened this issue May 5, 2019 · 3 comments
Closed
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools A-ffi Area: Foreign Function Interface (FFI) C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@Minoru
Copy link

Minoru commented May 5, 2019

I tried to use std::ffi::CString as an output buffer for libc::snprintf like this (playground):

fn obtain_answer() -> Option<String> {
    let format = "The answer ... is: %llu";

    const BUF_SIZE: usize = 1024;
    let buffer = Vec::<u8>::with_capacity(BUF_SIZE);
    unsafe {
        CString::new(buffer).ok().and_then(|buffer| {
            CString::new(format).ok().and_then(|format| {
                let buffer_ptr = buffer.into_raw();
                libc::snprintf(
                    buffer_ptr,
                    BUF_SIZE as libc::size_t,
                    format.as_ptr() as *const libc::c_char,
                    std::u64::MAX,
                );
                let buffer = CString::from_raw(buffer_ptr);
                buffer.into_string().ok()
            })
        })
    }
}

I thought I'm creating a buffer of 1024 bytes, but in reality, CString::new appends a null byte and calls Vec::into_boxed_slice on my Vec, shrinking it to 1 byte. This leads to memory corruption, since I pass snprintf a pointer to a one-byte buffer but tell it there are 1024 bytes available.

I'm a bit unsure if CString is even a good choice here, but if it is, I think CString's docs should be more explicit on what happens to the buffer. I suggest adding a comment to CString::new doc saying the same as Vec::into_boxed_slice: "Note that this will drop any excess capacity".

@jonas-schievink jonas-schievink added A-ffi Area: Foreign Function Interface (FFI) C-enhancement Category: An issue proposing an enhancement or a PR with one. A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels May 5, 2019
@kyrias
Copy link
Contributor

kyrias commented May 5, 2019

On the other hand, nowhere does it say that it will actually use the same allocation, just the bytes from what you pass in, so you can't make any kind of assumptions as to what happens to the allocation you pass in. And the implementation could for whatever reason be changed not to do that in the future as well.

@Minoru
Copy link
Author

Minoru commented May 5, 2019

Yeah, I'm probably just misusing that CString, and should've used Vec<u8> instead. I feel a bit silly suggesting to add something like "don't pass CString as a mutable buffer to fill, use this or that instead", but that might be better than my previous suggestion.

@Mark-Simulacrum Mark-Simulacrum removed the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Sep 22, 2019
@jonas-schievink jonas-schievink added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Mar 6, 2020
@Minoru
Copy link
Author

Minoru commented Mar 10, 2021

I'm a bit unsure if CString is even a good choice here

It's not. I should've used Vec<u8> instead. Closing the issue because I no longer think that CString's doc needs amending.

@Minoru Minoru closed this as completed Mar 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools A-ffi Area: Foreign Function Interface (FFI) C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants