Fix mb_strimwidth RC info #9254
Conversation
val is an embedded array in zend_string. The value would decay to a pointer which can never be NULL.
|
Thanks very much!! |
| @@ -118,7 +118,6 @@ function mb_strcut(string $string, int $start, ?int $length = null, ?string $enc | |||
|
|
|||
| function mb_strwidth(string $string, ?string $encoding = null): int {} | |||
|
|
|||
| /** @refcount 1 */ | |||
There was a problem hiding this comment.
I guess this hint is used for some kind of optimizations by the JIT? Probably for cases where the returned string doesn't "escape" and can be immediately freed at the end of a block?
I just looked through the entire list and the annotations look accurate for the other functions. There are some other cases where potentially we could just return the input string unmodified, which might help to reduce the number of dynamic allocations. If I ever do that I will remember to adjust the annotations here.
There was a problem hiding this comment.
I do wonder why mb_scrub isn't annotated as @refcount 1. I believe that function should always return a newly allocated string with refcount 1. Or... is there something I'm missing?
There was a problem hiding this comment.
Good catch, thank you! This looks good, except maybe the change to PHP_INI_MH(OnUpdate_mbstring_http_input).
cc @alexdowad
| @@ -761,7 +761,7 @@ static PHP_INI_MH(OnUpdate_mbstring_http_input) | |||
| php_error_docref("ref.mbstring", E_DEPRECATED, "Use of mbstring.http_input is deprecated"); | |||
| } | |||
|
|
|||
| if (!new_value || !ZSTR_VAL(new_value)) { | |||
| if (!new_value) { | |||
There was a problem hiding this comment.
Did you change this deliberately? If so, the implementation of the function could be simplified; basically to an if-else, then swap the branches, then drop the else, since the then branch guards.
There was a problem hiding this comment.
Yeah, it produced a compiler warning (as the check is redundant) but I didn't want to change anything else.
There was a problem hiding this comment.
Is that check really redundant? What about an empty zend_string (or more precisely, a zend_string where the first byte is NUL)?
There was a problem hiding this comment.
@cmb69 Then it's still decayed to a pointer that points to a '\0'.
There was a problem hiding this comment.
Just to demonstrate: https://godbolt.org/z/6x9z7d8a9
There was a problem hiding this comment.
Sounds like adding a new regression test is in order.
There was a problem hiding this comment.
I assume this is a bug stemming from the time we changed from
char*tozend_string*
Doesn't look like it. It was added here: 379d9a1 But maybe it was just always wrong.
There was a problem hiding this comment.
Just tried checking what the manual says about mbstring.http_input: https://www.php.net/manual/en/mbstring.configuration.php#ini.mbstring.http-input
It doesn't document what is "supposed" to happen if mbstring.http_input is set to an empty string. I suggest we stick to whatever the historical behavior was.
There was a problem hiding this comment.
Yeah, might be best for now. After all, these deprecated INI settings were supposed to be removed long ago.
There was a problem hiding this comment.
I'll merge this without that commit for now.
php-src/ext/mbstring/mbstring.c
Line 2459 in bd3cd6a
This function can now return values with RC != 1.
Associated failure on nightly:
https://dev.azure.com/phpazuredevops/PHP/_build/results?buildId=27444&view=ms.vss-test-web.build-test-results-tab&runId=608358&resultId=107821&paneView=debug