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

Fix error handling in Stream::getContents() #252

Merged
merged 1 commit into from
Nov 13, 2023

Conversation

nicolas-grekas
Copy link
Collaborator

@nicolas-grekas nicolas-grekas commented Nov 10, 2023

stream_get_contents() swallows errors. This creates issues like symfony/symfony#52490, where errors reported by the stream layer aren't propagated to user-land.

This is even in the C source:
https://github.com/php/php-src/blob/311cae03e730c76aed343312319ed8cf1c37ade0/main/streams/streams.c#L1512

@nicolas-grekas
Copy link
Collaborator Author

guzzle/psr7 looks OK BTW:
https://github.com/guzzle/psr7/blame/2.6/src/Utils.php#L404

@nicolas-grekas
Copy link
Collaborator Author

@stof
Copy link

stof commented Nov 10, 2023

@nicolas-grekas is this a case for which we could write a test in php-http/psr7-integration-tests so that all PSR-7 implementations could know whether they handle that case properly by running the shared testsuite ? Or is it not testable this way ?

@nicolas-grekas
Copy link
Collaborator Author

slim/psr7 is affected: https://github.com/slimphp/Slim-Psr7/blob/4299f1650acad6df77fbaeb6f18c59e46af761b9/src/Stream.php#L357 /cc @l0gicgate

@nicolas-grekas
Copy link
Collaborator Author

@nicolas-grekas
Copy link
Collaborator Author

@nicolas-grekas
Copy link
Collaborator Author

@nicolas-grekas
Copy link
Collaborator Author

I added a test case at php-http/psr7-integration-tests#74 and updated the implementation to cover more situations (like guzzle does).

@nicolas-grekas nicolas-grekas force-pushed the fix-get-contents branch 8 times, most recently from 9cf0693 to aa17767 Compare November 11, 2023 18:16
Copy link
Owner

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Lovely. Thank you for the tests and for checking all PRS7 implementation.

Well done!

@Nyholm Nyholm merged commit aa5fc27 into Nyholm:master Nov 13, 2023
11 of 12 checks passed
@nicolas-grekas nicolas-grekas deleted the fix-get-contents branch November 13, 2023 09:31
@vbartusevicius
Copy link

Hi @nicolas-grekas, thanks for fixing such a lurking bug! @Nyholm, if possible, when can we expect a release with the fix?

@Nyholm
Copy link
Owner

Nyholm commented Nov 13, 2023

It was released in 1.8.1.

See https://github.com/Nyholm/psr7/releases/tag/1.8.1

Comment on lines +265 to +274
\set_error_handler(static function ($type, $message) use (&$exception) {
throw $exception = new \RuntimeException('Unable to read stream contents: ' . $message);
});

try {
return \stream_get_contents($this->stream);
} catch (\Throwable $e) {
throw $e === $exception ? $e : new \RuntimeException('Unable to read stream contents: ' . $e->getMessage(), 0, $e);
} finally {
\restore_error_handler();

Choose a reason for hiding this comment

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

I don't understand how setting an error handler fixes this issue when other tools and even Guzzle check for a false return. Nothing about this code seems like it will actually solve the issue, because the return value of stream_get_contents is not being checked.

Copy link
Collaborator Author

@nicolas-grekas nicolas-grekas Nov 13, 2023

Choose a reason for hiding this comment

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

When stream_get_contents returns false, it first always triggers a PHP warning. Since we catch those in the error handler, the function will never really return false.

Choose a reason for hiding this comment

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

Considering that such behavior is not documented, why take that approach over checking the return value is false? Every other fix I've looked at around this issue is doing a $ret === false check...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That would be dead code. Many things are not documented, especially regarding error handling. Returning false always means an error, and PHP has to trigger a warning to tell about the error so this is still solid IMHO. Of course, feel free to add the check on your side if you're not comfortable with the logic.

Choose a reason for hiding this comment

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

That would be dead code.

Are you saying that checking === false does not work, or are you saying that this monkey patching with the error handler identifies the impending return false more quickly?

Copy link

@odan odan Nov 13, 2023

Choose a reason for hiding this comment

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

I have a question to understand this patch a little better.
Why do you declare an "$exception" variable for the Exception although the catch block gets the same Exception instance? Is there some PHP specific logic / behavior behind this (that is not documented)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we remove this check, we would always wrap the exception thrown in the error handler in another exception. This check allows removing this needless wrapping exception.

Copy link

@odan odan Nov 13, 2023

Choose a reason for hiding this comment

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

Ok, thanks. Then my next question is, why not move the set_error_handler into the try-block to handle that (scope) in the same place?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if set_error_handler fails for any reasons, we don't want restore_error_handler to be called

Copy link

Choose a reason for hiding this comment

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

And in PHP, try/catch does not create a scope anyway.

@shadowhand
Copy link

It might also be helpful to have a test case for this in http-factory-tests.

@devanych
Copy link

Hi @nicolas-grekas, thanks 👍

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

Successfully merging this pull request may close these issues.

8 participants