Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds Unix PTY-backed remote exec: dependencies and server config, protocol variants and tests, tunnel readiness/Exit handling, server PTY relay, client terminal/raw-mode I/O, CLI RemoteExec wiring and hot-reload, plus supporting docs. ChangesRemote Exec Feature
Sequence DiagramsequenceDiagram
participant User as User (Client)
participant ClientExec as exec::run()
participant Tunnel as Tunnel (SSE)
participant Server as server handler
participant PTY as PTY & Child
User->>ClientExec: start remote exec (tunnel, cmd)
ClientExec->>Tunnel: Message::Exec(cmd, cols, rows)
Tunnel->>Server: HTTP POST (encrypted)
Server->>Server: check hot.allow_exec
Server->>PTY: create pty & spawn child
Server-->>Tunnel: encrypted ACK
Tunnel-->>ClientExec: ACK
ClientExec->>ClientExec: enter raw mode
User->>ClientExec: stdin bytes
ClientExec->>Tunnel: Message::Data(conn_id, bytes)
Tunnel->>Server: forward Data
Server->>PTY: write to child stdin
PTY-->>Server: child stdout bytes
Server->>Tunnel: Message::Data(conn_id, bytes)
Tunnel->>ClientExec: Data -> stdout
ClientExec->>User: display output
User->>ClientExec: resize signal
ClientExec->>Tunnel: Message::Resize(conn_id, cols, rows)
Tunnel->>Server: apply resize
PTY->>Server: child exits(code)
Server->>Tunnel: Message::ExitStatus(conn_id, code)
Tunnel->>ClientExec: TunnelEvent::Exit(code)
ClientExec->>ClientExec: restore terminal, send Close
ClientExec->>User: exit(code)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a remote command execution feature (tunnix remote-exec) using PTYs, adding a new CLI command, configuration options, and PTY-based data relaying on the server. The review feedback highlights several critical issues: potential compilation failures on Windows due to Unix-specific APIs and commands that require conditional compilation, a potential deadlock in the PTY relay if background processes keep the slave PTY open, a connection registration leak on early errors in exec::run, high lock contention from locking the session mutex on every data chunk, and a lack of Windows shell fallbacks for command execution.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/reload.rs (1)
143-159:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve
--allow-execacross hot reloads.Line 158 always reapplies the file value, so a server started with
--allow-execwill silently revert on the next reload. This should follow the other CLI override semantics and ignore file changes when the flag was supplied.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/reload.rs` around lines 143 - 159, The reload logic currently always applies the file's sc.allow_exec value, reverting any CLI-provided flag; update the logic so CLI overrides take precedence: when comparing sc.allow_exec != current.allow_exec, if the CLI supplied the flag (i.e., current.allow_exec reflects a CLI override) do not push "allow_exec" into changed and do not use sc.allow_exec when constructing the new HotServerConfig; instead set allow_exec in the HotServerConfig to current.allow_exec (preserving the CLI choice). Ensure the check around changed.push("allow_exec") and the HotServerConfig field assignment use this rule so file changes are ignored when the flag was supplied.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/exec.rs`:
- Line 80: The code currently initializes let mut exit_code = 0 and returns it
even when no ExitStatus was received, turning transport/server failures into
success; change this by introducing a sentinel (e.g., Option<i32> observed_exit)
or a boolean (e.g., saw_exit_status) alongside exit_code in the same scope, set
it only when you actually observe an ExitStatus in the loop that handles process
events, and replace any early/closure/error return paths (the branches that
return exit_code when the tunnel closes or errors before an ExitStatus) to
instead return an Err or a non-zero fallback when observed is None/false; update
all places that currently return exit_code (including the tunnel-closed and
error branches) to check the new observed flag and convert to a proper error or
a defined non-zero code if no real exit was seen.
In `@src/main.rs`:
- Around line 330-333: The code builds cmd by flattening ra.cmd with a raw space
join (let cmd = if ra.cmd.is_empty() { None } else { Some(ra.cmd.join(" ")) }),
which loses argument boundaries and will break commands with spaces or
metacharacters when executed via /bin/sh -c; fix it by either 1) properly
shell-escaping each element of ra.cmd before joining (use a robust escape
function that wraps/escapes items for POSIX sh) so the resulting String
preserves arguments when passed to /bin/sh -c, or 2) better, change the
protocol/handler to transmit ra.cmd as a Vec<String> and spawn the process
directly without /bin/sh -c (preserve argv and use
Command::new(&cmd0).args(&argv[1..]) or equivalent). Update the code that
constructs and uses the cmd variable and the execution path that invokes /bin/sh
-c to use one of these approaches.
- Line 3: The build exposes a Unix-only module and subcommand unconditionally:
remove or gate the unconditional mod exec; and Command::RemoteExec export and
instead add cfg gating or a non-Unix stub to avoid failing on non-Unix targets.
Specifically, wrap the module declaration and any uses/variants of
Command::RemoteExec with #[cfg(unix)] (and #[cfg(not(unix))] for a stub
implementation in src/exec.rs), or provide a platform-specific stub module that
implements the same public API but returns a clear error on non-Unix; update
references to exec::RemoteExec / Command::RemoteExec so they compile under both
cfg branches.
In `@src/server.rs`:
- Around line 664-679: The PTY child can outlive crashed clients because the
current tokio::select only kills the child when write_rx actually closes
(write_blocking path), but a crashed client can leave its sender parked in
sess.tcp_writers so write_rx never closes; update the shutdown logic to detect
abandoned sessions — for example, wrap the child wait/select with a timeout
(tokio::time::timeout) or add a heartbeat/inactivity check that observes
sess.tcp_writers and forces killer.kill() when the writer has been idle or
removed; specifically modify the code around child.clone_killer, wait_handle,
and the write_blocking/select block to fall back to a forced kill when the
timeout/heartbeat indicates the client is gone.
- Line 391: The current info! log prints the full user-supplied command via cmd
which may expose secrets; change the info! call in server.rs to only log conn_id
and whether it was a shell or command (e.g., use cmd.is_some() to emit "cmd" vs
"<shell>") along with cols and rows, and move the full command string into a
debug! log (or redact it) using cmd.as_deref() only at debug level so sensitive
data is not present in normal logs; update the log invocation that currently
uses info!("[{}] EXEC {} ({}x{})", conn_id, cmd.as_deref().unwrap_or("<shell>"),
cols, rows) accordingly.
---
Outside diff comments:
In `@src/reload.rs`:
- Around line 143-159: The reload logic currently always applies the file's
sc.allow_exec value, reverting any CLI-provided flag; update the logic so CLI
overrides take precedence: when comparing sc.allow_exec != current.allow_exec,
if the CLI supplied the flag (i.e., current.allow_exec reflects a CLI override)
do not push "allow_exec" into changed and do not use sc.allow_exec when
constructing the new HotServerConfig; instead set allow_exec in the
HotServerConfig to current.allow_exec (preserving the CLI choice). Ensure the
check around changed.push("allow_exec") and the HotServerConfig field assignment
use this rule so file changes are ignored when the flag was supplied.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cee0e9ea-4ae0-4df7-b7de-ea2fed4d7ce8
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
Cargo.tomlconfig.example.tomlsrc/config.rssrc/exec.rssrc/main.rssrc/protocol.rssrc/relay.rssrc/reload.rssrc/server.rssrc/tunnel.rs
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0fc6e2c26b
ℹ️ 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".
|
/gemini review |
…processing first data frame
There was a problem hiding this comment.
Code Review
This pull request introduces a remote command execution feature (tunnix remote-exec) that allows running commands or interactive shells on the server via a PTY. It includes configuration options to enable this feature, terminal raw mode management, resize handling, and improved client-side SSE connection synchronization. The review feedback highlights two key improvements: implementing a retry mechanism when sending PTY data over a temporarily disconnected SSE stream to prevent data loss, and querying stdout/stderr as fallbacks in terminal_size() to correctly detect terminal dimensions when stdin is redirected.
… SSE output reliability during client reconnects
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a remote command execution feature (tunnix remote-exec) using a PTY, allowing users to run interactive shells or commands on the server. Feedback on these changes highlights three key issues: abrupt client disconnections can cause remote processes and PTY relay tasks to leak and hang on the server; joining command arguments with a space loses argument boundaries and should instead use a shell-quoting library like shlex; and restricting the EOF character transmission to when input has been sent causes commands with empty stdin to hang indefinitely.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/exec.rs`:
- Around line 99-115: The current EOF handling in the stdin read branch skips
sending an EOF if sent_input is false, which causes remote commands reading
until EOF to hang; update the match arm inside the tokio::select! branch (the
stdin.read(&mut stdin_buf) handling) so that when Ok(0) is returned you always
send a terminal EOF/close signal to the remote (e.g. reuse Message::Data {
conn_id, data: vec![0x04] } or a dedicated close/EOF message) via
tunnel.send_message(&eof).await even if sent_input is false, or alternatively
send a specific stdin-close message if the protocol supports it; ensure
sent_input is not used to suppress the EOF/close signal in that branch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1a55f014-bf61-4dea-be5a-c25e5b295c1e
📒 Files selected for processing (3)
src/exec.rssrc/server.rssrc/tunnel.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/tunnel.rs
- src/server.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 82e36e0703
ℹ️ 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".
- server: add an SSE-dead watchdog to relay_pty_connection so an abrupt
client disconnect no longer orphans the child process. The watchdog
polls Session::sse_tx.is_closed() every 2s; once the SSE receiver is
gone (network drop, process killed, tunnel gone) it kills the child
and joins the wait handle. Previously write_blocking would block on
write_rx forever and the child would run indefinitely.
- server: tunnel sse_ready now also fires on the first event during an
existing-session reconnect (not just on a data: frame). The first
event on a reconnect is typically the keepalive comment, so
send_message's retry path was burning the full 10s RECONNECT_WAIT
before retrying the POST. Initial connect still gates on the first
data: frame so a new-session Reset is processed before
register_connection lands.
- exec: wrap session() body in an inner async block so Message::Close
is sent to the server on every exit path (including ? early returns
on stdin send failure and TunnelEvent::Error). Without this, a
tunnel error during the session would leave the server's PTY and
child process running.
- exec: always send the terminal EOF byte (0x04) on local stdin EOF,
even when no input was forwarded. Gating on sent_input made
'tunnix remote-exec cat </dev/null' and similar commands hang
forever waiting for input to close.
- main: use shlex::try_join to forward remote-exec arguments so spaces
and quotes inside args survive the trip through the server's
'sh -c'. Plain join(" ") re-tokenized args, so 'tunnix remote-exec
echo "a b" "c"' would reach the server as four args.
- reload: annotate danger_accept_invalid_certs(true) as intentional
(corporate TLS-inspecting proxy deployments rely on it). Wiki
enriched-context gains an 'Intentional security trade-offs' section
to keep future maintainers from 'fixing' it in a security pass.
|
/gemini review |
|
@codex review |
There was a problem hiding this comment.
Code Review
This pull request introduces a remote command execution and interactive shell feature (tunnix remote-exec) using PTY allocation. Key changes include adding the RemoteExec subcommand, introducing new protocol messages (Exec, Resize, ExitStatus), implementing server-side PTY spawning and relaying, and adding client-side terminal raw mode handling. The review feedback highlights three main areas for improvement: making the nix dependency platform-specific in Cargo.toml to prevent breaking Windows compilation, adding platform-specific shell support (e.g., cmd.exe on Windows) in the server's execution logic, and decoupling stdin reading from network sending in src/exec.rs using a background task to prevent terminal lag and freezing.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 97e9bb06a9
ℹ️ 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".
…ing) - main: pass single-arg `cmd` verbatim to the server's `sh -c`. shlex would otherwise wrap strings like `echo $HOME && id` in single quotes, making the shell look for a literal command named that. - exec: track whether the last forwarded stdin byte was '\n'. On local EOF, send '\n' + 0x04 (not just 0x04) when the line is unterminated, so canonical-mode consumers (cat, read, ...) see real EOF and `printf abc | remote-exec cat` no longer hangs. - exec: move all outbound messages (stdin data, EOF, resize) through a background sender task fed by an mpsc. The main select! only enqueues, so a slow POST /send can no longer stall stdout writes or SIGWINCH handling. The channel is dropped and the sender joined before sending Close, so the final Close is always the last message on the wire. - server: gate `Message::Exec` / `Message::Resize`, the `pty_masters` field, the `portable_pty` import, the `relay_pty_connection` fn, and the new `forward_pty_chunk` helper behind `#[cfg(unix)]`. On non-unix, Exec returns an encrypted `Error` and Resize is a no-op — PTY support is intentionally Unix-only. - server: on child exit, signal the forward task via `Notify` so it does a final `try_recv` flush of the PTY output channel and exits, rather than waiting forever for the channel to close (it never does when a backgrounded process inherited the slave PTY). The 2s drain budget is preserved; the encrypt+send retry path is extracted into `forward_pty_chunk` and shared by the live and drain paths. - cargo: move `portable-pty` and `nix` under `[target.'cfg(unix)'.dependencies]`. `libc` stays top-level.
|
/gemini review |
|
@codex review |
There was a problem hiding this comment.
Code Review
This pull request introduces remote command execution and interactive shell support (tunnix remote-exec) over the tunnel using a PTY on Unix-based servers. Key changes include the addition of a client-side execution module (src/exec.rs), server-side PTY spawning and relaying logic (src/server.rs), and configuration options to safely enable or disable this feature. Feedback on these changes highlights two main issues: first, a potential thread leak and blocking thread pool exhaustion in the PTY reader if a background process inherits the slave PTY, which can be resolved using tokio::io::unix::AsyncFd; second, a conflict where the watchdog immediately kills the child process during transient disconnects, bypassing the intended five-second reconnect tolerance.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a2c22cb92
ℹ️ 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".
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Cargo.toml (1)
64-64: <!-- [scratchpad_start]
[task review_file_1/1]
[observations]
Cargo.toml: line 64 has
shlex = "1.3".Cargo.toml: line 66-68 comment says "Remote exec (PTY) — Unix only", but
libc = "0.2"is in root[dependencies](line 68).Cargo.toml: line 70 starts
[target.'cfg(unix)'.dependencies]withportable-pty = "0.9"andnix = { version = "0.29", features = ["term"] }.Cargo.lock: package
shlexresolves toversion = "1.3.0"with checksum.GitHub advisory query for
shlexreturns LOW severity issues withvulnerableVersionRange: "< 1.3.0"andfirstPatchedVersion.identifier: "1.3.0".src/main.rs:
mod exec;andRemoteExecare gated behind#[cfg(unix)].src/exec.rs: contains
libc::STDIN_FILENO/libc::winsize/libc::ioctlusages (no further cfg on those lines).
[analysis]
shlex = "1.3"is safe relative to the reported advisory because Cargo.lock pins to 1.3.0, which is the first patched version for the< 1.3.0range.The
libcplacement isn’t a functional gating bug (libc is only used from the#[cfg(unix)]exec module), but it does contradict the manifest comment and prevents matching the "Unix only" intent at the dependency level.
[done]
[ scratchpad_end ] -->
Align Unix-only dependency intent
shlex = "1.3"resolves toshlex 1.3.0inCargo.lock; the reportedshlexsecurity advisory affectsshlex < 1.3.0and lists1.3.0as the first patched version (LOW), so the pinned version is covered.
Cargo.tomlkeepslibc = "0.2"in root[dependencies]even though it’s described as “Unix only”; sincelibcis only used fromsrc/exec.rswhich is compiled under#[cfg(unix)], this is mostly a manifest/intent mismatch—movelibcinto[target.'cfg(unix)'.dependencies]if you want to avoid pulling it for non-Unix builds and match the comment.Consider checking advisories for
portable-pty/nixas well.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Cargo.toml` at line 64, Cargo.toml currently declares libc in the root [dependencies] even though it’s only used by the Unix-gated exec module (mod exec / RemoteExec); move the libc = "0.2" entry into the existing [target.'cfg(unix)'.dependencies] section alongside portable-pty and nix so the dependency is only pulled for Unix builds, then run a full cargo build/test to verify src/exec.rs (which references libc::STDIN_FILENO, libc::winsize, libc::ioctl) still compiles under cfg(unix); also re-check advisories for portable-pty and nix while you’re updating the manifest.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Cargo.toml`:
- Around line 66-68: The dependency "libc" is currently in the root
[dependencies] section but must be compiled only on Unix; remove the line `libc
= "0.2"` from the main dependencies and add it under the Unix-specific Cargo
section by placing it in [target.'cfg(unix)'.dependencies] so the crate is only
enabled on Unix targets (reference the dependency name "libc" and the target
section "[target.'cfg(unix)'.dependencies]").
---
Nitpick comments:
In `@Cargo.toml`:
- Line 64: Cargo.toml currently declares libc in the root [dependencies] even
though it’s only used by the Unix-gated exec module (mod exec / RemoteExec);
move the libc = "0.2" entry into the existing [target.'cfg(unix)'.dependencies]
section alongside portable-pty and nix so the dependency is only pulled for Unix
builds, then run a full cargo build/test to verify src/exec.rs (which references
libc::STDIN_FILENO, libc::winsize, libc::ioctl) still compiles under cfg(unix);
also re-check advisories for portable-pty and nix while you’re updating the
manifest.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 98bd859e-5158-4420-8067-74b598f1673c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.tomlsrc/exec.rssrc/main.rssrc/reload.rssrc/server.rssrc/tunnel.rswiki/enriched-context.md
✅ Files skipped from review due to trivial changes (1)
- wiki/enriched-context.md
🚧 Files skipped from review as they are similar to previous changes (5)
- src/main.rs
- src/reload.rs
- src/exec.rs
- src/tunnel.rs
- src/server.rs
There was a problem hiding this comment.
Code Review
This pull request introduces a remote command execution feature (remote-exec) to tunnix, allowing users to run commands or interactive shells over the tunnel. It adds PTY allocation and management on Unix platforms, introduces new protocol messages (Exec, Resize, ExitStatus), and updates the server to handle these requests when explicitly enabled. The code review feedback highlights a potential memory exhaustion (OOM) risk in src/exec.rs due to the use of an unbounded channel for stdin streaming without backpressure, recommending a bounded channel and a separate task instead. Additionally, reviewers noted that a failure in try_clone_reader within src/server.rs could cause the client to hang indefinitely, suggesting that an error message should be sent to the client before returning.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server.rs (1)
640-654:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftFix:
spawn_blockingmakes your 2s EOF-timeout cleanup ineffective (can hang)Moving the PTY reader to
tokio::task::spawn_blockingfixes the WouldBlock busy-spin, but it breaks the timeout cleanup:JoinHandle::abort()does not cancel aspawn_blockingclosure that’s already running, andread_task.awaitwill wait until the blockingreader.readreturns rather than returning a cancelled JoinError.So the scenario your comment claims to handle (child keeps the PTY slave open →
reader.readblocks forever) is not actually handled:
read_task.abort()won’t unblock or stop the blocking read.- The dup’d fd won’t be closed as part of cancellation, and the blocking thread can remain stuck.
- Cleanup after the 2s timeout can hang, preventing
ExitStatus/Closeand leaving session state entries around.The read loop (
Ok(0)/Ok(n)/Err→break+blocking_send) is fine; the cleanup contract needs to actively unblockreader.read(e.g., close the underlying dup’d fd on timeout, or use a nonblocking/AsyncFd-driven read that can be cancelled by closing/unregistering).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server.rs` around lines 640 - 654, The blocking PTY read spawned with tokio::task::spawn_blocking (the closure using reader.read and pty_out_tx, assigned to read_task) cannot be cancelled by read_task.abort(), so the 2s timeout cleanup can hang; fix by making the reader cancelable: either switch to a non-blocking/AIO approach (use tokio::io::unix::AsyncFd or set O_NONBLOCK on the dup'd FD and use async reads so the task can be aborted) and perform reads on the async handle instead of spawn_blocking, or, if you must use spawn_blocking, ensure the timeout path actively closes the dup'd file descriptor (the underlying FD held by reader) before awaiting/aborting read_task so reader.read returns and the thread exits; update the code paths around spawn_blocking, reader, read_task, and the timeout/cleanup logic to implement one of these two approaches.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/server.rs`:
- Around line 640-654: The blocking PTY read spawned with
tokio::task::spawn_blocking (the closure using reader.read and pty_out_tx,
assigned to read_task) cannot be cancelled by read_task.abort(), so the 2s
timeout cleanup can hang; fix by making the reader cancelable: either switch to
a non-blocking/AIO approach (use tokio::io::unix::AsyncFd or set O_NONBLOCK on
the dup'd FD and use async reads so the task can be aborted) and perform reads
on the async handle instead of spawn_blocking, or, if you must use
spawn_blocking, ensure the timeout path actively closes the dup'd file
descriptor (the underlying FD held by reader) before awaiting/aborting read_task
so reader.read returns and the thread exits; update the code paths around
spawn_blocking, reader, read_task, and the timeout/cleanup logic to implement
one of these two approaches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 18a27b03-a4f0-4c8c-8849-a8feab498ed3
📒 Files selected for processing (2)
src/server.rssrc/tunnel.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/tunnel.rs
…message to session
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a remote command execution (remote-exec) feature to tunnix, allowing users to run commands or open interactive shells on the server via a pseudo-terminal (PTY) on Unix platforms. Key feedback highlights a cancel-safety issue in src/exec.rs when reading directly from tokio::io::Stdin within a select! block, which can be resolved by forwarding input through an mpsc channel. Additionally, a thread leak risk was identified in src/server.rs because read_task.abort() does not terminate blocking spawn_blocking tasks, and a defensive check was recommended to handle an empty SHELL environment variable by defaulting to /bin/sh.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
…on to ensure proper cleanup
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a remote command execution (remote-exec) feature to tunnix, allowing users to run commands or open interactive shells on a server via a pseudo-terminal (PTY). The changes include new CLI subcommands, configuration options to safely opt-in to this feature, protocol updates, and PTY stream relaying logic. The review comments identify two critical issues: first, a lack of backpressure on stdin in src/exec.rs due to an unbounded channel, which could lead to high memory usage; second, a concurrency risk in src/server.rs where a MutexGuard is held across an .await point during error handling, potentially causing deadlocks or starvation.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| let (msg_tx, mut msg_rx) = tokio::sync::mpsc::unbounded_channel::<Message>(); | ||
| let sender_tunnel = tunnel.clone(); | ||
| let sender_task = tokio::spawn(async move { | ||
| while let Some(msg) = msg_rx.recv().await { | ||
| if let Err(e) = sender_tunnel.send_message(&msg).await { | ||
| debug!("send_message failed: {}", e); | ||
| break; | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| let mut stdin = tokio::io::stdin(); | ||
| let mut stdout = tokio::io::stdout(); | ||
| let mut stdin_buf = vec![0u8; 8192]; | ||
| let mut stdin_open = true; | ||
| let mut winch = signal(SignalKind::window_change())?; | ||
| let mut exit_code = 0; | ||
| // Only a real ExitStatus from the server makes the command's exit code | ||
| // meaningful. If the tunnel drops or errors first, we must not report 0. | ||
| let mut saw_exit = false; | ||
| // Track whether the last forwarded byte was '\n'. On local stdin EOF, | ||
| // canonical-mode consumers (cat, read, ...) need a real EOF, not just a | ||
| // VEOF flush of an unterminated pending line. If the line is still | ||
| // buffered (no trailing newline), one Ctrl-D only delivers the partial | ||
| // line; the consumer then waits for more input forever. We send | ||
| // '\n' + 0x04 in that case so the line is delivered AND the next read | ||
| // sees EOF. | ||
| let mut last_was_newline = true; | ||
|
|
||
| loop { | ||
| tokio::select! { | ||
| n = stdin.read(&mut stdin_buf), if stdin_open => { | ||
| match n { | ||
| Ok(0) => { | ||
| // Local stdin hit EOF. Send a real EOF to the | ||
| // remote PTY so canonical-mode consumers terminate. | ||
| let mut eof_data = Vec::with_capacity(2); | ||
| if !last_was_newline { | ||
| // Terminate the pending line so the VEOF below | ||
| // can actually signal EOF on an empty buffer. | ||
| eof_data.push(b'\n'); | ||
| } | ||
| eof_data.push(0x04); | ||
| let _ = msg_tx.send(Message::Data { conn_id, data: eof_data }); | ||
| stdin_open = false; | ||
| } | ||
| Ok(n) => { | ||
| last_was_newline = stdin_buf[n - 1] == b'\n'; | ||
| let msg = Message::Data { conn_id, data: stdin_buf[..n].to_vec() }; | ||
| if msg_tx.send(msg).is_err() { | ||
| anyhow::bail!("sender channel closed"); | ||
| } | ||
| } | ||
| Err(e) => { | ||
| debug!("stdin read error: {}", e); | ||
| stdin_open = false; | ||
| } | ||
| } | ||
| } | ||
| event = event_rx.recv() => { | ||
| match event { | ||
| Some(TunnelEvent::Data(data)) => { | ||
| if stdout.write_all(&data).await.is_err() { | ||
| break; | ||
| } | ||
| let _ = stdout.flush().await; | ||
| } | ||
| Some(TunnelEvent::Exit(code)) => { | ||
| exit_code = code; | ||
| saw_exit = true; | ||
| } | ||
| Some(TunnelEvent::Error(msg)) => { | ||
| anyhow::bail!("remote error: {}", msg); | ||
| } | ||
| Some(TunnelEvent::Close) | None => break, | ||
| } | ||
| } | ||
| _ = winch.recv() => { | ||
| let (cols, rows) = terminal_size(); | ||
| let _ = msg_tx.send(Message::Resize { conn_id, cols, rows }); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Close the sender channel and wait for it to flush all queued | ||
| // messages. This guarantees the final Message::Close below is the | ||
| // last thing sent on the wire for this conn_id. | ||
| drop(msg_tx); | ||
| let _ = sender_task.await; |
There was a problem hiding this comment.
Unbounded Channel and Lack of Backpressure on Stdin
Using an unbounded channel (unbounded_channel) for forwarding stdin data to the network sender task can lead to unbounded memory growth and potential Out-Of-Memory (OOM) crashes. If a user pipes a large file or a continuous stream into tunnix remote-exec (e.g., cat large_file | tunnix remote-exec ...), stdin.read will read the data as fast as possible and buffer it in the unbounded channel, while the network sender task is bottlenecked by the slower HTTP POST requests.
To implement proper backpressure, we can:
- Use a bounded channel (e.g.,
mpsc::channel(100)). - Spawn a separate async task to read from
stdinand forward to the channel. This naturally suspends reading fromstdinwhen the channel is full, propagating backpressure. - Abort the
stdintask when the main loop exits to ensure clean shutdown.
let (msg_tx, mut msg_rx) = tokio::sync::mpsc::channel::<Message>(100);
let sender_tunnel = tunnel.clone();
let sender_task = tokio::spawn(async move {
while let Some(msg) = msg_rx.recv().await {
if let Err(e) = sender_tunnel.send_message(&msg).await {
debug!("send_message failed: {}", e);
break;
}
}
});
let stdin_sender = msg_tx.clone();
let stdin_task = tokio::spawn(async move {
let mut stdin = tokio::io::stdin();
let mut stdin_buf = vec![0u8; 8192];
let mut last_was_newline = true;
loop {
match stdin.read(&mut stdin_buf).await {
Ok(0) => {
let mut eof_data = Vec::with_capacity(2);
if !last_was_newline {
eof_data.push(b'\n');
}
eof_data.push(0x04);
let _ = stdin_sender.send(Message::Data { conn_id, data: eof_data }).await;
break;
}
Ok(n) => {
last_was_newline = stdin_buf[n - 1] == b'\n';
if stdin_sender.send(Message::Data { conn_id, data: stdin_buf[..n].to_vec() }).await.is_err() {
break;
}
}
Err(_) => break,
}
}
});
let mut stdout = tokio::io::stdout();
let mut winch = signal(SignalKind::window_change())?;
let mut exit_code = 0;
let mut saw_exit = false;
loop {
tokio::select! {
event = event_rx.recv() => {
match event {
Some(TunnelEvent::Data(data)) => {
if stdout.write_all(&data).await.is_err() {
break;
}
let _ = stdout.flush().await;
}
Some(TunnelEvent::Exit(code)) => {
exit_code = code;
saw_exit = true;
}
Some(TunnelEvent::Error(msg)) => {
anyhow::bail!("remote error: {}", msg);
}
Some(TunnelEvent::Close) | None => break,
}
}
_ = winch.recv() => {
let (cols, rows) = terminal_size();
let _ = msg_tx.try_send(Message::Resize { conn_id, cols, rows });
}
}
}
stdin_task.abort();
let _ = stdin_task.await;
drop(msg_tx);
let _ = sender_task.await;
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/reload.rs`:
- Around line 43-47: The comment next to .danger_accept_invalid_certs(true)
understates the scope: that call disables certificate validation for all TLS
connections created by this client (a global TLS validation bypass), and
hostname verification is a separate option controlled by
danger_accept_invalid_hostnames(true); update the comment to explicitly state it
disables certificate chain validation globally and the security implications,
and either gate this call behind an explicit opt-in config/feature flag or
change behavior to only disable hostname checks if that was intended (reference
.danger_accept_invalid_certs(true) and .danger_accept_invalid_hostnames(true) to
locate the client builder).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2a5485f0-726a-4fac-9169-eaa989f99091
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
Cargo.tomlconfig.example.tomlsrc/config.rssrc/exec.rssrc/main.rssrc/protocol.rssrc/relay.rssrc/reload.rssrc/server.rssrc/tunnel.rswiki/enriched-context.md
…r session cleanup and message transmission
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements a remote command execution (remote-exec) feature for Unix platforms, allowing clients to run commands or open interactive shells on the server via a pseudo-terminal (PTY). The changes introduce new protocol messages (Exec, Resize, ExitStatus), configuration options to toggle execution permissions, and the necessary client and server relay logic. The review feedback highlights critical improvement opportunities, including a thread leak risk in src/server.rs when aborting spawn_blocking tasks, a cancellation leak risk in src/exec.rs that could bypass connection unregistration, and a potential spawn failure if the SHELL environment variable is set but empty.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
…O and improve cleanup on teardown
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a remote command execution feature (remote-exec) to tunnix, allowing users to run commands or open interactive shells on a remote server over the tunnel using a PTY. The feedback suggests monitoring the background sender task in exec.rs to prevent client hangs if it exits prematurely. Additionally, the reviewer recommends dynamically forwarding the client's local TERM environment variable to the server instead of hardcoding it, which involves updating the Message::Exec protocol, client-side execution, server-side PTY configuration, and unit tests.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| break; | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
Pin the sender_task so that we can safely poll it in the select! loop. This allows us to detect immediately if the background sender task fails, preventing the client from hanging in a half-dead state.
});
tokio::pin!(sender_task);| loop { | ||
| tokio::select! { |
There was a problem hiding this comment.
Add the sender_task to the select! loop to immediately detect if the background sender task exits prematurely (e.g., due to a persistent network failure or tunnel error).
loop {
tokio::select! {
res = &mut sender_task => {
anyhow::bail!("sender task exited prematurely");
}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server.rs (1)
443-450:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKill the PTY child on post-spawn setup failures.
spawn_commandhas already succeeded before both of these branches. If either one returns early, the request fails but the child is never explicitly terminated, so a command that ignores PTY hangup can keep running on the server.🛠️ Minimal fix
let writer = match master.take_writer() { Ok(w) => w, Err(e) => { + let mut killer = child.clone_killer(); + let _ = killer.kill(); error!("[{}] take writer failed: {}", conn_id, e); return encrypted_response( &hot.crypto, @@ let (read_afd, write_afd) = match setup { Ok(pair) => pair, Err(e) => { + let mut killer = child.clone_killer(); + let _ = killer.kill(); error!("[{}] PTY async setup failed: {}", conn_id, e); let sse_tx = { let mut sess = session.lock().await;Also applies to: 668-685
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server.rs` around lines 443 - 450, The spawned PTY child (returned from spawn_command and stored in child) is not being terminated when master.take_writer or other post-spawn setup fails, allowing the child to keep running; before returning the encrypted_response in these error branches (e.g., the master.take_writer Err branch and the similar branch at lines 668-685), explicitly terminate the child by calling its kill routine and awaiting/handling its exit (e.g., call child.kill() and then wait on the child/handle to reap it) so the process is not left running; apply the same change in both error branches and reference the spawn_command result (child) and the encrypted_response return sites when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/server.rs`:
- Around line 668-685: The PTY setup failure path currently only sends
Message::Error and returns, leaving the client waiting after the earlier exec
ACK; update the Err(e) branch to also send the terminal shutdown sequence by
creating and sending a Message::ExitStatus (with the conn_id and appropriate
exit code) and a Message::Close (or Close equivalent) after sending
Message::Error, using the same crypto.encrypt(...) and sse_tx.send(...) flow
(reusing conn_id, sse_tx and msg.to_bytes()) so the client receives the
ExitStatus/Close pair and the local remote-exec session can clean up; ensure you
still perform the existing session.lock().await cleanup
(sess.tcp_writers.remove, sess.pty_resize.remove) before sending these final
messages.
---
Outside diff comments:
In `@src/server.rs`:
- Around line 443-450: The spawned PTY child (returned from spawn_command and
stored in child) is not being terminated when master.take_writer or other
post-spawn setup fails, allowing the child to keep running; before returning the
encrypted_response in these error branches (e.g., the master.take_writer Err
branch and the similar branch at lines 668-685), explicitly terminate the child
by calling its kill routine and awaiting/handling its exit (e.g., call
child.kill() and then wait on the child/handle to reap it) so the process is not
left running; apply the same change in both error branches and reference the
spawn_command result (child) and the encrypted_response return sites when making
the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| Err(e) => { | ||
| error!("[{}] PTY async setup failed: {}", conn_id, e); | ||
| let sse_tx = { | ||
| let mut sess = session.lock().await; | ||
| sess.tcp_writers.remove(&conn_id); | ||
| sess.pty_resize.remove(&conn_id); | ||
| sess.sse_tx.clone() | ||
| }; | ||
| let msg = Message::Error { | ||
| conn_id: Some(conn_id), | ||
| message: format!("PTY setup failed: {}", e), | ||
| }; | ||
| if let Ok(bytes) = msg.to_bytes() { | ||
| if let Ok(encrypted) = crypto.encrypt(&bytes) { | ||
| let _ = sse_tx.send(encrypted).await; | ||
| } | ||
| } | ||
| return; |
There was a problem hiding this comment.
Emit ExitStatus/Close when PTY setup fails after the exec ACK.
Line 467 already ACKs the exec as established. This failure path only sends Message::Error and returns, so the client never gets the ExitStatus/Close pair that the exec flow waits on. That can leave the local remote-exec session hanging with a stale conn_id.
🛠️ Suggested follow-up
Err(e) => {
error!("[{}] PTY async setup failed: {}", conn_id, e);
let sse_tx = {
let mut sess = session.lock().await;
sess.tcp_writers.remove(&conn_id);
sess.pty_resize.remove(&conn_id);
sess.sse_tx.clone()
};
- let msg = Message::Error {
- conn_id: Some(conn_id),
- message: format!("PTY setup failed: {}", e),
- };
- if let Ok(bytes) = msg.to_bytes() {
- if let Ok(encrypted) = crypto.encrypt(&bytes) {
- let _ = sse_tx.send(encrypted).await;
- }
- }
+ for msg in [
+ Message::Error {
+ conn_id: Some(conn_id),
+ message: format!("PTY setup failed: {}", e),
+ },
+ Message::ExitStatus { conn_id, code: -1 },
+ Message::Close { conn_id },
+ ] {
+ if let Ok(bytes) = msg.to_bytes() {
+ if let Ok(encrypted) = crypto.encrypt(&bytes) {
+ let _ = sse_tx.send(encrypted).await;
+ }
+ }
+ }
return;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server.rs` around lines 668 - 685, The PTY setup failure path currently
only sends Message::Error and returns, leaving the client waiting after the
earlier exec ACK; update the Err(e) branch to also send the terminal shutdown
sequence by creating and sending a Message::ExitStatus (with the conn_id and
appropriate exit code) and a Message::Close (or Close equivalent) after sending
Message::Error, using the same crypto.encrypt(...) and sse_tx.send(...) flow
(reusing conn_id, sse_tx and msg.to_bytes()) so the client receives the
ExitStatus/Close pair and the local remote-exec session can clean up; ensure you
still perform the existing session.lock().await cleanup
(sess.tcp_writers.remove, sess.pty_resize.remove) before sending these final
messages.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a remote-exec feature, enabling users to run commands or interactive shells on the server through a PTY. The changes include updates to the protocol, configuration, and server/client logic, alongside documentation regarding security trade-offs. I have provided feedback on optimizing performance by replacing heap allocations with stack-based buffers and using stack pinning for futures.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
/gemini review |
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
There was a problem hiding this comment.
Code Review
This pull request introduces a remote command execution (remote-exec) feature to Tunnix, allowing users to run commands or open interactive shells over the tunnel using a pseudo-terminal (PTY) on Unix systems. It adds client-side terminal handling in src/exec.rs, server-side PTY spawning and relaying in src/server.rs, and configuration/CLI support. The review feedback suggests a performance optimization during PTY session teardown: introducing a client_gone flag to skip or minimize the 50-retry loop when sending final shutdown messages to an already disconnected client, avoiding up to 10 seconds of unnecessary delays.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a remote command execution (remote-exec) feature to tunnix on Unix platforms, allowing users to run commands or open interactive shells over the tunnel. It adds a new exec module, updates the protocol with Exec, Resize, and ExitStatus messages, and implements PTY allocation and asynchronous relaying on the server. The review feedback highlights a critical compilation error in relay_pty_connection where .ok_or_else() is incorrectly called on a primitive RawFd returned by as_raw_fd(). Additionally, it is recommended to validate or restrict the SHELL environment variable when spawning the default shell on the server to prevent potential security or execution issues.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| let setup = (|| -> std::io::Result<(AsyncFd<OwnedFd>, AsyncFd<OwnedFd>)> { | ||
| let master_fd = master | ||
| .as_raw_fd() | ||
| .ok_or_else(|| std::io::Error::other("PTY master has no fd"))?; | ||
| unsafe { | ||
| let flags = libc::fcntl(master_fd, libc::F_GETFL); | ||
| if flags < 0 { | ||
| return Err(std::io::Error::last_os_error()); | ||
| } | ||
| if libc::fcntl(master_fd, libc::F_SETFL, flags | libc::O_NONBLOCK) < 0 { | ||
| return Err(std::io::Error::last_os_error()); | ||
| } | ||
| } | ||
| let dup_afd = |interest| -> std::io::Result<AsyncFd<OwnedFd>> { | ||
| let fd = unsafe { libc::dup(master_fd) }; | ||
| if fd < 0 { | ||
| return Err(std::io::Error::last_os_error()); | ||
| } | ||
| AsyncFd::with_interest(unsafe { OwnedFd::from_raw_fd(fd) }, interest) | ||
| }; | ||
| Ok((dup_afd(Interest::READABLE)?, dup_afd(Interest::WRITABLE)?)) | ||
| })(); |
There was a problem hiding this comment.
In relay_pty_connection, the code calls master.as_raw_fd() and then attempts to use .ok_or_else(...) on it. However, AsRawFd::as_raw_fd returns a RawFd (which is a primitive i32 on Unix), not an Option<RawFd>. Calling .ok_or_else(...) on a primitive i32 will result in a compilation error.
To fix this, since AsRawFd is implemented for dyn MasterPty on Unix, you can directly use the returned RawFd without wrapping it in an Option check.
let setup = (|| -> std::io::Result<(AsyncFd<OwnedFd>, AsyncFd<OwnedFd>)> {
let master_fd = master.as_raw_fd();
unsafe {
let flags = libc::fcntl(master_fd, libc::F_GETFL);
if flags < 0 {
return Err(std::io::Error::last_os_error());
}
if libc::fcntl(master_fd, libc::F_SETFL, flags | libc::O_NONBLOCK) < 0 {
return Err(std::io::Error::last_os_error());
}
}
let dup_afd = |interest| -> std::io::Result<AsyncFd<OwnedFd>> {
let fd = unsafe { libc::dup(master_fd) };
if fd < 0 {
return Err(std::io::Error::last_os_error());
}
AsyncFd::with_interest(unsafe { OwnedFd::from_raw_fd(fd) }, interest)
};
Ok((dup_afd(Interest::READABLE)?, dup_afd(Interest::WRITABLE)?))
})();| let shell = std::env::var("SHELL").unwrap_or_default(); | ||
| CommandBuilder::new(if shell.is_empty() { "/bin/sh".to_string() } else { shell }) |
There was a problem hiding this comment.
When spawning the default shell, std::env::var("SHELL") is read from the server's environment. If the server is running as a daemon or a system service, SHELL might be unset or set to a shell that doesn't exist or shouldn't be executed. It is safer to default to a known safe shell like /bin/sh or /bin/bash if SHELL is not present, but also consider validating the environment variable or strictly using /bin/sh to avoid executing arbitrary binaries defined in the environment.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Documentation
Tests