From 134aa524d8569d8c771a62e73b6e28e5f67e51d5 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Sat, 6 Jul 2024 01:10:04 +0200 Subject: [PATCH 1/4] GH-14286 (ffi enum type (when enum has no name) make memory leak) For top-level anonymous type definition we never store the declaration anywhere else nor the type anywhere else. The declaration keeps owning the type and it goes out of scope. For anonymous fields this gets handled by the add_anonymous_field code that removes the type from the declaration. This patch does something similar in the parsing code when it is detected we're dealing with an anonymous enum in a top-level declaration. --- ext/ffi/ffi.c | 2 +- ext/ffi/ffi.g | 1 + ext/ffi/ffi_parser.c | 1 + ext/ffi/php_ffi.h | 1 + ext/ffi/tests/gh14286.phpt | 40 ++++++++++++++++++++++++++++++++++++++ 5 files changed, 44 insertions(+), 1 deletion(-) create mode 100644 ext/ffi/tests/gh14286.phpt diff --git a/ext/ffi/ffi.c b/ext/ffi/ffi.c index 560338e71f33..8b0b58105d32 100644 --- a/ext/ffi/ffi.c +++ b/ext/ffi/ffi.c @@ -3544,7 +3544,7 @@ ZEND_METHOD(FFI, scope) /* {{{ */ } /* }}} */ -static void zend_ffi_cleanup_dcl(zend_ffi_dcl *dcl) /* {{{ */ +void zend_ffi_cleanup_dcl(zend_ffi_dcl *dcl) /* {{{ */ { if (dcl) { zend_ffi_type_dtor(dcl->type); diff --git a/ext/ffi/ffi.g b/ext/ffi/ffi.g index b70e3f129962..2b8a11bf24fd 100644 --- a/ext/ffi/ffi.g +++ b/ext/ffi/ffi.g @@ -137,6 +137,7 @@ declaration_specifiers(zend_ffi_dcl *dcl): | attributes(dcl) | type_qualifier(dcl) | type_specifier(dcl) + {if (((dcl->flags & (ZEND_FFI_DCL_ENUM | ZEND_FFI_DCL_STORAGE_CLASS)) == ZEND_FFI_DCL_ENUM)) zend_ffi_cleanup_dcl(dcl);} ) )+ ; diff --git a/ext/ffi/ffi_parser.c b/ext/ffi/ffi_parser.c index b956f885ee00..b595d7a72f40 100644 --- a/ext/ffi/ffi_parser.c +++ b/ext/ffi/ffi_parser.c @@ -2166,6 +2166,7 @@ static int parse_declaration_specifiers(int sym, zend_ffi_dcl *dcl) { case YY_ENUM: case YY_ID: sym = parse_type_specifier(sym, dcl); + if (((dcl->flags & (ZEND_FFI_DCL_ENUM | ZEND_FFI_DCL_STORAGE_CLASS)) == ZEND_FFI_DCL_ENUM)) zend_ffi_cleanup_dcl(dcl); break; default: yy_error_sym("unexpected", sym); diff --git a/ext/ffi/php_ffi.h b/ext/ffi/php_ffi.h index 02a241c6bb69..430b8a2e568f 100644 --- a/ext/ffi/php_ffi.h +++ b/ext/ffi/php_ffi.h @@ -208,6 +208,7 @@ typedef struct _zend_ffi_val { zend_result zend_ffi_parse_decl(const char *str, size_t len); zend_result zend_ffi_parse_type(const char *str, size_t len, zend_ffi_dcl *dcl); +void zend_ffi_cleanup_dcl(zend_ffi_dcl *dcl); /* parser callbacks */ void ZEND_NORETURN zend_ffi_parser_error(const char *msg, ...); diff --git a/ext/ffi/tests/gh14286.phpt b/ext/ffi/tests/gh14286.phpt new file mode 100644 index 000000000000..1d326919e2ab --- /dev/null +++ b/ext/ffi/tests/gh14286.phpt @@ -0,0 +1,40 @@ +--TEST-- +GH-14286 (ffi enum type (when enum has no name) make memory leak) +--EXTENSIONS-- +ffi +--INI-- +ffi.enable=1 +--FILE-- +TEST_ONE); +var_dump($ffi->TEST_TWO); +var_dump($ffi->TEST_THREE); +var_dump($ffi->TEST_FOUR); +var_dump($ffi->TEST_FIVE); +var_dump($ffi->TEST_SIX); +?> +--EXPECT-- +int(1) +int(2) +int(3) +int(4) +int(5) +int(6) From 443269822dd92fac821dd7a437cb73f2071cfff5 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Mon, 22 Jul 2024 15:47:44 +0200 Subject: [PATCH 2/4] Fix code review problem --- ext/ffi/ffi.g | 4 +++- ext/ffi/ffi_parser.c | 4 +++- ext/ffi/tests/gh14286.phpt | 12 ++++++++++++ 3 files changed, 18 insertions(+), 2 deletions(-) diff --git a/ext/ffi/ffi.g b/ext/ffi/ffi.g index 2b8a11bf24fd..8b9974355a2b 100644 --- a/ext/ffi/ffi.g +++ b/ext/ffi/ffi.g @@ -70,6 +70,7 @@ static void yy_error_sym(const char *msg, int sym); declarations: ( {zend_ffi_dcl common_dcl = ZEND_FFI_ATTR_INIT;} + {bool has_name = false;} "__extension__"? declaration_specifiers(&common_dcl) ( @@ -86,6 +87,7 @@ declarations: /*TODO*/ ")" )? + {has_name = true;} attributes(&dcl)? initializer? {zend_ffi_declare(name, name_len, &dcl);} @@ -97,6 +99,7 @@ declarations: {zend_ffi_declare(name, name_len, &dcl);} )* )? + {if (!has_name && ((common_dcl.flags & (ZEND_FFI_DCL_ENUM | ZEND_FFI_DCL_STORAGE_CLASS)) == ZEND_FFI_DCL_ENUM)) zend_ffi_cleanup_dcl(&common_dcl);} ";" )* ; @@ -137,7 +140,6 @@ declaration_specifiers(zend_ffi_dcl *dcl): | attributes(dcl) | type_qualifier(dcl) | type_specifier(dcl) - {if (((dcl->flags & (ZEND_FFI_DCL_ENUM | ZEND_FFI_DCL_STORAGE_CLASS)) == ZEND_FFI_DCL_ENUM)) zend_ffi_cleanup_dcl(dcl);} ) )+ ; diff --git a/ext/ffi/ffi_parser.c b/ext/ffi/ffi_parser.c index b595d7a72f40..104404df02d2 100644 --- a/ext/ffi/ffi_parser.c +++ b/ext/ffi/ffi_parser.c @@ -2012,6 +2012,7 @@ static int synpred_6(int sym) { static int parse_declarations(int sym) { while (YY_IN_SET(sym, (YY___EXTENSION__,YY_TYPEDEF,YY_EXTERN,YY_STATIC,YY_AUTO,YY_REGISTER,YY_INLINE,YY___INLINE,YY___INLINE__,YY__NORETURN,YY__ALIGNAS,YY___ATTRIBUTE,YY___ATTRIBUTE__,YY___DECLSPEC,YY___CDECL,YY___STDCALL,YY___FASTCALL,YY___THISCALL,YY___VECTORCALL,YY_CONST,YY___CONST,YY___CONST__,YY_RESTRICT,YY___RESTRICT,YY___RESTRICT__,YY_VOLATILE,YY___VOLATILE,YY___VOLATILE__,YY__ATOMIC,YY_VOID,YY_CHAR,YY_SHORT,YY_INT,YY_LONG,YY_FLOAT,YY_DOUBLE,YY_SIGNED,YY_UNSIGNED,YY__BOOL,YY__COMPLEX,YY_COMPLEX,YY___COMPLEX,YY___COMPLEX__,YY_STRUCT,YY_UNION,YY_ENUM,YY_ID), "\202\377\377\377\377\107\360\017\000\000\000\002\000")) { zend_ffi_dcl common_dcl = ZEND_FFI_ATTR_INIT; + bool has_name = false; if (sym == YY___EXTENSION__) { sym = get_sym(); } @@ -2037,6 +2038,7 @@ static int parse_declarations(int sym) { } sym = get_sym(); } + has_name = true; if (YY_IN_SET(sym, (YY___ATTRIBUTE,YY___ATTRIBUTE__,YY___DECLSPEC,YY___CDECL,YY___STDCALL,YY___FASTCALL,YY___THISCALL,YY___VECTORCALL), "\000\000\000\000\000\000\360\017\000\000\000\000\000")) { sym = parse_attributes(sym, &dcl); } @@ -2057,6 +2059,7 @@ static int parse_declarations(int sym) { zend_ffi_declare(name, name_len, &dcl); } } + if (!has_name && ((common_dcl.flags & (ZEND_FFI_DCL_ENUM | ZEND_FFI_DCL_STORAGE_CLASS)) == ZEND_FFI_DCL_ENUM)) zend_ffi_cleanup_dcl(&common_dcl); if (sym != YY__SEMICOLON) { yy_error_sym("';' expected, got", sym); } @@ -2166,7 +2169,6 @@ static int parse_declaration_specifiers(int sym, zend_ffi_dcl *dcl) { case YY_ENUM: case YY_ID: sym = parse_type_specifier(sym, dcl); - if (((dcl->flags & (ZEND_FFI_DCL_ENUM | ZEND_FFI_DCL_STORAGE_CLASS)) == ZEND_FFI_DCL_ENUM)) zend_ffi_cleanup_dcl(dcl); break; default: yy_error_sym("unexpected", sym); diff --git a/ext/ffi/tests/gh14286.phpt b/ext/ffi/tests/gh14286.phpt index 1d326919e2ab..80e5d647ed50 100644 --- a/ext/ffi/tests/gh14286.phpt +++ b/ext/ffi/tests/gh14286.phpt @@ -30,6 +30,17 @@ var_dump($ffi->TEST_THREE); var_dump($ffi->TEST_FOUR); var_dump($ffi->TEST_FIVE); var_dump($ffi->TEST_SIX); + +try { + $ffi = FFI::cdef(" + enum { + TEST_ONE=1, + TEST_TWO=2, + } x; + "); +} catch (Throwable $e) { + echo $e->getMessage(), "\n"; +} ?> --EXPECT-- int(1) @@ -38,3 +49,4 @@ int(3) int(4) int(5) int(6) +Failed resolving C variable 'x' From 573b404680163d7ed52175a96f8e3accea83e254 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Mon, 22 Jul 2024 16:25:05 +0200 Subject: [PATCH 3/4] Review fixes --- ext/ffi/ffi.g | 8 +++--- ext/ffi/ffi_parser.c | 7 +++--- .../tests/{gh14286.phpt => gh14286_1.phpt} | 12 --------- ext/ffi/tests/gh14286_2.phpt | 25 +++++++++++++++++++ 4 files changed, 33 insertions(+), 19 deletions(-) rename ext/ffi/tests/{gh14286.phpt => gh14286_1.phpt} (76%) create mode 100644 ext/ffi/tests/gh14286_2.phpt diff --git a/ext/ffi/ffi.g b/ext/ffi/ffi.g index 8b9974355a2b..61ddeb205a0f 100644 --- a/ext/ffi/ffi.g +++ b/ext/ffi/ffi.g @@ -70,7 +70,6 @@ static void yy_error_sym(const char *msg, int sym); declarations: ( {zend_ffi_dcl common_dcl = ZEND_FFI_ATTR_INIT;} - {bool has_name = false;} "__extension__"? declaration_specifiers(&common_dcl) ( @@ -87,7 +86,6 @@ declarations: /*TODO*/ ")" )? - {has_name = true;} attributes(&dcl)? initializer? {zend_ffi_declare(name, name_len, &dcl);} @@ -98,8 +96,10 @@ declarations: initializer? {zend_ffi_declare(name, name_len, &dcl);} )* - )? - {if (!has_name && ((common_dcl.flags & (ZEND_FFI_DCL_ENUM | ZEND_FFI_DCL_STORAGE_CLASS)) == ZEND_FFI_DCL_ENUM)) zend_ffi_cleanup_dcl(&common_dcl);} + | + /* empty */ + {if (common_dcl.flags & ZEND_FFI_DCL_ENUM) zend_ffi_cleanup_dcl(&common_dcl);} + ) ";" )* ; diff --git a/ext/ffi/ffi_parser.c b/ext/ffi/ffi_parser.c index 104404df02d2..64d6fceaff85 100644 --- a/ext/ffi/ffi_parser.c +++ b/ext/ffi/ffi_parser.c @@ -2012,7 +2012,6 @@ static int synpred_6(int sym) { static int parse_declarations(int sym) { while (YY_IN_SET(sym, (YY___EXTENSION__,YY_TYPEDEF,YY_EXTERN,YY_STATIC,YY_AUTO,YY_REGISTER,YY_INLINE,YY___INLINE,YY___INLINE__,YY__NORETURN,YY__ALIGNAS,YY___ATTRIBUTE,YY___ATTRIBUTE__,YY___DECLSPEC,YY___CDECL,YY___STDCALL,YY___FASTCALL,YY___THISCALL,YY___VECTORCALL,YY_CONST,YY___CONST,YY___CONST__,YY_RESTRICT,YY___RESTRICT,YY___RESTRICT__,YY_VOLATILE,YY___VOLATILE,YY___VOLATILE__,YY__ATOMIC,YY_VOID,YY_CHAR,YY_SHORT,YY_INT,YY_LONG,YY_FLOAT,YY_DOUBLE,YY_SIGNED,YY_UNSIGNED,YY__BOOL,YY__COMPLEX,YY_COMPLEX,YY___COMPLEX,YY___COMPLEX__,YY_STRUCT,YY_UNION,YY_ENUM,YY_ID), "\202\377\377\377\377\107\360\017\000\000\000\002\000")) { zend_ffi_dcl common_dcl = ZEND_FFI_ATTR_INIT; - bool has_name = false; if (sym == YY___EXTENSION__) { sym = get_sym(); } @@ -2038,7 +2037,6 @@ static int parse_declarations(int sym) { } sym = get_sym(); } - has_name = true; if (YY_IN_SET(sym, (YY___ATTRIBUTE,YY___ATTRIBUTE__,YY___DECLSPEC,YY___CDECL,YY___STDCALL,YY___FASTCALL,YY___THISCALL,YY___VECTORCALL), "\000\000\000\000\000\000\360\017\000\000\000\000\000")) { sym = parse_attributes(sym, &dcl); } @@ -2058,8 +2056,11 @@ static int parse_declarations(int sym) { } zend_ffi_declare(name, name_len, &dcl); } + } else if (sym == YY__SEMICOLON) { + if (common_dcl.flags & ZEND_FFI_DCL_ENUM) zend_ffi_cleanup_dcl(&common_dcl); + } else { + yy_error_sym("unexpected", sym); } - if (!has_name && ((common_dcl.flags & (ZEND_FFI_DCL_ENUM | ZEND_FFI_DCL_STORAGE_CLASS)) == ZEND_FFI_DCL_ENUM)) zend_ffi_cleanup_dcl(&common_dcl); if (sym != YY__SEMICOLON) { yy_error_sym("';' expected, got", sym); } diff --git a/ext/ffi/tests/gh14286.phpt b/ext/ffi/tests/gh14286_1.phpt similarity index 76% rename from ext/ffi/tests/gh14286.phpt rename to ext/ffi/tests/gh14286_1.phpt index 80e5d647ed50..1d326919e2ab 100644 --- a/ext/ffi/tests/gh14286.phpt +++ b/ext/ffi/tests/gh14286_1.phpt @@ -30,17 +30,6 @@ var_dump($ffi->TEST_THREE); var_dump($ffi->TEST_FOUR); var_dump($ffi->TEST_FIVE); var_dump($ffi->TEST_SIX); - -try { - $ffi = FFI::cdef(" - enum { - TEST_ONE=1, - TEST_TWO=2, - } x; - "); -} catch (Throwable $e) { - echo $e->getMessage(), "\n"; -} ?> --EXPECT-- int(1) @@ -49,4 +38,3 @@ int(3) int(4) int(5) int(6) -Failed resolving C variable 'x' diff --git a/ext/ffi/tests/gh14286_2.phpt b/ext/ffi/tests/gh14286_2.phpt new file mode 100644 index 000000000000..683929780c05 --- /dev/null +++ b/ext/ffi/tests/gh14286_2.phpt @@ -0,0 +1,25 @@ +--TEST-- +GH-14286 (ffi enum type (when enum has no name) make memory leak) +--EXTENSIONS-- +ffi +--SKIPIF-- + +--INI-- +ffi.enable=1 +--FILE-- +getMessage(), "\n"; +} +?> +--EXPECT-- +Failed resolving C variable 'x' From 2cb5e9f298203f47e0f8dc61baca3b4993038f7e Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Mon, 22 Jul 2024 17:09:47 +0200 Subject: [PATCH 4/4] Fix more leaks --- ext/ffi/ffi.g | 2 +- ext/ffi/ffi_parser.c | 2 +- ext/ffi/tests/gh14286_1.phpt | 6 ++++++ 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/ext/ffi/ffi.g b/ext/ffi/ffi.g index 61ddeb205a0f..6628386a5079 100644 --- a/ext/ffi/ffi.g +++ b/ext/ffi/ffi.g @@ -98,7 +98,7 @@ declarations: )* | /* empty */ - {if (common_dcl.flags & ZEND_FFI_DCL_ENUM) zend_ffi_cleanup_dcl(&common_dcl);} + {if (common_dcl.flags & (ZEND_FFI_DCL_ENUM | ZEND_FFI_DCL_STRUCT | ZEND_FFI_DCL_UNION)) zend_ffi_cleanup_dcl(&common_dcl);} ) ";" )* diff --git a/ext/ffi/ffi_parser.c b/ext/ffi/ffi_parser.c index 64d6fceaff85..94d50d59b5d3 100644 --- a/ext/ffi/ffi_parser.c +++ b/ext/ffi/ffi_parser.c @@ -2057,7 +2057,7 @@ static int parse_declarations(int sym) { zend_ffi_declare(name, name_len, &dcl); } } else if (sym == YY__SEMICOLON) { - if (common_dcl.flags & ZEND_FFI_DCL_ENUM) zend_ffi_cleanup_dcl(&common_dcl); + if (common_dcl.flags & (ZEND_FFI_DCL_ENUM | ZEND_FFI_DCL_STRUCT | ZEND_FFI_DCL_UNION)) zend_ffi_cleanup_dcl(&common_dcl); } else { yy_error_sym("unexpected", sym); } diff --git a/ext/ffi/tests/gh14286_1.phpt b/ext/ffi/tests/gh14286_1.phpt index 1d326919e2ab..19701f634a63 100644 --- a/ext/ffi/tests/gh14286_1.phpt +++ b/ext/ffi/tests/gh14286_1.phpt @@ -23,6 +23,12 @@ $ffi = FFI::cdef(" } test2; }; typedef enum { TEST_SIX=6 } TestEnum3; + struct { + int x; + }; + union { + int x; + }; "); var_dump($ffi->TEST_ONE); var_dump($ffi->TEST_TWO);