Skip to content

FFI use after free for nested FFI::addr() calls #9598

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

Closed
iluuu1994 opened this issue Sep 22, 2022 · 2 comments
Closed

FFI use after free for nested FFI::addr() calls #9598

iluuu1994 opened this issue Sep 22, 2022 · 2 comments

Comments

@iluuu1994
Copy link
Member

iluuu1994 commented Sep 22, 2022

Description

The following code:

<?php

$ffi = \FFI::cdef(<<<'CPP'
typedef struct {
    int8_t a;
} Example;
CPP);

$struct = $ffi->new('Example');

// This works fine
// $structPtr = \FFI::addr($struct);
// $structPtrPtr = \FFI::addr($structPtr);

// This does not
$structPtrPtr = \FFI::addr(\FFI::addr($struct));

var_dump($structPtrPtr[0][0]);

Resulted in this output:

==41372==ERROR: AddressSanitizer: heap-use-after-free on address 0x6080000155e8 at pc 0x000000abe914 bp 0x7fff49086750 sp 0x7fff49086748
READ of size 8 at 0x6080000155e8 thread T0
    #0 0xabe913 in zend_ffi_cdata_to_zval /home/ilutov/Developer/php-src/ext/ffi/ffi.c:536
    #1 0xac9a12 in zend_ffi_cdata_read_dim /home/ilutov/Developer/php-src/ext/ffi/ffi.c:1358
    #2 0x199e053 in zend_fetch_dimension_address_read /home/ilutov/Developer/php-src/Zend/zend_execute.c:2776
    #3 0x199e98c in zend_fetch_dimension_address_read_R_slow /home/ilutov/Developer/php-src/Zend/zend_execute.c:2815
    #4 0x1be4c29 in ZEND_FETCH_DIM_R_SPEC_CV_CONST_HANDLER /home/ilutov/Developer/php-src/Zend/zend_vm_execute.h:40435
    #5 0x1c8d02a in execute_ex /home/ilutov/Developer/php-src/Zend/zend_vm_execute.h:59300
    #6 0x1c922b1 in zend_execute /home/ilutov/Developer/php-src/Zend/zend_vm_execute.h:60133
    #7 0x18badc7 in zend_execute_scripts /home/ilutov/Developer/php-src/Zend/zend.c:1799
    #8 0x1570f16 in php_execute_script /home/ilutov/Developer/php-src/main/main.c:2541
    #9 0x204c5fc in do_cli /home/ilutov/Developer/php-src/sapi/cli/php_cli.c:965
    #10 0x20502d7 in main /home/ilutov/Developer/php-src/sapi/cli/php_cli.c:1367
    #11 0x7fb5dca2954f in __libc_start_call_main (/usr/lib64/../lib64/libc.so.6+0x2954f)
    #12 0x7fb5dca29608 in __libc_start_main@@GLIBC_2.34 (/usr/lib64/../lib64/libc.so.6+0x29608)
    #13 0x603ef4 in _start (/home/ilutov/Developer/php-src/sapi/cli/php+0x603ef4)

But I expected this output instead:

object(FFI\CData:struct <anonymous>)#6 (1) {
  ["a"]=>
  int(0)
}

PHP Version

PHP-8.1, probably also PHP-8.0

Operating System

No response

@bwoebi
Copy link
Member

bwoebi commented Sep 23, 2022

This is not a bug. With FFI you're in general required to keep references to things you want to address. FFI is not refcounting addresses for you. After all that'll break down in many other scenarios, e.g.:

void *identity(void *ptr) { return ptr; }

Now, if you do \FFI::addr($ffi->identity(\FFI::addr($struct)));, FFI has no knowledge of the returned ptr being actually a reference to the original addr.

While it would be technically feasible in some limited scenarios to refcount an addr of a value, it probably removes also some of the explicit-ness of FFI.

If you want to, though, you could introduce a owned reference (like the $owned parameter on FFI::new()), which only works if the input is an owned CData.

@iluuu1994
Copy link
Member Author

@bwoebi Thanks for explaining. Dmitry mentioned the same in #9599. I'm closing this, although I'll still see if can improve error handling by throwing an exception when passing temporary values to FFI::addr().

@iluuu1994 iluuu1994 closed this as not planned Won't fix, can't repro, duplicate, stale Sep 23, 2022
nicolas-grekas added a commit to symfony/symfony that referenced this issue Sep 23, 2022
…n test (iluuu1994)

This PR was merged into the 6.2 branch.

Discussion
----------

[VarDumper] Fix use-after-free with nested FFI::addr() in test

| Q             | A
| ------------- | ---
| Branch?       | 6.2
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | php/php-src#9598
| License       | MIT
| Doc PR        |

This issue was found by the PHP community build, after which I created php/php-src#9598. Basically, the inner `\FFI::addr()` `CData` will be deallocated after the second `\FFI::addr()` call which will be stored and then points to invalid memory. However, [it turns out](php/php-src#9599 (comment)) nesting `\FFI::addr()` calls without temporarily storing the result is actually not allowed.

https://www.php.net/manual/en/ffi.addr.php

> Creates an unmanaged pointer to the C data represented by the given `FFI\CData`. The source ptr must survive the resulting pointer. This function is mainly useful to pass arguments to C functions by pointer.

We'll see if we can improve `FFI` by throwing an exception when passing temporary values to `FFI::addr()`. Either way, this test is not valid.

Commits
-------

4c79915 Fix use-after-free with nested FFI::addr() in VarDumper test
symfony-splitter pushed a commit to symfony/var-dumper that referenced this issue Sep 23, 2022
…n test (iluuu1994)

This PR was merged into the 6.2 branch.

Discussion
----------

[VarDumper] Fix use-after-free with nested FFI::addr() in test

| Q             | A
| ------------- | ---
| Branch?       | 6.2
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | php/php-src#9598
| License       | MIT
| Doc PR        |

This issue was found by the PHP community build, after which I created php/php-src#9598. Basically, the inner `\FFI::addr()` `CData` will be deallocated after the second `\FFI::addr()` call which will be stored and then points to invalid memory. However, [it turns out](php/php-src#9599 (comment)) nesting `\FFI::addr()` calls without temporarily storing the result is actually not allowed.

https://www.php.net/manual/en/ffi.addr.php

> Creates an unmanaged pointer to the C data represented by the given `FFI\CData`. The source ptr must survive the resulting pointer. This function is mainly useful to pass arguments to C functions by pointer.

We'll see if we can improve `FFI` by throwing an exception when passing temporary values to `FFI::addr()`. Either way, this test is not valid.

Commits
-------

4c799152ec Fix use-after-free with nested FFI::addr() in VarDumper test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants