-
Notifications
You must be signed in to change notification settings - Fork 13k
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
strengthen doc warning about CString::from_raw #36018
Conversation
r? @aturon (rust_highfive has picked a reviewer for you, use r? to override) |
r? @steveklabnik or @GuillaumeGomez |
@bors: r+ rollup |
📌 Commit 70aa463 has been approved by |
strengthen doc warning about CString::from_raw Saw unsound code using this function on IRC.
/// of the string will be recalculated from the pointer. | ||
/// obtained by calling `into_raw` on a `CString`. In particular, using this method | ||
/// to create a `CString` pointing at memory that will be freed by other code | ||
/// (such as a C library) will lead to undefined behavior! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creating CString
from arbitrary allocation is not UB. Doing anything other than mem::forget(create)
and CString::from_raw(create)
what is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like we’ve brainstormed a nice way to say this somewhere already. The way String
and Vec
state this invariant is pretty good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. That's why I didn't say "is UB". I said "will lead to UB" which is indeed likely if you use from_raw
on anything that didn't come from into_raw
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, sure, it doesn’t always lead to UB regardless of its arguments either (i.e. if you immediately forget it, however unreasonable such code would be). Moreover using similar format as String::from_raw
and Vec::from_raw
helps consistency :)
@bors: r- sure, let's talk about it 😄 |
PS: also this fails tidy. |
Hmmm... want to squash those commits? ;) |
@jonathandturner sure I will squash if this goes in as is. Sounds like it needs some more work. I can copy the style from Vec::from_raw_parts as @nagisa suggested. |
Yes, let's copy that style and get this PR merged, finally. |
Done. |
@bors r=steveklabnik rollup |
📌 Commit 0d3d23b has been approved by |
strengthen doc warning about CString::from_raw Saw unsound code using this function on IRC.
💔 Test failed - auto-mac-32-opt |
strengthen doc warning about CString::from_raw Saw unsound code using this function on IRC.
Saw unsound code using this function on IRC.