GH-14286 (ffi enum type (when enum has no name) make memory leak) #14839
Conversation
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_parser.c
Outdated
| @@ -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); | |||
There was a problem hiding this comment.
This seems not enough. The following code starts to fail.
<?php
$ffi = FFI::cdef("
enum {
TEST_ONE=1,
TEST_TWO=2,
} x;
");
?>Uncaught FFI\ParserException: Unsupported type specifier combination at line 5
ext/ffi/ffi.g
Outdated
| )? | ||
| {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);} |
There was a problem hiding this comment.
This may be done in a simpler way.
diff --git a/ext/ffi/ffi.g b/ext/ffi/ffi.g
index e30d86b21c7..7d9646d2961 100644
--- a/ext/ffi/ffi.g
+++ b/ext/ffi/ffi.g
@@ -97,7 +97,10 @@ declarations:
initializer?
{zend_ffi_declare(name, name_len, &dcl);}
)*
- )?
+ |
+ /* empty */
+ {if ((common_dcl.flags & (ZEND_FFI_DCL_ENUM | ZEND_FFI_DCL_STORAGE_CLASS)) == ZEND_FFI_DCL_ENUM) zend_ffi_cleanup_dcl(&common_dcl);}
+ )
";"
)*
;Most probably the ((common_dcl.flags & (ZEND_FFI_DCL_ENUM | ZEND_FFI_DCL_STORAGE_CLASS)) == ZEND_FFI_DCL_ENUM) check also may be simplified.
ext/ffi/ffi.g
Outdated
| )? | ||
| | | ||
| /* empty */ | ||
| {if (common_dcl.flags & ZEND_FFI_DCL_ENUM) zend_ffi_cleanup_dcl(&common_dcl);} |
There was a problem hiding this comment.
This looks right.
I'm still not completely sure if common_dcl.flags & ZEND_FFI_DCL_ENUM is the right condition.
Currently we may also have similar structures and they also may lead to memory leaks.
GCC allows this syntax with a warning: unnamed struct/union that defines no instances.
There was a problem hiding this comment.
Ah indeed it seems the following leaks too:
$ffi = FFI::cdef("
struct {
int x;
};
union {
int x;
};
");I think besides those are the remaining two indeed. I changed the condition. Thanks.
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.