Skip to content

Commit

Permalink
TLS: SSL_peek is not a const operation
Browse files Browse the repository at this point in the history 
Calling SSL_peek can cause bytes to be read from the raw socket which in
turn can upset the select machinery that determines whether there's data
available on the socket.

Since Curl_ossl_check_cxn only tries to determine whether the socket is
alive and doesn't actually need to see the bytes SSL_peek seems like
the wrong function to call.

We're able to occasionally reproduce a connect timeout due to this
bug. What happens is that Curl doesn't know to call SSL_connect again
after the peek happens since data is buffered in the SSL buffer and thus
select won't fire for this socket.

Closes #795
  • Loading branch information
@Andersbakken @bagder
Andersbakken authored and bagder committed May 10, 2016
1 parent f6767f5 commit 856baf5
Showing 1 changed file with 8 additions and 8 deletions.
16 changes: 8 additions & 8 deletions lib/vtls/openssl.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -759,17 +759,17 @@ void Curl_ossl_cleanup(void)
*/
int Curl_ossl_check_cxn(struct connectdata *conn)
{
int rc;
#ifdef MSG_PEEK
char buf;

rc = SSL_peek(conn->ssl[FIRSTSOCKET].handle, (void*)&buf, 1);
if(rc > 0)
return 1; /* connection still in place */

if(rc == 0)
if(recv((RECV_TYPE_ARG1)conn->sock[FIRSTSOCKET], (RECV_TYPE_ARG2)&buf,
(RECV_TYPE_ARG3)1, (RECV_TYPE_ARG4)MSG_PEEK) == 0) {
return 0; /* connection has been closed */

}
else
return 1; /* connection still in place */
#else
return -1; /* connection status unknown */
#endif
}

/* Selects an OpenSSL crypto engine
Expand Down

6 comments on commit 856baf5

@jay
Copy link
Member

@jay jay commented on 856baf5 May 11, 2016

Choose a reason for hiding this comment

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

if recv returns -1 the connection status should be unknown shouldn't it?

@Andersbakken
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well. The most common reason for recv to return -1 would probably be EWOULDBLOCK in which case the socket definitely is open. One could maybe make a case that any errno value for should indicate unknown or even closed.

@jay
Copy link
Member

@jay jay commented on 856baf5 May 11, 2016

Choose a reason for hiding this comment

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

No you're right, exclude einprogress ewouldblock eagain. how about

if(nread == 0)
  return 0
else if(nread == 1)
  return 1
else {
  if(nread == -1) {
    int err = SOCKERRNO;
    if(err == einprogress || err == ewouldblock || err == eagain)
      return 1;
  }
  return -1;
}

@bagder
Copy link
Member

@bagder bagder commented on 856baf5 May 11, 2016

Choose a reason for hiding this comment

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

Let's do that, it seems more complete and should remove any doubts or possible side-effects that will trigger once in a million and only late Friday evenings. =)

@Andersbakken
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks good to me.

Note also that roughly the same code exists in:

curl_socket_t Curl_getconnectinfo(struct SessionHandle _data, struct connectdata *_connp)

which, as far as I can tell is more or less the only caller of Curl_ssl_check_cxn.

There doesn't really seem to be a need to special-case this for ssl anymore IMHO.

@jay
Copy link
Member

@jay jay commented on 856baf5 May 12, 2016

Choose a reason for hiding this comment

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

Yes, it's the only caller at the moment. How it's handled varies from one SSL backend to another. Most don't support the function and just return -1. DarwinSSL it is implemented though. Changes landed in 2968c83 because Friday comes once a week...

Please sign in to comment.