fix(umsg): reject too few arguments instead of reading past the varargs - #408
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
`umsg_format` is a C variadic function. It decides how many arguments to
read from the *pattern*, not from what the caller passed. The number it
reads is the highest argument index in the pattern, plus one.
So `message_format!(fmt, {s => String})` on the pattern `"String : {1}"`
makes ICU read two arguments when only one was passed. It reads the
second from past the end of the argument list and, because a bare `{1}`
is a string argument, dereferences that indeterminate value as a
`UChar*`. That segfaults.
Scan the pattern for its highest argument index when the formatter is
built, and refuse a format call that supplies fewer arguments than that.
The check happens before the variadic call, so the out-of-bounds read
never occurs.
The scanner mirrors ICU's `MessagePattern` grammar: apostrophe quoting
(including the `#`-in-plural and `|`-in-choice cases), nested
submessages in `plural`, `select`, `selectordinal` and `choice`, and
simple argument styles, whose braces and quotes hold no arguments. It is
conservative by construction: anything it does not model exactly makes
it decline to report a count, which leaves the previous behavior in
place. A count that is too large would reject calls ICU handles fine, so
declining is always the safer answer.
Verified against ICU 74.2 by measuring, for 47 patterns, how many
arguments `umsg_format` really reads, and comparing that with the
scanner. All 47 agree. The measurement passes k valid `UChar*` arguments
followed by NULLs: ICU raises U_ILLEGAL_ARGUMENT_ERROR as soon as it
reads a NULL for a string argument, so the smallest k that succeeds is
the number of arguments ICU reads.
The program from the bug report changes from a segfault to:
Error: Wrapper(message pattern needs 2 argument(s) because it
refers to argument index 1, but 1 argument(s) were supplied)
`arg_count_mismatch_is_undefined_behavior` documented the crash and was
`#[ignore]`d because it aborted the test binary. It now runs in the
normal suite as `too_few_arguments_is_an_error`. The type-mismatch
hazard is unaffected and stays `#[ignore]`d: a pattern cannot tell you
what Rust type the caller wrote.
$ cargo test -p rust_icu_umsg
test result: ok. 23 passed; 0 failed; 1 ignored; 0 measured
$ cargo check --workspace
Finished `dev` profile [unoptimized + debuginfo] target(s)
$ cargo test --workspace
test result: ok. (all suites, 0 failed)
aba612d to
f7097b9
Compare
clydegerber
left a comment
There was a problem hiding this comment.
Would it be a better idea to fail formatting when the argument count cannot be scanned rather than continuing and passing it along to ICU? If a consumer fat fingers a message format such that it fails the scan, how easily will they be able to pinpoint the cause of the failure?
We might want to not restrict things that icu4c already permits, even if confusing. |
Fixes #371
Root cause
umsg_formatis a C variadic function. It decides how many arguments to read from the pattern, not from what the caller passed. The number it reads is the highest argument index in the pattern, plus one.The pattern in the bug report,
"String : {1}", refers to index 1, so ICU reads two arguments. The report supplies one. ICU reads the second from past the end of the argument list and, because a bare{1}is a string argument, dereferences that indeterminate value as aUChar*.Whether that crashes depends on what happens to be in the register or stack slot ICU reads. The reported program is on the lucky side on Linux with ICU 74.2 — the slot reads as NULL and ICU returns
U_ILLEGAL_ARGUMENT_ERROR. Push the missing arguments onto the stack instead and it is not lucky:That is the same defect as the one reported, just with the indeterminate value reliably non-NULL.
The change
Scan the pattern for its highest argument index when the formatter is built, and refuse a format call that supplies fewer arguments than that. The check runs before the variadic call, so the out-of-bounds read never happens.
The scanner in the new
pattern.rsmirrors ICU'sMessagePatterngrammar:'#only quotes insideplural/selectordinal,'|only insidechoice, and a doubled apostrophe inside quoted text keeps the text quoted;plural,select,selectordinalandchoice, whose arguments count too, while the braces that open a submessage after a selector do not;{0,number,##.#}, whose braces and quotes hold no arguments;offset:in a plural style.It is conservative by construction. Anything it does not model exactly makes it decline to report a count, which leaves the previous behavior in place for that pattern. A count that is too large would reject calls ICU handles fine, so declining is always the safer answer. Named arguments (
{name}) also return no count;umsg_formatrejects those patterns itself, before it reads anything.The argument type hazard is unaffected. A pattern cannot tell you what Rust type the caller wrote, so that one stays the caller's obligation and its
#[ignore]d test stays as it is. The docs onmessage_format!andtry_formatnow say which of the two obligations is checked and which is not.Verification
Built against ICU 74.2 on Linux.
The scanner was checked against what ICU actually does, not just against its own expectations. For 47 patterns, a throwaway harness measured how many arguments
umsg_formatreally reads and compared that with the scanner's answer. The measurement passes k validUChar*arguments followed by NULLs: ICU raisesU_ILLEGAL_ARGUMENT_ERRORas soon as it reads a NULL for a string argument, so the smallest k that succeeds is the number of arguments ICU reads. A second variant passes a leadingf64for the patterns whose argument 0 is a double (plural,choice,number,date), since a double travels in a different register class.All 47 agree exactly. A sample:
The harness is not part of the change — it calls
umsg_formatwith deliberately wrong arguments, which does not belong in the test suite.The program from the bug report, unchanged, now prints an error rather than crashing:
arg_count_mismatch_is_undefined_behaviordocumented this crash and was#[ignore]d because it aborted the test binary. It now runs in the normal suite astoo_few_arguments_is_an_error, alongside tests for contiguous indices, extra arguments still being harmless, nested arguments counting, and quoted arguments not counting.rust_icu_intlis the one in-tree caller ofmessage_format!; itsPluralRulespattern needs one argument and supplies one, and its tests pass.cargo clippy -p rust_icu_umsg --all-targetsreports the same single pre-existing warning asmain.Nothing under
.github/workflows/is touched.rust_icu_umsg/BUILD.bazelglobssrc/**/*.rs, so the new module needs no Bazel change.Generated by Claude Code