ext/mbstring: Fix truncated encoding names resolving to full names

[kamil-tekiela]

Introduced by 3ad422ebd0b

@ndossche

Fixes #23958

[youkidearitai]

Thanks, nice catch!
LGTM.

[alexdowad]

I don't know if I would say that the problem was really caused by 3ad422ebd0b. That just moved the strlen call into the caller. Even before that commit, we were not checking that the provided name length matched the encoding name length.

[alexdowad]

...But otherwise, the fix is good. :)

@ndossche Do you think we should cache strlen((*encoding)->name) in the mbfl_encoding struct, rather than deriving it with strlen every time?

[iliaal]

php_mb_parse_encoding_list() has the same prefix match for "auto" (strncasecmp(p1, "auto", p1_length)), so mb_detect_order("aut") is accepted as auto. 8.3 rejected it.

[kamil-tekiela]

php_mb_parse_encoding_list() has the same prefix match for "auto" (strncasecmp(p1, "auto", p1_length)), so mb_detect_order("aut") is accepted as auto. 8.3 rejected it.

@alexdowad Maybe you can help me with this one. https://3v4l.org/0qeOF#v I think neither "a", "au", nor "aut" should match, but it looks like this is a fallback mechanism that converts empty list into an "auto". I don't actually know how this should be fixed. What do you recommend?

There is a comment in gh15824.phpt that makes me think this was an intentional change.

[alexdowad]

@kamil-tekiela Hmm. I just looked at gh15824.phpt, but I don't understand how it implies that "this was an intentional change".
There is a comment there which implies that accepting a trailing comma in a list of text encodings is intended. It doesn't seem to apply to the issue raised in this PR.

[ndossche]

@ndossche Do you think we should cache strlen((*encoding)->name) in the mbfl_encoding struct, rather than deriving it with strlen every time?

Perhaps, should be simple enough for the master branch.

@kamil-tekiela Hmm. I just looked at gh15824.phpt, but I don't understand how it implies that "this was an intentional change". There is a comment there which implies that accepting a trailing comma in a list of text encodings is intended. It doesn't seem to apply to the issue raised in this PR.

Same here, this is about a trailing comma.

[kamil-tekiela]

@kamil-tekiela Hmm. I just looked at gh15824.phpt, but I don't understand how it implies that "this was an intentional change". There is a comment there which implies that accepting a trailing comma in a list of text encodings is intended. It doesn't seem to apply to the issue raised in this PR.

Maybe I misunderstood the comment. So in your opinion what should be done about p1_length == 0 with auto?

[alexdowad]

Maybe I misunderstood the comment. So in your opinion what should be done about p1_length == 0 with auto?

I think it does make sense to validate p1_length == strlen("auto").

For both that and the main bug fix which @kamil-tekiela kindly included in this PR, Hyrum's law does apply. It will almost certainly break some code written by someone, somewhere. But we can't keep unintended changes forever because of that. If this behavior was introduced in PHP 8.4, it's an unintended change and I think it should be fixed.

[kamil-tekiela]

@alexdowad given your statement, I added another commit to fix the "auto" to pre-PHP 8.4. I think all of it is now back to PHP 8.3 behaviour https://3v4l.org/L5V1Y#v

I also removed the misleading trailing comma from the other test which was the easiest way to fix that test, but I still don't understand what that comment meant.

[alexdowad]

I also removed the misleading trailing comma from the other test which was the easiest way to fix that test, but I still don't understand what that comment meant.

It looks like @youkidearitai added that test. I think he probably felt that it would be good to make mb_detect_encoding tolerate a trailing comma (i.e. don't break on poorly formatted input). If we agree that is desirable, the best way to do it would be to detect a trailing empty string in the list and just filter it out, not by interpreting that empty string as matching some random text encoding.

[ndossche]

What exactly is this blocked on rn?

[kamil-tekiela]

I didn't want to merge it without someone deciding on the newly added commit.

[ndossche]

In a sense, you can't even fix segfaults because someone may use them as a quick way to end a request.