Skip to content

fix: set close-on-exec on pty fds in the parent process #986

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Nick Darvey (NickDarvey) wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: set close-on-exec on pty fds in the parent process #986

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 12 additions & 2 deletions src/unix/pty.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,8 +112,6 @@ struct ExitEvent {
int exit_code = 0, signal_code = 0;
};

#if defined(__linux__)

static int
SetCloseOnExec(int fd) {
int flags = fcntl(fd, F_GETFD, 0);
Expand All @@ -124,6 +122,8 @@ SetCloseOnExec(int fd) {
return fcntl(fd, F_SETFD, flags | FD_CLOEXEC);
}

#if defined(__linux__)

/**
* Close all file descriptors >= 3 to prevent FD leakage to child processes.
* Uses close_range() syscall on Linux 5.9+, falls back to /proc/self/fd iteration.
Expand Down Expand Up @@ -415,6 +415,9 @@ Napi::Value PtyFork(const Napi::CallbackInfo& info) {
if (pty_nonblock(master) == -1) {
throw Napi::Error::New(napiEnv, "Could not set master fd to nonblocking.");
}
if (SetCloseOnExec(master) == -1) {
throw Napi::Error::New(napiEnv, "Could not set master fd to close-on-exec.");
}
#else
int argc = argv_.Length();
int argl = argc + 2;
Expand Down Expand Up @@ -488,6 +491,9 @@ Napi::Value PtyFork(const Napi::CallbackInfo& info) {
if (pty_nonblock(master) == -1) {
throw Napi::Error::New(napiEnv, "Could not set master fd to nonblocking.");
}
if (SetCloseOnExec(master) == -1) {
throw Napi::Error::New(napiEnv, "Could not set master fd to close-on-exec.");
}
}
#endif

Expand Down Expand Up @@ -535,6 +541,10 @@ Napi::Value PtyOpen(const Napi::CallbackInfo& info) {
throw Napi::Error::New(env, "Could not set slave fd to nonblocking.");
}

if (SetCloseOnExec(master) == -1 || SetCloseOnExec(slave) == -1) {
throw Napi::Error::New(env, "Could not set pty fds to close-on-exec.");
}

Napi::Object obj = Napi::Object::New(env);
obj.Set("master", Napi::Number::New(env, master));
obj.Set("slave", Napi::Number::New(env, slave));
Expand Down
22 changes: 22 additions & 0 deletions src/unixTerminal.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,14 @@ if (process.platform !== 'win32') {
term.slave!.write('slave\n');
term.master!.write('master\n');
});
if (process.platform === 'linux') {
it('should not leak the master or slave fd to processes the host spawns', () => {
term = UnixTerminal.open({});
const fds = cp.execFileSync('/bin/sh', ['-c', 'ls -l /proc/$$/fd'], { stdio: ['ignore', 'pipe', 'ignore'] }).toString();
assert.ok(!fds.includes('ptmx'), `child holds a pty master:\n${fds}`);
assert.ok(!fds.includes(term.ptsName), `child holds the pty slave ${term.ptsName}:\n${fds}`);
});
}
});
describe('close', () => {
const term = new UnixTerminal('node');
Expand Down Expand Up @@ -287,6 +295,20 @@ if (process.platform !== 'win32') {
done();
}, 1000);
});
it('should not leak pty master fds to processes the host spawns', () => {
const ptys: UnixTerminalType[] = [];
for (let i = 0; i < 2; i++) {
ptys.push(new UnixTerminal('/bin/bash', [], {}));
}
try {
const fds = cp.execFileSync('/bin/sh', ['-c', 'ls -l /proc/$$/fd'], { stdio: ['ignore', 'pipe', 'ignore'] }).toString();
assert.ok(!fds.includes('ptmx'), `child holds a pty master:\n${fds}`);
} finally {
for (const pty of ptys) {
pty.kill();
}
}
});
}
if (process.platform === 'darwin') {
it('should return the name of the process', (done) => {
Expand Down