Skip to content

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

Merged
merged 1 commit into from
May 2, 2025

Conversation

SakiTakamachi
Copy link
Member

grapheme_split_string returns -1 in case of an error.

@SakiTakamachi SakiTakamachi force-pushed the fix/grapheme_levenshtein branch from 98d8c09 to 6ea4225 Compare May 1, 2025 13:07
@SakiTakamachi SakiTakamachi marked this pull request as ready for review May 1, 2025 13:45
@SakiTakamachi SakiTakamachi requested a review from devnexen as a code owner May 1, 2025 13:45
Copy link
Contributor

@youkidearitai youkidearitai left a 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)) {
Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

@devnexen

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…

if (U_FAILURE(ustatus)) {

Copy link
Member

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.

Copy link
Member

@devnexen devnexen left a 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.

@SakiTakamachi SakiTakamachi merged commit f1d259a into php:master May 2, 2025
9 checks passed
@SakiTakamachi SakiTakamachi deleted the fix/grapheme_levenshtein branch May 2, 2025 06:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants