From be9f0e19af5cfae9f5e1b4529156d1eb9f72c676 Mon Sep 17 00:00:00 2001 From: Gina Peter Banyard Date: Tue, 21 Jan 2025 15:36:47 +0000 Subject: [PATCH 1/2] ext/pdo: Fix memory leak if GC needs to free PDO Statement --- ext/pdo/pdo_stmt.c | 7 +- ext/pdo/tests/pdo_stmt_cyclic_references.phpt | 132 ++++++++++++++++++ 2 files changed, 137 insertions(+), 2 deletions(-) create mode 100644 ext/pdo/tests/pdo_stmt_cyclic_references.phpt diff --git a/ext/pdo/pdo_stmt.c b/ext/pdo/pdo_stmt.c index a7d898221f881..d17a40dbc242e 100644 --- a/ext/pdo/pdo_stmt.c +++ b/ext/pdo/pdo_stmt.c @@ -2079,8 +2079,11 @@ static zend_function *dbstmt_method_get(zend_object **object_pp, zend_string *me static HashTable *dbstmt_get_gc(zend_object *object, zval **gc_data, int *gc_count) { pdo_stmt_t *stmt = php_pdo_stmt_fetch_object(object); - *gc_data = &stmt->fetch.into; - *gc_count = 1; + + zend_get_gc_buffer *gc_buffer = zend_get_gc_buffer_create(); + zend_get_gc_buffer_add_zval(gc_buffer, &stmt->database_object_handle); + zend_get_gc_buffer_add_zval(gc_buffer, &stmt->fetch.into); + zend_get_gc_buffer_use(gc_buffer, gc_data, gc_count); /** * If there are no dynamic properties and the default property is 1 (that is, there is only one property diff --git a/ext/pdo/tests/pdo_stmt_cyclic_references.phpt b/ext/pdo/tests/pdo_stmt_cyclic_references.phpt new file mode 100644 index 0000000000000..01df6e0b46d60 --- /dev/null +++ b/ext/pdo/tests/pdo_stmt_cyclic_references.phpt @@ -0,0 +1,132 @@ +--TEST-- +PDO Common: Cyclic PDOStatement child class +--EXTENSIONS-- +pdo +--SKIPIF-- + +--FILE-- +exec('CREATE TABLE test(id INT NOT NULL PRIMARY KEY, val VARCHAR(10), val2 VARCHAR(10))'); +$db->exec("INSERT INTO test VALUES(1, 'A', 'AA')"); +$db->exec("INSERT INTO test VALUES(2, 'B', 'BB')"); +$db->exec("INSERT INTO test VALUES(3, 'C', 'CC')"); + +$db->setAttribute(PDO::ATTR_STATEMENT_CLASS, ['CyclicStatement', [new Ref]]); + +echo "Column fetch:\n"; +$stmt = $db->query('SELECT id, val2, val FROM test'); +$stmt->ref->stmt = $stmt; +$stmt->setFetchMode(PDO::FETCH_COLUMN, 2); +foreach($stmt as $obj) { + var_dump($obj); +} + +echo "Class fetch:\n"; +$stmt = $db->query('SELECT id, val2, val FROM test'); +$stmt->ref->stmt = $stmt; +$stmt->setFetchMode(PDO::FETCH_CLASS, 'TestRow', ['Hello world']); +foreach($stmt as $obj) { + var_dump($obj); +} + +echo "Fetch into:\n"; +$stmt = $db->query('SELECT id, val2, val FROM test'); +$stmt->ref->stmt = $stmt; +$stmt->setFetchMode(PDO::FETCH_INTO, new TestRow('I am being fetch into')); +foreach($stmt as $obj) { + var_dump($obj); +} + +?> +--EXPECTF-- +Column fetch: +string(1) "A" +string(1) "B" +string(1) "C" +Class fetch: +object(TestRow)#%d (4) { + ["id"]=> + string(1) "1" + ["val"]=> + string(1) "A" + ["val2"]=> + string(2) "AA" + ["arg"]=> + string(11) "Hello world" +} +object(TestRow)#%d (4) { + ["id"]=> + string(1) "2" + ["val"]=> + string(1) "B" + ["val2"]=> + string(2) "BB" + ["arg"]=> + string(11) "Hello world" +} +object(TestRow)#%d (4) { + ["id"]=> + string(1) "3" + ["val"]=> + string(1) "C" + ["val2"]=> + string(2) "CC" + ["arg"]=> + string(11) "Hello world" +} +Fetch into: +object(TestRow)#4 (4) { + ["id"]=> + string(1) "1" + ["val"]=> + string(1) "A" + ["val2"]=> + string(2) "AA" + ["arg"]=> + string(21) "I am being fetch into" +} +object(TestRow)#4 (4) { + ["id"]=> + string(1) "2" + ["val"]=> + string(1) "B" + ["val2"]=> + string(2) "BB" + ["arg"]=> + string(21) "I am being fetch into" +} +object(TestRow)#4 (4) { + ["id"]=> + string(1) "3" + ["val"]=> + string(1) "C" + ["val2"]=> + string(2) "CC" + ["arg"]=> + string(21) "I am being fetch into" +} From 68fcd4795ab674bfaf8b4619a4d3a97171998709 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Fri, 24 Jan 2025 20:19:42 +0100 Subject: [PATCH 2/2] Fix crash in firebird statement dtor If both the driver object and statement end up in the GC buffer and are freed by the GC, then the destruction order is not deterministic and it is possible that the driver object is freed before the statement. In that case, accessing S->H will cause a UAF. As the resources are already released we simply skip the destruction if the driver object is already destroyed. --- ext/pdo_firebird/firebird_statement.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/ext/pdo_firebird/firebird_statement.c b/ext/pdo_firebird/firebird_statement.c index 1fe894cd631df..7f4e7aeed87c0 100644 --- a/ext/pdo_firebird/firebird_statement.c +++ b/ext/pdo_firebird/firebird_statement.c @@ -87,8 +87,15 @@ static int firebird_stmt_dtor(pdo_stmt_t *stmt) /* {{{ */ pdo_firebird_stmt *S = (pdo_firebird_stmt*)stmt->driver_data; int result = 1; - /* release the statement */ - if (isc_dsql_free_statement(S->H->isc_status, &S->stmt, DSQL_drop)) { + /* TODO: for master, move this check to a separate function shared between pdo drivers. + * pdo_pgsql and pdo_mysql do this exact same thing */ + bool server_obj_usable = !Z_ISUNDEF(stmt->database_object_handle) + && IS_OBJ_VALID(EG(objects_store).object_buckets[Z_OBJ_HANDLE(stmt->database_object_handle)]) + && !(OBJ_FLAGS(Z_OBJ(stmt->database_object_handle)) & IS_OBJ_FREE_CALLED); + + /* release the statement. + * Note: if the server object is already gone then the statement was closed already as well. */ + if (server_obj_usable && isc_dsql_free_statement(S->H->isc_status, &S->stmt, DSQL_drop)) { RECORD_ERROR(stmt); result = 0; }