endianness issue with FFI (ext/ffi/tests/gh11934.phpt)

[NattyNarwhal]

Description

Caught this when provisioning a big endian box for CI.

diff output on test:

--
     --- Struct ---
     bool(true)
     --- Callback return type ---
044- int(42)
044+ int(0)
     --- Other FFI\CData assignment ---
     int(123)
     --- Array element ---
--

The relevant part of that test:

echo "--- Callback return type ---\n";

$ffi = FFI::cdef('
typedef uint32_t (*test_callback)();
typedef struct {
        test_callback call_me;
} my_struct;
');

$struct = $ffi->new('my_struct');
$struct->call_me = function () use ($ffi) {
        $int = $ffi->new('uint32_t');
        $int->cdata = 42;
        return $int;
};

var_dump(($struct->call_me)());

Not familiar with FFI, so haven't looked into it yet, but making a note.

PHP Version

PHP 8.4.0-DEV

Operating System

Gentoo/ppc64

[cmb69]

For instance, the following looks fishy:

php-src/ext/ffi/ffi.c

Lines 787 to 790 in ba748e7

	case ZEND_FFI_TYPE_UINT32:
	lval = zval_get_long(value);
	*(uint32_t*)ptr = lval;
	break;

[NattyNarwhal]

I think reading and writing to the cdata is OK; if I do i.e.

<?php

echo "--- Callback return type ---\n";

$ffi = FFI::cdef('
typedef uint32_t (*test_callback)();
typedef struct {
        test_callback call_me;
} my_struct;
');

$struct = $ffi->new('my_struct');
$struct->call_me = function () use ($ffi) {
        $int = $ffi->new('uint32_t');
        $int->cdata = 42;
        var_dump($int->cdata);
        return $int;
};

var_dump(($struct->call_me)());

...the $int->data var_dump will return 42. What seems broken is function return types. I recall vaguely facing this kind of issue with return types years ago, which made me less interested in using FFI.

[NattyNarwhal]

Oh, found it. Still holds on Linux, which makes it easier to debug.

[NattyNarwhal]

Breakpoint 1, zend_ffi_cdata_to_zval (cdata=0x0, ptr=0x3ffff7a030f0, type=0x1008900a0 <zend_ffi_type_sint32>, read_type=0, rv=0x3ffff7a150d0, 
    flags=0, is_ret=true, debug_union=false) at /home/calvin/php-src/ext/ffi/ffi.c:549
549	fprintf(stderr, "Reading pointer %p type %d\n", ptr, kind);
[...]
(gdb) p/x ((uint32_t*)ptr)[0]
$5 = 0x0
(gdb) p/x ((uint32_t*)ptr)[1]
$6 = 0x11223344

the classic

[NattyNarwhal]

I think the problem is types smaller than machine register size are widened to the size of registers; ffi_trampoline is already doing this with ret = do_alloca(MAX(ret_type->size, sizeof(ffi_arg)), ret_use_heap);. On little endian, this doesn't matter, because of how it gets laid out in memory, but it's fatal for big endian.

Since you can i.e. be working with actual 32-bit stuff referenced by pointer for a cdata and that works fine, I'm suspecting the fix might be to change the return FFI type before calling zend_ffi_cdata_to_zval to one that matches the machine width. I think values are widened in arguments too, so it might need to be done there too - except it's really ABI dependent.

[nielsdos]

And most annoyingly is that almost none of the core developers have access to big endian machines. A similar problem already exists for AArch64, but less problematic than big endian platforms.

[NattyNarwhal]

I can provide access to a BE machine for developers. I'm also going to use it for CI, but it should have plenty of spare resources.

[nielsdos]

I have a preliminary patch that makes all tests pass and fixes support for bool, char, etc as well on big endian: https://gist.github.com/nielsdos/b9b5cae93caa5c485e9b259dc3a43600
It's incomplete as I still need to fully fix casting support and support for cdata assignment (preferably in a generic way)
I'll continue tomorrow or so

[fds242]

I just noticed this issue myself, and was about to file another dupe bug report; glad I searched (I see there were plenty of other mentions of this issue even beyond this). What a coincidence it's being worked on right now after all this time. :) Thank you!!

The patch looks a little complicated but I failed to notice there were issues with input parameters as well -- must have got lucky with the parameter types or values of my calls. I should look into it more.

On the return value issue, I patched zend_ffi_cdata_to_zval() as follows, adding the following before the pre-existing switch:

		if (is_ret && type->size <= sizeof(ffi_arg)) {
			switch (kind) {
				case ZEND_FFI_TYPE_SINT8:
				case ZEND_FFI_TYPE_SINT16:
				case ZEND_FFI_TYPE_SINT32:
				case ZEND_FFI_TYPE_SINT64:
					ZVAL_LONG(rv, *(ffi_sarg*)ptr);
					return;
				case ZEND_FFI_TYPE_CHAR:
				case ZEND_FFI_TYPE_BOOL:
				case ZEND_FFI_TYPE_UINT8:
				case ZEND_FFI_TYPE_UINT16:
				case ZEND_FFI_TYPE_UINT32:
				case ZEND_FFI_TYPE_UINT64:
					ZVAL_LONG(rv, *(ffi_arg*)ptr);
					return;
				default:
					break;
			}
		}

Which seemed to be sufficient to make everything function correctly in my PHP scripts in practice.

[nielsdos]

Interesting, thanks for sharing!

On the return value issue, I patched zend_ffi_cdata_to_zval() as follows, adding the following before the pre-existing switch:

Right, this would work too I suppose. I just added a macro and integrated it in the cases where narrowing occurs, that way we don't end up with a special code path. Simplification is probably possible though but refactoring how the switch is done is something for the master branch.
Most of the complication comes from the cdata copy handling in zend_ffi_zval_to_cdata where I now have an (incomplete) special case switch that would benefit from a generic solution.

[fds242]

This is just extra code added on in front, not refactoring or touching the preexisting switch in any way, since that was and remains correct for the !is_ret case -- it's only special (and hopefully, correct, on any system) handling for the is_ret case which is only where the widening occurs, and hence quite reasonable to separate out from the rest.

I see now the rest of your patch, and all that extensive surgery (to zend_ffi_zval_to_cdata, for example) concern solely to correct the case of PHP callbacks supplying a return value to libffi. And input parameters were already correctly handled, after all, making it no surprise that my PHP scripts already functioned well with my short fix only -- I'm thankfully not using callbacks. The PHP ffi docs quite discourage their use.

I don't immediately see a way to make things simpler there, only noting that maybe you could find a way to make use of the fact that libffi typedefs, along with the always unsigned ffi_arg, a signed counterpart of the same size called ffi_sarg. But, again, I don't see a way to really take advantage of that in your "TODO" commented section where cdata->ptr has to be typecast to the correct size either way. Maybe in whatever future needed fixes you noticed.