FAQ
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);}

Search Discussions

  • Etienne Kneuss at Sep 23, 2011 at 12:28 pm
    Hi,
    On Fri, Sep 23, 2011 at 14:06, Daniel K. wrote:
    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);}

    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php


    --
    Etienne Kneuss
    http://www.colder.ch
  • Daniel K. at Sep 23, 2011 at 1:41 pm

    On 09/23/2011 02:28 PM, Etienne Kneuss wrote:
    On Fri, Sep 23, 2011 at 14:06, Daniel K. wrote:
    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?

    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 */
    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.
  • Tjerk Meesters at Sep 23, 2011 at 2:04 pm
    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.
    On Sep 23, 2011 8:30 PM, "Etienne Kneuss" wrote:
    Hi,
    On Fri, Sep 23, 2011 at 14:06, Daniel K. wrote:
    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);}

    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php


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

    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php
  • Daniel K. at Sep 23, 2011 at 6:52 pm

    On 09/23/2011 03:41 PM, Daniel K. wrote:
    On 09/23/2011 02:28 PM, Etienne Kneuss wrote:
    On Fri, Sep 23, 2011 at 14:06, Daniel K. wrote:
    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.
  • Daniel K. at Oct 6, 2011 at 3:31 pm

    On 09/23/2011 02:06 PM, I wrote:
    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.
  • Stas Malyshev at Oct 6, 2011 at 3:55 pm
    Hi!
    On 10/6/11 5:31 PM, Daniel K. wrote:
    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
  • Daniel K. at Oct 6, 2011 at 3:59 pm

    On 10/06/2011 05:55 PM, Stas Malyshev wrote:
    On 10/6/11 5:31 PM, Daniel K. wrote:
    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.
  • Etienne Kneuss at Oct 6, 2011 at 4:03 pm
    Hi,
    On Thu, Oct 6, 2011 at 17:31, Daniel K. wrote:
    On 09/23/2011 02:06 PM, I wrote:
    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
  • Johannes Schlüter at Oct 6, 2011 at 4:16 pm

    On Thu, 2011-10-06 at 17:59 +0200, Daniel K. wrote:
    On 10/06/2011 05:55 PM, Stas Malyshev wrote:
    On 10/6/11 5:31 PM, Daniel K. wrote:
    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
  • Daniel K. at Oct 6, 2011 at 4:26 pm

    On 10/06/2011 06:03 PM, Etienne Kneuss wrote:
    On Thu, Oct 6, 2011 at 17:31, Daniel K. wrote:
    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.
  • Daniel K. at Oct 6, 2011 at 4:59 pm

    On 10/06/2011 06:15 PM, Johannes Schlüter wrote:
    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.
  • Pierre Joye at Oct 7, 2011 at 8:05 am
    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,
  • Stas Malyshev at Oct 7, 2011 at 9:27 am
    Hi!
    On 10/6/11 5:59 PM, Daniel K. wrote:
    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
  • Daniel K. at Oct 7, 2011 at 6:51 pm

    On 10/07/2011 10:04 AM, Pierre Joye wrote:
    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.

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupphp-internals @
categoriesphp
postedSep 23, '11 at 12:06p
activeOct 7, '11 at 6:51p
posts15
users6
websitephp.net

People

Translate

site design / logo © 2022 Grokbase