RE: [PHP-DEV] Memory leak in eval()'d code

This is only part of a thread. view whole thread
  106108
June 29, 2019 15:30 me@jhdxr.com (=?utf-8?b?Q0hVIFpoYW93ZWk=?=)
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 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 morel@gmail.com>; PHP Internals <internals@lists.php.net>
Subject: Re: [PHP-DEV] Memory leak in eval()'d code

 

On Sat, Jun 29, 2019 at 9:07 AM CHU Zhaowei <me@jhdxr.com <mailto:me@jhdxr.com> > wrote:

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 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 > > > > On 6/28/19 3:37 PM, Benjamin Morel wrote: > > 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> > > -- > PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: > http://www.php.net/unsub.php >
-- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
  106111
June 29, 2019 16:20 nikita.ppv@gmail.com (Nikita Popov)
On Sat, Jun 29, 2019 at 5:31 PM CHU Zhaowei <me@jhdxr.com> wrote:

> 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 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 > > > > On Sat, Jun 29, 2019 at 9:07 AM CHU Zhaowei <me@jhdxr.com> wrote: > > 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 morel@gmail.com> > > Cc: PHP Internals <internals@lists.php.net> > > Subject: Re: [PHP-DEV] Memory leak in eval()'d code > > > > > > > > On 6/28/19 3:37 PM, Benjamin Morel wrote: > > > 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 > > > > -- > > PHP Internals - PHP Runtime Development Mailing List To unsubscribe, > visit: > > http://www.php.net/unsub.php > > > > > > > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: http://www.php.net/unsub.php > >
  106115
June 29, 2019 21:25 benjamin.morel@gmail.com (Benjamin Morel)
> 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
  106116
June 29, 2019 21:51 nikita.ppv@gmail.com (Nikita Popov)
On Sat, Jun 29, 2019 at 11:25 PM Benjamin Morel 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
  106117
June 29, 2019 21:56 dave@mudsite.com (David Walker)
On Sat, Jun 29, 2019 at 15:52 Nikita Popov ppv@gmail.com> wrote:

> On Sat, Jun 29, 2019 at 11:25 PM Benjamin Morel 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
> >
  106118
June 29, 2019 22:04 nikita.ppv@gmail.com (Nikita Popov)
On Sat, Jun 29, 2019 at 11:56 PM David Walker <dave@mudsite.com> wrote:

> > > On Sat, Jun 29, 2019 at 15:52 Nikita Popov ppv@gmail.com> wrote: > >> On Sat, Jun 29, 2019 at 11:25 PM Benjamin Morel 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
  106119
June 29, 2019 22:19 benjamin.morel@gmail.com (Benjamin Morel)
> > 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