[SanderMuller]

The native ReturnHandler took the throw points, impure points and dependencies of the returned expression as pointers into its ExpressionResult. It released that result at the end of an inner block, before the node callback ran. If a rule then calls getType() on the returned expression, for example from a ClassMethod callback, the freed memory can be reused. The statement result then gets an unrelated object as its impure points.

On 2.3.2 this crashes a project that uses NarrowReturnObjectTypeRule from tomasvotruba/type-coverage on a method that returns once(function () { ... }):

Internal error: array_merge(): Argument #2 must be of type array, PHPStan\Type\ClosureType given

The fix keeps the result alive until the end of processStmt(). With the extension loaded, the new test fails with that error without the fix and passes with it. The other statement handlers already keep their result alive while they use it.

🤖 Generated with Claude Code

[staabm]

what's the root cause of this problem? do other handlers have the same issue?

[SanderMuller]

The native ReturnHandler kept pointers to the throw points, impure points and dependencies of the returned expression's ExpressionResult. The accessors return a pointer into the object's property table without a reference count. The ExpressionResult was declared in an inner block, so it was freed when that block ended. The pointers were used after the node callback, and a rule calling getType() in that callback reused the freed memory. The PHP ReturnHandler keeps the result alive, so only the native one had the bug.

I checked the other native handlers for the same pattern and found none. I looked for a pointer read from a result that is freed, or used after a callback while the result can change. NewHandler and StaticCallHandler have the same inner-block shape, but they copy the throw points before the block ends. Pointers into a live ExpressionResult are safe, because its properties are only set in the constructor.

[staabm]

my feeling is that the underlying problem is something we should be able to detect in the CI pipeline (for the bigger picture; separate PR).

would the underlying problem be detectable with a static code analysis tool for the cpp code?
(I don't know whether ondrej would like this direction; I just sharing my thoughts)

[SanderMuller]

I tried both on the pre-fix ReturnHandler.

Static: Apple clang 21 finds nothing, even with [[clang::lifetimebound]] on the accessors and -fexperimental-lifetime-safety. The static analyzer finds nothing too. It does not even flag a plain pointer to an int declared in an inner block and read after it. I did not try clang-tidy.

AddressSanitizer: a -fsanitize=address build of the extension, run with USE_ZEND_ALLOC=0, reports the regression test as heap-use-after-free, freed at ReturnHandler.cpp:127 and read at :156. But NodeScopeResolverTest, ReturnTypeRuleTest and an analysis of src/Type do not trigger it, because the bad read needs the result's last reference to be gone. An ASan CI job would help, but mainly over a large corpus. make sanitize-turbo runs UBSan today, which does not see use-after-free.