13 years ago by Daniel K. — view source

unread

When a built-in function is defined with ZEND_SEND_PREFER_REF, PHP will
issue a strict warning if you use an assignment expression as the parameter.

As an example, current() show this behaviour.

current() has this arginfo defined:

ZEND_BEGIN_ARG_INFO(arginfo_current, ZEND_SEND_PREFER_REF)
ZEND_ARG_INFO(ZEND_SEND_PREFER_REF, arg)
ZEND_END_ARG_INFO()

and a call like:

<?php
current($foo = array("bar"));
?>

Presents you with:

PHP Strict Standards: Only variables should be passed by reference in
%s on line %d

I think it is wrong to warn about this, because, to me, the PREFER
part of ZEND_SEND_PREFER_REF, conveys that we should prefer to pass by
reference, if possible, and not care otherwise.

The below patch implements the not care part that was missing until now.

This is done by having the lexer mark the result variable of the
assignment expression as not passable by reference, and changing the
function zend_do_pass_param() to ignore the marked variables when
considering if a parameter could be passed by reference or not.

I have run make test, before and after, with no regressions. The only
test affected was the one I sent earlier to specifically test for this bug.

I am, however, not sure if this is the right approach to solve the
problem, in which case, I hope to at least have put one of you on the
right track to the real solution, and to have made you interested in
fixing it properly.

The patch is for php-5.3.8

Daniel K.

diff --git a/Zend/zend_compile.c b/Zend/zend_compile.c
index c325a7e..df30d3d 100644
--- a/Zend/zend_compile.c
+++ b/Zend/zend_compile.c
@@ -2096,7 +2096,7 @@ void zend_do_pass_param(znode param, zend_uchar
op, int offset TSRMLS_DC) / {{

if (function_ptr) {
	if (ARG_MAY_BE_SENT_BY_REF(function_ptr, (zend_uint) offset)) {

  	if (param->op_type & (IS_VAR|IS_CV)) {

  	if (param->op_type & (IS_VAR|IS_CV) && param->u.EA.type !=

ZEND_PARSED_EXPR_NO_PASS_BY_REF) {
send_by_reference = 1;
if (op == ZEND_SEND_VAR && zend_is_function_or_method_call(param)) {
/* Method call */
diff --git a/Zend/zend_compile.h b/Zend/zend_compile.h
index 15f24df..475f976 100644
--- a/Zend/zend_compile.h
+++ b/Zend/zend_compile.h
@@ -650,6 +650,7 @@ int zendlex(znode *zendlval TSRMLS_DC);
#define ZEND_PARSED_VARIABLE (1<<4)
#define ZEND_PARSED_REFERENCE_VARIABLE (1<<5)
#define ZEND_PARSED_NEW (1<<6)
+#define ZEND_PARSED_EXPR_NO_PASS_BY_REF (1<<7)

/* unset types */
diff --git a/Zend/zend_language_parser.y b/Zend/zend_language_parser.y
index 1753e97..666b9e2 100644
--- a/Zend/zend_language_parser.y
+++ b/Zend/zend_language_parser.y
@@ -578,7 +578,7 @@ non_empty_for_expr:

expr_without_variable:
T_LIST '(' { zend_do_list_init(TSRMLS_C); } assignment_list ')' '='
expr { zend_do_list_end(&$$, &$7 TSRMLS_CC); }

| variable '=' expr { zend_check_writable_variable(&$1);
zend_do_assign(&$$, &$1, &$3 TSRMLS_CC); }

| variable '=' expr { zend_check_writable_variable(&$1);
zend_do_assign(&$$, &$1, &$3 TSRMLS_CC); $$.u.EA.type =
ZEND_PARSED_EXPR_NO_PASS_BY_REF; }
| variable '=' '&' variable { zend_check_writable_variable(&$1);
zend_do_end_variable_parse(&$4, BP_VAR_W, 1 TSRMLS_CC);
zend_do_end_variable_parse(&$1, BP_VAR_W, 0 TSRMLS_CC);
zend_do_assign_ref(&$$, &$1, &$4 TSRMLS_CC); }
| variable '=' '&' T_NEW class_name_reference {
zend_error(E_DEPRECATED, "Assigning the return value of new by reference
is deprecated"); zend_check_writable_variable(&$1);
zend_do_extended_fcall_begin(TSRMLS_C); zend_do_begin_new_object(&$4,
&$5 TSRMLS_CC); } ctor_arguments { zend_do_end_new_object(&$3, &$4, &$7
TSRMLS_CC); zend_do_extended_fcall_end(TSRMLS_C);
zend_do_end_variable_parse(&$1, BP_VAR_W, 0 TSRMLS_CC); $3.u.EA.type =
ZEND_PARSED_NEW; zend_do_assign_ref(&$$, &$1, &$3 TSRMLS_CC); }
| T_NEW class_name_reference { zend_do_extended_fcall_begin(TSRMLS_C);
zend_do_begin_new_object(&$1, &$2 TSRMLS_CC); } ctor_arguments {
zend_do_end_new_object(&$$, &$1, &$4 TSRMLS_CC);
zend_do_extended_fcall_end(TSRMLS_C);}

13 years ago by Etienne Kneuss — view source

unread

Hi,

When a built-in function is defined with ZEND_SEND_PREFER_REF, PHP will
issue a strict warning if you use an assignment expression as the parameter.

As an example, current() show this behaviour.

current() has this arginfo defined:

ZEND_BEGIN_ARG_INFO(arginfo_current, ZEND_SEND_PREFER_REF)
ZEND_ARG_INFO(ZEND_SEND_PREFER_REF, arg)
ZEND_END_ARG_INFO()

and a call like:

<?php
current($foo = array("bar"));
?>

Presents you with:

PHP Strict Standards: Only variables should be passed by reference in
%s on line %d

I think it is wrong to warn about this, because, to me, the PREFER
part of ZEND_SEND_PREFER_REF, conveys that we should prefer to pass by
reference, if possible, and not care otherwise.

The below patch implements the not care part that was missing until now.

This is done by having the lexer mark the result variable of the
assignment expression as not passable by reference, and changing the
function zend_do_pass_param() to ignore the marked variables when
considering if a parameter could be passed by reference or not.

I have run make test, before and after, with no regressions. The only
test affected was the one I sent earlier to specifically test for this bug.

I am, however, not sure if this is the right approach to solve the
problem, in which case, I hope to at least have put one of you on the
right track to the real solution, and to have made you interested in
fixing it properly.

The patch looks strange to me, why would you only consider stuff like
foo($a = 2) ? what about passing any other expressions:
foo(array(..)), foo(funcNotReturningARef()) etc... ?

To me it makes not much sense to distinguish different non-ref
expressions in that regard, they should all be handled the same.
Whether we want the actual error on some of our functions like
current()/end() etc.. is another question, but that should be fixed at
a totally different level.

The patch is for php-5.3.8

Daniel K.

diff --git a/Zend/zend_compile.c b/Zend/zend_compile.c
index c325a7e..df30d3d 100644
--- a/Zend/zend_compile.c
+++ b/Zend/zend_compile.c
@@ -2096,7 +2096,7 @@ void zend_do_pass_param(znode param, zend_uchar
op, int offset TSRMLS_DC) / {{

if (function_ptr) {
if (ARG_MAY_BE_SENT_BY_REF(function_ptr, (zend_uint) offset)) {

if (param->op_type & (IS_VAR|IS_CV)) {

if (param->op_type & (IS_VAR|IS_CV) && param->u.EA.type !=
ZEND_PARSED_EXPR_NO_PASS_BY_REF) {
send_by_reference = 1;
if (op == ZEND_SEND_VAR && zend_is_function_or_method_call(param)) {
/* Method call */
diff --git a/Zend/zend_compile.h b/Zend/zend_compile.h
index 15f24df..475f976 100644
--- a/Zend/zend_compile.h
+++ b/Zend/zend_compile.h
@@ -650,6 +650,7 @@ int zendlex(znode *zendlval TSRMLS_DC);
#define ZEND_PARSED_VARIABLE (1<<4)
#define ZEND_PARSED_REFERENCE_VARIABLE (1<<5)
#define ZEND_PARSED_NEW (1<<6)
+#define ZEND_PARSED_EXPR_NO_PASS_BY_REF (1<<7)

/* unset types */
diff --git a/Zend/zend_language_parser.y b/Zend/zend_language_parser.y
index 1753e97..666b9e2 100644
--- a/Zend/zend_language_parser.y
+++ b/Zend/zend_language_parser.y
@@ -578,7 +578,7 @@ non_empty_for_expr:

expr_without_variable:
T_LIST '(' { zend_do_list_init(TSRMLS_C); } assignment_list ')' '='
expr { zend_do_list_end(&$$, &$7 TSRMLS_CC); }

| variable '=' expr { zend_check_writable_variable(&$1);
zend_do_assign(&$$, &$1, &$3 TSRMLS_CC); }

| variable '=' expr { zend_check_writable_variable(&$1);
zend_do_assign(&$$, &$1, &$3 TSRMLS_CC); $$.u.EA.type =
ZEND_PARSED_EXPR_NO_PASS_BY_REF; }
| variable '=' '&' variable { zend_check_writable_variable(&$1);
zend_do_end_variable_parse(&$4, BP_VAR_W, 1 TSRMLS_CC);
zend_do_end_variable_parse(&$1, BP_VAR_W, 0 TSRMLS_CC);
zend_do_assign_ref(&$$, &$1, &$4 TSRMLS_CC); }
| variable '=' '&' T_NEW class_name_reference {
zend_error(E_DEPRECATED, "Assigning the return value of new by reference
is deprecated"); zend_check_writable_variable(&$1);
zend_do_extended_fcall_begin(TSRMLS_C); zend_do_begin_new_object(&$4,
&$5 TSRMLS_CC); } ctor_arguments { zend_do_end_new_object(&$3, &$4, &$7
TSRMLS_CC); zend_do_extended_fcall_end(TSRMLS_C);
zend_do_end_variable_parse(&$1, BP_VAR_W, 0 TSRMLS_CC); $3.u.EA.type =
ZEND_PARSED_NEW; zend_do_assign_ref(&$$, &$1, &$3 TSRMLS_CC); }
| T_NEW class_name_reference { zend_do_extended_fcall_begin(TSRMLS_C);
zend_do_begin_new_object(&$1, &$2 TSRMLS_CC); } ctor_arguments {
zend_do_end_new_object(&$$, &$1, &$4 TSRMLS_CC);
zend_do_extended_fcall_end(TSRMLS_C);}

--

--
Etienne Kneuss
http://www.colder.ch

13 years ago by Daniel K. — view source

unread

When a built-in function is defined with ZEND_SEND_PREFER_REF, PHP will
issue a strict warning if you use an assignment expression as the parameter.

The patch looks strange to me, why would you only consider stuff like
foo($a = 2) ? what about passing any other expressions:
foo(array(..)), foo(funcNotReturningARef()) etc... ?

foo(array(..)) is not a problem, as the op_type of 'array(..)' in this
context is IS_TMP_VAR, and thus not a candidate for passing by
reference, as far as zend_do_pass_param() is concerned.

For foo(funcNotReturningARef()) the lexer sets EA.type for the value of
funcNotReturningARef() to ZEND_PARSED_FUNCTION_CALL which in turn gets
checked in zend_do_pass_param(), just a few lines below where I added
the check to avoid the warning for assignment expressions (see below).

If ZEND_PARSED_FUNCTION_CALL is found, ZEND_ARG_SEND_SILENT is
eventually added to the opline, which in turn is checked for in
zend_vm_execute.c (ZEND_SEND_VAR_NO_REF_SPEC_VAR_HANDLER) where the
Strict warning is suppressed if ZEND_ARG_SEND_SILENT is set.

We could of course use the same mechanism, to just silence the warning,
but then the temporary variable holding the result of the assignment
expression would still be sent by reference, which I consider a bug.

I believe the right thing would be to pass it by value, to avoid nasty
surprises if any C code were to check if the argument was sent by
reference, and try to act accordingly.

To me it makes not much sense to distinguish different non-ref
expressions in that regard, they should all be handled the same.

It makes very much sense to make the distinction, as not all expressions
are made the same. Only IS_VAR and IS_CV ones are considered candidates
for passing by reference in zend_do_pass_param()

Whether we want the actual error on some of our functions like
current()/end() etc.. is another question, but that should be fixed at
a totally different level.

This might very well be, PHP internals is not my first language, and I
might be completely off the track here. Do you by any chance have an
opinion as to where this 'totally different level' might be?

   if (function_ptr) {
           if (ARG_MAY_BE_SENT_BY_REF(function_ptr, (zend_uint) offset)) {

                  if (param->op_type & (IS_VAR|IS_CV)) {

                  if (param->op_type & (IS_VAR|IS_CV) && param->u.EA.type != ZEND_PARSED_EXPR_NO_PASS_BY_REF) {
                         send_by_reference = 1;
                         if (op == ZEND_SEND_VAR && zend_is_function_or_method_call(param)) {
                                 /* Method call */

                                    op = ZEND_SEND_VAR_NO_REF;
                                    send_function = ZEND_ARG_SEND_FUNCTION | ZEND_ARG_SEND_SILENT;

zend_is_function_or_method_call(param) returns true if
ZEND_PARSED_FUNCTION_CALL is set by the lexer, and then the warning is
silenced by virtue of adding ZEND_ARG_SEND_SILENT to send_function.

I think my method, to set ZEND_PARSED_EXPR_NO_PASS_BY_REF is in the same
vein as the similar existing method, and sufficient to avoid the buggy
behaviour.

Daniel K.

13 years ago by Daniel K. — view source

unread

When a built-in function is defined with ZEND_SEND_PREFER_REF, PHP will
issue a strict warning if you use an assignment expression as the parameter.

The patch looks strange to me, why would you only consider stuff like
foo($a = 2) ? what about passing any other expressions:
foo(array(..)), foo(funcNotReturningARef()) etc... ?

[SNIP Lengthy description of why the patch is right]

To me it makes not much sense to distinguish different non-ref
expressions in that regard, they should all be handled the same.

It makes very much sense to make the distinction, as not all expressions
are made the same. Only IS_VAR and IS_CV ones are considered candidates
for passing by reference in zend_do_pass_param()

However, it can be done with less complexity, we only have to make sure
that the function arguments that the lexer sends to zend_do_pass_param
is not promoted to be sent by reference if the lexer already has decided
that you did not specify a variable. See: Zend/zend_language_parser.y ->
non_empty_function_call_parameter_list:

In other words, only consider promoting to send by reference if the
argument passed to the function is not of type ZEND_SEND_VAL.

--- a/Zend/zend_compile.c
+++ b/Zend/zend_compile.c
@@ -2096,7 +2096,7 @@ void zend_do_pass_param(znode param, zend_uchar
op, int offset TSRMLS_DC) / {{

if (function_ptr) {
	if (ARG_MAY_BE_SENT_BY_REF(function_ptr, (zend_uint) offset)) {

  	if (param->op_type & (IS_VAR|IS_CV)) {

  	if (param->op_type & (IS_VAR|IS_CV) && original_op != ZEND_SEND_VAL) {
  		send_by_reference = 1;
  		if (op == ZEND_SEND_VAR && zend_is_function_or_method_call(param)) {
  			/* Method call */

Thanks for questioning my patch, and making me recheck what I was doing,
so that this minimal patch could come to life.

This still passes the test-suite with flying colours.

As a bonus this version of the fix applies to both 5.3-latest and
5.4-latest.

Whether we want the actual error on some of our functions like
current()/end() etc.. is another question, but that should be fixed at
a totally different level.

I think I misunderstood you last time around.

I am of the opinion that the strict warning in question is completely
bogus. If the arginfo says: ZEND_SEND_PREFER_REF, Zend had better shut
up when a value is passed.

Daniel K.

13 years ago by Tjerk Meesters — view source

unread

Hi,

Ran into this exact issue just the other day. In the pre-5.4 days, to get
the first element of a returned array I would use current(). I still think
that's correct and shouldn't raise a digital eyebrow.

Hi,

When a built-in function is defined with ZEND_SEND_PREFER_REF, PHP will
issue a strict warning if you use an assignment expression as the
parameter.

As an example, current() show this behaviour.

current() has this arginfo defined:

ZEND_BEGIN_ARG_INFO(arginfo_current, ZEND_SEND_PREFER_REF)
ZEND_ARG_INFO(ZEND_SEND_PREFER_REF, arg)
ZEND_END_ARG_INFO()

and a call like:

<?php
current($foo = array("bar"));
?>

Presents you with:

PHP Strict Standards: Only variables should be passed by reference in
%s on line %d

I think it is wrong to warn about this, because, to me, the PREFER
part of ZEND_SEND_PREFER_REF, conveys that we should prefer to pass by
reference, if possible, and not care otherwise.

The below patch implements the not care part that was missing until now.

This is done by having the lexer mark the result variable of the
assignment expression as not passable by reference, and changing the
function zend_do_pass_param() to ignore the marked variables when
considering if a parameter could be passed by reference or not.

I have run make test, before and after, with no regressions. The only
test affected was the one I sent earlier to specifically test for this
bug.

I am, however, not sure if this is the right approach to solve the
problem, in which case, I hope to at least have put one of you on the
right track to the real solution, and to have made you interested in
fixing it properly.

The patch looks strange to me, why would you only consider stuff like
foo($a = 2) ? what about passing any other expressions:
foo(array(..)), foo(funcNotReturningARef()) etc... ?

To me it makes not much sense to distinguish different non-ref
expressions in that regard, they should all be handled the same.
Whether we want the actual error on some of our functions like
current()/end() etc.. is another question, but that should be fixed at
a totally different level.
The patch is for php-5.3.8

Daniel K.

diff --git a/Zend/zend_compile.c b/Zend/zend_compile.c
index c325a7e..df30d3d 100644
--- a/Zend/zend_compile.c
+++ b/Zend/zend_compile.c
@@ -2096,7 +2096,7 @@ void zend_do_pass_param(znode param, zend_uchar
op, int offset TSRMLS_DC) / {{
   if (function_ptr) {
           if (ARG_MAY_BE_SENT_BY_REF(function_ptr, (zend_uint)

offset)) {

                  if (param->op_type & (IS_VAR|IS_CV)) {

                  if (param->op_type & (IS_VAR|IS_CV) &&

param->u.EA.type !=

ZEND_PARSED_EXPR_NO_PASS_BY_REF) {
send_by_reference = 1;
if (op == ZEND_SEND_VAR &&
zend_is_function_or_method_call(param)) {
/* Method call */
diff --git a/Zend/zend_compile.h b/Zend/zend_compile.h
index 15f24df..475f976 100644
--- a/Zend/zend_compile.h
+++ b/Zend/zend_compile.h
@@ -650,6 +650,7 @@ int zendlex(znode *zendlval TSRMLS_DC);
#define ZEND_PARSED_VARIABLE (1<<4)
#define ZEND_PARSED_REFERENCE_VARIABLE (1<<5)
#define ZEND_PARSED_NEW (1<<6)
+#define ZEND_PARSED_EXPR_NO_PASS_BY_REF (1<<7)

/* unset types */
diff --git a/Zend/zend_language_parser.y b/Zend/zend_language_parser.y
index 1753e97..666b9e2 100644
--- a/Zend/zend_language_parser.y
+++ b/Zend/zend_language_parser.y
@@ -578,7 +578,7 @@ non_empty_for_expr:

expr_without_variable:
T_LIST '(' { zend_do_list_init(TSRMLS_C); }
assignment_list ')' '='
expr { zend_do_list_end(&$$, &$7 TSRMLS_CC); }
  |       variable '=' expr               {

zend_check_writable_variable(&$1);

zend_do_assign(&$$, &$1, &$3 TSRMLS_CC); }
  |       variable '=' expr               {

zend_check_writable_variable(&$1);

zend_do_assign(&$$, &$1, &$3 TSRMLS_CC); $$.u.EA.type =
ZEND_PARSED_EXPR_NO_PASS_BY_REF; }
| variable '=' '&' variable {
zend_check_writable_variable(&$1);
zend_do_end_variable_parse(&$4, BP_VAR_W, 1 TSRMLS_CC);
zend_do_end_variable_parse(&$1, BP_VAR_W, 0 TSRMLS_CC);
zend_do_assign_ref(&$$, &$1, &$4 TSRMLS_CC); }
| variable '=' '&' T_NEW class_name_reference {
zend_error(E_DEPRECATED, "Assigning the return value of new by reference
is deprecated"); zend_check_writable_variable(&$1);
zend_do_extended_fcall_begin(TSRMLS_C); zend_do_begin_new_object(&$4,
&$5 TSRMLS_CC); } ctor_arguments { zend_do_end_new_object(&$3, &$4, &$7
TSRMLS_CC); zend_do_extended_fcall_end(TSRMLS_C);
zend_do_end_variable_parse(&$1, BP_VAR_W, 0 TSRMLS_CC); $3.u.EA.type =
ZEND_PARSED_NEW; zend_do_assign_ref(&$$, &$1, &$3 TSRMLS_CC); }
| T_NEW class_name_reference {
zend_do_extended_fcall_begin(TSRMLS_C);
zend_do_begin_new_object(&$1, &$2 TSRMLS_CC); } ctor_arguments {
zend_do_end_new_object(&$$, &$1, &$4 TSRMLS_CC);
zend_do_extended_fcall_end(TSRMLS_C);}

--

--
Etienne Kneuss
http://www.colder.ch

13 years ago by Daniel K. — view source

unread

When a built-in function is defined with ZEND_SEND_PREFER_REF, PHP will
issue a strict warning if you use an assignment expression as the parameter.

As an example, current() show this behaviour.

<?php
current($foo = array("bar"));
?>

Presents you with:

PHP Strict Standards: Only variables should be passed by reference in
%s on line %d

A patch was appended, discussed, and improved, and I have uploaded a
test-case, as well as a minimal patch that fixes the problem (attached)
to the original bug-report.

https://bugs.php.net/bug.php?id=55754

The patch still applies to trunk, and I think it should be applied to
the 5.3 and 5.4 branches as well.

What to do next?

The feedback has been constructive, but I feel it has stagnated a bit.
Of course, you may all be busy with other interesting stuff, but is
there anything else you require from me to get this rolling?

Should I explain better why I think it's an issue, and why the patch is
needed?

An indication as to if anyone is reviewing the proposed patch, and
considering applying it, or telling me that this is the completely wrong
approach to solve the problem and dropping a few hints would be most
appreciated, but any feedback is welcome.

Daniel K.

13 years ago by Stas Malyshev — view source

unread

Hi!

A patch was appended, discussed, and improved, and I have uploaded a
test-case, as well as a minimal patch that fixes the problem (attached)
to the original bug-report.

https://bugs.php.net/bug.php?id=55754

The patch still applies to trunk, and I think it should be applied to
the 5.3 and 5.4 branches as well.

As far as I can see, the patch looks good, so it can be applied to 5.4
and trunk. If I don't hear any objections and nobody beats me to it,
I'll do it in a couple of days (a bit busy now).

Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

13 years ago by Daniel K. — view source

unread

A patch was appended, discussed, and improved, and I have uploaded a
test-case, as well as a minimal patch that fixes the problem (attached)
to the original bug-report.

https://bugs.php.net/bug.php?id=55754

The patch still applies to trunk, and I think it should be applied to
the 5.3 and 5.4 branches as well.

As far as I can see, the patch looks good, so it can be applied to 5.4
and trunk. If I don't hear any objections and nobody beats me to it,
I'll do it in a couple of days (a bit busy now).

OK, thanks.

But what about 5.3? there is no ABI issue with this, just a spurious
warning that goes away.

Daniel K.

13 years ago by johannes@schlueters.de — view source

unread

A patch was appended, discussed, and improved, and I have uploaded a
test-case, as well as a minimal patch that fixes the problem (attached)
to the original bug-report.

https://bugs.php.net/bug.php?id=55754

The patch still applies to trunk, and I think it should be applied to
the 5.3 and 5.4 branches as well.

As far as I can see, the patch looks good, so it can be applied to 5.4
and trunk. If I don't hear any objections and nobody beats me to it,
I'll do it in a couple of days (a bit busy now).

OK, thanks.

But what about 5.3? there is no ABI issue with this, just a spurious
warning that goes away.

I think 5.3 is fine, too. But please extend the test a bit. The current
one will pass even if the process segfaults. At least print something at
the end.

johannes

13 years ago by Daniel K. — view source

unread

But please extend the test a bit. The current
one will pass even if the process segfaults. At least print something at
the end.

Ah, so that's why some of the tests have 'echo "DONE";' at the end.

New test-case uploaded, and attached.

Daniel K.

13 years ago by Pierre Joye — view source

unread

hi,

2011/10/6 Johannes Schlüter johannes@schlueters.de:

I think 5.3 is fine, too. But please extend the test a bit. The current
one will pass even if the process segfaults. At least print something at
the end.

I think it is not safe for 5.3. While the patch looks trivial or
harmless, it is in an area where the consequences of a new bug, even
for an edge case, can be very bad. The ratio risk/benefit is high,
especially when it is only about fixing a strict warning. I would
rather wait (maybe 5.4 last RC or final) before applying it to 5.3. We
don't need a 5.3.7/8 again :)

Cheers,

Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

13 years ago by Daniel K. — view source

unread

especially when it is only about fixing a strict warning.

It's not just about silencing a strict warning. It's about preventing
non-obvious fuck-ups in extensions as well, as I alluded to earlier in
the thread.

Consider an extension implementing a function like:

ZEND_BEGIN_ARG_INFO(arginfo_test, ZEND_SEND_PREFER_REF)
ZEND_END_ARG_INFO()

zend_function_entry test_functions[] = {
ZEND_FE(test, arginfo_test)
{NULL, NULL, NULL}
};

ZEND_FUNCTION(test)
{
zval ***argv;
int i, is_ref;
int argc = ZEND_NUM_ARGS();

argv = malloc(sizeof(zval **) * argc);

zend_get_parameters_array_ex(argc, argv);

for (i = 0; i < argc; i++) {
	is_ref	= PZVAL_IS_REF(*(argv[i]));

	if (is_ref) {
		/* Let's update it, and possibly
		   be in for a nice surprise */
		if (Z_TYPE_PP(argv[i]) == IS_LONG)
			Z_LVAL_PP(argv[i])++;
		if (Z_TYPE_PP(argv[i]) == IS_STRING)
			ZVAL_STRING(*argv[i], "surprise", 1);
	}
	zend_error(E_USER_NOTICE,
		"Param %d was%s received by ref",
		i, is_ref ? "" : " not");
}

free(argv);

RETURN_TRUE;

}

Then doing...

php -d extension=test.so -r '$a = 1; $b = 2; $str1 = "foo";
test($str1, $str2 = "bar", $a, $b = 3, NULL, "str");
echo "a: $a, b: $b, str1: $str1, str2: $str2\n";'

...would currently result in:

===
Notice: Param 0 was received by ref in Command line code on line 1

Notice: Param 1 was received by ref in Command line code on line 1

Notice: Param 2 was received by ref in Command line code on line 1

Notice: Param 3 was received by ref in Command line code on line 1

a: 2, b: 4, str1: surprise, str2: surprise

Which I think is a bit surprising in the case of 'b' and 'str2', whereas
after my patch it would be:

===
Notice: Param 0 was received by ref in Command line code on line 1

Notice: Param 1 was not received by ref in Command line code on line 1

Notice: Param 2 was received by ref in Command line code on line 1

Notice: Param 3 was not received by ref in Command line code on line 1

a: 2, b: 3, str1: surprise, str2: bar

Which I think is not as surprising.

I would
rather wait (maybe 5.4 last RC or final) before applying it to 5.3. We
don't need a 5.3.7/8 again :)

Sure, it does not seem like people have been tripping over this left and
right, and given the change in behaviour, it may not be worth the risk.
That said, I'd be very surprised, and a bit sad if anyone is relying
on the current buggy behaviour.

Daniel K.

13 years ago by Stas Malyshev — view source

unread

Hi!

But what about 5.3? there is no ABI issue with this, just a spurious
warning that goes away.

Yeah, I agree with Pierre - it's not a huge problem, and the risk is
there, so I'd hold it for 5.3 as it is the stable version. When 5.4 gets
tested enough that we're confident this fix is 100% fine, we may
backport then maybe.

Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

13 years ago by Etienne Kneuss — view source

unread

Hi,

When a built-in function is defined with ZEND_SEND_PREFER_REF, PHP will
issue a strict warning if you use an assignment expression as the parameter.

As an example, current() show this behaviour.

<?php
current($foo = array("bar"));
?>

Presents you with:

PHP Strict Standards: Only variables should be passed by reference in
%s on line %d

A patch was appended, discussed, and improved, and I have uploaded a
test-case, as well as a minimal patch that fixes the problem (attached)
to the original bug-report.

https://bugs.php.net/bug.php?id=55754

The patch still applies to trunk, and I think it should be applied to
the 5.3 and 5.4 branches as well.

The new patch seems to be indeed better. My only concern is whether it
covers all those cases now. Did you try passing all possible kinds of
value expressions ? Or is it guaranteed to cover all cases by design?
If not, we should probably add more of those cases in the tests.

Other than that, this looks fine.

What to do next?

The feedback has been constructive, but I feel it has stagnated a bit.
Of course, you may all be busy with other interesting stuff, but is
there anything else you require from me to get this rolling?

Should I explain better why I think it's an issue, and why the patch is
needed?

An indication as to if anyone is reviewing the proposed patch, and
considering applying it, or telling me that this is the completely wrong
approach to solve the problem and dropping a few hints would be most
appreciated, but any feedback is welcome.

Daniel K.

--
Etienne Kneuss
http://www.colder.ch

13 years ago by Daniel K. — view source

unread

The patch still applies to trunk, and I think it should be applied to
the 5.3 and 5.4 branches as well.

The new patch seems to be indeed better. My only concern is whether it
covers all those cases now. Did you try passing all possible kinds of
value expressions ? Or is it guaranteed to cover all cases by design?
If not, we should probably add more of those cases in the tests.

It now covers all possible cases by design. The patched function
(zend_do_pass_param) is only called from the language parser, and in
Zend/zend_language_parser.y zend_do_pass_param() is called with the
second parameter set to ZEND_SEND_VAL for all expr_without_variable type
parameters.

From php5.4-201110061230/Zend/zend_language_parser.y: line 538-541

non_empty_function_call_parameter_list:
expr_without_variable { Z_LVAL($$.u.constant) = 1;
zend_do_pass_param(&$1, ZEND_SEND_VAL, Z_LVAL($$.u.constant) TSRMLS_CC); }
| variable { Z_LVAL($$.u.constant) = 1;
zend_do_pass_param(&$1, ZEND_SEND_VAR, Z_LVAL($$.u.constant) TSRMLS_CC); }
| '&' w_variable { Z_LVAL($$.u.constant) = 1;
zend_do_pass_param(&$2, ZEND_SEND_REF, Z_LVAL($$.u.constant) TSRMLS_CC); }
[...]

So your concern should be addressed.

Other than that, this looks fine.

Super.

Daniel K.

[PATCH] Fix for bug #55754

As far as I can see, the patch looks good, so it can be applied to 5.4 and trunk. If I don't hear any objections and nobody beats me to it, I'll do it in a couple of days (a bit busy now).

Cheers,

a: 2, b: 4, str1: surprise, str2: surprise

a: 2, b: 3, str1: surprise, str2: bar

Yeah, I agree with Pierre - it's not a huge problem, and the risk is there, so I'd hold it for 5.3 as it is the stable version. When 5.4 gets tested enough that we're confident this fix is 100% fine, we may backport then maybe.

As far as I can see, the patch looks good, so it can be applied to 5.4
and trunk. If I don't hear any objections and nobody beats me to it,
I'll do it in a couple of days (a bit busy now).

Yeah, I agree with Pierre - it's not a huge problem, and the risk is
there, so I'd hold it for 5.3 as it is the stable version. When 5.4 gets
tested enough that we're confident this fix is 100% fine, we may
backport then maybe.