Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug "Parameter 1 to ... expected to be a reference, value given" if nested function call #73

Closed
ASchmidt1 opened this issue Mar 20, 2018 · 6 comments

Comments

@ASchmidt1
Copy link

The following code:

<?php
declare(strict_types=1);

function myfunction( &$anobject ) { return 'success'; }

function callmyfunction( $args ) { 
    var_dump( call_user_func_array( 'myfunction', $args ) .'2' ); // fails!
    var_dump( call_user_func_array( 'myfunction', array( &$args[0] ) ) .'3'); //circumvention
}

class MyClass 
{
    function __construct() {
        var_dump( call_user_func_array( 'myfunction', array( &$this ) ) .'1' ); //works
        callmyfunction( array( &$this ) ); // loses array-embedded reference!
    }
}
$myobject = new MyClass;

will result in:

Parameter 1 to myfunction() expected to be a reference, value given in sample.php on line 7
Stack trace:
1. {main}() sample.php:0
2. MyClass->__construct() sample.php:18
3. callmyfunction() sample.php:15
4. call_user_func_array:{sample.php:7}() sample.php:7
@mszabo-wikia
Copy link
Contributor

@ASchmidt1 Are you sure you are not running afoul of the changed handling of $this in PHP 7.1 and above?

@cmb69
Copy link
Collaborator

cmb69 commented Jul 4, 2018

The code works as expected without uopz: https://3v4l.org/QXr3i

@ASchmidt1
Copy link
Author

@mszabo-wikia Confirmed with PHP 7.2.3. Without UOPZ extension, ONLY this code (using &$this) will work and NO issue warnings:

var_dump( call_user_func_array( 'myfunction', array( &$this ) ) .'1' ); 
callmyfunction( array( &$this ) ); 

If you do NOT use a $this reference, then PHP 7.2.3 without UOPZ will return warnings:
PHP Warning: Parameter 1 to myfunction() expected to be a reference, value given.

So native PHP works, and ONLY works, if &$this is used in BOTH lines.

However, the moment UOPZ is active, that working PHP 7.2.3 code fails.

@mszabo-wikia
Copy link
Contributor

mszabo-wikia commented Jul 15, 2018

Interesting. Based on PHP bug 73751 I'd have expected such usage of $this to trigger a warning irrespective of uopz being present. Maybe PHP's handling here is not fully consistent?

@goshlanguage
Copy link

goshlanguage commented Oct 7, 2018

Running the code above is successful regardless of PHP version:

▶ docker run --rm -v $(pwd)/test:/root/test php:5.6 php /root/test/test.php
string(8) "success1"
string(8) "success2"
string(8) "success3"

~
▶ docker run --rm -v $(pwd)/test:/root/test php:7.0 php /root/test/test.php
string(8) "success1"
string(8) "success2"
string(8) "success3"

~
▶ docker run --rm -v $(pwd)/test:/root/test php:7.2 php /root/test/test.php
string(8) "success1"
string(8) "success2"
string(8) "success3"

This issue specifically affects the greater Wordpress community, would love to help, but the comments are pretty terse regarding PHP_FUNCTION(uopz_call_user_func)

Is it possible that the parameter copying in util.c (uopz_caller_init) is mutating the parameters, or when its copying the function from the hashtable, that it's losing some bit of metadata, such as that the parameter should be a reference?

I also looked into the zpp api to see if it could be an issue with the zend data types, which I'm still not convinced couldn't be the issue.

@krakjoe Do you have office hours to discuss?

@goshlanguage
Copy link

I and a colleague spent a good amount of time debugging this with gdb in hopes we could spot the issue. We even tried commenting out various bits of functionality we suspected within UOPZ, such as the callers_init bits that perform the memcpy, and much more. Each time we reran the test by @ASchmidt1 above on Php7.2, it failed, but building the same code against the Zend libs in 7.0 seems to pass.

Dockerfile:

FROM php:7.0

RUN mkdir -p /usr/local/src/uopz
COPY . /usr/local/src/uopz

WORKDIR /usr/local/src/uopz
RUN phpize && \
    ./configure && \
    make && \
    make test && \
    make install && \
    echo ";priority=5" > /usr/local/etc/php/conf.d/uopz.ini && \
    echo "extension=uopz.so" >> /usr/local/etc/php/conf.d/uopz.ini

CMD sleep 3600

After running docker build -t uopz7 . ; docker run -d --rm --name uopz7 uopz7

  gdbtest  docker exec -ti uopz7 bash
root@c7b164ae20d9:/usr/local/src/uopz# php -v
PHP 7.0.32 (cli) (built: Sep 15 2018 04:37:22) ( NTS )
Copyright (c) 1997-2017 The PHP Group
Zend Engine v3.0.0, Copyright (c) 1998-2017 Zend Technologies

root@c7b164ae20d9:/usr/local/src/uopz# php -m
[PHP Modules]
...
uopz

root@c7b164ae20d9:/usr/local/src/uopz# php test/test.php
string(8) "success1"
string(8) "success2"
string(8) "success3"

At this point, we suspect that UOPZ could be exposing a bug within Zend, but don't have a definitive example that proves it.

We see Zend calls in the backtraces within gdb while calling the mostly commented out bits of UOPZ, which still produces failures on php7.2.

@krakjoe krakjoe mentioned this issue Jan 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants