[go: up one dir, main page]
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

mb_convert_encoding crashes PHP on Windows #10627

Closed
liviuconcioiu opened this issue Feb 19, 2023 · 3 comments
Closed

mb_convert_encoding crashes PHP on Windows #10627

liviuconcioiu opened this issue Feb 19, 2023 · 3 comments
Labels
Bug Status: Needs Triage

Comments

@liviuconcioiu
Copy link

Description

The following code:

<?php

if (($handle = fopen('https://www.google.com/', 'r')) !== false) {
    while (($data = fgetcsv($handle, 1000, ";")) !== false) {
        $data = mb_convert_encoding($data, 'UTF-8', 'auto');
        print_r($data);
    }
    fclose($handle);
}

Resulted in this output:

Faulting application name: php-cgi.exe, version: 8.2.3.0, time stamp: 0x63eb5a8b
Faulting module name: php_mbstring.dll, version: 8.2.3.0, time stamp: 0x63eb5fc1
Exception code: 0xc0000005
Fault offset: 0x000000000002faeb
Faulting process id: 0x990c
Faulting application start time: 0x01d944767c2c1fad
Faulting application path: z:\nginx\php\php-cgi.exe
Faulting module path: z:\nginx\php\ext\php_mbstring.dll
Report Id: 32e2f828-cdb9-43ea-82bb-91f6816d7a8d
Faulting package full name: 
Faulting package-relative application ID:

DebugDiag Analysis Report

php-cgi.exe_230219_162706_MultipleRules.zip

PHP Version

8.2.3

Operating System

Windows 10
@nielsdos
Copy link
Member

Reproducing this depends on getting the exact google.com version as you got, which is locale dependent. I can't reproduce it with the version google.com gives me here.
I can see the crash happens in php_mb_convert_encoding_recursive, but without a reproducer it's gonna be hard to find the root cause.
Could you maybe share the html you get from google.com?
I.e. could you please execute this script and give us the resulting out.html ?

<?php
$f = fopen('https://www.google.com/', 'r');
$o = fopen('out.html', 'w');
while ($l = fread($f, 1024)) {
        fwrite($o, $l);
}
fclose($f);
fclose($o);

@liviuconcioiu
Copy link
Author
liviuconcioiu commented Feb 19, 2023 •
edited
Loading

Here it is.

out.zip

Also, on CentOS with php-fpm this is the result:

...
(
    [0] => </script></head><body bgcolor="#fff"><script nonce="ck2GIhMadw8bv6pQ1DQJBw">(function(){var src='/images/nav_logo229.png'
    [1] => var iesg=false
    [2] => document.body. && window.n()
    [3] => if (document.images){new Image().src=src
    [4] => }
)
Array
(
    [0] => if (!iesg){document.f&&document.f.q.focus()
    [1] => document.gbqf&&document.gbqf.q.focus()
    [2] => }
)
Array
(
    [0] => }
)
PHP Warning:  mb_convert_encoding(): Unable to detect character encoding in /var/.../test.php on line 5
Segmentation fault (core dumped)

@nielsdos
Copy link
Member

Thank you! I can repro it on 8.2, not on 8.1. However, the bug really is there in 8.1. It's just that the encoding handling changed in 8.2 so that we can actually trigger it with your PoC in 8.2. I found the root cause being missing error handling and I'll work on a fix now.

nielsdos added a commit to nielsdos/php-src that referenced this issue Feb 19, 2023
@nielsdos
Fix phpGH-10627: mb_convert_encoding crashes PHP on Windows
f5530ff 
Fixes phpGH-10627

The php_mb_convert_encoding() function can return NULL on error, but
this case was not handled, which led to a NULL pointer dereference and
hence a crash.
@nielsdos nielsdos mentioned this issue Feb 19, 2023
Closed
Girgias added a commit that referenced this issue Feb 20, 2023
@Girgias
Merge branch 'PHP-8.1' into PHP-8.2
73f9ffc 
* PHP-8.1:
  Fix GH-10627: mb_convert_encoding crashes PHP on Windows
  ext/mbstring: fix new_value length check
Girgias added a commit that referenced this issue Feb 20, 2023
@Girgias
Merge branch 'PHP-8.2'
0685f30 
* PHP-8.2:
  Fix GH-10627: mb_convert_encoding crashes PHP on Windows
  ext/mbstring: fix new_value length check
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Status: Needs Triage
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@nielsdos @liviuconcioiu and others