Everything

php

GC issue wrt. to resource <-> object cycles?

php.internals

Christoph Becker

10 years ago

Hi!

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.

--
Christoph M. Becker

Nikita Popov

10 years ago

On Wed, Aug 10, 2016 at 12:52 PM, Christoph M. Becker <cmbecker69@gmx.de>
wrote:

> Hi!
>
> 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.

The simplest way to fix ext/xml in particular is probably to migrate it to
use an object instead of a resource.

Nikita

Kalle Sommer Nielsen

10 years ago

2016-08-10 12:59 GMT+02:00 Nikita Popov <nikita.ppv@gmail.com>:

> On Wed, Aug 10, 2016 at 12:52 PM, Christoph M. Becker <cmbecker69@gmx.de>
> wrote:
> The simplest way to fix ext/xml in particular is probably to migrate it to
> use an object instead of a resource.
>
> Nikita

If my memory strikes me right, then we did do this for ext/gmp too a
while back for the operator overloading (I even think it was you that
did that?). Maybe it would be time to evaluate the situation in 7.2,
for extensions that still use resources[1] that could be affected by
similar issues and adapt them hereafter.

Looking at the code we also got this "special" resource, which is not
exposed to the userland, called an "index pointer"[2], which only a
few extensions are using (interbase, oci8, odbc & pgsql), is this a
hack?


[1] https://gist.github.com/KalleZ/df08b99efc7c7166cfa36aab2dbe8160
[2] https://gist.github.com/KalleZ/b5b8d47f3d523c9e806188c85aa10c68

--
regards,

Kalle Sommer Nielsen
kalle@php.net

Christoph Becker

10 years ago

On 10.08.2016 at 13:38, Kalle Sommer Nielsen wrote:

> 2016-08-10 12:59 GMT+02:00 Nikita Popov <nikita.ppv@gmail.com>:
>> On Wed, Aug 10, 2016 at 12:52 PM, Christoph M. Becker <cmbecker69@gmx.de>
>> wrote:
>> The simplest way to fix ext/xml in particular is probably to migrate it to
>> use an object instead of a resource.
>
> If my memory strikes me right, then we did do this for ext/gmp too a
> while back for the operator overloading (I even think it was you that
> did that?). Maybe it would be time to evaluate the situation in 7.2,
> for extensions that still use resources[1] that could be affected by
> similar issues and adapt them hereafter.

I would welcome that, but it appears to be a lot of work.

BTW: ext/mcrypt is deprecated as of PHP 7.1 so I don't think a rework is
apt.  Furthermore, we may consider to deprecate some other extensions
(stas has a draft RFC for that).  IIRC, ext/odbc/birdstep doesn't even
work/is not used.

--
Christoph M. Becker

Christoph Becker

10 years ago

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

Fleshgrinder

10 years ago

On 8/10/2016 4:17 PM, Christoph M. Becker wrote:

> On 10.08.2016 at 12:59, Nikita Popov wrote:
>
>> 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>
>

It does not have to be a BC if we simply introduce a new API for this
extension and keep the old one as is. The whole set of functions just
cries out loud for a class that encapsulates the state of the parser and
both the utf8_* functions simply do not belong in this extension.

This new API can be introduced in any PHP minor along with a deprecation
and removal (move to PECL and abandon) of the resource based things in
PHP 8.

--
Richard "Fleshgrinder" Fussenegger

Christoph Becker

10 years ago

On 10.08.2016 at 19:34, Fleshgrinder wrote:

> On 8/10/2016 4:17 PM, Christoph M. Becker wrote:
>> On 10.08.2016 at 12:59, Nikita Popov wrote:
>>
>>> 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>
>
> It does not have to be a BC if we simply introduce a new API for this
> extension and keep the old one as is. The whole set of functions just
> cries out loud for a class that encapsulates the state of the parser and
> both the utf8_* functions simply do not belong in this extension.

That might be a good idea, even if we already have the XMLReader.  And
yes, the utf8_* functions don't really belong into ext/xml; frankly, I
think they don't belong anywhere – we already have iconv, mbstring and
recode which offer a subset of utf8_decode()/utf8_encode.  OTOH,
utf8_decode()/_encode() appear to be often used (IIRC there are a lot of
user notes suggesting to use these functions).

--
Christoph M. Becker

Michał Brzuchalski

10 years ago

Maybe it's a good idea to move them to another ext, and then move xmlparser
back to PECL ?

2016-08-10 20:23 GMT+02:00 Christoph M. Becker <cmbecker69@gmx.de>:

> On 10.08.2016 at 19:34, Fleshgrinder wrote:
>
> > On 8/10/2016 4:17 PM, Christoph M. Becker wrote:
> >> On 10.08.2016 at 12:59, Nikita Popov wrote:
> >>
> >>> 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>
> >
> > It does not have to be a BC if we simply introduce a new API for this
> > extension and keep the old one as is. The whole set of functions just
> > cries out loud for a class that encapsulates the state of the parser and
> > both the utf8_* functions simply do not belong in this extension.
>
> That might be a good idea, even if we already have the XMLReader.  And
> yes, the utf8_* functions don't really belong into ext/xml; frankly, I
> think they don't belong anywhere – we already have iconv, mbstring and
> recode which offer a subset of utf8_decode()/utf8_encode.  OTOH,
> utf8_decode()/_encode() appear to be often used (IIRC there are a lot of
> user notes suggesting to use these functions).
>
> --
> Christoph M. Becker
>
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: http://www.php.net/unsub.php
>
>

--
pozdrawiam
--
Michał Brzuchalski

Rowan Collins

10 years ago

On 10/08/2016 19:23, Christoph M. Becker wrote:

> That might be a good idea, even if we already have the XMLReader.  And
> yes, the utf8_* functions don't really belong into ext/xml; frankly, I
> think they don't belong anywhere – we already have iconv, mbstring and
> recode which offer a subset of utf8_decode()/utf8_encode.  OTOH,
> utf8_decode()/_encode() appear to be often used (IIRC there are a lot of
> user notes suggesting to use these functions).

utf8_decode()/utf8_encode are, at best, extremely misleading names. Many
uses of them in my experience go something like this: "I have an
encoding problem, it's something to do with UTF-8, I'll try utf8_encode;
hm, that didn't work, I'll try utf8_decode instead".

I hadn't even realised they're in the same extension as the equally
misunderstood xml_parse_into_struct() which exposes a somewhat baffling
intermediate parsing structure which is incredibly hard to work with. It
mostly seems to be used to reinvent event-based parsing, which is
already the main facility offered by that extension. (I just looked at
the example on the manual page; it's bizarre.)

I'm not sure if we need yet another XML parsing extension, or just
sign-post that we already have better ones for most tasks. Maybe
XMLReader could be expanded to attach callbacks for passed nodes, and
have a "readAll" method, so that it acted as an event-based parser?

Regards,

--
Rowan Collins
[IMSoP]

Christoph M. Becker

10 years ago

On 11.08.2016 at 12:04, Rowan Collins wrote:

> On 10/08/2016 19:23, Christoph M. Becker wrote:
>> That might be a good idea, even if we already have the XMLReader.  And
>> yes, the utf8_* functions don't really belong into ext/xml; frankly, I
>> think they don't belong anywhere – we already have iconv, mbstring and
>> recode which offer a subset of utf8_decode()/utf8_encode.  OTOH,
>> utf8_decode()/_encode() appear to be often used (IIRC there are a lot of
>> user notes suggesting to use these functions).
>
> utf8_decode()/utf8_encode are, at best, extremely misleading names. Many
> uses of them in my experience go something like this: "I have an
> encoding problem, it's something to do with UTF-8, I'll try utf8_encode;
> hm, that didn't work, I'll try utf8_decode instead".

You're dead on, AFAICT.

> I hadn't even realised they're in the same extension as the equally
> misunderstood xml_parse_into_struct() which exposes a somewhat baffling
> intermediate parsing structure which is incredibly hard to work with. It
> mostly seems to be used to reinvent event-based parsing, which is
> already the main facility offered by that extension. (I just looked at
> the example on the manual page; it's bizarre.)

xml_parse_into_struct() is, in my opinion, also a legacy to better get
rid of; we have DOM and SimpleXML for this.

> I'm not sure if we need yet another XML parsing extension, or just
> sign-post that we already have better ones for most tasks. Maybe
> XMLReader could be expanded to attach callbacks for passed nodes, and
> have a "readAll" method, so that it acted as an event-based parser?

I wouldn't add that to XMLReader, but rather introduce a new abstract
class, say SAXParser.  I don't mind in which extension this class would
live.

--
Christoph M. Becker

Christoph Becker

10 years ago

On 11.08.2016 at 12:04, Rowan Collins wrote:

> On 10/08/2016 19:23, Christoph M. Becker wrote:
>
>> That might be a good idea, even if we already have the XMLReader.  And
>> yes, the utf8_* functions don't really belong into ext/xml; frankly, I
>> think they don't belong anywhere – we already have iconv, mbstring and
>> recode which offer a subset of utf8_decode()/utf8_encode.  OTOH,
>> utf8_decode()/_encode() appear to be often used (IIRC there are a lot of
>> user notes suggesting to use these functions).
>
> utf8_decode()/utf8_encode are, at best, extremely misleading names. Many
> uses of them in my experience go something like this: "I have an
> encoding problem, it's something to do with UTF-8, I'll try utf8_encode;
> hm, that didn't work, I'll try utf8_decode instead".

You're dead on, AFAICT.

> I hadn't even realised they're in the same extension as the equally
> misunderstood xml_parse_into_struct() which exposes a somewhat baffling
> intermediate parsing structure which is incredibly hard to work with. It
> mostly seems to be used to reinvent event-based parsing, which is
> already the main facility offered by that extension. (I just looked at
> the example on the manual page; it's bizarre.)

xml_parse_into_struct() is, in my opinion, also a legacy to better get
rid of; we have DOM and SimpleXML for this.

> I'm not sure if we need yet another XML parsing extension, or just
> sign-post that we already have better ones for most tasks. Maybe
> XMLReader could be expanded to attach callbacks for passed nodes, and
> have a "readAll" method, so that it acted as an event-based parser?

I wouldn't add that to XMLReader, but rather introduce a new abstract
class, say SAXParser.  I don't mind in which extension this class would
exist.

--
Christoph M. Becker