Repository navigation
Fix mysqlnd pipe crash - #13782
Fix mysqlnd pipe crash#13782ndossche wants to merge 6 commits into
Conversation
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 in close_stream gets confused between persistent vs non-persistent allocations when EG(active) is false. 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 was fixed on the master branch in 5941cda. This fixes the crash reason of phpGH-10599.
This fixes the memory leak part of phpGH-10599.
This prevents the code from getting desynced again, which was the reason for the leak of phpGH-10599.
…i connection The code originally posted in phpGH-10599 triggers the bug for non-persistent connections, but changing the host to `p:.` reveals that there is also a crash bug for persistent connections.
c8fb571 to
b30796f
Compare
SakiTakamachi
left a comment
There was a problem hiding this comment.
I've looked over the code three times and it looks good to me.
I'll wait for @kamil-tekiela 's review.
|
I recuse myself from reviewing this because I have no idea what's going on here. But I have no objections to this. If you believe this is the right fix, go ahead and merge it. Maybe some other senior developer can take a quick look at it too if you want someone else's review. |
|
Okay, thanks for trying to check anyway, I'll ask somebody for another review. |
|
This is very ugly. I don't mean your particular change but already the existing way of doing this. I will try to think if we could come up with something cleaner (at least for master). Need to get a bit better understanding of the whole problem first. |
|
I'll leave the review up to Jakub then. @bukka Ping me if you don't want to look at this after all. |
There was a problem hiding this comment.
So I looked properly into this and think this is fine as a fix. It's just extending the current hacky way how to make stream not to register (meaning delete them immediately after registration) the resource...
I have been thinking about a proper solution and I think this should be addressed in the stream layer by introuction of the new stream open option. This is however not such a small task because creation of the stream is handled by each wrapper independently (in stream_opener) so it will require updating wrappers (we probably just need it in plain, xp and ssl for now) and introducing various new extended functions that will propagate this option all the way to _php_stream_alloc which can skip the resource registration. Created stream should be then aware so there won't be a need to use further hacks like passing PHP_STREAM_FREE_RSRC_DTOR which is completely misused here (it is just used to skip removing item from the list but that is originally there because we wont to do the removal in shutdown but that won't happen here because it's not in the list - so hacky... :) ). This is something that can happen in master only though.
Yeah, I had also tried looking into this but quickly gave up on this 😅 |
This partially backports that PR to stable branches as it has been in master without reported problems so far. It's only a partial backport because the stable branches don't have the ZTS persistent resource fix that would fix shutdown crashes, i.e. the code change in mysqlnd_vio's close_stream is not backported. Closes phpGH-10599.
This partially backports that PR to stable branches as it has been in master without reported problems so far. It's only a partial backport because the stable branches don't have the ZTS persistent resource fix that would fix shutdown crashes, i.e. the code change in mysqlnd_vio's close_stream is not backported. This is fully fixed on master. Closes GH-14324. Closes GH-10599.
* PHP-8.2: Partially backport GH-13782 to stable branches
* PHP-8.3: Partially backport GH-13782 to stable branches
The individual commits have descriptions that explain the change. This fixes the bug fully on the master branch.
I tested this with both persistent and non-persistent connections. As mysqlnd's stream handling is a bit tricky and has some hacks, it's best to review this with at least 2 people.
As for backporting: All commits except the first one can be backported to stable branches I think, which means that in that case it's fixed on stable branches that use NTS. On ZTS, I'm not entirely sure if there's still cases where it will crash. Since the resources are removed from the persistent list, and managed by the extension itself now, it may work around the crash that the old code was protecting against.
Should fix GH-10599.
To test with ASAN on Windows (to reproduce the crash), you need to modify
configure.js:Find
ADD_FLAG('CFLAGS', ' /wd4996 ');and change it intoADD_FLAG('CFLAGS', ' /wd4996 /fsanitize=address ');.