From a343b67154beb51d6c4f8eec82f64fb39d7f6485 Mon Sep 17 00:00:00 2001 From: Ilija Tovilo Date: Thu, 20 Jul 2023 13:28:02 +0200 Subject: [PATCH] Fix leaking definitions on FFI::cdef()->new() Previously, FFI_G(symbols) and FFI_G(tags) were never cleaned up when calling new on an existing object. However, if cdef() is called without parameters these globals are NULL and might be created when new() creates new definitions. These would then be discarded without freeing them. Furthermore, when calling new() on an existing object ffi->symbols and ffi->tags are reused for the operation. new() might add new definitions, leaking them into the next new() call. I'm not sure if this is desired behavior. --- ext/ffi/ffi.c | 112 ++++++++++++-------------- ext/ffi/tests/cdef_new.phpt | 14 ++++ ext/ffi/tests/leaking_definition.phpt | 21 +++++ 3 files changed, 88 insertions(+), 59 deletions(-) create mode 100644 ext/ffi/tests/cdef_new.phpt create mode 100644 ext/ffi/tests/leaking_definition.phpt diff --git a/ext/ffi/ffi.c b/ext/ffi/ffi.c index ee5183ce9d46c..efb4524c76b1a 100644 --- a/ext/ffi/ffi.c +++ b/ext/ffi/ffi.c @@ -3684,22 +3684,22 @@ ZEND_METHOD(FFI, new) /* {{{ */ FFI_G(symbols) = NULL; FFI_G(tags) = NULL; } + bool clean_symbols = FFI_G(symbols) == NULL; + bool clean_tags = FFI_G(tags) == NULL; FFI_G(default_type_attr) = 0; if (zend_ffi_parse_type(ZSTR_VAL(type_def), ZSTR_LEN(type_def), &dcl) != SUCCESS) { zend_ffi_type_dtor(dcl.type); - if (Z_TYPE(EX(This)) != IS_OBJECT) { - if (FFI_G(tags)) { - zend_hash_destroy(FFI_G(tags)); - efree(FFI_G(tags)); - FFI_G(tags) = NULL; - } - if (FFI_G(symbols)) { - zend_hash_destroy(FFI_G(symbols)); - efree(FFI_G(symbols)); - FFI_G(symbols) = NULL; - } + if (clean_tags && FFI_G(tags)) { + zend_hash_destroy(FFI_G(tags)); + efree(FFI_G(tags)); + FFI_G(tags) = NULL; + } + if (clean_symbols && FFI_G(symbols)) { + zend_hash_destroy(FFI_G(symbols)); + efree(FFI_G(symbols)); + FFI_G(symbols) = NULL; } return; } @@ -3709,15 +3709,13 @@ ZEND_METHOD(FFI, new) /* {{{ */ is_const = 1; } - if (Z_TYPE(EX(This)) != IS_OBJECT) { - if (FFI_G(tags)) { - zend_ffi_tags_cleanup(&dcl); - } - if (FFI_G(symbols)) { - zend_hash_destroy(FFI_G(symbols)); - efree(FFI_G(symbols)); - FFI_G(symbols) = NULL; - } + if (clean_tags && FFI_G(tags)) { + zend_ffi_tags_cleanup(&dcl); + } + if (clean_symbols && FFI_G(symbols)) { + zend_hash_destroy(FFI_G(symbols)); + efree(FFI_G(symbols)); + FFI_G(symbols) = NULL; } FFI_G(symbols) = NULL; FFI_G(tags) = NULL; @@ -3828,22 +3826,22 @@ ZEND_METHOD(FFI, cast) /* {{{ */ FFI_G(symbols) = NULL; FFI_G(tags) = NULL; } + bool clean_symbols = FFI_G(symbols) == NULL; + bool clean_tags = FFI_G(tags) == NULL; FFI_G(default_type_attr) = 0; if (zend_ffi_parse_type(ZSTR_VAL(type_def), ZSTR_LEN(type_def), &dcl) != SUCCESS) { zend_ffi_type_dtor(dcl.type); - if (Z_TYPE(EX(This)) != IS_OBJECT) { - if (FFI_G(tags)) { - zend_hash_destroy(FFI_G(tags)); - efree(FFI_G(tags)); - FFI_G(tags) = NULL; - } - if (FFI_G(symbols)) { - zend_hash_destroy(FFI_G(symbols)); - efree(FFI_G(symbols)); - FFI_G(symbols) = NULL; - } + if (clean_tags && FFI_G(tags)) { + zend_hash_destroy(FFI_G(tags)); + efree(FFI_G(tags)); + FFI_G(tags) = NULL; + } + if (clean_symbols && FFI_G(symbols)) { + zend_hash_destroy(FFI_G(symbols)); + efree(FFI_G(symbols)); + FFI_G(symbols) = NULL; } return; } @@ -3853,15 +3851,13 @@ ZEND_METHOD(FFI, cast) /* {{{ */ is_const = 1; } - if (Z_TYPE(EX(This)) != IS_OBJECT) { - if (FFI_G(tags)) { - zend_ffi_tags_cleanup(&dcl); - } - if (FFI_G(symbols)) { - zend_hash_destroy(FFI_G(symbols)); - efree(FFI_G(symbols)); - FFI_G(symbols) = NULL; - } + if (clean_tags && FFI_G(tags)) { + zend_ffi_tags_cleanup(&dcl); + } + if (clean_symbols && FFI_G(symbols)) { + zend_hash_destroy(FFI_G(symbols)); + efree(FFI_G(symbols)); + FFI_G(symbols) = NULL; } FFI_G(symbols) = NULL; FFI_G(tags) = NULL; @@ -3994,35 +3990,33 @@ ZEND_METHOD(FFI, type) /* {{{ */ FFI_G(symbols) = NULL; FFI_G(tags) = NULL; } + bool clean_symbols = FFI_G(symbols) == NULL; + bool clean_tags = FFI_G(tags) == NULL; FFI_G(default_type_attr) = 0; if (zend_ffi_parse_type(ZSTR_VAL(type_def), ZSTR_LEN(type_def), &dcl) != SUCCESS) { zend_ffi_type_dtor(dcl.type); - if (Z_TYPE(EX(This)) != IS_OBJECT) { - if (FFI_G(tags)) { - zend_hash_destroy(FFI_G(tags)); - efree(FFI_G(tags)); - FFI_G(tags) = NULL; - } - if (FFI_G(symbols)) { - zend_hash_destroy(FFI_G(symbols)); - efree(FFI_G(symbols)); - FFI_G(symbols) = NULL; - } - } - return; - } - - if (Z_TYPE(EX(This)) != IS_OBJECT) { - if (FFI_G(tags)) { - zend_ffi_tags_cleanup(&dcl); + if (clean_tags && FFI_G(tags)) { + zend_hash_destroy(FFI_G(tags)); + efree(FFI_G(tags)); + FFI_G(tags) = NULL; } - if (FFI_G(symbols)) { + if (clean_symbols && FFI_G(symbols)) { zend_hash_destroy(FFI_G(symbols)); efree(FFI_G(symbols)); FFI_G(symbols) = NULL; } + return; + } + + if (clean_tags && FFI_G(tags)) { + zend_ffi_tags_cleanup(&dcl); + } + if (clean_symbols && FFI_G(symbols)) { + zend_hash_destroy(FFI_G(symbols)); + efree(FFI_G(symbols)); + FFI_G(symbols) = NULL; } FFI_G(symbols) = NULL; FFI_G(tags) = NULL; diff --git a/ext/ffi/tests/cdef_new.phpt b/ext/ffi/tests/cdef_new.phpt new file mode 100644 index 0000000000000..ca3cba5d459fd --- /dev/null +++ b/ext/ffi/tests/cdef_new.phpt @@ -0,0 +1,14 @@ +--TEST-- +Definitions should not leak when using FFI::cdef()->new(...) +--EXTENSIONS-- +ffi +--FILE-- +new('struct Example { uint32_t x; }'); +var_dump($struct); +?> +--EXPECT-- +object(FFI\CData:struct Example)#2 (1) { + ["x"]=> + int(0) +} diff --git a/ext/ffi/tests/leaking_definition.phpt b/ext/ffi/tests/leaking_definition.phpt new file mode 100644 index 0000000000000..17bb195eee65c --- /dev/null +++ b/ext/ffi/tests/leaking_definition.phpt @@ -0,0 +1,21 @@ +--TEST-- +FFI::cdef()->new() shouldn't leak definitions into outer scope +--EXTENSIONS-- +ffi +--XFAIL-- +--FILE-- +new('struct Example2 { uint32_t x; }')); +try { + var_dump($ffi->new('struct Example2')); +} catch (\FFI\ParserException $e) { + echo $e->getMessage(), "\n"; +} +?> +--EXPECTF-- +object(FFI\CData:struct Example2)#2 (1) { + ["x"]=> + int(0) +} +Incomplete struct "Example2" at line %d