ICU-22517 Limit the closure expansion loop and return error #2629

FrankYFTang · 2023-09-26T04:21:36Z

To avoid very slow return from the constructor, we return an error while the Collation rule expand too big.
Add a limit constant to limit to the number of loop needed for 8 Hanguls.
Necessary number of loops: H(0)=0; H(i)=3H(i-1)+2.
Where i is the length of Hangul in the rule.
H(1) = 2, H(2) = 8, H(3)=26, H(4)=80, H(5) = 242 ...

Checklist

Required: Issue filed: https://unicode-org.atlassian.net/browse/ICU-22517
Required: The PR title must be prefixed with a JIRA Issue number.
Required: The PR description must include the link to the Jira Issue, for example by completing the URL in the first checklist item
Required: Each commit message must be prefixed with a JIRA Issue number.
Issue accepted (done by Technical Committee after discussion)
Tests included, if applicable
API docs and/or User Guide docs changed or added, if applicable

jira-pull-request-webhook · 2023-09-29T01:28:59Z

Notice: the branch changed across the force-push!

icu4c/source/i18n/collationbuilder.cpp is now changed in the branch
icu4c/source/test/intltest/regcoll.cpp is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

jira-pull-request-webhook · 2023-09-29T05:24:01Z

Notice: the branch changed across the force-push!

icu4c/source/i18n/collationbuilder.cpp is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

FrankYFTang · 2023-09-29T18:34:04Z

@markusicu I try to set the limit to smaller value, such as 242 (which allow 5 hanguls) , but that break collate/CollationTest/TestBuilderContextsOverflow so I have to change it to 6560 which could allow 8 hanguls on the righthand side and also passing collate/CollationTest/TestBuilderContextsOverflow

markusicu

Please remember to port the test and the fix to Java as well.

icu4c/source/i18n/collationbuilder.cpp

markusicu · 2023-09-29T20:40:16Z

icu4c/source/test/intltest/regcoll.cpp

+    IcuTestErrorCode errorCode(*this, "TestICU22517");
+    char16_t data[] = u"&a=b쫊쫊쫊쫊쫊쫊쫊쫊";
+    icu::UnicodeString rule(true, data, -1);
+    for (int i = 4; i <= rule.length(); i++) {


How long does this test run? Should we test only up to rule.length() - 1 or - 2 unless in exhaustive mode?

Almost a minute... is too long I think for normal testing.
Please test only up to rule.length() - 1 or - 2 unless in exhaustive mode.

icu4c/source/i18n/collationbuilder.cpp

markusicu · 2023-09-29T22:25:13Z

Please also fix the typo in the commit message and in the PR title: s/clouser/closure/

See unicode-org#2629

jira-pull-request-webhook · 2023-09-30T09:32:42Z

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

jira-pull-request-webhook · 2023-09-30T09:38:43Z

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

FrankYFTang · 2023-10-01T06:41:21Z

I have some problem to run Java test w/ mvn now (could be my disk full issue). I will try to change Java in a different PR

markusicu · 2023-10-02T18:48:46Z

Please also fix the typo in the commit message and in the PR title: s/clouser/closure/

The PR title is fixed but the commit message still has this typo.

markusicu · 2023-10-02T18:49:49Z

I have some problem to run Java test w/ mvn now (could be my disk full issue). I will try to change Java in a different PR

ok

jira-pull-request-webhook · 2023-10-02T21:24:43Z

Notice: the branch changed across the force-push!

icu4c/source/test/intltest/regcoll.cpp is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

FrankYFTang · 2023-10-02T21:25:03Z

Please also fix the typo in the commit message and in the PR title: s/clouser/closure/

The PR title is fixed but the commit message still has this typo.

done

jira-pull-request-webhook · 2023-10-03T01:09:11Z

Notice: the branch changed across the force-push!

icu4j/main/collate/src/main/java/com/ibm/icu/impl/coll/CollationBuilder.java is now changed in the branch
icu4j/main/collate/src/test/java/com/ibm/icu/dev/test/collator/CollationRegressionTest.java is now changed in the branch

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

FrankYFTang · 2023-10-03T01:09:33Z

I have some problem to run Java test w/ mvn now (could be my disk full issue). I will try to change Java in a different PR

I added them now.

To avoid very slow return from the constructor, we return error while the Collation rule expand too big. Add a soft limit to limit to the number of loop needed for 8 Hanguls Necessary number of loop: H(0)=0; H(i)=3H(i-1)+2. Where i is the length of Hangul in the rule. H(1) = 2, H(2) = 8, H(3)=26, H(4)=80, H(5) = 242 ...

jira-pull-request-webhook · 2023-10-03T01:11:39Z

Notice: the branch changed across the force-push!

icu4j/main/collate/src/main/java/com/ibm/icu/impl/coll/CollationBuilder.java is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

markusicu

lgtm tnx

markusicu added the incomplete Needs work; do not approve/merge as is. label Sep 28, 2023

FrankYFTang force-pushed the ICU-22517-ICU-22517 branch from a700f3b to 370e49a Compare September 29, 2023 01:28

FrankYFTang changed the title ~~ICU-22517 CollationBuilder::addRelation very slow with multiple Hangul~~ ICU-22517 Limit the clouser expansion loop and return error Sep 29, 2023

FrankYFTang removed the incomplete Needs work; do not approve/merge as is. label Sep 29, 2023

FrankYFTang requested a review from markusicu September 29, 2023 01:30

FrankYFTang force-pushed the ICU-22517-ICU-22517 branch from 370e49a to ccd2064 Compare September 29, 2023 05:23

markusicu reviewed Sep 29, 2023

View reviewed changes

FrankYFTang added a commit to FrankYFTang/icu that referenced this pull request Sep 30, 2023

ICU-22517 Limit the clouser expansion loop and return error

1c8f2c2

See unicode-org#2629

FrankYFTang force-pushed the ICU-22517-ICU-22517 branch from 1a6903e to 1c8f2c2 Compare September 30, 2023 09:32

FrankYFTang force-pushed the ICU-22517-ICU-22517 branch from 1c8f2c2 to d5d2946 Compare September 30, 2023 09:38

FrankYFTang changed the title ~~ICU-22517 Limit the clouser expansion loop and return error~~ ICU-22517 Limit the closure expansion loop and return error Oct 1, 2023

FrankYFTang closed this Oct 1, 2023

FrankYFTang reopened this Oct 1, 2023

FrankYFTang requested a review from markusicu October 2, 2023 17:36

FrankYFTang force-pushed the ICU-22517-ICU-22517 branch from d5d2946 to c0a3818 Compare October 2, 2023 21:24

FrankYFTang force-pushed the ICU-22517-ICU-22517 branch from c0a3818 to bc5f912 Compare October 3, 2023 01:09

FrankYFTang force-pushed the ICU-22517-ICU-22517 branch from bc5f912 to 9e29347 Compare October 3, 2023 01:11

markusicu approved these changes Oct 3, 2023

View reviewed changes

FrankYFTang merged commit 05b0e7a into unicode-org:main Oct 3, 2023

FrankYFTang deleted the ICU-22517-ICU-22517 branch October 3, 2023 02:06

ilonatommy mentioned this pull request Nov 3, 2025

Upgrade to ICU 72.1.0.4 dotnet/runtime#121202

Open

Uh oh!

ICU-22517 Limit the closure expansion loop and return error #2629

ICU-22517 Limit the closure expansion loop and return error #2629

Uh oh!

Conversation

FrankYFTang commented Sep 26, 2023 • edited by markusicu Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

jira-pull-request-webhook bot commented Sep 29, 2023

Uh oh!

jira-pull-request-webhook bot commented Sep 29, 2023

Uh oh!

FrankYFTang commented Sep 29, 2023

Uh oh!

markusicu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

markusicu Sep 29, 2023

Choose a reason for hiding this comment

Uh oh!

FrankYFTang Sep 29, 2023

Choose a reason for hiding this comment

Uh oh!

markusicu Oct 2, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

markusicu commented Sep 29, 2023

Uh oh!

jira-pull-request-webhook bot commented Sep 30, 2023

Uh oh!

jira-pull-request-webhook bot commented Sep 30, 2023

Uh oh!

FrankYFTang commented Oct 1, 2023

Uh oh!

markusicu commented Oct 2, 2023

Uh oh!

markusicu commented Oct 2, 2023

Uh oh!

jira-pull-request-webhook bot commented Oct 2, 2023

Uh oh!

FrankYFTang commented Oct 2, 2023

Uh oh!

jira-pull-request-webhook bot commented Oct 3, 2023

Uh oh!

FrankYFTang commented Oct 3, 2023

Uh oh!

jira-pull-request-webhook bot commented Oct 3, 2023

Uh oh!

markusicu left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

FrankYFTang commented Sep 26, 2023 •

edited by markusicu

Loading