diff --git a/src/runtime/shell/Builtin.rs b/src/runtime/shell/Builtin.rs index 6ce494deb4ed..6927194ab6a3 100644 --- a/src/runtime/shell/Builtin.rs +++ b/src/runtime/shell/Builtin.rs @@ -1077,22 +1077,22 @@ impl Builtin { e: &ParseError, set_wait_err: impl FnOnce(), ) -> Yield { - let buf: Vec = match e { - ParseError::IllegalOption(_) => Self::fmt_error_arena( + let buf: Vec = match *e { + ParseError::IllegalOption(ch) => Self::fmt_error_arena( interp, cmd, Some(kind), - format_args!("illegal option -- {}\n", bstr::BStr::new(e.opt())), + format_args!("illegal option -- {}\n", bstr::BStr::new(&[ch])), ) .to_vec(), ParseError::ShowUsage => kind.usage_string().to_vec(), - ParseError::Unsupported(_) => Self::fmt_error_arena( + ParseError::Unsupported(name) => Self::fmt_error_arena( interp, cmd, Some(kind), format_args!( "unsupported option, please open a GitHub issue -- {}\n", - bstr::BStr::new(e.opt()) + bstr::BStr::new(name) ), ) .to_vec(), diff --git a/src/runtime/shell/builtin/cat.rs b/src/runtime/shell/builtin/cat.rs index bd510becc71d..c242a918e6e2 100644 --- a/src/runtime/shell/builtin/cat.rs +++ b/src/runtime/shell/builtin/cat.rs @@ -410,7 +410,7 @@ impl FlagParser for Opts { None } - fn parse_short(&mut self, ch: u8, smallflags: &[u8], i: usize) -> Option { + fn parse_short(&mut self, ch: u8) -> Option { match ch { b'b' => Some(ParseFlagResult::Unsupported(unsupported_flag(b"-b"))), b'e' => Some(ParseFlagResult::Unsupported(unsupported_flag(b"-e"))), @@ -419,9 +419,7 @@ impl FlagParser for Opts { b't' => Some(ParseFlagResult::Unsupported(unsupported_flag(b"-t"))), b'u' => Some(ParseFlagResult::Unsupported(unsupported_flag(b"-u"))), b'v' => Some(ParseFlagResult::Unsupported(unsupported_flag(b"-v"))), - _ => Some(ParseFlagResult::IllegalOption( - &raw const smallflags[1 + i..], - )), + _ => Some(ParseFlagResult::IllegalOption(ch)), } } } diff --git a/src/runtime/shell/builtin/cp.rs b/src/runtime/shell/builtin/cp.rs index 8a7a39d19a51..475af8b22ced 100644 --- a/src/runtime/shell/builtin/cp.rs +++ b/src/runtime/shell/builtin/cp.rs @@ -792,7 +792,7 @@ impl FlagParser for Opts { None } - fn parse_short(&mut self, ch: u8, smallflags: &[u8], i: usize) -> Option { + fn parse_short(&mut self, ch: u8) -> Option { match ch { b'f' => Some(ParseFlagResult::Unsupported(unsupported_flag(b"-f"))), b'H' => Some(ParseFlagResult::Unsupported(unsupported_flag(b"-H"))), @@ -809,7 +809,7 @@ impl FlagParser for Opts { Some(ParseFlagResult::ContinueParsing) } b'n' => Some(ParseFlagResult::ContinueParsing), - _ => Some(ParseFlagResult::IllegalOption(&raw const smallflags[i..])), + _ => Some(ParseFlagResult::IllegalOption(ch)), } } } diff --git a/src/runtime/shell/builtin/ls.rs b/src/runtime/shell/builtin/ls.rs index 8f6213a5dcad..71502210be60 100644 --- a/src/runtime/shell/builtin/ls.rs +++ b/src/runtime/shell/builtin/ls.rs @@ -44,7 +44,8 @@ pub struct ExecState { enum ParseFlag { ContinueParsing, Done, - IllegalOption(Box<[u8]>), + /// The rejected option byte, as getopt(3) reports it. + IllegalOption(u8), } impl Ls { @@ -72,12 +73,12 @@ impl Ls { // which case we run once with ".". let paths_start = match Self::parse_opts(interp, cmd) { Ok(p) => p, - Err(opt) => { + Err(ch) => { let buf: Vec = Builtin::fmt_error_arena( interp, cmd, Some(Kind::Ls), - format_args!("illegal option -- {}\n", bstr::BStr::new(&opt[..])), + format_args!("illegal option -- {}\n", bstr::BStr::new(&[ch])), ) .to_vec(); Self::state_mut(interp, cmd).state = State::WaitingWriteErr; @@ -221,7 +222,7 @@ impl Ls { /// Returns the index of the first non-flag arg, or `None` if there are no /// positional args. `Err` carries the offending flag byte. - fn parse_opts(interp: &Interpreter, cmd: NodeId) -> Result, Box<[u8]>> { + fn parse_opts(interp: &Interpreter, cmd: NodeId) -> Result, u8> { let argc = Builtin::of(interp, cmd).args_slice().len(); if argc == 0 { return Ok(None); @@ -232,7 +233,7 @@ impl Ls { match Self::parse_flag(&mut Self::state_mut(interp, cmd).opts, flag) { ParseFlag::Done => return Ok(Some(idx)), ParseFlag::ContinueParsing => {} - ParseFlag::IllegalOption(s) => return Err(s), + ParseFlag::IllegalOption(ch) => return Err(ch), } idx += 1; } @@ -245,7 +246,7 @@ impl Ls { } // FIXME windows if flag.len() == 1 { - return ParseFlag::IllegalOption(Box::from(&b"-"[..])); + return ParseFlag::IllegalOption(b'-'); } for &ch in &flag[1..] { match ch { @@ -259,7 +260,7 @@ impl Ls { | b'h' | b'H' | b'i' | b'I' | b'k' | b'L' | b'm' | b'n' | b'N' | b'o' | b'p' | b'q' | b'Q' | b's' | b'S' | b't' | b'T' | b'u' | b'U' | b'v' | b'w' | b'x' | b'X' | b'Z' => {} - _ => return ParseFlag::IllegalOption(Box::from(&flag[1..2])), + _ => return ParseFlag::IllegalOption(ch), } } ParseFlag::ContinueParsing diff --git a/src/runtime/shell/builtin/mkdir.rs b/src/runtime/shell/builtin/mkdir.rs index a4695d61c6b2..8c882d9cbdd8 100644 --- a/src/runtime/shell/builtin/mkdir.rs +++ b/src/runtime/shell/builtin/mkdir.rs @@ -443,7 +443,7 @@ impl FlagParser for Opts { None } - fn parse_short(&mut self, ch: u8, smallflags: &[u8], i: usize) -> Option { + fn parse_short(&mut self, ch: u8) -> Option { match ch { b'm' => Some(ParseFlagResult::Unsupported(unsupported_flag(b"-m "))), b'p' => { @@ -454,9 +454,7 @@ impl FlagParser for Opts { self.verbose = true; None } - _ => Some(ParseFlagResult::IllegalOption( - &raw const smallflags[1 + i..], - )), + _ => Some(ParseFlagResult::IllegalOption(ch)), } } } diff --git a/src/runtime/shell/builtin/mv.rs b/src/runtime/shell/builtin/mv.rs index 915d34c3bfaf..a4decf0d7b32 100644 --- a/src/runtime/shell/builtin/mv.rs +++ b/src/runtime/shell/builtin/mv.rs @@ -46,7 +46,8 @@ pub enum MvState { /// mv uses its own simpler parser. enum MvParseError { - IllegalOption(&'static [u8]), + /// The rejected option byte. + IllegalOption(u8), ShowUsage, } @@ -94,11 +95,11 @@ impl Mv { Tag::Idle => { if let Err(e) = Self::parse_opts(interp, cmd) { let buf: Vec = match e { - MvParseError::IllegalOption(s) => Builtin::fmt_error_arena( + MvParseError::IllegalOption(ch) => Builtin::fmt_error_arena( interp, cmd, Some(Kind::Mv), - format_args!("illegal option -- {}\n", bstr::BStr::new(s)), + format_args!("illegal option -- {}\n", bstr::BStr::new(&[ch])), ) .to_vec(), MvParseError::ShowUsage => Kind::Mv.usage_string().to_vec(), @@ -360,7 +361,7 @@ impl Mv { return Ok(()); } MvFlag::ContinueParsing => {} - MvFlag::IllegalOption(s) => return Err(MvParseError::IllegalOption(s)), + MvFlag::IllegalOption(ch) => return Err(MvParseError::IllegalOption(ch)), } idx += 1; } @@ -374,7 +375,7 @@ impl Mv { for &ch in &flag[1..] { match ch { b'f' | b'h' | b'i' | b'n' | b'v' => {} - _ => return MvFlag::IllegalOption(b"-"), + _ => return MvFlag::IllegalOption(ch), } } MvFlag::ContinueParsing @@ -396,7 +397,8 @@ impl Drop for Mv { enum MvFlag { ContinueParsing, Done, - IllegalOption(&'static [u8]), + /// The rejected option byte. + IllegalOption(u8), } /// `openat(target, O_RDONLY|O_DIRECTORY)` diff --git a/src/runtime/shell/builtin/rm.rs b/src/runtime/shell/builtin/rm.rs index 8c4cde84e18d..6c27a343fcd3 100644 --- a/src/runtime/shell/builtin/rm.rs +++ b/src/runtime/shell/builtin/rm.rs @@ -92,8 +92,8 @@ pub enum PromptBehaviour { enum RmParseFlag { ContinueParsing, Done, - IllegalOption, - IllegalOptionWithFlag, + /// The rejected option byte, as getopt(3) reports it (`-` for an unknown `--long` option). + IllegalOption(u8), } impl Rm { @@ -240,40 +240,15 @@ impl Rm { }); continue; } - RmParseFlag::IllegalOption => { - return Self::write_err_literal( - interp, - cmd, - idx, - b"rm: illegal option -- -\n", - ); - } - RmParseFlag::IllegalOptionWithFlag => { - if let Some(safeguard) = Builtin::of(interp, cmd).stderr.needs_io() { - Self::state_mut(interp, cmd).state = RmState::ParseOpts { - idx, - wait_write_err: true, - }; - let child = ChildPtr::new(cmd, WriterTag::Builtin); - return Builtin::of_mut(interp, cmd).stderr.enqueue_fmt( - child, - Some(Kind::Rm), - format_args!( - "illegal option -- {}\n", - bstr::BStr::new(&arg[1..]) - ), - safeguard, - ); - } + RmParseFlag::IllegalOption(ch) => { let buf = Builtin::fmt_error_arena( interp, cmd, Some(Kind::Rm), - format_args!("illegal option -- {}\n", bstr::BStr::new(&arg[1..])), + format_args!("illegal option -- {}\n", bstr::BStr::new(&[ch])), ) .to_vec(); - let _ = Builtin::write_no_io(interp, cmd, IoKind::Stderr, &buf); - return Builtin::done(interp, cmd, 1); + return Self::write_err_literal(interp, cmd, idx, &buf); } } } @@ -518,7 +493,7 @@ impl Rm { opts.prompt_behaviour = PromptBehaviour::Always; RmParseFlag::ContinueParsing } - _ => RmParseFlag::IllegalOption, + _ => RmParseFlag::IllegalOption(b'-'), }; } for &ch in &flag[1..] { @@ -532,7 +507,7 @@ impl Rm { b'd' => opts.remove_empty_dirs = true, b'i' => opts.prompt_behaviour = PromptBehaviour::Once { removed_count: 0 }, b'I' => opts.prompt_behaviour = PromptBehaviour::Always, - _ => return RmParseFlag::IllegalOptionWithFlag, + _ => return RmParseFlag::IllegalOption(ch), } } RmParseFlag::ContinueParsing diff --git a/src/runtime/shell/builtin/touch.rs b/src/runtime/shell/builtin/touch.rs index 95ae1398deee..65ccc33eb09a 100644 --- a/src/runtime/shell/builtin/touch.rs +++ b/src/runtime/shell/builtin/touch.rs @@ -364,7 +364,7 @@ impl FlagParser for Opts { } } - fn parse_short(&mut self, ch: u8, smallflags: &[u8], i: usize) -> Option { + fn parse_short(&mut self, ch: u8) -> Option { match ch { b'a' => Some(ParseFlagResult::Unsupported(unsupported_flag(b"-a"))), b'c' => Some(ParseFlagResult::Unsupported(unsupported_flag(b"-c"))), @@ -373,9 +373,7 @@ impl FlagParser for Opts { b'm' => Some(ParseFlagResult::Unsupported(unsupported_flag(b"-m"))), b'r' => Some(ParseFlagResult::Unsupported(unsupported_flag(b"-r"))), b't' => Some(ParseFlagResult::Unsupported(unsupported_flag(b"-t"))), - _ => Some(ParseFlagResult::IllegalOption( - &raw const smallflags[1 + i..], - )), + _ => Some(ParseFlagResult::IllegalOption(ch)), } } } diff --git a/src/runtime/shell/interpreter.rs b/src/runtime/shell/interpreter.rs index f0e6da780e48..985fa650919f 100644 --- a/src/runtime/shell/interpreter.rs +++ b/src/runtime/shell/interpreter.rs @@ -2303,44 +2303,26 @@ pub(crate) fn shell_openat( // ──────────────────────────────────────────────────────────────────────────── /// Custom parse error for invalid options. -/// -/// Payload slices borrow from the builtin's argv (NUL-terminated arena strings) -/// or are `'static` literals; the builtin formats them into an error message -/// before the next argv mutation, so a raw fat pointer is safe. pub(crate) enum ParseError { - IllegalOption(*const [u8]), - Unsupported(*const [u8]), + /// The rejected option byte, as getopt(3) reports it. + IllegalOption(u8), + Unsupported(&'static [u8]), ShowUsage, } -impl ParseError { - /// Borrow the option-name payload. The pointer borrows either a `'static` - /// literal (e.g. `b"-"`) or the owning `Builtin`'s argv storage - /// (NUL-terminated `Vec` in `Cmd::args`, live for the `Builtin`'s - /// lifetime — see [`Builtin::arg_bytes`](crate::shell::builtin::Builtin::arg_bytes)). - /// Builtins format the error before any argv mutation. - #[inline] - pub(crate) fn opt(&self) -> &[u8] { - match self { - // SAFETY: see doc comment. - ParseError::IllegalOption(s) | ParseError::Unsupported(s) => unsafe { &**s }, - ParseError::ShowUsage => b"", - } - } -} - pub enum ParseFlagResult { ContinueParsing, Done, - IllegalOption(*const [u8]), - Unsupported(*const [u8]), + /// The rejected option byte, as getopt(3) reports it. + IllegalOption(u8), + Unsupported(&'static [u8]), } /// Returns just `name` and lets the caller's `fmt_error_arena` add the /// "unsupported option" prefix once. #[inline] -pub(crate) const fn unsupported_flag(name: &'static [u8]) -> *const [u8] { - std::ptr::from_ref::<[u8]>(name) +pub(crate) const fn unsupported_flag(name: &'static [u8]) -> &'static [u8] { + name } /// Per-builtin opts type implements this to plug into `FlagParser::parse_flags`. @@ -2348,7 +2330,7 @@ pub trait FlagParser { /// Handle a `--long` flag. Return `None` to fall through to short parsing. fn parse_long(&mut self, flag: &[u8]) -> Option; /// Handle one byte of a `-abc` cluster. Return `None` to keep iterating. - fn parse_short(&mut self, ch: u8, smallflags: &[u8], i: usize) -> Option; + fn parse_short(&mut self, ch: u8) -> Option; } /// Returns the trailing non-flag args (`args[idx..]`) on success. @@ -2379,16 +2361,15 @@ fn parse_one_flag(opts: &mut O, flag: &[u8]) -> ParseFlagResult { return ParseFlagResult::Done; } if flag.len() == 1 { - return ParseFlagResult::IllegalOption(std::ptr::from_ref::<[u8]>(b"-")); + return ParseFlagResult::IllegalOption(b'-'); } if flag.len() > 2 && flag[1] == b'-' { if let Some(r) = opts.parse_long(flag) { return r; } } - let small_flags = &flag[1..]; - for (i, &ch) in small_flags.iter().enumerate() { - if let Some(r) = opts.parse_short(ch, small_flags, i) { + for &ch in &flag[1..] { + if let Some(r) = opts.parse_short(ch) { return r; } } diff --git a/test/js/bun/shell/commands/ls.test.ts b/test/js/bun/shell/commands/ls.test.ts index 3c7ea5176cc3..07d11d4dae7e 100644 --- a/test/js/bun/shell/commands/ls.test.ts +++ b/test/js/bun/shell/commands/ls.test.ts @@ -288,17 +288,11 @@ describe.concurrent("bunshell ls", () => { }); test("invalid flag", async () => { - await TestBuilder.command`ls -z` - .exitCode(1) - .stderr(s => expect(s).toContain("illegal option")) - .run(); + await TestBuilder.command`ls -z`.exitCode(1).stderr("ls: illegal option -- z\n").run(); }); test("invalid combined flags", async () => { - await TestBuilder.command`ls -az` - .exitCode(1) - .stderr(s => expect(s).toContain("illegal option")) - .run(); + await TestBuilder.command`ls -az`.exitCode(1).stderr("ls: illegal option -- z\n").run(); }); test.if(isPosix)("permission denied directory", async () => { diff --git a/test/js/bun/shell/commands/rm.test.ts b/test/js/bun/shell/commands/rm.test.ts index d442eb197773..92f14a2a31fd 100644 --- a/test/js/bun/shell/commands/rm.test.ts +++ b/test/js/bun/shell/commands/rm.test.ts @@ -30,6 +30,13 @@ describe.concurrent("bunshell rm", () => { .doesNotExist("node_modules") .runAsTest("node_modules"); + // With .quiet() stderr is a buffer rather than an fd, the other way a parse error gets written. + test("illegal option in a cluster", async () => { + const { stderr, exitCode } = await $`rm -rz`.quiet(); + expect(stderr.toString()).toBe("rm: illegal option -- z\n"); + expect(exitCode).toBe(1); + }); + test("force", async () => { const files = { "existent.txt": "", diff --git a/test/js/bun/shell/exec.test.ts b/test/js/bun/shell/exec.test.ts index b7c272197124..d85a08bb1d91 100644 --- a/test/js/bun/shell/exec.test.ts +++ b/test/js/bun/shell/exec.test.ts @@ -46,8 +46,8 @@ describe("bun exec", () => { // prettier-ignore const programs = [ // ["cat", 1, "", ""], - ["touch", 1, "touch: illegal option -- help\n", ""], - ["mkdir", 1, "mkdir: illegal option -- help\n", ""], + ["touch", 1, "touch: illegal option -- -\n", ""], + ["mkdir", 1, "mkdir: illegal option -- -\n", ""], // ["cd", 1, "cd: no such file or directory: --help\n", ""], ["echo", 0, "", "--help\n"], ["pwd", 1, "pwd: too many arguments\n", ""], @@ -71,6 +71,49 @@ describe("bun exec", () => { } }); + // Like getopt(3), the message names the one byte that was rejected, wherever + // it sits in a cluster of short flags. An unknown --long option is rejected + // at its second `-`, so it is reported as `-` (what BSD getopt prints too). + describe.concurrent("illegal option names the rejected flag", () => { + // prettier-ignore + const programs: [program: string, cases: [args: string, rejected: string][]][] = [ + ["cat", [["-z", "z"], ["-zb", "z"], ["--bogus", "-"]]], + ["touch", [["-z", "z"], ["-za", "z"], ["--bogus", "-"]]], + ["mkdir", [["-z", "z"], ["-pz", "z"], ["-zp", "z"], ["--bogus", "-"]]], + ["cp", [["-z", "z"], ["-zR", "z"], ["--bogus", "-"]]], + ["ls", [["-z", "z"], ["-az", "z"], ["-za", "z"], ["--bogus", "-"]]], + ["rm", [["-z", "z"], ["-rz", "z"], ["-zr", "z"], ["--bogus", "-"]]], + ["mv", [["-z", "z"], ["-fz", "z"], ["-zf", "z"], ["--bogus", "-"]]], + ]; + for (const [program, cases] of programs) { + for (const [args, rejected] of cases) { + TestBuilder.command`${BUN} exec ${`${program} ${args}`}` + // cat and cp are builtins only on Windows unless this flag is set. + .env({ ...bunEnv, BUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS: "1" }) + .exitCode(1) + .stderr(`${program}: illegal option -- ${rejected}\n`) + .stdout("") + .runAsTest(`${program} ${args}`); + } + } + }); + + // Recognised but unimplemented options take the other branch of the same parser result. + describe.concurrent("unsupported option names the option", () => { + for (const [program, option] of [ + ["cat", "-n"], + ["touch", "--no-create"], + ["cp", "-i"], + ]) { + TestBuilder.command`${BUN} exec ${`${program} ${option}`}` + .env({ ...bunEnv, BUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS: "1" }) + .exitCode(1) + .stderr(`${program}: unsupported option, please open a GitHub issue -- ${option}\n`) + .stdout("") + .runAsTest(`${program} ${option}`); + } + }); + TestBuilder.command`${BUN} exec cd` .env(bunEnv) .exitCode(0)