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

strengthen doc warning about CString::from_raw #36018

Merged
merged 1 commit into from
Sep 24, 2016
Merged

Conversation

durka
Copy link
Contributor

@durka durka commented Aug 26, 2016

Saw unsound code using this function on IRC.

@rust-highfive
Copy link
Collaborator

r? @aturon

(rust_highfive has picked a reviewer for you, use r? to override)

@durka
Copy link
Contributor Author

durka commented Aug 26, 2016

r? @steveklabnik or @GuillaumeGomez

@rust-highfive rust-highfive assigned steveklabnik and unassigned aturon Aug 26, 2016
@steveklabnik
Copy link
Member

@bors: r+ rollup

@bors
Copy link
Contributor

bors commented Aug 26, 2016

📌 Commit 70aa463 has been approved by steveklabnik

steveklabnik added a commit to steveklabnik/rust that referenced this pull request Aug 26, 2016
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!
Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

@nagisa nagisa Aug 27, 2016

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 :)

@steveklabnik
Copy link
Member

@bors: r-

sure, let's talk about it 😄

@sophiajt
Copy link
Contributor

PS: also this fails tidy.

@sophiajt
Copy link
Contributor

screw you, tidy

Hmmm... want to squash those commits? ;)

@durka
Copy link
Contributor Author

durka commented Aug 28, 2016

@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.

@steveklabnik
Copy link
Member

Yes, let's copy that style and get this PR merged, finally.

@durka
Copy link
Contributor Author

durka commented Sep 14, 2016

Done.

@arielb1
Copy link
Contributor

arielb1 commented Sep 17, 2016

@bors r=steveklabnik rollup

@bors
Copy link
Contributor

bors commented Sep 17, 2016

📌 Commit 0d3d23b has been approved by steveklabnik

@bors
Copy link
Contributor

bors commented Sep 18, 2016

⌛ Testing commit 0d3d23b with merge 7cbb58c...

bors added a commit that referenced this pull request Sep 18, 2016
strengthen doc warning about CString::from_raw

Saw unsound code using this function on IRC.
@bors
Copy link
Contributor

bors commented Sep 18, 2016

💔 Test failed - auto-mac-32-opt

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Sep 23, 2016
strengthen doc warning about CString::from_raw

Saw unsound code using this function on IRC.
bors added a commit that referenced this pull request Sep 24, 2016
Rollup of 7 pull requests

- Successful merges: #36018, #36498, #36500, #36559, #36566, #36578, #36664
- Failed merges:
@bors bors merged commit 0d3d23b into rust-lang:master Sep 24, 2016
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.

None yet

8 participants