Skip to content

Normalize substr() behavior #6182

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

Normalize substr() behavior #6182

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c435fff
Normalize strpos() behavior
nikic Sep 22, 2020
b8f0a5a
Align grapheme_substr() behavior with substr()
nikic Sep 23, 2020
ec91fa3
Normalize iconv_substr() behavior as well
nikic Sep 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 8 additions & 13 deletions ext/iconv/iconv.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -638,27 +638,22 @@ static php_iconv_err_t _php_iconv_substr(smart_str *pretval,
return err;
}

if (len < 0) {
if ((len += (total_len - offset)) < 0) {
return PHP_ICONV_ERR_SUCCESS;
}
}

if (offset < 0) {
if ((offset += total_len) < 0) {
return PHP_ICONV_ERR_SUCCESS;
offset = 0;
}
} else if ((size_t)offset > total_len) {
offset = total_len;
}

if((size_t)len > total_len) {
if (len < 0) {
if ((len += (total_len - offset)) < 0) {
len = 0;
}
} else if ((size_t)len > total_len) {
len = total_len;
}


if ((size_t)offset > total_len) {
return PHP_ICONV_ERR_SUCCESS;
}

if ((size_t)(offset + len) > total_len ) {
/* trying to compute the length */
len = total_len - offset;
Expand Down
12 changes: 6 additions & 6 deletions ext/iconv/tests/iconv_substr.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,8 @@ var_dump(iconv("ISO-2022-JP", "EUC-JP", iconv_substr(iconv("EUC-JP", "ISO-2022-J
666768696a6b6c
a6a4a8a4aaa4ab
a4aba4ada4afa4b1a4b3a4b5a4b7
bool(false)
bool(false)
string(0) ""
string(0) ""
string(14) "This is a test"
string(14) "This is a test"
string(3) "est"
Expand All @@ -55,8 +55,8 @@ string(3) "est"
string(3) "est"
string(5) "This "
string(5) "This "
bool(false)
bool(false)
bool(false)
bool(false)
string(0) ""
string(0) ""
string(0) ""
string(0) ""
string(10) "���� ISO-2"
48 changes: 48 additions & 0 deletions ext/iconv/tests/iconv_substr_out_of_bounds.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
--TEST--
iconv_substr() with out of bounds offset
--SKIPIF--
<?php extension_loaded('iconv') or die('skip iconv extension is not available'); ?>
--FILE--
<?php

var_dump(iconv_substr("foo", 3));
var_dump(iconv_substr("foo", -3));
var_dump(iconv_substr("foo", 4));
var_dump(iconv_substr("foo", -4));
var_dump(iconv_substr("äöü", 3));
var_dump(iconv_substr("äöü", -3));
var_dump(iconv_substr("äöü", 4));
var_dump(iconv_substr("äöü", -4));
var_dump(iconv_substr("foo", 0, 3));
var_dump(iconv_substr("foo", 0, -3));
var_dump(iconv_substr("foo", 0, 4));
var_dump(iconv_substr("foo", 0, -4));
var_dump(iconv_substr("äöü", 0, 3));
var_dump(iconv_substr("äöü", 0, -3));
var_dump(iconv_substr("äöü", 0, 4));
var_dump(iconv_substr("äöü", 0, -4));
var_dump(iconv_substr("äöü", -4, 1));
var_dump(iconv_substr("äöü", -4, -1));
var_dump(iconv_substr("äöü", 2, -2));

?>
--EXPECT--
string(0) ""
string(3) "foo"
string(0) ""
string(3) "foo"
string(0) ""
string(6) "äöü"
string(0) ""
string(6) "äöü"
string(3) "foo"
string(0) ""
string(3) "foo"
string(0) ""
string(6) "äöü"
string(0) ""
string(6) "äöü"
string(0) ""
string(2) "ä"
string(4) "äö"
string(0) ""
43 changes: 20 additions & 23 deletions ext/intl/grapheme/grapheme_string.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,22 +371,20 @@ PHP_FUNCTION(grapheme_substr)
RETURN_THROWS();
}

if ( OUTSIDE_STRING(lstart, str_len)) {
zend_argument_value_error(2, "must be contained in argument #1 ($string)");
if (lstart < INT32_MIN || lstart > INT32_MAX) {
zend_argument_value_error(2, "is too large");
RETURN_THROWS();
}

/* we checked that it will fit: */
start = (int32_t) lstart;

if(no_length) {
if (no_length) {
length = str_len;
}

if(length < INT32_MIN) {
length = INT32_MIN;
} else if(length > INT32_MAX) {
length = INT32_MAX;
if (length < INT32_MIN || length > INT32_MAX) {
zend_argument_value_error(3, "is too large");
RETURN_THROWS();
}

/* the offset is 'grapheme count offset' so it still might be invalid - we'll check it later */
Expand Down Expand Up @@ -451,15 +449,17 @@ PHP_FUNCTION(grapheme_substr)
start += iter_val;
}

if ( 0 != start || sub_str_start_pos >= ustr_len ) {

intl_error_set( NULL, U_ILLEGAL_ARGUMENT_ERROR, "grapheme_substr: start not contained in string", 1 );

if (ustr) {
efree(ustr);
if (0 != start) {
if (start > 0) {
if (ustr) {
efree(ustr);
}
ubrk_close(bi);
RETURN_EMPTY_STRING();
}
ubrk_close(bi);
RETURN_FALSE;

sub_str_start_pos = 0;
ubrk_first(bi);
}

/* OK to convert here since if str_len were big, convert above would fail */
Expand Down Expand Up @@ -526,20 +526,17 @@ PHP_FUNCTION(grapheme_substr)
ubrk_close(bi);

if ( UBRK_DONE == sub_str_end_pos) {
if(length < 0) {
zend_argument_value_error(3, "must be contained in argument #1 ($string)");
if (length < 0) {
efree(ustr);
RETURN_THROWS();
RETURN_EMPTY_STRING();
} else {
sub_str_end_pos = ustr_len;
}
}

if(sub_str_start_pos > sub_str_end_pos) {
intl_error_set( NULL, U_ILLEGAL_ARGUMENT_ERROR, "grapheme_substr: length is beyond start", 1 );

if (sub_str_start_pos > sub_str_end_pos) {
efree(ustr);
RETURN_FALSE;
RETURN_EMPTY_STRING();
}

status = U_ZERO_ERROR;
Expand Down
33 changes: 6 additions & 27 deletions ext/intl/grapheme/grapheme_util.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,20 +57,6 @@ void grapheme_substr_ascii(char *str, size_t str_len, int32_t f, int32_t l, char
return;
}

if ((l < 0 && -l > str_len2)) {
return;
} else if (l > 0 && l > str_len2) {
l = str_len2;
}

if (f > str_len2 || (f < 0 && -f > str_len2)) {
return;
}

if (l < 0 && str_len2 < f - l) {
return;
}

/* if "from" position is negative, count start position from the end
* of the string
*/
Expand All @@ -79,8 +65,9 @@ void grapheme_substr_ascii(char *str, size_t str_len, int32_t f, int32_t l, char
if (f < 0) {
f = 0;
}
}

} else if (f > str_len2) {
f = str_len2;
}

/* if "length" position is negative, set it to the length
* needed to stop that many chars from the end of the string
Expand All @@ -90,20 +77,12 @@ void grapheme_substr_ascii(char *str, size_t str_len, int32_t f, int32_t l, char
if (l < 0) {
l = 0;
}
}

if (f >= str_len2) {
return;
}

if ((f + l) > str_len2) {
l = str_len - f;
}
} else if (l > str_len2 - f) {
l = str_len2 - f;
}

*sub_str = str + f;
*sub_str_len = l;

return;
}
/* }}} */

Expand Down
10 changes: 5 additions & 5 deletions ext/intl/tests/bug62759.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,11 @@ var_dump(intl_get_error_message());
var_dump(grapheme_substr('déjà', -1, 0));
?>
--EXPECT--
bool(false)
string(0) ""
bool(false)
string(61) "grapheme_substr: invalid parameters: U_ILLEGAL_ARGUMENT_ERROR"
string(0) ""
bool(false)
string(65) "grapheme_substr: length is beyond start: U_ILLEGAL_ARGUMENT_ERROR"
string(0) ""
string(12) "U_ZERO_ERROR"
string(0) ""
string(0) ""
string(12) "U_ZERO_ERROR"
string(0) ""
Loading