Fix pgsql use after free trying to reuse closed connection

nikic · nikic · commit c7a86a38a3f6 · 2019-04-10T12:18:57.000+02:00
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
@@ -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
@@ -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
@@ -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
@@ -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)

Original file line number	Diff line number	Diff line change
`@@ -61,7 +61,7 @@ static void _zend_is_inconsistent(const HashTable ht, const char file, int lin`
`61`	`61`	`zend_output_debug_string(1, "%s(%d) : ht=%p is inconsistent", file, line, ht);`
`62`	`62`	`break;`
`63`	`63`	`}`
`64`		`- zend_bailout();`
	`64`	`+ ZEND_ASSERT(0);`
`65`	`65`	`}`
`66`	`66`	`#define IS_CONSISTENT(a) _zend_is_inconsistent(a, __FILE__, __LINE__);`
`67`	`67`	`#define SET_INCONSISTENT(n) do { \`