Apache crashes on shutdown when using pg_pconnect() #12974
Comments
|
would it be possible to test with a 8.2.x release and sees if this still crashes (or even a different backtrace) ? |
I managed to reproduce the bug with PHP 8.2.31. |
|
So just to confirm, the crashes occurs only with persistent connection (when the extension tries to register the connection cleanup callback) ? |
yes, with pgsql.allow_persistent=0 in php.ini the crash doesn't occur. |
devnexen
added
Extension: pgsql
SAPI: apache2handler
OS: Windows
and removed
Status: Needs Triage
labels
|
Is there any other extension involved loaded eventually ? If so if you disable them do you still see the crashes ? |
I tried disabling every extension except pg_pgsql.dll, but the crash still occurs. |
|
"unfortunately", cannot reproduce under Linux. |
|
By any chance @nielsdos, do you have any idea what s going on with apache ? |
|
I can reproduce this. Script: <?php
echo "hi";
$conn = pg_pconnect("dbname=php");
var_dump($conn);Ran Apache on Linux using: Surfed to the page 5 times in my browser (don't know if the amount makes a difference). Got this: I don't know if I'll still have time to look today; otherwise tomorrow. |
|
The problem is the following: Under ZTS, upon shutdown, the pgsql module gets destroyed and MSHUTDOWN etc get called in Lines 3172 to 3183 in 5dfb2d9 Then later on, the resources get destroyed, and we end up in Lines 318 to 319 in 5dfb2d9 Solution is to either:
diff --git a/ext/pgsql/pgsql.c b/ext/pgsql/pgsql.c
index f3c4471837..3febfe66b1 100644
--- a/ext/pgsql/pgsql.c
+++ b/ext/pgsql/pgsql.c
@@ -314,8 +314,10 @@ static void _close_pgsql_plink(zend_resource *rsrc)
PQclear(res);
}
PQfinish(link);
- PGG(num_persistent)--;
- PGG(num_links)--;
+ if (EG(active)) {
+ PGG(num_persistent)--;
+ PGG(num_links)--;
+ }
rsrc->ptr = NULL;
}
|
|
Cool thanks, could odbc be affected in the same way in theory ? static void _close_odbc_pconn(zend_resource *rsrc)
{
zend_resource *p;
odbc_result *res;
odbc_connection *conn = (odbc_connection *)rsrc->ptr;
ZEND_HASH_FOREACH_PTR(&EG(regular_list), p) {
if (p->ptr && (p->type == le_result)) {
res = (odbc_result *)p->ptr;
if (res->conn_ptr == conn) {
zend_list_close(p);
}
}
} ZEND_HASH_FOREACH_END();
/* If aborted via timer expiration, don't try to call any unixODBC function */
if (!(PG(connection_status) & PHP_CONNECTION_TIMEOUT)) {
safe_odbc_disconnect(conn->hdbc);
SQLFreeConnect(conn->hdbc);
SQLFreeEnv(conn->henv);
}
free(conn);
ODBCG(num_links)--;
ODBCG(num_persistent)--;
} |
Looks like it yes |
I may tend towards the former solution but I m not 100% sure myself, maybe the next day I may have a fresher perspective unless someone else have a suggestion. |
|
Actually instead of I also checked the "Somehow don't change globals in that function" option: the problem is that we don't have an API to access the counts from the resource list, which is the only way right now I see how that could work. Anyway, I'll also give it a night rest. |
I like it better.
Good to know ! |
|
@devnexen I've been looking at this on and off. On master, we can maybe change the TSRM destruction logic to work in two steps: first do all the module destruction, and only later do the freeing. This would avoid the issue at the TSRM side. However, this may have side-effects that I can't predict and therefore certainly can't be done on stable branches. Therefore, I think for now we should go with the |
I would say yes. |
On ZTS, the global variables are stored in dynamically allocated memory. When the module gets shut down this memory is released. After the module is shut down, only then are the persistent resources cleared. Normally this isn't an issue, but pgsql and odbc refer to the globals to modify some counters, after the globals have been freed. Fix this by guarding the modification.
* PHP-8.2: Fix GH-12974: Apache crashes on shutdown when using pg_pconnect()
* PHP-8.3: Fix GH-12974: Apache crashes on shutdown when using pg_pconnect()
|
I am on PHP 8.2.15. The bug still reproduces for me unfortunately and backtrace information is the same😢 |
|
I see. I think this would solve it: https://gist.github.com/nielsdos/0dbee63d474554820eb856593b34c496 @devnexen Can you please check if this makes sense and think of possible problems? EDIT: ugh... that won't work for the static case because it'll try to destroy the hash table of resources twice. I need to think a bit more. |
|
Your fix makes sense, asan should not complain I think here ; hopefully will fix windows' case. |
|
I dug deeper into the rabbit hole. I think all persistent resources on ZTS are affected. On shutdown the following happens: However, there may still be persistent resources loaded, which will be freed by Line 1160 in c5fbcfa which will call Lines 833 to 835 in c5fbcfa Here we see that there can still be persistent resources destroyed after either the module globals have been destroyed (which was the original issue I found), or after the module is unloaded (the remaining issue). I thought of this fix: https://gist.github.com/nielsdos/e21360b9c100d23c1ae40af3ef998ee6 but I'm not sure if it's right. In particular, I'm afraid of this because we have bad ZTS test coverage and no Apache test coverage at all. |
|
Scratch that, my proposed patch won't work of course because we still need to destroy all persistent lists of all threads via the ZTS dtor, which I removed. |
|
Another idea that keeps BC would be to keep the current |
|
Here's that idea in patch form: https://gist.github.com/nielsdos/338774e915a1b528eaa8bd68ba7cc54c |
On shutdown in ZTS the following happens: - https://github.com/php/php-src/blob/master/Zend/zend.c#L1124-L1125 gets executed. This destroys global persistent resources and destroys the modules. Furthermore, the modules are unloaded too. - Further down, `ts_free_id(executor_globals_id)` gets executed, which calls `executor_globals_dtor`. This function destroys persistent resources for each thread. Notice that in the last step, the modules that the persistent resource belong to may already have been destroyed. This means that accessing globals will cause a crash (I previously fixed this with ifdef magic), or when the module is dynamically loaded we'll try jumping to a destructor that is no longer loaded in memory. These scenarios cause crashes. It's not possible to move the `ts_free_id` call upwards, because that may break assumptions of callers, and furthermore this would deallocate the executor globals structure, which means that any access to those will cause a segfault. This patch adds a new API to the TSRM that allows running a callback on a certain resource type. We use this API to destroy the persistent resources in all threads prior to the module destruction, and keep the rest of the resource dtor intact. I verified this fix on Apache with postgres, both dynamically and statically. Fixes phpGH-12974.
On shutdown in ZTS the following happens: - https://github.com/php/php-src/blob/master/Zend/zend.c#L1124-L1125 gets executed. This destroys global persistent resources and destroys the modules. Furthermore, the modules are unloaded too. - Further down, `ts_free_id(executor_globals_id)` gets executed, which calls `executor_globals_dtor`. This function destroys persistent resources for each thread. Notice that in the last step, the modules that the persistent resource belong to may already have been destroyed. This means that accessing globals will cause a crash (I previously fixed this with ifdef magic), or when the module is dynamically loaded we'll try jumping to a destructor that is no longer loaded in memory. These scenarios cause crashes. It's not possible to move the `ts_free_id` call upwards, because that may break assumptions of callers, and furthermore this would deallocate the executor globals structure, which means that any access to those will cause a segfault. This patch adds a new API to the TSRM that allows running a callback on a certain resource type. We use this API to destroy the persistent resources in all threads prior to the module destruction, and keep the rest of the resource dtor intact. I verified this fix on Apache with postgres, both dynamically and statically. Fixes GH-12974.
Description
Problem Scenario:
Start Apache (version 2.4.58).
Execute any PHP script that uses pg_pconnect function to establish a PostgreSQL persistent database connection (host= port= dbname= user=).
Stop Apache.
Observed Behavior:
After stopping Apache, the dump file is generated.
The dump file includes backtrace information:
Expected Outcome:
The script should execute without issues.
PHP Version
8.1.25
Operating System
Windows 11
The text was updated successfully, but these errors were encountered: