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

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