Memory leak in Reflection constructors

[DanielEScherzer]

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:

• ReflectionExtension
• ReflectionObject
• ReflectionParameter (2 leaks)
• ReflectionProperty

PHP Version

8.5-dev

Operating System

No response

[DanielEScherzer]

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]

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]

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]

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

• RecursiveIteratorIterator
• RecursiveTreeIterator
• SplFileObject
• SplTempFileObject

[nielsdos]

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

[DanielEScherzer]

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

[mvorisek]

We should consider to forbid calling constructors manually (and probably extend this to all magic methods). Any ->__*() is doing it wrong, in my opinion.

I would go defensive, __construct, __destruct and __clone first. These are lifecycle methods with some expectations. Calling __isset manually is not harmful.