Skip to content

Fix GH-24582: Memory leaks in ext/ffi parser when finalize_type() bails out - #24591

Closed
matyapiro31 wants to merge 2 commits into
php:PHP-8.4from
matyapiro31:PHP-8.4
Closed

matyapiro31 wants to merge 2 commits into
php:PHP-8.4from
matyapiro31:PHP-8.4

Conversation

@matyapiro31

Copy link
Copy Markdown
Contributor

Problem

zend_ffi_finalize_type() calls zend_ffi_parser_error() for unsupported specifier combinations, which always ends in LONGJMP(FFI_G(bailout), FAILURE). It only does so when dcl->type == NULL, so dcl owns nothing, but four callbacks own other memory at that point:

callback leaked
zend_ffi_make_pointer_type() the type it had just allocated
zend_ffi_make_func_type() args, nested_dcl
zend_ffi_add_arg() *args, including the arguments collected so far
zend_ffi_nested_declaration() nested_dcl

Reproducer (a debug build reports it at shutdown):

FFI::cdef("double _Complex *x;");   // FFI\ParserException: Unsupported type _Complex

Change

  • zend_ffi_try_finalize_type() throws the parser exception and returns FAILURE, like the zend_ffi_validate_* helpers. zend_ffi_finalize_type() stays as a wrapper that bails out, so the other callers are not touched.
  • The four callbacks use the new function and release what they own before LONGJMP, with the same calls the later error paths of the same functions already use.
  • zend_ffi_make_pointer_type() allocates after the checks, as zend_ffi_make_array_type() does.

Testing

Debug ZTS build on 8.4.28-dev (Windows, Visual C++ 2022):

  • New test ext/ffi/tests/finalize_type_bailout_leak.phpt checks the exception message and that memory_get_usage() does not grow, so it also detects the leak on non-debug builds. Without the fix it reports a leak for all seven declarations; with the fix it passes.
  • ext/ffi suite: 87 passed, 0 failed (24 skipped, no libc.so.6 on Windows).
  • Double free: the default debug allocator does not detect a double free (I checked with a deliberate one), so I also ran the suite and 42 declarations x 300 runs with USE_ZEND_ALLOC=0: no crash.
  • 42 declarations (nested declarators, function pointers, struct fields, typedefs, valid controls), one process each: unpatched 33 leak, patched 10; 23 fully fixed, 7 reduced, none worse.

Not covered

Ten of those declarations still leak. The cause is different: a callee bails out while an enclosing declarator (nested_dcl of parse_declarator(), or the dcl of an outer *) owns a type the callee cannot reach, for example double _Complex (*x)[3];, int *f(double _Complex x); and, without _Complex, int *f(@);. A proper fix needs to be at the parser level; see the discussion in GH-24582. If that is the direction, the cleanups here have to be dropped with it to avoid releasing a type twice.

The persistent (ffi.preload) path was not run (no opcache in my build); the new code uses the same FFI_G(persistent) expressions as the existing error paths.

🤖 Generated with Claude Code

✔ Checked by me.

matyapiro31 and others added 2 commits October 23, 2026 08:59
…bails out

zend_ffi_finalize_type() reports unsupported specifier combinations
(e.g. _Complex) through zend_ffi_parser_error(), which always ends in
LONGJMP(). Four parser callbacks called it while they already owned memory
that is not reachable from dcl->type, so every failed FFI::cdef(),
FFI::new() or FFI::type() leaked it:

- zend_ffi_make_pointer_type(): the new pointer type
- zend_ffi_make_func_type(): args and nested_dcl
- zend_ffi_add_arg(): *args (including the arguments collected so far)
- zend_ffi_nested_declaration(): nested_dcl

Add zend_ffi_try_finalize_type(), which throws the parser exception and
returns FAILURE instead of bailing out, and keep zend_ffi_finalize_type()
as a thin wrapper that bails out so that the other callers are unchanged.
The four callbacks above release what they own before bailing out, using
the same calls as the later error paths of the same functions.
zend_ffi_make_pointer_type() now allocates after the checks, like
zend_ffi_make_array_type() already does.

Closes phpGH-24582

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…bails out php#2

Add ext/ffi/tests/finalize_type_bailout_leak.phpt for test.
@devnexen

Copy link
Copy Markdown
Member

It seems a bit too intrusive of a change for a stable branch, it might be doable to do something along these lines

diff --git a/ext/ffi/ffi.c b/ext/ffi/ffi.c
--- a/ext/ffi/ffi.c
+++ b/ext/ffi/ffi.c
@@ -3644,6 +3644,9 @@ static void zend_ffi_throw_parser_error(const char *format, ...) /* {{{ */
        va_list va;
        char *message = NULL;

+       if (FFI_G(parse_failed)) {
+               goto done;
+       }
        va_start(va, format);
        zend_vspprintf(&message, 0, format, va);

@@ -3655,6 +3658,8 @@ static void zend_ffi_throw_parser_error(const char *format, ...) /* {{{ */

        efree(message);
        va_end(va);
+done:
+       ;
 }
 /* }}} */

@@ -5755,6 +5760,9 @@ void zend_ffi_parser_error(const char *format, ...) /* {{{ */
        va_list va;
        char *message = NULL;

+       if (FFI_G(parse_failed)) {
+               goto done;
+       }
        va_start(va, format);
        zend_vspprintf(&message, 0, format, va);

@@ -5766,7 +5774,7 @@ void zend_ffi_parser_error(const char *format,

        efree(message);
        va_end(va);
-
+done:
        LONGJMP(FFI_G(bailout), FAILURE);
 }
 /* }}} */
@@ -5851,10 +5859,14 @@ static void zend_ffi_finalize_type(zend_ffi_dcl *dcl) /* {{{ */
                        case ZEND_FFI_DCL_FLOAT|ZEND_FFI_DCL_COMPLEX
                        case ZEND_FFI_DCL_DOUBLE|ZEND_FFI_DCL_COMPLEX:
                        case ZEND_FFI_DCL_DOUBLE|ZEND_FFI_DCL_LONG|Z
-                               zend_ffi_parser_error("Unsupported type _Complex at line %d", FFI_G(line));
+                               zend_ffi_throw_parser_error("Unsuppo", FFI_G(line));
+                               FFI_G(parse_failed) = 1;
+                               dcl->type = (zend_ffi_type*)&zend_ff
                                break;
                        default:
-                               zend_ffi_parser_error("Unsupported type specifier combination at line %d", FFI_G(line));
+                               zend_ffi_throw_parser_error("Unsuppoon at line %d", FFI_G(line));
+                               FFI_G(parse_failed) = 1;
+                               dcl->type = (zend_ffi_type*)&zend_ff
                                break;
                }
                dcl->flags &= ~ZEND_FFI_DCL_TYPE_SPECIFIERS;
diff --git a/ext/ffi/ffi.g b/ext/ffi/ffi.g
--- a/ext/ffi/ffi.g
+++ b/ext/ffi/ffi.g
@@ -890,9 +890,15 @@ zend_result zend_ffi_parse_decl(const char *str, size_t len) {
                FFI_G(attribute_parsing) = 0;
                yy_buf = (unsigned char*)str;
                yy_end = yy_buf + len;
+               FFI_G(parse_failed) = 0;
                parse();
+               if (FFI_G(parse_failed)) {
+                       FFI_G(parse_failed) = 0;
+                       return FAILURE;
+               }
                return SUCCESS;
        } else {
+               FFI_G(parse_failed) = 0;
                return FAILURE;
        }
 }
@@ -906,13 +912,19 @@ zend_result zend_ffi_parse_type(const char *str, size_t len, zend_ffi_dcl *dcl)
                yy_pos = yy_text = yy_buf = (unsigned char*)str;
                yy_end = yy_buf + len;
                yy_line = 1;
+               FFI_G(parse_failed) = 0;
                sym = parse_type_name(get_sym(), dcl);
                if (sym != YY_EOF) {
                        yy_error_sym("<EOF> expected, got", sym);
                }
                zend_ffi_validate_type_name(dcl);
+               if (FFI_G(parse_failed)) {
+                       FFI_G(parse_failed) = 0;
+                       return FAILURE;
+               }
                return SUCCESS;
        } else {
+               FFI_G(parse_failed) = 0;
                return FAILURE;
        };
 }
diff --git a/ext/ffi/ffi_parser.c b/ext/ffi/ffi_parser.c
    (same two hunks as ffi.g, at lines 3623 and 3639)
diff --git a/ext/ffi/php_ffi.h b/ext/ffi/php_ffi.h
--- a/ext/ffi/php_ffi.h
+++ b/ext/ffi/php_ffi.h
@@ -57,6 +57,7 @@ ZEND_BEGIN_MODULE_GLOBALS(ffi)
        bool allow_vla;
        bool attribute_parsing;
        bool persistent;
+       bool parse_failed;
        uint32_t  default_type_attr;
 ZEND_END_MODULE_GLOBALS(ffi)

@matyapiro31

Copy link
Copy Markdown
Contributor Author

@devnexen I newly created #24630 as less intrusive commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants