diff --git a/pkg/commands/oscommands/pty.go b/pkg/commands/oscommands/pty.go index 8a803fe60..a4a369633 100644 --- a/pkg/commands/oscommands/pty.go +++ b/pkg/commands/oscommands/pty.go @@ -1,7 +1,6 @@ package oscommands import ( - "errors" "io" "os" ) @@ -29,10 +28,6 @@ type StartedPty struct { Wait func() error } -// ErrPtyUnsupported is returned by StartPty on platforms without a pty -// implementation. Callers may fall back to running the command without a pty. -var ErrPtyUnsupported = errors.New("pty not supported on this platform") - // StartPty runs cmd in a pseudo-terminal with the given initial dimensions. // Implemented per-platform in pty_unix.go / pty_windows.go. // diff --git a/pkg/commands/oscommands/pty_windows.go b/pkg/commands/oscommands/pty_windows.go index 6d0d7c6ff..0a4a06477 100644 --- a/pkg/commands/oscommands/pty_windows.go +++ b/pkg/commands/oscommands/pty_windows.go @@ -1,12 +1,259 @@ package oscommands import ( + "fmt" + "os" "os/exec" + "sync" + "unsafe" + + "golang.org/x/sys/windows" ) -// StartPty is a stub on Windows for now; callers fall back to the non-pty -// path when ErrPtyUnsupported is returned. A real ConPTY implementation -// replaces this in a follow-up commit. -func StartPty(cmd *exec.Cmd, cols, rows uint16) (StartedPty, error) { - return StartedPty{}, ErrPtyUnsupported +type winPty struct { + hpc windows.Handle + inWrite *os.File + outRead *os.File + + // mu guards the teardown state below and serializes it against Resize. + // hpcClosed gates ClosePseudoConsole (it must run exactly once) and also + // keeps Resize from touching the HPCON once it's been freed: the + // background waiter in StartPty closes the pseudoconsole on child exit, + // which would otherwise race a concurrent onResize and hand + // ResizePseudoConsole a freed handle. + mu sync.Mutex + hpcClosed bool + closed bool +} + +func (p *winPty) Read(buf []byte) (int, error) { return p.outRead.Read(buf) } +func (p *winPty) Write(buf []byte) (int, error) { return p.inWrite.Write(buf) } + +func (p *winPty) Resize(cols, rows uint16) error { + p.mu.Lock() + defer p.mu.Unlock() + if p.hpcClosed { + // The child already exited and the pseudoconsole was torn down, so + // there is nothing left to resize. + return nil + } + return windows.ResizePseudoConsole(p.hpc, windows.Coord{X: int16(cols), Y: int16(rows)}) +} + +// closeHpc closes the pseudoconsole exactly once. Safe to call from multiple +// goroutines and at any time. We need this separately from Close because the +// background waiter in StartPty closes the pseudoconsole as soon as the child +// exits — that's what makes outRead return EOF, matching the Unix behavior +// where the master fd EOFs when the slave closes — while the pipe fds stay +// open until somebody explicitly tears the pty down. +func (p *winPty) closeHpc() { + p.mu.Lock() + defer p.mu.Unlock() + p.closeHpcLocked() +} + +// closeHpcLocked closes the pseudoconsole; the caller must hold p.mu. +func (p *winPty) closeHpcLocked() { + if p.hpcClosed { + return + } + p.hpcClosed = true + windows.ClosePseudoConsole(p.hpc) +} + +func (p *winPty) Close() error { + p.mu.Lock() + defer p.mu.Unlock() + if p.closed { + return nil + } + p.closed = true + // Closing the pseudoconsole breaks the pipes; the child's next write + // fails and it exits. Then we close our ends of the pipes. + p.closeHpcLocked() + p.inWrite.Close() + p.outRead.Close() + return nil +} + +// startWaiter runs proc.Wait in a goroutine and, as soon as the child exits, +// closes the pseudoconsole so that any pending Read on outRead returns EOF +// after buffered output drains. Returns a Wait func that blocks until the +// child has exited and reports its exit status with *exec.Cmd.Wait semantics. +// +// This shape exists because on Unix the master fd EOFs naturally when the +// slave closes on child exit, but ConPTY keeps the pipe alive until we call +// ClosePseudoConsole explicitly. Without doing that on child exit, the +// scanner in pkg/tasks.NewCmdTask would block forever on the next read and +// the post-content view never gets cleared (FlushStaleCells never fires). +func startWaiter(proc *os.Process, p *winPty) func() error { + done := make(chan struct{}) + var waitErr error + go func() { + defer close(done) + state, err := proc.Wait() + p.closeHpc() + if err != nil { + waitErr = err + return + } + if !state.Success() { + waitErr = fmt.Errorf("exit status %d", state.ExitCode()) + } + }() + return func() error { + <-done + return waitErr + } +} + +func StartPty(cmd *exec.Cmd, cols, rows uint16) (sp StartedPty, err error) { + // Two pipes: one for the child's stdin (we never write to it, but ConPTY + // needs a handle), one for the child's stdout/stderr multiplexed through + // the pseudoconsole. + var inRead, inWrite, outRead, outWrite windows.Handle + if err = windows.CreatePipe(&inRead, &inWrite, nil, 0); err != nil { + return StartedPty{}, fmt.Errorf("CreatePipe (in): %w", err) + } + defer func() { + if err != nil { + _ = windows.CloseHandle(inWrite) + } + }() + if err = windows.CreatePipe(&outRead, &outWrite, nil, 0); err != nil { + _ = windows.CloseHandle(inRead) + return StartedPty{}, fmt.Errorf("CreatePipe (out): %w", err) + } + defer func() { + if err != nil { + _ = windows.CloseHandle(outRead) + } + }() + + // CreatePseudoConsole dupes the handles it needs internally; we release + // our references to the child-side ends immediately after. + var hpc windows.Handle + size := windows.Coord{X: int16(cols), Y: int16(rows)} + if err = windows.CreatePseudoConsole(size, inRead, outWrite, 0, &hpc); err != nil { + _ = windows.CloseHandle(inRead) + _ = windows.CloseHandle(outWrite) + return StartedPty{}, fmt.Errorf("CreatePseudoConsole: %w", err) + } + _ = windows.CloseHandle(inRead) + _ = windows.CloseHandle(outWrite) + defer func() { + if err != nil { + windows.ClosePseudoConsole(hpc) + } + }() + + // Attach the pseudoconsole to the child via a process attribute list. + attrList, err := windows.NewProcThreadAttributeList(1) + if err != nil { + return StartedPty{}, fmt.Errorf("NewProcThreadAttributeList: %w", err) + } + defer attrList.Delete() + // PROC_THREAD_ATTRIBUTE_PSEUDOCONSOLE wants the HPCON value itself as + // the attribute value, not a pointer to it — an HPCON is already a + // pointer-sized handle, per Microsoft's ConPTY sample. Spelling that as + // unsafe.Pointer(hpc) trips go vet's unsafeptr check (a uintptr-based + // type converted straight to unsafe.Pointer), which gopls surfaces in + // the editor. Reinterpret the handle's bits through its address instead: + // &hpc is a real pointer, so none of these conversions is the flagged + // uintptr→unsafe.Pointer cast, while the resulting value is identical. + if err = attrList.Update( + windows.PROC_THREAD_ATTRIBUTE_PSEUDOCONSOLE, + *(*unsafe.Pointer)(unsafe.Pointer(&hpc)), + unsafe.Sizeof(hpc), + ); err != nil { + return StartedPty{}, fmt.Errorf("UpdateProcThreadAttribute: %w", err) + } + + var si windows.StartupInfoEx + si.Cb = uint32(unsafe.Sizeof(si)) + si.ProcThreadAttributeList = attrList.List() + + var appNamePtr *uint16 + if cmd.Path != "" { + if appNamePtr, err = windows.UTF16PtrFromString(cmd.Path); err != nil { + return StartedPty{}, err + } + } + cmdLinePtr, err := windows.UTF16PtrFromString(windows.ComposeCommandLine(cmd.Args)) + if err != nil { + return StartedPty{}, err + } + var dirPtr *uint16 + if cmd.Dir != "" { + if dirPtr, err = windows.UTF16PtrFromString(cmd.Dir); err != nil { + return StartedPty{}, err + } + } + envBlock, err := createEnvBlock(cmd.Env) + if err != nil { + return StartedPty{}, err + } + var envPtr *uint16 + if envBlock != nil { + envPtr = &envBlock[0] + } + + var pi windows.ProcessInformation + err = windows.CreateProcess( + appNamePtr, + cmdLinePtr, + nil, // process security + nil, // thread security + false, + windows.EXTENDED_STARTUPINFO_PRESENT|windows.CREATE_UNICODE_ENVIRONMENT, + envPtr, + dirPtr, + &si.StartupInfo, + &pi, + ) + if err != nil { + return StartedPty{}, fmt.Errorf("CreateProcess: %w", err) + } + _ = windows.CloseHandle(pi.Thread) + + // Re-open the process by PID to get an *os.Process to wait on. Do this + // while pi.Process is still open: Windows won't recycle a PID while any + // handle to the process remains, so FindProcess can't latch onto a + // different process that has since reused the PID. Release the original + // handle once we have our own. + proc, err := os.FindProcess(int(pi.ProcessId)) + _ = windows.CloseHandle(pi.Process) + if err != nil { + return StartedPty{}, err + } + + wp := &winPty{ + hpc: hpc, + inWrite: os.NewFile(uintptr(inWrite), "conpty-in"), + outRead: os.NewFile(uintptr(outRead), "conpty-out"), + } + return StartedPty{ + Pty: wp, + Process: proc, + Wait: startWaiter(proc, wp), + }, nil +} + +// createEnvBlock packs env vars into the UTF-16 double-null-terminated block +// that CreateProcess expects. Returns nil if env is empty, which tells +// CreateProcess to inherit the parent's environment. +func createEnvBlock(env []string) ([]uint16, error) { + if len(env) == 0 { + return nil, nil + } + var block []uint16 + for _, s := range env { + utf16s, err := windows.UTF16FromString(s) + if err != nil { + return nil, err + } + block = append(block, utf16s...) + } + block = append(block, 0) + return block, nil } diff --git a/pkg/gui/pty.go b/pkg/gui/pty.go index e5ef2a770..1a774fc3d 100644 --- a/pkg/gui/pty.go +++ b/pkg/gui/pty.go @@ -1,7 +1,6 @@ package gui import ( - "errors" "fmt" "io" "os" @@ -99,9 +98,7 @@ func (gui *Gui) newPtyTask(view *gocui.View, cmd *exec.Cmd, prefix string) error cols, rows := gui.desiredPtySize(view) sp, err := oscommands.StartPty(cmd, cols, rows) if err != nil { - if !errors.Is(err, oscommands.ErrPtyUnsupported) { - gui.c.Log.Error(err) - } + gui.c.Log.Error(err) return tasks.ExecCmd{Cmd: cmd}, nil } p = sp.Pty