Skip to content

feat(umsg): add unsafe try_format method surfacing umsg_format's contract - #372

Open
clydegerber wants to merge 1 commit into
google:mainfrom
clydegerber:fix/umsg-message-format-unsafe
Open

feat(umsg): add unsafe try_format method surfacing umsg_format's contract#372
clydegerber wants to merge 1 commit into
google:mainfrom
clydegerber:fix/umsg-message-format-unsafe

Conversation

@clydegerber

@clydegerber clydegerber commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

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 function umsg_format, which derives the number and types of the arguments it reads from the pattern passed to UMessageFormat::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! itself unsafe. As @filmil noted, that breaks existing callers. This revision instead leaves message_format! unchanged and fully backward compatible, and adds an opt-in alternative that surfaces the contract.

Changes (purely additive)

  • New UMessageFormat::try_format — an unsafe fn taking a tuple of arguments (f64/i32/i64/UChar). It carries the same contract as the macro but forces the caller to acknowledge it with an unsafe { .. } block:
    // SAFETY: pattern references one double argument, matching the tuple.
    let result = unsafe { fmt.try_format((43.4_f64,)) }?;
  • Documentation — a # Safety section on try_format and a # Warning: memory-safety hazards section on message_format!, both spelling out the argument-count and argument-type invariants and linking Segfault when using message_format! on an invalid message format #371.
  • Tests — a positive test for try_format, plus two #[ignore]d demonstrator tests reproducing the count and type mismatches through message_format! (kept out of CI because they trigger UB).

message_format!, format_args, and checkarg! are byte-for-byte unchanged relative to main; 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:

  • type mismatchSIGSEGV;
  • arg-count mismatch → does not crash here; silently returns garbage ("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.

@filmil

filmil commented Jul 30, 2026

Copy link
Copy Markdown
Member

This is not OK, since it breaks existing APIs. We have code that would break as a result of this change.
Please introduce a new formatter if you want to emphasize unsafety.

…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.
@clydegerber
clydegerber force-pushed the fix/umsg-message-format-unsafe branch from 195dba5 to 27cf1b4 Compare July 30, 2026 16:00
@clydegerber clydegerber changed the title feat(umsg)!: make message_format! unsafe to reflect its variadic contract feat(umsg): add unsafe try_format method surfacing umsg_format's contract Jul 30, 2026
@clydegerber

Copy link
Copy Markdown
Contributor Author

Thanks for the review, reverting the breaking change.

message_format! is now byte-for-byte identical to main (all call sites, including rust_icu_intl::PluralRules::select, are back to what they were). Instead I've added an opt-in UMessageFormat::try_format — an unsafe fn that takes a tuple of arguments and carries the same contract, so callers who want the unsafety surfaced can use:

let result = unsafe { fmt.try_format((43.4_f64,)) }?;

The # Safety/# Warning docs and the #[ignore]d demonstrator tests are kept, since they document the hazard regardless of which entry point is used.

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
supervision.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants