From 99f8ec33d9713e43c27681c37a7618db10b7c066 Mon Sep 17 00:00:00 2001 From: Gina Peter Banyard Date: Tue, 21 Jan 2025 15:36:47 +0000 Subject: [PATCH 1/3] 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 a7d898221f8..d17a40dbc24 100644 --- a/ext/pdo/pdo_stmt.c +++ b/ext/pdo/pdo_stmt.c @@ -2079,8 +2079,11 @@ out: 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 00000000000..01df6e0b46d --- /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 2ae897fff7af3a794a31a8aeeeeb4f21f6a41393 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/3] 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 1fe894cd631..7f4e7aeed87 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; } From e6d917e4c9822c221cac9ed1c3c22aace2387f8a Mon Sep 17 00:00:00 2001 From: Gina Peter Banyard Date: Fri, 24 Jan 2025 20:14:23 +0000 Subject: [PATCH 3/3] Add NEWS entries Closes GH-17539 --- NEWS | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/NEWS b/NEWS index 1084d620deb..c4fd35de806 100644 --- a/NEWS +++ b/NEWS @@ -42,6 +42,10 @@ PHP NEWS - Opcache: . Fixed bug GH-17307 (Internal closure causes JIT failure). (nielsdos) +- PDO: + . Fixed a memory leak when the GC is used to free a PDOStatment. (Girgias) + . Fixed a crash in the PDO Firebird Statement destructor. (nielsdos) + - Phar: . Fixed bug GH-17518 (offset overflow phar extractTo()). (nielsdos)