extend process spawn to allow allocating a tty, attaching #96
Conversation
Mesa Descriptionusing the same approach that
demo running against the image running in unikraft: https://screen.studio/share/6imUEYCv Note This PR adds the ability to spawn processes with a pseudo-terminal (PTY) and attach to them for interactive sessions, similar to
Written by Cursor Bugbot for commit 55adbad. This will update automatically on new commits. Configure here. Description generated by Mesa. Update settings |
![mesa-dot-dev[bot]](https://avatars.githubusercontent.com/in/1050077?s=60&v=4){kind=small}
There was a problem hiding this comment.
Performed full review of 1d2d45e...7d69152
Analysis
-
Goroutine and Connection Leaks: The
HandleProcessAttachfunction creates multiple goroutines for I/O copying without proper cleanup mechanisms. There's no context-based cancellation, connection timeouts, or tracking to prevent resource exhaustion when clients disconnect improperly. -
PTY Process Cleanup Issues: If
pty.Start(cmd)fails, there's no guaranteed cleanup of potentially started processes, which could lead to zombie process leaks. -
Security Vulnerabilities: The
/attachendpoint lacks proper authentication beyond process ID validation, has no permission validation for process attachment, and allows multiple clients to attach to the same PTY which could cause output duplication and potential DoS attacks. -
Client Timeout Problems: While the client implementation handles terminal modes and resize events properly, it has indefinite blocking for I/O operations without proper timeout handling.
Tip
Help
Slash Commands:
/review- Request a full code review/review latest- Review only changes since the last review/describe- Generate PR description. This will update the PR body or issue comment depending on your configuration/help- Get help with Mesa commands and configuration options
9 files reviewed | 0 comments | Edit Agent Settings • Read Docs
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
|
||
| // Pipe: client -> process | ||
| go func() { | ||
| _, _ = io.Copy(processRW, clientConn) |
There was a problem hiding this comment.
Bug: Hijack Loses Buffered Data
After hijacking the HTTP connection, the code reads from the raw clientConn instead of the buffered reader buf returned by Hijack(). If the HTTP server's buffered reader read ahead and buffered client data before the hijack, that data will be lost. Reading from buf ensures any buffered data is consumed first before reading from the underlying connection.
| if !h.isTTY || h.ptyFile == nil { | ||
| return oapi.ProcessResize400JSONResponse{BadRequestErrorJSONResponse: oapi.BadRequestErrorJSONResponse{Message: "process is not PTY-backed"}}, nil | ||
| } | ||
| ws := &pty.Winsize{Rows: uint16(rows), Cols: uint16(cols)} |
There was a problem hiding this comment.
Bug: Silent Overflow Corrupts Terminal Size
In ProcessResize, the rows and cols values are converted from int to uint16 without checking for overflow. Values greater than 65535 will silently wrap around, causing incorrect terminal dimensions. For example, a value of 65537 becomes 1 after conversion. The function should validate that both values are within the valid uint16 range (1-65535) before conversion.
| cols = uint16(*request.Body.Cols) | ||
| } | ||
| if rows > 0 && cols > 0 { | ||
| _ = pty.Setsize(ptyFile, &pty.Winsize{Rows: rows, Cols: cols}) |
There was a problem hiding this comment.
Bug: Silent Overflow Corrupts Terminal Dimensions
In ProcessSpawn, when allocating a PTY, the initial rows and cols values are converted from int to uint16 without checking for overflow. Values greater than 65535 will silently wrap around, causing incorrect initial terminal dimensions. The code should validate that both values are within the valid uint16 range before conversion.
Sayan-
left a comment
Sayan-
left a comment
There was a problem hiding this comment.
overall lgtm! nothing blocking ^^
| // Raw attach endpoint (HTTP hijack) - not part of OpenAPI spec | ||
| r.Get("/process/{process_id}/attach", func(w http.ResponseWriter, r *http.Request) { | ||
| id := chi.URLParam(r, "process_id") | ||
| apiService.HandleProcessAttach(w, r, id) | ||
| }) |
There was a problem hiding this comment.
from my understanding these routes will keep using all the router's middlewares. does that match yours? specifically curious on the stz implications
| _, _ = io.Copy(processRW, clientConn) | ||
| shutdown() |
There was a problem hiding this comment.
nice how simple this one gets to be
using the same approach that
docker exec ...uses:./cmd/shellcontains a proof of concept for what would eventually land in thekernelCLI.demo running against the image running in unikraft:
https://screen.studio/share/6imUEYCv
Note
Adds PTY/TTY support to process spawn with HTTP hijack attach and resize endpoints, updates OpenAPI/client, and introduces a shell CLI PoC to interactively attach.
POST /process/spawncan allocate a PTY (allocate_tty,rows,cols), switches to PTY I/O paths, and avoids stdout/stderr readers in PTY mode.GET /process/{process_id}/attachfor raw TCP attach with single-session guard and safe PTY->conn copy using readiness polling.POST /process/{process_id}/resizeendpoint to change PTY size; OpenAPI and generated client/server updated.server/cmd/shell: spawns/bin/bashwith TTY, HTTP/1.1 raw-hijack attach, terminal raw mode, SIGWINCH-driven resize; supports HTTP/HTTPS.HOSTNAMEearly (headful/headless).github.com/creack/pty,golang.org/x/sys,golang.org/x/term.Written by Cursor Bugbot for commit b4ff4e0. This will update automatically on new commits. Configure here.