Skip to content

ICU-21410 fix int32_t overflow in listFormat#1479

Merged
FrankYFTang merged 1 commit intounicode-org:masterfrom
FrankYFTang:ICU-21410-crash-ListFormat
Nov 24, 2020
Merged

ICU-21410 fix int32_t overflow in listFormat#1479
FrankYFTang merged 1 commit intounicode-org:masterfrom
FrankYFTang:ICU-21410-crash-ListFormat

Conversation

@FrankYFTang
Copy link
Copy Markdown
Contributor

@FrankYFTang FrankYFTang commented Nov 20, 2020

Checklist

if (U_FAILURE(status)) {
return count;
}
U_ASSERT(position >= 0);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you put all these U_ASSERTs inside prepareForInsert?

char16_t *oldChars = getCharPtr();
Field *oldFields = getFieldPtr();
if (fLength + count > oldCapacity) {
if ((fLength + count) > std::numeric_limits<int32_t>::max() / 2) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You can use INT32_MAX instead in ICU4C

@FrankYFTang
Copy link
Copy Markdown
Contributor Author

PTAL

@FrankYFTang FrankYFTang requested a review from sffc November 23, 2020 19:10
sffc
sffc previously approved these changes Nov 23, 2020
@sffc
Copy link
Copy Markdown
Member

sffc commented Nov 23, 2020

LGTM please squash

@jira-pull-request-webhook
Copy link
Copy Markdown

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

~ Your Friendly Jira-GitHub PR Checker Bot

@FrankYFTang FrankYFTang requested a review from sffc November 23, 2020 19:39
@FrankYFTang
Copy link
Copy Markdown
Contributor Author

squashed , please approve again

@FrankYFTang FrankYFTang merged commit 275d59a into unicode-org:master Nov 24, 2020
@FrankYFTang FrankYFTang deleted the ICU-21410-crash-ListFormat branch November 24, 2020 01:12
sffc pushed a commit that referenced this pull request Dec 2, 2020
sffc pushed a commit that referenced this pull request Dec 9, 2020
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