rust-cli / rexpect

.github/workflows/ci.yml
https://docs.rs/rexpect
MIT License
328 stars 56 forks source link

Prevent file descriptor leak in the spawned child. #97

Closed kosayoda closed 1 year ago

kosayoda commented 1 year ago

Problem

After forking the child process, 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

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:

image

On the PR branch:

image
matthiasbeyer commented 1 year ago

I approved CI, but as I am currently on vacation, and because of manners (:laughing:) I would like my co-maintainers to review this as well and thus I am not merging immediately.

souze commented 1 year ago

But you need to drop the trailing "." from your commit message heading for CI to pass. Then there is a failing test. It looks like our flaky test scenario, so might work if we retry

kosayoda commented 1 year ago

But you need to drop the trailing "." from your commit message heading for CI to pass. Then there is a failing test. It looks like our flaky test scenario, so might work if we retry

Done! Thanks for pointing that out.

matthiasbeyer commented 1 year ago

bors merge

bors[bot] commented 1 year ago

Build succeeded:

matthiasbeyer commented 1 year ago

@souze should we release this patch as a patch release? If yes, the merge commit from master, or a dedicated release-0.5.x branch with only the patch from this PR?

souze commented 1 year ago

No strong opinion. Releasing a patch-release from master seems like a good option to me :)