diff --git a/index.d.ts b/index.d.ts index bc281ff..efdcc53 100644 --- a/index.d.ts +++ b/index.d.ts @@ -27,7 +27,7 @@ export function setCloseOnExec(fd: number, closeOnExec: boolean): void *_CLOEXEC` under the covers. */ export function getCloseOnExec(fd: number): boolean -export class Pty { +export declare class Pty { /** The pid of the forked process. */ pid: number constructor(opts: PtyOptions) diff --git a/package-lock.json b/package-lock.json index a695d53..164b6b2 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@replit/ruspty", - "version": "3.1.0", + "version": "3.1.1", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@replit/ruspty", - "version": "3.1.0", + "version": "3.1.1", "license": "MIT", "devDependencies": { "@napi-rs/cli": "^2.18.2", diff --git a/package.json b/package.json index 821c16c..b50b062 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@replit/ruspty", - "version": "3.1.0", + "version": "3.1.1", "main": "dist/wrapper.js", "types": "dist/wrapper.d.ts", "author": "Szymon Kaliski ", diff --git a/src/lib.rs b/src/lib.rs index 32eae70..59cf337 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,3 +1,13 @@ +use std::collections::HashMap; +use std::io::Error; +use std::io::ErrorKind; +use std::os::fd::{AsRawFd, OwnedFd}; +use std::os::fd::{BorrowedFd, FromRawFd, RawFd}; +use std::os::unix::process::CommandExt; +use std::process::{Command, Stdio}; +use std::thread; +use std::time::Duration; + use backoff::backoff::Backoff; use backoff::ExponentialBackoffBuilder; use libc::{self, c_int}; @@ -10,15 +20,6 @@ use nix::fcntl::{fcntl, FcntlArg, FdFlag}; use nix::poll::{poll, PollFd, PollFlags, PollTimeout}; use nix::pty::{openpty, Winsize}; use nix::sys::termios::{self, SetArg}; -use std::collections::HashMap; -use std::io::Error; -use std::io::ErrorKind; -use std::os::fd::{AsRawFd, OwnedFd}; -use std::os::fd::{BorrowedFd, FromRawFd, RawFd}; -use std::os::unix::process::CommandExt; -use std::process::{Command, Stdio}; -use std::thread; -use std::time::Duration; #[macro_use] extern crate napi_derive; @@ -115,10 +116,13 @@ impl Pty { cmd.args(args); } - // open pty pair + // open pty pair, and set close-on-exec to avoid unwanted copies of the FDs from finding their + // way into subprocesses. let pty_res = openpty(&window_size, None).map_err(cast_to_napi_error)?; let controller_fd = pty_res.master; let user_fd = pty_res.slave; + set_close_on_exec(controller_fd.as_raw_fd(), true)?; + set_close_on_exec(user_fd.as_raw_fd(), true)?; // duplicate pty user_fd to be the child's stdin, stdout, and stderr cmd.stdin(Stdio::from(user_fd.try_clone()?)); @@ -158,6 +162,16 @@ impl Pty { // and it's not safe to keep it open libc::close(raw_controller_fd); + // just to be safe, mark every single file descriptor as close-on-exec. + // needs to use the raw syscall to avoid dependencies on newer versions of glibc. + #[cfg(target_os = "linux")] + libc::syscall( + libc::SYS_close_range, + 3, + libc::c_uint::MAX, + libc::CLOSE_RANGE_CLOEXEC as c_int, + ); + // set input modes let user_fd = OwnedFd::from_raw_fd(raw_user_fd); if let Ok(mut termios) = termios::tcgetattr(&user_fd) { diff --git a/tests/index.test.ts b/tests/index.test.ts index f7ca7a0..03721c9 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -4,6 +4,9 @@ import { describe, test, expect } from 'vitest'; const EOT = '\x04'; const procSelfFd = '/proc/self/fd/'; +const IS_DARWIN = process.platform === 'darwin'; + +const testSkipOnDarwin = IS_DARWIN ? test.skip : test; type FdRecord = Record; function getOpenFds(): FdRecord { @@ -17,7 +20,9 @@ function getOpenFds(): FdRecord { const linkTarget = readlinkSync(procSelfFd + filename); if ( linkTarget === 'anon_inode:[timerfd]' || - linkTarget.startsWith('socket:[') + linkTarget.startsWith('socket:[') || + // node likes to asynchronously read stuff mid-test. + linkTarget.includes('/rustpy/') ) { continue; } @@ -86,10 +91,9 @@ describe( let buffer = ''; // We have local echo enabled, so we'll read the message twice. - const result = - process.platform === 'darwin' - ? 'hello cat\r\n^D\b\bhello cat\r\n' - : 'hello cat\r\nhello cat\r\n'; + const result = IS_DARWIN + ? 'hello cat\r\n^D\b\bhello cat\r\n' + : 'hello cat\r\nhello cat\r\n'; const pty = new Pty({ command: '/bin/cat', @@ -213,30 +217,86 @@ describe( }); })); - test('ordering is correct', () => - new Promise((done) => { - const oldFds = getOpenFds(); - let buffer = Buffer.from(''); - const n = 1024; - const pty = new Pty({ - command: '/bin/sh', - args: ['-c', `for i in $(seq 0 ${n}); do /bin/echo $i; done && exit`], - onExit: (err, exitCode) => { - expect(err).toBeNull(); - expect(exitCode).toBe(0); - expect(buffer.toString().trim()).toBe( - [...Array(n + 1).keys()].join('\r\n'), + test( + 'ordering is correct', + () => + new Promise((done) => { + const oldFds = getOpenFds(); + let buffer = Buffer.from(''); + const n = 1024; + const pty = new Pty({ + command: '/bin/sh', + args: [ + '-c', + `for i in $(seq 0 ${n}); do /bin/echo $i; done && exit`, + ], + onExit: (err, exitCode) => { + expect(err).toBeNull(); + expect(exitCode).toBe(0); + expect(buffer.toString().trim()).toBe( + [...Array(n + 1).keys()].join('\r\n'), + ); + expect(getOpenFds()).toStrictEqual(oldFds); + done(); + }, + }); + + const readStream = pty.read; + readStream.on('data', (data) => { + buffer = Buffer.concat([buffer, data]); + }); + }), + { repeats: 1 }, + ); + + testSkipOnDarwin( + 'does not leak files', + () => + new Promise((done) => { + const oldFds = getOpenFds(); + const promises = []; + for (let i = 0; i < 10; i++) { + promises.push( + new Promise((accept) => { + let buffer = Buffer.from(''); + const pty = new Pty({ + command: '/bin/sh', + args: [ + '-c', + 'sleep 0.1 ; ls /proc/$$/fd', + ], + onExit: (err, exitCode) => { + expect(err).toBeNull(); + expect(exitCode).toBe(0); + expect( + buffer + .toString() + .trim() + .split(/\s+/) + .filter((fd) => { + // Some shells dup stdio to fd 255 for reasons. + return fd !== '255'; + }) + .toSorted(), + ).toStrictEqual(['0', '1', '2']); + accept(); + }, + }); + + const readStream = pty.read; + readStream.on('data', (data) => { + buffer = Buffer.concat([buffer, data]); + }); + }), ); + } + Promise.allSettled(promises).then(() => { expect(getOpenFds()).toStrictEqual(oldFds); done(); - }, - }); - - const readStream = pty.read; - readStream.on('data', (data) => { - buffer = Buffer.concat([buffer, data]); - }); - })); + }); + }), + { repeats: 4 }, + ); test("doesn't break when executing non-existing binary", () => new Promise((done) => {