mbstring: UTF-8 sequences with out-of-bounds follow bytes are no longer substituted correctly

[ausi]

Description

The following code:

<?php
mb_substitute_character(ord('?'));
var_dump(mb_convert_encoding("\xDF\xC0", 'UTF-8', 'UTF-8'));
var_dump(mb_convert_encoding("\xEF\xBF\xC0", 'UTF-8', 'UTF-8'));
var_dump(mb_convert_encoding("\xF4\x8F\xBF\xC0", 'UTF-8', 'UTF-8'));

Resulted in this output:

string(1) "?"
string(1) "?"
string(1) "?"

But I expected this output instead (as it is in PHP 7.x and 8.0):

string(2) "??"
string(2) "??"
string(2) "??"

See https://3v4l.org/Qt2HJ

As far as I know mb_convert_encoding(…, 'UTF-8', 'UTF-8') is commonly used to convert (potentially) non-conforming UTF-8 to valid UTF-8.

Before PHP 8.1.0 this worked as expected and behaved the same way as all major web browsers do. But since PHP 8.1.0 the behavior has changed and UTF-8 sequences with out-of-bounds follow bytes are no longer substituted with two substitute characters (usually represented as �) but with only one.

To my understanding this is defined in the encoding specification: https://encoding.spec.whatwg.org/#utf-8-decoder
The step 8.1.1.4.2 “Prepend byte to ioQueue.” seems to be missing in these cases.

PHP Version

PHP 8.1.5 (all 8.1.x are affected)

Operating System

No response

[cmb69]

Indeed, this looks wrong. UConverter agrees:

$converter = new UConverter("UTF-8", "UTF-8");
var_dump($converter->convert("\xDF\xC0"));
var_dump($converter->convert("\xEF\xBF\xC0"));
var_dump($converter->convert("\xF4\x8F\xBF\xC0"));

string(6) "��"
string(6) "��"
string(6) "��"

[nikic]

cc @alexdowad

[alexdowad]

Hi, @ausi. Thanks very much for this report.

I had to search a bit to find the cause of this change and then jog my memory to remember why I made it. (The change was in October 2020.)

The change was made in an attempt to make mbstring's behavior when handling UTF-8 consistent with its behavior on other text encodings. For most text encodings, mbstring emits a single error marker as soon as it sees an illegal byte, then resets itself back to its "starting state" and continues processing the next byte.

If we take \xDF\xC0 as an example, the byte \xDF is not illegal. But then \xC0 is. This is why I made it emit a single error marker.

However, I was not aware that there was any specific standard regarding how many errors should be signaled for a particular invalid UTF-8 string. (RFC 3629 just specifies how valid UTF-8 strings should be handled, not invalid ones.) In general, we want mbstring to comply as closely as possible with all relevant standards and to be compatible with as many legacy computing systems as possible.

Hence, I am very happy to revert this change. This time around, I would also like to 1) add some code comments explaining why error handling for UTF-8 is done the way it is, and 2) add another test to our test suite specifically to verify the error handling behavior for UTF-8.

To that end, I would like to ask if you have any other test cases which you can suggest should be added to the test suite. This will help to ensure that future maintenance and ongoing development of mbstring will not cause any further breakage.

[ausi]

If we take \xDF\xC0 as an example, the byte \xDF is not illegal. But then \xC0 is. This is why I made it emit a single error marker.

Which totally makes sense ☺️
It took me quite some time to see how the specification ends up with two error markers here 🙈

In general, we want mbstring to comply as closely as possible with all relevant standards and to be compatible with as many legacy computing systems as possible.

❤️

… I would like to ask if you have any other test cases which you can suggest should be added to the test suite.

These are the test cases I’ve written some time ago for an implementation of https://encoding.spec.whatwg.org/#utf-8-decoder
(every first string is the source, second is the target)

["\x80", "\u{FFFD}"],
["\xFF", "\u{FFFD}"],
["\xC2\x7F", "\u{FFFD}\x7F"],
["\xC2\x80", "\xC2\x80"],
["\xDF\xBF", "\xDF\xBF"],
["\xDF\xC0", "\u{FFFD}\u{FFFD}"],
["\xE0\xA0\x7F", "\u{FFFD}\x7F"],
["\xE0\xA0\x80", "\xE0\xA0\x80"],
["\xEF\xBF\xBF", "\xEF\xBF\xBF"],
["\xEF\xBF\xC0", "\u{FFFD}\u{FFFD}"],
["\xF0\x90\x80\x7F", "\u{FFFD}\x7F"],
["\xF0\x90\x80\x80", "\xF0\x90\x80\x80"],
["\xF4\x8F\xBF\xBF", "\xF4\x8F\xBF\xBF"],
["\xF4\x8F\xBF\xC0", "\u{FFFD}\u{FFFD}"],
["\xFA\x80\x80\x80\x80", "\u{FFFD}\u{FFFD}\u{FFFD}\u{FFFD}\u{FFFD}"],
["\xFB\xBF\xBF\xBF\xBF", "\u{FFFD}\u{FFFD}\u{FFFD}\u{FFFD}\u{FFFD}"],
["\xFD\x80\x80\x80\x80\x80", "\u{FFFD}\u{FFFD}\u{FFFD}\u{FFFD}\u{FFFD}\u{FFFD}"],
["\xFD\xBF\xBF\xBF\xBF\xBF", "\u{FFFD}\u{FFFD}\u{FFFD}\u{FFFD}\u{FFFD}\u{FFFD}"],

return array_map(function($i) {
    return [chr($i), $i < 128 ? chr($i) : "\u{FFFD}"];
}, range(0, 255));

There might be some test cases to borrow from libicu or the web-platform-tests but I couldn’t find them just now.

[alexdowad]

Opened a PR: #8384

[ausi]

Wow, that was fast!
Thank you very much @alexdowad !