Add TTY and interactive mode support (-it flags)#108
Conversation
Implement full PTY support matching docker/podman -i and -t flags: - `-i`: Keep stdin open (forward user input) - `-t`: Allocate pseudo-TTY (colors, line editing, vim) - `-it`: Both (interactive shell) Host side (src/commands/tty.rs): - Raw terminal mode via tcsetattr/cfmakeraw - Binary exec_proto framing over vsock - Async reader/writer tasks for stdin/stdout Guest side (fc-agent/src/tty.rs): - PTY allocation via openpty() - Fork child with PTY as controlling terminal - Relay between vsock and PTY master CLI changes: - Add -i/-t flags to `fcvm podman run` - Change exec `name` from positional to `--name` flag - Fix argument conflict between --pid and trailing command Documentation: - Add TTY architecture section to DESIGN.md - Add Interactive Mode section to README.md - Update CLI examples to use --name flag Tested: - 4 TTY tests pass (tty, interactive, parallel stress) - 6 exec tests pass - Manual verification of README examples
ejc3
force-pushed
the
tty-interactive-mode
branch
from
8eb5e56 to
d24ef7a
Compare
- Remove needless return statement in fc-agent exec TTY handler - Fix formatting in tty.rs and test_exec.rs
🔍 Claude ReviewSEVERITY: critical Findings[CRITICAL] Terminal state not restored on signals (SIGTERM/SIGQUIT)Location: When the user presses Ctrl+\ (SIGQUIT) or the process receives SIGTERM/SIGKILL, the terminal is LEFT IN RAW MODE. The user's shell becomes unusable (no echo, no line editing) until they manually run Issue: No signal handlers are installed. Terminal restoration only happens at normal function exit. Fix: Install signal handlers using [CRITICAL] Writer thread not joined - potential zombie threadsLocation: drop(writer_thread); // ← DOES NOT JOIN!Issue: Fix: if let Some(handle) = writer_thread {
handle.join().ok();
}[CRITICAL] Guest-side FD leaks on error pathsLocation: Multiple error paths fail to close file descriptors:
Over time, repeated failures exhaust the file descriptor limit. Fix: Add explicit FD cleanup in error handlers. [CRITICAL] waitpid return value ignored - zombie processesLocation: unsafe {
libc::waitpid(child_pid, &mut status, 0); // Return value ignored!
}Issue: If Fix: let ret = unsafe { libc::waitpid(child_pid, &mut status, 0) };
if ret < 0 {
eprintln!("[fc-agent] waitpid failed: {}", std::io::Error::last_os_error());
return 1;
}[CRITICAL] Buffer allocation DoS vulnerabilityLocation: let mut payload = vec![0u8; len]; // Allocates immediately!Issue: A malicious sender can send large length prefixes (up to 16MB limit) then disconnect without sending data. Multiple connections can exhaust memory. Fix: Use progressive allocation or read-in-chunks to avoid allocating full buffer upfront. Medium Issues[MEDIUM] Socket cleanup raceLocation: If the process is killed between binding the Unix socket and cleanup, the socket file is orphaned. Mitigated by cleanup at start of next run, but still resource pollution. [MEDIUM] tcsetattr failure ignoredLocation: Terminal restoration failure is silently ignored. Should at least log a warning. [MEDIUM] Escape sequences unfilteredLocation: Raw terminal data from guest is written directly to stdout. A compromised guest can send malicious ANSI/OSC sequences to manipulate the terminal, change title, or exploit terminal emulator bugs. Recommendation: Document the risk or add escape sequence filtering for high-security environments. [MEDIUM] Vsock disconnect returns exit code 0Location: Protocol errors cause the reader loop to exit, but default to exit code 0 (success). Should return 1 to indicate error. SummaryThe TTY implementation has good overall design with proper binary framing protocol and thread coordination. However, there are critical gaps in cleanup and signal handling:
These issues will significantly impact user experience and system stability under error conditions. Recommended Priority:
Review by Claude | Run |
Fixes critical issues identified in PR #108 review: 1. **Thread joining**: Writer thread is now properly joined instead of being dropped/detached. Prevents zombie threads when the reader exits while writer is blocked in poll(). 2. **FD leak fixes (guest)**: Added explicit FD cleanup on error paths: - vsock clone failure: now closes master_fd/pipe FDs before return - dup() failure: now closes master_fd before return Prevents FD exhaustion after repeated failures. 3. **waitpid error handling**: Check return value and handle failures (EINTR, ECHILD). Prevents zombie processes and garbage exit codes. 4. **Terminal restoration**: Check tcsetattr return value and log warnings on failure. 5. **Protocol error handling**: Return exit code 1 instead of 0 when vsock protocol errors occur. 6. **Stdout write errors**: Detect and handle stdout write failures in reader loop. These fixes address resource leaks and improve error handling that could accumulate over time and degrade system stability. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Fixes from Claude review (PR #108 comment): Host-side (src/commands/tty.rs): - Add signal handlers (SIGTERM, SIGQUIT, SIGHUP) to restore terminal on abnormal exit - prevents terminal corruption - Join writer thread properly instead of drop() - prevents zombie threads - Log warning on tcsetattr failure instead of silent ignore - Return exit code 1 on protocol errors/EOF instead of 0 Guest-side (fc-agent/src/tty.rs): - Fix FD leaks on vsock.try_clone() failure (now closes master_fd or stdout_read/stdin_write depending on mode) - Fix FD leak on dup() failure (now closes master_fd) - Check waitpid return value and log error on failure Protocol (exec-proto/src/lib.rs): - Reduce max message size from 16MB to 1MB (sufficient for TTY) - Read payload progressively in 8KB chunks instead of allocating full buffer upfront - prevents memory exhaustion on disconnect Tested: cargo fmt --check && cargo clippy --all-targets -D warnings
🔍 Claude ReviewSEVERITY: critical Findings[CRITICAL] Signal handler is not async-signal-safeLocation: The signal handler calls From POSIX signal-safety requirements:
Impact: Process hangs instead of terminating when user presses Ctrl+\ during terminal restoration. Fix: Use atomic operations or a lock-free approach: // Option 1: Use static mut with atomic flag (unsafe but signal-safe)
static mut ORIG_FD: i32 = -1;
static mut ORIG_TERMIOS: libc::termios = unsafe { std::mem::zeroed() };
static TERMIOS_SAVED: AtomicBool = AtomicBool::new(false);
extern "C" fn signal_handler(sig: libc::c_int) {
if TERMIOS_SAVED.load(Ordering::Relaxed) {
unsafe {
libc::tcsetattr(ORIG_FD, libc::TCSANOW, &ORIG_TERMIOS);
}
}
unsafe {
libc::signal(sig, libc::SIG_DFL);
libc::raise(sig);
}
}[CRITICAL] Using deprecated signal() API instead of sigaction()Location: The code uses
From POSIX.1-2008:
Fix: Use let mut sa: libc::sigaction = unsafe { std::mem::zeroed() };
sa.sa_sigaction = signal_handler as usize;
sa.sa_flags = libc::SA_RESETHAND; // Auto-reset to SIG_DFL after first invocation
unsafe {
libc::sigaction(libc::SIGTERM, &sa, std::ptr::null_mut());
libc::sigaction(libc::SIGQUIT, &sa, std::ptr::null_mut());
libc::sigaction(libc::SIGHUP, &sa, std::ptr::null_mut());
}[MEDIUM] Confusing control flow in reader_loopLocation: Lines 256-257 return
Recommendation: Add }
// Should not reach here - all branches above return
unreachable\!("reader_loop: exited without return")
}SummaryThe TTY implementation has excellent improvements addressing all previous critical issues (thread joining, FD leaks, waitpid errors, DoS mitigation). However, the signal handling implementation is fundamentally broken:
These issues only manifest under specific timing (signal during mutex hold) or on certain platforms, which is why they may not show up in testing. Good news: The previous [CRITICAL] issues are all fixed! 🎉
Recommended action: Fix the signal-safety issues before merging. The terminal restoration logic is important for user experience. Review by Claude | Run |
🔧 Auto-Fix CreatedI found issues and created a fix PR: #116 The fix addresses critical signal-safety issues in the TTY signal handler that could cause deadlocks or undefined behavior. Fixed Issues
Please review and merge the fix PR first, then this PR. |
Address review feedback from second Claude review:
1. Replace Mutex with static mut + atomic flag
- Mutex::lock() is not async-signal-safe per POSIX
- Could deadlock if signal arrives while mutex is held
- Now uses static mut ORIG_FD/ORIG_TERMIOS_STORAGE with
TERMIOS_SAVED atomic flag for synchronization
- Uses std::ptr::addr_of!() to avoid reference to mutable static
2. Replace signal() with sigaction()
- signal() has undefined behavior in multithreaded programs
- sigaction() provides well-defined POSIX semantics
- Use SA_RESETHAND flag for auto-reset to SIG_DFL
3. Improve reader_loop comment
- Clarify when post-loop code is reached (done flag set externally)
- Add debug log for this case
Tested: cargo fmt --check && cargo clippy --all-targets -D warnings
🔍 Claude ReviewSEVERITY: medium Findings[MEDIUM] Incorrect sigaction field used for signal handlerLocation: sa.sa_sigaction = signal_handler as usize; // WRONG FIELD\!Issue: The code sets
The signal handler has the correct signature for extern "C" fn signal_handler(_sig: libc::c_int) // ← sa_handler signatureImpact: This is undefined behavior - may work on some platforms due to memory layout overlap, but is not portable and violates POSIX. Fix: // In libc crate, sigaction uses a union for sa_handler/sa_sigaction
// On most platforms, assigning to the wrong field works due to union layout,
// but it's technically UB. Use sa_handler when not using SA_SIGINFO:
sa.sa_handler = signal_handler as usize; // Correct fieldSummaryThe TTY implementation has addressed all previous critical issues:
The remaining issue is medium severity - while the current code may work on most platforms due to union memory layout, it's technically undefined behavior and should be corrected for portability. Good work overall! The implementation is solid and all critical safety issues have been resolved. 🎉 Review by Claude | Run |
🔧 Auto-Fix CreatedI found a portability issue and created a fix PR: #117 Issue Fixed
This is a simple one-line fix for undefined behavior. While it may work on most platforms due to union layout, it's not portable per POSIX. Please review and merge the fix PR first, then this PR. |
Replace raw libc calls with nix crate for cleaner, more idiomatic Rust:
- Use nix::sys::signal::{sigaction, SigAction, SigHandler, Signal}
instead of raw libc::sigaction
- Use nix::sys::termios::{Termios, tcgetattr, tcsetattr, cfmakeraw}
instead of raw libc termios calls
- Use nix::unistd::isatty instead of libc::isatty
- Use nix::fcntl for O_NONBLOCK handling
The signal handler still uses unsafe for accessing static mut, but the
rest of the code is now using safe Rust wrappers. The async-signal-safety
properties are preserved - tcsetattr is still async-signal-safe per POSIX.
Tested: cargo fmt --check && cargo clippy --all-targets -D warnings
🔍 Claude ReviewSEVERITY: none FindingsAll previous critical and medium issues have been successfully resolved! 🎉 The implementation now has:
Minor observation (not requiring a fix):[LOW] poll() doesn't retry on EINTR (src/commands/tty.rs:324) The Not worth fixing given the workaround is already in place. SummaryExcellent work addressing all the feedback! The TTY implementation is production-ready. The signal-safety refactoring using the Recommendation: Ready to merge ✅ Review by Claude | Run |
|
Tests that SIGINT sent via terminal interrupt character (^C/0x03) through a PTY is properly received by fcvm's tokio signal handler and triggers clean VM shutdown. This verifies the fix from PR #108 (TTY handling) works correctly when Ctrl+C is pressed in a real terminal context. Tested: make test FILTER=ctrlc (passes in ~7s)
Tests that SIGINT sent via terminal interrupt character (^C/0x03) through a PTY is properly received by fcvm's tokio signal handler and triggers clean VM shutdown. This verifies the fix from PR #108 (TTY handling) works correctly when Ctrl+C is pressed in a real terminal context. Tested: make test FILTER=ctrlc (passes in ~7s)
Tests that SIGINT sent via terminal interrupt character (^C/0x03) through a PTY is properly received by fcvm's tokio signal handler and triggers clean VM shutdown. This verifies the fix from PR #108 (TTY handling) works correctly when Ctrl+C is pressed in a real terminal context. Tested: make test FILTER=ctrlc (passes in ~7s)
Fixes from Claude review (PR #108 comment): Host-side (src/commands/tty.rs): - Add signal handlers (SIGTERM, SIGQUIT, SIGHUP) to restore terminal on abnormal exit - prevents terminal corruption - Join writer thread properly instead of drop() - prevents zombie threads - Log warning on tcsetattr failure instead of silent ignore - Return exit code 1 on protocol errors/EOF instead of 0 Guest-side (fc-agent/src/tty.rs): - Fix FD leaks on vsock.try_clone() failure (now closes master_fd or stdout_read/stdin_write depending on mode) - Fix FD leak on dup() failure (now closes master_fd) - Check waitpid return value and log error on failure Protocol (exec-proto/src/lib.rs): - Reduce max message size from 16MB to 1MB (sufficient for TTY) - Read payload progressively in 8KB chunks instead of allocating full buffer upfront - prevents memory exhaustion on disconnect Tested: cargo fmt --check && cargo clippy --all-targets -D warnings
Add TTY and interactive mode support (-it flags)
Tests that SIGINT sent via terminal interrupt character (^C/0x03) through a PTY is properly received by fcvm's tokio signal handler and triggers clean VM shutdown. This verifies the fix from PR #108 (TTY handling) works correctly when Ctrl+C is pressed in a real terminal context. Tested: make test FILTER=ctrlc (passes in ~7s)
Fixes from Claude review (PR #108 comment): Host-side (src/commands/tty.rs): - Add signal handlers (SIGTERM, SIGQUIT, SIGHUP) to restore terminal on abnormal exit - prevents terminal corruption - Join writer thread properly instead of drop() - prevents zombie threads - Log warning on tcsetattr failure instead of silent ignore - Return exit code 1 on protocol errors/EOF instead of 0 Guest-side (fc-agent/src/tty.rs): - Fix FD leaks on vsock.try_clone() failure (now closes master_fd or stdout_read/stdin_write depending on mode) - Fix FD leak on dup() failure (now closes master_fd) - Check waitpid return value and log error on failure Protocol (exec-proto/src/lib.rs): - Reduce max message size from 16MB to 1MB (sufficient for TTY) - Read payload progressively in 8KB chunks instead of allocating full buffer upfront - prevents memory exhaustion on disconnect Tested: cargo fmt --check && cargo clippy --all-targets -D warnings
Add TTY and interactive mode support (-it flags)
Tests that SIGINT sent via terminal interrupt character (^C/0x03) through a PTY is properly received by fcvm's tokio signal handler and triggers clean VM shutdown. This verifies the fix from PR #108 (TTY handling) works correctly when Ctrl+C is pressed in a real terminal context. Tested: make test FILTER=ctrlc (passes in ~7s)
1 participant
Summary
Implements full PTY/interactive mode support for
fcvm podman runandfcvm exec, matching docker/podman-iand-tflag semantics.Changes
New Features
-i, --interactive: Keep stdin open (forward user input)-t, --tty: Allocate pseudo-TTY (colors, line editing, full-screen apps)-it: Both flags together for interactive shellImplementation
src/commands/tty.rs): Raw terminal mode, binary exec_proto framing over vsockfc-agent/src/tty.rs): PTY allocation via openpty(), fork with PTY as controlling terminalCLI Changes
-i/-tflags tofcvm podman runnamefrom positional to--nameflag (fixes argument conflict)Documentation
--nameflagTest Plan
make test-root FILTER=tty- 4 TTY tests passmake test-root FILTER=exec- 6 exec tests passfcvm exec --name my-vm -- cat /etc/os-release✅fcvm exec --name my-vm --vm -- hostname✅fcvm exec --pid <PID> -- hostname✅fcvm podman run ... alpine:latest echo "hello"✅ (trailing args)