From 4bbc27e004530942b6dcbd71885b749bf68b68eb Mon Sep 17 00:00:00 2001 From: Baptiste Bouillot Date: Tue, 8 Sep 2026 12:23:13 +0200 Subject: [PATCH 1/3] Fix PTY session lifecycle and strengthen integration coverage --- .github/workflows/tests.yml | 22 ++- README.md | 21 ++- SECURITY.md | 5 +- phpstan.neon | 4 + phpstan/cdata.stub | 21 +++ phpstan/ffi.stub | 18 ++ src/Pty.php | 120 ++++++++---- src/PtyException.php | 32 +++- src/Session.php | 103 ++++++++--- tests/AbiRegressionTest.php | 25 +-- tests/Fixtures/failed-exec.php | 27 +++ tests/Fixtures/receive.php | 29 +++ tests/LifecycleTest.php | 321 +++++++++++++++++++++++++++++++++ tests/PartialWriteTest.php | 148 ++++++--------- tests/Pest.php | 42 +++++ tests/PtyTest.php | 38 ++-- 16 files changed, 789 insertions(+), 187 deletions(-) create mode 100644 phpstan/cdata.stub create mode 100644 phpstan/ffi.stub create mode 100644 tests/Fixtures/failed-exec.php create mode 100644 tests/Fixtures/receive.php create mode 100644 tests/LifecycleTest.php diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index cecda6a..f5eb89b 100644 --- a/.github/workflows/tests.yml +++ b/.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 diff --git a/README.md b/README.md index ae61467..7e70de4 100644 --- a/README.md +++ b/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: diff --git a/SECURITY.md b/SECURITY.md index d69fe9a..830ab12 100644 --- a/SECURITY.md +++ b/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. diff --git a/phpstan.neon b/phpstan.neon index 6e451fa..815fdd2 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -2,3 +2,7 @@ parameters: level: 6 paths: - src + stubFiles: + - phpstan/ffi.stub + scanFiles: + - phpstan/cdata.stub diff --git a/phpstan/cdata.stub b/phpstan/cdata.stub new file mode 100644 index 0000000..3a8da5b --- /dev/null +++ b/phpstan/cdata.stub @@ -0,0 +1,21 @@ + $command Argv. The first element is resolved against PATH. - * @param array|null $env Child environment. Null inherits the current one. + * @param list $command Argv. The first element is resolved against PATH. + * @param array|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,10 +282,20 @@ 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) { @@ -259,6 +303,9 @@ private static function resolve(string $binary): string 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 */ 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); + } } } diff --git a/src/PtyException.php b/src/PtyException.php index 95f21bc..e806489 100644 --- a/src/PtyException.php +++ b/src/PtyException.php @@ -12,7 +12,7 @@ public static function missingExtension(string $name): self { return new self( "The `{$name}` extension is required by croustibat/pty but is not loaded. " - . 'Check `php -m`; with Herd or Homebrew you may need to enable it in php.ini.' + .'Check `php -m`; with Herd or Homebrew you may need to enable it in php.ini.' ); } @@ -20,8 +20,8 @@ public static function ffiDisabled(): self { return new self( 'FFI is not usable in this SAPI. croustibat/pty only supports the CLI SAPI, ' - . 'where `ffi.enable` defaults to true. Under FPM you would need `ffi.enable=preload`, ' - . 'which this library does not support — see the README section on fork safety.' + .'where `ffi.enable` defaults to true. Under FPM you would need `ffi.enable=preload`, ' + .'which this library does not support — see the README section on fork safety.' ); } @@ -29,7 +29,7 @@ public static function unsupportedPlatform(string $os): self { return new self( "croustibat/pty supports macOS and Linux; this platform reports `{$os}`. " - . 'Windows has no PTY: it exposes ConPTY, an unrelated API, and is explicitly out of scope.' + .'Windows has no PTY: it exposes ConPTY, an unrelated API, and is explicitly out of scope.' ); } @@ -37,8 +37,8 @@ public static function bindingFailed(): self { return new self( 'Could not bind openpty/login_tty through FFI. ' - . 'On Linux these live in libutil (install libutil / glibc dev headers are not needed, ' - . 'but `libutil.so.1` must be present).' + .'On Linux these live in libutil (install libutil / glibc dev headers are not needed, ' + .'but `libutil.so.1` must be present).' ); } @@ -71,4 +71,24 @@ public static function alreadyClosed(): self { return new self('This session has already been closed.'); } + + public static function invalidWorkingDirectory(string $cwd): self + { + return new self("Working directory `{$cwd}` does not exist or cannot be entered."); + } + + public static function invalidWinSize(int $rows, int $cols): self + { + return new self("Terminal dimensions must be between 1 and 65535; got {$rows}x{$cols}."); + } + + public static function winSizeFailed(): self + { + return new self('ioctl(TIOCGWINSZ) failed while reading the terminal dimensions.'); + } + + public static function invalidTimeout(): self + { + return new self('Timeout must be finite and greater than or equal to zero.'); + } } diff --git a/src/Session.php b/src/Session.php index c55a89c..2bdbc6f 100644 --- a/src/Session.php +++ b/src/Session.php @@ -4,6 +4,8 @@ namespace Croustibat\Pty; +use WeakMap; + /** * One process attached to one pseudo-terminal. * @@ -12,6 +14,9 @@ */ final class Session { + /** @var WeakMap|null Does not keep abandoned sessions alive. */ + private static ?WeakMap $sessions = null; + /** @var resource|null */ private $stream; @@ -31,6 +36,25 @@ public function __construct( private readonly string $ttyName, ) { $this->stream = $stream; + self::$sessions ??= new WeakMap; + self::$sessions[$this] = true; + } + + /** @internal Close only the forked child's copies, before exec. */ + public static function closeInheritedSessions(): void + { + if (self::$sessions !== null) { + $inherited = []; + foreach (self::$sessions as $session => $_) { + $inherited[] = $session; + } + + // close() removes its entry. Snapshot keys first so deleting the + // current WeakMap entry cannot skip the next inherited session. + foreach ($inherited as $session) { + $session->close(); + } + } } public function pid(): int @@ -51,7 +75,7 @@ public function ttyName(): string */ public function stream() { - if ($this->stream === null) { + if (! is_resource($this->stream)) { throw PtyException::alreadyClosed(); } @@ -70,6 +94,7 @@ public function stream() */ public function write(string $data, float $timeout = 5.0): int { + self::validateTimeout($timeout); if ($data === '') { return 0; } @@ -77,7 +102,7 @@ public function write(string $data, float $timeout = 5.0): int $stream = $this->stream(); $total = 0; $length = strlen($data); - $deadline = microtime(true) + $timeout; + $deadline = hrtime(true) / 1_000_000_000 + $timeout; while ($total < $length) { // A bounded deadline is not belt-and-braces, it is required. If @@ -86,7 +111,7 @@ public function write(string $data, float $timeout = 5.0): int // canonical mode and waiting for a newline that never comes — the // pty buffer stays full forever. Without this, write() hangs the // caller with no way out. - if (microtime(true) >= $deadline) { + if (hrtime(true) / 1_000_000_000 >= $deadline) { break; } @@ -98,7 +123,7 @@ public function write(string $data, float $timeout = 5.0): int $write = [$stream]; $except = []; - $remaining = max(0.0, $deadline - microtime(true)); + $remaining = max(0.0, $deadline - hrtime(true) / 1_000_000_000); $seconds = (int) $remaining; $microseconds = (int) (($remaining - $seconds) * 1_000_000); @@ -129,9 +154,7 @@ public function read(int $length = 65536): string */ public function resize(int $rows, int $cols): void { - if ($this->stream === null) { - throw PtyException::alreadyClosed(); - } + $this->stream(); Pty::setWinSize($this->masterFd, $rows, $cols); } @@ -139,16 +162,14 @@ public function resize(int $rows, int $cols): void /** @return array{0:int,1:int} [rows, cols] */ public function winSize(): array { - if ($this->stream === null) { - throw PtyException::alreadyClosed(); - } + $this->stream(); return Pty::getWinSize($this->masterFd); } public function signal(int $signal): bool { - return posix_kill($this->pid, $signal); + return $this->isRunning() && posix_kill($this->pid, $signal); } public function terminate(): bool @@ -169,7 +190,9 @@ public function isRunning(): bool } $status = 0; - $result = pcntl_waitpid($this->pid, $status, WNOHANG); + do { + $result = pcntl_waitpid($this->pid, $status, WNOHANG); + } while ($result === -1 && pcntl_get_last_error() === PCNTL_EINTR); if ($result === $this->pid) { $this->reap($status); @@ -177,6 +200,10 @@ public function isRunning(): bool return false; } + if ($result === -1 && pcntl_get_last_error() === PCNTL_ECHILD) { + $this->reaped = true; + } + return $result === 0; } @@ -188,10 +215,14 @@ public function isRunning(): bool * no escape is a defect, however unlikely the case. Pass `null` for the * old blocking behaviour, explicitly. * - * @return int Exit code, `128 + signal` when killed, or -1 on timeout. + * @return int Exit code, `128 + signal` when killed, or -1 on timeout/unavailable status. */ public function wait(?float $timeout = 10.0): int { + if ($timeout !== null) { + self::validateTimeout($timeout); + } + if ($this->reaped) { return $this->exitCode ?? -1; } @@ -199,15 +230,22 @@ public function wait(?float $timeout = 10.0): int $status = 0; if ($timeout === null) { - pcntl_waitpid($this->pid, $status); - $this->reap($status); + do { + $result = pcntl_waitpid($this->pid, $status); + } while ($result === -1 && pcntl_get_last_error() === PCNTL_EINTR); + + if ($result === $this->pid) { + $this->reap($status); + } elseif ($result === -1 && pcntl_get_last_error() === PCNTL_ECHILD) { + $this->reaped = true; + } return $this->exitCode ?? -1; } - $deadline = microtime(true) + $timeout; + $deadline = hrtime(true) / 1_000_000_000 + $timeout; - while (microtime(true) < $deadline) { + do { $result = pcntl_waitpid($this->pid, $status, WNOHANG); if ($result === $this->pid) { @@ -217,14 +255,20 @@ public function wait(?float $timeout = 10.0): int } if ($result === -1) { - // Already reaped elsewhere, or no such child. - $this->reaped = true; + if (pcntl_get_last_error() === PCNTL_ECHILD) { + $this->reaped = true; + } - return $this->exitCode ?? -1; + if (pcntl_get_last_error() !== PCNTL_EINTR) { + return -1; + } } - usleep(2_000); - } + $remaining = $deadline - hrtime(true) / 1_000_000_000; + if ($remaining > 0) { + usleep((int) min(2_000, $remaining * 1_000_000)); + } + } while (hrtime(true) / 1_000_000_000 < $deadline); return -1; } @@ -242,8 +286,14 @@ public function exitCode(): ?int public function close(): void { if ($this->stream !== null) { - @fclose($this->stream); + if (is_resource($this->stream)) { + fclose($this->stream); + } + + // php://fd owns a duplicate, not the original used by ioctl. + Pty::closeFd($this->masterFd); $this->stream = null; + unset(self::$sessions[$this]); } } @@ -260,4 +310,11 @@ private function reap(int $status): void ? pcntl_wexitstatus($status) : (pcntl_wifsignaled($status) ? 128 + pcntl_wtermsig($status) : -1); } + + private static function validateTimeout(float $timeout): void + { + if (! is_finite($timeout) || $timeout < 0) { + throw PtyException::invalidTimeout(); + } + } } diff --git a/tests/AbiRegressionTest.php b/tests/AbiRegressionTest.php index 96df5c5..d6e12fc 100644 --- a/tests/AbiRegressionTest.php +++ b/tests/AbiRegressionTest.php @@ -31,26 +31,29 @@ * why CI must run on macos-latest and not only ubuntu-latest. */ it('resizes through ioctl and the child agrees', function (): void { - $session = Pty::spawn(['/bin/sh', '-c', 'sleep 0.6; stty size'], rows: 24, cols: 80); - - usleep(150_000); - $session->resize(30, 120); - - $output = drain($session, 3.0, '/\d+\s+\d+/'); - $session->wait(); - $session->close(); + $session = Pty::spawn(['/bin/sh', '-c', 'stty -echo; echo READY; read -r line; stty size'], rows: 24, cols: 80); + + try { + expect(drain($session, 3.0, '/READY/'))->toContain('READY'); + $session->resize(30, 120); + $session->write("measure\n"); + $output = drain($session, 3.0, '/\d+\s+\d+/'); + expect($session->wait(2.0))->toBe(0); + } finally { + stopSession($session); + } $size = parseSttySize($output); expect($size)->not->toBeNull( - "The child printed nothing usable. Raw output: ".var_export($output, true) + 'The child printed nothing usable. Raw output: '.var_export($output, true) ); expect($size)->toBe( [30, 120], 'Window size is wrong after ioctl(TIOCSWINSZ). If this reads something ' - . 'like [0, 2046], the ioctl declaration in Pty::CDEF has lost its `...` ' - . 'and is being called with the wrong ABI.' + .'like [0, 2046], the ioctl declaration in Pty::CDEF has lost its `...` ' + .'and is being called with the wrong ABI.' ); }); diff --git a/tests/Fixtures/failed-exec.php b/tests/Fixtures/failed-exec.php new file mode 100644 index 0000000..8e85d3c --- /dev/null +++ b/tests/Fixtures/failed-exec.php @@ -0,0 +1,27 @@ +wait(2.0); + echo 'CHILD-OUTPUT:'.$session->read()."\n"; + echo 'EXIT:'.$exit."\n"; +} finally { + if ($session->isRunning()) { + $session->kill(); + $session->wait(2.0); + } + $session->close(); +} diff --git a/tests/Fixtures/receive.php b/tests/Fixtures/receive.php new file mode 100644 index 0000000..45fef28 --- /dev/null +++ b/tests/Fixtures/receive.php @@ -0,0 +1,29 @@ +close(); + expect($session->wait(2.0))->toBe(128 + SIGHUP); + expect(openDescriptors())->toBe($before); + } finally { + stopSession($session); + } +}); + +it('does not leak descriptors when executable resolution fails', function (): void { + $before = openDescriptors(); + + for ($attempt = 0; $attempt < 5; $attempt++) { + expect(fn () => Pty::spawn(['php-pty-nonexistent-executable']))->toThrow(PtyException::class); + } + + expect(openDescriptors())->toBe($before); +}); + +it('does not leak descriptors across repeated successful sessions', function (): void { + $before = openDescriptors(); + + for ($attempt = 0; $attempt < 5; $attempt++) { + $session = Pty::spawn(['/bin/sh', '-c', 'exit 42']); + try { + expect($session->wait(2.0))->toBe(42); + } finally { + stopSession($session); + } + } + + expect(openDescriptors())->toBe($before); +}); + +it('closes every inherited session in subsequent children', function (): void { + $sessions = []; + try { + for ($i = 0; $i < 3; $i++) { + $sessions[] = readySession(); + } + + foreach (array_slice($sessions, 0, 2) as $session) { + $session->close(); + expect($session->wait(2.0))->toBe(128 + SIGHUP); + } + + $sessions[2]->write("still-alive\n"); + expect(drain($sessions[2], 2.0, '/still-alive/'))->toContain('still-alive'); + } finally { + foreach ($sessions as $session) { + stopSession($session); + } + } +}); + +it('does not keep abandoned sessions alive in the registry', function (): void { + $session = readySession(); + $pid = $session->pid(); + $reference = WeakReference::create($session); + unset($session); + + try { + expect($reference->get())->toBeNull(); + $deadline = microtime(true) + 2.0; + do { + $result = pcntl_waitpid($pid, $status, WNOHANG); + if ($result !== 0) { + break; + } + usleep(2_000); + } while (microtime(true) < $deadline); + expect($result)->toBe($pid); + expect(pcntl_wifsignaled($status))->toBeTrue(); + expect(pcntl_wtermsig($status))->toBe(SIGHUP); + } finally { + if ($result === 0) { + posix_kill($pid, SIGKILL); + pcntl_waitpid($pid, $status); + } + } +}); + +it('does not close reused descriptors on repeated close or destruction', function (): void { + $first = readySession(); + $second = null; + try { + $first->close(); + expect($first->wait(2.0))->toBe(128 + SIGHUP); + $second = readySession(); + $first->close(); + unset($first); + + $second->resize(30, 120); + expect($second->winSize())->toBe([30, 120]); + $second->write("second-session\n"); + expect(drain($second, 2.0, '/second-session/'))->toContain('second-session'); + } finally { + if (isset($first)) { + stopSession($first); + } + if ($second !== null) { + stopSession($second); + } + } +}); + +it('releases the raw master even when the caller already closed the stream', function (): void { + $before = openDescriptors(); + $session = readySession(); + try { + fclose($session->stream()); + expect(fn () => $session->stream())->toThrow(PtyException::class); + $session->close(); + expect($session->wait(2.0))->toBe(128 + SIGHUP); + expect(openDescriptors())->toBe($before); + } finally { + stopSession($session); + } +}); + +it('keeps an unavailable child status unknown', function (?float $timeout, bool $checkRunning): void { + $session = Pty::spawn(['/bin/sh', '-c', 'exit 42']); + try { + expect(pcntl_waitpid($session->pid(), $status))->toBe($session->pid()); + expect(pcntl_wexitstatus($status))->toBe(42); + if ($checkRunning) { + expect($session->isRunning())->toBeFalse(); + } + expect($session->wait($timeout))->toBe(-1); + expect($session->wait())->toBe(-1); + expect($session->exitCode())->toBeNull(); + expect($session->signal(0))->toBeFalse(); + } finally { + stopSession($session); + } +})->with([[null, false], [1.0, false], [null, true], [1.0, true]]); + +it('can wait again after a timeout without losing the exit status', function (): void { + $session = readySession(); + try { + $start = hrtime(true); + expect($session->wait(0.05))->toBe(-1); + expect((hrtime(true) - $start) / 1e9)->toBeLessThan(1.0); + expect($session->exitCode())->toBeNull(); + expect($session->isRunning())->toBeTrue(); + $session->kill(); + expect($session->wait(2.0))->toBe(128 + SIGKILL); + expect($session->wait(null))->toBe(128 + SIGKILL); + expect($session->isRunning())->toBeFalse(); + } finally { + stopSession($session); + } +}); + +it('retries an interrupted blocking wait without inventing an exit status', function (): void { + $session = Pty::spawn(['/bin/sh', '-c', 'stty -echo; echo READY; read -r line; exit 42']); + $previous = pcntl_signal_get_handler(SIGUSR1); + $async = pcntl_async_signals(true); + $sender = null; + try { + expect(drain($session, 3.0, '/READY/'))->toContain('READY'); + $interrupted = false; + pcntl_signal(SIGUSR1, function () use (&$interrupted, $session): void { + $interrupted = true; + $session->write("exit\n"); + }, restart_syscalls: false); + + // The target cannot exit until our signal handler releases it. The + // short delay only arranges interruption, not child readiness. + $sender = Pty::spawn([ + PHP_BINARY, '-r', 'usleep(100000); posix_kill((int) $argv[1], SIGUSR1);', (string) getmypid(), + ]); + expect($session->wait(null))->toBe(42); + expect($interrupted)->toBeTrue(); + expect($session->exitCode())->toBe(42); + } finally { + if ($sender !== null) { + stopSession($sender); + } + stopSession($session); + pcntl_signal(SIGUSR1, $previous); + pcntl_async_signals($async); + } +}); + +it('makes a non-blocking status check with a zero timeout', function (): void { + $session = readySession(); + try { + expect($session->wait(0.0))->toBe(-1); + expect($session->write('not-sent', timeout: 0.0))->toBe(0); + expect($session->isRunning())->toBeTrue(); + $session->kill(); + $deadline = microtime(true) + 2.0; + do { + $exit = $session->wait(0.0); + if ($exit !== -1) { + break; + } + usleep(2_000); + } while (microtime(true) < $deadline); + expect($exit)->toBe(128 + SIGKILL); + } finally { + stopSession($session); + } +}); + +it('rejects invalid working directories before launching', function (): void { + $before = openDescriptors(); + expect(fn () => Pty::spawn(['/bin/pwd'], cwd: __DIR__.'/nonexistent-directory')) + ->toThrow(PtyException::class, 'Working directory'); + expect(openDescriptors())->toBe($before); +}); + +it('validates explicit executable paths', function (): void { + $before = openDescriptors(); + expect(fn () => Pty::spawn(['/php-pty-no-such-binary']))->toThrow(PtyException::class); + expect(openDescriptors())->toBe($before); +}); + +it('preserves the invoked executable symlink', function (): void { + $link = tempnam(sys_get_temp_dir(), 'php-pty-shell-'); + unlink($link); + symlink('/bin/sh', $link); + $session = null; + try { + $session = Pty::spawn([$link, '-c', 'printf "%s" "$0"']); + expect(drain($session))->toBe($link); + expect($session->wait(2.0))->toBe(0); + } finally { + if ($session !== null) { + stopSession($session); + } + unlink($link); + } +}); + +it('resolves relative PATH entries against the child directory', function (): void { + $path = getenv('PATH'); + $session = null; + try { + putenv('PATH=.'); + $session = Pty::spawn(['sh', '-c', 'printf "%s" "$0"'], cwd: '/bin'); + expect(drain($session))->toBe('/bin/./sh'); + expect($session->wait(2.0))->toBe(0); + } finally { + if ($session !== null) { + stopSession($session); + } + putenv($path === false ? 'PATH' : 'PATH='.$path); + } +}); + +it('does not run inherited PHP shutdown code when exec fails', function (): void { + $executable = tempnam(sys_get_temp_dir(), 'php-pty-invalid-'); + file_put_contents($executable, "#!/php-pty-no-such-interpreter\n"); + chmod($executable, 0700); + $session = null; + try { + $session = Pty::spawn([PHP_BINARY, __DIR__.'/Fixtures/failed-exec.php', $executable]); + $output = drain($session, 3.0); + expect($session->wait(2.0))->toBe(0); + expect($output)->toContain('EXIT:127'); + // Only the outer PHP process may run its own shutdown callback. + expect(substr_count($output, 'SHUTDOWN'))->toBe(1); + expect($output)->not->toContain('Fatal error'); + } finally { + if ($session !== null) { + stopSession($session); + } + unlink($executable); + } +}); + +it('honours the child working directory and literal arguments', function (): void { + $session = Pty::spawn( + ['./sh', '-c', 'printf "%s|%s|%s" "$PWD" "$1" "$PTY_TEST"', 'sh', 'a b;$(literal)'], + cwd: '/bin', + env: ['PTY_TEST' => 'environment-ok'], + ); + try { + expect(drain($session))->toBe(realpath('/bin').'|a b;$(literal)|environment-ok'); + expect($session->wait(2.0))->toBe(0); + } finally { + stopSession($session); + } +}); + +it('rejects invalid dimensions without allocating or changing a terminal', function (int $rows, int $cols): void { + $before = openDescriptors(); + expect(fn () => Pty::spawn(['/bin/cat'], rows: $rows, cols: $cols))->toThrow(PtyException::class); + expect(openDescriptors())->toBe($before); + + $session = readySession(); + try { + expect(fn () => $session->resize($rows, $cols))->toThrow(PtyException::class); + expect($session->winSize())->toBe([24, 80]); + } finally { + stopSession($session); + } +})->with([[-1, 80], [24, -1], [0, 80], [24, 0], [65536, 80], [24, 65536]]); + +it('rejects invalid timeouts', function (float $timeout): void { + $session = readySession(); + try { + expect(fn () => $session->wait($timeout))->toThrow(PtyException::class); + expect(fn () => $session->write('data', $timeout))->toThrow(PtyException::class); + } finally { + stopSession($session); + } +})->with([-1.0, INF, NAN]); diff --git a/tests/PartialWriteTest.php b/tests/PartialWriteTest.php index 25d08be..0031640 100644 --- a/tests/PartialWriteTest.php +++ b/tests/PartialWriteTest.php @@ -4,98 +4,70 @@ use Croustibat\Pty\Pty; -/** - * Guards Session::write() against partial writes. - * - * fwrite() on a pty master routinely writes fewer bytes than you asked for: - * the kernel buffer is small and the reader may be slow. Code that ignores the - * return value drops the tail silently. When the cut lands in the middle of an - * escape sequence, the terminal renders the remainder as literal text — a - * stray `7G` on screen where a cursor move was intended. - * - * Observed 2026-08-14 while relaying Claude Code through a pty. - * - * Two traps these tests exist to avoid, both hit while writing them: - * - * 1. `stty -echo` alone is NOT enough. It silences the echo but leaves the - * line discipline in canonical mode, where the input queue holds at most - * MAX_CANON bytes while it waits for a newline. Push half a megabyte with - * no `\n` and the queue jams. You need `raw`, which clears ICANON. - * - * 2. Never write a large payload to a child that echoes it back without - * draining as you go. The master's output buffer fills, the child blocks - * writing, so it stops reading, so your write blocks. Textbook deadlock. - */ -it('writes large payloads completely', function (): void { - // `raw` clears ICANON (no MAX_CANON jam) and `-echo` stops the line - // discipline bouncing everything back. `cat > /dev/null` consumes without - // producing, so there is nothing to drain and nothing to deadlock on. - $session = Pty::spawn(['/bin/sh', '-c', 'stty raw -echo; exec cat > /dev/null']); - - usleep(400_000); // let stty apply before the first byte goes out - - $payload = str_repeat("abcdefghij\n", 46_000); // ~506 KB - - $written = $session->write($payload, timeout: 10.0); - - $session->kill(); - $session->wait(); - $session->close(); +it('delivers a large payload completely to the child', function (): void { + $payload = str_repeat("abcdefghij\n", 46_000); + $session = Pty::spawn([ + '/bin/sh', '-c', 'stty raw -echo; exec "$@"', 'sh', + PHP_BINARY, __DIR__.'/Fixtures/receive.php', (string) strlen($payload), + ]); + + try { + expect(drain($session, 3.0, '/READY/'))->toContain('READY'); + expect($session->write($payload, timeout: 10.0))->toBe(strlen($payload)); + $receipt = 'RECEIVED:'.strlen($payload).':'.hash('sha256', $payload); + expect(drain($session, 3.0, '/RECEIVED:[0-9]+:[a-f0-9]{64}/'))->toContain($receipt); + expect($session->wait(2.0))->toBe(0); + } finally { + stopSession($session); + } +}); - expect($written)->toBe( - strlen($payload), - 'Session::write() returned a short count, which means it stopped at a ' - . 'partial write instead of looping. Escape sequences would be truncated.' - ); +it('bounds a saturated write and can resume without dropping or duplicating bytes', function (): void { + $payload = str_repeat("\033[7Gabcdefghij\n", 150_000); + $session = Pty::spawn([ + '/bin/sh', '-c', 'stty raw -echo; exec "$@"', 'sh', + PHP_BINARY, __DIR__.'/Fixtures/receive.php', (string) strlen($payload), 'wait', + ]); + + try { + expect(drain($session, 3.0, '/READY/'))->toContain('READY'); + $start = hrtime(true); + $written = $session->write($payload, timeout: 0.05); + $elapsed = (hrtime(true) - $start) / 1e9; + + expect($written)->toBeGreaterThan(0)->toBeLessThan(strlen($payload)); + expect($elapsed)->toBeLessThan(1.0); + expect($session->signal(SIGUSR1))->toBeTrue(); + expect($session->write(substr($payload, $written), timeout: 10.0))->toBe(strlen($payload) - $written); + + $receipt = 'RECEIVED:'.strlen($payload).':'.hash('sha256', $payload); + expect(drain($session, 3.0, '/RECEIVED:[0-9]+:[a-f0-9]{64}/'))->toContain($receipt); + expect($session->wait(2.0))->toBe(0); + } finally { + stopSession($session); + } }); it('does not mangle escape sequences it writes', function (): void { - // Deliberately small. A pty is not a lossless pipe: saturate an echoing - // child and bytes go missing, which is a property of the kernel buffers - // and the scheduler, not of this package. Testing that would be testing - // the OS. What we promise is narrower and worth guarding: an escape - // sequence handed to write() arrives byte-for-byte, and write() reports - // the full length. - $session = Pty::spawn(['/bin/sh', '-c', 'stty raw -echo; exec cat']); - - usleep(400_000); - - $needle = "\033[7GMARKER\033[0m"; - $payload = str_repeat('.', 512).$needle.str_repeat('.', 512); - - $written = $session->write($payload, timeout: 5.0); - - // Read until we have as many bytes as we sent — NOT until some marker - // shows up. Stopping on a marker that sits in the middle of the needle - // truncates the very thing being asserted. - $received = ''; - $deadline = microtime(true) + 3.0; - - while (microtime(true) < $deadline && strlen($received) < strlen($payload)) { - $read = [$session->stream()]; - $write = []; - $except = []; - - if (@stream_select($read, $write, $except, 0, 50_000) > 0) { - $received .= $session->read(); + $session = Pty::spawn(['/bin/sh', '-c', 'stty raw -echo; printf "READY\n"; exec cat']); + + try { + expect(drain($session, 3.0, '/READY/'))->toContain('READY'); + $payload = str_repeat('.', 512)."\033[7GMARKER\033[0m".str_repeat('.', 512); + expect($session->write($payload, timeout: 5.0))->toBe(strlen($payload)); + + $received = ''; + $deadline = microtime(true) + 3.0; + while (microtime(true) < $deadline && strlen($received) < strlen($payload)) { + $read = [$session->stream()]; + $write = []; + $except = []; + if (stream_select($read, $write, $except, 0, 50_000) > 0) { + $received .= $session->read(); + } } + expect($received)->toBe($payload); + } finally { + stopSession($session); } - - $session->kill(); - $session->wait(); - $session->close(); - - expect($written)->toBe(strlen($payload), 'write() reported a short count.'); - - // Asserted in widening order so a failure localises itself: wrong length - // means bytes were dropped, wrong ESC count means the line discipline ate - // the escapes, and only then does the needle itself get checked. - // - // NOTE — do not pass a message to toContain(). It is variadic: every - // argument is treated as another needle to look for, so a "message" - // silently becomes a second assertion that can never pass. That mistake - // cost an hour of chasing phantom kernel bugs on 2026-08-14. - expect(strlen($received))->toBe(strlen($payload), 'Bytes were lost in the round trip.'); - expect(substr_count($received, "\033"))->toBe(2, 'Escape bytes were stripped.'); - expect(str_contains($received, $needle))->toBeTrue(); }); diff --git a/tests/Pest.php b/tests/Pest.php index bdd484b..d671e17 100644 --- a/tests/Pest.php +++ b/tests/Pest.php @@ -2,6 +2,7 @@ declare(strict_types=1); +use Croustibat\Pty\Pty; use Croustibat\Pty\Session; /** @@ -50,3 +51,44 @@ function parseSttySize(string $output): ?array return [(int) $matches[1], (int) $matches[2]]; } + +function readySession(): Session +{ + $session = Pty::spawn(['/bin/sh', '-c', 'stty -echo; printf "READY\n"; exec cat']); + + try { + expect(drain($session, 3.0, '/READY/'))->toContain('READY'); + } catch (Throwable $error) { + stopSession($session); + throw $error; + } + + return $session; +} + +function stopSession(Session $session): void +{ + if ($session->isRunning()) { + $session->kill(); + } + $session->wait(2.0); + $session->close(); +} + +/** @return list */ +function openDescriptors(): array +{ + static $ffi; + $ffi ??= FFI::cdef('int fcntl(int fd, int cmd, ...);'); + $open = []; + + // All descriptors allocated by these small test processes are below 1024. + // F_GETFD is 1 on both supported platforms; this creates no descriptors. + for ($fd = 0; $fd < 1024; $fd++) { + if ($ffi->fcntl($fd, 1) !== -1) { + $open[] = $fd; + } + } + + return $open; +} diff --git a/tests/PtyTest.php b/tests/PtyTest.php index 642d59b..298c1a0 100644 --- a/tests/PtyTest.php +++ b/tests/PtyTest.php @@ -39,34 +39,30 @@ it('delivers SIGWINCH on resize', function (): void { $session = Pty::spawn( - ['/bin/sh', '-c', 'trap "echo GOT-WINCH" WINCH; sleep 2 & wait'], + ['/bin/sh', '-c', 'trap "echo GOT-WINCH" WINCH; echo READY; sleep 10 & wait'], rows: 24, cols: 80, ); - usleep(300_000); - $session->resize(40, 100); - - $output = drain($session, 2.0, '/GOT-WINCH/'); - $session->kill(); - $session->wait(); - $session->close(); - - expect($output)->toContain('GOT-WINCH'); + try { + expect(drain($session, 3.0, '/READY/'))->toContain('READY'); + $session->resize(40, 100); + expect(drain($session, 2.0, '/GOT-WINCH/'))->toContain('GOT-WINCH'); + } finally { + stopSession($session); + } }); it('round-trips data through the pty', function (): void { - $session = Pty::spawn(['/bin/cat']); - - usleep(200_000); - $session->write("hello pty\n"); - - $output = drain($session, 2.0, '/hello pty/'); - $session->kill(); - $session->wait(); - $session->close(); - - expect($output)->toContain('hello pty'); + $session = Pty::spawn(['/bin/sh', '-c', 'stty -echo; echo READY; IFS= read -r line; printf "REPLY:%s\n" "$line"']); + try { + expect(drain($session, 3.0, '/READY/'))->toContain('READY'); + $session->write("hello pty\n"); + expect(drain($session, 2.0, '/REPLY:hello pty/'))->toContain('REPLY:hello pty'); + expect($session->wait(2.0))->toBe(0); + } finally { + stopSession($session); + } }); it('reports the exit code', function (): void { From c496f035323f254f18c4b25fb6bf8a9f1fff1d67 Mon Sep 17 00:00:00 2001 From: Baptiste Bouillot Date: Tue, 8 Sep 2026 12:23:54 +0200 Subject: [PATCH 2/3] Restore parent error handling in failed-exec fixture --- tests/Fixtures/failed-exec.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/Fixtures/failed-exec.php b/tests/Fixtures/failed-exec.php index 8e85d3c..b9d1fb0 100644 --- a/tests/Fixtures/failed-exec.php +++ b/tests/Fixtures/failed-exec.php @@ -14,6 +14,9 @@ }); $session = Pty::spawn([$argv[1]]); +// Only the forked child should turn exec warnings into exceptions. Linux +// can report EIO while the parent drains a PTY whose slave has closed. +restore_error_handler(); try { $exit = $session->wait(2.0); echo 'CHILD-OUTPUT:'.$session->read()."\n"; From fdbf53f7a1a8225b1a99a2469e1b1e98e4066fd5 Mon Sep 17 00:00:00 2001 From: Baptiste Bouillot Date: Tue, 8 Sep 2026 12:29:39 +0200 Subject: [PATCH 3/3] Make terminal startup and resize checks deterministic across runners --- tests/Fixtures/watch-resize.php | 20 ++++++++++++++++++++ tests/LifecycleTest.php | 8 ++++---- tests/PtyTest.php | 17 +++++++++++------ 3 files changed, 35 insertions(+), 10 deletions(-) create mode 100644 tests/Fixtures/watch-resize.php diff --git a/tests/Fixtures/watch-resize.php b/tests/Fixtures/watch-resize.php new file mode 100644 index 0000000..853d4ac --- /dev/null +++ b/tests/Fixtures/watch-resize.php @@ -0,0 +1,20 @@ +toBe($link); + expect(drain($session, 3.0, '#'.preg_quote($link, '#').'#'))->toBe($link); expect($session->wait(2.0))->toBe(0); } finally { if ($session !== null) { @@ -251,7 +251,7 @@ try { putenv('PATH=.'); $session = Pty::spawn(['sh', '-c', 'printf "%s" "$0"'], cwd: '/bin'); - expect(drain($session))->toBe('/bin/./sh'); + expect(drain($session, 3.0, '#/bin/\./sh#'))->toBe('/bin/./sh'); expect($session->wait(2.0))->toBe(0); } finally { if ($session !== null) { @@ -268,7 +268,7 @@ $session = null; try { $session = Pty::spawn([PHP_BINARY, __DIR__.'/Fixtures/failed-exec.php', $executable]); - $output = drain($session, 3.0); + $output = drain($session, 3.0, '/SHUTDOWN/'); expect($session->wait(2.0))->toBe(0); expect($output)->toContain('EXIT:127'); // Only the outer PHP process may run its own shutdown callback. @@ -289,7 +289,7 @@ env: ['PTY_TEST' => 'environment-ok'], ); try { - expect(drain($session))->toBe(realpath('/bin').'|a b;$(literal)|environment-ok'); + expect(drain($session, 3.0, '/environment-ok/'))->toBe(realpath('/bin').'|a b;$(literal)|environment-ok'); expect($session->wait(2.0))->toBe(0); } finally { stopSession($session); diff --git a/tests/PtyTest.php b/tests/PtyTest.php index 298c1a0..e31cf63 100644 --- a/tests/PtyTest.php +++ b/tests/PtyTest.php @@ -8,11 +8,15 @@ it('gives the child a real tty', function (): void { $session = Pty::spawn(['/bin/sh', '-c', 'tty']); - $output = drain($session, 2.0, '#/dev/#'); - $session->wait(); - $session->close(); - - expect($output)->toContain('/dev/'); + try { + // Keep draining during startup: on Darwin even a small final write + // can delay child exit until its output has been consumed. + $pattern = '#'.preg_quote($session->ttyName(), '#').'#'; + expect(drain($session, 10.0, $pattern))->toContain($session->ttyName()); + expect($session->wait(2.0))->toBe(0); + } finally { + stopSession($session); + } }); it('reports the slave device path', function (): void { @@ -39,7 +43,7 @@ it('delivers SIGWINCH on resize', function (): void { $session = Pty::spawn( - ['/bin/sh', '-c', 'trap "echo GOT-WINCH" WINCH; echo READY; sleep 10 & wait'], + [PHP_BINARY, __DIR__.'/Fixtures/watch-resize.php'], rows: 24, cols: 80, ); @@ -48,6 +52,7 @@ expect(drain($session, 3.0, '/READY/'))->toContain('READY'); $session->resize(40, 100); expect(drain($session, 2.0, '/GOT-WINCH/'))->toContain('GOT-WINCH'); + expect($session->wait(2.0))->toBe(0); } finally { stopSession($session); }