Fix PTY resource leaks, child lifecycle and integration coverage

.github/workflows/tests.yml

@@ -17,7 +17,7 @@ jobs:
       fail-fast: false
       matrix:
         os: [ubuntu-latest, macos-latest]
-        php: ['8.2', '8.3', '8.4']
+        php: ['8.2', '8.3', '8.4', '8.5']
 
     name: PHP ${{ matrix.php }} · ${{ matrix.os }}
 
@@ -36,8 +36,28 @@ jobs:
         run: |
           php -r 'printf("PHP %s · %s · %s%s", PHP_VERSION, PHP_OS, php_uname("m"), PHP_EOL);'
 
+      - name: Require Apple Silicon for the ABI regression
+        if: runner.os == 'macOS'
+        run: test "$(uname -m)" = arm64
+
       - name: Install dependencies
         run: composer update --prefer-dist --no-interaction --no-progress
 
       - name: Run tests
+        timeout-minutes: 5
         run: vendor/bin/pest --colors=always
+
+  quality:
+    name: Static analysis and style
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+      - uses: shivammathur/setup-php@v2
+        with:
+          php-version: '8.5'
+          extensions: ffi, pcntl, posix
+          coverage: none
+      - run: composer update --prefer-dist --no-interaction --no-progress
+      - run: composer validate --strict
+      - run: vendor/bin/phpstan analyse --no-progress
+      - run: vendor/bin/pint --test

README.md

@@ -158,9 +158,28 @@ Two related traps, both worth knowing before you write your own relay:
 | `Session::winSize(): array` | `[rows, cols]` |
 | `Session::ttyName(): string` | e.g. `/dev/ttys018` |
 | `Session::pid()`, `isRunning()`, `signal()`, `terminate()`, `kill()` | |
-| `Session::wait(): int` | Blocks; `128 + signal` when killed |
+| `Session::wait(?float $timeout = 10.0): int` | Exit code, `128 + signal` when killed; `-1` on timeout or unavailable status. `null` waits indefinitely |
 | `Session::close()` | Closes the master; the child gets `SIGHUP` |
 
+Rows and columns must be between **1 and 65535**, both at spawn and resize.
+An invalid working directory or executable is rejected before allocating a
+terminal. Explicit relative executable paths are resolved against `cwd` when
+provided. If the working directory disappears before the child enters it, the
+child exits with code 127 without executing the command.
+
+`write()` returns the number of bytes accepted before its deadline; on a short
+write, retain and retry the remaining suffix. Timeouts must be finite and
+non-negative. `write(timeout: 0)` returns immediately without writing;
+`wait(timeout: 0)` makes one non-blocking status check. A timed-out `wait()`
+does not stop the child. If another process manager has already reaped the
+child, `wait()` returns `-1` and `exitCode()` remains `null`.
+
+`close()` is idempotent and releases both the original master descriptor and
+the PHP stream's duplicate. Newly spawned children close their inherited
+copies of other php-pty sessions. Closing a session does not reap its process:
+call `wait()` afterwards to collect its status. A child can handle or ignore
+`SIGHUP`; use `terminate()` or `kill()` when explicit termination is needed.
+
 ## Scope
 
 Out of scope, deliberately:

SECURITY.md

@@ -35,7 +35,7 @@ of them are bugs, and all of them are your responsibility to contain.
 
 `ext-ffi` executes arbitrary C with no memory safety. A wrong struct layout or
 a bad pointer is a memory corruption, not an exception. The library only binds
-`openpty`, `login_tty`, `ioctl` and `close`, and the declarations are covered
+`openpty`, `login_tty`, `ioctl`, `close` and `_exit`, and the declarations are covered
 by tests — including `tests/AbiRegressionTest.php`, which exists because a
 wrong `ioctl` declaration corrupts memory *while returning 0*.
 
@@ -47,7 +47,8 @@ not your biggest problem, but it is not for you either.
 `pcntl_fork()` duplicates everything: open database connections, Redis sockets,
 file handles, credentials in memory, and any secret your process is holding.
 The child then `exec`s, so the exposure window is short — but it exists, and if
-`exec` fails the child briefly runs your code with a copy of your state.
+`exec` fails the child exits with code 127 using `_exit`, without running
+inherited PHP shutdown callbacks or destructors.
 
 The library refuses to load outside the CLI SAPI for this reason. Do not work
 around that check.

phpstan.neon

@@ -2,3 +2,7 @@ parameters:
     level: 6
     paths:
         - src
+    stubFiles:
+        - phpstan/ffi.stub
+    scanFiles:
+        - phpstan/cdata.stub

phpstan/cdata.stub

@@ -0,0 +1,21 @@
+<?php
+
+namespace FFI;
+
+/**
+ * Analysis-only model of the int and struct winsize fields used by Pty::CDEF.
+ * Scanned, never executed: CData implements property access in native code,
+ * which PHPStan cannot infer from reflection or magic-property annotations.
+ */
+final class CData
+{
+    public int $cdata;
+
+    public int $ws_row;
+
+    public int $ws_col;
+
+    public int $ws_xpixel;
+
+    public int $ws_ypixel;
+}

phpstan/ffi.stub

@@ -0,0 +1,18 @@
+<?php
+
+// Analysis-only declarations matching Pty::CDEF. FFI exposes these symbols
+// dynamically; this file is never autoloaded or executed.
+namespace {
+    /**
+     * @method int openpty(\FFI\CData $master, \FFI\CData $slave, \FFI\CData $name, ?\FFI\CData $termios, \FFI\CData $size)
+     * @method int login_tty(int $fd)
+     * @method int ioctl(int $fd, int $request, \FFI\CData $argument)
+     * @method int close(int $fd)
+     * @method never _exit(int $status)
+     */
+    final class FFI {}
+}
+
+namespace FFI {
+    final class CData {}
+}

src/Pty.php

@@ -60,15 +60,16 @@ final class Pty
         int login_tty(int fd);
         int ioctl(int fd, unsigned long request, ...);
         int close(int fd);
+        void _exit(int status);
         C;
 
     private static ?FFI $ffi = null;
 
     /**
      * Spawns a command attached to a fresh pseudo-terminal.
      *
-     * @param  list<string>          $command Argv. The first element is resolved against PATH.
-     * @param  array<string,string>|null $env  Child environment. Null inherits the current one.
+     * @param  list<string>  $command  Argv. The first element is resolved against PATH.
+     * @param  array<string,string>|null  $env  Child environment. Null inherits the current one.
      */
     public static function spawn(
         array $command,
@@ -81,6 +82,27 @@ public static function spawn(
             throw PtyException::executableNotFound('');
         }
 
+        self::validateWinSize($rows, $cols);
+
+        if ($cwd !== null && (! is_dir($cwd) || ! is_executable($cwd))) {
+            throw PtyException::invalidWorkingDirectory($cwd);
+        }
+
+        // Resolve before allocating descriptors: a rejected command must not
+        // leave an open master and slave behind.
+        $directory = $cwd ?? getcwd();
+        if ($directory === false) {
+            throw PtyException::invalidWorkingDirectory('.');
+        }
+        if (! str_starts_with($directory, '/')) {
+            $parent = getcwd();
+            if ($parent === false) {
+                throw PtyException::invalidWorkingDirectory($directory);
+            }
+            $directory = $parent.'/'.$directory;
+        }
+        $executable = self::resolve($command[0], $directory);
+        $environment = $env ?? self::currentEnvironment();
         $ffi = self::ffi();
 
         // The initial size is passed straight to openpty(), which is NOT
@@ -110,12 +132,27 @@ public static function spawn(
         $slaveFd = $slave->cdata;
         $ttyName = FFI::string($name);
 
-        $executable = self::resolve($command[0]);
-        $environment = $env ?? self::currentEnvironment();
+        // php://fd duplicates the descriptor. Open it before forking so a
+        // failed duplication cannot leave a running child behind.
+        $stream = @fopen("php://fd/{$masterFd}", 'r+');
+
+        if ($stream === false) {
+            $ffi->close($masterFd);
+            $ffi->close($slaveFd);
+
+            throw PtyException::masterUnreadable($masterFd);
+        }
+
+        stream_set_blocking($stream, false);
+        // Disable PHP's hidden flush loop: a full PTY must return control to
+        // Session::write() so its deadline can be enforced.
+        stream_set_write_buffer($stream, 0);
+        stream_set_read_buffer($stream, 0);
 
         $pid = pcntl_fork();
 
         if ($pid === -1) {
+            fclose($stream);
             $ffi->close($masterFd);
             $ffi->close($slaveFd);
 
@@ -127,40 +164,34 @@ public static function spawn(
             // login_tty() does setsid + TIOCSCTTY + dup2 onto 0/1/2. Without
             // it the child has a tty but no *controlling* tty: no job control,
             // no signal delivery on Ctrl-C, and interactive TUIs misbehave.
-            $ffi->close($masterFd);
+            try {
+                fclose($stream);
+                $ffi->close($masterFd);
+                Session::closeInheritedSessions();
 
-            if ($ffi->login_tty($slaveFd) !== 0) {
-                exit(127);
-            }
+                if ($ffi->login_tty($slaveFd) !== 0) {
+                    $ffi->_exit(127);
+                }
 
-            if ($cwd !== null) {
-                @chdir($cwd);
-            }
+                // The directory can disappear after parent-side validation.
+                // Never execute the command in an unintended working directory.
+                if ($cwd !== null && ! @chdir($cwd)) {
+                    $ffi->_exit(127);
+                }
 
-            pcntl_exec($executable, array_slice($command, 1), $environment);
+                @pcntl_exec($executable, array_slice($command, 1), $environment);
+            } catch (Throwable) {
+                // An inherited error handler can turn an exec warning into an
+                // exception. It must never unwind into the caller in the child.
+            }
 
-            exit(127); // only reached if exec failed
+            // Do not run inherited PHP shutdown callbacks or destructors.
+            $ffi->_exit(127);
         }
 
         // ---- parent ----
         $ffi->close($slaveFd);
 
-        $stream = @fopen("php://fd/{$masterFd}", 'r+');
-
-        if ($stream === false) {
-            throw PtyException::masterUnreadable($masterFd);
-        }
-
-        stream_set_blocking($stream, false);
-
-        // PHP buffers stream writes in userspace and retries the flush itself.
-        // On a pty master whose buffer is full that retry loop is invisible to
-        // us and cannot be interrupted — fwrite() simply does not return. Set
-        // the buffer to 0 so every fwrite() maps to exactly one write(2) and
-        // hands EAGAIN straight back.
-        stream_set_write_buffer($stream, 0);
-        stream_set_read_buffer($stream, 0);
-
         return new Session($pid, $stream, $masterFd, $ttyName);
     }
 
@@ -172,6 +203,7 @@ public static function spawn(
      */
     public static function setWinSize(int $fd, int $rows, int $cols): void
     {
+        self::validateWinSize($rows, $cols);
         $ffi = self::ffi();
 
         $winsize = $ffi->new('struct winsize');
@@ -195,7 +227,9 @@ public static function getWinSize(int $fd): array
         $ffi = self::ffi();
 
         $winsize = $ffi->new('struct winsize');
-        $ffi->ioctl($fd, self::TIOCGWINSZ, FFI::addr($winsize));
+        if ($ffi->ioctl($fd, self::TIOCGWINSZ, FFI::addr($winsize)) !== 0) {
+            throw PtyException::winSizeFailed();
+        }
 
         return [$winsize->ws_row, $winsize->ws_col];
     }
@@ -248,17 +282,30 @@ private static function ffi(): FFI
     }
 
     /** Resolves a binary against PATH without shelling out. */
-    private static function resolve(string $binary): string
+    private static function resolve(string $binary, string $cwd): string
     {
         if (str_contains($binary, '/')) {
-            return $binary;
+            $candidate = str_starts_with($binary, '/')
+                ? $binary
+                : rtrim($cwd, '/').'/'.$binary;
+
+            if (is_file($candidate) && is_executable($candidate)) {
+                // Keep executable symlinks: argv[0] selects BusyBox applets
+                // and the invocation path identifies Python virtualenvs.
+                return $candidate;
+            }
+
+            throw PtyException::executableNotFound($binary);
         }
 
         foreach (explode(':', (string) getenv('PATH')) as $directory) {
             if ($directory === '') {
                 continue;
             }
 
+            if (! str_starts_with($directory, '/')) {
+                $directory = rtrim($cwd, '/').'/'.$directory;
+            }
             $candidate = rtrim($directory, '/').'/'.$binary;
 
             if (is_file($candidate) && is_executable($candidate)) {
@@ -272,8 +319,13 @@ private static function resolve(string $binary): string
     /** @return array<string,string> */
     private static function currentEnvironment(): array
     {
-        $env = getenv();
+        return getenv();
+    }
 
-        return is_array($env) ? $env : [];
+    private static function validateWinSize(int $rows, int $cols): void
+    {
+        if ($rows < 1 || $rows > 65535 || $cols < 1 || $cols > 65535) {
+            throw PtyException::invalidWinSize($rows, $cols);
+        }
     }
 }