Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement stream_eof in FileReadTrapStreamWrapper, disable OpCache on PHP < 7.4, #4881 #501

Merged
merged 2 commits into from Apr 23, 2021

Conversation

rrazor
Copy link
Contributor

@rrazor rrazor commented Apr 22, 2021

Overview

This PR resolves PHPStan issue #4881 which had two parts:

  1. PHP emitted warnings about a missing stream_eof() implementation, and
  2. PHP on versions prior to 7.4 segfaulted with zend_mm_heap corrupted error messages, likely due to an OpCache issue.

We address item 1 by implementing more of the stream wrapper.

We address item 2 by disabling OpCache for PHPStan when the version of PHP is earlier than 7.4.x.

Testing

Thanks to the diligent work of @alfredbez and others in the issue 4881 thread, we have a repository which allows for a Dockerized reproduction of the issue.

Before

$ docker run -it --rm -v $(pwd):/app -w /app spryker/php:7.3 vendor/bin/phpstan
Note: Using configuration file /app/phpstan.neon.dist.
PHP Warning:  include(): PHPStan\Reflection\BetterReflection\SourceLocator\FileReadTrapStreamWrapper::stream_eof is not implemented! Assuming EOF in phar:///app/vendor/phpstan/phpstan/phpstan.phar/vendor/composer/ClassLoader.php on line 478
Warning: include(): PHPStan\Reflection\BetterReflection\SourceLocator\FileReadTrapStreamWrapper::stream_eof is not implemented! Assuming EOF in phar:///app/vendor/phpstan/phpstan/phpstan.phar/vendor/composer/ClassLoader.php on line 478
zend_mm_heap corrupted

After

$ docker run -it --rm -v $(pwd):/app -v $(pwd)/../phpstan-src:/opt/phpstan-src -w /app spryker/php:7.3 bash -c "cp /opt/phpstan-src/tmp/phpstan.phar vendor/phpstan/phpstan/phpstan.phar && vendor/bin/phpstan"
Note: Using configuration file /app/phpstan.neon.dist.
 1/1 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%



 [OK] No errors

e2e tests

It seems that the code triggering the EOF warnings relies on having PHPStan itself in composer.json, I was not sure how to craft a companion PR in the phpstan/phpstan repository that would do this without introducing a circular dependency.

@ondrejmirtes
Copy link
Member

About E2E tests: I'm trying to reproduce it, this should be sufficient, but with no luck: phpstan/phpstan@2d3e0c9

Also I've tried it on PHP 7.3 (phpstan/phpstan@68a0f6c), so let's wait and see if it fails. It should fail, because the compiled PHPStan PHAR is involved in there - that's what's at the ../../phpstan path.

if ($offset < 0) {
return false;
}
$this->seekPosition = $offset;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be possible to set the offset to any positive integer value? An empty file is empty so this seems weird. This note applies to the next case as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delayed response on this; I tried to characterize the behavior of the various file stream functions in PHP with an empty file to inspire my fake implementation.

You'd think it shouldn't be possible to set the cursor beyond the end of the file, and yet...

>>> file_put_contents('test.txt', '')
=> 0
>>> $r = fopen('test.txt', 'rb')
=> stream resource #679
>>> fseek($r, 1234)
=> 0
>>> ftell($r)
=> 1234
>>> fread($r, 1024)
=> ""
>>> ftell($r)
=> 1234
>>> feof($r)
=> true

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thank you for the research, the implementation is fine then 👍

@alfredbez
Copy link

I've added an e2e test that reproduces that problem: phpstan/phpstan#4895

@ondrejmirtes
Copy link
Member

Awesome, thank you both!

@rrazor I'm gonna merge this, we can discuss the seekPosition afterwards and modify it if we agree how.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants