php / php-src

The PHP Interpreter
https://www.php.net
Other
38.15k stars 7.74k forks source link

Memory leak in Reflection constructors #16601

Open DanielEScherzer opened 2 hours ago

DanielEScherzer commented 2 hours ago

Description

The following code:

<?php

$r = new ReflectionProperty(Exception::class, 'message');
$r->__construct(Exception::class, 'message');

Resulted in this output:

root@b19f2b6240fd:/usr/src/php# php /var/www/html/test.php
[Fri Oct 25 18:18:20 2024]  Script:  '/var/www/html/test.php'
/usr/src/php/ext/reflection/php_reflection.c(5837) :  Freeing 0x00007f853ca01120 (16 bytes), script=/var/www/html/test.php
=== Total 1 memory leaks detected ===

But I expected this output instead:

[none]

Also tested and have a memory leak:

PHP Version

8.5-dev

Operating System

No response

DanielEScherzer commented 2 hours ago

There is an existing test ReflectionConstant_double_construct.phpt that ensures this isn't a problem for the ReflectionConstant class - maybe we should add one for each class?

nielsdos commented 2 hours ago

There is an existing test ReflectionConstant_double_construct.phpt that ensures this isn't a problem for the ReflectionConstant class - maybe we should add one for each class?

Probably yes. If you want to write the tests and give this a shot, go ahead :)

cmb69 commented 2 hours ago

A related issue came up not that long ago (maybe @iluuu1994 remembers). We should consider to forbid calling constructors manually (and probably extend this to all magic methods). Any ->__*() is doing it wrong, in my opinion.

DanielEScherzer commented 1 hour ago

I found this in a number of other classes in other extensions as well, should I file a dedicated task for each?

nielsdos commented 1 hour ago

It doesn't matter too much how we keep track of it. Separate issue for reflection vs spl is perhaps a bit cleaner.

DanielEScherzer commented 1 hour ago

It doesn't matter too much how we keep track of it. Separate issue for reflection vs spl is perhaps a bit cleaner.

Okay, filed #16604