Skip to content

Promote some warnings in MBString Regex #5341

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 7 commits into from
Closed

Promote some warnings in MBString Regex #5341

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
fafc86c
Promote some warnings in MBString Regex
Girgias Apr 2, 2020
323cde7
Reviews
Girgias Aug 6, 2020
547be3f
Reviews
Girgias Sep 8, 2020
c9e3879
Drop Todo about invalid regex
Girgias Sep 8, 2020
31a471d
Use RETVAL_FALSE again instead of RETURN_FALSE
Girgias Sep 8, 2020
c0cb689
Return False on invalid encoded string
Girgias Sep 8, 2020
fdac9bd
Drop option_arg_num which now unused
Girgias Sep 8, 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
75 changes: 39 additions & 36 deletions ext/mbstring/php_mbregex.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -591,8 +591,8 @@ static size_t _php_mb_regex_get_option_string(char *str, size_t len, OnigOptionT
/* }}} */

/* {{{ _php_mb_regex_init_options */
static void
_php_mb_regex_init_options(const char *parg, size_t narg, OnigOptionType *option, OnigSyntaxType **syntax, int *eval)
static bool _php_mb_regex_init_options(const char *parg, size_t narg, OnigOptionType *option,
OnigSyntaxType **syntax)
{
size_t n;
char c;
Expand Down Expand Up @@ -651,14 +651,16 @@ _php_mb_regex_init_options(const char *parg, size_t narg, OnigOptionType *option
*syntax = ONIG_SYNTAX_POSIX_EXTENDED;
break;
case 'e':
if (eval != NULL) *eval = 1;
break;
zend_value_error("Option \"e\" is not supported");
Copy link
Member

Choose a reason for hiding this comment

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

You can remove this as a separate case (handle in default). Or alternatively, use a different error message like "no longer supported".

return false;
default:
break;
zend_value_error("Option \"%c\" is not supported", c);
return false;
}
}
if (option != NULL) *option|=optm;
}
return true;
}
/* }}} */

Expand Down Expand Up @@ -900,6 +902,11 @@ static void _php_mb_regex_ereg_exec(INTERNAL_FUNCTION_PARAMETERS, int icase)
RETURN_THROWS();
}

if (arg_pattern_len == 0) {
zend_argument_value_error(1, "must not be empty");
RETURN_THROWS();
}

if (array != NULL) {
array = zend_try_array_init(array);
if (!array) {
Expand All @@ -920,12 +927,6 @@ static void _php_mb_regex_ereg_exec(INTERNAL_FUNCTION_PARAMETERS, int icase)
options |= ONIG_OPTION_IGNORECASE;
}

if (arg_pattern_len == 0) {
php_error_docref(NULL, E_WARNING, "Empty pattern");
RETVAL_FALSE;
goto out;
}

re = php_mbregex_compile_pattern(arg_pattern, arg_pattern_len, options, MBREX(regex_default_syntax));
if (re == NULL) {
RETVAL_FALSE;
Expand Down Expand Up @@ -1007,15 +1008,14 @@ static void _php_mb_regex_ereg_replace_exec(INTERNAL_FUNCTION_PARAMETERS, OnigOp
smart_str out_buf = {0};
smart_str eval_buf = {0};
smart_str *pbuf;
int err, eval, n;
int err, n;
OnigUChar *pos;
OnigUChar *string_lim;
char *description = NULL;

const mbfl_encoding *enc = php_mb_regex_get_mbctype_encoding();
ZEND_ASSERT(enc != NULL);

eval = 0;
{
char *option_str = NULL;
size_t option_str_len = 0;
Expand Down Expand Up @@ -1043,20 +1043,15 @@ static void _php_mb_regex_ereg_replace_exec(INTERNAL_FUNCTION_PARAMETERS, OnigOp
}

if (option_str != NULL) {
_php_mb_regex_init_options(option_str, option_str_len, &options, &syntax, &eval);
/* Initialize option and in case of failure it means there is a value error */
if (!_php_mb_regex_init_options(option_str, option_str_len, &options, &syntax)) {
RETURN_THROWS();
}
} else {
options |= MBREX(regex_default_options);
syntax = MBREX(regex_default_syntax);
}
}
if (eval) {
if (is_callable) {
php_error_docref(NULL, E_WARNING, "Option 'e' cannot be used with replacement callback");
} else {
php_error_docref(NULL, E_WARNING, "The 'e' option is no longer supported, use mb_ereg_replace_callback instead");
}
RETURN_FALSE;
}

/* create regex pattern buffer */
re = php_mbregex_compile_pattern(arg_pattern, arg_pattern_len, options, syntax);
Expand Down Expand Up @@ -1122,7 +1117,9 @@ static void _php_mb_regex_ereg_replace_exec(INTERNAL_FUNCTION_PARAMETERS, OnigOp
zval_ptr_dtor(&retval);
} else {
if (!EG(exception)) {
php_error_docref(NULL, E_WARNING, "Unable to call custom replacement function");
zend_throw_error(NULL, "Unable to call custom replacement function");
zval_ptr_dtor(&subpats);
RETURN_THROWS();
}
}
zval_ptr_dtor(&subpats);
Expand Down Expand Up @@ -1251,6 +1248,7 @@ PHP_FUNCTION(mb_split)
onig_region_free(regs, 1);

/* see if we encountered an error */
// ToDo investigate if this can actually/should happen ...
if (err <= -2) {
OnigUChar err_str[ONIG_MAX_ERROR_MESSAGE_LEN];
onig_error_code_to_str(err_str, err);
Expand Down Expand Up @@ -1295,7 +1293,9 @@ PHP_FUNCTION(mb_ereg_match)
}

if (option_str != NULL) {
_php_mb_regex_init_options(option_str, option_str_len, &option, &syntax, NULL);
if(!_php_mb_regex_init_options(option_str, option_str_len, &option, &syntax)) {
RETURN_THROWS();
}
} else {
option |= MBREX(regex_default_options);
syntax = MBREX(regex_default_syntax);
Expand Down Expand Up @@ -1348,7 +1348,7 @@ static void _php_mb_regex_ereg_search_exec(INTERNAL_FUNCTION_PARAMETERS, int mod
}

if (arg_options) {
_php_mb_regex_init_options(arg_options, arg_options_len, &option, &syntax, NULL);
_php_mb_regex_init_options(arg_options, arg_options_len, &option, &syntax);
} else {
option |= MBREX(regex_default_options);
syntax = MBREX(regex_default_syntax);
Expand All @@ -1375,13 +1375,13 @@ static void _php_mb_regex_ereg_search_exec(INTERNAL_FUNCTION_PARAMETERS, int mod
}

if (MBREX(search_re) == NULL) {
php_error_docref(NULL, E_WARNING, "No regex given");
RETURN_FALSE;
zend_throw_error(NULL, "No pattern was provided");
RETURN_THROWS();
}

if (str == NULL) {
php_error_docref(NULL, E_WARNING, "No string given");
RETURN_FALSE;
zend_throw_error(NULL, "No string was provided");
RETURN_THROWS();
}

MBREX(search_regs) = onig_region_new();
Expand Down Expand Up @@ -1480,13 +1480,13 @@ PHP_FUNCTION(mb_ereg_search_init)
}

if (arg_pattern && arg_pattern_len == 0) {
php_error_docref(NULL, E_WARNING, "Empty pattern");
RETURN_FALSE;
zend_argument_value_error(2, "must not be empty");
RETURN_THROWS();
}

if (arg_options) {
option = 0;
_php_mb_regex_init_options(arg_options, arg_options_len, &option, &syntax, NULL);
_php_mb_regex_init_options(arg_options, arg_options_len, &option, &syntax);
} else {
option = MBREX(regex_default_options);
syntax = MBREX(regex_default_syntax);
Expand Down Expand Up @@ -1557,6 +1557,7 @@ PHP_FUNCTION(mb_ereg_search_getregs)
onig_foreach_name(MBREX(search_re), mb_regex_groups_iter, &args);
}
} else {
// TODO This seems to be some logical error, promote to Error
RETVAL_FALSE;
}
}
Expand Down Expand Up @@ -1588,12 +1589,12 @@ PHP_FUNCTION(mb_ereg_search_setpos)
}

if (position < 0 || (!Z_ISUNDEF(MBREX(search_str)) && Z_TYPE(MBREX(search_str)) == IS_STRING && (size_t)position > Z_STRLEN(MBREX(search_str)))) {
php_error_docref(NULL, E_WARNING, "Position is out of range");
MBREX(search_pos) = 0;
RETURN_FALSE;
zend_argument_value_error(1, "is out of range");
RETURN_THROWS();
}

MBREX(search_pos) = position;
// TODO Return void
RETURN_TRUE;
}
/* }}} */
Expand Down Expand Up @@ -1628,7 +1629,9 @@ PHP_FUNCTION(mb_regex_set_options)
if (string != NULL) {
opt = 0;
syntax = NULL;
_php_mb_regex_init_options(string, string_len, &opt, &syntax, NULL);
if(!_php_mb_regex_init_options(string, string_len, &opt, &syntax)) {
RETURN_THROWS();
}
_php_mb_regex_set_options(opt, syntax, &prev_opt, &prev_syntax);
opt = prev_opt;
syntax = prev_syntax;
Expand Down
104 changes: 37 additions & 67 deletions ext/mbstring/tests/bug43994.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,106 +25,76 @@ foreach($inputs as $input) {
}
echo "\n-- Iteration $iterator --\n";
echo "Without \$regs arg:\n";
var_dump( mb_ereg($input, 'hello, world') );
try {
var_dump( mb_ereg($input, 'hello, world') );
} catch (\ValueError $e) {
echo $e->getMessage() . \PHP_EOL;
}

echo "With \$regs arg:\n";
var_dump(mb_ereg($input, 'hello, world', $mb_regs));
try {
var_dump(mb_ereg($input, 'hello, world', $mb_regs));
} catch (\ValueError $e) {
echo $e->getMessage() . \PHP_EOL;
}

var_dump($mb_regs);
$iterator++;
};
?>
--EXPECTF--
--EXPECT--
-- Iteration 1 --
Without $regs arg:

Warning: mb_ereg(): Empty pattern in %s on line %d
bool(false)
mb_ereg(): Argument #1 ($pattern) must not be empty
With $regs arg:

Warning: mb_ereg(): Empty pattern in %s on line %d
bool(false)
array(0) {
}
mb_ereg(): Argument #1 ($pattern) must not be empty
NULL

-- Iteration 2 --
Without $regs arg:

Warning: mb_ereg(): Empty pattern in %s on line %d
bool(false)
mb_ereg(): Argument #1 ($pattern) must not be empty
With $regs arg:

Warning: mb_ereg(): Empty pattern in %s on line %d
bool(false)
array(0) {
}
mb_ereg(): Argument #1 ($pattern) must not be empty
NULL

-- Iteration 3 --
Without $regs arg:

Warning: mb_ereg(): Empty pattern in %s on line %d
bool(false)
mb_ereg(): Argument #1 ($pattern) must not be empty
With $regs arg:

Warning: mb_ereg(): Empty pattern in %s on line %d
bool(false)
array(0) {
}
mb_ereg(): Argument #1 ($pattern) must not be empty
NULL

-- Iteration 4 --
Without $regs arg:

Warning: mb_ereg(): Empty pattern in %s on line %d
bool(false)
mb_ereg(): Argument #1 ($pattern) must not be empty
With $regs arg:

Warning: mb_ereg(): Empty pattern in %s on line %d
bool(false)
array(0) {
}
mb_ereg(): Argument #1 ($pattern) must not be empty
NULL

-- Iteration 5 --
Without $regs arg:

Warning: mb_ereg(): Empty pattern in %s on line %d
bool(false)
mb_ereg(): Argument #1 ($pattern) must not be empty
With $regs arg:

Warning: mb_ereg(): Empty pattern in %s on line %d
bool(false)
array(0) {
}
mb_ereg(): Argument #1 ($pattern) must not be empty
NULL

-- Iteration 6 --
Without $regs arg:

Warning: mb_ereg(): Empty pattern in %s on line %d
bool(false)
mb_ereg(): Argument #1 ($pattern) must not be empty
With $regs arg:

Warning: mb_ereg(): Empty pattern in %s on line %d
bool(false)
array(0) {
}
mb_ereg(): Argument #1 ($pattern) must not be empty
NULL

-- Iteration 7 --
Without $regs arg:

Warning: mb_ereg(): Empty pattern in %s on line %d
bool(false)
mb_ereg(): Argument #1 ($pattern) must not be empty
With $regs arg:

Warning: mb_ereg(): Empty pattern in %s on line %d
bool(false)
array(0) {
}
mb_ereg(): Argument #1 ($pattern) must not be empty
NULL

-- Iteration 8 --
Without $regs arg:

Warning: mb_ereg(): Empty pattern in %s on line %d
bool(false)
mb_ereg(): Argument #1 ($pattern) must not be empty
With $regs arg:

Warning: mb_ereg(): Empty pattern in %s on line %d
bool(false)
array(0) {
}
mb_ereg(): Argument #1 ($pattern) must not be empty
NULL
15 changes: 8 additions & 7 deletions ext/mbstring/tests/bug69151.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,14 @@ if (!function_exists('mb_ereg')) die('skip mbregex support not available');
--FILE--
<?php
$str = "\x80";
var_dump(
false === mb_eregi('.', $str, $matches),
[] === $matches,
NULL === mb_ereg_replace('.', "\\0", $str),
false === mb_ereg_search_init("\x80", '.'),
false === mb_ereg_search()
);

var_dump(false === mb_eregi('.', $str, $matches));
var_dump([] === $matches);

var_dump(NULL === mb_ereg_replace('.', "\\0", $str));

var_dump(false === mb_ereg_search_init("\x80", '.'));
var_dump(false === mb_ereg_search());
?>
--EXPECT--
bool(true)
Expand Down
14 changes: 9 additions & 5 deletions ext/mbstring/tests/bug72164.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,13 @@ if (!function_exists('mb_ereg')) die('skip mbregex support not available');
$var0 = "e";
$var2 = "";
$var3 = NULL;
$var8 = mb_ereg_replace($var2,$var3,$var3,$var0);
var_dump($var8);
try {
$var8 = mb_ereg_replace($var2,$var3,$var3,$var0);
var_dump($var8);
} catch (\ValueError $e) {
echo $e->getMessage() . \PHP_EOL;
}

?>
--EXPECTF--
Warning: mb_ereg_replace(): The 'e' option is no longer supported, use mb_ereg_replace_callback instead in %s on line %d
bool(false)
--EXPECT--
Option "e" is not supported
Loading