Skip to content

Browscap crashes PHP 8.1.12 on request shutdown (apache2) #10052

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.

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

Closed
nradchenko opened this issue Dec 5, 2022 · 8 comments
Closed

Browscap crashes PHP 8.1.12 on request shutdown (apache2) #10052

nradchenko opened this issue Dec 5, 2022 · 8 comments

Comments

@nradchenko
Copy link

Description

The following code:

<?php
$default_browser='Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/104.0.5112.79 Safari/537.36';
$browser = get_browser($default_browser, true);
print_r($browser);

Resulted in "zend_mm_heap corrupted" and exit code 1.

But I expected this output instead (or similar):

Array
(
    [browser_name_regex] => ~^mozilla/5\.0 \(.*linux.*x86_64.*\) applewebkit.* \(.*khtml.*like.*gecko.*\) chrome/104\.0.*safari/.*$~
    [browser_name_pattern] => Mozilla/5.0 (*Linux*x86_64*) applewebkit* (*khtml*like*gecko*) Chrome/104.0*Safari/*
    [parent] => Chrome 104.0
    [browser_bits] => 64
    [platform] => Linux
    [platform_description] => Linux
    [platform_bits] => 64
    [platform_maker] => Linux Foundation
    [device_name] => Linux Desktop
    [device_code_name] => Linux Desktop
    [comment] => Chrome 104.0
    [browser] => Chrome
    [browser_type] => Browser
    [browser_maker] => Google Inc
    [version] => 104.0
    [majorver] => 104
    [frames] => 1
    [iframes] => 1
    [tables] => 1
    [cookies] => 1
    [javascript] => 1
    [cssversion] => 3
    [aolversion] => 0
    [device_type] => Desktop
    [device_pointing_method] => mouse
    [renderingengine_name] => Blink
    [renderingengine_description] => a WebKit Fork by Google
    [renderingengine_maker] => Google Inc
    [browser_modus] => unknown
    [minorver] => 0
    [platform_version] => unknown
    [alpha] =>
    [beta] =>
    [win16] =>
    [win32] =>
    [win64] =>
    [backgroundsounds] =>
    [vbscript] =>
    [javaapplets] =>
    [activexcontrols] =>
    [ismobiledevice] =>
    [istablet] =>
    [issyndicationreader] =>
    [crawler] =>
    [isfake] =>
    [isanonymized] =>
    [ismodified] =>
    [device_maker] => unknown
    [device_brand_name] => unknown
    [renderingengine_version] => unknown
)
(gdb) bt
#0  0x00007fc84f335140 in write () from target:/lib/x86_64-linux-gnu/libc.so.6
#1  0x00007fc84f2b01bd in _IO_file_write () from target:/lib/x86_64-linux-gnu/libc.so.6
#2  0x00007fc84f2b0b2f in _IO_file_xsputn () from target:/lib/x86_64-linux-gnu/libc.so.6
#3  0x00007fc84f283707 in ?? () from target:/lib/x86_64-linux-gnu/libc.so.6
#4  0x00007fc84f280726 in vfprintf () from target:/lib/x86_64-linux-gnu/libc.so.6
#5  0x00007fc84f3574c6 in __fprintf_chk () from target:/lib/x86_64-linux-gnu/libc.so.6
#6  0x00007fc84d9e4d4b in fprintf (__fmt=0x7fc84ddeee94 "%s\n", __stream=<optimized out>) at /usr/include/x86_64-linux-gnu/bits/stdio2.h:97
#7  zend_mm_panic (message=0x7fc84e627183 "zend_mm_heap corrupted") at /build/tmp-build/php-src-tag-beget-version-8.1.12-1/Zend/zend_alloc.c:359
#8  0x00007fc84dcd8301 in zend_mm_free_heap (ptr=<optimized out>, heap=<optimized out>) at /build/tmp-build/php-src-tag-beget-version-8.1.12-1/Zend/zend_alloc.c:1369
#9  _efree (ptr=<optimized out>) at /build/tmp-build/php-src-tag-beget-version-8.1.12-1/Zend/zend_alloc.c:2552
#10 0x00007fc84dbfff7b in zend_string_release_ex (persistent=false, s=<optimized out>) at /build/tmp-build/php-src-tag-beget-version-8.1.12-1/Zend/zend_string.h:336
#11 browscap_entry_dtor (zvalue=<optimized out>) at /build/tmp-build/php-src-tag-beget-version-8.1.12-1/ext/standard/browscap.c:73
#12 0x00007fc84dd124d2 in zend_hash_destroy (ht=0x7fc842a03380) at /build/tmp-build/php-src-tag-beget-version-8.1.12-1/Zend/zend_hash.c:1602
#13 0x00007fc84dbffe31 in browscap_bdata_dtor (bdata=bdata@entry=0x7fc84e9a69a0 <browscap_globals>, persistent=persistent@entry=0)
    at /build/tmp-build/php-src-tag-beget-version-8.1.12-1/ext/standard/browscap.c:464
#14 0x00007fc84dc01502 in zm_deactivate_browscap (type=type@entry=1, module_number=module_number@entry=38) at /build/tmp-build/php-src-tag-beget-version-8.1.12-1/ext/standard/browscap.c:523
#15 0x00007fc84dbf8f65 in zm_deactivate_basic (type=1, module_number=38) at /build/tmp-build/php-src-tag-beget-version-8.1.12-1/ext/standard/basic_functions.c:562
#16 0x00007fc84dd0751b in zend_deactivate_modules () at /build/tmp-build/php-src-tag-beget-version-8.1.12-1/Zend/zend_API.c:3046
#17 0x00007fc84dc9b8a5 in php_request_shutdown (dummy=dummy@entry=0x0) at /build/tmp-build/php-src-tag-beget-version-8.1.12-1/main/main.c:1824
#18 0x00007fc84dde89e7 in php_apache_request_dtor (r=0x563a7943d840) at /build/tmp-build/php-src-tag-beget-version-8.1.12-1/sapi/apache2handler/sapi_apache2.c:546
#19 php_handler (r=<optimized out>) at /build/tmp-build/php-src-tag-beget-version-8.1.12-1/sapi/apache2handler/sapi_apache2.c:718
#20 0x0000563a5626dc30 in ap_run_handler (r=r@entry=0x563a7943d840) at config.c:169
#21 0x0000563a5626e1ad in ap_invoke_handler (r=r@entry=0x563a7943d840) at config.c:443
#22 0x0000563a562a19bb in ap_process_async_request (r=0x563a7943d840) at http_request.c:452
#23 0x0000563a562a1b9e in ap_process_request (r=r@entry=0x563a7943d840) at http_request.c:487
#24 0x0000563a5629ddf4 in ap_process_http_sync_connection (c=0x563a79366800) at http_core.c:208
#25 ap_process_http_connection (c=0x563a79366800) at http_core.c:249
#26 0x0000563a56277770 in ap_run_process_connection (c=c@entry=0x563a79366800) at connection.c:42
#27 0x00007fc84e9b4087 in itk_fork_process (c=0x563a79366800) at mpm_itk.c:213
#28 0x0000563a56277770 in ap_run_process_connection (c=c@entry=0x563a79366800) at connection.c:42
#29 0x0000563a56277c9e in ap_process_connection (c=c@entry=0x563a79366800, csd=<optimized out>) at connection.c:210
#30 0x0000563a563018c8 in child_main (child_num_arg=child_num_arg@entry=8, child_bucket=child_bucket@entry=0) at prefork.c:654
#31 0x0000563a56301bd0 in make_child (s=0x563a56bfc418, slot=8) at prefork.c:756
#32 0x0000563a5630262a in perform_idle_server_maintenance (p=<optimized out>) at prefork.c:860
#33 prefork_run (_pconf=<optimized out>, plog=<optimized out>, s=<optimized out>) at prefork.c:1053
#34 0x0000563a5624f08e in ap_run_mpm (pconf=0x563a56bc5388, plog=0x563a56c032f8, s=0x563a56bfc418) at mpm_common.c:95
#35 0x0000563a56246ce6 in main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at main.c:894
(gdb) 

PHP Version

8.1.12

Operating System

Ubuntu 18.04

@cmb69
Copy link
Member

cmb69 commented Dec 5, 2022

What is the value of the php.ini setting browscap? Is it a relative path?

@nradchenko
Copy link
Author

Nope, it's an absolute one - /home/x/xxxxxxxx/xxxx/public_html/browscap.ini (redacted, the length is the same as of original).

The file itself is lite_php_browscap.ini.

@cmb69
Copy link
Member

cmb69 commented Dec 6, 2022

Thanks for the feedback. I still can't reproduce this issue, though. Looking at the code reveals no issues either, although there is a comment which hints at potential issues:

/* OBJECTS_FIXME: This whole extension needs going through. The use of objects looks pretty broken here */

Since this is about apache2handler, the issue might only happen with certain MPMs. I'm using mpm_winnt and there is "just" a memory leak, because PHP is started up in the control process, which is never used to handle requests (see #7865).

Which MPM do you use?

@nradchenko
Copy link
Author

It's apache 2.4.51 with mpm-itk.

Also this is the minimal reproducer I could come up with:

<?php
get_browser();
[foo]
Comment="foo"

[bar]
Parent="foo"
php_admin_value browscap "/home/x/xxxxxxxx/xxxxxxxxxxxxxxxxxxx/public_html/browscap.ini"

@nradchenko
Copy link
Author

I did some more tests: browscap doesn't trigger a crash in 7.2.34, but 7.3+ is certainly broken. I've stumbled upon 5eb1f92 while looking through browscap.c changes from 7.2 to 7.3, so I suppose that this fix might cause the problem, though I should do a proper bisect to confirm this.

@nradchenko
Copy link
Author

My guess was correct, bisect has revealed that 5eb1f92 is the first bad commit. I'd be happy if someone explains me what is the refactoring behind these changes, and how could they particularly affect mpm-itk.

Although the commit affects a bunch of other extension files, the only issue of a kind I know about so far is the browscap problem. Should I run some tests to make sure that my SAPI is sane? I know that PHP has a lot of tests already written, but they are usually run within CLI, and I don't know how to run them for apache2. Is that possible?

Thanks.

@cmb69 cmb69 self-assigned this Dec 7, 2022
@cmb69
Copy link
Member

cmb69 commented Dec 7, 2022

The Zend Engine distinguishes between persistent and regular memory allocations. Regular allocation are supposed to be freed after each request; persistent allocations are supposed to be freed when the process shuts down.

zend_string_release_ex() expects the user to provide the proper persistent flag, while zend_string_release() figures that out by itself, but is a bit slower. It looks like some of these changes are not correct (apparently, a persistent string is released as regular string). It is possible, that this only affects mpm-itk, which appears to use some uncommon approach regarding request handling (fork on each request).

@cmb69
Copy link
Member

cmb69 commented Dec 13, 2022

php_admin_value browscap "/home/x/xxxxxxxx/xxxxxxxxxxxxxxxxxxx/public_html/browscap.ini"

Ah, I think that explains the problem (previously, I tried the setting in php.ini, and that didn't cause issues). If I revert the respective changes to browscap.c, I can now go beyond the original failure, but still get an assertion violation in zend_shutdown_executor_values().

Anyhow, it seems that string interning is the concrete issue, but a more general problem is that the browscap ini is read again for every request what is wasteful. I shall have a closer look.

nielsdos added a commit to nielsdos/php-src that referenced this issue Mar 19, 2023
…che2)

get_browser() implements a lazy parse system for the browscap
INI configuration. There are two possible moments when a browscap
configuration can be loaded: during module startup or during request.
In case of module startup, the strings are persistent strings, while for
the request they are not.

The INI parser must therefore know whether to create persistent or
non-persistent strings. It does this by looking at
CG(ini_parser_unbuffered_errors). If that value is 1 it's persistent,
otherwise non-persistent. Note that this also controls how the errors
are reported: if it's 1 then the errors are sent to stderr, otherwise we
get E_WARNINGs.

Currently, a hardcoded value of 1 is always used for that CG value in
browscap_read_file(). This means we'll always create persistent strings
*and* we'll not report parse errors correctly as E_WARNINGs.
We fix both the crash and the lack of warnings by passing the value of
persistent instead of a hardcoded 1.

This is also in line with how other INI parsing code is called in
ext/standard: they also make sure that during request a value of 0 is
passed.
nielsdos added a commit that referenced this issue Mar 20, 2023
* PHP-8.1:
  Fix GH-10052: Browscap crashes PHP 8.1.12 on request shutdown (apache2)
nielsdos added a commit that referenced this issue Mar 20, 2023
* PHP-8.2:
  Fix GH-10052: Browscap crashes PHP 8.1.12 on request shutdown (apache2)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants
@cmb69 @nielsdos @nradchenko and others