Review suggestion for zend_ffi_zval_to_cdata()

Author: ndossche

ext/ffi/ffi.c

@@ -188,21 +188,6 @@ typedef struct _zend_ffi {
 #define ZEND_FFI_SIZEOF_ARG \
 	MAX(FFI_SIZEOF_ARG, sizeof(double))
 
-/* 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.
- */
-#define ZEND_FFI_WRITE_NARROW(ty, ptr, val, is_ret) do { \
-	if (is_ret) { \
-		*(ffi_arg *) ptr = (ffi_arg) (ty) val; \
-	} else { \
-		*(ty *) ptr = val; \
-	} \
-} while (0)
-
 typedef struct _zend_ffi_cdata {
 	zend_object            std;
 	zend_ffi_type         *type;
@@ -784,20 +769,21 @@ static void zend_ffi_zval_to_bit_field(void *ptr, zend_ffi_field *field, zval *v
 }
 /* }}} */
 
-static zend_always_inline zend_result zend_ffi_zval_to_cdata(void *ptr, zend_ffi_type *type, zval *value, bool is_ret) /* {{{ */
+static zend_always_inline zend_result zend_ffi_zval_to_cdata(void *ptr, zend_ffi_type *type, zend_ffi_type_kind kind, zval *value, bool is_ret) /* {{{ */
 {
 	zend_long lval;
 	double dval;
 	zend_string *tmp_str;
 	zend_string *str;
-	zend_ffi_type_kind kind = type->kind;
 
 	/* Pointer type has special handling of CData */
 	if (kind != ZEND_FFI_TYPE_POINTER && Z_TYPE_P(value) == IS_OBJECT && Z_OBJCE_P(value) == zend_ffi_cdata_ce) {
 		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)) {
+				/* Do not widen the reads because that may over-read bytes. */
+				kind = type->kind;
 again_narrowed:
 				switch (kind) {
 					case ZEND_FFI_TYPE_SINT8:
@@ -850,27 +836,27 @@ static zend_always_inline zend_result zend_ffi_zval_to_cdata(void *ptr, zend_ffi
 #endif
 		case ZEND_FFI_TYPE_UINT8:
 			lval = zval_get_long(value);
-			ZEND_FFI_WRITE_NARROW(uint8_t, ptr, lval, is_ret);
+			*(uint8_t*)ptr = lval;
 			break;
 		case ZEND_FFI_TYPE_SINT8:
 			lval = zval_get_long(value);
-			ZEND_FFI_WRITE_NARROW(int8_t, ptr, lval, is_ret);
+			*(int8_t*)ptr = lval;
 			break;
 		case ZEND_FFI_TYPE_UINT16:
 			lval = zval_get_long(value);
-			ZEND_FFI_WRITE_NARROW(uint16_t, ptr, lval, is_ret);
+			*(uint16_t*)ptr = lval;
 			break;
 		case ZEND_FFI_TYPE_SINT16:
 			lval = zval_get_long(value);
-			ZEND_FFI_WRITE_NARROW(int16_t, ptr, lval, is_ret);
+			*(int16_t*)ptr = lval;
 			break;
 		case ZEND_FFI_TYPE_UINT32:
 			lval = zval_get_long(value);
-			ZEND_FFI_WRITE_NARROW(uint32_t, ptr, lval, is_ret);
+			*(uint32_t*)ptr = lval;
 			break;
 		case ZEND_FFI_TYPE_SINT32:
 			lval = zval_get_long(value);
-			ZEND_FFI_WRITE_NARROW(int32_t, ptr, lval, is_ret);
+			*(int32_t*)ptr = lval;
 			break;
 		case ZEND_FFI_TYPE_UINT64:
 			lval = zval_get_long(value);
@@ -881,12 +867,12 @@ static zend_always_inline zend_result zend_ffi_zval_to_cdata(void *ptr, zend_ffi
 			*(int64_t*)ptr = lval;
 			break;
 		case ZEND_FFI_TYPE_BOOL:
-			ZEND_FFI_WRITE_NARROW(uint8_t, ptr, zend_is_true(value), is_ret);
+			*(uint8_t*)ptr = zend_is_true(value);
 			break;
 		case ZEND_FFI_TYPE_CHAR:
 			str = zval_get_tmp_string(value, &tmp_str);
 			if (ZSTR_LEN(str) == 1) {
-				ZEND_FFI_WRITE_NARROW(char, ptr, ZSTR_VAL(str)[0], is_ret);
+				*(uint8_t*)ptr = ZSTR_VAL(str)[0];
 			} else {
 				zend_tmp_string_release(tmp_str);
 				zend_ffi_assign_incompatible(value, type);
@@ -1065,7 +1051,38 @@ 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, true);
+		zend_ffi_type_kind kind = ret_type->kind;
+
+		/* Return values are widened to a machine register. */
+		if (kind == ZEND_FFI_TYPE_ENUM) {
+			kind = ret_type->enumeration.kind;
+		}
+		switch (kind) {
+			case ZEND_FFI_TYPE_BOOL:
+			case ZEND_FFI_TYPE_CHAR:
+			case ZEND_FFI_TYPE_UINT8:
+			case ZEND_FFI_TYPE_UINT16:
+#if SIZEOF_SIZE_T == 8
+			case ZEND_FFI_TYPE_UINT32:
+				kind = ZEND_FFI_TYPE_UINT64;
+#else
+				kind = ZEND_FFI_TYPE_UINT32;
+#endif
+				break;
+			case ZEND_FFI_TYPE_SINT8:
+			case ZEND_FFI_TYPE_SINT16:
+#if SIZEOF_SIZE_T == 8
+			case ZEND_FFI_TYPE_SINT32:
+				kind = ZEND_FFI_TYPE_SINT64;
+#else
+				kind = ZEND_FFI_TYPE_SINT32;
+#endif
+				break;
+			default:
+				break;
+		}
+
+		zend_ffi_zval_to_cdata(ret, ret_type, kind, &retval, true);
 	}
 
 	zval_ptr_dtor(&retval);
@@ -1219,7 +1236,7 @@ static zval *zend_ffi_cdata_set(zend_object *obj, zend_string *member, zval *val
 		return &EG(uninitialized_zval);
 	}
 
-	zend_ffi_zval_to_cdata(cdata->ptr, type, value, false);
+	zend_ffi_zval_to_cdata(cdata->ptr, type, type->kind, value, false);
 
 	return value;
 }
@@ -1441,7 +1458,7 @@ static zval *zend_ffi_cdata_write_field(zend_object *obj, zend_string *field_nam
 
 	if (EXPECTED(!field->bits)) {
 		ptr = (void*)(((char*)ptr) + field->offset);
-		zend_ffi_zval_to_cdata(ptr, ZEND_FFI_TYPE(field->type), value, false);
+		zend_ffi_zval_to_cdata(ptr, ZEND_FFI_TYPE(field->type), ZEND_FFI_TYPE(field->type)->kind, value, false);
 	} else {
 		zend_ffi_zval_to_bit_field(ptr, field, value);
 	}
@@ -1555,7 +1572,7 @@ static void zend_ffi_cdata_write_dim(zend_object *obj, zval *offset, zval *value
 		return;
 	}
 
-	zend_ffi_zval_to_cdata(ptr, type, value, false);
+	zend_ffi_zval_to_cdata(ptr, type, type->kind, value, false);
 }
 /* }}} */
 
@@ -2620,7 +2637,7 @@ static zval *zend_ffi_write_var(zend_object *obj, zend_string *var_name, zval *v
 		return value;
 	}
 
-	zend_ffi_zval_to_cdata(sym->addr, ZEND_FFI_TYPE(sym->type), value, false);
+	zend_ffi_zval_to_cdata(sym->addr, ZEND_FFI_TYPE(sym->type), ZEND_FFI_TYPE(sym->type)->kind, value, false);
 	return value;
 }
 /* }}} */
@@ -4097,7 +4114,7 @@ ZEND_METHOD(FFI, cast) /* {{{ */
 			cdata->std.handlers = &zend_ffi_cdata_value_handlers;
 			cdata->type = type_ptr;
 			cdata->ptr = emalloc(type->size);
-			zend_ffi_zval_to_cdata(cdata->ptr, type, zv, false);
+			zend_ffi_zval_to_cdata(cdata->ptr, type, type->kind, zv, false);
 			cdata->flags = ZEND_FFI_FLAG_OWNED;
 			if (is_const) {
 				cdata->flags |= ZEND_FFI_FLAG_CONST;