mb_check_encoding() returns true for incorrect but interpretable ISO-2022-JP byte sequences

[pakutoma]

Description

Since PHP 8.1, mb_check_encoding returns true for many incorrect but interpretable ISO-2022-JP (JIS) byte sequences.
For example, IETF RFC 1468, often referenced as the definition of ISO-2022-JP, says "the text must end in ASCII." https://datatracker.ietf.org/doc/html/rfc1468
This means that an ISO-2022-JP byte sequence must end with the escape sequence 0x1b 0x28 0x42 to switch to ASCII.
However, mb_check_encoding() returns true without the escape sequence in PHP 8.1 and later.

The documentation says it returns true when "valid", but what should mb_check_encoding return in such a case?
https://www.php.net/manual/en/function.mb-check-encoding.php

3v4l:
https://3v4l.org/9i19F

The following code:

<?php

$jis_bytes = '1b244224221b2842'; // 'あ' in ISO-2022-JP
$jis_bytes_without_esc = '1b24422422'; // 'あ' in ISO-2022-JP without escape sequence
var_dump(mb_check_encoding(hex2bin($jis_bytes), 'JIS'));
var_dump(mb_check_encoding(hex2bin($jis_bytes_without_esc), 'JIS'));

Resulted in this output:

bool(true)
bool(true)

But I expected this output instead:

bool(true)
bool(false)

PHP Version

PHP 8.1.16

Operating System

No response

[youkidearitai]

Also, the text must end in ASCII.

Yes, probably indeed. @alexdowad Would you look this issue?

[alexdowad]

@pakutoma Thanks very much for raising this issue. This is a result of unifying the code for converting text encoding and checking the validity of text encoding.

I would like to hear your opinion. What do you think mb_convert_encoding should return when converting $jis_bytes_without_esc? Should it insert an error marker and increment mb_get_info('illegal_chars')? I think the answer is no.

What if an ISO-2022-JP string ends with something like "\x1B"? That is, an incomplete escape sequence. Should we insert an error marker and increment mb_get_info('illegal_chars')? Or should we ignore the incomplete trailing escape sequence for conversion, but still treat it as an error for validity checking?

[pakutoma]

Indeed, I think there are many situations where mb_check_encoding would be happier if it were true, since the $jis_bytes_without_esc example is simply missing the tail.

I found this problem when I discovered the behavior that mb_check_encoding returns true when interpreting the byte sequence 0xb1 0xc7 in ISO-2022-JP.
https://3v4l.org/kMuBG
This byte sequence represents the character "映" in EUC-JP, but it clearly does not seem to be a valid ISO-2022-JP string.
However, since this is a 1-byte character within the scope of JIS X 0201, the current judgement is "true".
I would like this to be judged as "false".

Alternatively, I think it would make sense if you could clarify the use of this function.
If mb_check_encoding is a function to avoid conversions that result in abnormal byte sequences, then this judgment is also correct.

[alexdowad]

@pakutoma Thanks for explaining the background of this report.

I wouldn't say that mb_check_encoding is a function "to avoid conversions that result in abnormal byte sequences". Whether conversion results in an abnormal byte sequence or not, mb_check_encoding should simply tell us: is this sequence of bytes legal in this text encoding, or not?

Let me try to explain more about the background of my comments above. Perhaps what I am asking will make more sense that way.

I understand that as a programmer and a user of mbstring, you have a very specific problem with PHP 8.1 right now. You are interested primarily in solving this specific problem.

As the library maintainer, my view is a bit different. I am interested in helping you with your problem, but even more, I am interested in fixing all instances of the same problem, for all possible uses of mbstring.

As library maintainer, I am also thinking: should I only adjust the behavior of mb_check_encoding here? Is there any other function in mbstring which might also need to be changed? What else will be affected by the fix?

I think that your issue is a real problem and should be fixed. Depending on how I fix it, it is possible that mb_convert_encoding will also be affected; or, it is possible to fix this problem without making any changes to mb_convert_encoding.

[alexdowad]

To be clear, my inclination here is to fix only mb_check_encoding and mb_detect_encoding and to leave the behavior of mb_convert_encoding unchanged.

[alexdowad]

What if an ISO-2022-JP string ends with something like "\x1B"? That is, an incomplete escape sequence. Should we insert an error marker and increment mb_get_info('illegal_chars')? Or should we ignore the incomplete trailing escape sequence for conversion, but still treat it as an error for validity checking?

@youkidearitai Any opinion on this?

[pakutoma]

@alexdowad
Thank you for your detailed explanation.
I agree with you that libraries are used for many purposes and we need to be consistent between those purposes.
If I am not interrupting, let me also comment on this discussion.

I read "insert an error marker" to mean insert MBFL_BAD_INPUT which is converted to "?".
If it is correct, then people who use mb_convert_encoding() will see mystery characters and will be troubled.

On the other hand, I think mb_check_encoding() looks good to return "false" if a broken escape sequence is passed.
In my opinion, it is also acceptable to return "false" even if the escape sequence is completely missing.
I think it would be useful as a heuristic to have an element that checks whether the correct escape sequence is used in mb_detect_encoding() as well.

To make a short summary, I agree with you.

[alexdowad]

@pakutoma OK, well, this sounds good.

I saw you opened a PR today. Are you interested in doing some coding on mbstring? If you would like to work on this issue, I can give you some guidance on what to do.

I think this problem should be fixed on PHP-8.2 first, merged down to master (PHP-8.3), then finally, we can also fix it on PHP-8.1. This is because the code has changed in PHP-8.2 and the fix for PHP-8.1 will not apply to PHP-8.2.

[pakutoma]

@alexdowad

Are you interested in doing some coding on mbstring?

Thanks for the great suggestions!
I'd love to contribute to this issue!

I understand about the merge destination.
I also understand that the active branch for PHP8.2 is PHP-8.2.
I will send a PR there.

I think we will start implementation only after the discussion is over, so I will try to learn about mbfilter until then.

I first thought of doing filter->num_illegalchar++; when there is an input inconsistent with the current filter->status (or no input in flush).
I thought this way I could flag mb_check_encoding for failure without affecting mb_convert_encoding.
However, such code does not seem to be used elsewhere.
There seems to be a mechanism I am unaware of to handle these issues.

[alexdowad]

@pakutoma Thanks for participating in mbstring development. These are the first things you should look at:

1. First, check out the latest PHP-8.2. You will find that in PHP-8.2, new code for converting text encodings was added. The original libmbfl conversion filters had an interface like:

int mbfl_conv_function(int c, mbfl_convert_filter *filter)

Each such function would take a single int, which would represent either a byte value or a Unicode codepoint value. It would convert that single value, pass another value to filter->output_function, then generally return 0.

The problem with this is that we have to make one function call for each byte which is converted to Unicode, and one call for each Unicode codepoint which is converted back to bytes. This is very slow.

The new conversion code takes an entire input string, fills a buffer with wchars (Unicode codepoints), then we convert the entire buffer to output bytes, then go back and refill the buffer with wchars again, etc. On average, this is about 3 times faster than the older libmbfl code.

2. See the interface for the new conversion functions in mbfl_encoding.h. Pick any specific encoding to see an example of their implementation; perhaps you might look at mbfilter_jis.c, since you seem to be interested in ISO-2022-JP.

Important question: Can you see what each of the arguments to mb_to_wchar_fn and mb_from_wchar_fn are for?

3. For an example of code using the new conversion functions, go to the bottom of mbfl_convert.c and see mb_fast_convert. Make sure you understand the usage of the mb_to_wchar_fn and mb_from_wchar_fn.

4. Now see the implementation of mb_check_encoding. The key part is in mbstring.c, in the function php_mb_check_encoding.

5. Ask questions about anything which you don't understand so far.

[alexdowad]

(You should also examine the definition of mb_convert_buf in mbfl_encoding.h. Note, this is another reason for the speed of the new conversion code; the older libmbfl code would emit output bytes into a malloc'd buffer, and then copy that buffer to create a zend_string; the new code directly builds a zend_string, so no copy is required.)