From de04cd516235205339414e71a8942a2a626d575d Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Fri, 8 May 2020 10:24:42 +0200 Subject: [PATCH 1/4] Fix #79571: FFI: var_dumping unions may segfault We must not attempt to access arbitrary union members when retrieving debug info, because that may not be valid. Therefore we do no longer dereference pointer types inside of unions, but report their address as integer instead. --- ext/ffi/ffi.c | 28 ++++++++++++++++------------ ext/ffi/tests/bug79571.phpt | 28 ++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 12 deletions(-) create mode 100644 ext/ffi/tests/bug79571.phpt diff --git a/ext/ffi/ffi.c b/ext/ffi/ffi.c index 9626c4b374827..ec70e4958721e 100644 --- a/ext/ffi/ffi.c +++ b/ext/ffi/ffi.c @@ -459,7 +459,7 @@ static zend_never_inline zend_ffi_cdata *zend_ffi_cdata_to_zval_slow_ret(void *p } /* }}} */ -static zend_always_inline void zend_ffi_cdata_to_zval(zend_ffi_cdata *cdata, void *ptr, zend_ffi_type *type, int read_type, zval *rv, zend_ffi_flags flags, zend_bool is_ret) /* {{{ */ +static zend_always_inline void zend_ffi_cdata_to_zval(zend_ffi_cdata *cdata, void *ptr, zend_ffi_type *type, int read_type, zval *rv, zend_ffi_flags flags, zend_bool is_ret, zend_bool debug_union) /* {{{ */ { if (read_type == BP_VAR_R) { zend_ffi_type_kind kind = type->kind; @@ -511,6 +511,10 @@ static zend_always_inline void zend_ffi_cdata_to_zval(zend_ffi_cdata *cdata, voi kind = type->enumeration.kind; goto again; case ZEND_FFI_TYPE_POINTER: + if (debug_union) { + ZVAL_LONG(rv, (uintptr_t)*(void**)ptr); + return; + } if (*(void**)ptr == NULL) { ZVAL_NULL(rv); return; @@ -864,7 +868,7 @@ static void zend_ffi_callback_trampoline(ffi_cif* cif, void* ret, void** args, v ZEND_HASH_FOREACH_PTR(callback_data->type->func.args, arg_type) { arg_type = ZEND_FFI_TYPE(arg_type); - zend_ffi_cdata_to_zval(NULL, args[n], arg_type, BP_VAR_R, &fci.params[n], (zend_ffi_flags)(arg_type->attr & ZEND_FFI_ATTR_CONST), 0); + zend_ffi_cdata_to_zval(NULL, args[n], arg_type, BP_VAR_R, &fci.params[n], (zend_ffi_flags)(arg_type->attr & ZEND_FFI_ATTR_CONST), 0, 0); n++; } ZEND_HASH_FOREACH_END(); } @@ -999,7 +1003,7 @@ static zval *zend_ffi_cdata_get(zval *object, zval *member, int read_type, void return &EG(uninitialized_zval);; } - zend_ffi_cdata_to_zval(cdata, cdata->ptr, type, BP_VAR_R, rv, 0, 0); + zend_ffi_cdata_to_zval(cdata, cdata->ptr, type, BP_VAR_R, rv, 0, 0, 0); return rv; } /* }}} */ @@ -1171,7 +1175,7 @@ static zval *zend_ffi_cdata_read_field(zval *object, zval *member, int read_type } } ptr = (void*)(((char*)ptr) + field->offset); - zend_ffi_cdata_to_zval(NULL, ptr, field_type, read_type, rv, (cdata->flags & ZEND_FFI_FLAG_CONST) | (zend_ffi_flags)field->is_const, 0); + zend_ffi_cdata_to_zval(NULL, ptr, field_type, read_type, rv, (cdata->flags & ZEND_FFI_FLAG_CONST) | (zend_ffi_flags)field->is_const, 0, 0); } else { zend_ffi_bit_field_to_zval(ptr, field, rv); } @@ -1312,7 +1316,7 @@ static zval *zend_ffi_cdata_read_dim(zval *object, zval *offset, int read_type, return &EG(uninitialized_zval); } - zend_ffi_cdata_to_zval(NULL, ptr, dim_type, read_type, rv, is_const, 0); + zend_ffi_cdata_to_zval(NULL, ptr, dim_type, read_type, rv, is_const, 0, 0); return rv; } /* }}} */ @@ -1863,7 +1867,7 @@ static zval *zend_ffi_cdata_it_get_current_data(zend_object_iterator *it) /* {{{ ptr = (void*)((char*)cdata->ptr + dim_type->size * iter->it.index); zval_ptr_dtor(&iter->value); - zend_ffi_cdata_to_zval(NULL, ptr, dim_type, iter->by_ref ? BP_VAR_RW : BP_VAR_R, &iter->value, (cdata->flags & ZEND_FFI_FLAG_CONST) | (zend_ffi_flags)(type->attr & ZEND_FFI_ATTR_CONST), 0); + zend_ffi_cdata_to_zval(NULL, ptr, dim_type, iter->by_ref ? BP_VAR_RW : BP_VAR_R, &iter->value, (cdata->flags & ZEND_FFI_FLAG_CONST) | (zend_ffi_flags)(type->attr & ZEND_FFI_ATTR_CONST), 0, 0); return &iter->value; } /* }}} */ @@ -1958,7 +1962,7 @@ static HashTable *zend_ffi_cdata_get_debug_info(zval *object, int *is_temp) /* { case ZEND_FFI_TYPE_SINT32: case ZEND_FFI_TYPE_UINT64: case ZEND_FFI_TYPE_SINT64: - zend_ffi_cdata_to_zval(cdata, ptr, type, BP_VAR_R, &tmp, ZEND_FFI_FLAG_CONST, 0); + zend_ffi_cdata_to_zval(cdata, ptr, type, BP_VAR_R, &tmp, ZEND_FFI_FLAG_CONST, 0, 0); ht = zend_new_array(1); zend_hash_str_add(ht, "cdata", sizeof("cdata")-1, &tmp); *is_temp = 1; @@ -1978,7 +1982,7 @@ static HashTable *zend_ffi_cdata_get_debug_info(zval *object, int *is_temp) /* { *is_temp = 1; return ht; } else { - zend_ffi_cdata_to_zval(NULL, *(void**)ptr, ZEND_FFI_TYPE(type->pointer.type), BP_VAR_R, &tmp, ZEND_FFI_FLAG_CONST, 0); + zend_ffi_cdata_to_zval(NULL, *(void**)ptr, ZEND_FFI_TYPE(type->pointer.type), BP_VAR_R, &tmp, ZEND_FFI_FLAG_CONST, 0, 0); ht = zend_new_array(1); zend_hash_index_add_new(ht, 0, &tmp); *is_temp = 1; @@ -1991,7 +1995,7 @@ static HashTable *zend_ffi_cdata_get_debug_info(zval *object, int *is_temp) /* { if (key) { if (!f->bits) { void *f_ptr = (void*)(((char*)ptr) + f->offset); - zend_ffi_cdata_to_zval(NULL, f_ptr, ZEND_FFI_TYPE(f->type), BP_VAR_R, &tmp, ZEND_FFI_FLAG_CONST, 0); + zend_ffi_cdata_to_zval(NULL, f_ptr, ZEND_FFI_TYPE(f->type), BP_VAR_R, &tmp, ZEND_FFI_FLAG_CONST, 0, type->attr & ZEND_FFI_ATTR_UNION); zend_hash_add(ht, key, &tmp); } else { zend_ffi_bit_field_to_zval(ptr, f, &tmp); @@ -2004,7 +2008,7 @@ static HashTable *zend_ffi_cdata_get_debug_info(zval *object, int *is_temp) /* { case ZEND_FFI_TYPE_ARRAY: ht = zend_new_array(type->array.length); for (n = 0; n < type->array.length; n++) { - zend_ffi_cdata_to_zval(NULL, ptr, ZEND_FFI_TYPE(type->array.type), BP_VAR_R, &tmp, ZEND_FFI_FLAG_CONST, 0); + zend_ffi_cdata_to_zval(NULL, ptr, ZEND_FFI_TYPE(type->array.type), BP_VAR_R, &tmp, ZEND_FFI_FLAG_CONST, 0, 0); zend_hash_index_add(ht, n, &tmp); ptr = (void*)(((char*)ptr) + ZEND_FFI_TYPE(type->array.type)->size); } @@ -2373,7 +2377,7 @@ static zval *zend_ffi_read_var(zval *object, zval *member, int read_type, void * zend_tmp_string_release(tmp_var_name); if (sym->kind == ZEND_FFI_SYM_VAR) { - zend_ffi_cdata_to_zval(NULL, sym->addr, ZEND_FFI_TYPE(sym->type), read_type, rv, (zend_ffi_flags)sym->is_const, 0); + zend_ffi_cdata_to_zval(NULL, sym->addr, ZEND_FFI_TYPE(sym->type), read_type, rv, (zend_ffi_flags)sym->is_const, 0, 0); } else if (sym->kind == ZEND_FFI_SYM_FUNC) { zend_ffi_cdata *cdata; zend_ffi_type *new_type = emalloc(sizeof(zend_ffi_type)); @@ -2746,7 +2750,7 @@ static ZEND_FUNCTION(ffi_trampoline) /* {{{ */ free_alloca(arg_values, arg_values_use_heap); } - zend_ffi_cdata_to_zval(NULL, ret, ZEND_FFI_TYPE(type->func.ret_type), BP_VAR_R, return_value, 0, 1); + zend_ffi_cdata_to_zval(NULL, ret, ZEND_FFI_TYPE(type->func.ret_type), BP_VAR_R, return_value, 0, 1, 0); free_alloca(ret, ret_use_heap); exit: diff --git a/ext/ffi/tests/bug79571.phpt b/ext/ffi/tests/bug79571.phpt new file mode 100644 index 0000000000000..da80828acccb2 --- /dev/null +++ b/ext/ffi/tests/bug79571.phpt @@ -0,0 +1,28 @@ +--TEST-- +Bug #79571 (FFI: var_dumping unions may segfault) +--SKIPIF-- + +--FILE-- +new('my_union'); +$union->num = 42; +var_dump($union); +var_dump($union->num); +?> +--EXPECTF-- +object(FFI\CData:union )#%d (2) { + ["num"]=> + int(42) + ["str"]=> + int(42) +} +int(42) From d9c9b90013d73093c4ed30262d9efa76eeae61c7 Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Mon, 11 May 2020 13:56:55 +0200 Subject: [PATCH 2/4] Report NULL pointer as NULL even for union members --- ext/ffi/ffi.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/ext/ffi/ffi.c b/ext/ffi/ffi.c index ec70e4958721e..102c54473adc0 100644 --- a/ext/ffi/ffi.c +++ b/ext/ffi/ffi.c @@ -511,13 +511,12 @@ static zend_always_inline void zend_ffi_cdata_to_zval(zend_ffi_cdata *cdata, voi kind = type->enumeration.kind; goto again; case ZEND_FFI_TYPE_POINTER: - if (debug_union) { - ZVAL_LONG(rv, (uintptr_t)*(void**)ptr); - return; - } if (*(void**)ptr == NULL) { ZVAL_NULL(rv); return; + } else if (debug_union) { + ZVAL_LONG(rv, (uintptr_t)*(void**)ptr); + return; } else if ((type->attr & ZEND_FFI_ATTR_CONST) && ZEND_FFI_TYPE(type->pointer.type)->kind == ZEND_FFI_TYPE_CHAR) { ZVAL_STRING(rv, *(char**)ptr); return; From 6757b9964ddf1491a6d12cf93166e59cc4ce5fc1 Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Mon, 11 May 2020 13:57:52 +0200 Subject: [PATCH 3/4] Show pointer types as string in %p format --- ext/ffi/ffi.c | 2 +- ext/ffi/tests/bug79571.phpt | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ext/ffi/ffi.c b/ext/ffi/ffi.c index 102c54473adc0..e1a7e8db3461e 100644 --- a/ext/ffi/ffi.c +++ b/ext/ffi/ffi.c @@ -515,7 +515,7 @@ static zend_always_inline void zend_ffi_cdata_to_zval(zend_ffi_cdata *cdata, voi ZVAL_NULL(rv); return; } else if (debug_union) { - ZVAL_LONG(rv, (uintptr_t)*(void**)ptr); + ZVAL_STR(rv, zend_strpprintf(2 * SIZEOF_SIZE_T + sizeof("0x"), "%p", *(void**)ptr)); return; } else if ((type->attr & ZEND_FFI_ATTR_CONST) && ZEND_FFI_TYPE(type->pointer.type)->kind == ZEND_FFI_TYPE_CHAR) { ZVAL_STRING(rv, *(char**)ptr); diff --git a/ext/ffi/tests/bug79571.phpt b/ext/ffi/tests/bug79571.phpt index da80828acccb2..cff3fab43e2d9 100644 --- a/ext/ffi/tests/bug79571.phpt +++ b/ext/ffi/tests/bug79571.phpt @@ -23,6 +23,6 @@ object(FFI\CData:union )#%d (2) { ["num"]=> int(42) ["str"]=> - int(42) + string(4) "0x2a" } int(42) From 7ddbc44e070702ef45b520d6ee2f01d73cfb3076 Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Mon, 11 May 2020 14:35:27 +0200 Subject: [PATCH 4/4] Don't constrain the length --- ext/ffi/ffi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ext/ffi/ffi.c b/ext/ffi/ffi.c index e1a7e8db3461e..fa81868230871 100644 --- a/ext/ffi/ffi.c +++ b/ext/ffi/ffi.c @@ -515,7 +515,7 @@ static zend_always_inline void zend_ffi_cdata_to_zval(zend_ffi_cdata *cdata, voi ZVAL_NULL(rv); return; } else if (debug_union) { - ZVAL_STR(rv, zend_strpprintf(2 * SIZEOF_SIZE_T + sizeof("0x"), "%p", *(void**)ptr)); + ZVAL_STR(rv, zend_strpprintf(0, "%p", *(void**)ptr)); return; } else if ((type->attr & ZEND_FFI_ATTR_CONST) && ZEND_FFI_TYPE(type->pointer.type)->kind == ZEND_FFI_TYPE_CHAR) { ZVAL_STRING(rv, *(char**)ptr);