Considerations

[VictorVolpe]

Hi, first of all I want to thank you for your great work. I'm implementing SSL in an multi threaded event-driven project and I have some considerations about your code:

1. The usage of the flags fl_want_read and fl_want_write is pointless. Every time you get Result <= 0 from the SSL_read() or SSL_write(), you set the aproprieted flag (fl_reading/fl_writing) and in the Error switch if not soft errors (SSL_ERROR_WANT_READ, SSL_ERROR_WANT_WRITE, etc) occur, you break the event loop, so, even if the fl_reading/fl_writing is set, there is no more event to dispatch.
2. The SSL_connect() doesn't return SSL_ERROR_WANT_CONNECT. In this OpenSSL doc we read: SSL_ERROR_WANT_CONNECT, SSL_ERROR_WANT_ACCEPT _The operation did not complete; the same TLS/SSL I/O function should be called again later. The underlying BIO was not connected yet to the peer and the call would block in connect()/accept(). The SSL function should be called again when the connection is established. These messages can only appear with a BIO_s_connect() or BIO_s_accept() BIO, respectively. In order to find out, when the connection has been successfully established, on many platforms select() or poll() for writing on the socket file descriptor can be used._
3. And for the simplication of your SSLBuffer_func(), there is no need to return in every condition, even because the event doesn't set EV_READ and EV_WRITE in same dispatch. Here is the simplified code:

void SSLBuffer_func(evutil_socket_t fd, short event, void *arg)
{
    struct sslbuffer_t *sslbuffer = (struct sslbuffer_t *) arg;
    int res;
    unsigned long error;

    if (sslbuffer->fl_connecting)
    {
        res = SSL_connect(sslbuffer->ssl);

        if (res <= 0)
        {
            error = SSL_get_error(sslbuffer->ssl, res);

            if (error != SSL_ERROR_WANT_READ && error != SSL_ERROR_WANT_WRITE)
            {
                printf("%s:%d: unknown error %lu\n", __FILE__, __LINE__, error);
                print_errors();
            }
        }
        else
        {
            (sslbuffer->eventcb)(sslbuffer, EVT_CONNECTED, sslbuffer->cb_ctx);

            sslbuffer->fl_connecting = 0;
        }
    }
    else if (sslbuffer->fl_writing)
        SSLBufferTryWrite(sslbuffer);
    else if (sslbuffer->fl_reading)
        SSLBufferTryRead(sslbuffer);
    else if (event & EV_WRITE)
        SSLBufferTryWrite(sslbuffer);
    else if (event & EV_READ)
        SSLBufferTryRead(sslbuffer);
}

Thank you for reading.

[vogonsoft]

Thank you for looking so thoroughly at the code! My response:

1. I don't think that fl_want_read and fl_want_write are pointless. Let's look at fl_want_read: suppose we call SSL_write and it returns with error SSL_ERROR_WANT_READ. We have to remember both that we were trying to do a write and the error was SSL_ERROR_WANT_READ. Why? Because in the middle of our write a request for a new handshake can come and OpenSSL will need to do several reads and writes in order to complete the new handshake before it can write the data that we supplied in SSL_write. Since it gave us error code SSL_ERROR_WANT_READ, that means it wants to do a read, so when the next event EV_READ comes, we should again call SSL_write, but if we see EV_WRITE first, we can simply ignore it because OpenSSL has already told us that it needs to read and not to write. From the documentation page you link to: SSL_ERROR_WANT_READ, SSL_ERROR_WANT_WRITE The operation did not complete; the same TLS/SSL I/O function should be called again later. If, by then, the underlying BIO has data available for reading (if the result code is SSL_ERROR_WANT_READ) or allows writing data (SSL_ERROR_WANT_WRITE), then some TLS/SSL protocol progress will take place, i.e. at least part of an TLS/SSL record will be read or written. Note that the retry may again lead to a SSL_ERROR_WANT_READ or SSL_ERROR_WANT_WRITE condition. There is no fixed upper limit for the number of iterations that may be necessary until progress becomes visible at application protocol level.
2. Maybe I misunderstand what you are saying here, but after SSL_connect returns with an error, SSL_get_error will return SSL_ERROR_WANT_CONNECT and we will have to repeat the call SSL_connect on the next read or write event. If I remember correctly, the documentation previously said erroneously that SSL_connect can set the error code to SSL_ERROR_WANT_READ or SSL_ERROR_WANT_WRITE and I was surprised when I discovered by running my code and debugging through it step by step that SSL_connect in fact sets the error code to SS_ERROR_WANT_CONNECT. Looks like they have corrected the doc in the meantime.
3. That is indeed a simplification, but please don't send a PR for it before we have made sure that the code is correct.

[VictorVolpe]

Thank you for reply.

1. Sure, but your code behaves the same way with any of these two conditions: if ( sslbuffer->fl_writing && ( sslbuffer->fl_want_read || sslbuffer->fl_want_write ) ) or if (sslbuffer->fl_writing). Why? Because you set the fl_writing in both cases of fl_want_read or fl_want_write. There is no condition in your code for sslbuffer->fl_writing == 0 and fl_want_read == 1 or fl_want_write == 1.
2. In my tests and in OpenSSL documentation the SSL_connect() only returns (in case of error, of course) SSL_ERROR_WANT_READ or SSL_ERROR_WANT_WRITE. The SSL_ERROR_WANT_CONNECT is exclusive of BIO_s_connect(). Hope I have been clear this time...
3. I'm testing at localhost, with small packets (<1KB). When I finish the code, I'll try to exchange some heavy files to formalize that.

[vogonsoft]

1. You are right. I described it right, but that is not what the code was doing. Please take a look at the latest version that I just pushed to GitHub. I think the code now does what it should. Here is one part that deals with writing:

   if (sslbuffer->fl_writing) { if ( (sslbuffer->fl_want_read && (event & EV_READ)) || (sslbuffer->fl_want_write && (event & EV_WRITE))) { SSLBufferTryWrite(sslbuffer); } return; }

Reading is analogous.

1. Now I understand what you are saying. But that is not what I am seeing. Since SSL_connect is on an asynchronous socket, it returns immediately and return value is 0. Then a call to SSL_get_error returns 7, which is SSL_ERROR_WANT_CONNECT. Yes, I know that the documentation says otherwise, but this is what I am seeing. Could it be that we are using different versions of OpenSSL? Currently the one I am trying this on locally is OpenSSL 1.0.1e 11 Feb 2013.

[VictorVolpe]

1. Yeah! Now the usage of fl_want_read and fl_want_write makes sense.
2. I'm using OpenSSL 1.0.2f 28 Jan 2016, both in client (Windows 10) and server (FreeBSD 10.2), with asynchronous socket. At every first call of SSL_accept() (server) I receive SSL_ERROR_WANT_READ and in SSL_connect() (client) sometimes I receive SSL_ERROR_WANT_READ and sometimes SSL_ERROR_WANT_WRITE...

Thank you very much for your attention.

[vogonsoft]

Great, then I guess for 2. the safest solution would be for the code to expect any of SSL_ERROR_WANT_READ, SSL_ERROR_WANT_WRITE and SSL_ERROR_WANT_CONNECT. Right now it is midnight here in Toronto and I will have to leave it for tomorrow.

[vogonsoft]

Done. Now the code expects that SSL_connect can genarate any of the three error codes. Victor, please take a look and let me know if you see anything wrong. If all is OK I can close this issue.

Oh, and thank you for your comments, questions and answers!

[VictorVolpe]

Nice! I'm currently working at my project. If I find any issue related to your code, I report directly to you.

Appreciated the talk. Thanks.

[vogonsoft]

Thanks and good luck with your project! I am now closing this issue. If you find any problems, please open a new issue.