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

EOF check on invalid resource #211

Closed
wants to merge 1 commit into from
Closed

EOF check on invalid resource #211

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Sep 4, 2019

During the debugging of TLS support for a MySQL driver, I've encountered a SSL error which made the stream resource close and thus emit an error when we checked whether there's an EOF.

Error on feof was supplied resource is not a valid stream resource due to stream_socket_enable_crypto already failing with the error An existing connection was forcibly closed by the remote host.

This PR therefore only checks EOF if the resource is still valid.

@ghost ghost changed the title Only check EOF if there's no error EOF check on invalid resource Sep 4, 2019
Copy link
Member

@clue clue left a comment

Choose a reason for hiding this comment

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

@CharlotteDunois Thanks for the fix, updated code looks good to me 👍

I wonder if it's reasonable to come up with a test case to reproduce this particular issue?

@ghost
Copy link
Author

ghost commented Sep 14, 2019

@clue I've been thinking about this aswell, but I'm not sure how this can be reliably tested. Quickly testing whether closing the socket from the server is enough was negative.

My other idea would be starting a new process for the server and then killing the server process after getting a successful connect while trying to setup TLS, to get a TCP reset. However I've yet to get to test this and see if that works.

If you have an idea how this can be tested, I'd like to add that as test.

@clue
Copy link
Member

clue commented Sep 20, 2019

@CharlotteDunois Perhaps take a look at #169 which originally introduced this feof() check. It's my understanding that if the resource is closed already, the stream_socket_crypto_enable() call should also report a warning, so perhaps just switching the statements as in your original version would be preferred? In the test suite, we can just call the internal toggleCrypto() method with some invalid arguments, this should already trigger the faulty behavior.

@ghost ghost closed this Dec 5, 2019
This pull request was closed.
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.

1 participant