Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ICU-21041 Fix fuzzer memory read error. #1622

Merged

Conversation

FrankYFTang
Copy link
Contributor

Checklist
  • Required: Issue filed: https://unicode-org.atlassian.net/browse/ICU-21041
  • 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

@@ -261,6 +261,10 @@ DataBuilderCollationIterator::getCE32FromBuilderData(uint32_t ce32, UErrorCode &
return utrie2_get32(builder.trie, jamo);
} else {
ConditionalCE32 *cond = builder.getConditionalCE32ForCE32(ce32);
if (cond == nullptr) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. This whole function needs, as the very first thing, to check its incoming UErrorCode, and return immediately if failure.
  2. The fuzzer failure might disappear if this were done (but this would need to be verified)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added that check, as

 uint32_t
 DataBuilderCollationIterator::getCE32FromBuilderData(uint32_t ce32, UErrorCode &errorCode) {
+    if (U_FAILURE(errorCode)) {
+        return 0;
+    }
     U_ASSERT(Collation::hasCE32Tag(ce32, Collation::BUILDER_DATA_TAG));

but still have the problem

@FrankYFTang
Copy link
Contributor Author

Ping

@FrankYFTang
Copy link
Contributor Author

PTAL

Copy link
Member

@markusicu markusicu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm pse squash

@markusicu
Copy link
Member

By the way, the .txt file is empty. Is that on purpose, just as a marker for the fuzzer system?

@FrankYFTang
Copy link
Contributor Author

By the way, the .txt file is empty. Is that on purpose, just as a marker for the fuzzer system?

The test case file is a UTF16 encoded file. I think github just does not like the fact I use the .txt extension and the content is in UTF16. Change the file extension to .case now.

FrankYFTang added a commit to FrankYFTang/icu that referenced this pull request Mar 10, 2021
@jira-pull-request-webhook
Copy link

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

~ Your Friendly Jira-GitHub PR Checker Bot

@markusicu
Copy link
Member

The copyright scanner is failing for the new file, but of course you can't add a copyright there.

I propose that we use .fuzz as an extension of binary fuzzer case files, and then you can add a *.fuzz line into the "suffix matches" section of https://github.com/unicode-org/icu/blob/master/.cpyskip.txt

@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • .cpyskip.txt is now changed in the branch
  • icu4c/source/test/fuzzer/collator_rulebased_ICU-21041.case is no longer changed in the branch
  • icu4c/source/test/fuzzer/collator_rulebased_ICU-21041.fuzz is now changed in the branch

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@jira-pull-request-webhook
Copy link

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

~ Your Friendly Jira-GitHub PR Checker Bot

Copy link
Member

@markusicu markusicu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm tnx

@FrankYFTang FrankYFTang merged commit 63b9a8a into unicode-org:master Mar 10, 2021
@FrankYFTang FrankYFTang deleted the ICU-21041-collator-address branch March 10, 2021 23:26
daniel-ju added a commit to microsoft/icu that referenced this pull request Mar 17, 2021
This change cherry-picks an upstream fix for a null-dereference error.

Upstream ticket: https://unicode-org.atlassian.net/browse/ICU-21041
Upstream PR: unicode-org/icu#1622

Note: The upstream change modifies 3 files, but only 2 files are modified here as we don't include the .cpyskip.txt file in MS-ICU (used for syntax highlighting on GitHub).
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.

4 participants