Fix GH-16013 and bug #80857: Big endian issues #17255

nielsdos · 2024-12-24T14:22:59Z

Fixes GH-16013 and https://bugs.php.net/bug.php?id=80857

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 on big endian.
This patch was developed and tested on a PowerPC 64-bit big endian system.
This worked by coincidence on little endian because the bytes are in the right place.

nielsdos · 2024-12-24T14:23:58Z

ext/ffi/ffi.c

@@ -746,6 +762,36 @@ static zend_always_inline zend_result zend_ffi_zval_to_cdata(void *ptr, zend_ffi
 		zend_ffi_cdata *cdata = (zend_ffi_cdata*)Z_OBJ_P(value);
 		if (zend_ffi_is_compatible_type(type, ZEND_FFI_TYPE(cdata->type)) &&
 			type->size == ZEND_FFI_TYPE(cdata->type)->size) {
+			if (is_ret && type->size < sizeof(ffi_arg)) {


This is an ugly solution but I don't see a better way so far. (memcpy trick may be possible for unsigned types but gets a bit uglier for sign extending signed types)
Also, perhaps it is better to put this inside an ifdef for big endian to reduce code bloat but I'm not sure.

dstogov

This may be fixed in a much simpler way. See comments.

dstogov · 2024-12-25T06:04:37Z

ext/ffi/ffi.c

-		zend_ffi_zval_to_cdata(ret, ret_type, &retval);
+		zend_ffi_zval_to_cdata(ret, ret_type, &retval, true);


Instead of zend_ffi_zval_to_cdata() modification, we should widen ret_type to intptr_t (or whatever else that ABI requests). We should keep the signess of the origianal ret_type and the capacity of ffi_arg.

I got rid of the ZEND_FFI_WRITE_NARROW macro in a separate commit to this PR.
I needed to keep the is_ret addition to handle cdata objects specially because we must not overread the pointer. I do see the appeal of this approach although I found the ZEND_FFI_WRITE_NARROW approach also clear.

dstogov · 2024-12-25T06:10:15Z

ext/ffi/ffi.c

@@ -560,22 +576,22 @@ static zend_always_inline void zend_ffi_cdata_to_zval(zend_ffi_cdata *cdata, voi
 				return;
 #endif
 			case ZEND_FFI_TYPE_UINT8:
-				ZVAL_LONG(rv, *(uint8_t*)ptr);
+				ZVAL_LONG(rv, ZEND_FFI_READ_NARROW(uint8_t, ptr, is_ret));


Instead of zend_ffi_cdata_to_zval() modification, we should pass the "proper" type at the first place (in ffi_trampoline).

I committed this separately to this PR.
I was able to get rid of the ZEND_FFI_READ_NARROW macros, but I needed to keep the special handling of ZEND_FFI_TYPE_BOOL to use ZVAL_BOOL and ZEND_FFI_TYPE_CHAR with ZVAL_CHAR. So therefore I kept the return handling in this function. I'm not sure this is much simpler, I preferred the old way to be honest.

nielsdos

I applied your suggestion in 2 new commits. I'm not entirely convinced it's much simpler, but maybe I did something wrong

nielsdos · 2024-12-25T14:37:53Z

ext/ffi/ffi.c

@@ -560,22 +576,22 @@ static zend_always_inline void zend_ffi_cdata_to_zval(zend_ffi_cdata *cdata, voi
 				return;
 #endif
 			case ZEND_FFI_TYPE_UINT8:
-				ZVAL_LONG(rv, *(uint8_t*)ptr);
+				ZVAL_LONG(rv, ZEND_FFI_READ_NARROW(uint8_t, ptr, is_ret));


I committed this separately to this PR.
I was able to get rid of the ZEND_FFI_READ_NARROW macros, but I needed to keep the special handling of ZEND_FFI_TYPE_BOOL to use ZVAL_BOOL and ZEND_FFI_TYPE_CHAR with ZVAL_CHAR. So therefore I kept the return handling in this function. I'm not sure this is much simpler, I preferred the old way to be honest.

nielsdos · 2024-12-25T14:39:35Z

ext/ffi/ffi.c

-		zend_ffi_zval_to_cdata(ret, ret_type, &retval);
+		zend_ffi_zval_to_cdata(ret, ret_type, &retval, true);


I got rid of the ZEND_FFI_WRITE_NARROW macro in a separate commit to this PR.
I needed to keep the is_ret addition to handle cdata objects specially because we must not overread the pointer. I do see the appeal of this approach although I found the ZEND_FFI_WRITE_NARROW approach also clear.

dstogov · 2024-12-25T17:16:32Z

I came to the following patch. May be we may wrap this code with #ifdef WORDS_BIGENDIAN. Please verify.

diff --git a/ext/ffi/ffi.c b/ext/ffi/ffi.c
index d1713b20d75..a660274e75e 100644
--- a/ext/ffi/ffi.c
+++ b/ext/ffi/ffi.c
@@ -985,6 +985,24 @@ 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);
+		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;
+			}
+		}
 	}
 
 	zval_ptr_dtor(&retval);
@@ -2855,7 +2873,28 @@ 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 *ret_type = ZEND_FFI_TYPE(type->func.ret_type);
+
+		if (ret_type->size < sizeof(ffi_arg)
+		 && ret_type->kind >= ZEND_FFI_TYPE_UINT8
+		 && ret_type->kind < ZEND_FFI_TYPE_POINTER) {
+
+			/* We need to narrow the value (truncate) */
+			switch (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;
+			}
+		}
+		zend_ffi_cdata_to_zval(NULL, ret, ret_type, BP_VAR_R, return_value, 0, 1, 0);
 	} else {
 		ZVAL_NULL(return_value);
 	}

nielsdos · 2024-12-25T17:37:15Z

Yes that works. Adding the ifdef is a good idea too.
At one point I was thinking in that direction but I got confused by the need to sign extend, but it doesn't matter as the top bits will be dropped later anyway. Bummer that I wasn't able to come up with this..

I'll force push this PR with the ifdef and commit after CI passes.

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. Co-authored-by: Dmitry Stogov <dmitry@zend.com>

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 phpGH-17255. Co-authored-by: Dmitry Stogov <dmitry@zend.com>

nielsdos linked an issue Dec 24, 2024 that may be closed by this pull request

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

Closed

github-actions bot added Extension: ffi Extension: zend_test labels Dec 24, 2024

nielsdos commented Dec 24, 2024

View reviewed changes

nielsdos marked this pull request as ready for review December 24, 2024 15:04

nielsdos requested a review from dstogov as a code owner December 24, 2024 15:04

NattyNarwhal mentioned this pull request Dec 24, 2024

Big endian PowerPC CI #17258

Closed

dstogov requested changes Dec 25, 2024

View reviewed changes

nielsdos commented Dec 25, 2024

View reviewed changes

nielsdos force-pushed the fix-ffi-16013 branch from 762dd09 to 020b2e9 Compare December 25, 2024 17:49

dstogov approved these changes Dec 25, 2024

View reviewed changes

nielsdos closed this in 99a14b8 Dec 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix GH-16013 and bug #80857: Big endian issues #17255

Fix GH-16013 and bug #80857: Big endian issues #17255

Uh oh!

nielsdos commented Dec 24, 2024 •

edited

Loading

Uh oh!

nielsdos Dec 24, 2024 •

edited

Loading

Uh oh!

dstogov left a comment

Uh oh!

dstogov Dec 25, 2024

Uh oh!

nielsdos Dec 25, 2024

Uh oh!

dstogov Dec 25, 2024

Uh oh!

nielsdos Dec 25, 2024

Uh oh!

nielsdos left a comment

Uh oh!

nielsdos Dec 25, 2024

Uh oh!

nielsdos Dec 25, 2024

Uh oh!

dstogov commented Dec 25, 2024

Uh oh!

nielsdos commented Dec 25, 2024

Uh oh!

Uh oh!

		zend_ffi_zval_to_cdata(ret, ret_type, &retval);
		zend_ffi_zval_to_cdata(ret, ret_type, &retval, true);

Fix GH-16013 and bug #80857: Big endian issues #17255

Fix GH-16013 and bug #80857: Big endian issues #17255

Uh oh!

Conversation

nielsdos commented Dec 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nielsdos Dec 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dstogov left a comment

Choose a reason for hiding this comment

Uh oh!

dstogov Dec 25, 2024

Choose a reason for hiding this comment

Uh oh!

nielsdos Dec 25, 2024

Choose a reason for hiding this comment

Uh oh!

dstogov Dec 25, 2024

Choose a reason for hiding this comment

Uh oh!

nielsdos Dec 25, 2024

Choose a reason for hiding this comment

Uh oh!

nielsdos left a comment

Choose a reason for hiding this comment

Uh oh!

nielsdos Dec 25, 2024

Choose a reason for hiding this comment

Uh oh!

nielsdos Dec 25, 2024

Choose a reason for hiding this comment

Uh oh!

dstogov commented Dec 25, 2024

Uh oh!

nielsdos commented Dec 25, 2024

Uh oh!

Uh oh!

nielsdos commented Dec 24, 2024 •

edited

Loading

nielsdos Dec 24, 2024 •

edited

Loading