From c7a86a38a3f657ab81163ac794450fc7ada2ba3c Mon Sep 17 00:00:00 2001 From: Nikita Popov Date: Wed, 10 Apr 2019 12:18:57 +0200 Subject: Fix pgsql use after free trying to reuse closed connection When a connection is closed, we also need to remove the hash entry from the regular_list, as it now points to freed memory. To do this store a reverse mapping from the connection to the hash string. It would be nicer to introduce a wrapping structure for the pgsql link resource that could store the hash (and notices), but that would require large changes to the extension, so I'm going for a more minimal fix here. --- ext/pgsql/pgsql.c | 34 ++++++++++++++++++++++++-------- ext/pgsql/php_pgsql.h | 1 + ext/pgsql/tests/connect_after_close.phpt | 19 ++++++++++++++++++ 3 files changed, 46 insertions(+), 8 deletions(-) create mode 100644 ext/pgsql/tests/connect_after_close.phpt (limited to 'ext/pgsql') diff --git a/ext/pgsql/pgsql.c b/ext/pgsql/pgsql.c index 658b03baaf..c97c600b66 100644 --- a/ext/pgsql/pgsql.c +++ b/ext/pgsql/pgsql.c @@ -939,12 +939,20 @@ static void _close_pgsql_link(zend_resource *rsrc) { PGconn *link = (PGconn *)rsrc->ptr; PGresult *res; + zval *hash; while ((res = PQgetResult(link))) { PQclear(res); } PQfinish(link); PGG(num_links)--; + + /* Remove connection hash for this link */ + hash = zend_hash_index_find(&PGG(hashes), (uintptr_t) link); + if (hash) { + zend_hash_index_del(&PGG(hashes), (uintptr_t) link); + zend_hash_del(&EG(regular_list), Z_STR_P(hash)); + } } /* }}} */ @@ -1099,6 +1107,7 @@ static PHP_GINIT_FUNCTION(pgsql) memset(pgsql_globals, 0, sizeof(zend_pgsql_globals)); /* Initilize notice message hash at MINIT only */ zend_hash_init_ex(&pgsql_globals->notices, 0, NULL, ZVAL_PTR_DTOR, 1, 0); + zend_hash_init_ex(&pgsql_globals->hashes, 0, NULL, ZVAL_PTR_DTOR, 1, 0); } /* }}} */ @@ -1215,6 +1224,7 @@ PHP_MSHUTDOWN_FUNCTION(pgsql) { UNREGISTER_INI_ENTRIES(); zend_hash_destroy(&PGG(notices)); + zend_hash_destroy(&PGG(hashes)); return SUCCESS; } @@ -1236,6 +1246,7 @@ PHP_RSHUTDOWN_FUNCTION(pgsql) { /* clean up notice messages */ zend_hash_clean(&PGG(notices)); + zend_hash_clean(&PGG(hashes)); /* clean up persistent connection */ zend_hash_apply(&EG(persistent_list), (apply_func_t) _rollback_transactions); return SUCCESS; @@ -1431,14 +1442,11 @@ static void php_pgsql_do_connect(INTERNAL_FUNCTION_PARAMETERS, int persistent) } link = (zend_resource *)index_ptr->ptr; - if (link->ptr && (link->type == le_link || link->type == le_plink)) { - php_pgsql_set_default_link(link); - GC_REFCOUNT(link)++; - RETVAL_RES(link); - goto cleanup; - } else { - zend_hash_del(&EG(regular_list), str.s); - } + ZEND_ASSERT(link->ptr && (link->type == le_link || link->type == le_plink)); + php_pgsql_set_default_link(link); + GC_REFCOUNT(link)++; + RETVAL_RES(link); + goto cleanup; } 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)); @@ -1484,6 +1492,16 @@ static void php_pgsql_do_connect(INTERNAL_FUNCTION_PARAMETERS, int persistent) if (zend_hash_update_mem(&EG(regular_list), str.s, (void *) &new_index_ptr, sizeof(zend_resource)) == NULL) { goto err; } + + /* Keep track of link => hash mapping, so we can remove the hash entry from regular_list + * when the connection is closed. This uses the address of the connection rather than the + * zend_resource, because the resource destructor is passed a stack copy of the resource + * structure. */ + { + zval tmp; + ZVAL_STR_COPY(&tmp, str.s); + zend_hash_index_update(&PGG(hashes), (uintptr_t) pgsql, &tmp); + } PGG(num_links)++; } /* set notice processor */ diff --git a/ext/pgsql/php_pgsql.h b/ext/pgsql/php_pgsql.h index 98b396c9f1..45807ea4eb 100644 --- a/ext/pgsql/php_pgsql.h +++ b/ext/pgsql/php_pgsql.h @@ -319,6 +319,7 @@ ZEND_BEGIN_MODULE_GLOBALS(pgsql) int ignore_notices,log_notices; HashTable notices; /* notice message for each connection */ zend_resource *default_link; /* default link when connection is omitted */ + HashTable hashes; /* hashes for each connection */ ZEND_END_MODULE_GLOBALS(pgsql) ZEND_EXTERN_MODULE_GLOBALS(pgsql) diff --git a/ext/pgsql/tests/connect_after_close.phpt b/ext/pgsql/tests/connect_after_close.phpt new file mode 100644 index 0000000000..65f954570b --- /dev/null +++ b/ext/pgsql/tests/connect_after_close.phpt @@ -0,0 +1,19 @@ +--TEST-- +Reopen connection after it was closed +--SKIPIF-- + +--FILE-- + +--EXPECT-- +bool(true) +bool(true) -- cgit v1.2.1