Skip to content

Commit

Permalink
Fix GH-13519: PGSQL_CONNECT_FORCE_RENEW with persistent connections.
Browse files Browse the repository at this point in the history 
persistent connections did not take in account this flag, after the
usual link sanity checks, we remove its entry.

Close GH-13519
  • Loading branch information
@devnexen
devnexen committed Feb 27, 2024
1 parent b8a1041 commit b9a9790
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 0 deletions.
2 changes: 2 additions & 0 deletions NEWS
View file
Expand Up @@ -31,6 +31,8 @@ PHP NEWS
- PGSQL:
. Fixed bug GH-13354 (pg_execute/pg_send_query_params/pg_send_execute
with null value passed by reference). (George Barbarosie)
. Fixed bug GH-13519 (PGSQL_CONNECT_FORCE_RENEW not working with persistent
connections. (David Carlier)

- Standard:
. Fixed array key as hash to string (case insensitive) comparison typo
Expand Down
7 changes: 7 additions & 0 deletions ext/pgsql/pgsql.c
View file
Expand Up @@ -562,6 +562,7 @@ static void php_pgsql_do_connect(INTERNAL_FUNCTION_PARAMETERS, int persistent)

/* try to find if we already have this link in our persistent list */
if ((le = zend_hash_find_ptr(&EG(persistent_list), str.s)) == NULL) { /* we don't */
newpconn:
if (PGG(max_links) != -1 && PGG(num_links) >= PGG(max_links)) {
php_error_docref(NULL, E_WARNING,
"Cannot create new link. Too many open links (" ZEND_LONG_FMT ")", PGG(num_links));
Expand Down Expand Up @@ -590,6 +591,12 @@ static void php_pgsql_do_connect(INTERNAL_FUNCTION_PARAMETERS, int persistent)
PGG(num_links)++;
PGG(num_persistent)++;
} else { /* we do */
if ((connect_type & PGSQL_CONNECT_FORCE_NEW)) {
if (zend_hash_del(&EG(persistent_list), str.s) != SUCCESS) {

This comment has been minimized.

Sign in to view

Copy link
@devnexen

devnexen Feb 27, 2024 •
edited

Author Member

@nielsdos just asking your opinion since you know well this area. Is there a need to do more steps or using a different approach when removing persistent resources, seems then when the module shutdown it segfaults with non ZTS build/release. Thanks.

This comment has been minimized.

Sign in to view

Copy link
@nielsdos

nielsdos Feb 27, 2024

Member

Intuitively, I expect that removing it from the persistent list means the resource gets destroyed, and thus causing a UAF.

This comment has been minimized.

Sign in to view

Copy link
@nielsdos

nielsdos Feb 27, 2024

Member

That is it indeed:

==198501== Invalid read of size 4
==198501==    at 0x48AC2BE: UnknownInlinedFun (fe-misc.c:80)
==198501==    by 0x48AC2BE: pqParseInput3 (fe-protocol3.c:77)
==198501==    by 0x48AD83A: UnknownInlinedFun (fe-exec.c:1995)
==198501==    by 0x48AD83A: PQgetResult (fe-exec.c:2043)
==198501==    by 0x55194F: pgsql_link_free (pgsql.c:177)
==198501==    by 0x551A55: pgsql_link_free_obj (pgsql.c:203)
==198501==    by 0x93141B: zend_objects_store_del (zend_objects_API.c:200)
==198501==    by 0x829F65: rc_dtor_func (zend_variables.c:57)
==198501==    by 0x862BC1: zend_assign_to_variable (zend_execute.h:173)
==198501==    by 0x8E5D07: ZEND_ASSIGN_SPEC_CV_VAR_RETVAL_UNUSED_HANDLER (zend_vm_execute.h:48175)
==198501==    by 0x8F6614: execute_ex (zend_vm_execute.h:62216)
==198501==    by 0x8F6D31: zend_execute (zend_vm_execute.h:62752)
==198501==    by 0x82EF95: zend_execute_script (zend.c:1888)
==198501==    by 0x77C513: php_execute_script_ex (main.c:2507)
==198501==  Address 0x833668c is 908 bytes inside a block of size 1,192 free'd
==198501==    at 0x48468CF: free (vg_replace_malloc.c:985)
==198501==    by 0x551D80: _close_pgsql_plink (pgsql.c:317)
==198501==    by 0x850C77: plist_entry_destructor (zend_list.c:196)
==198501==    by 0x849FA9: _zend_hash_del_el_ex (zend_hash.c:1482)
==198501==    by 0x84A397: zend_hash_del (zend_hash.c:1552)
==198501==    by 0x552D4E: php_pgsql_do_connect (pgsql.c:593)
==198501==    by 0x553412: zif_pg_pconnect (pgsql.c:742)
==198501==    by 0x8748E5: ZEND_DO_ICALL_SPEC_RETVAL_USED_HANDLER (zend_vm_execute.h:1349)
==198501==    by 0x8F235B: execute_ex (zend_vm_execute.h:57350)
==198501==    by 0x8F6D31: zend_execute (zend_vm_execute.h:62752)
==198501==    by 0x82EF95: zend_execute_script (zend.c:1888)
==198501==    by 0x77C513: php_execute_script_ex (main.c:2507)
goto err;
}
goto newpconn;
}
if (le->type != le_plink) {
goto err;
}
Expand Down
30 changes: 30 additions & 0 deletions ext/pgsql/tests/gh13519.phpt
View file
@@ -0,0 +1,30 @@
--TEST--
GH-13519 - PGSQL_CONNECT_FORCE_NEW with persistent connections.
--EXTENSIONS--
pgsql
--SKIPIF--
<?php include("skipif.inc"); ?>
--FILE--
<?php
include 'config.inc';

$db1 = pg_pconnect($conn_str);
$pid1 = pg_get_pid($db1);
for ($i = 0; $i < 3; $i ++) {
$db2 = pg_pconnect($conn_str);
var_dump($pid1 === pg_get_pid($db2));
}
for ($i = 0; $i < 3; $i ++) {
$db2 = pg_pconnect($conn_str, PGSQL_CONNECT_FORCE_NEW);
var_dump($pid1 === pg_get_pid($db2));
pg_close($db2);
}
pg_close($db1);
?>
--EXPECT--
bool(true)
bool(true)
bool(true)
bool(false)
bool(false)
bool(false)

1 comment on commit b9a9790

@bukka
Copy link
Member

@bukka bukka commented on b9a9790 Feb 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@saundefined This change needs to be reverted from 8.2.17

Please sign in to comment.