fix(pty): replace unsafe forkpty with posix_spawn on Unix - #107
Conversation
feb1542 to
101ab25
Compare
101ab25 to
8823d57
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8823d57f1c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // 7. Spawn child process (temporarily changing CWD to align workingDirectory) | ||
| string originalCwd = Directory.GetCurrentDirectory(); | ||
| if (!string.IsNullOrEmpty(workingDirectory)) | ||
| { | ||
| try | ||
| { | ||
| Directory.SetCurrentDirectory(workingDirectory); |
There was a problem hiding this comment.
Avoid mutating the process-wide working directory
When two terminal sessions start concurrently with different workingDirectory values, this process-global mutation can interleave: one child can spawn in the other session's directory and the final restore can leave the entire application in the wrong directory. Other threads can also resolve relative paths against the temporary directory during this window. Set the child directory through a spawn/file action (or equivalent child-only mechanism) rather than changing Directory.CurrentDirectory in the host.
Useful? React with 👍 / 👎.
| // Set setsid flag to launch child shell as the session leader | ||
| PosixSpawnAttrSetFlags(pAttr, POSIX_SPAWN_SETSID); |
There was a problem hiding this comment.
Restore terminal signal defaults in the spawned child
When the host inherits ignored terminal-control signals (for example, SIGINT or SIGTSTP), exec preserves those ignored dispositions. The removed fork path explicitly reset them, but this spawn path only sets POSIX_SPAWN_SETSID; as a result Ctrl+C/Ctrl+Z can be ignored by interactive shells in that environment. Configure POSIX_SPAWN_SETSIGDEF with the terminal-control signals (and check the attribute call result) before spawning.
Useful? React with 👍 / 👎.
Pull Request Summary: Replace Unsafe forkpty with posix_spawn on Unix
This Pull Request resolves the immediate startup crashes and memory corruption issues encountered when spawning a local PTY process on Linux.
The Problem (Root Cause)
Originally,
UnixPty.csused theforkptyAPI fromlibutil(Linux) andlibSystem(macOS). Calling a raw POSIXfork()in a multi-threaded.NETapplication is unsupported by the CLR.When
fork()clones the process:Even when attempting to execute zero managed instructions in the child thread immediately post-fork, returning from the system clone call in the parent thread corrupts the CLR JIT compilation/page protection table state on Linux. This r
The Solution (
posix_spawn)To avoid unsafe process cloning inside the multi-threaded C# host entirely, we migrated Unix process spawning to the standard POSIX
posix_spawnAPI.1. Controlling Terminal (
tty) AssociationA major hurdle when using
posix_spawnwith PTYs is associating the spawned child process session with the PTY master/slave.POSIX_SPAWN_SETSIDattribute, the child process is spawned as a new session leader.posix_spawn_file_actions_addopento open the slave PTY device path (e.g./dev/pts/X) asfd 0(stdin) inside the child process without theO_NOCTTYflag.O_NOCTTY, the tty automatically becomes the controlling terminal of the session.SIGINT/Ctrl+CandSIGTSTP/Ctrl+Z) to propagate correctly to foreground processes and job control to function natively.2. Environment & Spawning Details
fd 1) and stderr (fd 2) in the child viaposix_spawn_file_actions_adddup2.posix_spawncall and restoring it in afinallyblock im/bin/sh -cwrappers.Deleted Boilerplate & Simplifications
By moving the spawn orchestration to the OS kernel level before executing the target binary, we safely removed several complex, error-prone C# workarounds:
LoadLibcHandle: Replaced by standard[DllImport("libc")]which is resolved automatically on both macOS and Linux by the .NET runtime.GetSignalsToResetForExec: Manual signal disposition resets are no longer needed because the OS kernel automatically handles resetting caught signal handlers to defaults upon process execution.AllocNativeString/FreeNative: Since memory allocation and preparation occur synchronously on the parent thread before execution, standard unmanaged allocations are safely released using standard C#finallyblocks rather tMulti-Platform Safety
RoyalTerminal.Terminal.Pty.Windows).UnixPty.cs. Dynamic platform checks align compile-time constants (e.g.POSIX_SPAWN_SETSIDandO_NOCTTYvalues) to ensure correctness on both Darwin and Linux kernels.Verification & Tests
All
702unit and integration tests run successfully, including: