Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:95011
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmx.de designates 212.227.17.21 as permitted sender)
To: Nikita Popov <nikita.ppv@gmail.com>
References: <ed7bc05c-90a3-c21a-b43e-186221a73006@gmx.de>
 <CAF+90c9VBtbveOtbK7rRGAd+DJtq7dSzR4aYNo2q2vOs5624kQ@mail.gmail.com>
Cc: PHP Internals List <internals@lists.php.net>
Message-ID: <9e9c440e-05d2-a452-556e-a6c62a9f195e@gmx.de>
Date: Wed, 10 Aug 2016 16:17:23 +0200
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101
 Thunderbird/45.2.0
MIME-Version: 1.0
In-Reply-To: <CAF+90c9VBtbveOtbK7rRGAd+DJtq7dSzR4aYNo2q2vOs5624kQ@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Subject: Re: [PHP-DEV] GC issue wrt. to resource <-> object cycles?
From: cmbecker69@gmx.de ("Christoph M. Becker")

On 10.08.2016 at 12:59, Nikita Popov wrote:

> On Wed, Aug 10, 2016 at 12:52 PM, Christoph M. Becker <cmbecker69@gmx.de>
> wrote:
>>
>> I've tried to fix <https://bugs.php.net/72793>, and it looks like
>> there's a general GC issue wrt. to resources referencing objects and
>> vice versa.  Aren't resource ZVALS put in the root buffer?
>>
>> See <https://3v4l.org/JYIQs>, which demonstrates the issue better than
>> the test script in the bug report.  A steady increase of allocated
>> memory can be seen, even though gc_collect_cycles() is called.  When
>> uncommenting `unset($this->parser);`, everything is fine (the GC
>> wouldn't be involved at all in this case).
>>
>> Wrt. to the PHP 5.6 behavior: this appears fine, but actually it's in
>> error, because of <https://github.com/php/php-src/commit/72ec2e8f>.  Not
>> increasing the refcount of `parser->object` might theoretically lead to
>> use-after-free scenarios.
> 
> As you correctly deduced, we currently do not support GC for resources.
> This would require introducing something akin to the get_gc handler for
> resources.

Thanks for the clarification!

> The simplest way to fix ext/xml in particular is probably to migrate it to
> use an object instead of a resource.

I guess that is the best action in the long run (i.e. 7.2+), but that
would of course cause a BC break, so probably it's best to document that
the user has to break the cycle manually, if xml_set_object() is used
(7.0, 7.1).

Wrt. to PHP 5.6 it appears there are no issues at all.  While the code
out-commented by Thies is still there (which should be removed to avoid
further confusion), in the following a (shallow) copy of the object is
made, and apparently due to the additional refcount in the object store
of PHP 5, everything is okay.

Another alternative for PHP 7.2 might be to drop (after deprecation)
xml_set_object(); actually, it seems to me this function is a relict of
pre PHP 5.  Cf. example #1[1].  Instead of doing:

  xml_set_object($this->parser, $this);
  xml_set_element_handler($this->parser, "tag_open", "tag_close");

one could simply do:

  xml_set_element_handler($this->parser,
      [$this, "tag_open"], [$this, "tag_close]);

[1]
<http://php.net/manual/en/function.xml-set-object.php#refsect1-function.xml-set-object-examples>

-- 
Christoph M. Becker