kisli / vmime

VMime Mail Library
http://www.vmime.org
GNU General Public License v3.0
273 stars 110 forks source link

[vmime source code] Using of OpenSSL API in the VMIME source code #102

Open ghost opened 9 years ago

ghost commented 9 years ago

Hi,

I do not reproach here I present just my way of seeing things and I try to understand your code that works fine.

I'm reading the vmime source source in order to code my own OpenSSL wraper. And I don't understand the programming style with the method "sendRaw(const byte_t* buffer, const size_t count)" in the vmime/net/tls/openssl/TLSSocket_OpenSSL.cpp. this function work with underlying blocking socket and call the select() function in order to see if there are data to read or write (depend the error SSL_ERROR_WANT_READ/SSL_ERROR_WANT_WRITE) and BIO_set_retry_read() for read() the data. normally this way is for non-blocking socket. SendRaw() work with blocking socket, no? otherwise why create sendRawNonBlocking(const byte_t* buffer, const size_t count)? (select() is for multiplexing input/output? no?)

  1. vmime/net/tls/openssl/TLSSocket_OpenSSL.cpp -> sendRaw(const byte_t* buffer, const size_t count)) if(error == SSL_ERROR_WANT_read) { m_wrapped->waitForRead(); ... handleError(rc);
  2. vmime/plateforms/posix/posixSocket.cpp -> waitForRead(const int msecs) return waitForData(/read/ true, /* write*/ false, msecs);
  3. vmime/plateforms/posix/posixSocket.cpp -> waitForData(const bool read, const bool write, const msecs) for(int i = 0; i<= msecs /10 ; ++i) // why subdivise the time out with 10 .... select()
  4. vmime/plateforms/posix/posixSocket.cpp -> handleError(int rc) case SSL_ERROR_WANT_READ: BIO_set_retry_read(SSL_get_rbio(m_ssl)); break;

In the same times I'm reading "Network Security with OpenSSL" By Pravir Chandra, Matt Messier, John Viega and at the page 132 it says about blocking socket : ... "In order to handle a blocking call correctly, we need to retry the SSL I/O operation if we receive SSL_ERROR_WANT_READ or SSL_ERROR_WANT_WRITE . Don't let the names of these errors confuse you. Even though they tell us the SSL connection needs to wait until we can read or write, respectively, we just need to retry whatever operation caused the condition." ... "In many ways, implementing a blocking call is similar to implementing a non-blocking one. It's similar in the sense that we must loop for retries, but it differs in that we don't need to check for I/O availability on the underlying layer, as with poll or select . In the end, we will not show an example of looping to accomplish the blocking call; there is an easier way." ...

In other words using the select() function in the sendRaw() is unnecessary if we trust the way of this book and a basic loop with a flag is suffisant like this: ...

void TLSSocket_OpenSSL::sendRaw(const byte_t* buffer, const size_t count) { int flag = 0; if (!m_ssl) throw exceptions::socket_not_connected_exception();

m_status &= ~(STATUS_WANT_WRITE | STATUS_WANT_READ);

while((size>0) && (flag == 1))
{
            flag = 0;
    int rc = SSL_write(m_ssl, buffer, static_cast <int>(size));

    if (rc <= 0)
    {
        int error = SSL_get_error(m_ssl, rc);

        if (error == SSL_ERROR_WANT_READ)
        {
                            flag = 1;
            continue;
        }
        else if (error == SSL_ERROR_WANT_WRITE)
        {
            flag = 1;
            continue;
        }
    }
    else
    {
        buffer += rc;
        size -= rc;
                    flag = 1;
    }
}

}

thank you in advance for understanding that you can bring me.

;)

vincent-richard commented 9 years ago

Hi!

Actually default implementation of sockets in VMime is non-blocking, for both POSIX and Windows platforms. This is a requirement for handling timeouts, both when connecting and when sending/receiving data.

All vmime::socket functions (connect/send/receive) are blocking for easier client-server protocol implementation (read/write response). There also exist a non-blocking version sendRawNonBlocking which is actually only used internally by socket implementations.

Vincent

ghost commented 9 years ago

thank you for your help.

ghost commented 9 years ago

I understand that you use a socket in non-blocking mode for the connexion but I think that is not the best way for handling a time-out for your socket input/output,because this way increase the complexity of the code. I think it's better to use the socket options like SO_RCVTIMEO and SO_SNDTIMEO which are faster to operating and a code more readable.

"O_RCVTIMEO and SO_SNDTIMEO Specify the receiving or sending timeouts until reporting an error. The argument is a struct timeval. If an input or output function blocks for this period of time, and data has been sent or received, the return value of that function will be the amount of data transferred; if no data has been transferred and the timeout has been reached, then -1 is returned with errno set to EAGAIN or EWOULDBLOCK, or EINPROGRESS (for connect(2)) just as if the socket was specified to be nonblocking. If the timeout is set to zero (the default), then the operation will never timeout. Timeouts only have effect for system calls that perform socket I/O (e.g., read(2), recvmsg(2), send(2), sendmsg(2)); timeouts have no effect for select(2), poll(2), epoll_wait(2), and so on."

http://man7.org/linux/man-pages/man7/socket.7.html https://msdn.microsoft.com/en-us/library/windows/desktop/ms740476%28v=vs.85%29.aspx

What do you think?

UPDATE:

do you think that the non-blocking socket is really necessary in this library? and why?