nash: accept bash's -c operand ordering; fix test socket paths
`bash -c` does not bind to the token that follows it: short-option parsing
continues and the command string is the first operand, so `bash -c -l <cmd>`
runs `<cmd>` as a login shell. brush's clap CLI instead binds `-c` to the next
token and rejects one shaped like an option, so every command the agent CLI
sent as `-c -l <cmd>` inside naros-agent failed with
error: a value is required for '-c <COMMAND>' but none was supplied
Since narOS diverts /bin/sh, /bin/bash and /bin/dash to nash, that broke every
command in the container. nash's compat fallback did not cover it either: it
read "-l" as the command, which parses fine as bash, so no fallback fired.
Normalize the argv before clap sees it, hoisting short options that sit between
the `-c` group and its command ahead of that group (`-c -l cmd` -> `-l -c cmd`),
including value-taking ones (`-c -o xtrace cmd`, `-c -oxtrace cmd`). Only short
options are hoisted — bash itself rejects long options once short-option parsing
has begun — and anything not positively identified as an option ends the scan,
so a command string is never mistaken for a flag. Teach nash's dash_c_command()
the same rule so the parse-failure fallback checks the real command.
Also repair the observe test harness, which could not run under a normal macOS
TMPDIR: it named its socket directory with `{:?}` of an Instant (~45 chars),
overflowing sun_path, and never cleaned up, so a recycled pid inherited a stale
socket and failed the bind with AlreadyExists. Route every scratch directory
through a helper that keeps names short, falls back to /tmp when the socket path
would not fit, clears stale state, and drops the directory when the sink dies.
Co-Authored-By: Claude Opus 4.8 <[email protected]>
This commit is contained in:
+17
-1
@@ -42,6 +42,11 @@ fn main() {
|
||||
|
||||
/// Extracts the command string from a bash-style `-c` invocation, handling
|
||||
/// combined short options (`-lc <cmd>`, `-elc <cmd>`, …).
|
||||
///
|
||||
/// bash keeps parsing short options after `-c` and takes the command from the
|
||||
/// first operand, so `-c -l <cmd>` and `-c -- <cmd>` both name `<cmd>`. Skip
|
||||
/// those intervening option tokens rather than mistaking one for the command
|
||||
/// (which would send the compat fallback the wrong input to check).
|
||||
fn dash_c_command() -> Option<String> {
|
||||
let args: Vec<String> = std::env::args().skip(1).collect();
|
||||
let mut iter = args.iter();
|
||||
@@ -54,7 +59,18 @@ fn dash_c_command() -> Option<String> {
|
||||
&& flags.chars().all(|c| c.is_ascii_alphanumeric())
|
||||
&& flags.ends_with('c')
|
||||
{
|
||||
return iter.next().cloned();
|
||||
for candidate in iter.by_ref() {
|
||||
// `--` terminates options: the very next token is the command.
|
||||
if candidate == "--" {
|
||||
return iter.next().cloned();
|
||||
}
|
||||
// Any other `-x`-shaped token is a further option, not the command.
|
||||
if candidate.len() > 1 && candidate.starts_with('-') {
|
||||
continue;
|
||||
}
|
||||
return Some(candidate.clone());
|
||||
}
|
||||
return None;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -0,0 +1,59 @@
|
||||
//! Regression tests for bash-compatible invocation shapes reaching the shipped
|
||||
//! binary. bash keeps parsing short options after `-c` and takes the command
|
||||
//! from the first operand, so `nash -c -l <cmd>` must run `<cmd>` rather than
|
||||
//! reject the invocation for a missing `-c` value.
|
||||
|
||||
use std::process::{Command, Output};
|
||||
|
||||
fn run(args: &[&str]) -> Output {
|
||||
Command::new(env!("CARGO_BIN_EXE_nash"))
|
||||
.args(args)
|
||||
.output()
|
||||
.expect("spawn nash")
|
||||
}
|
||||
|
||||
fn stdout_of(args: &[&str]) -> String {
|
||||
let output = run(args);
|
||||
assert!(
|
||||
output.status.success(),
|
||||
"nash {args:?} failed: {}",
|
||||
String::from_utf8_lossy(&output.stderr)
|
||||
);
|
||||
String::from_utf8_lossy(&output.stdout).into_owned()
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn command_follows_short_flags_after_dash_c() {
|
||||
assert_eq!(
|
||||
stdout_of(&["--noprofile", "--norc", "-c", "-l", "echo hi"]),
|
||||
"hi\n"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn command_follows_several_short_flags_after_dash_c() {
|
||||
assert_eq!(
|
||||
stdout_of(&["--noprofile", "--norc", "-c", "-e", "-u", "echo hi"]),
|
||||
"hi\n"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn short_flags_after_dash_c_still_take_effect() {
|
||||
// `-u` is hoisted ahead of `-c`, so the unset expansion must still fail.
|
||||
let output = run(&["--noprofile", "--norc", "-c", "-u", "echo ${undefined_var}"]);
|
||||
assert!(!output.status.success());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn positional_args_survive_short_flags_after_dash_c() {
|
||||
assert_eq!(
|
||||
stdout_of(&["--noprofile", "--norc", "-c", "-l", "echo $0", "myzero"]),
|
||||
"myzero\n"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn combined_dash_lc_still_works() {
|
||||
assert_eq!(stdout_of(&["--noprofile", "--norc", "-lc", "echo hi"]), "hi\n");
|
||||
}
|
||||
+52
-9
@@ -5,9 +5,46 @@
|
||||
use std::io::{Read, Write};
|
||||
use std::os::unix::net::UnixListener;
|
||||
use std::process::Command;
|
||||
use std::sync::atomic::{AtomicU32, Ordering};
|
||||
use std::sync::mpsc;
|
||||
use std::time::Duration;
|
||||
|
||||
/// Conservative floor for `sockaddr_un.sun_path` (104 on macOS, 108 on Linux),
|
||||
/// leaving room for the NUL terminator. A socket path over this fails to bind
|
||||
/// with "path must be shorter than SUN_LEN".
|
||||
const MAX_SOCKET_PATH: usize = 100;
|
||||
|
||||
/// A fresh, uniquely named scratch directory for one test.
|
||||
///
|
||||
/// `$TMPDIR` is a long `/var/folders/…` path on macOS and can be set anywhere
|
||||
/// by the caller, so a socket placed under it can blow the `sun_path` limit —
|
||||
/// keeping the directory name short is necessary but not sufficient. Fall back
|
||||
/// to `/tmp` whenever the resulting socket path would not fit; the bind limit
|
||||
/// applies to the path we pass, and `/tmp` is short on every platform we run on.
|
||||
///
|
||||
/// Names combine the pid with a per-process counter, so concurrent test threads
|
||||
/// never collide; any leftover directory from an earlier run with a recycled
|
||||
/// pid is removed rather than inherited (a stale socket file would otherwise
|
||||
/// fail the bind with `AlreadyExists`).
|
||||
fn scratch_dir(prefix: &str) -> std::path::PathBuf {
|
||||
static COUNTER: AtomicU32 = AtomicU32::new(0);
|
||||
let name = format!(
|
||||
"{prefix}-{}-{}",
|
||||
std::process::id(),
|
||||
COUNTER.fetch_add(1, Ordering::Relaxed)
|
||||
);
|
||||
|
||||
let dir = [std::env::temp_dir(), std::path::PathBuf::from("/tmp")]
|
||||
.into_iter()
|
||||
.map(|base| base.join(&name))
|
||||
.find(|dir| dir.join("control.sock").as_os_str().len() <= MAX_SOCKET_PATH)
|
||||
.expect("no temp base short enough for a unix socket path");
|
||||
|
||||
let _ = std::fs::remove_dir_all(&dir);
|
||||
std::fs::create_dir_all(&dir).unwrap();
|
||||
dir
|
||||
}
|
||||
|
||||
struct Sink {
|
||||
dir: std::path::PathBuf,
|
||||
rx: mpsc::Receiver<(Option<String>, serde_json::Value)>,
|
||||
@@ -17,10 +54,10 @@ impl Sink {
|
||||
/// Starts a unix-socket HTTP sink; each posted batch is parsed and sent
|
||||
/// through the channel along with its Authorization header.
|
||||
fn start() -> Self {
|
||||
let dir = std::env::temp_dir().join(format!("nash-test-{}-{:?}", std::process::id(), std::time::Instant::now()));
|
||||
std::fs::create_dir_all(&dir).unwrap();
|
||||
let dir = scratch_dir("nash-test");
|
||||
let socket_path = dir.join("control.sock");
|
||||
let listener = UnixListener::bind(&socket_path).unwrap();
|
||||
let listener = UnixListener::bind(&socket_path)
|
||||
.unwrap_or_else(|e| panic!("bind {} ({} bytes): {e}", socket_path.display(), socket_path.as_os_str().len()));
|
||||
let (tx, rx) = mpsc::channel();
|
||||
std::thread::spawn(move || {
|
||||
for stream in listener.incoming() {
|
||||
@@ -94,6 +131,14 @@ impl Sink {
|
||||
}
|
||||
}
|
||||
|
||||
impl Drop for Sink {
|
||||
/// Don't leave the socket behind: a stale one under a recycled pid is what
|
||||
/// turned a rerun into an `AlreadyExists` bind failure.
|
||||
fn drop(&mut self) {
|
||||
let _ = std::fs::remove_dir_all(&self.dir);
|
||||
}
|
||||
}
|
||||
|
||||
fn full_body(buf: &[u8]) -> Option<&[u8]> {
|
||||
let headers_end = buf.windows(4).position(|w| w == b"\r\n\r\n")? + 4;
|
||||
let len: usize = header(buf, "content-length")?.parse().ok()?;
|
||||
@@ -413,8 +458,7 @@ fn pipe_sigpipe_consumer_exits_early() {
|
||||
#[test]
|
||||
fn pipe_preserves_binary_stream() {
|
||||
// Byte-for-byte integrity across a tapped pipe: md5 through nash must match bash.
|
||||
let dir = std::env::temp_dir().join(format!("nash-pipebin-{}", std::process::id()));
|
||||
std::fs::create_dir_all(&dir).unwrap();
|
||||
let dir = scratch_dir("nash-pipebin");
|
||||
let src = dir.join("rand.bin");
|
||||
let data: Vec<u8> = (0..100_000u32).map(|i| (i.wrapping_mul(2654435761) >> 16) as u8).collect();
|
||||
std::fs::write(&src, &data).unwrap();
|
||||
@@ -438,8 +482,7 @@ fn redirect_preserves_seek_semantics() {
|
||||
// A seeking writer (dd with seek=) must see a real, seekable fd — the file
|
||||
// must end up byte-identical to bash, proving nash didn't interpose a pipe.
|
||||
let sink = Sink::start();
|
||||
let parent = std::env::temp_dir().join(format!("nash-seek-{}", std::process::id()));
|
||||
std::fs::create_dir_all(&parent).unwrap();
|
||||
let parent = scratch_dir("nash-seek");
|
||||
let script = "printf '0123456789' > f.bin; dd if=/dev/zero of=f.bin bs=1 seek=3 count=2 conv=notrunc 2>/dev/null; od -An -tx1 f.bin";
|
||||
let out = Command::new(env!("CARGO_BIN_EXE_nash"))
|
||||
.args(["-c", script])
|
||||
@@ -455,12 +498,12 @@ fn redirect_preserves_seek_semantics() {
|
||||
let text = String::from_utf8_lossy(&out.stdout);
|
||||
let hex: String = text.split_whitespace().collect::<Vec<_>>().join(" ");
|
||||
assert_eq!(hex, "30 31 32 00 00 35 36 37 38 39");
|
||||
let _ = std::fs::remove_dir_all(&parent);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn spool_fallback_when_socket_absent() {
|
||||
let dir = std::env::temp_dir().join(format!("nash-spool-{}", std::process::id()));
|
||||
let _ = std::fs::remove_dir_all(&dir);
|
||||
let dir = scratch_dir("nash-spool");
|
||||
let status = Command::new(env!("CARGO_BIN_EXE_nash"))
|
||||
.args(["-c", "echo spooled"])
|
||||
.env("NUCLEIC_SHELL_SOCKET", "/nonexistent/control.sock")
|
||||
|
||||
Reference in New Issue
Block a user