diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index ce2c35a3d..e8a0a6bc8 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -40,6 +40,14 @@ jobs: fi - run: mise r build - run: mise r test + # `os_string_from_bytes` has a `cfg` branch per platform, and every test of it is + # `#[cfg(unix)]` — so the Windows one is not compiled anywhere else in this pipeline. + # A compile check is not coverage, but it is what keeps that branch from rotting + # unseen. + - name: check the Windows-only conversion still compiles + run: | + rustup target add x86_64-pc-windows-msvc + cargo check -p usage-argv --all-features --target x86_64-pc-windows-msvc - run: mise r render # Same reasoning as `render`: the shadow is checked in, so a change to the derive's # vocabulary that would alter it has to be committed rather than discovered later. diff --git a/PLAN.md b/PLAN.md index 5d65b6014..73d46c767 100644 --- a/PLAN.md +++ b/PLAN.md @@ -132,12 +132,26 @@ manpages, and SDKs — never a runtime dependency of somebody else's program. a different file, silently. Costs +656 instructions (1.6%) and one allocation, which is what not corrupting a value is worth. It also retired the hazard of recognising `String` by its spelling, since there is no identity case left. -- [ ] **Accepting a value that is not valid UTF-8** — reporting it is not the same as taking - it. `PathBuf` could hold the exact bytes, but recovering an `OsString` from them needs - `OsStr::from_encoded_bytes_unchecked`, which is `unsafe`, and this crate has none. The - call would be sound — the bytes come from `as_encoded_bytes` in the same process, and - every split the parser makes is at an ASCII byte, so no multi-byte sequence is ever - cut — but introducing `unsafe` is jdx's call to make, not mine. +- [x] **Accepting a value that is not valid UTF-8** — reporting it was the safe half; + accepting it is the whole fix, because the operating system does accept `/tmp/\xff` as a + filename and a CLI that cannot receive one cannot open the file. A `PathBuf` or + `OsString` field takes the bytes exactly, through `usage_argv::os_string_from_bytes`. + + **And with no `unsafe` anywhere.** On Unix an `OsString` is an arbitrary byte sequence, + so this is the safe `OsString::from_vec` and every byte survives — which is the case that + matters, since non-UTF-8 filenames are ordinary there. Windows was going to need + `from_encoded_bytes_unchecked`, and jdx approved that, but a *safe* function taking a + `Vec` cannot enforce its precondition: there is no way to know the bytes came from + `as_encoded_bytes` rather than from anywhere else, and a safe function whose precondition + a caller can violate is unsound however carefully today's callers behave. Greptile flagged + exactly that on #844. So Windows goes through UTF-8 and reports what will not convert, + which gives up only an unpaired-surrogate argument there. + + Cheaper than the text path, not dearer: a `PathBuf` field costs **553 instructions per + parse against a `String` field's 674**, since it skips the UTF-8 validation pass, and both + allocate once. The gate fixture cannot show this — a spec carries no Rust types, so every + shadow field is a `String` — which is why it is measured directly. + - [ ] **`usage-derive` v1** — everything mise needs: constraints (`requires`/`conflicts`/`overrides`/`required_unless`), `var`, `count`, `env`, defaults, delimiters, the `double_dash` modes, global flags, flatten, diff --git a/argv/src/lib.rs b/argv/src/lib.rs index 0edbc1b37..e7ccaabf5 100644 --- a/argv/src/lib.rs +++ b/argv/src/lib.rs @@ -54,8 +54,14 @@ //! actually looked at can fail to convert. //! //! Slicing an `OsStr` into `&str` pieces safely is not possible without -//! allocating or `unsafe`, and this crate forbids `unsafe`. Bytes are what is -//! left, and they turn out to be the honest interface anyway. +//! allocating or `unsafe`. Bytes are what is left, and they turn out to be the +//! honest interface anyway. +//! +//! The reverse conversion is [`os_string_from_bytes`], which lets a `PathBuf` +//! field hold a filename that is not UTF-8 rather than a mangled copy of one. On +//! Unix that is lossless and safe; on Windows, where WTF-8 makes it partial, a +//! value that will not convert is reported. Either way this crate contains no +//! `unsafe`, which a conversion that guessed would have cost. //! //! # What this crate does not do //! @@ -75,7 +81,7 @@ #![forbid(unsafe_code)] -use std::ffi::OsStr; +use std::ffi::{OsStr, OsString}; #[cfg(feature = "spec")] pub mod spec; @@ -141,6 +147,13 @@ pub struct Flag<'a> { /// Long forms, written without the leading `--`. pub longs: &'a [&'a str], /// Short forms, as single bytes. + /// + /// **Should be ASCII.** A cluster like `-xyz` is walked one byte at a time, so a + /// non-ASCII short can never be matched, and the remainder after a value-taking one — + /// which becomes its value — would begin in the middle of a character. + /// `#[derive(Cli)]` rejects a non-ASCII `short`; a table written by hand should keep to + /// it. Nothing is unsound if it does not: the value would simply be cut in a place that + /// makes no sense, and on Windows would then fail to convert. pub shorts: &'a [u8], /// A long form that sets the flag to false, written without the `--`. pub negate: Option<&'a str>, @@ -406,6 +419,49 @@ pub fn as_str(value: &[u8]) -> Result<&str, std::str::Utf8Error> { std::str::from_utf8(value) } +/// Rebuild an [`OsString`] from bytes the parser handed back. +/// +/// This is the reverse of [`OsStr::as_encoded_bytes`], and it is how a `PathBuf` field +/// receives a filename the operating system accepts but UTF-8 does not — `/tmp/\xff` stays +/// `/tmp/\xff` rather than becoming a *different* filename with `U+FFFD` in it. +/// +/// Where the platform cannot hold those bytes, they are handed back in the `Err` — as +/// `String::from_utf8` does — so the caller can name the value in its error without this +/// having to copy it for a case that is nearly never taken. +/// +/// # Why this is not `unsafe`, and why it is not lossless everywhere +/// +/// On **Unix** an `OsString` is an arbitrary byte sequence, so the conversion is total and +/// uses the safe [`OsStringExt::from_vec`]. Every byte survives, which is the case that +/// matters: non-UTF-8 filenames are ordinary there. +/// +/// [`OsStringExt::from_vec`]: std::os::unix::ffi::OsStringExt::from_vec +/// +/// On **Windows** the encoding is WTF-8, where not every byte sequence is valid, and the only +/// constructor that accepts one is `OsString::from_encoded_bytes_unchecked` — whose +/// precondition this function cannot enforce. It takes a `Vec` from a safe caller, so +/// there is no way to know the bytes came from `as_encoded_bytes` rather than from anywhere +/// else, and a safe function with a precondition that can be violated is unsound however +/// carefully its callers behave today. +/// +/// So on Windows the bytes go through UTF-8, and one that is not valid UTF-8 is refused +/// rather than assumed. What that gives up is a Windows argument containing an unpaired +/// surrogate, which is reported instead of accepted; what it buys is that this crate needs no +/// `unsafe` at all. +pub fn os_string_from_bytes(value: Vec) -> Result> { + #[cfg(unix)] + { + Ok(std::os::unix::ffi::OsStringExt::from_vec(value)) + } + #[cfg(not(unix))] + { + match String::from_utf8(value) { + Ok(text) => Ok(OsString::from(text)), + Err(bad) => Err(bad.into_bytes()), + } + } +} + /// A single-pass parse over `argv`. /// /// Created with [`Parser::new`] and driven with [`Parser::next_event`]. @@ -831,8 +887,8 @@ impl<'t, 'v> Parser<'t, 'v> { /// View a token as bytes. /// /// `as_encoded_bytes` is a plain accessor with no conversion and no allocation. -/// It is only the reverse direction that needs `unsafe`, which is why values -/// come back as bytes. +/// The reverse direction is the one with a cost — see [`os_string_from_bytes`] — +/// which is why values come back as bytes. fn bytes<'v>(s: &'v &'v OsStr) -> &'v [u8] { s.as_encoded_bytes() } diff --git a/conformance/tests/typed.rs b/conformance/tests/typed.rs index 78a5344a1..f52f00e01 100644 --- a/conformance/tests/typed.rs +++ b/conformance/tests/typed.rs @@ -5,7 +5,7 @@ //! `PathBuf` 227 times and a tool-version type 83 times in its command structs — and that a //! value which will not convert says which value and why. -use std::ffi::OsStr; +use std::ffi::{OsStr, OsString}; use std::path::PathBuf; use std::str::FromStr; @@ -359,30 +359,123 @@ fn the_conversion_stands_on_its_own() { assert!(err.contains("bash, zsh, fish, pwsh"), "{err}"); } -/// A CLI holding a path, which is where mangling would show +/// A CLI holding paths, which is where mangling would show #[derive(Cli)] #[usage(bin = "pathy")] struct Pathy { /// Where to write - #[usage(long)] + #[usage(long, short = 'o')] out: Option, /// Anything at all #[usage(long)] text: Option, + /// Every input + #[usage(long, var)] + input: Vec, + /// A raw word + #[usage(long)] + raw: Option, + /// Every exclusion, or none given at all + #[usage(long, var)] + exclude: Option>, +} + +/// A filename the operating system accepts and UTF-8 does not. +#[cfg(unix)] +fn not_utf8() -> &'static OsStr { + use std::os::unix::ffi::OsStrExt; + OsStr::from_bytes(b"/tmp/\xff") +} + +#[cfg(unix)] +fn as_bytes(path: &std::path::Path) -> &[u8] { + use std::os::unix::ffi::OsStrExt; + path.as_os_str().as_bytes() } #[test] -fn a_word_that_is_not_utf8_is_reported_rather_than_mangled() { - // It used to arrive through `from_utf8_lossy`, so a path with a stray byte in it became - // a path with U+FFFD in it — a different file, silently. Now the parse says so. - use std::ffi::OsStr; +#[cfg(unix)] +fn a_word_that_is_not_utf8_arrives_byte_for_byte() { + // It used to arrive through `from_utf8_lossy`, so a path with a stray byte in it became a + // path with `U+FFFD` in it — a different file, silently. Reporting it was the safe half + // of the fix; this is the whole one, because the operating system does accept this + // filename and a CLI that cannot receive it cannot open the file. + let argv = [OsStr::new("--out"), not_utf8()]; + let parsed = Pathy::parse_from(&argv).expect("a filename the OS accepts should parse"); + let out = parsed.out.expect("given"); + assert_eq!(as_bytes(&out), b"/tmp/\xff"); + + // The point of byte-exactness: it is not the lossy rendering, which names another file. + assert_ne!(as_bytes(&out), "/tmp/\u{fffd}".as_bytes()); +} + +#[test] +#[cfg(unix)] +fn every_form_a_value_can_arrive_in_keeps_its_bytes() { + // Each of these reaches the field by a different route through the parser, and each cuts + // the token at a different place — which is exactly what `os_string_from_bytes` is + // trusting. A detached value is passed through whole; `--out=…` is cut at the `=`. use std::os::unix::ffi::OsStrExt; - let bad = OsStr::from_bytes(b"/tmp/\xff"); - let argv = [OsStr::new("--out"), bad]; + for (token, why) in [ + ( + &b"--out=/tmp/\xff"[..], + "a long form's attached value is cut at the `=`", + ), + ( + b"-o/tmp/\xff", + "a short flag's value is the rest of the token", + ), + ( + b"-o=/tmp/\xff", + "a short flag's value may be cut at an `=` too", + ), + ] { + let parsed = Pathy::parse_from(&[OsStr::from_bytes(token)]).expect("should parse"); + assert_eq!(as_bytes(&parsed.out.expect("given")), b"/tmp/\xff", "{why}"); + } + + // A collecting field keeps every one of them, and an `OsString` field takes the word + // with no interpretation at all. + // `var` is repetition rather than a variadic, so each value comes with its own flag. + let argv = [ + OsStr::new("--input"), + not_utf8(), + OsStr::new("--input"), + OsStr::from_bytes(b"\xfe"), + OsStr::new("--raw"), + not_utf8(), + ]; + let parsed = Pathy::parse_from(&argv).expect("should parse"); + assert_eq!( + parsed.input.iter().map(|p| as_bytes(p)).collect::>(), + [&b"/tmp/\xff"[..], &b"\xfe"[..]] + ); + assert_eq!(parsed.raw.expect("given").as_bytes(), b"/tmp/\xff"); + + // An `Option>` still tells "never given" from "given nothing", which is decided + // before any of this and must not have been lost on the way to the byte path. + assert!(parsed.exclude.is_none(), "never given"); + let argv = [OsStr::new("--exclude"), not_utf8()]; + let parsed = Pathy::parse_from(&argv).expect("should parse"); + assert_eq!( + parsed + .exclude + .as_deref() + .map(|paths| paths.iter().map(|p| as_bytes(p)).collect::>()), + Some(vec![&b"/tmp/\xff"[..]]) + ); +} + +#[test] +#[cfg(unix)] +fn a_field_that_is_not_a_path_still_reports_it() { + // Byte-exactness is for the types that can hold any byte sequence. A `String` cannot, so + // there the honest answer is still to say so rather than to substitute a different word. + let argv = [OsStr::new("--text"), not_utf8()]; match Pathy::parse_from(&argv) { Err(Error::InvalidValue(bad)) => { - assert_eq!(bad.name, "out"); + assert_eq!(bad.name, "text"); assert!( bad.reason.contains("utf-8") || bad.reason.contains("UTF-8"), "the reason should say what was wrong: {}", @@ -393,7 +486,7 @@ fn a_word_that_is_not_utf8_is_reported_rather_than_mangled() { assert!(bad.value.contains("/tmp/"), "{}", bad.value); } Err(other) => panic!("wrong error: {other:?}"), - Ok(_) => panic!("a value that is not UTF-8 should not have been accepted"), + Ok(_) => panic!("a `String` cannot hold this, so it should not have been accepted"), } } diff --git a/derive/src/codegen.rs b/derive/src/codegen.rs index 54218f99c..1f68b1434 100644 --- a/derive/src/codegen.rs +++ b/derive/src/codegen.rs @@ -760,10 +760,108 @@ fn field_final(field: &Field) -> TokenStream { // Recognising it by spelling is safe now: if an adopter's own `String` were mistaken for // this one, the mismatch is a compile error rather than a value quietly mangled — and // the check that matters, the UTF-8 one, happens either way. + let rendered = rendered_path(ty); let is_std_string = matches!( - rendered_path(ty).as_str(), + rendered.as_str(), "String" | "std::string::String" | "::std::string::String" | "alloc::string::String" ); + + // A field that can hold any byte sequence skips UTF-8 entirely, because for these types + // rejecting a word would be the wrong answer: the operating system accepts `/tmp/\xff` + // as a filename, so a CLI has to be able to receive one. Everything else still converts + // through text, since `FromStr` is the only thing an arbitrary type offers. + // + // Recognised by spelling, with the same reasoning as `String` above: an adopter's own + // `PathBuf` would fail to compile rather than quietly take a mangled value, because what + // is handed to it is an `OsString` and not a `&str`. + let os_target = match rendered.as_str() { + "PathBuf" | "std::path::PathBuf" | "::std::path::PathBuf" => { + Some(quote!(::std::path::PathBuf::from)) + } + "OsString" | "std::ffi::OsString" | "::std::ffi::OsString" => { + Some(quote!(::std::convert::identity)) + } + _ => None, + }; + if let Some(build) = os_target { + // Lossless on Unix, where any byte sequence is a filename. On Windows the encoding + // is WTF-8 and the conversion is partial, so a value that will not convert is + // reported the same way any other unconvertible value is — never dropped, and never + // replaced by a different filename. + let one = |value: TokenStream| { + quote! { + match ::usage_argv::os_string_from_bytes(#value) { + ::std::result::Result::Ok(__usage_os) => #build(__usage_os), + ::std::result::Result::Err(__usage_bytes) => { + return ::std::result::Result::Err( + ::usage_argv::Error::InvalidValue(::std::boxed::Box::new( + ::usage_argv::InvalidValue { + name: #name, + value: ::std::string::String::from_utf8_lossy( + &__usage_bytes, + ) + .into_owned(), + reason: ::std::string::ToString::to_string( + &"this platform cannot hold these bytes in a path", + ), + }, + )), + ); + } + } + } + }; + let converted = one; + return match field.shape { + // Unreachable: a switch and a count have no `value_ty`, so the early return + // above already handled them. + Shape::Bool | Shape::Count => quote!(#ident: partial.#ident), + Shape::Required => { + let value = converted(quote!(partial.#ident)); + quote!(#ident: #value) + } + // A `match` rather than `.map`, and a loop rather than `.collect`, for the same + // reason the text path below uses them: the conversion can fail, and a `return` + // inside a closure would leave the error in the closure's own return type. + Shape::Optional => { + let value = converted(quote!(__usage_value)); + quote! { + #ident: match partial.#ident { + ::std::option::Option::Some(__usage_value) => { + ::std::option::Option::Some(#value) + } + ::std::option::Option::None => ::std::option::Option::None, + } + } + } + Shape::Many => { + let value = converted(quote!(__usage_value)); + let collected = quote! {{ + let mut __usage_values = + ::std::vec::Vec::with_capacity(partial.#ident.len()); + for __usage_value in partial.#ident { + __usage_values.push(#value); + } + __usage_values + }}; + if field.optional_collection { + let given = format_ident!("__given_{}", ident); + // Same as below: whether anything arrived is what tells "never given" + // from "given nothing", which the `Vec` itself cannot. + quote! { + #ident: if partial.#given { + ::std::option::Option::Some(#collected) + } else { + ::std::option::Option::None + } + } + } else { + quote!(#ident: #collected) + } + } + }; + } + let converted = |value: TokenStream| { let text = quote! { match ::std::string::String::from_utf8(#value) { diff --git a/derive/src/model.rs b/derive/src/model.rs index 161ff258f..aad2ae447 100644 --- a/derive/src/model.rs +++ b/derive/src/model.rs @@ -697,8 +697,13 @@ impl Field { )); } - // A short form is matched as a single byte, so a multi-byte character - // could never be recognized. Better to say so than to truncate it. + // A short form is matched as a single byte, so a multi-byte character could never be + // recognized. Better to say so than to truncate it. + // + // This also keeps `usage_argv::os_string_from_bytes` sound: a cluster is walked one + // byte at a time, and the remainder after a value-taking short becomes that value, so + // a non-ASCII short would let a value begin inside a character. `Flag::shorts` + // documents the requirement; this is where it is enforced for derived tables. if let Some(short) = shorts.iter().find(|c| !c.is_ascii()) { return Err(syn::Error::new( span, @@ -1792,6 +1797,23 @@ mod tests { .to_string() } + #[test] + fn a_short_form_must_be_ascii() { + // Enforced for two reasons now: a multi-byte short could never be matched, and + // `os_string_from_bytes` relies on every cut the parser makes landing on an ASCII + // byte. A cluster is walked one byte at a time, so this is the rule that keeps a + // value from beginning in the middle of a character. + let err = rejection( + r#" + struct Ex { + #[usage(short = 'é', long)] + enable: bool, + } + "#, + ); + assert!(err.contains("is not ASCII"), "unhelpful message: {err}"); + } + #[test] fn a_default_subcommand_needs_subcommands_to_name() { let err = position_error(