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.

diff --git a/Zend/zend_hash.c b/Zend/zend_hash.c
index 80938694e55025634add1490d67f04479b3688f1..f5fba679826c77b28e4a831df4432e878a08a71d 100644 (file)
--- a/Zend/zend_hash.c
+++ b/Zend/zend_hash.c
@@ -61,7 +61,7 @@ static void _zend_is_inconsistent(const HashTable *ht, const char *file, int lin
                        zend_output_debug_string(1, "%s(%d) : ht=%p is inconsistent", file, line, ht);
                        break;
        }
-       zend_bailout();
+       ZEND_ASSERT(0);
 }
 #define IS_CONSISTENT(a) _zend_is_inconsistent(a, __FILE__, __LINE__);
 #define SET_INCONSISTENT(n) do { \

diff --git a/ext/pgsql/pgsql.c b/ext/pgsql/pgsql.c
index 658b03baaf10e30e1f36ed2a2e1587c92ff050b6..c97c600b6616ac7bd672e7a10452fc8450cf510f 100644 (file)
--- 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 98b396c9f18ef52ad9202739c6b5e928b484afa7..45807ea4eb78b87c74d518dc1f58c3042b6b2d7b 100644 (file)
--- 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 (file)
index 0000000..65f9545
--- /dev/null
+++ b/ext/pgsql/tests/connect_after_close.phpt
@@ -0,0 +1,19 @@
+--TEST--
+Reopen connection after it was closed
+--SKIPIF--
+<?php include("skipif.inc"); ?>
+--FILE--
+<?php
+include('config.inc');
+
+/* Run me under valgrind */
+$db1 = pg_connect($conn_str);
+unset($db1);
+var_dump(pg_close());
+$db2 = pg_connect($conn_str);
+unset($db2);
+var_dump(pg_close());
+?>
+--EXPECT--
+bool(true)
+bool(true)