search

adafruit / Adafruit_CircuitPython_Requests

Requests-like interface for web interfacing
MIT License
51 stars 36 forks source link

SSL handshake failures are not caught #67

Closed hierophect closed 3 years ago

hierophect commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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

tannewt commented 3 years ago

@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 commented 3 years ago

Closed by #70