search

vimeo / psalm

A static analysis tool for finding errors in PHP applications
https://psalm.dev
MIT License
5.55k stars 660 forks source link

First-class callable on `strtolower(...)` causes --taint-analyzis to throw Undefined property: PhpParser\Node\VariadicPlaceholder::$value #8853

Open etremblay opened 1 year ago

etremblay commented 1 year ago

A simple array_map(strtolower(...), array) cause the --taint-analysis to throw an exception.

https://psalm.dev/r/4180f36bb1

The regular code scanning pass.

It does not crash with user function or class method like : 

function asdf(string $string): string {
    return strtolower($string);
}
array_map(asdf(...), array)
./vendor/bin/psalm --taint-analysis --report=psalm-taint-analysis.sarif -- a.php
Target PHP version: 8.1 (set by config file) Extensions enabled: dom, pdo, simplexml (unsupported extensions: json, intl, fileinfo, tidy, pcntl, posix, gearman, readline, xsl)
Scanning files...
Analyzing files..

Uncaught RuntimeException: PHP Error: Undefined property: PhpParser\Node\VariadicPlaceholder::$value in /directory/vendor/vimeo/psalm/src/Psalm/Internal/Analyzer/Statements/Expression/Call/FunctionCallReturnTypeFetcher.php:750 for command with CLI args "./vendor/bin/psalm --taint-analysis --report=psalm-taint-analysis.sarif -- a.php" in /directory/vendor/vimeo/psalm/src/Psalm/Internal/ErrorHandler.php:77
Stack trace:
#0 /directory/vendor/vimeo/psalm/src/Psalm/Internal/Analyzer/Statements/Expression/Call/FunctionCallReturnTypeFetcher.php(750): Psalm\Internal\ErrorHandler::Psalm\Internal\{closure}()
#1 /directory/vendor/vimeo/psalm/src/Psalm/Internal/Analyzer/Statements/Expression/Call/FunctionCallReturnTypeFetcher.php(687): Psalm\Internal\Analyzer\Statements\Expression\Call\FunctionCallReturnTypeFetcher::taintUsingFlows()
#2 /directory/vendor/vimeo/psalm/src/Psalm/Internal/Analyzer/Statements/Expression/Call/FunctionCallReturnTypeFetcher.php(266): Psalm\Internal\Analyzer\Statements\Expression\Call\FunctionCallReturnTypeFetcher::taintReturnType()
#3 /directory/vendor/vimeo/psalm/src/Psalm/Internal/Analyzer/Statements/Expression/Call/FunctionCallAnalyzer.php(242): Psalm\Internal\Analyzer\Statements\Expression\Call\FunctionCallReturnTypeFetcher::fetch()
#4 /directory/vendor/vimeo/psalm/src/Psalm/Internal/Analyzer/Statements/ExpressionAnalyzer.php(296): Psalm\Internal\Analyzer\Statements\Expression\Call\FunctionCallAnalyzer::analyze()
#5 /directory/vendor/vimeo/psalm/src/Psalm/Internal/Analyzer/Statements/ExpressionAnalyzer.php(86): Psalm\Internal\Analyzer\Statements\ExpressionAnalyzer::handleExpression()
#6 /directory/vendor/vimeo/psalm/src/Psalm/Internal/Analyzer/Statements/Expression/Call/ArgumentsAnalyzer.php(244): Psalm\Internal\Analyzer\Statements\ExpressionAnalyzer::analyze()
#7 /directory/vendor/vimeo/psalm/src/Psalm/Internal/Analyzer/Statements/Expression/Call/FunctionCallAnalyzer.php(180): Psalm\Internal\Analyzer\Statements\Expression\Call\ArgumentsAnalyzer::analyze()
#8 /directory/vendor/vimeo/psalm/src/Psalm/Internal/Analyzer/Statements/ExpressionAnalyzer.php(296): Psalm\Internal\Analyzer\Statements\Expression\Call\FunctionCallAnalyzer::analyze()
#9 /directory/vendor/vimeo/psalm/src/Psalm/Internal/Analyzer/Statements/ExpressionAnalyzer.php(86): Psalm\Internal\Analyzer\Statements\ExpressionAnalyzer::handleExpression()
#10 /directory/vendor/vimeo/psalm/src/Psalm/Internal/Analyzer/Statements/Expression/AssignmentAnalyzer.php(243): Psalm\Internal\Analyzer\Statements\ExpressionAnalyzer::analyze()
#11 /directory/vendor/vimeo/psalm/src/Psalm/Internal/Analyzer/Statements/ExpressionAnalyzer.php(546): Psalm\Internal\Analyzer\Statements\Expression\AssignmentAnalyzer::analyze()
#12 /directory/vendor/vimeo/psalm/src/Psalm/Internal/Analyzer/Statements/ExpressionAnalyzer.php(178): Psalm\Internal\Analyzer\Statements\ExpressionAnalyzer::analyzeAssignment()
#13 /directory/vendor/vimeo/psalm/src/Psalm/Internal/Analyzer/Statements/ExpressionAnalyzer.php(86): Psalm\Internal\Analyzer\Statements\ExpressionAnalyzer::handleExpression()
#14 /directory/vendor/vimeo/psalm/src/Psalm/Internal/Analyzer/StatementsAnalyzer.php(586): Psalm\Internal\Analyzer\Statements\ExpressionAnalyzer::analyze()
#15 /directory/vendor/vimeo/psalm/src/Psalm/Internal/Analyzer/StatementsAnalyzer.php(212): Psalm\Internal\Analyzer\StatementsAnalyzer::analyzeStatement()
#16 /directory/vendor/vimeo/psalm/src/Psalm/Internal/Analyzer/FileAnalyzer.php(205): Psalm\Internal\Analyzer\StatementsAnalyzer->analyze()
#17 /directory/vendor/vimeo/psalm/src/Psalm/Internal/Codebase/Analyzer.php(1600): Psalm\Internal\Analyzer\FileAnalyzer->analyze()
#18 /directory/vendor/vimeo/psalm/src/Psalm/Internal/Codebase/Analyzer.php(539): Psalm\Internal\Codebase\Analyzer->analysisWorker()
#19 /directory/vendor/vimeo/psalm/src/Psalm/Internal/Codebase/Analyzer.php(290): Psalm\Internal\Codebase\Analyzer->doAnalysis()
#20 /directory/vendor/vimeo/psalm/src/Psalm/Internal/Analyzer/ProjectAnalyzer.php(1227): Psalm\Internal\Codebase\Analyzer->analyzeFiles()
#21 /directory/vendor/vimeo/psalm/src/Psalm/Internal/Cli/Psalm.php(382): Psalm\Internal\Analyzer\ProjectAnalyzer->checkPaths()
#22 /directory/vendor/vimeo/psalm/psalm(9): Psalm\Internal\Cli\Psalm::run()
#23 /directory/vendor/bin/psalm(120): include('...')
#24 {main}
(Psalm 5.1.0@4defa177c89397c5e14737a80fe4896584130674 crashed due to an uncaught Throwable)
psalm-github-bot[bot] commented 1 year ago

I found these snippets:

https://psalm.dev/r/4180f36bb1 ```php
weirdan commented 1 year ago

https://psalm.dev/r/8bc8c90173

psalm-github-bot[bot] commented 1 year ago

I found these snippets:

https://psalm.dev/r/8bc8c90173 ```php
nicwortel commented 2 days ago

I ran into the same issue, and only when --taint-analysis is enabled as @etremblay reported.

The method FunctionCallReturnTypeFetcher::taintUsingFlows assumes that the $args parameter contains an array of PhpParser\Node\Arg instances and calls its value property. Instead, it contains a PhpParser\Node\VariadicPlaceholder instance.

https://github.com/vimeo/psalm/blob/0f796f1c1f1dce7f9096b0086549744ee4fc2271/src/Psalm/Internal/Analyzer/Statements/Expression/Call/FunctionCallReturnTypeFetcher.php#L695-L700

https://github.com/vimeo/psalm/blob/0f796f1c1f1dce7f9096b0086549744ee4fc2271/src/Psalm/Internal/Analyzer/Statements/Expression/Call/FunctionCallReturnTypeFetcher.php#L726-L730

In the lines above, there is already a check $current_arg_is_variadic, but this does not seem to prevent calling the PhpParser\Node\Arg property:

https://github.com/vimeo/psalm/blob/0f796f1c1f1dce7f9096b0086549744ee4fc2271/src/Psalm/Internal/Analyzer/Statements/Expression/Call/FunctionCallReturnTypeFetcher.php#L716-L724

I'm assuming the solution would be to change the type of $args to array<PhpParser\Node\Arg|PhpParser\Node\VariadicPlaceholder> and if $args[$arg_index] is an instance of PhpParser\Node\VariadicPlaceholder, instantiate CodeLocation in a different way.

But I'm new to the codebase of Psalm, and I'm not really sure what is the best way to solve this. If someone can point me in the right direction I'd be happy to contribute a pull request.