Throw in FFI::addr() when referencing temporary pointer

[iluuu1994]

We're only disallowing pointers, as they always rely not only on the data but also the CData.ptr_holder to be there. Other unowned structures should be allowed, like structs, even if refcount == 1. Targeting master as this is an improvement rather than a bugfix.

[bwoebi]

I think that's okay. At least I cannot imagine any possible value from taking the address of a rc=1 adress.

[iluuu1994]

This could probably be further improved by not allowing owned data with refcount=1. I'll see if I can add that check as well. Nevermind. The ownership is transferred onto the pointer, so this should be fine. This behavior was untested, so I added a test.

[dstogov]

This looks fine.
The error message might be done more clear e.g. "Cannot take FFI::addr() from temporary poiner", but I'm not sure.

[bwoebi]

I just tried upgrading my FFI code to PHP 8.3 and it ... blew up, thanks to this.

More precisely, my code looks like FFI::addr($cdata->struct_member)[0] = $value;. (The addr()[0] assignment works around some FFI peculiarities regarding assignment of values directly.)
The $cdata->struct_member is a RC=1 value here and thus matches the issue here.

It can be worked around by assigning $cdata->struct_member to a temp var by reference, but that's even uglier?

What this PR probably wanted to solve is catching when the owned value is destroyed. I'm not sure how to do that, but I think we may need to revert the arbitrarily restrictive behaviour of this PR?

(The irony that I initially approved this is not lost on me.)

[dstogov]

@bwoebi could you please provide the complete reproduction case.

[bwoebi]

Sure @dstogov, simplified reproducer:

~/php-src/sapi/cli/php -r '$f = FFI::cdef("typedef struct { char *bar; } other;"); $t = $f->new("other"); $data = $f->new("char[2]"); $data[0] = "1"; FFI::addr($_ = &$t->bar)[0] = $f->cast("char*", FFI::addr($data)); var_dump($t);'
object(FFI\CData:struct <anonymous>)#2 (1) {
  ["bar"]=>
  object(FFI\CData:char*)#5 (1) {
    [0]=>
    string(1) "1"
  }
}

~/php-src/sapi/cli/php -r '$f = FFI::cdef("typedef struct { char *bar; } other;"); $t = $f->new("other"); $data = $f->new("char[2]"); $data[0] = "1"; FFI::addr($t->bar)[0] = $f->cast("char*", FFI::addr($data)); var_dump($t);'

Fatal error: Uncaught FFI\Exception: FFI::addr() cannot create a reference to a temporary pointer in Command line code:1
Stack trace:
#0 Command line code(1): FFI::addr(Object(FFI\CData:char*))
#1 {main}
  thrown in Command line code on line 1

Note the difference, in the latter case the &$_ = is missing

[dstogov]

@bwoebi I wonder, why do you need these addr. The following code (with addr removed) works fine

$  sapi/cli/php -r '$f = FFI::cdef("typedef struct { char *bar; } other;"); $t = $f->new("other"); $data = $f->new("char[2]"); $data[0] = "1"; $t->bar = $f->cast("char*", $data); var_dump($t);'

object(FFI\CData:struct <anonymous>)#2 (1) {
  ["bar"]=>
  object(FFI\CData:char*)#4 (1) {
    [0]=>
    string(1) "1"
  }
}

This code works in PHP-7.4 and master

[bwoebi]

@dstogov I ended up with a RC=1 reference from a VAR, this was just simplified to illustrate the problem, let's say:

$f = FFI::cdef("typedef struct { char *bar; } other;");
class Container {
    public $data;
    function __construct($f) { $this->data = $f->new("other"); }
    function &getBar() { return $this->data->bar; } // return by ref to get CData instead of null
}
$container = new Container($f);
$data = $f->new("char[2]");
$data[0] = "1";
FFI::addr($container->getBar())[0] = $f->cast("char*", $data); // directly write it
var_dump($container);

Now, certainly $t = $container->getBar(); FFI::addr($t)[0] = $f->cast("char*", $data); works too, but just saying, this is where I encountered the problem in my code.
It's a weird error message to encounter in seemingly accurate code. At least I was quite baffled by the error message, before looking up the source and understanding why it triggers. And - obviously - it generally is fine, except if the returned struct member happens to be a pointer.

[bwoebi]

But I generally need the addr trick to assign the contents directly to a variable, if it's a struct or array member, then the dim&prop write handlers handle it, but obviously $cdata = $otherCdata; does not assign the value to the cdata, but just replaces the variable content.

[dstogov]

I see. This probably may be fixed by the following patch

diff --git a/ext/ffi/ffi.c b/ext/ffi/ffi.c
index f91092ab03b..daca017f69f 100644
--- a/ext/ffi/ffi.c
+++ b/ext/ffi/ffi.c
@@ -4290,7 +4290,8 @@ ZEND_METHOD(FFI, addr) /* {{{ */
        cdata = (zend_ffi_cdata*)Z_OBJ_P(zv);
        type = ZEND_FFI_TYPE(cdata->type);
 
-       if (GC_REFCOUNT(&cdata->std) == 1 && Z_REFCOUNT_P(arg) == 1 && type->kind == ZEND_FFI_TYPE_POINTER) {
+       if (GC_REFCOUNT(&cdata->std) == 1 && Z_REFCOUNT_P(arg) == 1 && type->kind == ZEND_FFI_TYPE_POINTER
+        && cdata->ptr_holder) {
                zend_throw_error(zend_ffi_exception_ce, "FFI::addr() cannot create a reference to a temporary pointer");
                RETURN_THROWS();
        }

@iluuu1994 what do you think?

[iluuu1994]

@dstogov Your patch doesn't fix Bobs example for me. TBH I'm not very familiar with FFI. I remember a use-after-free for a Symfony test that used FFI:

$struct = $ffi->new('Foo');
$structPtrPtr = \FFI::addr(\FFI::addr($struct));
var_dump($structPtrPtr);

I don't know how to fix Bobs test, so I'm ok with just reverting this change. FFI is inherently unsafe, so users are responsible for making sure the data survives.

[dstogov]

@dstogov Your patch doesn't fix Bobs example for me

Are you sure? I just verified this again and it works.

<?php
$f = FFI::cdef("typedef struct { char *bar; } other;");
class Container {
    public $data;
    function __construct($f) { $this->data = $f->new("other"); }
    function &getBar() { return $this->data->bar; } // return by ref to get CData instead of null
}
$container = new Container($f);
$data = $f->new("char[2]");
$data[0] = "1";
FFI::addr($container->getBar())[0] = $f->cast("char*", $data); // directly write it
var_dump($container);

I remember a use-after-free for a Symfony test that used FFI: $structPtrPtr = \FFI::addr(\FFI::addr($struct));

My patch doesn't affect \FFI::addr(\FFI::addr()) case.

[dstogov]

I see, this doesn't fix the other case:

~/php-src/sapi/cli/php -r '$f = FFI::cdef("typedef struct { char *bar; } other;"); $t = $f->new("other"); $data = $f->new("char[2]"); $data[0] = "1"; FFI::addr($t->bar)[0] = $f->cast("char*", FFI::addr($data)); var_dump($t);'

[iluuu1994]

I just verified this again and it works.

🤔 Yes, it fails for the code you just posted below for me.

I see, this doesn't fix the other case:

I didn't even try this one.

[dstogov]

@bwoebi will the patch posted above help you? Or it's not enough anyway.
As I showed, you didn't have to use FFI::addr() in the first case.

[bwoebi]

@dstogov Yes, this helps me. The other case just happened as part of the reproducer.

[iluuu1994]

Well, that patch still doesn't work for me ^^

<?php
$f = FFI::cdef("typedef struct { char *bar; } other;");
class Container {
    public $data;
    function __construct($f) { $this->data = $f->new("other"); }
    function &getBar() { return $this->data->bar; } // return by ref to get CData instead of null
}
$container = new Container($f);
$data = $f->new("char[2]");
$data[0] = "1";
FFI::addr($container->getBar())[0] = $f->cast("char*", $data); // directly write it
var_dump($container);
?>

Fatal error: Uncaught FFI\Exception: FFI::addr() cannot create a reference to a temporary pointer in %s:%d
Stack trace:
#0 %s(%d): FFI::addr(Object(FFI\CData:char*))
#1 {main}
  thrown in %s on line %d

I tried on PHP-8.3 and master. But if it works for you then great 🤷‍♂️ 😄

[bwoebi]

No, I meant, it's enough if this fixes the latter case. I can test later.

[dstogov]

@bwoebi the problem should be fixed now