php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #79072 If passed via a stream processor, the included file contents get corrupted
Submitted: 2020-01-06 16:52 UTC Modified: 2021-07-26 22:33 UTC
Votes:1
Avg. Score:5.0 ± 0.0
Reproduced:1 of 1 (100.0%)
Same Version:1 (100.0%)
Same OS:1 (100.0%)
From: morozov at tut dot by Assigned: cmb (profile)
Status: Not a bug Package: Streams related
PHP Version: 7.4.1 OS: Linux
Private report: No CVE-ID: None
View Add Comment Developer Edit
Welcome! If you don't have a Git account, you can't do anything here.
You can add a comment by following this link or if you reported this bug, you can edit this bug over here.
(description)
Block user comment
Status: Assign to:
Package:
Bug Type:
Summary:
From: morozov at tut dot by
New email:
PHP Version: OS:

 

 [2020-01-06 16:52 UTC] morozov at tut dot by
Description:
------------
There are some undocumented changes in PHP 7.4 which make the stream filter that worked on PHP 7.3 and earlier not work on PHP 7.4.

Specifically, if an included file contents are passed through a stream filter, the file contents get corrupted (assumingly, truncated) resulting in a parse error.

It is impossible to reproduce the issue using a single file, so there's a repository dedicated to reproducing the issue: https://github.com/janvernieuwe/php-vcr-debug.

Interestingly, if the classes are included in the same file as the code using them, the issue is _not_ reproducible.

Test script:
---------------
Cannot include the files since the bug tracker detects them as spam.

Expected result:
----------------
No error

Actual result:
--------------
PHP Parse error:  syntax error, unexpected end of file in IncludeFile.php on line 7


Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2020-01-06 17:50 UTC] nikic@php.net
-Status: Open +Status: Verified
 [2020-01-06 18:04 UTC] nikic@php.net
-Status: Verified +Status: Feedback
 [2020-01-06 18:04 UTC] nikic@php.net
I'd say the code is incorrect. You are modifying the stream contents, but still report the old file size in your stream_stat() override. You need to adjust the size there to account for the modification made by the stream filter, or else report the stream as un-statable.

PS: Your use of stream filters here seems unnecessary. I would recommend you to simply open a php://temp stream instead of dealing with that.
 [2020-01-06 19:55 UTC] morozov at tut dot by
-Status: Feedback +Status: Open
 [2020-01-06 19:55 UTC] morozov at tut dot by
> You are modifying the stream contents, but still report the old file size in your stream_stat() override.

This is true.

> You need to adjust the size there to account for the modification made by the stream filter

Not sure if that's possible given that the filtering is done by the underlying stream filter. We don't know the resulting file size upfront.

> or else report the stream as un-statable.

How can this be done? Removing either of the stream_stat() and url_stat() methods from the stream wrapper makes PHP unable to include the file:

$ php run.php # with stream_stat removed

Warning: include(): StreamProcessor::stream_stat is not implemented! in run.php on line 13

Call Stack:
    0.0001     472552   1. {main}() run.php:0

$ php run.php # with url_stat removed

Warning: file_exists(): StreamProcessor::url_stat is not implemented! in StreamProcessor.php on line 57

Call Stack:
    0.0001     472552   1. {main}() run.php:0
    0.0004     497232   2. StreamProcessor->stream_open() run.php:13
    0.0004     497232   3. file_exists() StreamProcessor.php:57


Warning: include(IncludeFile.php): failed to open stream: "StreamProcessor::stream_open" call failed in run.php on line 13

Call Stack:
    0.0001     472552   1. {main}() run.php:0


Warning: include(): Failed opening 'IncludeFile.php' for inclusion (include_path='.:') in run.php on line 13

Call Stack:
    0.0001     472552   1. {main}() run.php:0

> Your use of stream filters here seems unnecessary. I would recommend you to simply open a php://temp stream instead of dealing with that.

Not sure I understand the suggestion. The purpose of this code is to register a file:// protocol handler that would change the file contents on the fly w/o having to modify the code that includes or reads those files. How does simply open a php://temp stream solve this problem?
 [2020-01-06 20:02 UTC] nikic@php.net
> How can this be done? Removing either of the stream_stat() and url_stat() methods from the stream wrapper makes PHP unable to include the file:

You can do this by returning false from stream_stat(). You'll likely only want to do that if the stream was opened for include.

> Not sure I understand the suggestion. The purpose of this code is to register a file:// protocol handler that would change the file contents on the fly w/o having to modify the code that includes or reads those files. How does simply open a php://temp stream solve this problem?

It so happens that I've recently been dealing with on-the-fly include transforms as well, and the solution I adopted looks like this: https://github.com/nikic/include-interceptor/blob/8cffd8f75212eed30cbeaaf8ee8749d1b88d5c7f/src/Interceptor.php#L44-L47

That is, the transformed file contents are written to a php://temp stream (php://memory would probably also work) and that is then used as the file resource in the stream wrapper.

Your stream filter solution should also work fine though as long as you return false from stream_stat() (or explicitly adjust the length).
 [2020-01-06 22:30 UTC] morozov at tut dot by
> Your stream filter solution should also work fine though as long as you return false from stream_stat() (or explicitly adjust the length).

Thank you for the recommendation. It worked.

The approach of using a temporary stream seems to be not that easy to integrate into the existing code of php-vcr since its API designed around using `php_user_filter`s while in your example the entire file content is processed at once (although it indeed looks like a more reasonable approach for processing included files).

You suggested to return false from stream_stat(), however the documentation says it should return an array. Is it safe to return false and be sure the return type of `array` won't be strictly enforced in a future PHP version? Returning an empty array seems to work as well.
 [2020-01-17 11:56 UTC] nikic@php.net
> You suggested to return false from stream_stat(), however the documentation says it should return an array. Is it safe to return false and be sure the return type of `array` won't be strictly enforced in a future PHP version? Returning an empty array seems to work as well.

Looking at the code, it treats any non-array return value as the stat failing...
 [2021-07-26 21:31 UTC] cmb@php.net
-Status: Open +Status: Feedback -Assigned To: +Assigned To: cmb
 [2021-07-26 21:31 UTC] cmb@php.net
It seems the original issue has been resolved, hasn't it?

> Looking at the code, it treats any non-array return value as the
> stat failing...

Yeah, but I think we should leave documentation as is, and have to
leave implementation as is until next major at least.
 [2021-07-26 22:17 UTC] morozov at tut dot by
-Status: Feedback +Status: Assigned
 [2021-07-26 22:17 UTC] morozov at tut dot by
> It seems the original issue has been resolved, hasn't it?

Yes, it's been resolved by changing the PHP code. Here's the actual patch: https://github.com/php-vcr/php-vcr/pull/293

> Yeah, but I think we [...] have to leave implementation as is until next major at least.

You mean how it's implemented in 7.4? In this case, I agree.
 [2021-07-26 22:33 UTC] cmb@php.net
-Status: Assigned +Status: Not a bug
 [2021-07-26 22:33 UTC] cmb@php.net
> Here's the actual patch: https://github.com/php-vcr/php-vcr/pull/293

Okay, so this is more like not-a-bug (since the userland code
should never have reported an invalid stat size.

> You mean how it's implemented in 7.4?

Yes (and this is also how its implemented in PHP 8).
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved.
Last updated: Tue Mar 19 10:01:30 2024 UTC