Skip to content

Commit

Permalink
Merge #97
Browse files Browse the repository at this point in the history
97: Prevent file descriptor leak in the spawned child. r=matthiasbeyer a=kosayoda

## Problem
After [forking the child process](https://github.com/rust-cli/rexpect/blob/cee87fc7a2ef743f09e1e880c8cf7b268a59b351/src/process.rs#L98), the file descriptors for the master and slave aren't closed since we created these descriptors ourselves (Rust sets CLOEXEC on any fds created in the stdlib).

I stumbled upon the problem when my child became a orphan after sending SIGINT to its parent. Since the drop handlers weren't run, the child isn't terminated/waited on. The expected scenario (what pexpect does) is that the master side of the pty is closed by the kernel on process exit, and SIGHUP is sent to the child process (which kills the child by default). However, since the controlling terminal is left open by the child, SIGHUP is not sent.

## Reproduction and Fix
```rust
use rexpect::spawn;

fn main() {
    let mut p = spawn("sleep 100", Some(30_000)).unwrap();

    // Hang parent
    let mut s = String::new();
    std::io::stdin().read_line(&mut s).unwrap();
}
```

On the current master:
<img width="375" alt="image" src="https://user-images.githubusercontent.com/41782385/229242068-01c6b68e-30b4-4c3b-a99d-62ed513fd9e9.png">

On the PR branch:
<img width="372" alt="image" src="https://user-images.githubusercontent.com/41782385/229242179-98e6085e-a671-42d2-93fc-9075a2ba72b4.png">


Co-authored-by: kosayoda <[email protected]>
  • Loading branch information
bors[bot] and kosayoda committed Apr 10, 2023
2 parents cee87fc + 10e68bf commit 59213cd
Showing 1 changed file with 9 additions and 1 deletion.
10 changes: 9 additions & 1 deletion src/process.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use nix::libc::{STDERR_FILENO, STDIN_FILENO, STDOUT_FILENO};
use nix::pty::{grantpt, posix_openpt, unlockpt, PtyMaster};
pub use nix::sys::{signal, wait};
use nix::sys::{stat, termios};
use nix::unistd::{dup, dup2, fork, setsid, ForkResult, Pid};
use nix::unistd::{close, dup, dup2, fork, setsid, ForkResult, Pid};
use std;
use std::fs::File;
use std::os::unix::io::{AsRawFd, FromRawFd};
Expand Down Expand Up @@ -97,6 +97,9 @@ impl PtyProcess {

match unsafe { fork()? } {
ForkResult::Child => {
// Avoid leaking master fd
close(master_fd.as_raw_fd())?;

setsid()?; // create new session with child as session leader
let slave_fd = open(
std::path::Path::new(&slave_name),
Expand All @@ -109,6 +112,11 @@ impl PtyProcess {
dup2(slave_fd, STDOUT_FILENO)?;
dup2(slave_fd, STDERR_FILENO)?;

// Avoid leaking slave fd
if slave_fd > STDERR_FILENO {
close(slave_fd)?;
}

// set echo off
let mut flags = termios::tcgetattr(STDIN_FILENO)?;
flags.local_flags &= !termios::LocalFlags::ECHO;
Expand Down

0 comments on commit 59213cd

Please sign in to comment.