Skip to content

SSL handshake failures are not caught #67

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

Closed
hierophect opened this issue Jan 30, 2021 · 6 comments
Closed

SSL handshake failures are not caught #67

hierophect opened this issue Jan 30, 2021 · 6 comments

Comments

@hierophect
Copy link

In the request method, while attempting to send a request, only _SendFailed exceptions are caught and all others (such as a Failed SSL handshake, which may occur after too much time has elapsed between requests) are ignored. This then leads to recv or recv_into being run on an invalid connection, resulting in an exception and potentially a socket leak.

This was discovered while debugging adafruit/circuitpython#4061, but does not wholly fix the issue as the code will then run into issue #66. May be related to #63.

@hierophect
Copy link
Author

I fixed this for my example program by simply replacing except _sendFailed: with except: but I don't know if there's issues with that?

@tannewt
Copy link
Member

tannewt commented Feb 1, 2021

Ya, don't do except: because you'll catch absolutely everything. You can add a second except for the error type of "Failed SSL handshake" though.

@hierophect
Copy link
Author

@tannewt what's the issue with catching everything here? Is there an exception that's considered acceptable? All the step does is mark that the socket is not ready to receive data and close it, which seems like the right thing to do for any exception.

@hierophect
Copy link
Author

Alternatively I could add in an else: to actually raise exceptions that aren't of the two specified types.

@tannewt
Copy link
Member

tannewt commented Feb 2, 2021

@tannewt what's the issue with catching everything here? Is there an exception that's considered acceptable? All the step does is mark that the socket is not ready to receive data and close it, which seems like the right thing to do for any exception.

Catching everything is bad because you'll catch extra stuff. Reload, keyboard and deep sleep all use exceptions. Those things (or others) may be caught higher and the code will expect the socket to continue. So, always be explicit with catching exceptions.

@anecdata
Copy link
Member

anecdata commented Feb 5, 2021

Closed by #70

@anecdata anecdata closed this as completed Feb 5, 2021
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

No branches or pull requests

3 participants