Skip to content

EOF check on invalid resource - #211

Closed
ghost wants to merge 1 commit into
masterfrom
unknown repository
Closed

EOF check on invalid resource#211
ghost wants to merge 1 commit into
masterfrom
unknown repository

Conversation

@ghost

@ghost ghost commented Sep 4, 2019 •
edited by ghost
Loading

Copy link
Copy Markdown

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
clue
clue reviewed Sep 14, 2019
View reviewed changes

@clue clue left a comment

Copy link
Copy Markdown
Member

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

ghost commented Sep 14, 2019

Copy link
Copy Markdown
Author

@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

clue commented Sep 20, 2019

Copy link
Copy Markdown
Member

@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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@clue