For some reason we only checked for the closed
socket case as failure and returned 0 in that case,
for error cases, we returned 1. Likely the API
had been modified in the early days, but this
return code was left lopsided.
This meant that even when the handshake failed, we
still called readIncomingData or writeOutgoindData,
depending on whether we wanted to read or write,
causing a rare race-condition.
When a client (HTTP request) connects to a server,
it needs to send the request, right after the
SSL handshake. SSL_do_handshake could need data
from the socket to complete the handshake. In such
a case it returns WANT_READ. Unfortunately,
because we always called SSL_read, the missing data
could have arrived between the SSL_do_handshake call
and the SSL_read call (a rather short duration, to
be sure, but an open window all the same).
SSL_read would of course read said data from the
socket and, since it still needs to finish the
handshake, will buffer it. It then returns the very
same error that the SSL_do_handshake returned:
WANT_READ. Of course we will oblige by polling with
POLLIN, which will time out (there is no more data
to come, and the server is waiting for *our* request
and has nothing to send us).
The only way this deadlock could break if
SSL_do_handshake was called (which will consume
the buffered data, return 1 to indicate handshake
has completed). Since we wouldn't call it unless
and until we get POLLIN, per WANT_READ, which won't
happen in this case. And since SSL_read doesn't call
SSL_do_handshake either, the request times out and
that's the end of it.
The fix is to not call SSL_read when the handshake
isn't complete and needs more data, which we do now.
Fortunately, we have very few SSL clients, outside
of unit-tests. Most notably the WOPI client. But
even then it's not a heavily used connection and
might not even be SSL-enabled (for LAN servers).
Change-Id: I04fd3dae151904194f3d7579dbf8c671b2580ffb
Signed-off-by: Ashod Nakashian <ashod.nakashian@collabora.co.uk>
b556381406