diff options
author | bors[bot] <26634292+bors[bot]@users.noreply.github.com> | 2021-02-21 00:08:45 +0000 |
---|---|---|
committer | GitHub <noreply@github.com> | 2021-02-21 00:08:45 +0000 |
commit | 7704f5d5d75a6c0067499280f027e0cc2510998d (patch) | |
tree | 1a9a5d1286e45c6aef101eceb69c687b2b7d439a | |
parent | 7e6096f12106aeeff742c50939a889848aefce85 (diff) | |
parent | c6c851403b6bd16c93227c309abad3e95f5476bf (diff) | |
download | nix-7704f5d5d75a6c0067499280f027e0cc2510998d.zip |
Merge #1390
1390: pty: Make forkpty() unsafe r=asomers a=tavianator
After the child returns from a fork() of a multi-threaded process, it is
undefined behaviour to call non-async-signal-safe functions according to
POSIX. Since forkpty() is implemented in terms of fork(), those
restrictions should apply to it too.
Fixes #1388
Co-authored-by: Tavian Barnes <tavianator@tavianator.com>
-rw-r--r-- | CHANGELOG.md | 4 | ||||
-rw-r--r-- | src/pty.rs | 29 | ||||
-rw-r--r-- | test/test_pty.rs | 4 |
3 files changed, 25 insertions, 12 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index 35c8fd7a..00212924 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,11 @@ This project adheres to [Semantic Versioning](http://semver.org/). ## [Unreleased] - ReleaseDate ### Added + ### Changed +- Made `forkpty` unsafe, like `fork` + (#[1390](https://github.com/nix-rust/nix/pull/1390)) + ### Fixed ### Removed @@ -302,7 +302,19 @@ pub fn openpty<'a, 'b, T: Into<Option<&'a Winsize>>, U: Into<Option<&'b Termios> /// 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 /// terminal settings of the slave will be set to the values in `termios`. -pub fn forkpty<'a, 'b, T: Into<Option<&'a Winsize>>, U: Into<Option<&'b Termios>>>( +/// +/// # Safety +/// +/// In a multithreaded program, only [async-signal-safe] functions like `pause` +/// and `_exit` may be called by the child (the parent isn't restricted). Note +/// that memory allocation may **not** be async-signal-safe and thus must be +/// prevented. +/// +/// Those functions are only a small subset of your operating system's API, so +/// special care must be taken to only invoke code you can control and audit. +/// +/// [async-signal-safe]: http://man7.org/linux/man-pages/man7/signal-safety.7.html +pub unsafe fn forkpty<'a, 'b, T: Into<Option<&'a Winsize>>, U: Into<Option<&'b Termios>>>( winsize: T, termios: U, ) -> Result<ForkptyResult> { @@ -323,20 +335,15 @@ pub fn forkpty<'a, 'b, T: Into<Option<&'a Winsize>>, U: Into<Option<&'b Termios> .map(|ws| ws as *const Winsize as *mut _) .unwrap_or(ptr::null_mut()); - let res = unsafe { - libc::forkpty(master.as_mut_ptr(), ptr::null_mut(), term, win) - }; + let res = libc::forkpty(master.as_mut_ptr(), ptr::null_mut(), term, win); let fork_result = Errno::result(res).map(|res| match res { 0 => ForkResult::Child, res => ForkResult::Parent { child: Pid::from_raw(res) }, })?; - unsafe { - Ok(ForkptyResult { - master: master.assume_init(), - fork_result, - }) - } + Ok(ForkptyResult { + master: master.assume_init(), + fork_result, + }) } - diff --git a/test/test_pty.rs b/test/test_pty.rs index ab347bb0..3b654443 100644 --- a/test/test_pty.rs +++ b/test/test_pty.rs @@ -255,7 +255,9 @@ fn test_forkpty() { let string = "naninani\n"; let echoed_string = "naninani\r\n"; - let pty = forkpty(None, None).unwrap(); + let pty = unsafe { + forkpty(None, None).unwrap() + }; match pty.fork_result { Child => { write(STDOUT_FILENO, string.as_bytes()).unwrap(); |