Repository navigation
Apache crash on Windows when using a self-referencing anonymous function inside a class with an active mysqli connection #10599
Description
Description
The following code:
<?php
$x = new X();
$x->y();
class X
{
public function __construct()
{
$this->link = @new mysqli();
@$this->link->connect('.', 'root', 'password', 'instance', null, '\\\\.\\pipe\\pipeName');
}
public function y()
{
$parseConnection = null;
$parseConnection = function() use (&$parseConnection) {
};
}
}Resulted in this output:
The Apache service crashes after accessing this page multiple times (reload the page 2 to 5 times).
In the Windows Event viewer you can find this message (sorry for the German text, but the OS is in German):
--------------------------------------------------------------
Name der fehlerhaften Anwendung: httpd.exe, Version: 21.3.0.49152, Zeitstempel: 0x61646032
Name des fehlerhaften Moduls: php8ts.dll, Version: 8.1.14.0, Zeitstempel: 0x63b57733
Ausnahmecode: 0xc0000005
Fehleroffset: 0x000000000005cc55
ID des fehlerhaften Prozesses: 0xdfc
Startzeit der fehlerhaften Anwendung: 0x01d9413adafb9578
Pfad der fehlerhaften Anwendung: C:\apache\bin\httpd.exe
Pfad des fehlerhaften Moduls: C:\apache\php-8.1.14-Win32-vs16-x64\php8ts.dll
Berichtskennung: b79274d7-45ed-48f8-a592-478bcbe8df41
Vollständiger Name des fehlerhaften Pakets:
Anwendungs-ID, die relativ zum fehlerhaften Paket ist:
But I expected this output instead:
It should just work, without any process crashes.
In PHP 7.3 this code works fine. Starting with PHP 7.4 I got the crash.
I've tested much to find this bug. It only appears if you have a mysqli connection inside of the class instance.
Also you need to use a anonymous function with a use of the variable by reference where you assign the anonymous function.
As a workaround for me I will get rid of this anonymous function now. But however it should not be possible to crash the executing process like this.
PHP Version
PHP 8.1.14 (the bug exists since 7.4, in 7.3 there is no crash)
Operating System
Windows
Activity
Girgias commentedon Feb 20, 2023
@kamil-tekiela can you maybe have a look at this to see if you can reproduce it? Don't remember the sets to getting ext/mysqli test suit running locally
kamil-tekiela commentedon Feb 20, 2023
I can reproduce it on Windows.
The MCVE is:
This begs the question: if it's so easily reproducible, does this mean nobody uses this feature?
ndossche commentedon Mar 9, 2024
Because it only crashes on shutdown, so for most SAPIs that takes a while until it crashes; and the SAPI just start the process automatically on a crash. It's very visible on CGI and CLI though where it will crash after handling the request/file.
What happens is that the persistent network stream resource gets freed, yet stays inside EG(persistent_list). This causes a crash on shutdown when the persistent list is getting cleared, as the engine will try to free the network stream again.
The code at fault is in
MYSQLND_METHOD(mysqlnd_vio, close_stream), where it does the mixup between persistent vs non-persistent destruction:php-src/ext/mysqlnd/mysqlnd_vio.c
Lines 654 to 665 in bb1ef4f
Notice how it checks for
EG(active)to check if we're in shutdown or not. TheThe EG(active) check here is dead -- check IN_SHUTDOWN?comment is misleading. This code was introduced in c3019a1 to fix crashes when the persistent list gets destroyed before module shutdown is called. This is indeed a potential problem that I fixed permanently on the master branch in 5941cdaSo for master, the fix it simple: remove that
EG(active)check and remove the misleading comment and it works. I checked this change, and indeed, when trying on Windows with OP's code I no longer get an ASAN error.On lower branches, it's trickier. Although on the surface level it seems that the same fix also works there (I tried this), it might still run into the issue that the persistent list is destroyed, although I'm not entirely certain how hard it is to trigger that. Then again, if we crash already with the existing code, we might as well change it anyway?
In any case: on master it's a safe fix, on lower branches not necessarily.
I could create a PR for master, and leave this as-is for lower branches; the only thing that remains then is how we're going to create a phpt file for this. This would need a named pipe (or are there other ways to reproduce this too?) and I'm not sure how to set that up into CI...
@kamil-tekiela If you have any thoughts on this, I'd be happy to read them 🙂
kamil-tekiela commentedon Mar 21, 2024
@nielsdos If you can fix it on master and test locally that it resolves the problem then I think that would be sufficient for now. We can keep this issue open and think how to add a proper test and fix it on lower branches.
1 remaining item
Fix crash in shutdown when using a named pipe with a persistent mysql…
Fix crash in shutdown when using a named pipe with a persistent mysql…
Remove broken hack in mysqlnd_vio::close_stream
Fix memory leak in mysqlnd_vio::open_pipe
Factor common code out into mysqlnd_fixup_regular_list()
Fix crash in shutdown when using a named pipe with a persistent mysql…
Partially backport phpGH-13782 to stable branches
Partially backport GH-13782 to stable branches