Memory leak in eval()'d code

Hi internals,

I've tracked down a memory leak to an anonymous class created within eval():

for ($i = 0; $i < 1000000; $i++) {
        $object = eval('return new class {};');

        if ($i % 1000 == 0) {
                echo `memory_get_usage()` . "\n";
        }
}

The memory usage quickly ramps up and memory_limit is reached in seconds.
Without eval(), the memory usage stays flat. I'm on 7.3.6.

Is this a bug? Or is this some inherent limitation of eval() that cannot
be fixed?

Thanks for any insight.
Ben

In case you're wondering (I'm sure you are!), I need this to dynamically
create an object that implements an arbitrary number of interfaces
https://github.com/brick/schema/blob/master/src/ObjectFactory.php, for a
schema.org http://schema.org structured data reader library.

Hi internals,

I've tracked down a memory leak to an anonymous class created within
eval():

for ($i = 0; $i < 1000000; $i++) {
        $object = eval('return new class {};');

        if ($i % 1000 == 0) {
                echo `memory_get_usage()` . "\n";
        }
}

The memory usage quickly ramps up and memory_limit is reached in
seconds.
Without eval(), the memory usage stays flat. I'm on 7.3.6.

Is this a bug? Or is this some inherent limitation of eval() that
cannot
be fixed?

I case this is non-trivial to fix. Each invocation recompile a new
piece of code, which creates a new class.

Similar to

for ($i = 0; $i < 1000000; $i++) {
  eval("function f$i(){}");
}
f1();
f2();

...

or

for ($i = 0; $i < 1000000; $i++) {
file_put_contents("c$i.php", 'return new class {};');
$object = include "c$i.php";
}

Theoretical it is thinkable we could either detect equal classes (but
unlikely to happen ... if it's the same why do it in eval, zthus cost
likely is higher than benefit in most cases?) or unload the class once
it's not needed anymore (which is hard to detect with reflection and
other mechanisms which aren't bound to an instance's zval and also not
cheap)

johannes

Theoretical it is thinkable we could either detect equal classes (but
unlikely to happen ... if it's the same why do it in eval, zthus cost
likely is higher than benefit in most cases?)

Thanks for your reply. I do it in eval() because schema.org has many types
(more than 600 at the moment), and each object ("Thing") can be an instance
of one or more of these types.
This creates an unmanageable number of possible combinations that I cannot
pre-compute, so I need to dynamically create a definition for an object
that implements an arbitrary number of interfaces.

or unload the class once
it's not needed anymore (which is hard to detect with reflection and
other mechanisms which aren't bound to an instance's zval and also not
cheap)

AFAIK, unloading a class is not possible from userland, I guess you're
talking about detecting this in PHP itself?

Anyway, that's an edge case, I found a workaround which is to keep a cache
of freshly created objects, indexed by interface name(s).
Anytime I request a combination of types that's already been handled in the
past, I return a clone of the cached object.

Even though the possible combinations of types are huge and impossible to
predict in advance, there are never more than a handful of such
combinations used in a single document, and I'd be surprised if there are
actually that many combinations actually used in the wild. So this should
hardly be a problem.

Thank you,
Ben

On Fri, 28 Jun 2019 at 17:59, Johannes Schlüter johannes@schlueters.de
wrote:

Hi internals,

I've tracked down a memory leak to an anonymous class created within
eval():

for ($i = 0; $i < 1000000; $i++) {
        $object = eval('return new class {};');

        if ($i % 1000 == 0) {
                echo `memory_get_usage()` . "\n";
        }
}

The memory usage quickly ramps up and memory_limit is reached in
seconds.
Without eval(), the memory usage stays flat. I'm on 7.3.6.

Is this a bug? Or is this some inherent limitation of eval() that
cannot
be fixed?

I case this is non-trivial to fix. Each invocation recompile a new
piece of code, which creates a new class.

Similar to

for ($i = 0; $i < 1000000; $i++) {
  eval("function f$i(){}");
}
f1();
f2();

...

or

for ($i = 0; $i < 1000000; $i++) {
file_put_contents("c$i.php", 'return new class {};');
$object = include "c$i.php";
}

Theoretical it is thinkable we could either detect equal classes (but
unlikely to happen ... if it's the same why do it in eval, zthus cost
likely is higher than benefit in most cases?) or unload the class once
it's not needed anymore (which is hard to detect with reflection and
other mechanisms which aren't bound to an instance's zval and also not
cheap)

johannes

or unload the class once
it's not needed anymore (which is hard to detect with reflection and
other mechanisms which aren't bound to an instance's zval and also
not
cheap)

AFAIK, unloading a class is not possible from userland, I guess you're
talking about detecting this in PHP itself?

Correct.

Anyway, that's an edge case, I found a workaround which is to keep a
cache
of freshly created objects, indexed by interface name(s).
Anytime I request a combination of types that's already been handled in
the
past, I return a clone of the cached object.

Even though the possible combinations of types are huge and impossible
to
predict in advance, there are never more than a handful of such
combinations used in a single document, and I'd be surprised if there
are
actually that many combinations actually used in the wild. So this
should
hardly be a problem.

This is going to be off-topic for his list: you could rethink your approach. Maybe you can generate the classes only on demand (if you can derive the information from a class name an evil way is function __autoload($classname) { /* figure out what is needed */ eval("class $classname { ...}")} ... absolutely evil, but hides the machinery) or you could rethink whether you really need distinct classes or whether a single type with accessor routines (either some custom or __call/__set/__get) isn't sufficient. Or something else ...

johannes

Hi!

I case this is non-trivial to fix. Each invocation recompile a new
piece of code, which creates a new class.

That's not a "leak". You create new objects (in this case, classes),
they take memory.

--
Stas Malyshev
smalyshev@gmail.com

That's not a "leak". You create new objects (in this case, classes),
they take memory.

Why do they not "leak" memory without eval() then? Replace with $object = new class {}; and memory usage stays flat.
There has do be some kind of garbage collection for these anonymous classes.

Ben

That's not a "leak". You create new objects (in this case, classes),
they take memory.

Why do they not "leak" memory without eval() then? Replace with $object = new class {}; and memory usage stays flat.
There has do be some kind of garbage collection for these anonymous classes.

AFAIR this does not create new classes, since it's the same code, and
same code means same class. But eval() has new code every time, thus new
class. Generally I don't think PHP has any operation that can destroy an
existing class. It won't be easy too since you don't know whether there
are any objects of this class around (unless you're in shutdown).

--
Stas Malyshev
smalyshev@gmail.com

I think we missed the point here. Clousre, or anonymous class, should not be considered as normal class. We expect normal class existing once we declare it till end of script. However, for anonymous class, it's usually used within certain scope, so not only the instances, the class itself should be included in the GC as well.
I guess the problem here is we didn't GC the space for definition of anonymous classes.

Regards,
CHU Zhaowei

-----Original Message-----
From: Stanislav Malyshev smalyshev@gmail.com
Sent: Saturday, June 29, 2019 6:52 AM
To: Benjamin Morel benjamin.morel@gmail.com
Cc: PHP Internals internals@lists.php.net
Subject: Re: [PHP-DEV] Memory leak in eval()'d code

That's not a "leak". You create new objects (in this case, classes),
they take memory.

Why do they not "leak" memory without eval() then? Replace with
$object = new class {}; and memory usage stays flat.
There has do be some kind of garbage collection for these anonymous classes.

AFAIR this does not create new classes, since it's the same code, and same code
means same class. But eval() has new code every time, thus new class. Generally
I don't think PHP has any operation that can destroy an existing class. It won't be
easy too since you don't know whether there are any objects of this class
around (unless you're in shutdown).

--
Stas Malyshev
smalyshev@gmail.com

--
To unsubscribe, visit:
http://www.php.net/unsub.php

I think we missed the point here. Clousre, or anonymous class, should not
be considered as normal class. We expect normal class existing once we
declare it till end of script. However, for anonymous class, it's usually
used within certain scope, so not only the instances, the class itself
should be included in the GC as well.
I guess the problem here is we didn't GC the space for definition of
anonymous classes.

Regards,
CHU Zhaowei

As Johannes already pointed out, we cannot garbage collect anonymous class
definitions due to the existence of opaque references. A simple example of
code that currently works:

$obj = eval('return new class {}');
$class = get_class($obj); // create opaque reference
unset($obj); // drop last direct reference
$obj = new $class;

In the end, an anonymous class is essentially a class with a random name
and some inline construction sugar, but apart from that does not differ
from ordinary classes. (I've also seen some calls to allow syntax like
$class = class {}; that would make that even more obvious.)

The situation here would be different if we had first-class classes and did
not refer to classes by name. But as-is, I don't think garbage collecting
anonymous classes is a possibility.

Nikita

-----Original Message-----

From: Stanislav Malyshev smalyshev@gmail.com
Sent: Saturday, June 29, 2019 6:52 AM
To: Benjamin Morel benjamin.morel@gmail.com
Cc: PHP Internals internals@lists.php.net
Subject: Re: [PHP-DEV] Memory leak in eval()'d code

That's not a "leak". You create new objects (in this case,

classes),

they take memory.

Why do they not "leak" memory without eval() then? Replace with
$object = new class {}; and memory usage stays flat.
There has do be some kind of garbage collection for these anonymous
classes.

AFAIR this does not create new classes, since it's the same code, and
same code
means same class. But eval() has new code every time, thus new class.
Generally
I don't think PHP has any operation that can destroy an existing class.
It won't be
easy too since you don't know whether there are any objects of this class
around (unless you're in shutdown).

--
Stas Malyshev
smalyshev@gmail.com

--
To unsubscribe,
visit:
http://www.php.net/unsub.php

As Johannes already pointed out, we cannot garbage collect anonymous
class definitions due to the existence of opaque references.

I still don't understand why, then, the memory stays flat when the new
class does not happen in eval():

for ($i = 0; $i < 1000000; $i++) {
        $object = new class {};

        if ($i % 1000 == 0) {
                echo `memory_get_usage()` . "\n";
        }
}

I checked get_class() for all instances returned, it's always the same
class name whether it's called from eval() or not, so this code should
only ever create a single class in memory, right? Why the leak with eval()?

$class = get_class($obj);
$obj = new $class;

Interesting. Can this behaviour be relied upon (is it documented?), or is
this an implementation detail that may change at any time?

Thank you,
Ben

Thanks for the example. Now I understand the root cause. There is a similar bug report(https://bugs.php.net/bug.php?id=74938) for anonymous function. I thought it leaked for the same reason, but now I think I’m wrong. We don’t have to create new classes for them since they are all instances of Closure class, and it’s not possible to create opaque reference for functions. FUNCTION returns “{closure}”. Could you take a look?

Back to the anonymous class, I’m thinking if we can have any workaround for this issue, otherwise it will be a huge problem if someone wants to write applications running for a long time. It will be a memory leak that cannot be fixed in userland code unless he decides to drop anonymous class at all. Since most of the use cases should not have opaque references, can we add a flag in zval to indicate if an object has this kind of reference? The number of methods to create such opaque references should be limited. CLASS, get_class(), reflections, clone. Did I miss any other way? With the help of this flag, we are able to know if it’s safe to remove the class definition during destruction.

Regards,

CHU Zhaowei

From: Nikita Popov nikita.ppv@gmail.com
Sent: Saturday, June 29, 2019 3:58 PM
To: CHU Zhaowei me@jhdxr.com
Cc: Stanislav Malyshev smalyshev@gmail.com; Benjamin Morel benjamin.morel@gmail.com; PHP Internals internals@lists.php.net
Subject: Re: [PHP-DEV] Memory leak in eval()'d code

I think we missed the point here. Clousre, or anonymous class, should not be considered as normal class. We expect normal class existing once we declare it till end of script. However, for anonymous class, it's usually used within certain scope, so not only the instances, the class itself should be included in the GC as well.
I guess the problem here is we didn't GC the space for definition of anonymous classes.

Regards,
CHU Zhaowei

As Johannes already pointed out, we cannot garbage collect anonymous class definitions due to the existence of opaque references. A simple example of code that currently works:

$obj = eval('return new class {}');

$class = get_class($obj); // create opaque reference

unset($obj); // drop last direct reference

$obj = new $class;

In the end, an anonymous class is essentially a class with a random name and some inline construction sugar, but apart from that does not differ from ordinary classes. (I've also seen some calls to allow syntax like $class = class {}; that would make that even more obvious.)

The situation here would be different if we had first-class classes and did not refer to classes by name. But as-is, I don't think garbage collecting anonymous classes is a possibility.

Nikita

-----Original Message-----
From: Stanislav Malyshev <smalyshev@gmail.com mailto:smalyshev@gmail.com >
Sent: Saturday, June 29, 2019 6:52 AM
To: Benjamin Morel <benjamin.morel@gmail.com mailto:benjamin.morel@gmail.com >
Cc: PHP Internals <internals@lists.php.net mailto:internals@lists.php.net >
Subject: Re: [PHP-DEV] Memory leak in eval()'d code

That's not a "leak". You create new objects (in this case, classes),
they take memory.

Why do they not "leak" memory without eval() then? Replace with
$object = new class {}; and memory usage stays flat.
There has do be some kind of garbage collection for these anonymous classes.

AFAIR this does not create new classes, since it's the same code, and same code
means same class. But eval() has new code every time, thus new class. Generally
I don't think PHP has any operation that can destroy an existing class. It won't be
easy too since you don't know whether there are any objects of this class
around (unless you're in shutdown).

--
Stas Malyshev
smalyshev@gmail.com mailto:smalyshev@gmail.com

--
To unsubscribe, visit:
http://www.php.net/unsub.php

Thanks for the example. Now I understand the root cause. There is a
similar bug report(https://bugs.php.net/bug.php?id=74938) for anonymous
function. I thought it leaked for the same reason, but now I think I’m
wrong. We don’t have to create new classes for them since they are all
instances of Closure class, and it’s not possible to create opaque
reference for functions. FUNCTION returns “{closure}”. Could you take
a look?

This leaks for the same reason (function definition is not garbage
collected), but in this case we could indeed implement the garbage
collection without any observable changes to semantics. I don't think it
will be simple to do this though.

Back to the anonymous class, I’m thinking if we can have any workaround

for this issue, otherwise it will be a huge problem if someone wants to
write applications running for a long time. It will be a memory leak that
cannot be fixed in userland code unless he decides to drop anonymous class
at all.

I think you got caught up in a wrong premise here: Just using anonymous
classes does not cause leaks. The problem here is that OP is creating
many classes (the fact that they are anonymous ultimately doesn't matter)
by eval'ing code. That is what causes the leak. Unless you are doing
something like this (which you obviously shouldn't, for more reasons than
this), there is nothing to worry about. As such, I don't really think there
is a problem worth solving here.

Nikita

Since most of the use cases should not have opaque references, can we add a

flag in zval to indicate if an object has this kind of reference? The
number of methods to create such opaque references should be limited.
CLASS, get_class(), reflections, clone. Did I miss any other way? With
the help of this flag, we are able to know if it’s safe to remove the class
definition during destruction.

Regards,

CHU Zhaowei

From: Nikita Popov nikita.ppv@gmail.com
Sent: Saturday, June 29, 2019 3:58 PM
To: CHU Zhaowei me@jhdxr.com
Cc: Stanislav Malyshev smalyshev@gmail.com; Benjamin Morel <
benjamin.morel@gmail.com>; PHP Internals internals@lists.php.net
Subject: Re: [PHP-DEV] Memory leak in eval()'d code

I think we missed the point here. Clousre, or anonymous class, should not
be considered as normal class. We expect normal class existing once we
declare it till end of script. However, for anonymous class, it's usually
used within certain scope, so not only the instances, the class itself
should be included in the GC as well.
I guess the problem here is we didn't GC the space for definition of
anonymous classes.

Regards,
CHU Zhaowei

As Johannes already pointed out, we cannot garbage collect anonymous class
definitions due to the existence of opaque references. A simple example of
code that currently works:

$obj = eval('return new class {}');

$class = get_class($obj); // create opaque reference

unset($obj); // drop last direct reference

$obj = new $class;

In the end, an anonymous class is essentially a class with a random name
and some inline construction sugar, but apart from that does not differ
from ordinary classes. (I've also seen some calls to allow syntax like
$class = class {}; that would make that even more obvious.)

The situation here would be different if we had first-class classes and
did not refer to classes by name. But as-is, I don't think garbage
collecting anonymous classes is a possibility.

Nikita

-----Original Message-----
From: Stanislav Malyshev smalyshev@gmail.com
Sent: Saturday, June 29, 2019 6:52 AM
To: Benjamin Morel benjamin.morel@gmail.com
Cc: PHP Internals internals@lists.php.net
Subject: Re: [PHP-DEV] Memory leak in eval()'d code

That's not a "leak". You create new objects (in this case,

classes),

they take memory.

Why do they not "leak" memory without eval() then? Replace with
$object = new class {}; and memory usage stays flat.
There has do be some kind of garbage collection for these anonymous
classes.

AFAIR this does not create new classes, since it's the same code, and
same code
means same class. But eval() has new code every time, thus new class.
Generally
I don't think PHP has any operation that can destroy an existing class.
It won't be
easy too since you don't know whether there are any objects of this class
around (unless you're in shutdown).

--
Stas Malyshev
smalyshev@gmail.com

--
To unsubscribe,
visit:
http://www.php.net/unsub.php

The problem here is that OP is creating many classes (the fact that
they are anonymous ultimately doesn't matter) by eval'ing code. That is
what causes the leak.

I still don't understand why there is no leak when not using eval(),
though.

Ben

On Sat, Jun 29, 2019 at 11:25 PM Benjamin Morel benjamin.morel@gmail.com
wrote:

The problem here is that OP is creating many classes (the fact that
they are anonymous ultimately doesn't matter) by eval'ing code. That is
what causes the leak.

I still don't understand why there is no leak when not using eval(),
though.

Ben

Without eval you are creating many objects of the same anonymous class.
With eval you are creating a single object each for many anonymous classes.
In the first case there is only a single class, in the second there are as
many classes as there are eval()s.

That's the theory. In practice we have a long-standing bug with RTD key
collisions -- this means that the names generated for anonymous classes
(and closures for that matter) are not actually unique. For anonymous
classes the current workaround is to reuse the existing class definition if
an RTD key collision occurs. For your original code (and I should say that
this is very specific to the code because it is susceptible to memory
layout details) it happens that the RTD key for all those anonymous classes
is the same, so in the end you end up with just one class definition also
in the eval() case. The leaked memory is an allocated but unused class
entry. We could plug that particular leak -- but I should emphasize that
the current behavior is incorrect and once the key collision issue is
resolved this will create a new class for each eval.

Nikita

On Sat, Jun 29, 2019 at 11:25 PM Benjamin Morel benjamin.morel@gmail.com
wrote:

The leaked memory is an allocated but unused class
entry. We could plug that particular leak -- but I should emphasize that
the current behavior is incorrect and once the key collision issue is
resolved this will create a new class for each eval.

To clarify; you’re saying that the behavior seen in the non scaled case is
kind of “wrong” in the sense that it’s exposing the big in naming of the
anon class. Once the bug is resolved, the two behaviors shown should mimic
each other, wherein the memory footprint would grow as in the example
eval() case.

—
Dave

On Sat, Jun 29, 2019 at 11:25 PM Benjamin Morel <benjamin.morel@gmail.com

wrote:

The leaked memory is an allocated but unused class
entry. We could plug that particular leak -- but I should emphasize that
the current behavior is incorrect and once the key collision issue is
resolved this will create a new class for each eval.

To clarify; you’re saying that the behavior seen in the non scaled case is
kind of “wrong” in the sense that it’s exposing the big in naming of the
anon class. Once the bug is resolved, the two behaviors shown should mimic
each other, wherein the memory footprint would grow as in the example
eval() case.

What is the "non scaled case"? To repeat what I said earlier: Without eval
there is no and will not be a leak.

The bug is that eval() currently reuses an existing class instead of
creating a new one each time. Calling get_class(eval("return new class
{};")) should be returning a new string every time, while for OPs script it
currently does not.

tl;dr:

while (1) {
get_class(new class {}); // should always return the same
get_class(eval("return new class {};")); // should always return
different, currently may return same due to bug
}

Nikita

What is the "non scaled case"? To repeat what I said earlier: Without eval
there is no and will not be a leak.
The bug is that eval() currently reuses an existing class instead of
creating a new one each time. Calling get_class(eval("return new class
{};")) should be returning a new string every time, while for OPs script it
currently does not.
tl;dr:
while (1) {
get_class(new class {}); // should always return the same
get_class(eval("return new class {};")); // should always return
different, currently may return same due to bug
}

Ok thank you, anyway the good news is that I found a workaround for now
(creating classes of a given type once then cloning). And if I understand
correctly, once the issue you mention is resolved (and get_class() returns
a distinct class every time), I should have another alternative:

$object = eval('return new class (...) {};');
$class = get_class($object);
$anotherObjectOfSameClass = new $class();

And I do understand that my use case is border line, and does not deserve
too much attention.
It's very useful to be able to perform instanceof on objects that
dynamically implement an arbritary set of interfaces, though.

Ben

Hi!

I still don't understand why, then, the memory stays flat when the new
class does not happen in eval():

Because that code creates only one class.

I checked get_class() for all instances returned, it's always the same
class name whether it's called from eval() or not, so this code should
only ever create a single class in memory, right? Why the leak with eval()?

There's no leak, eval() just creates new class each time.

Stas Malyshev
smalyshev@gmail.com

Eval is not the only case that will cause memory usage growing actually. https://3v4l.org/fPhlU is an example using include inside a loop, which is used to simulate including same file several times. I agree it might be trivial to improve for eval since it's not recommended to use due to security concern nowadays. But I think we should pay attention to this issue since include/require behave the same.

In theory, I understand how it happens. You won't be surprised if you copy the code and paste it several times and more memory is used. It's reasonable that more memory is needed to compile longer codes. Include several times just work like copy&paste several times. But since we are including the same file, is it possible to detect and reuse the space?

Regards,
CHU Zhaowei

-----Original Message-----
From: Stanislav Malyshev smalyshev@gmail.com
Sent: Sunday, June 30, 2019 10:43 AM
To: Benjamin Morel benjamin.morel@gmail.com; Nikita Popov
nikita.ppv@gmail.com
Cc: CHU Zhaowei me@jhdxr.com; PHP Internals internals@lists.php.net
Subject: Re: [PHP-DEV] Memory leak in eval()'d code

Hi!

I still don't understand why, then, the memory stays flat when the
new class does not happen in eval():

Because that code creates only one class.

I checked get_class() for all instances returned, it's always the
same class name whether it's called from eval() or not, so this code
should only ever create a single class in memory, right? Why the leak with
eval()?

There's no leak, eval() just creates new class each time.

Stas Malyshev
smalyshev@gmail.com

--
To unsubscribe, visit:
http://www.php.net/unsub.php