feat(umsg): add unsafe try_format method surfacing umsg_format's contract - #372
feat(umsg): add unsafe try_format method surfacing umsg_format's contract#372clydegerber wants to merge 1 commit into
Conversation
|
This is not OK, since it breaks existing APIs. We have code that would break as a result of this change. |
…ract `message_format!` expands to a call into the ICU C variadic function `umsg_format`, which derives the number and types of the arguments it reads from the *pattern*, not from the arguments actually supplied. Neither the macro nor the compiler reconciles the two, so a mismatch is undefined behavior (segfault or silent memory corruption) reachable from safe-looking code, as reported in google#371. Rather than change `message_format!` (which would break existing callers), add an opt-in, explicit-`unsafe` alternative and document the hazard: - Add `UMessageFormat::try_format`, an `unsafe fn` taking a tuple of arguments. It carries the same contract as the macro but forces the caller to acknowledge it with an `unsafe { .. }` block. `message_format!` is left unchanged and fully backward compatible. - Document both hazards (argument-count and argument-type mismatch) in a "Safety" section on `try_format` and a "Warning" section on the macro, linking google#371. - Add a positive test for `try_format`, and two `#[ignore]`d demonstrator tests reproducing the count and type mismatches via `message_format!`. The count mismatch is platform-dependent: on macOS with ICU 73 it does not crash but silently returns garbage rather than segfaulting as in google#371. This commit was created by an automated coding assistant, with human supervision.
195dba5 to
27cf1b4
Compare
|
Thanks for the review, reverting the breaking change.
let result = unsafe { fmt.try_format((43.4_f64,)) }?;The Since this PR does not actually stop the #371 segfault I've changed the PR to say "Refs #371" rather than "Closes" for that reason. This comment was created by an automated coding assistant, with human |
Summary
Addresses the segfault reported in #371 without breaking the existing API, per review feedback.
message_format!expands to a call into the ICU C variadic functionumsg_format, which derives the number and types of the arguments it reads from the pattern passed toUMessageFormat::try_from, not from the arguments actually supplied. Nothing reconciles the two, so a mismatch (wrong argument count or wrong argument type) is undefined behavior — segfault or silent memory corruption — reachable from safe-looking code.The previous revision of this PR made
message_format!itselfunsafe. As @filmil noted, that breaks existing callers. This revision instead leavesmessage_format!unchanged and fully backward compatible, and adds an opt-in alternative that surfaces the contract.Changes (purely additive)
UMessageFormat::try_format— anunsafe fntaking a tuple of arguments (f64/i32/i64/UChar). It carries the same contract as the macro but forces the caller to acknowledge it with anunsafe { .. }block:# Safetysection ontry_formatand a# Warning: memory-safety hazardssection onmessage_format!, both spelling out the argument-count and argument-type invariants and linking Segfault when usingmessage_format!on an invalid message format #371.try_format, plus two#[ignore]d demonstrator tests reproducing the count and type mismatches throughmessage_format!(kept out of CI because they trigger UB).message_format!,format_args, andcheckarg!are byte-for-byte unchanged relative tomain; the diff is +207/−2 (the two deletions are a superseded doc note).Note on platform-dependence
Observed when running the ignored tests on macOS with ICU 73:
SIGSEGV;"String : ").So #371's segfault is platform/ICU-version-dependent (same shape seen previously in
rust_icu_utext). Either outcome is unsound.Testing
cargo test -p rust_icu_umsg -p rust_icu_intl: unit + doc tests pass; the two UB tests are ignored; no warnings.Refs #371
This commit was created by an automated coding assistant, with human
supervision.