From 6acb8f24661e53ca6df195174e0fdad808572e3e Mon Sep 17 00:00:00 2001 From: lhchavez Date: Sat, 13 Jul 2024 19:57:28 -0700 Subject: [PATCH] 3.1.1 Do not leak file descriptors (#35) The `openpty` function was created on a simpler time, when `O_CLOEXEC` was not even a thing. Leaking file descriptors was _fine_. But we don't live in that world anymore. This change now marks both ends of the PTY as close-on-exec, so that the file descriptors are not leaked. Importantly, since we rely on both copies (the one in ruspty and the one in the child) being closed to consider the stream done, _if_ we manage to leak the FD to another shell, there will be an unexpected third copy, so the stream will unnecessarily hang for a second (good that we added a failsafe cap on execution time)! --- index.d.ts | 2 +- package-lock.json | 4 +- package.json | 2 +- src/lib.rs | 34 ++++++++++---- tests/index.test.ts | 112 ++++++++++++++++++++++++++++++++++---------- 5 files changed, 114 insertions(+), 40 deletions(-) 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) => {