Fix interactive /bin/sh boot: four root causes#920
Open
dburkart wants to merge 7 commits into
Open
Conversation
- Remove dead_code gate from build_userspace_sh() in xtask - Fix Atomic::<u32>::new(0) ambiguity in std thread/vibix.rs (E0034) - Add C-ABI syscall shims to sh (fork, pipe, dup2, execve, etc.) using raw inline asm to avoid double-linking vibix_abi - Extend ext2_image::build() with build_with_extras() to install extra binaries (e.g. /bin/sh) alongside /init - Update init to fork+exec /bin/sh after the hello cycle - Add "init: launching /bin/sh" smoke marker - Add `cargo xtask sh` integration test subcommand - Expand vibix_libc with missing POSIX symbols (fork, wait4, kill, dup, dup2, pipe, access, setpgid, fcntl, sigaction) Closes #883 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… target The vibix PAL's _start entry point was passing argc=0, argv=null to main(), which meant std::env::args() always returned empty on vibix. This prevented the shell from seeing its -c flag when exec'd by init. - Replace the hardcoded _start function with a global_asm stub that reads the real argc/argv from the SysV AMD64 initial stack layout - Add a vibix-specific args module (sys/args/vibix.rs) that stores and retrieves argc/argv via atomics, matching the pattern used by unix targets - Register the vibix args module in sys/args/mod.rs (before the unix branch) - Forward argc/argv from the PAL init() to the args module - Guard std::env::vars() with #[cfg(not(target_os = "vibix"))] in the shell (the unsupported env module panics on vibix) - Guard mod syscalls with #[cfg(not(test))] so host tests can mock symbols - Set is_tty = true unconditionally (vibix serial is not a POSIX tty) - Update ext2 image hash for the rebuilt rootfs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The ext2-image CI check builds the base image without extra binaries. The previous commit incorrectly updated the hash to match a build that included /bin/sh. Restore the hash to match the base image. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Explicitly cast *const u8 to *const c_char for type safety, matching the pattern used by wasip1.rs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…lignment, and ELF size limit Four root causes prevented the shell from booting interactively: 1. EFBIG from execve — the shell binary (1.7 MB) exceeded the 1 MB READ_ALL_MAX limit in resolve_execve_binary. Raised to 4 MB. 2. Compiler-generated _start overriding PAL — the Rust compiler's default _start hardcoded argc=0/argv=null. Replaced global_asm! + _start_rust with a single #[naked] _start that reads directly from the SysV initial stack. 3. Initial RSP not 16-byte aligned — the SysV AMD64 ABI requires %rsp % 16 == 0 at process entry. The auxv stack builder only 8-byte-aligned, causing #GP on movaps instructions. Fixed by computing the parity of pushed 8-byte words and adjusting the alignment base accordingly. 4. Misaligned TLS TCB — compute_tls_layout placed the TCB at tdata_start + total_size which could be misaligned when p_memsz is not a multiple of 8. Round TCB VA up to 8-byte alignment. Additionally, execve now creates a VMA_GROWSDOWN stack with a guard page (matching init_process), so the shell's Rust runtime has room to grow beyond a single 4 KiB page. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR introduces a complete ChangesShell and Platform Foundation
Sequence DiagramsequenceDiagram
participant Init
participant Kernel
participant Shell
Init->>Init: run hello test
Init->>Kernel: fork()
Kernel-->>Init: returns child_pid
Init->>Kernel: child execve("/bin/sh")
Kernel->>Kernel: load ELF, setup stack (16-byte aligned)
Init->>Kernel: wait4(child_pid)
Kernel-->>Init: child status
Shell->>Kernel: read(stdin, buf)
Kernel-->>Shell: bytes
Shell->>Shell: parse command
Shell->>Kernel: fork()
Kernel-->>Shell: returns pid
Shell->>Kernel: wait4(pid)
Shell->>Kernel: execve(cmd_path, argv, envp)
Kernel->>Kernel: load command ELF
Shell->>Kernel: write(stdout, outbuf)
Kernel-->>Shell: bytes written
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
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)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@base/sh/src/main.rs`:
- Around line 63-67: The code unconditionally forces stdin interactive by
setting let is_tty = true; which breaks non-vibix sessions; change this to only
force true when running on vibix and otherwise use normal tty detection (e.g.
replace the assignment with a conditional such as: if running_on_vibix { true }
else { atty::is(atty::Stream::Stdin) } ), where "running_on_vibix" can be
implemented by checking an environment variable (std::env::var("VIBIX") ==
Ok("1")) or by inspecting the host name (gethostname()/std::env::var("HOSTNAME")
contains "vibix"); keep the variable name is_tty and preserve the existing
comment about vibix.
In `@kernel/src/mem/auxv.rs`:
- Around line 183-195: Preflight the remaining stack budget before touching
offset parity: compute the required stack space based on argc, envp_count, and
auxv pair count (the existing total_words calculation) and assert/return an
overflow error if offset has fewer than that many bytes remaining before doing
offset &= !15 or applying the -8 parity adjustment; update the logic around the
variables total_words, argc, envp_count and offset in auxv.rs so the boundary
check happens prior to changing offset (and still considers the extra 8 bytes
when total_words is odd).
In `@xtask/src/ext2_image.rs`:
- Around line 134-146: build_with_extras() currently updates/validates the same
fixtures hash as the base image which causes hash collisions between normal and
extra-bin variants; change the hash fixture used when update_hash is true for
the extra-bins variant so it writes/validates a distinct file (e.g. derive a
different fixture name like "ext2_image_extras.sha256" or append a suffix) and
update the code paths in build_with_extras (and any callers that pass
update_hash) to use that distinct filename when computing/writing the checksum
so the base image's tests continue to reference the original "ext2_image.sha256"
while extra-bin builds use the separate fixture.
In `@xtask/src/main.rs`:
- Around line 2853-2855: The test is asserting init’s pre-exec log
(SUCCESS_MARKER) instead of a shell-owned, post-exec signal; change the marker
and reader logic so the test waits for a shell-emitted token after exec rather
than "init: launching /bin/sh". Replace uses of SUCCESS_MARKER with a unique
post-exec sentinel produced by the shell (for example have the shell print a
distinct marker after successful exec or assert the shell prompt string) and
update the line-based reader to match substrings or bytes (since the prompt may
lack a trailing newline) so it detects the marker without requiring newline;
update the corresponding assertions that reference SUCCESS_MARKER (also at the
other block around the 2902-2953 diff) and keep PANIC_MARKER and HARD_CAP
behavior unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3b34cea0-9380-4889-88bd-87b96cc215ad
📒 Files selected for processing (18)
base/init/src/main.rsbase/sh/src/main.rsbase/sh/src/syscalls.rsbase/vibix_libc/src/fcntl.rsbase/vibix_libc/src/lib.rsbase/vibix_libc/src/signal.rsbase/vibix_libc/src/unistd.rskernel/src/arch/x86_64/syscall.rskernel/src/mem/auxv.rskernel/src/mem/elf.rskernel/src/shell/vfs_helpers.rslibrary/std/src/sys/args/mod.rslibrary/std/src/sys/args/vibix.rslibrary/std/src/sys/pal/vibix/mod.rslibrary/std/src/sys/thread/vibix.rstests/fixtures/ext2_image.sha256xtask/src/ext2_image.rsxtask/src/main.rs
Comment on lines
+63
to
+67
| // On vibix, the serial console is not a POSIX tty, so `isatty()` | ||
| // would return false even for the primary interactive session. | ||
| // Always treat stdin-mode as interactive to ensure the prompt | ||
| // appears over the serial console. | ||
| let is_tty = true; |
There was a problem hiding this comment.
Gate the forced interactive mode to vibix only.
This now makes every stdin-driven host session look interactive too, so pipes/files get prompts and interactive signal handling. Keep the forced true on vibix, but preserve normal tty detection everywhere else.
🤖 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 `@base/sh/src/main.rs` around lines 63 - 67, The code unconditionally forces
stdin interactive by setting let is_tty = true; which breaks non-vibix sessions;
change this to only force true when running on vibix and otherwise use normal
tty detection (e.g. replace the assignment with a conditional such as: if
running_on_vibix { true } else { atty::is(atty::Stream::Stdin) } ), where
"running_on_vibix" can be implemented by checking an environment variable
(std::env::var("VIBIX") == Ok("1")) or by inspecting the host name
(gethostname()/std::env::var("HOSTNAME") contains "vibix"); keep the variable
name is_tty and preserve the existing comment about vibix.
Comment on lines
+183
to
+195
| // SysV AMD64 ABI: rsp must be 16-byte aligned at process entry. | ||
| // We push a known number of 8-byte words below this point (auxv pairs, | ||
| // envp/argv pointer arrays + NULLs, and argc). If that total is odd | ||
| // the final rsp would land on 8-mod-16, so we start at 8-mod-16 here | ||
| // to compensate. | ||
| let argc = argv.len().min(MAX_STRINGS); | ||
| let envp_count = envp.len().min(MAX_STRINGS); | ||
| let total_words = 16 /* auxv pairs */ + 1 /* envp NULL */ + envp_count | ||
| + 1 /* argv NULL */ + argc + 1 /* argc */; | ||
| offset &= !15; // 16-byte align | ||
| if total_words % 2 != 0 { | ||
| offset -= 8; | ||
| } |
There was a problem hiding this comment.
Preflight the remaining stack budget before the parity adjustment.
Line 194 can wrap offset before the final boundary assert runs. If argv/envp string data leaves less than 8 bytes here, the next raw writes walk past the start of the frame instead of failing with the documented overflow panic.
Suggested fix
let argc = argv.len().min(MAX_STRINGS);
let envp_count = envp.len().min(MAX_STRINGS);
let total_words = 16 /* auxv pairs */ + 1 /* envp NULL */ + envp_count
+ 1 /* argv NULL */ + argc + 1 /* argc */;
offset &= !15; // 16-byte align
- if total_words % 2 != 0 {
- offset -= 8;
- }
+ let padding = if total_words % 2 != 0 { 8 } else { 0 };
+ let required = padding + total_words * 8;
+ assert!(
+ offset >= required,
+ "auxv: initial stack layout exceeds page boundary"
+ );
+ offset -= padding;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // SysV AMD64 ABI: rsp must be 16-byte aligned at process entry. | |
| // We push a known number of 8-byte words below this point (auxv pairs, | |
| // envp/argv pointer arrays + NULLs, and argc). If that total is odd | |
| // the final rsp would land on 8-mod-16, so we start at 8-mod-16 here | |
| // to compensate. | |
| let argc = argv.len().min(MAX_STRINGS); | |
| let envp_count = envp.len().min(MAX_STRINGS); | |
| let total_words = 16 /* auxv pairs */ + 1 /* envp NULL */ + envp_count | |
| + 1 /* argv NULL */ + argc + 1 /* argc */; | |
| offset &= !15; // 16-byte align | |
| if total_words % 2 != 0 { | |
| offset -= 8; | |
| } | |
| // SysV AMD64 ABI: rsp must be 16-byte aligned at process entry. | |
| // We push a known number of 8-byte words below this point (auxv pairs, | |
| // envp/argv pointer arrays + NULLs, and argc). If that total is odd | |
| // the final rsp would land on 8-mod-16, so we start at 8-mod-16 here | |
| // to compensate. | |
| let argc = argv.len().min(MAX_STRINGS); | |
| let envp_count = envp.len().min(MAX_STRINGS); | |
| let total_words = 16 /* auxv pairs */ + 1 /* envp NULL */ + envp_count | |
| 1 /* argv NULL */ + argc + 1 /* argc */; | |
| offset &= !15; // 16-byte align | |
| let padding = if total_words % 2 != 0 { 8 } else { 0 }; | |
| let required = padding + total_words * 8; | |
| assert!( | |
| offset >= required, | |
| "auxv: initial stack layout exceeds page boundary" | |
| ); | |
| offset -= padding; |
🤖 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 `@kernel/src/mem/auxv.rs` around lines 183 - 195, Preflight the remaining stack
budget before touching offset parity: compute the required stack space based on
argc, envp_count, and auxv pair count (the existing total_words calculation) and
assert/return an overflow error if offset has fewer than that many bytes
remaining before doing offset &= !15 or applying the -8 parity adjustment;
update the logic around the variables total_words, argc, envp_count and offset
in auxv.rs so the boundary check happens prior to changing offset (and still
considers the extra 8 bytes when total_words is odd).
Comment on lines
+134
to
+146
| /// Like [`build`], but also installs extra binaries into the image. | ||
| /// | ||
| /// Each entry in `extra_bins` is `(host_path, image_path)` — e.g. | ||
| /// `(&sh_bin, "/bin/sh")`. The host path must be whitespace-free (a | ||
| /// debugfs limitation). The image path's parent directory must already | ||
| /// exist in the fixture tree (currently: `/bin`, `/lib`, `/tmp`, `/dev`, | ||
| /// `/etc`, `/etc/init`). | ||
| pub fn build_with_extras( | ||
| workspace_root: &Path, | ||
| init_src: Option<&Path>, | ||
| extra_bins: &[(&Path, &str)], | ||
| update_hash: bool, | ||
| ) -> R<PathBuf> { |
There was a problem hiding this comment.
Use a distinct hash fixture for extra-bin ext2 variants.
build_with_extras() introduces a second image variant, but it still shares the base image’s expected-hash contract. That lets smoke/run/sh validate or rewrite tests/fixtures/ext2_image.sha256 against different bytes than xtask ext2-image, so the next plain image build can drift for the wrong reason.
🤖 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 `@xtask/src/ext2_image.rs` around lines 134 - 146, build_with_extras()
currently updates/validates the same fixtures hash as the base image which
causes hash collisions between normal and extra-bin variants; change the hash
fixture used when update_hash is true for the extra-bins variant so it
writes/validates a distinct file (e.g. derive a different fixture name like
"ext2_image_extras.sha256" or append a suffix) and update the code paths in
build_with_extras (and any callers that pass update_hash) to use that distinct
filename when computing/writing the checksum so the base image's tests continue
to reference the original "ext2_image.sha256" while extra-bin builds use the
separate fixture.
Comment on lines
+2853
to
+2855
| const HARD_CAP: Duration = Duration::from_secs(120); | ||
| const SUCCESS_MARKER: &str = "init: launching /bin/sh"; | ||
| const PANIC_MARKER: &str = "KERNEL PANIC:"; |
There was a problem hiding this comment.
Assert a post-exec shell signal here, not init’s pre-exec marker.
SUCCESS_MARKER is printed by init before the /bin/sh fork/exec, so this test still passes when the shell binary is missing or execve fails immediately. Also, the line-based reader cannot observe the shell’s $ prompt because it has no trailing newline. This needs a shell-owned, post-exec signal instead.
Also applies to: 2902-2953
🤖 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 `@xtask/src/main.rs` around lines 2853 - 2855, The test is asserting init’s
pre-exec log (SUCCESS_MARKER) instead of a shell-owned, post-exec signal; change
the marker and reader logic so the test waits for a shell-emitted token after
exec rather than "init: launching /bin/sh". Replace uses of SUCCESS_MARKER with
a unique post-exec sentinel produced by the shell (for example have the shell
print a distinct marker after successful exec or assert the shell prompt string)
and update the line-based reader to match substrings or bytes (since the prompt
may lack a trailing newline) so it detects the marker without requiring newline;
update the corresponding assertions that reference SUCCESS_MARKER (also at the
other block around the 2902-2953 diff) and keep PANIC_MARKER and HARD_CAP
behavior unchanged.
The shell's stdin was broken in two ways: std's Stdin::read() was a stub returning Ok(0) (immediate EOF), and SerialBackend::read() only polled the serial RX ring — PS/2 keyboard scancodes fed into PS2_TTY's NTtyLdisc never reached the readv syscall path. - vibix_abi: add read_stdin() using readv syscall (nr 19) - std/stdio/vibix: wire Stdin::read() to vibix_abi::stdio::read_stdin() - ntty: add NTty::try_read_byte() to pop committed bytes from raw ring - ntty: add ECHO support — echo typed characters to both serial and framebuffer (using try_lock for soft-IRQ safety) - ps2: extract PS2_LDISC static for direct NTtyLdisc access without trait-object downcasting; expose try_read_byte() and reader_len() - fs/SerialBackend::read(): drain PS/2 N_TTY raw ring with proper wait-queue blocking (register_wait + block_current pattern) - fs/SerialBackend::write(): mirror output to framebuffer console Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@kernel/src/fs/mod.rs`:
- Around line 656-668: The code can miss COM1 wakeups because it only registers
on ps2.read_wait after a poll; modify the loop so you register a serial wait
(e.g., crate::serial::read_wait.register_wait) before checking ps2::reader_len
and before blocking, or register a unified stdin wait that combines serial and
ps2; specifically, in the loop around crate::serial::try_read_byte() and
ps2.read_wait.register_wait(tid) add registration on the serial wait queue (or
the unified stdin wait) into a tok variable, check both reader_len() and
crate::serial::has_bytes() (or equivalent) after registering, cancel both tokens
if waking or continuing, and only call crate::task::block_current() when both
queues report empty to ensure COM1 bytes can wake the sleeper.
- Around line 635-668: The read method (fn read(&self, buf: &mut [u8])) must
return Ok(0) immediately for zero-length buffers to avoid indexing buf[0] or
blocking; add an early check like if buf.is_empty() { return Ok(0); } at the
start of read so the fallback path that calls crate::serial::try_read_byte() or
registers on ps2.read_wait never executes for empty slices, preserving current
behavior of try_read_byte(), ps2.read_wait.register_wait(), and
ps2.read_wait.cancel().
In `@kernel/src/tty/ntty.rs`:
- Around line 529-542: Replace the manual echo handling in the ECHO branch with
a call into the TTY's canonical echo pipeline: instead of replicating
ECHO/VERASE/OPOST/ECHOE/ECHONL logic here, call queue_echo(...) (so the byte is
enqueued and later transformed by process_output()) and ensure the actual
serial/framebuffer writes use the same bytes emitted by process_output()/output
queue (mirror those processed bytes to crate::serial::write_bytes and
fb_echo_str/fb_echo_byte) so visual erase and newline expansion follow the
existing termios rules.
- Around line 262-272: try_read_byte currently ignores RawRing::eof_pos so EOF
boundaries are lost; update try_read_byte to first check st.raw.eof_pos and if
it equals st.raw.head then consume that EOF (clear st.raw.eof_pos) and return a
dedicated EOF marker (e.g. Some(0) or whatever the codebase uses to represent
EOF), otherwise proceed to read a byte as before; specifically, in try_read_byte
check if let Some(ep) = st.raw.eof_pos { if st.raw.head == ep { st.raw.eof_pos =
None; return Some(0); } } before the existing empty/byte logic so a canonical
VEOF is observed once.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1f8c77d7-809c-45e4-b5bc-61a37515624e
📒 Files selected for processing (5)
base/vibix_abi/src/stdio.rskernel/src/fs/mod.rskernel/src/tty/ntty.rskernel/src/tty/ps2.rslibrary/std/src/sys/stdio/vibix.rs
Comment on lines
635
to
668
| fn read(&self, buf: &mut [u8]) -> Result<usize, i64> { | ||
| let caller = crate::process::current_pid(); | ||
| let tty = crate::tty::console_tty(); | ||
| if let Some(rc) = crate::tty::tty_check_sigttin(&tty, caller) { | ||
| // KERN_ERESTARTSYS is honoured by the syscall trampoline | ||
| // (see `signal::check_and_deliver_signals`): restart on | ||
| // SA_RESTART, otherwise -EINTR. | ||
| return Err(rc); | ||
| } | ||
| for (i, byte) in buf.iter_mut().enumerate() { | ||
| match crate::serial::try_read_byte() { | ||
| Some(b) => *byte = b, | ||
| None => { | ||
| return if i == 0 { Err(EAGAIN) } else { Ok(i) }; | ||
| let ps2 = crate::tty::ps2::tty(); | ||
| loop { | ||
| let mut i = 0; | ||
| while i < buf.len() { | ||
| match crate::tty::ps2::try_read_byte() { | ||
| Some(b) => { | ||
| buf[i] = b; | ||
| i += 1; | ||
| } | ||
| None => break, | ||
| } | ||
| } | ||
| if i > 0 { | ||
| return Ok(i); | ||
| } | ||
| if let Some(b) = crate::serial::try_read_byte() { | ||
| buf[0] = b; | ||
| return Ok(1); | ||
| } | ||
| let tid = crate::task::current_id(); | ||
| let tok = ps2.read_wait.register_wait(tid); | ||
| if crate::tty::ps2::reader_len() > 0 { | ||
| ps2.read_wait.cancel(tok); | ||
| continue; | ||
| } | ||
| crate::task::block_current(); | ||
| ps2.read_wait.cancel(tok); | ||
| } |
There was a problem hiding this comment.
Return immediately for zero-length reads.
read(&mut []) currently falls through into the fallback path. If COM1 already has a byte, Line 657 indexes buf[0] and panics; if not, the call blocks forever instead of returning Ok(0).
Suggested fix
fn read(&self, buf: &mut [u8]) -> Result<usize, i64> {
+ if buf.is_empty() {
+ return Ok(0);
+ }
let caller = crate::process::current_pid();
let tty = crate::tty::console_tty();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fn read(&self, buf: &mut [u8]) -> Result<usize, i64> { | |
| let caller = crate::process::current_pid(); | |
| let tty = crate::tty::console_tty(); | |
| if let Some(rc) = crate::tty::tty_check_sigttin(&tty, caller) { | |
| // KERN_ERESTARTSYS is honoured by the syscall trampoline | |
| // (see `signal::check_and_deliver_signals`): restart on | |
| // SA_RESTART, otherwise -EINTR. | |
| return Err(rc); | |
| } | |
| for (i, byte) in buf.iter_mut().enumerate() { | |
| match crate::serial::try_read_byte() { | |
| Some(b) => *byte = b, | |
| None => { | |
| return if i == 0 { Err(EAGAIN) } else { Ok(i) }; | |
| let ps2 = crate::tty::ps2::tty(); | |
| loop { | |
| let mut i = 0; | |
| while i < buf.len() { | |
| match crate::tty::ps2::try_read_byte() { | |
| Some(b) => { | |
| buf[i] = b; | |
| i += 1; | |
| } | |
| None => break, | |
| } | |
| } | |
| if i > 0 { | |
| return Ok(i); | |
| } | |
| if let Some(b) = crate::serial::try_read_byte() { | |
| buf[0] = b; | |
| return Ok(1); | |
| } | |
| let tid = crate::task::current_id(); | |
| let tok = ps2.read_wait.register_wait(tid); | |
| if crate::tty::ps2::reader_len() > 0 { | |
| ps2.read_wait.cancel(tok); | |
| continue; | |
| } | |
| crate::task::block_current(); | |
| ps2.read_wait.cancel(tok); | |
| } | |
| fn read(&self, buf: &mut [u8]) -> Result<usize, i64> { | |
| if buf.is_empty() { | |
| return Ok(0); | |
| } | |
| let caller = crate::process::current_pid(); | |
| let tty = crate::tty::console_tty(); | |
| if let Some(rc) = crate::tty::tty_check_sigttin(&tty, caller) { | |
| return Err(rc); | |
| } | |
| let ps2 = crate::tty::ps2::tty(); | |
| loop { | |
| let mut i = 0; | |
| while i < buf.len() { | |
| match crate::tty::ps2::try_read_byte() { | |
| Some(b) => { | |
| buf[i] = b; | |
| i += 1; | |
| } | |
| None => break, | |
| } | |
| } | |
| if i > 0 { | |
| return Ok(i); | |
| } | |
| if let Some(b) = crate::serial::try_read_byte() { | |
| buf[0] = b; | |
| return Ok(1); | |
| } | |
| let tid = crate::task::current_id(); | |
| let tok = ps2.read_wait.register_wait(tid); | |
| if crate::tty::ps2::reader_len() > 0 { | |
| ps2.read_wait.cancel(tok); | |
| continue; | |
| } | |
| crate::task::block_current(); | |
| ps2.read_wait.cancel(tok); | |
| } |
🤖 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 `@kernel/src/fs/mod.rs` around lines 635 - 668, The read method (fn read(&self,
buf: &mut [u8])) must return Ok(0) immediately for zero-length buffers to avoid
indexing buf[0] or blocking; add an early check like if buf.is_empty() { return
Ok(0); } at the start of read so the fallback path that calls
crate::serial::try_read_byte() or registers on ps2.read_wait never executes for
empty slices, preserving current behavior of try_read_byte(),
ps2.read_wait.register_wait(), and ps2.read_wait.cancel().
Comment on lines
+656
to
668
| if let Some(b) = crate::serial::try_read_byte() { | ||
| buf[0] = b; | ||
| return Ok(1); | ||
| } | ||
| let tid = crate::task::current_id(); | ||
| let tok = ps2.read_wait.register_wait(tid); | ||
| if crate::tty::ps2::reader_len() > 0 { | ||
| ps2.read_wait.cancel(tok); | ||
| continue; | ||
| } | ||
| crate::task::block_current(); | ||
| ps2.read_wait.cancel(tok); | ||
| } |
There was a problem hiding this comment.
Serial input can miss wakeups on the blocking path.
After the one-shot crate::serial::try_read_byte() poll, this code only waits on ps2.read_wait. A COM1 byte that arrives after Line 656 has no wake source here, so a serial-only stdin reader can sleep until unrelated PS/2 activity happens. This needs a serial wait queue, or a unified stdin wait source, before blocking.
🤖 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 `@kernel/src/fs/mod.rs` around lines 656 - 668, The code can miss COM1 wakeups
because it only registers on ps2.read_wait after a poll; modify the loop so you
register a serial wait (e.g., crate::serial::read_wait.register_wait) before
checking ps2::reader_len and before blocking, or register a unified stdin wait
that combines serial and ps2; specifically, in the loop around
crate::serial::try_read_byte() and ps2.read_wait.register_wait(tid) add
registration on the serial wait queue (or the unified stdin wait) into a tok
variable, check both reader_len() and crate::serial::has_bytes() (or equivalent)
after registering, cancel both tokens if waking or continuing, and only call
crate::task::block_current() when both queues report empty to ensure COM1 bytes
can wake the sleeper.
Comment on lines
+262
to
+272
| /// Pop a single committed byte from the raw ring, or `None` if empty. | ||
| pub fn try_read_byte(&self) -> Option<u8> { | ||
| let mut st = self.state.lock(); | ||
| if st.raw.len() == 0 { | ||
| return None; | ||
| } | ||
| let idx = st.raw.head & (RAW_RING_CAP - 1); | ||
| let b = st.raw.buf[idx]; | ||
| st.raw.head = st.raw.head.wrapping_add(1); | ||
| Some(b) | ||
| } |
There was a problem hiding this comment.
Expose EOF as a first-class read event.
RawRing::eof_pos is never observed or consumed here. After canonical VEOF, an empty line is indistinguishable from “no data yet”, so downstream readers block instead of returning 0; after draining a non-empty pre-EOF line, the one-shot EOF is lost for the same reason. This helper needs to surface EOF boundaries, not just bytes.
🤖 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 `@kernel/src/tty/ntty.rs` around lines 262 - 272, try_read_byte currently
ignores RawRing::eof_pos so EOF boundaries are lost; update try_read_byte to
first check st.raw.eof_pos and if it equals st.raw.head then consume that EOF
(clear st.raw.eof_pos) and return a dedicated EOF marker (e.g. Some(0) or
whatever the codebase uses to represent EOF), otherwise proceed to read a byte
as before; specifically, in try_read_byte check if let Some(ep) = st.raw.eof_pos
{ if st.raw.head == ep { st.raw.eof_pos = None; return Some(0); } } before the
existing empty/byte logic so a canonical VEOF is observed once.
Comment on lines
+529
to
+542
| // Echo the byte to the tty output if ECHO is set. | ||
| #[cfg(target_os = "none")] | ||
| if termios.c_lflag & ECHO != 0 { | ||
| if b == b'\n' && termios.c_oflag & ONLCR != 0 { | ||
| crate::serial::write_bytes(b"\r\n"); | ||
| fb_echo_str("\n"); | ||
| } else if matches_cc(&termios, VERASE, b) { | ||
| crate::serial::write_bytes(b"\x08 \x08"); | ||
| fb_echo_str("\x08 \x08"); | ||
| } else { | ||
| crate::serial::write_bytes(&[b]); | ||
| fb_echo_byte(b); | ||
| } | ||
| } |
There was a problem hiding this comment.
Reuse queue_echo() here instead of re-implementing echo rules.
This branch now diverges from the file’s own termios handling: VERASE gets visual erase even when ECHOE is clear, ECHONL is ignored when ECHO is clear, and \n is expanded even if OPOST is off. Routing echo through queue_echo()/process_output() and then mirroring those transformed bytes to serial/framebuffer keeps runtime behavior aligned with the tested rules above.
🤖 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 `@kernel/src/tty/ntty.rs` around lines 529 - 542, Replace the manual echo
handling in the ECHO branch with a call into the TTY's canonical echo pipeline:
instead of replicating ECHO/VERASE/OPOST/ECHOE/ECHONL logic here, call
queue_echo(...) (so the byte is enqueued and later transformed by
process_output()) and ensure the actual serial/framebuffer writes use the same
bytes emitted by process_output()/output queue (mirror those processed bytes to
crate::serial::write_bytes and fb_echo_str/fb_echo_byte) so visual erase and
newline expansion follow the existing termios rules.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
READ_ALL_MAXlimit. Raised to 4 MB._start: replacedglobal_asm!+_start_rustwith a single#[naked] _startthat reads SysV initial stack directly.%rsp % 16 == 0at process entry. Fixed auxv stack builder parity logic.compute_tls_layoutplaced TCB attdata_start + total_sizewhich was not 8-byte aligned whenp_memszisn't a multiple of 8.VMA_GROWSDOWNflag and guard page to execve's stack setup (matching init_process).After these fixes the shell fully initializes, prints
$, reads from stdin, and exits cleanly on EOF.Test plan
cargo xtask smoke— all 43 markers passcargo xtask sh— shell launch marker detectedcargo test -p vibix --lib— 716 kernel unit tests pass (including auxv alignment + TLS layout tests)cargo test(sh) — 399 shell host tests pass$prompt and exits cleanly on stdin EOF🤖 Generated with Claude Code