Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions src/fork/pty/master/mod.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
mod err;

#[cfg(any(target_os = "macos"))]
mod ptsname_r_macos;

use descriptor::Descriptor;

pub use self::err::{MasterError, Result};
Expand Down Expand Up @@ -66,11 +69,18 @@ impl Master {
/// subsequent calls.
pub fn ptsname_r(&self, buf: &mut [u8]) -> Result<()> {
if let Some(fd) = self.pty {
// Safety: the vector's memory is valid for the duration
// of the call
unsafe {
Copy link
Contributor

@ethanpailes ethanpailes Nov 3, 2025

Choose a reason for hiding this comment

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

I'd like to maintain the style that every unsafe block has a saftey comment above it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mistakenly moved this one, updated; 559aa75

let data: *mut u8 = &mut buf[0];
match libc::ptsname_r(fd, data as *mut libc::c_char, buf.len()) {

#[cfg(any(target_os = "linux", target_os = "android"))]
// Safety: the vector's memory is valid for the duration
// of the call
let result = libc::ptsname_r(fd, data as *mut libc::c_char, buf.len());

#[cfg(any(target_os = "macos"))]
let result = ptsname_r_macos::ptsname_r(fd, data as *mut libc::c_char, buf.len());

match result {
0 => Ok(()),
_ => Err(MasterError::PtsnameError), // should probably capture errno
}
Expand Down
133 changes: 133 additions & 0 deletions src/fork/pty/master/ptsname_r_macos.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
//! macOS implementation of ptsname_r
//!
//! As `ptsname_r()` is not available on macOS, this provides a compatible
//! implementation using the `TIOCPTYGNAME` ioctl syscall.
//!
//! Based on: https://tarq.net/posts/ptsname-on-osx-with-rust/

#[cfg(any(target_os = "macos"))]
pub unsafe fn ptsname_r(
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a "Saftey" comment that explains the formal precondtions that any call site must meet in order to use this function correctly (should be pretty easy, just stuff like "buf needs to point to allocated memory" and "buflen can't be longer than the allocation" and "fd must be an open file descriptor").

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added; 559aa75

fd: libc::c_int,
buf: *mut libc::c_char,
buflen: libc::size_t,
) -> libc::c_int {
const IOCTL_BUF_SIZE: usize = 128;

if buf.is_null() || buflen == 0 {
return libc::EINVAL;
}

let mut ioctl_buf: [libc::c_char; IOCTL_BUF_SIZE] = [0; IOCTL_BUF_SIZE];

if libc::ioctl(fd, libc::TIOCPTYGNAME as libc::c_ulong, &mut ioctl_buf) != 0 {
return *libc::__error();
}

let mut len = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Totally fine to leave it like this, but for this sort of null-seaking, memchr (either the pure rust crate https://crates.io/crates/memchr, or the libc function https://docs.rs/libc/0.2.177/libc/fn.memchr.html) will do this a lot faster because they take advantage of SIMD hardware acceleration. It definitely won't matter for performance since the strings are so small here, so this is mostly just me being nerd sniped.

Copy link
Contributor Author

@seruman seruman Nov 7, 2025

Choose a reason for hiding this comment

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

TIL thank you. To not bring a new dependency, went for the libc one; 3bca717

while len < IOCTL_BUF_SIZE && ioctl_buf[len] != 0 {
len += 1;
}
len += 1;

if len > buflen {
return libc::ERANGE;
}

libc::memcpy(
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use std::ptr::copy (which is how you pronounce libc::memmove in rust) here. It is safer because it can handle overlapping ranges, and on modern branch predicting CPUs the extra branch it requires is undetectable in microbenchmarks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated; 3bca717

buf as *mut libc::c_void,
ioctl_buf.as_ptr() as *const libc::c_void,
len,
);

0
}

#[cfg(test)]
mod tests {
use super::*;
use std::ffi::CStr;

#[cfg(any(target_os = "macos"))]
#[test]
fn test_ptsname_r_retrieves_valid_name() {
let master_fd = unsafe { libc::posix_openpt(libc::O_RDWR | libc::O_NOCTTY) };
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a Saftey comment. I know it's just a test, but having them on every block makes auditing easier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

assert!(master_fd >= 0, "Failed to open master PTY");

unsafe {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a Saftey comment. I know it's just a test, but having them on every block makes auditing easier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated each unsafe section in the tests, not sure about the wording though. How all these sounds like? 559aa75

assert_eq!(libc::grantpt(master_fd), 0, "grantpt failed");
assert_eq!(libc::unlockpt(master_fd), 0, "unlockpt failed");
}

let mut buf = vec![0u8; 1024];
let result =
unsafe { ptsname_r(master_fd, buf.as_mut_ptr() as *mut libc::c_char, buf.len()) };
Copy link
Contributor

Choose a reason for hiding this comment

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

Saftey comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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


unsafe {
Copy link
Contributor

Choose a reason for hiding this comment

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

Saftey comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

libc::close(master_fd);
}

assert_eq!(result, 0, "ptsname_r failed with error code: {}", result);

let name = unsafe { CStr::from_ptr(buf.as_ptr() as *const libc::c_char) };
Copy link
Contributor

Choose a reason for hiding this comment

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

Saftey comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let name_str = name.to_str().expect("Invalid UTF-8 in PTY name");

assert!(
name_str.starts_with("/dev/ttys") || name_str.starts_with("/dev/pty"),
"Unexpected PTY name format: {}",
name_str
);
}

#[cfg(any(target_os = "macos"))]
#[test]
fn test_ptsname_r_buffer_too_small() {
let master_fd = unsafe { libc::posix_openpt(libc::O_RDWR | libc::O_NOCTTY) };
Copy link
Contributor

Choose a reason for hiding this comment

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

Saftey

Copy link
Contributor Author

Choose a reason for hiding this comment

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


if master_fd >= 0 {
unsafe {
Copy link
Contributor

Choose a reason for hiding this comment

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

Saftey

Copy link
Contributor Author

Choose a reason for hiding this comment

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

libc::grantpt(master_fd);
libc::unlockpt(master_fd);
}

let mut buf = [0u8; 2];
let result =
unsafe { ptsname_r(master_fd, buf.as_mut_ptr() as *mut libc::c_char, buf.len()) };
Copy link
Contributor

Choose a reason for hiding this comment

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

Saftey

Copy link
Contributor Author

Choose a reason for hiding this comment

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


unsafe {
libc::close(master_fd);
}

assert_eq!(result, libc::ERANGE);
}
}

#[cfg(any(target_os = "macos"))]
#[test]
fn test_ptsname_r_invalid_fd() {
let mut buf = vec![0u8; 1024];
let result = unsafe { ptsname_r(-1, buf.as_mut_ptr() as *mut libc::c_char, buf.len()) };
Copy link
Contributor

Choose a reason for hiding this comment

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

Saftey

Copy link
Contributor Author

Choose a reason for hiding this comment

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


assert_ne!(result, 0, "Expected non-zero error code for invalid fd");
}

#[cfg(any(target_os = "macos"))]
#[test]
fn test_ptsname_r_null_buffer() {
let master_fd = unsafe { libc::posix_openpt(libc::O_RDWR | libc::O_NOCTTY) };
Copy link
Contributor

Choose a reason for hiding this comment

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

Saftey

Copy link
Contributor Author

Choose a reason for hiding this comment

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


if master_fd >= 0 {
unsafe {
Copy link
Contributor

Choose a reason for hiding this comment

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

Saftey

Copy link
Contributor Author

Choose a reason for hiding this comment

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

libc::grantpt(master_fd);
libc::unlockpt(master_fd);
}

let result = unsafe { ptsname_r(master_fd, std::ptr::null_mut(), 1024) };

unsafe {
libc::close(master_fd);
Copy link
Contributor

Choose a reason for hiding this comment

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

Saftey

Copy link
Contributor Author

Choose a reason for hiding this comment

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

}

assert_eq!(result, libc::EINVAL);
}
}
}
2 changes: 1 addition & 1 deletion tests/it_can_read_write.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,6 @@ fn it_can_read_write() {
assert_eq!(read_line(&mut master).trim(), "readme!");
let _ = master.write("exit\n".to_string().as_bytes());
} else {
let _ = Command::new("bash").env_clear().status();
let _ = Command::new("sh").env_clear().status();
}
}
Loading