squizlabs / PHP_CodeSniffer

PHP_CodeSniffer tokenizes PHP files and detects violations of a defined set of coding standards.
BSD 3-Clause "New" or "Revised" License
10.67k stars 1.48k forks source link

phpcs could hangs on invalid php code #580

Closed sormy closed 9 years ago

sormy commented 9 years ago

In some conditions phpcs could hang and eat 100% cpu and never exit.

General topic is here: https://github.com/SublimeLinter/SublimeLinter-php/issues/12

Environment:

Killing code:

<?php

class PhpCsHang
{
    private function magicMethod()
    {
        $this->service1->doIt(
            $this->service2-"patient_last_name"),
            $this->service2-"patient_first_name"),
            $this->service2-"patient_middle_name")
        );
    }
}

warnings:

PHP Notice:  Undefined index:  in /usr/local/Cellar/php-code-sniffer/2.3.0/CodeSniffer/File.php on line 3411
PHP Stack trace:
PHP   1. {main}() /usr/local/Cellar/php-code-sniffer/2.3.0/scripts/phpcs:0
PHP   2. PHP_CodeSniffer_CLI->runphpcs() /usr/local/Cellar/php-code-sniffer/2.3.0/scripts/phpcs:25
PHP   3. PHP_CodeSniffer_CLI->process() /usr/local/Cellar/php-code-sniffer/2.3.0/CodeSniffer/CLI.php:95
PHP   4. PHP_CodeSniffer->processFiles() /usr/local/Cellar/php-code-sniffer/2.3.0/CodeSniffer/CLI.php:867
PHP   5. PHP_CodeSniffer->processFile() /usr/local/Cellar/php-code-sniffer/2.3.0/CodeSniffer.php:619
PHP   6. PHP_CodeSniffer->_processFile() /usr/local/Cellar/php-code-sniffer/2.3.0/CodeSniffer.php:1679
PHP   7. PHP_CodeSniffer_File->start() /usr/local/Cellar/php-code-sniffer/2.3.0/CodeSniffer.php:1801
PHP   8. Generic_Sniffs_WhiteSpace_ScopeIndentSniff->process() /usr/local/Cellar/php-code-sniffer/2.3.0/CodeSniffer/File.php:567
PHP   9. PHP_CodeSniffer_File->findFirstOnLine() /usr/local/Cellar/php-code-sniffer/2.3.0/CodeSniffer/Standards/Generic/Sniffs/WhiteSpace/ScopeIndentSniff.php:219

and

PHP Notice:  Undefined index:  in /usr/local/Cellar/php-code-sniffer/2.3.0/CodeSniffer/File.php on line 3423
PHP Stack trace:
PHP   1. {main}() /usr/local/Cellar/php-code-sniffer/2.3.0/scripts/phpcs:0
PHP   2. PHP_CodeSniffer_CLI->runphpcs() /usr/local/Cellar/php-code-sniffer/2.3.0/scripts/phpcs:25
PHP   3. PHP_CodeSniffer_CLI->process() /usr/local/Cellar/php-code-sniffer/2.3.0/CodeSniffer/CLI.php:95
PHP   4. PHP_CodeSniffer->processFiles() /usr/local/Cellar/php-code-sniffer/2.3.0/CodeSniffer/CLI.php:867
PHP   5. PHP_CodeSniffer->processFile() /usr/local/Cellar/php-code-sniffer/2.3.0/CodeSniffer.php:619
PHP   6. PHP_CodeSniffer->_processFile() /usr/local/Cellar/php-code-sniffer/2.3.0/CodeSniffer.php:1679
PHP   7. PHP_CodeSniffer_File->start() /usr/local/Cellar/php-code-sniffer/2.3.0/CodeSniffer.php:1801
PHP   8. Generic_Sniffs_WhiteSpace_ScopeIndentSniff->process() /usr/local/Cellar/php-code-sniffer/2.3.0/CodeSniffer/File.php:567
PHP   9. PHP_CodeSniffer_File->findFirstOnLine() /usr/local/Cellar/php-code-sniffer/2.3.0/CodeSniffer/Standards/Generic/Sniffs/WhiteSpace/ScopeIndentSniff.php:219

and stupid workaround:

    public function findFirstOnLine($types, $start, $exclude=false, $value=null)
    {
        if (is_array($types) === false) {
            $types = array($types);
        }

        $foundToken = false;

        if ($start === "" || $start === null) {
            throw new \UnexpectedValueException("Died to save your money on electricity bill =)");
        }

        for ($i = $start; $i >= 0; $i--) {
            if ($this->_tokens[$i]['line'] < $this->_tokens[$start]['line']) {
                break;
            }
            ...
        }
        ...
    }
aik099 commented 9 years ago

Does it work in 2.3.2 or master branch?

sormy commented 9 years ago

just upgraded, phpcs v2.3.2 is not affected

aik099 commented 9 years ago

Then I guess it was fixed.

sormy commented 9 years ago

Yes, i think it could be closed.

But one of possible fixes for future bugs is to prevent code style checking if php -l do not see that code is valid. A lot of IDEs running phpcs in background when you type in IDE so sometime code became invalid because it's incomplete and it cause delays in phpcs and hangs in IDE (phpcs v2.3.0)

Konafets commented 9 years ago

But the IDE (like PHPStorm) shows syntax errors as well. Actually PHPCS is running constantly while you typing and most of the time the syntax is invalid. I never had any problems with that.

P.S.: Please close the issue by yourself :-)

sormy commented 9 years ago

Actually phpcs v2.3.0 + sublime 3.x + sublimelinter-php plugin converts my macbook pro into heater =)

sormy commented 9 years ago

Ok, it seems like there are a lot of conditions when phpcs could hangs. The best way is to find all of them and fix. But the quick fix is to prevent code sniffer to work if php -l see that php code is invalid.

this easy sample makes phpcs burn your cpu:

<?php
foreach

If you will type not enough fast in IDE there is a change that background process will get intermediate result with incomplete foreach statement and will hang

PHP_CodeSniffer version 2.3.2 (stable) by Squiz (http://www.squiz.net) PHP 5.6.7 (cli) (built: Apr 24 2015 15:49:52)

PHP Notice:  Undefined index: parenthesis_closer in /Users/sormy/.composer/vendor/squizlabs/php_codesniffer/CodeSniffer/Standards/Squiz/Sniffs/ControlStructures/ForEachLoopDeclarationSniff.php on line 77
PHP Stack trace:
PHP   1. {main}() /Users/sormy/.composer/vendor/squizlabs/php_codesniffer/scripts/phpcs:0
PHP   2. PHP_CodeSniffer_CLI->runphpcs() /Users/sormy/.composer/vendor/squizlabs/php_codesniffer/scripts/phpcs:25
PHP   3. PHP_CodeSniffer_CLI->process() /Users/sormy/.composer/vendor/squizlabs/php_codesniffer/CodeSniffer/CLI.php:95
PHP   4. PHP_CodeSniffer->processFiles() /Users/sormy/.composer/vendor/squizlabs/php_codesniffer/CodeSniffer/CLI.php:867
PHP   5. PHP_CodeSniffer->processFile() /Users/sormy/.composer/vendor/squizlabs/php_codesniffer/CodeSniffer.php:619
PHP   6. PHP_CodeSniffer->_processFile() /Users/sormy/.composer/vendor/squizlabs/php_codesniffer/CodeSniffer.php:1679
PHP   7. PHP_CodeSniffer_File->start() /Users/sormy/.composer/vendor/squizlabs/php_codesniffer/CodeSniffer.php:1801
PHP   8. Squiz_Sniffs_ControlStructures_ForEachLoopDeclarationSniff->process() /Users/sormy/.composer/vendor/squizlabs/php_codesniffer/CodeSniffer/File.php:567
PHP Notice:  Undefined offset: -1 in /Users/sormy/.composer/vendor/squizlabs/php_codesniffer/CodeSniffer/Standards/Squiz/Sniffs/ControlStructures/ForEachLoopDeclarationSniff.php on line 109
PHP Stack trace:
PHP   1. {main}() /Users/sormy/.composer/vendor/squizlabs/php_codesniffer/scripts/phpcs:0
PHP   2. PHP_CodeSniffer_CLI->runphpcs() /Users/sormy/.composer/vendor/squizlabs/php_codesniffer/scripts/phpcs:25
PHP   3. PHP_CodeSniffer_CLI->process() /Users/sormy/.composer/vendor/squizlabs/php_codesniffer/CodeSniffer/CLI.php:95
PHP   4. PHP_CodeSniffer->processFiles() /Users/sormy/.composer/vendor/squizlabs/php_codesniffer/CodeSniffer/CLI.php:867
PHP   5. PHP_CodeSniffer->processFile() /Users/sormy/.composer/vendor/squizlabs/php_codesniffer/CodeSniffer.php:619
PHP   6. PHP_CodeSniffer->_processFile() /Users/sormy/.composer/vendor/squizlabs/php_codesniffer/CodeSniffer.php:1679
PHP   7. PHP_CodeSniffer_File->start() /Users/sormy/.composer/vendor/squizlabs/php_codesniffer/CodeSniffer.php:1801
PHP   8. Squiz_Sniffs_ControlStructures_ForEachLoopDeclarationSniff->process() /Users/sormy/.composer/vendor/squizlabs/php_codesniffer/CodeSniffer/File.php:567
gsherwood commented 9 years ago

You should probably submit a bug to your IDE's creator so that it stops running code checking tools in the background if it finds a parse error.

The tokenizer has enough guard code in it that it shouldn't get hung up on syntax errors, but some of the sniffs are not as good. I'll fix the ones I can find with the invalid control structure syntax, but I'm not at a point where I want to run a syntax check on every file before checking. I shouldn't need to do that because the sniffs should be good enough to deal with errors.

gsherwood commented 9 years ago

I've fixed all the sniffs I could find for the code provided. If you find more, submit more and I'll fix whatever sniffs are breaking.

sormy commented 9 years ago

What do you think about "convert all warnings and notices into exceptions" on phpcs level? It's better to die than to burn cpu =)

gsherwood commented 9 years ago

What do you think about "convert all warnings and notices into exceptions" on phpcs level?

That's an interesting idea. I'll take a look into doing that in PHPCS version 3 (currently in development on the 3.0 branch).

gsherwood commented 9 years ago

Committed the error handler change here: https://github.com/squizlabs/PHP_CodeSniffer/commit/fbda38faf9ae23a271481150bf8db80f89e54e23

sormy commented 9 years ago

Neverending bug. Another sample how to burn cpu with phpcs. Now without any warnings/notices. composer global require "squizlabs/php_codesniffer" "3.0.*@dev" phpcs --report=checkstyle --standard=PSR2 test.php

<?php

class Test
{
    public function burn()
    {
        foreach

        switch ($anything) {
        }
    }
}

BTW, I'm still thinking that php -l <file> is the fastest, easiest and 100% working solution for that problem.

gsherwood commented 9 years ago

Fixed that new issue here: https://github.com/squizlabs/PHP_CodeSniffer/commit/9d4b31143ff1164c9bdc77255e6934ad7516532d