3.1.1 Do not leak file descriptors

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)

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 <[email protected]>",

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) {

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<string, string>;
 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<void>((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<void>((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<void>((done) => {
+          const oldFds = getOpenFds();
+          const promises = [];
+          for (let i = 0; i < 10; i++) {
+            promises.push(
+              new Promise<void>((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<void>((done) => {