ext/mbstring: Do some clean-up refactoring #12716

Girgias · 2023-11-18T04:15:41Z

To rebase after #12714 for the _php_mb_ini_mbstring_http_output_set() to make sense.

nielsdos

Looks OK.
I made a remark in a previous PR that there could be a subtly behaviour change because of strings containing \0, and I think those should be blocked to make sure we can always use ZSTR_LEN.
Leaving the final decision to Alex however as it's his extension.

nielsdos · 2023-11-19T16:14:27Z

ext/mbstring/mbstring.c

@@ -2537,7 +2537,7 @@ static zend_string* mb_trim_string(zend_string *input, zend_string *marker, cons
 		if (out_len <= to_skip) {
 			to_skip -= out_len;
 		} else {
-			for (int i = to_skip; i < out_len; i++) {
+			for (unsigned int i = to_skip; i < out_len; i++) {


Strictly speaking, there is an inconsistency here with how you did the previous changes. Previously you changed this to size_t because out_len was size_t. Here you changed it to unsigned int.
unsigned int, or even int, suffices in all cases because the length is limited to 128 anyway. Performance-wise it's hard to predict what these would do. I would expect no difference because it would just use a larger register , albeit with a larger stack spill.

Strictly speaking, there is an inconsistency here with how you did the previous changes. Previously you changed this to size_t because out_len was size_t. Here you changed it to unsigned int. unsigned int, or even int, suffices in all cases because the length is limited to 128 anyway. Performance-wise it's hard to predict what these would do. I would expect no difference because it would just use a larger register , albeit with a larger stack spill.

Right, I did kinda blindly do those changes. But if those are limited to 128, might as well use an unsigned char then?

Okay I remember how I got to this inconsistency, I used the type of the from parameter which is unsigned int here. But I wonder if this might not cause issues and that should be of type size_t in the first place?

The from variable should probably be of type size_t, otherwise >4GiB strings (okay unlikely, but still) won't work correctly I think.

ext/mbstring/mbstring.c

alexdowad · 2023-11-19T17:40:40Z

This looks good to me, but I need to investigate a bit more to fully understand the issue with embedded NUL bytes mentioned by @nielsdos.

Girgias · 2023-11-20T01:29:20Z

This looks good to me, but I need to investigate a bit more to fully understand the issue with embedded NUL bytes mentioned by @nielsdos.

Basically it boils down to a string "utf-8\0blah" would be interpreted as "utf-8" as the null byte terminates a C string.

alexdowad · 2023-11-20T09:00:12Z

Basically it boils down to a string "utf-8\0blah" would be interpreted as "utf-8" as the null byte terminates a C string.

Sorry for my mental slowness, but does this mean that "utf-8\0blah" would be interpreted as "utf-8" before, or now?

Girgias · 2023-11-20T13:01:47Z

Basically it boils down to a string "utf-8\0blah" would be interpreted as "utf-8" as the null byte terminates a C string.

Sorry for my mental slowness, but does this mean that "utf-8\0blah" would be interpreted as "utf-8" before, or now?

Before :) as the lengths wasn't passed to the function. Now with the patch it is getting interpreted as "utf-8\0blah" as it will compare the 10 bytes.

alexdowad · 2023-11-20T14:44:12Z

I suggest a note can be added in UPGRADING about this. Personally I think the BC break is not so big.

alexdowad · 2023-11-20T15:17:06Z

I suggest a note can be added in UPGRADING about this. Personally I think the BC break is not so big.

Ah, we should also add a test case for this.

nielsdos · 2023-11-20T16:51:54Z

It's actually quite subtle.
For example for name2encoding.
The function uses hashing and strncasecmp. If we pass ZSTR_LEN to the hashing function then the \0 byte will also be hashed.

Before this PR "utf8\0foo" would be interpreted as "utf8" because it internally used strlen. Then it uses strncasecmp (stops at \0) to compare the string. However now the length and string matching don't agree anymore, which causes the name to never match.
The mime types and aliases all use strncasecmp without hashing so those will match "utf8\0foo" as "utf8".
I would block NUL bytes at the parameter parsing side and avoid these subtle details.

ext/mbstring/mbstring.c

alexdowad · 2023-12-03T20:01:30Z

@Girgias How would it be if you merge some of the non-controversial commits in this patch series?

Girgias · 2023-12-04T15:55:29Z

@Girgias How would it be if you merge some of the non-controversial commits in this patch series?

This was my plan, I got distracted with other stuff :/

alexdowad · 2023-12-06T11:58:35Z

I think 39960e25a6fe0756c4ea5449568210b3f44eedca looks good to land, as long as the tests pass.

alexdowad · 2023-12-06T11:59:53Z

48bff2b625ee2d5ba06bfd53b8f3bfe54bdcc5f6 also looks good.

Girgias · 2023-12-06T23:48:41Z

I have merged the two commits @alexdowad

ext/mbstring/mbstring.c

alexdowad · 2023-12-07T05:50:32Z

I think 8fb651d61764b37c7cb8bdeca6d6f4ad616c74af is good and can be merged. It's just unfortunate that the update of UPGRADING is in a different commit.

Girgias · 2023-12-07T14:55:59Z

I think 8fb651d61764b37c7cb8bdeca6d6f4ad616c74af is good and can be merged. It's just unfortunate that the update of UPGRADING is in a different commit.

When I merge I will update UPGRADING to be part of the commit :)

alexdowad · 2023-12-07T18:47:51Z

I think 0048a3623469eb51b9ade358678c7dedc15bfd3d is good now.

Hopefully the unit tests we have are robust enough to catch any bugs. I added a number of tests for mb_strimwidth, though it is always nice to have more...

alexdowad · 2023-12-09T07:18:20Z

I think 1e94d7941685295a1a1815c3a452ea84b6db50b5 is also good to merge.

…set()

We have access to this information, so propagate it instead of calling strlen(). This also removes the newly introduced _ex() variant.

…ytes

alexdowad · 2024-04-23T23:27:06Z

@Girgias Thanks for working on these refactorings. I wonder how much it is worth to fix up mb_http_output... are we not planning to remove that in a future PHP release? But anyways, if you feel it is worth it, no objection. Again, thanks very much.

Girgias · 2024-04-24T14:39:43Z

@Girgias Thanks for working on these refactorings. I wonder how much it is worth to fix up mb_http_output... are we not planning to remove that in a future PHP release? But anyways, if you feel it is worth it, no objection. Again, thanks very much.

Is it deprecated? Or is that only the INI setting?

Anyway, I was just trying to finish one of my outstanding PRs :)

github-actions bot added the Extension: mbstring label Nov 18, 2023

Girgias force-pushed the mbstring-strlen-pass branch from a6f2619 to 4453b09 Compare November 19, 2023 07:14

Girgias marked this pull request as ready for review November 19, 2023 07:14

Girgias requested a review from alexdowad as a code owner November 19, 2023 07:14

Girgias requested a review from nielsdos November 19, 2023 07:14

nielsdos reviewed Nov 19, 2023

View reviewed changes

alexdowad reviewed Nov 19, 2023

View reviewed changes

ext/mbstring/mbstring.c Outdated Show resolved Hide resolved

Girgias commented Nov 22, 2023

View reviewed changes

ext/mbstring/mbstring.c Outdated Show resolved Hide resolved

Girgias force-pushed the mbstring-strlen-pass branch from 4453b09 to 36c90c3 Compare November 22, 2023 05:26

Girgias force-pushed the mbstring-strlen-pass branch from 36c90c3 to 6b88b01 Compare December 5, 2023 05:21

Girgias force-pushed the mbstring-strlen-pass branch from 6b88b01 to c995fc9 Compare December 6, 2023 23:48

alexdowad reviewed Dec 7, 2023

View reviewed changes

ext/mbstring/mbstring.c Outdated Show resolved Hide resolved

Girgias force-pushed the mbstring-strlen-pass branch from c995fc9 to 871b151 Compare December 7, 2023 17:28

Girgias force-pushed the mbstring-strlen-pass branch from 871b151 to ba4a66a Compare December 18, 2023 00:32

Girgias added 4 commits April 23, 2024 23:43

ext/mbstring: Pass string length to _php_mb_ini_mbstring_http_output_…

e1385b0

…set()

ext/mbstring: Always pass length to php_mb_get_encoding_or_pass()

1782745

We have access to this information, so propagate it instead of calling strlen(). This also removes the newly introduced _ex() variant.

ext/mbstring: Check encoding passed to mb_http_output() has no null b…

261198d

…ytes

UPGRADING

1016b00

Girgias force-pushed the mbstring-strlen-pass branch from ba4a66a to 1016b00 Compare April 23, 2024 22:48

Girgias requested a review from alexdowad April 23, 2024 22:48

Girgias merged commit f68d725 into php:master Apr 24, 2024

Girgias deleted the mbstring-strlen-pass branch April 24, 2024 14:39

ext/mbstring: Do some clean-up refactoring #12716

ext/mbstring: Do some clean-up refactoring #12716

Uh oh!

Conversation

Girgias commented Nov 18, 2023

Uh oh!

nielsdos left a comment

Choose a reason for hiding this comment

Uh oh!

nielsdos Nov 19, 2023

Choose a reason for hiding this comment

Uh oh!

Girgias Nov 20, 2023

Choose a reason for hiding this comment

Uh oh!

Girgias Nov 22, 2023

Choose a reason for hiding this comment

Uh oh!

nielsdos Nov 22, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alexdowad commented Nov 19, 2023

Uh oh!

Girgias commented Nov 20, 2023

Uh oh!

alexdowad commented Nov 20, 2023

Uh oh!

Girgias commented Nov 20, 2023

Uh oh!

alexdowad commented Nov 20, 2023

Uh oh!

alexdowad commented Nov 20, 2023

Uh oh!

nielsdos commented Nov 20, 2023

Uh oh!

Uh oh!

alexdowad commented Dec 3, 2023

Uh oh!

Girgias commented Dec 4, 2023

Uh oh!

alexdowad commented Dec 6, 2023

Uh oh!

alexdowad commented Dec 6, 2023

Uh oh!

Girgias commented Dec 6, 2023

Uh oh!

Uh oh!

alexdowad commented Dec 7, 2023

Uh oh!

Girgias commented Dec 7, 2023

Uh oh!

alexdowad commented Dec 7, 2023

Uh oh!

alexdowad commented Dec 9, 2023

Uh oh!

alexdowad commented Apr 23, 2024

Uh oh!

Girgias commented Apr 24, 2024

Uh oh!

Uh oh!