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

CString::from_raw should be document that changing the length of a string is UB #68456

Closed
KamilaBorowska opened this issue Jan 22, 2020 · 3 comments · Fixed by #72963
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) T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@KamilaBorowska
Copy link
Contributor

Consider the following code:

use std::ffi::CString;

fn main() {
    let c = CString::new(&b"Hello, world"[..]).unwrap();
    let ptr = c.into_raw();
    unsafe {
        *ptr = 0;
        CString::from_raw(ptr); // UB
    }
}

Shortening the string from CString::into_raw and then passing it to CString::from_raw will cause UB, but this is not documented in safety section of the documentation.

@Mark-Simulacrum
Copy link
Member

Hm, why do you think this causes UB? AFAICT, CString::from_raw explicitly notes that it will recalculate the length from the pointer.

@jonas-schievink jonas-schievink added A-FFI Area: Foreign function interface (FFI) 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 Jan 22, 2020
@Lokathor
Copy link
Contributor

This is a clever find. Well done.

Mark: The UB is that the length of the CString is recomputed from scratch, and also it's not the same, which means that the new CString is length 0, and so when it drops the reconstructed inner Box will pass a Layout value to the allocator's free function that doesn't match the Layout used to allocate that memory to begin with.

@Mark-Simulacrum
Copy link
Member

Ah, I see. So the reasoning is that we require "layout must be the same layout that was used to allocate that block of memory," on GlobalAlloc::dealloc. That makes sense.

I don't think there's anything we can do here (given the existing API) other than document this precondition.

Interestingly, we could imagine an API that would work when the underlying size would have changed (via realloc), but that requires us to know the prior Layout...

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) T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants