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

Mark and document pty::ptsname() as unsafe #744

Merged
merged 1 commit into from
Aug 25, 2017

Conversation

nelsonjchen
Copy link
Contributor

On some platforms, ptsname() mutates global variables and mutating
global variables is always considered unsafe by Rust.

Reference:

#742 (comment)

@asomers
Copy link
Member

asomers commented Aug 24, 2017

Looks good, but please add a CHANGELOG entry, too. Also, you should update the commit message. ptsname mutates global variables on all platforms.

@nelsonjchen
Copy link
Contributor Author

@asomers

Would thread local be global?

apple/swift-compiler-rt#11

nelsonjchen added a commit to nelsonjchen/nix that referenced this pull request Aug 24, 2017
nelsonjchen added a commit to nelsonjchen/nix that referenced this pull request Aug 24, 2017
@nelsonjchen
Copy link
Contributor Author

Ah, it would be. Let me update the commit message.

@asomers
Copy link
Member

asomers commented Aug 24, 2017

As far as Rust is concerned a thread-local global variable is still a global variable. Using a thread-local variable does not make ptsname fully reentrant. For example, it still has race conditions with signal handlers and coroutines.

CHANGELOG.md Outdated
@@ -45,6 +45,8 @@ This project adheres to [Semantic Versioning](http://semver.org/).
- `MapFlags`, `MmapAdvise`, and `MsFlags` expose some more variants and only
officially-supported variants are provided for each target.
([#731](https://github.com/nix-rust/nix/pull/731))
- Marked `pty::ptsname` function to be as `unsafe`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/to be as/as/

src/pty.rs Outdated
///
/// This value is useful for opening the slave pty once the master has already been opened with
/// `posix_openpt()`.
///
/// On some platforms, `ptsname()` mutates global variables and is *not* threadsafe.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/On some platforms, //

@nelsonjchen
Copy link
Contributor Author

@asomers Changes are done.

src/pty.rs Outdated
///
/// This value is useful for opening the slave pty once the master has already been opened with
/// `posix_openpt()`.
///
/// `ptsname()` mutates global variables and is *not* threadsafe.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should go in a section named "Safety". See here for details.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that certainly wasn't in my original reading of the Book. I've added that. 👍

`ptsname()` mutates global variables and mutating global variables is
always considered `unsafe` by Rust.

Reference:

nix-rust#742 (comment)
@nelsonjchen
Copy link
Contributor Author

@Susurrus Changes are done

@Susurrus
Copy link
Contributor

@asomers Are you okay to sign off on this? LGTM!

@asomers
Copy link
Member

asomers commented Aug 25, 2017

bors r+

bors bot added a commit that referenced this pull request Aug 25, 2017
744: Mark and document pty::ptsname() as unsafe r=asomers a=nelsonjchen

On some platforms, `ptsname()` mutates global variables and mutating
global variables is always considered `unsafe` by Rust.

Reference:

#742 (comment)
@bors
Copy link
Contributor

bors bot commented Aug 25, 2017

@bors bors bot merged commit 505bed3 into nix-rust:master Aug 25, 2017
@nelsonjchen nelsonjchen deleted the mark_unsafe_ptsname branch August 25, 2017 20:18
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.

3 participants