Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:97198
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmx.de designates 212.227.15.15 as permitted sender)
To: Nikita Popov <nikita.ppv@gmail.com>
References: <56e89e82-00aa-5521-f014-14cf4d588157@cubiclesoft.com>
 <CAJW__o2vn0PGJEYQ8-qojYCBTd7Czjf193NEzzaCqK+NHEHZ6A@mail.gmail.com>
 <5d865bc1-dc06-54b8-800e-99f1c2914632@cubiclesoft.com>
 <CAJW__o3qa=Fio_ZenLvVhL=DXiD=e-r8haxUcLEq2pjVib8xwQ@mail.gmail.com>
 <CAEKnhAFbXaydKSNP2pmDLnDnhx7M+vTVXiUhwcHCNX=ooHiKpA@mail.gmail.com>
 <024beaa8-b93a-10e5-bbe1-f1af1b950b81@cubiclesoft.com>
 <c2fbde5e-00cf-0cb7-f9d4-e25b8805c8e6@gmx.de>
 <CAF+90c-p+=zm3qq-kU3OxAt+GG4yJ88Z4tyBSBz02bZfR=OCgw@mail.gmail.com>
Cc: Thomas Hruska <thruska@cubiclesoft.com>, Jakub Zelenka <bukka@php.net>,
 Kalle Sommer Nielsen <kalle@php.net>,
 PHP internals list <internals@lists.php.net>
Message-ID: <ceeed1be-0167-a7bc-d312-e05bcebea4ae@gmx.de>
Date: Sat, 26 Nov 2016 13:55:34 +0100
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101
 Thunderbird/45.5.0
MIME-Version: 1.0
In-Reply-To: <CAF+90c-p+=zm3qq-kU3OxAt+GG4yJ88Z4tyBSBz02bZfR=OCgw@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Subject: Re: [PHP-DEV] Translating optional zval **
 withzend_parse_parameters()to PHP 7
From: cmbecker69@gmx.de ("Christoph M. Becker")

On 26.11.2016 at 12:45, Nikita Popov wrote:

> On Sat, Nov 26, 2016 at 12:29 PM, Christoph M. Becker <cmbecker69@gmx.de>
> wrote:
> 
>> On 26.11.2016 at 01:47, Thomas Hruska wrote:
>>
>>> Okay, everyone has been helpful.  Thanks.  I'll go with:
>>>
>>>
>>>     zval *zprevcount = NULL;
>>>     zend_long count;
>>>
>>>     if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "|z/",
>>> &zprevcount) == FAILURE)  return;
>>>
>>>     ...
>>>
>>>     if (zprevcount != NULL)
>>>     {
>>>         count = (zend_long)PrevCount;
>>>
>>>         zval_dtor(zprevcount);
>>>         ZVAL_LONG(zprevcount, count);
>>>     }
>>>
>>>
>>> Just one little thing I found in php_str_replace_common() in
>>> ext/standard/string.c.  That particular function calls zval_ptr_dtor()
>>> instead of zval_dtor() on zcount.  Just wondering why it does that - I'm
>>> thinking it has something to do with removing the ZPP call for PHP 7 and
>>> using the (mostly undocumented?) Z_PARAM_ZVAL_EX() macro.  However,
>>> php_do_pcre_match() over in ext/pcre/php_pcre.c calls zval_dtor() on
>>> subpats but also uses the Z_PARAM_ZVAL_EX() macro in a similar fashion.
>>> Looking back at PHP 5.6's php_str_replace_common() shows that it
>>> previously called zval_dtor().  So it might also be a bug of some sort.
>>
>> zval_dtor() destroys the value; zval_ptr_dtor() decrements the refcount,
>> and destroys the value only if the refcount has dropped to 0.  See also
>> <http://www.phpinternalsbook.com/zvals/memory_management.html> (which is
>> written for PHP 5, but is still useful).
>>
>> In your case, zval_dtor() is appropriate, as the zval has already been
>> separated (ZPP's /), so there can't be other references.
> 
> The situation here has become a bit confusing in PHP 7. zval_dtor() is now
> actually the same as zval_ptr_dtor_nogc(). As such, you can use zval_dtor()
> on zvals with rc>1, but collectible zvals will not be added to the GC root
> buffer.
> 
> In the vast majority of cases, you will want to use zval_ptr_dtor().
> zval_dtor() / zval_ptr_dtor_nogc() are useful either as an optimization in
> cases where you know for certain that a value is a root, or to prevent
> values from being placed in the GC root buffer, in cases where this
> violates the memory model (e.g. this is relevant for opcache'd literals).

Thanks for the explanation, Nikita.

> The fact that php_pcre.c uses zval_dtor() is simply a bug, because code like
> 
>     $obj = new stdClass;
>     $obj->obj = $obj;
>     preg_match('/./', 'x', $obj);
> 
> leaks.

Indeed.  At least preg_replace()'s $count parameter has the same issue.
Is there already a bug report about this?

-- 
Christoph M. Becker