Skip to content

Commit

Permalink
Fix GH-16013 and bug #80857: Big endian issues
Browse files Browse the repository at this point in the history
The FFI call return values follow widening rules.
We must widen to `ffi_arg` in the case we're handling a return value for types shorter than the machine width.
From http://www.chiark.greenend.org.uk/doc/libffi-dev/html/The-Closure-API.html:
> In most cases, ret points to an object of exactly the size of the type specified when cif was constructed.
> However, integral types narrower than the system register size are widened.
> In these cases your program may assume that ret points to an ffi_arg object.

If we don't do this, we get wrong values when reading the return values.

Closes GH-17255.

Co-authored-by: Dmitry Stogov <[email protected]>
  • Loading branch information
nielsdos and dstogov committed Dec 25, 2024
1 parent 643a77d commit 99a14b8
Show file tree
Hide file tree
Showing 4 changed files with 219 additions and 1 deletion.
1 change: 1 addition & 0 deletions NEWS
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ PHP NEWS
- FFI:
. Fixed bug #79075 (FFI header parser chokes on comments). (nielsdos)
. Fix memory leak on ZEND_FFI_TYPE_CHAR conversion failure. (nielsdos)
. Fixed bug GH-16013 and bug #80857 (Big endian issues). (Dmitry, nielsdos)

- Filter:
. Fixed bug GH-16944 (Fix filtering special IPv4 and IPv6 ranges, by using
Expand Down
47 changes: 46 additions & 1 deletion ext/ffi/ffi.c
Original file line number Diff line number Diff line change
Expand Up @@ -985,6 +985,27 @@ static void zend_ffi_callback_trampoline(ffi_cif* cif, void* ret, void** args, v
ret_type = ZEND_FFI_TYPE(callback_data->type->func.ret_type);
if (ret_type->kind != ZEND_FFI_TYPE_VOID) {
zend_ffi_zval_to_cdata(ret, ret_type, &retval);

#ifdef WORDS_BIGENDIAN
if (ret_type->size < sizeof(ffi_arg)
&& ret_type->kind >= ZEND_FFI_TYPE_UINT8
&& ret_type->kind < ZEND_FFI_TYPE_POINTER) {
/* We need to widen the value (zero extend) */
switch (ret_type->size) {
case 1:
*(ffi_arg*)ret = *(uint8_t*)ret;
break;
case 2:
*(ffi_arg*)ret = *(uint16_t*)ret;
break;
case 4:
*(ffi_arg*)ret = *(uint32_t*)ret;
break;
default:
break;
}
}
#endif
}

zval_ptr_dtor(&retval);
Expand Down Expand Up @@ -2855,7 +2876,31 @@ static ZEND_FUNCTION(ffi_trampoline) /* {{{ */
}

if (ZEND_FFI_TYPE(type->func.ret_type)->kind != ZEND_FFI_TYPE_VOID) {
zend_ffi_cdata_to_zval(NULL, ret, ZEND_FFI_TYPE(type->func.ret_type), BP_VAR_R, return_value, 0, 1, 0);
zend_ffi_type *func_ret_type = ZEND_FFI_TYPE(type->func.ret_type);

#ifdef WORDS_BIGENDIAN
if (func_ret_type->size < sizeof(ffi_arg)
&& func_ret_type->kind >= ZEND_FFI_TYPE_UINT8
&& func_ret_type->kind < ZEND_FFI_TYPE_POINTER) {

/* We need to narrow the value (truncate) */
switch (func_ret_type->size) {
case 1:
*(uint8_t*)ret = *(ffi_arg*)ret;
break;
case 2:
*(uint16_t*)ret = *(ffi_arg*)ret;
break;
case 4:
*(uint32_t*)ret = *(ffi_arg*)ret;
break;
default:
break;
}
}
#endif

zend_ffi_cdata_to_zval(NULL, ret, func_ret_type, BP_VAR_R, return_value, 0, 1, 0);
} else {
ZVAL_NULL(return_value);
}
Expand Down
137 changes: 137 additions & 0 deletions ext/ffi/tests/gh16013.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
--TEST--
GH-16013 (endianness issue with FFI)
--EXTENSIONS--
ffi
zend_test
--FILE--
<?php
require_once('utils.inc');

$header = <<<HEADER
enum bug_gh16013_enum {
BUG_GH16013_A = 1,
BUG_GH16013_B = 2,
};
struct bug_gh16013_int_struct {
int field;
};
struct bug_gh16013_callback_struct {
int8_t (*return_int8)(int8_t);
uint8_t (*return_uint8)(uint8_t);
int16_t (*return_int16)(int16_t);
uint16_t (*return_uint16)(uint16_t);
int32_t (*return_int32)(int32_t);
uint32_t (*return_uint32)(uint32_t);
float (*return_float)(float);
struct bug_gh16013_int_struct (*return_struct)(struct bug_gh16013_int_struct);
enum bug_gh16013_enum (*return_enum)(enum bug_gh16013_enum);
};
char bug_gh16013_return_char();
bool bug_gh16013_return_bool();
short bug_gh16013_return_short();
int bug_gh16013_return_int();
enum bug_gh16013_enum bug_gh16013_return_enum();
struct bug_gh16013_int_struct bug_gh16013_return_struct();
HEADER;

if (PHP_OS_FAMILY !== 'Windows') {
$ffi = FFI::cdef($header);
} else {
try {
$ffi = FFI::cdef($header, 'php_zend_test.dll');
} catch (FFI\Exception $ex) {
$ffi = FFI::cdef($header, ffi_get_php_dll_name());
}
}

echo "--- Return values ---\n";
var_dump($ffi->bug_gh16013_return_char());
var_dump($ffi->bug_gh16013_return_bool());
var_dump($ffi->bug_gh16013_return_short());
var_dump($ffi->bug_gh16013_return_int());
var_dump($ffi->bug_gh16013_return_enum());
var_dump($ffi->bug_gh16013_return_struct());

echo "--- Callback values ---\n";
$bug_gh16013_callback_struct = $ffi->new('struct bug_gh16013_callback_struct');
$bug_gh16013_callback_struct->return_int8 = function($val) use($ffi) {
$cdata = $ffi->new('int8_t');
$cdata->cdata = $val;
return $cdata;
};
$bug_gh16013_callback_struct->return_uint8 = function($val) use($ffi) {
$cdata = $ffi->new('uint8_t');
$cdata->cdata = $val;
return $cdata;
};
$bug_gh16013_callback_struct->return_int16 = function($val) use($ffi) {
$cdata = $ffi->new('int16_t');
$cdata->cdata = $val;
return $cdata;
};
$bug_gh16013_callback_struct->return_uint16 = function($val) use($ffi) {
$cdata = $ffi->new('uint16_t');
$cdata->cdata = $val;
return $cdata;
};
$bug_gh16013_callback_struct->return_int32 = function($val) use($ffi) {
$cdata = $ffi->new('int32_t');
$cdata->cdata = $val;
return $cdata;
};
$bug_gh16013_callback_struct->return_uint32 = function($val) use($ffi) {
$cdata = $ffi->new('uint32_t');
$cdata->cdata = $val;
return $cdata;
};
$bug_gh16013_callback_struct->return_float = function($val) use($ffi) {
$cdata = $ffi->new('float');
$cdata->cdata = $val;
return $cdata;
};
$bug_gh16013_callback_struct->return_struct = function($val) use($ffi) {
return $val;
};
$bug_gh16013_callback_struct->return_enum = function($val) use($ffi) {
$cdata = $ffi->new('enum bug_gh16013_enum');
$cdata->cdata = $val;
return $cdata;
};

var_dump(($bug_gh16013_callback_struct->return_int8)(-4));
var_dump(($bug_gh16013_callback_struct->return_uint8)(4));
var_dump(($bug_gh16013_callback_struct->return_int16)(-10000));
var_dump(($bug_gh16013_callback_struct->return_uint16)(10000));
var_dump(($bug_gh16013_callback_struct->return_int32)(-100000));
var_dump(($bug_gh16013_callback_struct->return_uint32)(100000));
var_dump(($bug_gh16013_callback_struct->return_float)(12.34));
$struct = $ffi->new('struct bug_gh16013_int_struct');
$struct->field = 10;
var_dump(($bug_gh16013_callback_struct->return_struct)($struct));
var_dump(($bug_gh16013_callback_struct->return_enum)($ffi->BUG_GH16013_B));
?>
--EXPECT--
--- Return values ---
string(1) "A"
bool(true)
int(12345)
int(123456789)
int(2)
object(FFI\CData:struct bug_gh16013_int_struct)#2 (1) {
["field"]=>
int(123456789)
}
--- Callback values ---
int(-4)
int(4)
int(-10000)
int(10000)
int(-100000)
int(100000)
float(12.34000015258789)
object(FFI\CData:struct bug_gh16013_int_struct)#13 (1) {
["field"]=>
int(10)
}
int(2)
35 changes: 35 additions & 0 deletions ext/zend_test/test.c
Original file line number Diff line number Diff line change
Expand Up @@ -1449,6 +1449,41 @@ PHP_ZEND_TEST_API void bug_gh9090_void_int_char_var(int i, char *fmt, ...) {

PHP_ZEND_TEST_API int gh11934b_ffi_var_test_cdata;

enum bug_gh16013_enum {
BUG_GH16013_A = 1,
BUG_GH16013_B = 2,
};

struct bug_gh16013_int_struct {
int field;
};

PHP_ZEND_TEST_API char bug_gh16013_return_char(void) {
return 'A';
}

PHP_ZEND_TEST_API bool bug_gh16013_return_bool(void) {
return true;
}

PHP_ZEND_TEST_API short bug_gh16013_return_short(void) {
return 12345;
}

PHP_ZEND_TEST_API int bug_gh16013_return_int(void) {
return 123456789;
}

PHP_ZEND_TEST_API enum bug_gh16013_enum bug_gh16013_return_enum(void) {
return BUG_GH16013_B;
}

PHP_ZEND_TEST_API struct bug_gh16013_int_struct bug_gh16013_return_struct(void) {
struct bug_gh16013_int_struct ret;
ret.field = 123456789;
return ret;
}

#ifdef HAVE_COPY_FILE_RANGE
/**
* This function allows us to simulate early return of copy_file_range by setting the limit_copy_file_range ini setting.
Expand Down

0 comments on commit 99a14b8

Please sign in to comment.