[danog]

Problem

Found while enabling taint analysis on a large codebase: a source returned by an implementation wasn't reported when the method was called through its interface.

interface Named {
    public function name(): string;
}
final class Tainted implements Named {
    public function name(): string { return (string) $_GET["name"]; }
}

function show(Named $named): void {
    echo $named->name(); // not reported
}

The same happened for a call on a base class whose method a subclass overrides, for an abstract method, and for static:: calls.

Cause

A call on an interface or base class sends its arguments to the parameters of the implementations and overrides, so a sink in their bodies is found. The other way isn't done: the result of the call only gets the return node of the method it names (Named::name), never those of the methods that may actually run.

Fix

• A virtual call of a method on a class with subclasses or implementations now also takes its result from a dispatch node, dispatch of Named::name. That node is linked from the return nodes of the methods the dependent classes run instead, including one a class inherits from a class that isn't a dependent.
• There is one dispatch node per called class and method, with no location, shared by all the virtual calls of that method on that class. The first such call links it to the dependents' return nodes. Each call then takes it through a node of its own at the call, one more parent of its result. So the number of paths doesn't grow with call sites × subclasses, and what is added to the result of one call (a plugin linking a callback's result to the nodes of the call's type, for example) doesn't reach the others.
• If no dependent class runs a method of its own (they all inherit the called one), there is no dispatch node, and the call's result is as on master.
• The method's own return node can't be used for this. Calls on a class that inherits the method go through it too, and those calls can't run a sibling's override.
• The return node of a method whose calls are specialized per call site (@psalm-taint-specialize, or speculatively when the method can't be overridden) can't take a path of its own. Resolution only leaves it through its specializations when it has no outgoing edge. So the dispatch is linked as one more specialization of it, a call site keyed by the method's location. A flow entering that method through a specialized call still leaves it only through that call.
• parent::, self:: and A:: calls run the method they name and nothing else, so they don't take the dispatch node. A call on a final method, a private method or a constructor doesn't either.

Tests

• taintTheResultOfACallOnAnInterfaceWithTheReturnOfAnImplementation, taintTheResultOfACallOnAParentClassWithTheReturnOfAnOverride, taintTheResultOfACallOfAnAbstractMethodWithTheReturnOfAnImplementation and taintTheResultOfAStaticCallOnStaticWithTheReturnOfAnOverride fail without this change.
• taintTheResultOfACallOnAnImplementationAlsoCalledOnItsInterface passes on master. It guards the specialized case: linking the dispatch straight from the implementation's return node made the call on the implementation lose its taint.
• dontTaintTheResultOfACallOnAClassInheritingTheMethodWithTheReturnOfASiblingOverride, dontTaintTheResultOfAParentCallWithTheReturnOfASiblingOverride and dontTaintTheResultOfACallOnAnInterfaceWithWhatASpecializedCallOfTheImplementationReturns check that the dispatch doesn't reach calls that can't run the override, and that specialized calls don't leak through it.

This is independent of #12100, which only dispatches arguments. The two PRs add the same TaintFlowGraph::hasNode().

The real-projects PHPUnit run now reports print $result->output() in Application::run() (an exception message returned by MigrateConfigurationCommand::execute() through Command::execute()): baselined in psalm/endtoend-test-phpunit#13.

[cursor]

Stale Bugbot comment from a previous run.

[cursor]

Dynamic static calls misuse dispatch

Medium Severity

Return dispatch is wired for static calls whose class operand is not a PhpParser\Node\Name, not only for static::. PHP resolves $class::method() and $this::method() to that exact class, without late static binding, so sibling override returns should not feed the call result through the shared dispatch node.

 

^{Reviewed by Cursor Bugbot for commit 8bf85e7. Configure here.}

[danog]

bugbot review

[cursor]

✅ Bugbot reviewed your changes and found no new issues!

1 issue from previous review remains unresolved.

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit 3bb338f. Configure here.}