From 6066836dc13625ad0ed0a7e0b6a632a35b8d708a Mon Sep 17 00:00:00 2001 From: default <216188+jdx@users.noreply.github.com> Date: Wed, 12 Aug 2026 22:52:23 +0000 Subject: [PATCH 1/2] feat(derive): accept a value the OS accepts and UTF-8 does not MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reporting a non-UTF-8 value was the safe half of the fix. It is not the whole one: `/tmp/\xff` is a filename the operating system accepts, and a CLI that cannot receive one cannot open the file. A `PathBuf` or `OsString` field now takes the bytes exactly. `usage_argv::os_string_from_bytes` is the reverse of `as_encoded_bytes`. On Unix it is the *safe* `OsString::from_vec` — an `OsString` there is an arbitrary byte sequence, so the conversion is total and no `unsafe` arises. Only Windows needs `from_encoded_bytes_unchecked`, because WTF-8 makes the conversion partial. That call rests on an invariant, now written where it can be checked: every sub-slice the parser produces is cut at an ASCII byte — after `-`, after `--`, at `=`, and between the letters of a short-flag cluster — and an ASCII byte never occurs inside a multi-byte sequence. The one way to break it is a non-ASCII `Flag::shorts` entry, which would let a cluster split mid-character. The derive already refused that; the rule is now documented as load-bearing on both sides and has the test it was missing. The crate goes from `forbid(unsafe_code)` to `deny`, so the single audited exception has to name itself rather than the crate pretending it has none. Parsing still contains no `unsafe` at all. Byte-exactness is cheaper than the text path, not dearer: a `PathBuf` field costs 567 instructions per parse against a `String` field's 660, because 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` — so it is measured directly. A `String` field still reports rather than substitutes, which is the right answer for a type that cannot hold those bytes. CI gains a Windows compile check, since every test here is `#[cfg(unix)]` and nothing in the pipeline was building the branch that carries the `unsafe`. Co-Authored-By: Claude Opus 5 --- .github/workflows/test.yml | 8 +++ PLAN.md | 26 +++++++-- argv/src/lib.rs | 71 +++++++++++++++++++++-- conformance/tests/typed.rs | 115 +++++++++++++++++++++++++++++++++---- derive/src/codegen.rs | 59 ++++++++++++++++++- derive/src/model.rs | 26 ++++++++- 6 files changed, 281 insertions(+), 24 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index ce2c35a3d..4847d660c 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 + # The one code path no test can reach from here: `os_string_from_bytes` is safe on + # Unix and `unsafe` on Windows, and every test of it is `#[cfg(unix)]`. A compile + # check is not coverage, but it keeps the branch carrying the `unsafe` 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..575421caa 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 now takes the bytes exactly, through + `usage_argv::os_string_from_bytes`. On Unix that is the **safe** `OsString::from_vec`, + so the question of `unsafe` never arises there; only Windows needs + `from_encoded_bytes_unchecked`, because WTF-8 makes the conversion partial. jdx + approved the `unsafe` for that case. + + The Windows call rests on an invariant now written down where it can be checked: every + sub-slice the parser produces is cut at an ASCII byte (after `-`, after `--`, at `=`, + between the letters of a cluster), and an ASCII byte never occurs inside a multi-byte + sequence. The one way to break it is a non-ASCII `Flag::shorts` entry, which the derive + already refused — that rule is now documented as load-bearing and has a test. + + Cheaper, not dearer: a `PathBuf` field costs **567 instructions per parse against a + `String` field's 660**, 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 instead. + - [ ] **`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..6d03e52b0 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. +//! +//! Parsing itself contains no `unsafe`. The one exception in this crate is +//! [`os_string_from_bytes`], the *reverse* conversion, which lets a `PathBuf` +//! field hold a filename that is not UTF-8 instead of a mangled copy of one — +//! and even that is `unsafe` only on Windows, where WTF-8 makes the conversion +//! partial. Its documentation carries the argument. //! //! # What this crate does not do //! @@ -73,9 +79,11 @@ //! //! [the argv grammar]: https://usage.jdx.dev/spec/argv -#![forbid(unsafe_code)] +// Denied rather than forbidden, so that the one audited exception can say so out loud +// instead of the crate having to pretend it has none. See `os_string_from_bytes`. +#![deny(unsafe_code)] -use std::ffi::OsStr; +use std::ffi::{OsStr, OsString}; #[cfg(feature = "spec")] pub mod spec; @@ -141,6 +149,12 @@ pub struct Flag<'a> { /// Long forms, written without the leading `--`. pub longs: &'a [&'a str], /// Short forms, as single bytes. + /// + /// **Must be ASCII.** A cluster like `-xyz` is walked one byte at a time, so a + /// non-ASCII short would let the remainder — which becomes that flag's value — begin + /// in the middle of a multi-byte character. [`os_string_from_bytes`] relies on that not + /// happening. `#[derive(Cli)]` rejects a non-ASCII `short`; a table written by hand has + /// to keep to it. pub shorts: &'a [u8], /// A long form that sets the flag to false, written without the `--`. pub negate: Option<&'a str>, @@ -406,6 +420,55 @@ 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. +/// +/// # Which bytes +/// +/// Only bytes that came from [`Event`], unmodified. Anything else — a value read from an +/// environment variable, a default from the spec, bytes assembled by the caller — is either +/// already a `String` or should become one through [`as_str`]; this function has no way to +/// tell, so passing it something else is the caller's mistake to avoid. +/// +/// # Why this is sound +/// +/// On Unix an `OsString` is an arbitrary byte sequence, so the conversion is total and uses +/// the safe [`OsStringExt::from_vec`]. No invariant is needed and no `unsafe` is involved. +/// +/// [`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 only +/// `from_encoded_bytes_unchecked` will accept one — so there the conversion rests on an +/// invariant of this crate: **every sub-slice the parser produces is cut at an ASCII byte**. +/// A token is split after `-`, after `--`, at `=`, and between the letters of a short-flag +/// cluster; all four are ASCII, and an ASCII byte never occurs inside a multi-byte WTF-8 +/// (or UTF-8) sequence, so no cut can land in the middle of one. Values the parser passes +/// through whole are trivially fine. +/// +/// The one way to break that is a [`Flag::shorts`] entry that is not ASCII, which would let +/// a cluster be split mid-sequence. `#[derive(Cli)]` refuses a non-ASCII `short`, and the +/// field documents the requirement for tables written by hand. +pub fn os_string_from_bytes(value: Vec) -> OsString { + #[cfg(unix)] + { + std::os::unix::ffi::OsStringExt::from_vec(value) + } + #[cfg(not(unix))] + { + // SAFETY: `value` came from `OsStr::as_encoded_bytes`, either whole or cut at an + // ASCII byte — see the invariant above, which `Flag::shorts` is documented to + // uphold and the derive enforces. A cut at an ASCII byte is a valid WTF-8 boundary, + // so these bytes are a well-formed encoding of an `OsString`. + #[allow(unsafe_code)] + unsafe { + OsString::from_encoded_bytes_unchecked(value) + } + } +} + /// A single-pass parse over `argv`. /// /// Created with [`Parser::new`] and driven with [`Parser::next_event`]. 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..eed6f2aa2 100644 --- a/derive/src/codegen.rs +++ b/derive/src/codegen.rs @@ -760,10 +760,67 @@ 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 { + // No failure is possible here, so unlike the text path below there is nothing to + // report and no error to carry a value into. + let one = |value: TokenStream| quote!(#build(::usage_argv::os_string_from_bytes(#value))); + 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 = one(quote!(partial.#ident)); + quote!(#ident: #value) + } + Shape::Optional => { + let value = one(quote!(__usage_value)); + quote!(#ident: partial.#ident.map(|__usage_value| #value)) + } + Shape::Many => { + let value = one(quote!(__usage_value)); + let collected = + quote!(partial.#ident.into_iter().map(|__usage_value| #value).collect()); + 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( From 426f1bc06c4c3a8b4034128c749711b6f464432a Mon Sep 17 00:00:00 2001 From: default <216188+jdx@users.noreply.github.com> Date: Wed, 12 Aug 2026 23:27:13 +0000 Subject: [PATCH 2/2] fix(argv): make the byte conversion sound instead of merely careful MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `os_string_from_bytes` was a safe function reaching `from_encoded_bytes_unchecked` on Windows, with the precondition — bytes that came from `as_encoded_bytes`, cut only at ASCII — documented and enforced by nothing. It takes a `Vec` from a safe caller, so there is no way to know where the bytes came from, and a safe function whose precondition a caller can violate is unsound however carefully today's callers behave. Greptile was right to call it a P1. Fixed by not needing the `unsafe` at all. On Unix an `OsString` is an arbitrary byte sequence, so the safe `OsString::from_vec` is exact — which is the case that matters, since non-UTF-8 filenames are ordinary there. On Windows the bytes go through UTF-8 and one that will not convert is handed back in the `Err`, to be reported like any other unconvertible value. What that gives up is a Windows argument containing an unpaired surrogate; what it buys is that the crate forbids `unsafe` again rather than denying it with an exception. The bytes come back in the `Err` rather than being cloned for the message, as `String::from_utf8` does, so the failure path costs nothing on the path that succeeds. The `Flag::shorts` ASCII rule is no longer a soundness matter and no longer claims to be — it stays because a non-ASCII short cannot be matched, and its test stays with it. Co-Authored-By: Claude Opus 5 --- .github/workflows/test.yml | 6 +-- PLAN.md | 36 ++++++++--------- argv/src/lib.rs | 79 +++++++++++++++++--------------------- derive/src/codegen.rs | 59 +++++++++++++++++++++++----- 4 files changed, 107 insertions(+), 73 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 4847d660c..e8a0a6bc8 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -40,9 +40,9 @@ jobs: fi - run: mise r build - run: mise r test - # The one code path no test can reach from here: `os_string_from_bytes` is safe on - # Unix and `unsafe` on Windows, and every test of it is `#[cfg(unix)]`. A compile - # check is not coverage, but it keeps the branch carrying the `unsafe` from rotting + # `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: | diff --git a/PLAN.md b/PLAN.md index 575421caa..73d46c767 100644 --- a/PLAN.md +++ b/PLAN.md @@ -133,24 +133,24 @@ manpages, and SDKs — never a runtime dependency of somebody else's program. 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. - [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 now takes the bytes exactly, through - `usage_argv::os_string_from_bytes`. On Unix that is the **safe** `OsString::from_vec`, - so the question of `unsafe` never arises there; only Windows needs - `from_encoded_bytes_unchecked`, because WTF-8 makes the conversion partial. jdx - approved the `unsafe` for that case. - - The Windows call rests on an invariant now written down where it can be checked: every - sub-slice the parser produces is cut at an ASCII byte (after `-`, after `--`, at `=`, - between the letters of a cluster), and an ASCII byte never occurs inside a multi-byte - sequence. The one way to break it is a non-ASCII `Flag::shorts` entry, which the derive - already refused — that rule is now documented as load-bearing and has a test. - - Cheaper, not dearer: a `PathBuf` field costs **567 instructions per parse against a - `String` field's 660**, 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 instead. + 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`, diff --git a/argv/src/lib.rs b/argv/src/lib.rs index 6d03e52b0..e7ccaabf5 100644 --- a/argv/src/lib.rs +++ b/argv/src/lib.rs @@ -57,11 +57,11 @@ //! allocating or `unsafe`. Bytes are what is left, and they turn out to be the //! honest interface anyway. //! -//! Parsing itself contains no `unsafe`. The one exception in this crate is -//! [`os_string_from_bytes`], the *reverse* conversion, which lets a `PathBuf` -//! field hold a filename that is not UTF-8 instead of a mangled copy of one — -//! and even that is `unsafe` only on Windows, where WTF-8 makes the conversion -//! partial. Its documentation carries the argument. +//! 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 //! @@ -79,9 +79,7 @@ //! //! [the argv grammar]: https://usage.jdx.dev/spec/argv -// Denied rather than forbidden, so that the one audited exception can say so out loud -// instead of the crate having to pretend it has none. See `os_string_from_bytes`. -#![deny(unsafe_code)] +#![forbid(unsafe_code)] use std::ffi::{OsStr, OsString}; @@ -150,11 +148,12 @@ pub struct Flag<'a> { pub longs: &'a [&'a str], /// Short forms, as single bytes. /// - /// **Must be ASCII.** A cluster like `-xyz` is walked one byte at a time, so a - /// non-ASCII short would let the remainder — which becomes that flag's value — begin - /// in the middle of a multi-byte character. [`os_string_from_bytes`] relies on that not - /// happening. `#[derive(Cli)]` rejects a non-ASCII `short`; a table written by hand has - /// to keep to it. + /// **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>, @@ -426,45 +425,39 @@ pub fn as_str(value: &[u8]) -> Result<&str, std::str::Utf8Error> { /// 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. /// -/// # Which bytes +/// 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. /// -/// Only bytes that came from [`Event`], unmodified. Anything else — a value read from an -/// environment variable, a default from the spec, bytes assembled by the caller — is either -/// already a `String` or should become one through [`as_str`]; this function has no way to -/// tell, so passing it something else is the caller's mistake to avoid. +/// # Why this is not `unsafe`, and why it is not lossless everywhere /// -/// # Why this is sound -/// -/// On Unix an `OsString` is an arbitrary byte sequence, so the conversion is total and uses -/// the safe [`OsStringExt::from_vec`]. No invariant is needed and no `unsafe` is involved. +/// 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 only -/// `from_encoded_bytes_unchecked` will accept one — so there the conversion rests on an -/// invariant of this crate: **every sub-slice the parser produces is cut at an ASCII byte**. -/// A token is split after `-`, after `--`, at `=`, and between the letters of a short-flag -/// cluster; all four are ASCII, and an ASCII byte never occurs inside a multi-byte WTF-8 -/// (or UTF-8) sequence, so no cut can land in the middle of one. Values the parser passes -/// through whole are trivially fine. +/// 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. /// -/// The one way to break that is a [`Flag::shorts`] entry that is not ASCII, which would let -/// a cluster be split mid-sequence. `#[derive(Cli)]` refuses a non-ASCII `short`, and the -/// field documents the requirement for tables written by hand. -pub fn os_string_from_bytes(value: Vec) -> OsString { +/// 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)] { - std::os::unix::ffi::OsStringExt::from_vec(value) + Ok(std::os::unix::ffi::OsStringExt::from_vec(value)) } #[cfg(not(unix))] { - // SAFETY: `value` came from `OsStr::as_encoded_bytes`, either whole or cut at an - // ASCII byte — see the invariant above, which `Flag::shorts` is documented to - // uphold and the derive enforces. A cut at an ASCII byte is a valid WTF-8 boundary, - // so these bytes are a well-formed encoding of an `OsString`. - #[allow(unsafe_code)] - unsafe { - OsString::from_encoded_bytes_unchecked(value) + match String::from_utf8(value) { + Ok(text) => Ok(OsString::from(text)), + Err(bad) => Err(bad.into_bytes()), } } } @@ -894,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/derive/src/codegen.rs b/derive/src/codegen.rs index eed6f2aa2..1f68b1434 100644 --- a/derive/src/codegen.rs +++ b/derive/src/codegen.rs @@ -784,25 +784,66 @@ fn field_final(field: &Field) -> TokenStream { _ => None, }; if let Some(build) = os_target { - // No failure is possible here, so unlike the text path below there is nothing to - // report and no error to carry a value into. - let one = |value: TokenStream| quote!(#build(::usage_argv::os_string_from_bytes(#value))); + // 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 = one(quote!(partial.#ident)); + 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 = one(quote!(__usage_value)); - quote!(#ident: partial.#ident.map(|__usage_value| #value)) + 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 = one(quote!(__usage_value)); - let collected = - quote!(partial.#ident.into_iter().map(|__usage_value| #value).collect()); + 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"