Skip to content

Commit

Permalink
Mark and document pty::ptsname() as unsafe
Browse files Browse the repository at this point in the history
`ptsname()` mutates global variables and mutating global variables is
always considered `unsafe` by Rust.

Reference:

#742 (comment)
  • Loading branch information
nelsonjchen committed Aug 24, 2017
1 parent b76cbee commit f9b16a3
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 14 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 as `unsafe`
([#744](https://github.com/nix-rust/nix/pull/744))

# Fixed
- Fix compilation and tests for OpenBSD targets
Expand Down
20 changes: 12 additions & 8 deletions src/pty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ pub fn grantpt(fd: &PtyMaster) -> Result<()> {
/// unlockpt(&master_fd)?;
///
/// // Get the name of the slave
/// let slave_name = ptsname(&master_fd)?;
/// let slave_name = unsafe { ptsname(&master_fd) }?;
///
/// // Try to open the slave
/// # #[allow(unused_variables)]
Expand All @@ -125,20 +125,24 @@ pub fn posix_openpt(flags: fcntl::OFlag) -> Result<PtyMaster> {
/// [ptsname(3)](http://man7.org/linux/man-pages/man3/ptsname.3.html))
///
/// `ptsname()` returns the name of the slave pseudoterminal device corresponding to the master
/// referred to by `fd`. Note that this function is *not* threadsafe. For that see `ptsname_r()`.
/// referred to by `fd`.
///
/// 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.
/// Mutating global variables is always considered `unsafe` by Rust and this
/// function is marked as `unsafe` to reflect that.
///
/// For a threadsafe and non-`unsafe` alternative on Linux, see `ptsname_r()`.
#[inline]
pub fn ptsname(fd: &PtyMaster) -> Result<String> {
let name_ptr = unsafe { libc::ptsname(fd.as_raw_fd()) };
pub unsafe fn ptsname(fd: &PtyMaster) -> Result<String> {
let name_ptr = libc::ptsname(fd.as_raw_fd());
if name_ptr.is_null() {
return Err(Error::last().into());
}

let name = unsafe {
CStr::from_ptr(name_ptr)
};
let name = CStr::from_ptr(name_ptr);
Ok(name.to_string_lossy().into_owned())
}

Expand Down Expand Up @@ -187,7 +191,7 @@ pub fn unlockpt(fd: &PtyMaster) -> Result<()> {

/// Create a new pseudoterminal, returning the slave and master file descriptors
/// in `OpenptyResult`
/// (see [openpty](http://man7.org/linux/man-pages/man3/openpty.3.html)).
/// (see [openpty](http://man7.org/linux/man-pages/man3/openpty.3.html)).
///
/// If `winsize` is not `None`, the window size of the slave will be set to
/// the values in `winsize`. If `termios` is not `None`, the pseudoterminal's
Expand Down
12 changes: 6 additions & 6 deletions test/test_pty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ fn test_ptsname_equivalence() {
assert!(master_fd.as_raw_fd() > 0);

// Get the name of the slave
let slave_name = ptsname(&master_fd).unwrap();
let slave_name = unsafe { ptsname(&master_fd) }.unwrap() ;
let slave_name_r = ptsname_r(&master_fd).unwrap();
assert_eq!(slave_name, slave_name_r);
}
Expand All @@ -53,8 +53,8 @@ fn test_ptsname_copy() {
assert!(master_fd.as_raw_fd() > 0);

// Get the name of the slave
let slave_name1 = ptsname(&master_fd).unwrap();
let slave_name2 = ptsname(&master_fd).unwrap();
let slave_name1 = unsafe { ptsname(&master_fd) }.unwrap();
let slave_name2 = unsafe { ptsname(&master_fd) }.unwrap();
assert!(slave_name1 == slave_name2);
// Also make sure that the string was actually copied and they point to different parts of
// memory.
Expand Down Expand Up @@ -92,8 +92,8 @@ fn test_ptsname_unique() {
assert!(master2_fd.as_raw_fd() > 0);

// Get the name of the slave
let slave_name1 = ptsname(&master1_fd).unwrap();
let slave_name2 = ptsname(&master2_fd).unwrap();
let slave_name1 = unsafe { ptsname(&master1_fd) }.unwrap();
let slave_name2 = unsafe { ptsname(&master2_fd) }.unwrap();
assert!(slave_name1 != slave_name2);
}

Expand All @@ -116,7 +116,7 @@ fn test_open_ptty_pair() {
unlockpt(&master_fd).expect("unlockpt failed");

// Get the name of the slave
let slave_name = ptsname(&master_fd).expect("ptsname failed");
let slave_name = unsafe { ptsname(&master_fd) }.expect("ptsname failed");

// Open the slave device
let slave_fd = open(Path::new(&slave_name), O_RDWR, stat::Mode::empty()).unwrap();
Expand Down

0 comments on commit f9b16a3

Please sign in to comment.