From cd230932b260f13209bc4c65122dbba080ba3bc7 Mon Sep 17 00:00:00 2001 From: Bernie Innocenti Date: Sat, 24 Oct 2026 13:44:43 +0900 Subject: [PATCH] ReadArgs: read the command line from the Input() buffer, as AmigaDOS does RunCommand stuffs the arguments into the buffer of Input() and restores it when the command returns, and ReadArgs, FGetC, FGets and FRead read them from there; ReadArgs no longer parses the guest's A0 buffer, which C startup code edits (vbcc's writes a NUL over the '\n'). RunCommand refuses an argument range past guest memory. Co-authored-by: Claude Opus 5.5 --- crates/volamos-core/src/dispatch.rs | 21 +++++-- crates/volamos-core/src/dosargs.rs | 48 +++++++--------- crates/volamos-core/src/dosbuf.rs | 28 ++++----- crates/volamos-core/src/dosfile.rs | 67 ++++++++++++++++------ crates/volamos-core/src/dosseg.rs | 89 +++++++++++++++++++++++------ 5 files changed, 172 insertions(+), 81 deletions(-) diff --git a/crates/volamos-core/src/dispatch.rs b/crates/volamos-core/src/dispatch.rs index 96aea89..8792af8 100644 --- a/crates/volamos-core/src/dispatch.rs +++ b/crates/volamos-core/src/dispatch.rs @@ -2767,11 +2767,9 @@ impl Runtime { // startup may read this instead of (or in addition to) A0/D0. mem.write_u32(task + exectask::PR_ARGUMENTS_OFFSET, args_addr); - // ReadArgs's default source (rdargs == NULL): the same - // command-line buffer just built above, per crate::dosargs's - // module docs. Registers hold this too, but a guest program is - // free to clobber A0/D0 before ever calling ReadArgs. - dos.cmdline = Some((args_addr, line_len)); + // The same text in the Input() buffer, where ReadArgs reads it, + // as RunCommand puts it there on AmigaOS: see DosState::input_buf. + dos.input_buf = line.clone(); Self { cpu, @@ -4675,6 +4673,19 @@ mod tests { ); } + #[test] + fn the_command_line_is_also_in_the_input_buffer_for_readargs() { + // vbcc's startup code writes a NUL over the command line's '\n' + // as it splits the A0 buffer into argv; ReadArgs, reading the A0 + // buffer, then saw "LIST\0" where the user typed LIST. It reads + // the Input() buffer now, as on AmigaOS. + let mut rt = runtime_with_program_and_args(&[RTS], vec!["LIST".to_string()]); + let a0 = rt.cpu.address_register(AddressRegister(0)); + let d0 = rt.cpu.data_register(DataRegister(0)); + rt.mem.write_u8(a0 + d0 - 1, 0); + assert_eq!(rt.dos.input_buf, b"LIST \n"); + } + #[test] fn empty_args_still_produce_a_bare_newline_command_line() { let rt = runtime_with_program_and_args(&[RTS], Vec::new()); diff --git a/crates/volamos-core/src/dosargs.rs b/crates/volamos-core/src/dosargs.rs index bda29e5..d57e706 100644 --- a/crates/volamos-core/src/dosargs.rs +++ b/crates/volamos-core/src/dosargs.rs @@ -6,15 +6,16 @@ //! //! # Scope and deviations from real `ReadArgs` //! -//! - **Default source only.** `rdargs == NULL` reads from -//! [`DosState::cmdline`] -- the same `A0`/`D0` command-line buffer -//! [`crate::dispatch::Runtime::new`] builds, exactly mirroring how real -//! AmigaOS delivers the CLI command tail through the process's buffered -//! `Input()`. [`DosState::cmdline_pos`] persists across calls, so -//! repeated `ReadArgs(NULL)` calls in one process walk forward through -//! the same buffer rather than each re-parsing it from the start -- -//! real `ReadArgs(NULL)` shares this property because it reads off a -//! stateful buffered stream. A caller-supplied `rdargs` with its own +//! - **Default source only.** `rdargs == NULL` reads the command line +//! `RunCommand` stuffed into the buffer of `Input()`, +//! [`DosState::input_buf`], as real `ReadArgs` "normally gets the +//! arguments by reading buffered IO from Input()" (autodoc). Reading +//! advances [`DosState::input_pos`], which buffered reads share, so +//! repeated `ReadArgs(NULL)` calls walk forward through the stream +//! rather than each re-parsing it from the start. Once that buffer is +//! used up, `ReadArgs` sees an empty line rather than waiting on host +//! stdin, as AROS does for an interactive `Input()` with an empty +//! buffer. A caller-supplied `rdargs` with its own //! `RDA_Source` (a custom string to parse instead of the command line) //! is not implemented: `rdargs` is accepted only as an *identity* for //! `FreeArgs` bookkeeping (see below), and parsing still always reads @@ -567,15 +568,10 @@ fn read_args( ) -> Result { let template = parse_template(template_bytes)?; - let (cmdline_addr, cmdline_len) = dos.cmdline.unwrap_or((0, 0)); - let mut buf = Vec::with_capacity(cmdline_len as usize); - for offset in 0..cmdline_len { - buf.push(mem.read_u8(cmdline_addr.wrapping_add(offset))); - } - - let mut reader = Reader::new(&buf, dos.cmdline_pos as usize); + let buf = dos.input_buf.clone(); + let mut reader = Reader::new(&buf, dos.input_pos); let result = match_args(&mut reader, &template); - dos.cmdline_pos = reader.pos as u32; + dos.input_pos = reader.pos; let slots = result?; let allocations = materialize(heap, mem, &template, slots, array_addr)?; @@ -685,7 +681,7 @@ fn find_arg_handler(ctx: &mut HandlerContext<'_, C>) -> Result<(), Dispa /// Registers `ReadArgs`/`FreeArgs` onto [`DOS_LIBRARY_BASE`], looked up /// by name through [`DOS_LVOS`]. Called from [`crate::dispatch:: /// Runtime::new`] alongside the other `dos.library` registrations; these -/// handlers need [`DosState::cmdline`] (always set by `Runtime::new`), +/// handlers need [`DosState::input_buf`] (always set by `Runtime::new`), /// not a `Vfs`, so they work regardless of whether one is installed. pub fn register_dosargs_handlers( table: &mut LibraryTable, @@ -728,20 +724,16 @@ mod tests { use super::*; use crate::memory::FlatMemory; - /// Builds a fresh `(heap, mem, dos)` triple with `dos.cmdline` - /// pointing at `cmdline` (space-joined-and-newline-terminated, same - /// framing `Runtime::new` gives the real `A0`/`D0` buffer). + /// Builds a fresh `(heap, mem, dos)` triple with `cmdline` in + /// `dos.input_buf` (space-joined and newline-terminated, the framing + /// `Runtime::new` gives it). fn setup(cmdline: &[&str]) -> (GuestHeap, FlatMemory, DosState) { - let mut mem = FlatMemory::new(0x4000); - let mut heap = GuestHeap::new(0x100, 0x3000); + let mem = FlatMemory::new(0x4000); + let heap = GuestHeap::new(0x100, 0x3000); let mut line = cmdline.join(" ").into_bytes(); line.push(b'\n'); - let addr = heap.alloc(&mut mem, line.len() as u32).unwrap(); - for (i, &b) in line.iter().enumerate() { - mem.write_u8(addr + i as u32, b); - } let mut dos = DosState::new(None); - dos.cmdline = Some((addr, line.len() as u32)); + dos.input_buf = line; (heap, mem, dos) } diff --git a/crates/volamos-core/src/dosbuf.rs b/crates/volamos-core/src/dosbuf.rs index abf456d..299d78a 100644 --- a/crates/volamos-core/src/dosbuf.rs +++ b/crates/volamos-core/src/dosbuf.rs @@ -25,13 +25,14 @@ //! `FGetC` consults it before ever touching the host file, so a //! `FGetC`/`UnGetC`/`FGetC` sequence round-trips correctly. //! +//! The other is the command line `RunCommand` stuffs into the buffer of +//! `Input()` (`crate::dosfile::DosState::input_buf`): buffered reads of +//! the current `Input()` return it before any host stdin, as they do on +//! AmigaOS, where `ReadArgs` reads its arguments that way. +//! //! `SelectInput`/`SelectOutput` (redirecting a *process's* default -//! `Input()`/`Output()` handle) and interactive-file-specific behavior -//! (`IsInteractive`, the BCPL command-line-parameters-in-the-input- -//! buffer quirk `files.md` documents for `Input()`) are out of scope -- -//! this runtime's `Input()` is backed by real host stdin already (see -//! `crate::dosfile`'s module docs), not a command-line buffer, so that -//! particular real-AmigaOS quirk doesn't apply here in the first place. +//! `Input()`/`Output()` handle) and other interactive-file-specific +//! behavior (`IsInteractive`) are out of scope. use crate::cpu::{Cpu, DataRegister}; use crate::dispatch::{DOS_LIBRARY_BASE, DispatchError, HandlerContext, LibraryTable}; @@ -54,9 +55,9 @@ fn fgetc(ctx: &mut HandlerContext<'_, impl Cpu>, addr: u32) -> i32 { let value = if let Some(pushed) = ctx.dos.ungetc_buf.remove(&addr) { pushed } else { - match ctx.dos.read(addr, 1) { - Ok(bytes) if !bytes.is_empty() => i32::from(bytes[0]), - Ok(_) => { + match ctx.dos.read_buffered_byte(addr) { + Ok(Some(byte)) => i32::from(byte), + Ok(None) => { ctx.dos.set_io_err(0); // clean EOF, not an error ENDSTREAMCH } @@ -155,13 +156,12 @@ fn fread_handler(ctx: &mut HandlerContext<'_, C>) -> Result<(), Dispatch let total_wanted = (block_len as u64) * (block_count as u64); let mut got = 0u64; while got < total_wanted { - match ctx.dos.read(addr, 1) { - Ok(bytes) if !bytes.is_empty() => { - ctx.mem - .write_u8(buf_addr.wrapping_add(got as u32), bytes[0]); + match ctx.dos.read_buffered_byte(addr) { + Ok(Some(byte)) => { + ctx.mem.write_u8(buf_addr.wrapping_add(got as u32), byte); got += 1; } - Ok(_) => break, // EOF + Ok(None) => break, // EOF Err(code) => { ctx.dos.set_io_err(code); break; diff --git a/crates/volamos-core/src/dosfile.rs b/crates/volamos-core/src/dosfile.rs index 0fdf3dd..2df716d 100644 --- a/crates/volamos-core/src/dosfile.rs +++ b/crates/volamos-core/src/dosfile.rs @@ -442,20 +442,25 @@ pub struct DosState { // the sibling module" convention as the T11/Phase-3-stage-7 fields // above), but the fields themselves are declared here per that same // convention. - /// Guest address of the process's command-line buffer (the `A0`/`D0` - /// buffer [`crate::dispatch::Runtime::new`] builds from - /// [`crate::dispatch::StartConfig::args`]) and its length in bytes - /// (including the trailing `'\n'`). This is `ReadArgs`'s default - /// input source (`rdargs == NULL`), mirroring how real AmigaOS - /// delivers the CLI command tail through the process's buffered - /// `Input()` -- see `crate::dosargs`'s module docs. - pub(crate) cmdline: Option<(u32, u32)>, - /// Byte offset into `cmdline` that the next default-source `ReadArgs` - /// call resumes from -- real `ReadArgs(NULL)` reads from a shared, - /// stateful input stream, so repeated calls in one process walk - /// forward through the same buffer rather than each re-parsing it - /// from the start. - pub(crate) cmdline_pos: u32, + /// The command line, as `RunCommand` stuffs it into the buffer of + /// the process's `Input()`, including the trailing `'\n'`: the text + /// of the `A0`/`D0` buffer [`crate::dispatch::Runtime::new`] builds + /// from [`crate::dispatch::StartConfig::args`], or of the arguments + /// `RunCommand` gets. The `RunCommand` autodoc: it "takes care of + /// setting up the current input filehandle in such a way that + /// ReadArgs() can be used in the program, and restores the state of + /// the buffering before returning"; `ReadArgs` "normally gets the + /// arguments by reading buffered IO from Input()". AROS does the same + /// (`rom/dos/runcommand.c`, `vbuf_inject()`), after the Guru Book. + /// + /// So `ReadArgs(NULL)` and buffered reads of the current `Input()` + /// (`FGetC`, `FGets`, `FRead`) take these bytes before the host + /// stream, and the guest's `A0` buffer is not involved: C startup + /// code splits that into argv in place (vbcc's writes a NUL over the + /// `'\n'`), which real `ReadArgs` never sees. + pub(crate) input_buf: Vec, + /// How much of `input_buf` has been read. + pub(crate) input_pos: usize, /// Live `ReadArgs` results, keyed by the `struct RDArgs*` anchor /// address returned in `D0` (see `crate::dosargs::RDARGS_STRUCT_SIZE`). /// `FreeArgs` looks up and frees every heap block an entry lists, plus @@ -545,8 +550,8 @@ impl DosState { seglists: HashMap::new(), overlay_roots: HashMap::new(), system_runner: None, - cmdline: None, - cmdline_pos: 0, + input_buf: Vec::new(), + input_pos: 0, rdargs: HashMap::new(), local_vars: HashMap::new(), anchor_states: HashMap::new(), @@ -962,6 +967,18 @@ impl DosState { } } + /// One byte of buffered input from the handle at `addr`, `None` at + /// EOF: the rest of [`Self::input_buf`] first when `addr` is the + /// current `Input()`, then the handle itself. Plain `Read` bypasses + /// the buffer, as it does on AmigaOS, so it uses [`Self::read`]. + pub fn read_buffered_byte(&mut self, addr: u32) -> Result, i32> { + if self.current_input == Some(addr) && self.input_pos < self.input_buf.len() { + self.input_pos += 1; + return Ok(Some(self.input_buf[self.input_pos - 1])); + } + Ok(self.read(addr, 1)?.first().copied()) + } + /// `Read`: reads up to `len` bytes from the handle at `addr`, /// returning the bytes actually read (may be shorter than `len`, /// including empty at EOF -- matching real `Read`'s "actual count" @@ -2280,6 +2297,24 @@ mod tests { assert_eq!(err, ERROR_INVALID_LOCK); } + #[test] + fn buffered_reads_of_input_return_the_command_line_first() { + // RunCommand stuffs the arguments into the buffer of Input(), so + // FGetC(Input()) sees them before anything else; another handle + // does not. + let mut heap = GuestHeap::new(0x1000, 0x4000); + let mut mem = FlatMemory::new(0x4000); + let mut dos = DosState::new(None); + dos.input_buf = b"A\n".to_vec(); + let input = dos.input_addr(&mut heap, &mut mem).unwrap(); + let other = dos.alloc_file_handle(&mut heap, &mut mem, false).unwrap(); + + assert_eq!(dos.read_buffered_byte(other), Err(ERROR_INVALID_LOCK)); + assert_eq!(dos.read_buffered_byte(input), Ok(Some(b'A'))); + assert_eq!(dos.read_buffered_byte(input), Ok(Some(b'\n'))); + assert_eq!(dos.input_pos, 2); + } + #[test] fn open_into_handle_resolves_relative_to_a_parent_lock_and_creates_the_file() { let tmp = TempDir::new("open-into-handle"); diff --git a/crates/volamos-core/src/dosseg.rs b/crates/volamos-core/src/dosseg.rs index d6e46cf..15a0fdd 100644 --- a/crates/volamos-core/src/dosseg.rs +++ b/crates/volamos-core/src/dosseg.rs @@ -247,7 +247,7 @@ //! [`crate::guestmem::MIN_STACK_SIZE`], matching //! [`crate::dispatch::Runtime::new`]'s own clamp) from the *caller's* //! [`GuestHeap`], saving the current task's `A7`/`tc_SPLower`/ -//! `tc_SPUpper`/`pr_Arguments`/[`DosState::cmdline`]/`cmdline_pos`/ +//! `tc_SPUpper`/`pr_Arguments`/[`DosState::input_buf`]/`input_pos`/ //! `A6` first -- //! same bookkeeping [`crate::exectask`]'s `StackSwap` handler already //! does for the same reason (a stack swap entirely within one @@ -285,6 +285,7 @@ use crate::dosfile::{ DosState, ERROR_FILE_NOT_OBJECT, ERROR_NO_FREE_STORE, MODE_OLDFILE, OFFSET_CURRENT, map_io_error, map_vfs_error, }; +use crate::dospattern::ERROR_LINE_TOO_LONG; use crate::exectask::{PR_ARGUMENTS_OFFSET, TC_SPLOWER, TC_SPUPPER}; use crate::guestmem::{ GuestHeap, GuestHeapError, MIN_STACK_SIZE, addr_from_bptr, bptr_from_addr, read_c_string, @@ -1022,6 +1023,15 @@ fn run_command_handler(ctx: &mut HandlerContext<'_, C>) -> Result<(), Di // segment's payload address, immediately past its header. let entry = alloc_addrs[0].wrapping_add(SEG_HEADER_SIZE); + // The arguments are copied into the Input() buffer below, so they + // must lie inside guest memory: a length past it cannot be a real + // argument string, and must not make the host allocate gigabytes. + if u64::from(param_ptr) + u64::from(param_len) > ctx.mem.len() as u64 { + ctx.dos.set_io_err(ERROR_LINE_TOO_LONG); + ctx.cpu.set_data_register(DataRegister(0), (-1i32) as u32); + return Ok(()); + } + let stack_size = requested_stack.max(MIN_STACK_SIZE); let stack_base = match ctx.heap.alloc(ctx.mem, stack_size) { Ok(addr) => addr, @@ -1047,8 +1057,8 @@ fn run_command_handler(ctx: &mut HandlerContext<'_, C>) -> Result<(), Di let old_sp_lower = ctx.mem.read_u32(task + TC_SPLOWER); let old_sp_upper = ctx.mem.read_u32(task + TC_SPUPPER); let old_pr_arguments = ctx.mem.read_u32(task + PR_ARGUMENTS_OFFSET); - let old_cmdline = ctx.dos.cmdline; - let old_cmdline_pos = ctx.dos.cmdline_pos; + let old_input_buf = std::mem::take(&mut ctx.dos.input_buf); + let old_input_pos = ctx.dos.input_pos; let entry_sp = stack_top & !3; ctx.cpu.set_address_register(AddressRegister(7), entry_sp); @@ -1058,15 +1068,15 @@ fn run_command_handler(ctx: &mut HandlerContext<'_, C>) -> Result<(), Di // a guest startup may read this instead of (or in addition to) // A0/D0, so it has to describe the same buffer. ctx.mem.write_u32(task + PR_ARGUMENTS_OFFSET, param_ptr); - ctx.dos.cmdline = Some((param_ptr, param_len)); - // ReadArgs's cursor into dos.cmdline -- must restart at the child's - // own buffer's beginning, not wherever the caller's own prior - // ReadArgs call(s) left it (this field persists across calls, see - // crate::dosargs's module doc): without resetting it here, a - // RunCommand'd program's own first ReadArgs call would start - // partway (often at the very end) of *its* command line instead of - // the start, parsing as if no arguments were given at all. - ctx.dos.cmdline_pos = 0; + // The child's arguments go into the Input() buffer, read from the + // start, and the caller's buffer comes back when the child returns: + // RunCommand "takes care of setting up the current input filehandle + // in such a way that ReadArgs() can be used in the program, and + // restores the state of the buffering before returning" (autodoc). + ctx.dos.input_buf = (0..param_len) + .map(|i| ctx.mem.read_u8(param_ptr.wrapping_add(i))) + .collect(); + ctx.dos.input_pos = 0; ctx.cpu .set_address_register(AddressRegister(6), DOS_LIBRARY_BASE); // A0/D0 = argptr/argsize verbatim, exactly as RunCommand's caller @@ -1095,8 +1105,8 @@ fn run_command_handler(ctx: &mut HandlerContext<'_, C>) -> Result<(), Di ctx.mem.write_u32(task + TC_SPUPPER, old_sp_upper); ctx.mem .write_u32(task + PR_ARGUMENTS_OFFSET, old_pr_arguments); - ctx.dos.cmdline = old_cmdline; - ctx.dos.cmdline_pos = old_cmdline_pos; + ctx.dos.input_buf = old_input_buf; + ctx.dos.input_pos = old_input_pos; if let Some(shadow) = ctx.mem.shadow_mut() { shadow.reset_stack_tracking(old_sp_lower, old_sp_upper, old_a7); } @@ -1829,6 +1839,49 @@ mod tests { ); } + #[test] + #[allow(clippy::vec_init_then_push)] // one `.push()` per word, each commented + fn run_command_rejects_arguments_past_guest_memory() { + // The arguments are copied into the Input() buffer, so a length + // reaching past guest memory has to fail before the host tries to + // allocate and copy it. The child would exit with 42. + let child_words = [move_imm_to_d(0), 0, 42, RTS]; + let tmp = TempDir::new("args-past-memory"); + fs::write(tmp.path().join("child"), hunk_file_from_words(&child_words)).unwrap(); + let name = b"SYS:child\0"; + + let mut words = Vec::new(); + let name_idx = words.len(); + words.push(move_imm_to_d(1)); // D1 = name ptr (patched below) + words.push(0); + words.push(0); + push_jsr(&mut words, 6, -150); // LoadSeg + words.push(move_d0_to_d(1)); // D1 = seglist BPTR + words.push(move_imm_to_d(2)); // D2 = stack size + words.push(0); + words.push(8192); + words.push(move_imm_to_d(3)); // D3 = paramptr + words.push(0); + words.push(0); + words.push(move_imm_to_d(4)); // D4 = paramlen: 4 GB + words.push(0xFFFF); + words.push(0xFF00); + push_jsr(&mut words, 6, -504); // RunCommand + words.push(RTS); + + let name_addr = TRAP_TABLE_END + (words.len() as u32) * 2; + patch_imm32(&mut words, name_idx, name_addr); + + let mut rt = runtime_with_program_and_extra(&words, name_addr, name, Some(tmp.path())); + let mut out = Vec::new(); + let code = rt.run(&mut out, None).expect("run should succeed"); + + assert_eq!( + code, -1, + "RunCommand should refuse the length, not run the child" + ); + } + #[test] fn run_command_child_inherits_the_callers_fpcr() { // Child: FMOVE.L FPCR,D0 ; RTS -- reports back, as its own exit @@ -1940,9 +1993,9 @@ mod tests { #[allow(clippy::vec_init_then_push)] // one `.push()` per word, each commented fn run_command_resets_readargs_cursor_for_the_child_instead_of_inheriting_the_callers() { // The parent calls ReadArgs on *its own* command line first, - // advancing DosState::cmdline_pos partway (here, all the way) + // advancing DosState::input_pos partway (here, all the way) // into the parent's buffer. Without resetting that cursor for - // the child (DosState::cmdline_pos is a single, shared field -- + // the child (DosState::input_pos is a single, shared field -- // see crate::dosargs's module doc), the child's own ReadArgs // call would start wherever the parent's left off, typically // past the end of the *child's* entirely different buffer, @@ -1988,7 +2041,7 @@ mod tests { let mut words = Vec::new(); // Parent's own ReadArgs call first, against its own command - // line ("PARENT_TOKEN", below) -- advances cmdline_pos. + // line ("PARENT_TOKEN", below) -- advances input_pos. let parent_template_idx = words.len(); words.push(move_imm_to_d(1)); words.push(0); @@ -2067,7 +2120,7 @@ mod tests { read_c_string(rt.memory(), matched_ptr), b"CHILD_TOKEN", "the child's ReadArgs should parse the child's own command line from its start, \ - not wherever the parent's prior ReadArgs call left DosState::cmdline_pos" + not wherever the parent's prior ReadArgs call left DosState::input_pos" ); }