8.3 crash with mb_detect_encoding

[brainfoolong]

Description

The following code:

<?php
// iso 8859-1 string of:
/*
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa;aaaaaaa aaa aaöaaaa aa.aöaaaa
*/

$str = base64_decode("YWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhDQphYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYTthYWFhYWFhIGFhYSBhYfZhYWFhIGFhLmH2YWFhYQ0K");
$detectedEncoding = mb_detect_encoding($str, mb_list_encodings(), true);
// never reaches, crash here with core dump
var_dump($detectedEncoding);

Resulted in this output:

None, because of silent crash.

But I expected this output instead:

Some encoding information

This script crashes in php8.3 alpha 1 on windows 10 64 bit in command line (and also via php-cgi in apache).
Core dump is attached.

php.exe_230619_100518_CrashHangAnalysis.mht.txt

Summary of crash: In php.exe_230619_100518.dmp the assembly instruction at php_mbstring!onig_unicode_define_user_property+290d in C:\srv\php8\ext\php_mbstring.dll from The PHP Group

Php 8.2 is fine.

PHP Version

8.3 alpha1

Operating System

Windows 10 64 bit

[damianwadley]

=================================================================
==284387==ERROR: AddressSanitizer: negative-size-param: (size=-40)
    #0 0x7f9f4be22f97 in __interceptor_memmove ../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:810
    #1 0x559de7c1bbbe in count_demerits /home/ubuntu/php/php-8.3.0alpha1/src/ext/mbstring/mbstring.c:3089
    #2 0x559de7c1c322 in mb_guess_encoding_for_strings /home/ubuntu/php/php-8.3.0alpha1/src/ext/mbstring/mbstring.c:3137
    #3 0x559de7c1c57d in mb_guess_encoding /home/ubuntu/php/php-8.3.0alpha1/src/ext/mbstring/mbstring.c:3159
    #4 0x559de7c1d37a in zif_mb_detect_encoding /home/ubuntu/php/php-8.3.0alpha1/src/ext/mbstring/mbstring.c:3220
    #5 0x559de85429e8 in ZEND_DO_ICALL_SPEC_RETVAL_USED_HANDLER /home/ubuntu/php/php-8.3.0alpha1/src/Zend/zend_vm_execute.h:1337
    #6 0x559de869ab6a in execute_ex /home/ubuntu/php/php-8.3.0alpha1/src/Zend/zend_vm_execute.h:57053
    #7 0x559de86adbef in zend_execute /home/ubuntu/php/php-8.3.0alpha1/src/Zend/zend_vm_execute.h:61433
    #8 0x559de849fc4b in zend_execute_scripts /home/ubuntu/php/php-8.3.0alpha1/src/Zend/zend.c:1859
    #9 0x559de83206ca in php_execute_script /home/ubuntu/php/php-8.3.0alpha1/src/main/main.c:2482
    #10 0x559de8885a0f in do_cli /home/ubuntu/php/php-8.3.0alpha1/src/sapi/cli/php_cli.c:959
    #11 0x559de8887745 in main /home/ubuntu/php/php-8.3.0alpha1/src/sapi/cli/php_cli.c:1328
    #12 0x7f9f48b32d8f in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
    #13 0x7f9f48b32e3f in __libc_start_main_impl ../csu/libc-start.c:392
    #14 0x559de760e724 in _start (/home/ubuntu/php/php-8.3.0alpha1/bin/php+0x60e724)

Address 0x7fff0e25ec60 is located in stack of thread T0 at offset 2016 in frame
    #0 0x559de7c1b84f in count_demerits /home/ubuntu/php/php-8.3.0alpha1/src/ext/mbstring/mbstring.c:3066

  This frame has 1 object(s):
    [32, 544) 'wchar_buf' (line 3067) <== Memory access at offset 2016 overflows this variable
HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
      (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: negative-size-param ../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:810 in __interceptor_memmove
==284387==ABORTING

(gdb) list mbstring.c:3089
3084                                                            /* This candidate encoding is not valid, eliminate it from consideration */
3085                                                            length--;
3086                                                            if (length == 0) {
3087                                                                    return 0;
3088                                                            }
3089                                                            memmove(&array[i], &array[i+1], (length - i) * sizeof(struct candidate));
3090                                                            goto try_next_encoding;
3091                                                    } else {
3092                                                            array[i].demerits += 1000;
3093                                                    }
(gdb) break mbstring.c:3089 if length < i
Breakpoint 1 at 0xc1bb46: file /home/ubuntu/php/php-8.3.0alpha1/src/ext/mbstring/mbstring.c, line 3089.
(gdb) run gh11476.php
Starting program: /home/ubuntu/php/php-8.3.0alpha1/bin/php gh11476.php
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".

Breakpoint 1, count_demerits (array=0x7fffffffa240, length=30, strict=true) at /home/ubuntu/php/php-8.3.0alpha1/src/ext/mbstring/mbstring.c:3089
3089                                                            memmove(&array[i], &array[i+1], (length - i) * sizeof(struct candidate));
(gdb) print i
$1 = 31
(gdb) print array[i+1]
$2 = {enc = 0x5555576ebde0 <mbfl_encoding_cp50222>, in = 0x7ffff027f098 'a' <repeats 107 times>, ";aaaaaaa aaa aa\366aaaa aa.a\366aaaa\r\n", in_len = 139, demerits = 128, state = 0, multiplier = 1}

So length was decreasing in that while loop and got overrun by i.

[iluuu1994]

I deleted my previous comment because I thought there was an issue in the patch, but it's correct after all. Here is the comment again.

Here's a simpler reproducer:

--TEST--
GH-11476: count_demerits negative-size-param
--EXTENSIONS--
mbstring
--FILE--
<?php
$str = str_repeat('a', 250) . chr(246) . str_repeat('a', 9) . chr(246) . str_repeat('a', 6);
$detectedEncoding = mb_detect_encoding($str, mb_list_encodings(), true);
var_dump($detectedEncoding);
?>
--EXPECT--
bool(false)

This patch avoids the issue:

diff --git a/ext/mbstring/mbstring.c b/ext/mbstring/mbstring.c
index 758c00b426..72d39fb6d3 100644
--- a/ext/mbstring/mbstring.c
+++ b/ext/mbstring/mbstring.c
@@ -3083,7 +3083,7 @@ static size_t count_demerits(struct candidate *array, size_t length, bool strict
 						if (strict) {
 							/* This candidate encoding is not valid, eliminate it from consideration */
 							length--;
-							if (length == 0) {
+							if (length == 0 || i >= length) {
 								return 0;
 							}
 							memmove(&array[i], &array[i+1], (length - i) * sizeof(struct candidate))

However, the output (false) looks wrong, as 246 is a valid code in Windows-1252: https://www.ascii-code.com/CP1252/246

/cc @alexdowad

[youkidearitai]

This change was made in Pull Request: #11036

In PHP 8.2 is Windows-1252(3v4l https://3v4l.org/4II3g).

[nielsdos]

@iluuu1994 @alexdowad I rather think the fix needs to be this (this also gives the right result for Ilija's test):

diff --git a/ext/mbstring/mbstring.c b/ext/mbstring/mbstring.c
index 758c00b426..959171e9b8 100644
--- a/ext/mbstring/mbstring.c
+++ b/ext/mbstring/mbstring.c
@@ -3066,6 +3066,7 @@ static size_t count_demerits(struct candidate *array, size_t length, bool strict
 {
 	uint32_t wchar_buf[128];
 	unsigned int finished = 0; /* For how many candidate encodings have we processed all the input? */
+	const size_t full_array_length = length;
 
 	while ((strict || length > 1) && finished < length) {
 		for (size_t i = 0; i < length; i++) {
@@ -3086,7 +3087,8 @@ static size_t count_demerits(struct candidate *array, size_t length, bool strict
 							if (length == 0) {
 								return 0;
 							}
-							memmove(&array[i], &array[i+1], (length - i) * sizeof(struct candidate));
+							/* Remove entry i from the array by shifting everything one place to the left. */
+							memmove(&array[i], &array[i+1], (full_array_length - i - 1) * sizeof(struct candidate));
 							goto try_next_encoding;
 						} else {
 							array[i].demerits += 1000;

[iluuu1994]

@nielsdos Of course, I didn't look at the memmove well enough 🙂 I think the memmove could actually be a memcpy, as they shouldn't overlap. Edit: Heh, wrong again. I thought we're only moving a single element. Sorry 😄

[nielsdos]

@nielsdos Of course, I didn't look at the memmove well enough 🙂 I think the memmove could actually be a memcpy, as they shouldn't overlap.

I'm not sure I understand. They have at least partial overlap, so memcpy cannot be used.

Edit: no worries!