mb_detect_encoding() detects UTF-8 emoji byte sequence as ISO-8859-1 since PHP 8.1

Description

I have a piece of code that tries to normalise the encoding of incoming strings using mb_detect_encoding(). While upgrading to PHP 8.1, I've noticed that a test which ensures that an urlencoded UTF-8 sequence (party hat emoji) now fails. It all comes down to a behaviour change of mb_detect_encoding() when passing UTF-8 and ISO-8859-1 (in which ever order) as $encodings in PHP 8.1.

I know that the way this method works (or the way that determining the enconding of a string works at all) can not be 100% realiable, so I'd also agree on not classifying this as a bug. However, it is an undocumented behaviour change introduced in 8.1 that might break existing code as it did with mine.

I assume that this change has been introduced with 28b346b.

Example

See https://3v4l.org/RgdfE

<?php
echo mb_detect_encoding('🥳', ['UTF-8', 'ISO-8859-1']);

Resulted in this output:

ISO-8859-1

But I expected this output instead:

UTF-8

PHP Version

8.1.1

Operating System

No response

@filecage

Hi, @filecage, and thanks for much for this report. Hope to look into it very soon.

@nikic

Hmm. One point to start:

Legacy versions of PHP return UTF-8, not because they are smart enough to tell that the string is actually UTF-8 text, but because you put UTF-8 first in the list of candidate encodings. If you put ISO-8859-1 first, then all versions return ISO-8859-1.

The string is actually valid under both encodings; if interpreted as ISO-8859-1, it's an accented "o", followed by an accented "Y", followed by a Yen sign, followed by a superscript digit 3. A funny combination, but not invalid.

The current detection code treats all codepoints higher than U+FFFF as "rare", and penalizes candidate encodings under which the string contains such codepionts. That is what is making it prefer ISO-8859-1 over UTF-8.

@nikic Do you think I need to teach it that some emoji over U+FFFF are not "rare"?

Another point... no matter what we do, encoding detection will always be very, very unreliable for short strings. If you want to auto-detect text encoding with any level of reliability, you really need to give it more input text to work with.

Even so, let us think a bit about this issue of emoji. It is a fact that some emoji are used by a lot of people, and they do occupy ranges of Unicode codepoints over U+FFFF.

@alexdowad

Thanks a lot @alexdowad for looking into this!

Legacy versions of PHP return UTF-8, not because they are smart enough to tell that the string is actually UTF-8 text, but because you put UTF-8 first in the list of candidate encodings. If you put ISO-8859-1 first, then all versions return ISO-8859-1.

When I saw that the output was different, I did not think that previous versions might be accidentally returning the correct results.

Even so, let us think a bit about this issue of emoji. It is a fact that some emoji are used by a lot of people, and they do occupy ranges of Unicode codepoints over U+FFFF.

I haven't yet had a chance to look deeply into it but I could imagine that if we're somehow able to reliably detect emojis in a string, it's a very strong indicator for a string to be UTF-8, even for short strings. It might not be worth the effort though.

I have the same problem, if text starts with emoji, it's not detected as UTF-8 (I am using Docker php:8.1.6-fpm)

I have a smiliar problem too, with a custom test suit in PhpUnit that throw us an error.

The test check if mb_detect_encoding find the correct encoding with an UTF-8 file content, with a BOM header.

You can see the simplify test here :
https://3v4l.org/83vWZ

The problem is present in php 8.1 and 8.2 too.

@titanz35

@titanz35's problem has nothing to do with emoji; rather, it's about detecting encoding for strings which use a BOM. I have just prepared a fix for that.

@filecage

@filecage, I am just amending mb_detect_encoding so it accepts the most basic emoji like "smiling face" and "frowning face". But I have seen that your test case is actually U+1F973 FACE WITH PARTY HORN AND PARTY HAT.

Do you think that qualifies as a 'common' and thus expected character to be found in input strings for mb_detect_encoding?

@alexdowad

@alexdowad according to unicode, U+1F973 is categorized as basic emoji: https://unicode.org/Public/emoji/15.0/emoji-sequences.txt

When it comes to what I'd expect from mb_detect_encoding(), I would probably go with: either all or nothing. I know that that's a tough one, but I assume that people (including myself) won't expect mb_detect_encoding() to work with with some, but not all.

Just out of personal interest, what would be a good reason not to include all unicode standardized emojis? Is it that it would slow down the detection itself or that we have no reliable way of having an exhaustive list of existing emojis? Or is it something else?

@filecage

@filecage The reason would be both to keep the detection code tight and fast and also to reduce the chances of wrongly detecting some random byte sequence as 'emoji'.

@iluuu1994

@iluuu1994 What's the best way to free the memory used by such an array on module shutdown?

@alexdowad

@alexdowad You don't have to do anything special, if you set the IS_ARRAY_PERSISTENT flag zend_hash_destroy should correctly call free instead of efree.

@iluuu1994

@iluuu1994 Thanks! I seem to be making some progress here. However, something seems to be dawning on me here...

Is it correct that the "immutable array" feature is only to be used for arrays which user code will never attempt to modify?

Because with my current test code, this causes an interpreter crash on shutdown...

$s = mb_list_encodings();
$s[0] = 'blahblah';

@alexdowad

@alexdowad It should be fine to "mutate" immutable arrays. The point is that the refcount = 2 guarantees that the array will be copied before actually mutating it. One gotcha: GC_TRY_ADDREF checks if the element is immutable, and only adds a ref if it isn't. Z_TRY_ADDREF doesn't do that. Instead, it checks if the element is Z_REFCOUNTED_P, which it is considered only if the (zval).u1.v.type_flags == 0.

This means, after using ZVAL_ARR (or whatever variation of that) you need to set Z_TYPE_INFO_P(zval) = IS_ARRAY (or Z_TYPE_FLAGS_P(zval) = 0).

I think, if you only share the value in the same process, you could also just not make the array immutable and set the RC to 2. This should also prevent it from being deallocated, while keeping refcounting.

@iluuu1994

@iluuu1994 Thanks for that. I need to dig in more and see if I can figure out why this crash is happening.

It looks like on $a[0] = 'blahblah';, the Zend VM replaces the persistent string which is stored in my (supposedly) immutable array, decs the refcount for the persistent string, notices that it is now unreferenced, tries to free it, then an assertion fails and the interpreter crashes because the string is persistent.

The persistent string is freed from zend_vm_execute.h:42445:

if (garbage) {
  GC_DTOR_NO_REF(garbage);
}

If that analysis is true, then I need to figure out why the array is not being copied.

If you have time to have a look, this is my test code: alexdowad@c8cf129

@alexdowad

@alexdowad Here's a simpler implementation that only uses a request-bound allocation.

master...iluuu1994:php-src:mbstring-shared-mb_list_encodings

@iluuu1994

@iluuu1994 Wow, thanks!

Now that I think about it, this will also achieve the same purpose... we can still tell if mb_list_encodings() is being passed to other mbstring functions, even if it's a per-request shared array and not a per-process shared array...

@alexdowad

@alexdowad Yes. You can just compare MBSTRG(all_encodings_list) == ht (or zend_is_identical/zend_compare if that's still faster than operating on the user-provided array).

@mrbase

Very early draft of the proposed change to text encoding detection: be5aa65

If 0.2 is changed to 0.3 in the following code, that is enough to make @mrbase's test case return the results which he wants:

https://github.com/alexdowad/php-src/blob/be5aa651685cdf7f8b7838fb459a58343c1bb89a/ext/mbstring/mbstring.c#L3022-L3025

So I don't forget, you'll also need to remove @refcount 1 from function mb_list_encodings() in ext/mbstring/mbstring.stub.php.

So I don't forget, you'll also need to remove @refcount 1 from function mb_list_encodings() in ext/mbstring/mbstring.stub.php.

Thank you very much!! I totally would have missed that.

@mrbase

I have just opened a PR which adds @mrbase's test case to the official test suite.

@alexdowad

@alexdowad Is this issue resolved or not?

@Girgias

@Girgias It's resolved. Thanks.

Navigation Menu

Repository navigation

`mb_detect_encoding()` detects UTF-8 emoji byte sequence as ISO-8859-1 since PHP 8.1 #7871

Description

Example

PHP Version

Operating System

30 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

mb_detect_encoding() detects UTF-8 emoji byte sequence as ISO-8859-1 since PHP 8.1 #7871

Description

Description

Example

PHP Version

Operating System

Activity

alexdowad commented on Jan 2, 2022

alexdowad commented on Jan 2, 2022

filecage commented on Jan 4, 2022

jurihahn commented on Jun 16, 2022

titanz35 commented on Sep 6, 2022

alexdowad commented on Jan 12, 2023

alexdowad commented on Jan 12, 2023

filecage commented on Jan 12, 2023

alexdowad commented on Jan 12, 2023

30 remaining items

alexdowad commented on Apr 29, 2023

iluuu1994 commented on Apr 29, 2023

alexdowad commented on Apr 29, 2023

iluuu1994 commented on Apr 29, 2023

alexdowad commented on Apr 29, 2023

iluuu1994 commented on Apr 30, 2023

alexdowad commented on Apr 30, 2023

iluuu1994 commented on Apr 30, 2023

alexdowad commented on Apr 30, 2023

iluuu1994 commented on Apr 30, 2023

alexdowad commented on May 14, 2023

alexdowad commented on May 14, 2023

Girgias commented on May 30, 2024

alexdowad commented on Jun 6, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

`mb_detect_encoding()` detects UTF-8 emoji byte sequence as ISO-8859-1 since PHP 8.1 #7871