-
Notifications
You must be signed in to change notification settings - Fork 7.8k
ext/intl: Fix for the issue where strlen could potentially become negative #18477
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
ext/intl: Fix for the issue where strlen could potentially become negative #18477
Conversation
1d13808
to
98d8c09
Compare
98d8c09
to
6ea4225
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's right. Make sense.
@@ -994,6 +994,10 @@ PHP_FUNCTION(grapheme_levenshtein) | |||
int32_t strlen_1, strlen_2; | |||
strlen_1 = grapheme_split_string(ustring1, ustring1_len, NULL, 0); | |||
strlen_2 = grapheme_split_string(ustring2, ustring2_len, NULL, 0); | |||
if (UNEXPECTED(strlen_1 < 0 || strlen_2 < 0)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
before merging, and since that s the whole point of this PR. can you think of few test cases to trigger this code path ? Cheers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logically, it seems to return -1
when a bad status is received, just like in the following location.
However, I can’t think of a specific way to intentionally reproduce that behavior…
php-src/ext/intl/grapheme/grapheme_string.c
Line 1010 in dd57332
if (U_FAILURE(ustatus)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried something simpler, grapheme_strlen
except U_MEMORY_ALLOCATION_ERROR
I can t think of cases as bad inputs are caught up before that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change itself is correct and is unlikely to happen indeed. If no one else objects, feel free to merge.
grapheme_split_string
returns-1
in case of an error.