Fix GH-11878: SQLite3 callback functions cause a memory leak with a callable array

[nielsdos]

In this test file, the free_obj handler is called with a refcount of 2, caused by the fact we do a GC_ADDREF() to increase its refcount while its refcount is still 1 because the Foo object hasn't been destroyed yet (due to the cycle caused by the sqlite function callback). Solve this by introducing a get_gc handler.

[youkidearitai]

@nielsdos Thank you very much. I confirmed that displays nothing about memory leak.

$ ~/php_nielsdos/bin/php -d 'ini_set("memory_limit", -1);'  phd/render.php --docbook doc-base/.manual.xml --package PHP --format xhtml
PHP:  syntax error, unexpected '(' in Unknown on line 7
[15:51:30 - Rendering Style       ] Running full build
[15:51:30 - Indexing              ] Skipping indexing
[15:51:31 - Rendering Style       ] Running full build
[15:51:31 - Rendering Format      ] Starting PHP-Chunked-XHTML rendering
[15:51:31 - E_USER_WARNING        ] /Users/tekimen/src/phpdoc/phd/phpdotnet/phd/Package/Generic/XHTML.php:635
	Impossible to copy the http://www.php.net/styles/theme-base.css file.
[15:51:32 - E_USER_WARNING        ] /Users/tekimen/src/phpdoc/phd/phpdotnet/phd/Package/Generic/XHTML.php:635
	Impossible to copy the http://www.php.net/styles/theme-medium.css file.
[15:51:47 - Rendering Format      ] Writing search indexes..
[15:51:47 - Rendering Format      ] Index written
[15:51:47 - Rendering Format      ] Finished rendering

[arnaud-lb]

It's expected that zend_objects_store_free_object_storage does not free objects themselves after calling their free handler. The rationale is that objects should have been freed by ref counting or cycle detection before zend_objects_store_free_object_storage, so objects seen by the function are already leaks, and should be reported as such.

In this case the root cause appears to be that the SQLlite3 class does not implement the get_gc handler, so the GC does not see the reference from the SQLlite3 class to the callables.

Implementing a get_gc handler would fix the issue. The handler must expose any zval on which SQLite3 incremented the refcount (like ext/curl does in curl_get_gc).

[nielsdos]

That makes sense. Thanks a lot for explaining to me how the collection of owned data works. I'll get to it tonight. In fact, now I also know how to fix another issue.

[nielsdos]

Fixed via get_gc now.
I'm not entirely sure the return zend_std_get_properties(object); is needed at the end, but it seems that's what all other exts do.

[arnaud-lb]

Thank you!

Looks good to me appart from nit picks.

Maybe just add an additional test in which not all functions are set, to avoid regressions. This may spot null-derefs in the future if we change the representation of the functions in php_sqlite3_db_object.

[arnaud-lb]

Nit: zend_std_get_properties will build the property hashtable if it wasn't already. You can avoid it in two cases:

• In the if (intern->funcs == NULL && intern->collations == NULL) { branch, return zend_std_get_gc(object, table, n)
• In the other branch, you can skip zend_std_get_properties() if ce->default_properties_count == 0 and object->properties is null, because it means that there are no declared nor dynamic properties

ce->default_properties_count will be > 0 in sub-classes with declared properties, and intern->std.properties will be non-null if dynamic properties was asasigned to the object.

[nielsdos]

Thank you Arnaud. Btw I've credited you as well for your help.