Translating optional zval ** with zend_parse_parameters() to PHP 7

8 years ago by Thomas Hruska — view source

unread

I'm working on updating an extension for PHP 7 compatibility. I have
one function that uses an optional zval ** with zend_parse_parameters().

zval **zprevcount = NULL;
int count;
int argc = ZEND_NUM_ARGS();

if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "|Z", &zprevcount)

== FAILURE) return;

...

if (argc > 0)
{
	count = (int)PrevCount;

	zval_dtor(*zprevcount);
	ZVAL_LONG(*zprevcount, count);
}

What's the correct way to translate that into PHP 7?

--
Thomas Hruska
CubicleSoft President

I've got great, time saving software that you will find useful.

http://cubiclesoft.com/

8 years ago by Kalle Sommer Nielsen — view source

unread

Hi Thomas

2016-11-25 4:13 GMT+01:00 Thomas Hruska thruska@cubiclesoft.com:

I'm working on updating an extension for PHP 7 compatibility. I have one
function that uses an optional zval ** with zend_parse_parameters().
    zval **zprevcount = NULL;
    int count;
    int argc = ZEND_NUM_ARGS();

    if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "|Z",
&zprevcount) == FAILURE) return;
    ...

    if (argc > 0)
    {
            count = (int)PrevCount;

            zval_dtor(*zprevcount);
            ZVAL_LONG(*zprevcount, count);
    }
What's the correct way to translate that into PHP 7?

Since you are working with integers, you can do:

zend_long prevcount = 0;

if(zend_parse_parameters(ZEND_NUM_ARGS(), "|l", &prevcount) == FAILURE)
{
return;
}

if(prevcount)
{
/* ... use prevcount ... */
}

Notice that all integers returned from the ZPP is a zend_long, you can
see more information about the ZPP in
php-src/README.PARAMETER_PARSING_API

--
regards,

Kalle Sommer Nielsen
kalle@php.net

8 years ago by Thomas Hruska — view source

unread

Hi Thomas

2016-11-25 4:13 GMT+01:00 Thomas Hruska thruska@cubiclesoft.com:
I'm working on updating an extension for PHP 7 compatibility. I have one
function that uses an optional zval ** with zend_parse_parameters().
    zval **zprevcount = NULL;
    int count;
    int argc = ZEND_NUM_ARGS();

    if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "|Z",
&zprevcount) == FAILURE) return;
    ...

    if (argc > 0)
    {
            count = (int)PrevCount;

            zval_dtor(*zprevcount);
            ZVAL_LONG(*zprevcount, count);
    }
What's the correct way to translate that into PHP 7?
Since you are working with integers, you can do:

zend_long prevcount = 0;

if(zend_parse_parameters(ZEND_NUM_ARGS(), "|l", &prevcount) == FAILURE)
{
return;
}

if(prevcount)
{
/* ... use prevcount ... */
}

Notice that all integers returned from the ZPP is a zend_long, you can
see more information about the ZPP in
php-src/README.PARAMETER_PARSING_API

Unless I missed something, zend_long is generally defined as a standard
int64_t or int32_t type (Zend/zend_long.h). I need to be able to
modify the original variable that was passed in. I already use "|l"
and zend_long elsewhere in the extension.

I'm thinking the correct ZPP string is "|z/" but the usage examples in
core that I've looked at so far are inconsistent, hence the original
question of what's the correct approach?

--
Thomas Hruska
CubicleSoft President

I've got great, time saving software that you will find useful.

http://cubiclesoft.com/

8 years ago by Kalle Sommer Nielsen — view source

unread

2016-11-25 13:30 GMT+01:00 Thomas Hruska thruska@cubiclesoft.com:

I need to be able to modify the original variable that was passed in. I already use "|l" and zend_long
elsewhere in the extension

Ah my bad I misread it!

In that case take a look at where such is implemented in php-src,
dns_get_record() is an example despite the slightly clouded code:
http://git.php.net/?p=php-src.git;a=blob;f=ext/standard/dns.c;h=f92015eee90d3e93a801e93d6381d89923825166;hb=refs/heads/master#l1011

See $weight_list:

Check if the argument is passed
zval_dtor()
Populate it (ZVAL_LONG is the same)

The main difference from your code is to use 'z' (lower case) in ZPP,
and then use zval* (which is what 'z' returns).

I'm not at my dev machine, so I cannot test it but that should be how it goes

--
regards,

Kalle Sommer Nielsen
kalle@php.net

8 years ago by Jakub Zelenka — view source

unread

2016-11-25 13:30 GMT+01:00 Thomas Hruska thruska@cubiclesoft.com:

I need to be able to modify the original variable that was passed in.
I already use "|l" and zend_long
elsewhere in the extension

Ah my bad I misread it!

In that case take a look at where such is implemented in php-src,
dns_get_record() is an example despite the slightly clouded code:
http://git.php.net/?p=php-src.git;a=blob;f=ext/standard/dns.c;h=
f92015eee90d3e93a801e93d6381d89923825166;hb=refs/heads/master#l1011

See $weight_list:

Check if the argument is passed
zval_dtor()
Populate it (ZVAL_LONG is the same)

The main difference from your code is to use 'z' (lower case) in ZPP,
and then use zval* (which is what 'z' returns).

You also need set 1 for the first param of ZEND_ARG_INFO (specify that it
is a ref param) otherwise it won't use the changed val...

Jakub

8 years ago by Thomas Hruska — view source

unread

2016-11-25 13:30 GMT+01:00 Thomas Hruska thruska@cubiclesoft.com:

I need to be able to modify the original variable that was passed in.
I already use "|l" and zend_long
elsewhere in the extension

Ah my bad I misread it!

In that case take a look at where such is implemented in php-src,
dns_get_record() is an example despite the slightly clouded code:
http://git.php.net/?p=php-src.git;a=blob;f=ext/standard/dns.c;h=
f92015eee90d3e93a801e93d6381d89923825166;hb=refs/heads/master#l1011

See $weight_list:

Check if the argument is passed

zval_dtor()

Populate it (ZVAL_LONG is the same)

The main difference from your code is to use 'z' (lower case) in ZPP,
and then use zval* (which is what 'z' returns).

You also need set 1 for the first param of ZEND_ARG_INFO (specify that it
is a ref param) otherwise it won't use the changed val...

Jakub

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.

--
Thomas Hruska
CubicleSoft President

I've got great, time saving software that you will find useful.

http://cubiclesoft.com/

8 years ago by Christoph M. Becker — view source

unread

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.

--
Christoph M. Becker

8 years ago by Nikita Popov — view source

unread

On Sat, Nov 26, 2016 at 12:29 PM, Christoph M. Becker cmbecker69@gmx.de
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).

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.

Nikita

8 years ago by Christoph M. Becker — view source

unread

On Sat, Nov 26, 2016 at 12:29 PM, Christoph M. Becker cmbecker69@gmx.de
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

8 years ago by Christoph M. Becker — view source

unread

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?

FTR: reported as https://bugs.php.net/bug.php?id=73612.

--
Christoph M. Becker