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

String::from_raw_parts example is potentially problematic #122743

Closed
CAD97 opened this issue Mar 19, 2024 · 3 comments
Closed

String::from_raw_parts example is potentially problematic #122743

CAD97 opened this issue Mar 19, 2024 · 3 comments

Comments

@CAD97
Copy link
Contributor

CAD97 commented Mar 19, 2024

Location

The example for String::from_raw_parts's documentation.

Summary

Two potential issues, which are potentially unsound depending on the exact dynamic borrow model and implementation of String:

  • The example uses .as_mut_ptr() to get the raw pointer before calling .len() and .capacity(). It's better practice to extract the raw pointer last because calling other methods could potentially impact the provenance validity of the pointer.
  • The as_mut_ptr called is str::as_mut_ptr. With a strict subslicing interpretation of provenance, the result pointer doesn't have provenance over the entire capacity

The example itself doesn't trip Miri, but a slight adjustment to get a nonexact capacity does trip UB because of the latter point: [playground]

use std::mem;

unsafe {
    let mut s = String::from("hello");
    s.push('!'); // added

    // Prevent automatically dropping the String's data
    let mut s = mem::ManuallyDrop::new(s);

    let ptr = s.as_mut_ptr();
    let len = s.len();
    let capacity = s.capacity();

    let s = String::from_raw_parts(ptr, len, capacity);

    assert_eq!(String::from("hello!"), s);
}

Vec::from_raw_parts's example also calls .as_mut_ptr() first, but has Vec::as_mut_ptr specifically to shadow <[_]>::as_mut_ptr and avoid an intermediate slice reference. String should definitely get its own as_mut_ptr method like Vec has.

Changing the order the deconstruction is done is more debatable, since it's unlikely to ever actually cause UB, this order is more natural (to the point of TB treating all mut borrows as "two-phase" in order to allow it), and the reason for the order is subtle enough that changing the example is unlikely to assist people in writing it in the better order when it actually matters.

cc @rust-lang/opsem (yet another case of ptr before len)

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Mar 19, 2024
@RalfJung
Copy link
Member

Probably a duplicate of #106593?

@RalfJung
Copy link
Member

String should definitely get its own as_mut_ptr method like Vec has.

This was attempted in #97483 but got rejected. Maybe it's worth another shot.

@CAD97
Copy link
Contributor Author

CAD97 commented Mar 19, 2024

Yes, missed that in my quick search. Closing as

Duplicate of #106593

@CAD97 CAD97 closed this as completed Mar 19, 2024
@CAD97 CAD97 reopened this Mar 19, 2024
@CAD97 CAD97 closed this as not planned Won't fix, can't repro, duplicate, stale Mar 19, 2024
@jieyouxu jieyouxu removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Mar 20, 2024
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

No branches or pull requests

4 participants