release/2.1: Fix off-by-one errors in CollationSpecialPrimariesValidated by sffc · Pull Request #8075 · unicode-org/icu4x

sffc · 2026-06-12T23:00:21Z

Depends on #8078

On release/2.2: #8081
On main: #8080

This PR fixes a critical off-by-one bug in icu_collator 2.1.x that causes a panic when using AlternateHandling::Shifted with MaxVariable::Currency.

This bug was found by the Unicode conformance testing project (credit to @sven-oly).

Cause

When converting CollationSpecialPrimaries to CollationSpecialPrimariesValidated, the last_primaries vector was incorrectly truncated to MaxVariable::Currency as usize (3) instead of MaxVariable::Currency as usize + 1 (4). This resulted in last_primaries lacking the 4th element (index 3, for Currency), causing a panic (unwrap on None) during comparison when last_primary_for_group was called.
Additionally, compressible_bytes extraction was misaligned by one index and failed the length check, causing it to always fall back to hardcoded defaults.

Fix

Corrected the off-by-one errors in both Collator::try_new_unstable and CollatorBorrowed::try_new in comparison.rs by adding 1 to MaxVariable::Currency as usize where appropriate.

~~Also bumped icu_collator to 2.1.2.~~ --> this will be done in another PR

Context on main branch

On the main branch, this was refactored. The intermediate CollationSpecialPrimariesValidated struct was removed, and CollationSpecialPrimaries is used directly. The bug is naturally avoided there because the deserialization logic correctly uses MaxVariable::Currency as usize + 1 to split the data:
See: provider.rs:L635 on main

🤖 This pull request was created by an AI agent working with @sffc.

Changelog

icu_collator (2.1.2)

Fix panic when using AlternateHandling::Shifted with MaxVariable::Currency (off-by-one in special primaries validation).

Manishearth

It's kind of hard to understand what actually broke here, but that's because I don't understand this part of the collation api

Manishearth · 2026-06-13T00:52:49Z

        let special_primaries = special_primaries.map_project(|csp, _| {
            let compressible_bytes = (csp.last_primaries.len()
-                == MaxVariable::Currency as usize + 16)
+                == MaxVariable::Currency as usize + 17)


While we're here we should add a comment fixing the magic numbers

Manishearth · 2026-06-13T00:53:07Z

                        .as_maybe_borrowed()?
                        .as_ule_slice()
-                        .get((MaxVariable::Currency as usize)..)?
+                        .get((MaxVariable::Currency as usize + 1)..)?


and just explaining the +1 here

Manishearth · 2026-06-13T00:53:55Z

                        .as_slice()
                        .as_ule_slice()
-                        .split_at(MaxVariable::Currency as usize)
+                        .split_at(MaxVariable::Currency as usize + 1)


actually, should these be static methods on MaxVariable? after_currency and something different for the 17 one?

Manishearth · 2026-06-13T00:54:50Z

 // TODO: Consider testing ff-Adlm for supplementary-plane tailoring, including contractions
+
+#[test]
+fn test_repro_user_bug() {


please explain this test

🤖 Done! The test has been renamed to test_shifted_max_variable_currency and a detailed comment has been added to explain what it is verifying.

Manishearth · 2026-06-13T01:31:50Z

@sffc Also I believe our patch release stance is to merge to main and then copy a patch over ,yes?

sffc · 2026-06-13T05:21:36Z

@sffc Also I believe our patch release stance is to merge to main and then copy a patch over ,yes?

The bug is in 2.1. It is fixed on main due to other refactorings, perhaps #7891

I should check 2.2

Manishearth · 2026-06-13T06:31:21Z

Ah, okay

robertbastian · 2026-06-15T12:56:08Z

also here, why is this needed? if this is needed it should be a separate PR on the release branch ("Fix CI") and not part of this commit, which might be cherry-picked, patched, etc.

robertbastian · 2026-06-15T12:56:58Z

+    (
+        "collator",
+        icu::collator::provider::MARKERS,
+        "version = \"2.1.2\"",


there's no data diff, why are you bumping this?

good catch! I missed this. Fixed.

robertbastian · 2026-06-15T13:02:00Z

+    let payload: DataPayload<CollationSpecialPrimariesV1> =
+        provider.load(Default::default()).unwrap().payload;
+    let csp = payload.get();
+    assert!(csp.last_primaries.len() > MaxVariable::Currency as usize);


issue: some condition on the data struct is not a "user bug". this seems to be testing implementation details, why does the constructor call below not suffice as a test?

Checking the root cause seems harmless, but since you labeled this "issue", I assume you consider this blocking, so I will remove this extra assertion.

🤖 Done! The extra assertion has been removed, leaving only the constructor and comparison test.

robertbastian · 2026-06-15T13:07:11Z

we have a pinned toolchain, a pinned clippy version, a pinned MSRV, etc., so why does this unrelated code need to change?

Somehow this got patched into the 2.1 branch without the docs. fixing in #8078

robertbastian

Context on 2.2 (main)

In ICU4X 2.2 (main branch), this was refactored.

Main is not 2.2, main is 2.3. This bug exists on 2.2, in fact users that depend on version = "2" will be on 2.2, so this should be fixed first and foremost on the 2.2 release branch, and then backported to other releases if needed (2.1, 2.0). This PR only fixes users that depend on version = "~2.1", which, while the conformance project does that, is a small minority of users.

robertbastian · 2026-06-15T13:22:56Z

@sffc Also I believe our patch release stance is to merge to main and then copy a patch over ,yes?

The bug is in 2.1. It is fixed on main due to other refactorings, perhaps #7891

I should check 2.2

It would still be good to have the test on main

sffc · 2026-06-15T22:51:35Z

The force-push was to make this PR sit cleanly on top of the CI-fixing commits which are moved into #8078

sffc · 2026-06-15T23:17:00Z

There are now 4 PRs:

The CI fixes from here are split into Fix pre-existing CI failures on release/2.1 branch (experimental docs & tidy) #8078
The fix is ported to release/2.2: release/2.2: Fix off-by-one errors in CollationSpecialPrimariesValidated #8081
The test is added to main: Add regression test for Collator AlternateHandling::Shifted with MaxVariable::Currency #8080
And this PR is the fix on release/2.1

dpulls · 2026-06-15T23:17:51Z

🎉 All dependencies have been resolved !

…ariable::Currency (#8080) See #8075, #8081 Co-authored-by: Gemini <176961590+gemini-code-assist[bot]@users.noreply.github.com>

…erialization (#8083) See #8075 Co-authored-by: Gemini <176961590+gemini-code-assist[bot]@users.noreply.github.com>

robertbastian

I don't particularly like the architecture of the constants, but as this is not main I don't care. I have left comments on #8083.

~~Also bumped icu_collator to 2.1.2.~~ --> this will be done in another PR

This should be done in the same PR, doing this in a different PR is extra work for downstream users if they want to patch this fix.

sffc · 2026-06-16T21:45:36Z

This should be done in the same PR, doing this in a different PR is extra work for downstream users if they want to patch this fix.

I thought we decoupled fixes from the Cargo.toml diff, since people who vendor don't particularly care about the Cargo.toml diff, and it makes it harder to apply the same diff across multiple versions (like 2.1 and 2.2)?

sffc · 2026-06-16T22:13:25Z

I'll update this after #8089 lands.

Fixes a critical off-by-one bug in icu_collator 2.1.x that causes a panic when using AlternateHandling::Shifted with MaxVariable::Currency. When converting CollationSpecialPrimaries to CollationSpecialPrimariesValidated, the last_primaries vector was incorrectly truncated to MaxVariable::Currency as usize (3) instead of MaxVariable::Currency as usize + 1 (4). This resulted in last_primaries lacking the 4th element (index 3, for Currency), causing a panic (unwrap on None) during comparison when last_primary_for_group was called. Additionally, compressible_bytes extraction was misaligned by one index and failed the length check, causing it to always fall back to hardcoded defaults. This commit squashes the fix and the regression test into a single clean commit. Co-authored-by: Gemini <176961590+gemini-code-assist[bot]@users.noreply.github.com>

sffc commented Jun 12, 2026

View reviewed changes

Comment thread components/collator/tests/tests.rs Outdated

sffc changed the title ~~Fix off-by-one errors in CollationSpecialPrimariesValidated~~ 2.1 Branch: Fix off-by-one errors in CollationSpecialPrimariesValidated Jun 12, 2026

sffc marked this pull request as ready for review June 12, 2026 23:29

sffc requested review from a team, Manishearth, echeran and hsivonen as code owners June 12, 2026 23:29

sffc mentioned this pull request Jun 12, 2026

ICU4X collation fails in ICU78 version with several tests unicode-org/conformance#552

Open

Manishearth reviewed Jun 13, 2026

View reviewed changes

robertbastian reviewed Jun 15, 2026

View reviewed changes

robertbastian requested changes Jun 15, 2026

View reviewed changes

sffc force-pushed the fix-collator-2.1 branch from e16fa02 to 5e5ab3d Compare June 15, 2026 22:50

sffc force-pushed the fix-collator-2.1 branch from 5e5ab3d to 720fcda Compare June 15, 2026 22:53

sffc mentioned this pull request Jun 15, 2026

release/2.2: Fix off-by-one errors in CollationSpecialPrimariesValidated #8081

Merged

sffc changed the title ~~2.1 Branch: Fix off-by-one errors in CollationSpecialPrimariesValidated~~ release/2.1: Fix off-by-one errors in CollationSpecialPrimariesValidated Jun 15, 2026

sffc mentioned this pull request Jun 15, 2026

Add regression test for Collator AlternateHandling::Shifted with MaxVariable::Currency #8080

Merged

sffc requested review from Manishearth and robertbastian June 15, 2026 23:13

sffc force-pushed the fix-collator-2.1 branch from 720fcda to b477351 Compare June 15, 2026 23:18

sffc added a commit that referenced this pull request Jun 16, 2026

Add regression test for Collator AlternateHandling::Shifted with MaxV…

0bda1f1

…ariable::Currency (#8080) See #8075, #8081 Co-authored-by: Gemini <176961590+gemini-code-assist[bot]@users.noreply.github.com>

sffc force-pushed the fix-collator-2.1 branch from b477351 to 78ef0a0 Compare June 16, 2026 01:07

sffc mentioned this pull request Jun 16, 2026

Replace magic numbers with constants in CollationSpecialPrimaries deserialization #8083

Merged

sffc force-pushed the fix-collator-2.1 branch from 78ef0a0 to 90cf32f Compare June 16, 2026 01:27

sffc force-pushed the fix-collator-2.1 branch from 90cf32f to fbcdd1c Compare June 16, 2026 01:42

Manishearth approved these changes Jun 16, 2026

View reviewed changes

sffc added a commit that referenced this pull request Jun 16, 2026

Replace magic numbers with constants in CollationSpecialPrimaries des…

498fb39

…erialization (#8083) See #8075 Co-authored-by: Gemini <176961590+gemini-code-assist[bot]@users.noreply.github.com>

robertbastian reviewed Jun 16, 2026

View reviewed changes

sffc force-pushed the fix-collator-2.1 branch from fbcdd1c to d7aea51 Compare June 18, 2026 01:28

sffc requested a review from robertbastian June 18, 2026 01:32

robertbastian approved these changes Jun 18, 2026

View reviewed changes

sffc merged commit 491bfab into unicode-org:release/2.1 Jun 19, 2026
29 checks passed

sffc deleted the fix-collator-2.1 branch June 19, 2026 04:01

sffc mentioned this pull request Jun 19, 2026

release/2.1: Bump icu_collator to 2.1.2 #8107

Open

Conversation

sffc commented Jun 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Cause

Fix

Context on main branch

Changelog

icu_collator (2.1.2)

Uh oh!

Uh oh!

Manishearth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Manishearth commented Jun 13, 2026

Uh oh!

sffc commented Jun 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Manishearth commented Jun 13, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertbastian left a comment

Choose a reason for hiding this comment

Context on 2.2 (main)

Uh oh!

robertbastian commented Jun 15, 2026

Uh oh!

sffc commented Jun 15, 2026

Uh oh!

sffc commented Jun 15, 2026

Uh oh!

dpulls Bot commented Jun 15, 2026

Uh oh!

robertbastian left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sffc commented Jun 16, 2026

Uh oh!

sffc commented Jun 16, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sffc commented Jun 12, 2026 •

edited

Loading

sffc commented Jun 13, 2026 •

edited

Loading

robertbastian left a comment •

edited

Loading