ext/pdo: Convert def_stmt_ctor_args field to Hashtable*

[Girgias]

Take 2 on #9803 now that the FCI/FCC thing has been converted separately.

[dstogov]

Why do you need to duplicate the array? Previously we just incremented reference counter.

[Girgias]

Looks like I was double freeing the HashTable, once manually in the free handler, and another time indirectly via the GC. Let me know if the new code still makes sense.

[dstogov]

See the question about zend_array_dup(). I don't see other problems.

[SakiTakamachi]

[IMHO]

It might be better to free dbh->def_stmt_ctor_args here as well.

Code like the following will cause memory leaks because dbh_get_gc is not called:

<?php
$db = new PDO('mysql:host=mysql;dbname=test;', 'root', ''); // my env
class Foo extends PDOStatement {}
$db->setAttribute(PDO::ATTR_STATEMENT_CLASS, array('Foo', ['param1']));

gdb:

(gdb) b dbh_get_gc
Breakpoint 1 at 0x364ad9: file /php-src/ext/pdo/pdo_dbh.c, line 1312.
(gdb) run /var/www/html/test_12154.php
Starting program: /php-src/sapi/cli/php /var/www/html/test_12154.php
[Mon Sep 11 15:25:29 2023]  Script:  '/var/www/html/test_12154.php'
/php-src/Zend/zend_hash.c(291) :  Freeing 0x00007ffff7a02600 (56 bytes), script=/var/www/html/test_12154.php
[Mon Sep 11 15:25:29 2023]  Script:  '/var/www/html/test_12154.php'
/php-src/Zend/zend_string.h(176) :  Freeing 0x00007ffff7a045c0 (32 bytes), script=/var/www/html/test_12154.php
[Mon Sep 11 15:25:29 2023]  Script:  '/var/www/html/test_12154.php'
/php-src/Zend/zend_hash.c(157) :  Freeing 0x00007ffff7a5e180 (136 bytes), script=/var/www/html/test_12154.php
=== Total 3 memory leaks detected ===
[Inferior 1 (process 4956) exited normally]

[Girgias]

Right, I was wondering about this. I'll add some tests for a GC and the non-GC case which made me think this (an SQLite bug)

[SakiTakamachi]

Isn't increasing the count here the cause of the memory leak?

Passing it to the constructor increments the ref count, so I thought maybe there was no need to increment the count at this point, so I commented it out and tried running the following code.
Memory leak no longer occurs.

<?php
$db = new PDO('mysql:host=mysql;dbname=test;', 'root', ''); // my env
class Foo extends PDOStatement {
    private function __construct($v) {
        var_dump($v);
    }
}
$db->setAttribute(PDO::ATTR_STATEMENT_CLASS, array('Foo', ['param1']));

$stmt = $db->query('SELECT 1');
var_dump($stmt);

result:

string(6) "param1"
object(Foo)#2 (1) {
  ["queryString"]=>
  string(8) "SELECT 1"
}

---

I also tried this as well.
This does not leak memory to begin with, but I tried it to confirm behavior.

<?php
class Foo extends PDOStatement {
    private function __construct($v) {
        var_dump($v);
    }
}

class Bar extends PDO {
    public $statementClass = 'Foo';
    function __construct($dsn, $username, $password, $driver_options = []) {
        $driver_options[PDO::ATTR_ERRMODE] = PDO::ERRMODE_EXCEPTION;
        parent::__construct($dsn, $username, $password, $driver_options);

        $this->setAttribute(PDO::ATTR_STATEMENT_CLASS, [$this->statementClass, [$this]]);
    }
}

$db = new Bar('mysql:host=mysql;dbname=test;', 'root', ''); // my env

$stmt = $db->query('SELECT 1');
var_dump($stmt);

result:

object(Bar)#1 (1) {
  ["statementClass"]=>
  string(3) "Foo"
}
object(Foo)#2 (1) {
  ["queryString"]=>
  string(8) "SELECT 1"
}

[Girgias]

I don't think not incrementing the reference counter is the correct approach.

As something along the lines of:

<?php
if (getenv('REDIR_TEST_DIR') === false) putenv('REDIR_TEST_DIR='.__DIR__ . '/../../pdo/tests/');
require_once getenv('REDIR_TEST_DIR') . 'pdo_test.inc';

class Foo extends PDOStatement {
    private function __construct($v) {
        var_dump($v);
    }
}

$db = PDOTest::factory();

$a = ['Foo', ['param1']];
$db->setAttribute(PDO::ATTR_STATEMENT_CLASS, $a);
unset($a);
$stmt = $db->query('SELECT 1');
var_dump($stmt);

?>

Should still work, I'm pretty sure this is why I did a dup.

[Girgias]

Also just removing this line causes use after frees so just that is definitely not the solution.

[SakiTakamachi]

@Girgias

Passing it to the constructor increments the ref count, so I thought maybe there was no need to increment the count at this point

This was clearly a mistake.
However, after looking around, I still don't think I should increment the refcount here, for other reasons.

The test code you provided that includes unset worked fine even after removing GC_TRY_ADDREF.

dbh->def_stmt_ctor_args = Z_ARRVAL_P(item); is a copy, so whatever happens to $a after this should have no effect at all (of course, there is no problem using unset()).

Sorry, the above is also incorrect.

---

In the following case, at the time of setAttribute, refcount is 2, and unset() reduces it to 1.
(with removing GC_TRY_ADDREF. I checked by gdb.)

<?php
$db = new PDO('mysql:host=mysql;dbname=test;', 'root', '');
class Foo extends PDOStatement {
    private function __construct($v) {
        var_dump($v);
    }
}
$a = ['Foo', ['param1']];
$db->setAttribute(PDO::ATTR_STATEMENT_CLASS, $a); // refcount 2
unset($a[1]);

$stmt = $db->query('SELECT 1'); // refcount 1
var_dump($stmt);

[SakiTakamachi]

If I remove GC_TRY_ADDREF, ht will be freed by the next process and then used again.

if (dbh->def_stmt_ctor_args) {
	zend_array_release(dbh->def_stmt_ctor_args);
	dbh->def_stmt_ctor_args = NULL;
}

// If I try to set the exact same thing twice, a problem will occur.
$db->setAttribute(PDO::ATTR_STATEMENT_CLASS, ['Foo', ['param1']]);
$db->setAttribute(PDO::ATTR_STATEMENT_CLASS, ['Foo', ['param1']]);

So in this case, it definitely need an GC_TRY_ADDREF here, I understood it.

[arnaud-lb]

The GC avoids this issue by setting GC_TYPE_INFO() to IS_NULL here and here so that zval_ptr_dtor() is a no-op on that value when it's already being destroyed.

Checking GC_TYPE(dbh->def_stmt_ctor_args) before calling zend_array_release(dbh->def_stmt_ctor_args) would fix the issue:

	if (dbh->def_stmt_ctor_args && GC_TYPE(dbh->def_stmt_ctor_args) == IS_ARRAY) {
		zend_array_release(dbh->def_stmt_ctor_args);

Alternatively we could use GC_DTOR(), which is equivalent to zval_ptr_dtor for refcounted values, but it may unnecessarily call gc_check_possible_root. We could introduce a new macro GC_DTOR_NOGC that does not call gc_check_possible_root, like zval_ptr_dtor_nogc.