Re: bk commit into 5.0 tree (msvensson:1.2517) BUG#28812

[Andrei Elkin]

Magnus, hej.

I approved your patch which is a result of pretty clever analysis.
What I gathered on the bug page is that there appeared to be at least two
issues.
As the problem does not look straightforward it'd be enormously
helpful to state its parts in the changeset comments. Don't urge you
redo the comments. Just next time.

cheers,

Andrei

> Below is the list of changes that have just been committed into a local
> 5.0 repository of msvensson. When msvensson does a push these changes will
> be propagated to the main repository and, within 24 hours after the
> push, to the public repository.
> For information on how to access the public repository
> see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html
>
> ChangeSet@stripped, 2007-08-24 16:59:25+02:00, msvensson@pilot.(none) +7 -0
>   Bug#28812 rpl_ssl fails due to assert in extra/yassl/src/socket_wrapper.cpp:117
>    - Remove three assert's from yaSSL


>    - Merge sslaccept and sslconnect.

>    - Atomically "reset" bio to VIO_TYPE_SSL when the SSL connection has
>      suceeded, this avoids hving to revert anything and thus protects
>      against "close_active_vio" in the middle.


>    - Protect against yaSSL::receive returns -1 
>    - Add some variance to the testcase
>
>   extra/yassl/include/openssl/prefix_ssl.h@stripped, 2007-08-24 16:59:23+02:00,
> msvensson@pilot.(none) +1 -0
>     Add 'SSL_get_fd'
>
>   extra/yassl/include/openssl/ssl.h@stripped, 2007-08-24 16:59:23+02:00,
> msvensson@pilot.(none) +1 -0
>     Add 'SSL_get_fd'
>
>   extra/yassl/src/handshake.cpp@stripped, 2007-08-24 16:59:23+02:00,
> msvensson@pilot.(none) +4 -0
>     Return with error if 'receive' returns -1. Since the -1 is returned as uint
>     it was interpreted by buffer.add_size by a very large value and
>     "blow the buffer" by hitting an assert put in place to detect things like this. 
>
>   extra/yassl/src/socket_wrapper.cpp@stripped, 2007-08-24 16:59:23+02:00,
> msvensson@pilot.(none) +0 -4
>     Remove "assert(socket_ != INVALID_SOCKET)", it's ok to call recv, send
>     or shutdown with socket set to INVALID_SOCKET, they will just return 
>     -1 and set errno.
>
>   extra/yassl/src/ssl.cpp@stripped, 2007-08-24 16:59:23+02:00, msvensson@pilot.(none) +5
> -0
>     Add 'SSL_get_fd'
>
>   mysql-test/t/rpl_ssl.test@stripped, 2007-08-24 16:59:23+02:00, msvensson@pilot.(none)
> +16 -1
>     Increase number of loops to start/stop the slave
>     Add some variance by running two selects before stopping the slave
>     Check that number of records in t1 are equal on master and slave
>
>   vio/viossl.c@stripped, 2007-08-24 16:59:23+02:00, msvensson@pilot.(none) +46 -96
>     Rewrite sslconnect and sslaccept to automically "reset" the vio
>     to VIO_TYPE_SSL. Also use the fd from 'SSL_get_fd' to avoid
>     setting vio->sd to -1, that previously occured when "close_active_vio"
>     was called during connect/accept.
>     
>     Merge the two function since they were exactly the same except for one line.
>     
>     Update the DBUG printouts to be generic(i.e use peer instead of client/server).
>
> diff -Nrup a/extra/yassl/include/openssl/prefix_ssl.h
> b/extra/yassl/include/openssl/prefix_ssl.h
> --- a/extra/yassl/include/openssl/prefix_ssl.h	2006-11-29 09:21:33 +01:00
> +++ b/extra/yassl/include/openssl/prefix_ssl.h	2007-08-24 16:59:23 +02:00
> @@ -30,6 +30,7 @@
>  #define SSL_CTX_new yaSSL_CTX_new
>  #define SSL_new yaSSL_new
>  #define SSL_set_fd yaSSL_set_fd
> +#define SSL_get_fd yaSSL_get_fd
>  #define SSL_connect yaSSL_connect
>  #define SSL_write yaSSL_write
>  #define SSL_read yaSSL_read
> diff -Nrup a/extra/yassl/include/openssl/ssl.h b/extra/yassl/include/openssl/ssl.h
> --- a/extra/yassl/include/openssl/ssl.h	2007-07-13 04:06:31 +02:00
> +++ b/extra/yassl/include/openssl/ssl.h	2007-08-24 16:59:23 +02:00
> @@ -201,6 +201,7 @@ typedef int YASSL_SOCKET_T;
>  SSL_CTX* SSL_CTX_new(SSL_METHOD*);
>  SSL* SSL_new(SSL_CTX*);
>  int  SSL_set_fd (SSL*, YASSL_SOCKET_T);
> +YASSL_SOCKET_T SSL_get_fd(const SSL*);
>  int  SSL_connect(SSL*);
>  int  SSL_write(SSL*, const void*, int);
>  int  SSL_read(SSL*, void*, int);
> diff -Nrup a/extra/yassl/src/handshake.cpp b/extra/yassl/src/handshake.cpp
> --- a/extra/yassl/src/handshake.cpp	2007-01-25 19:34:38 +01:00
> +++ b/extra/yassl/src/handshake.cpp	2007-08-24 16:59:23 +02:00
> @@ -719,6 +719,10 @@ int DoProcessReply(SSL& ssl)
>  
>      // add new data
>      uint read  = ssl.useSocket().receive(buffer.get_buffer() + buffSz, ready);
> +    if (read == static_cast<uint>(-1)) {
> +        ssl.SetError(receive_error);
> +        return 0;
> +    }
>      buffer.add_size(read);
>      uint offset = 0;
>      const MessageFactory& mf = ssl.getFactory().getMessage();
> diff -Nrup a/extra/yassl/src/socket_wrapper.cpp b/extra/yassl/src/socket_wrapper.cpp
> --- a/extra/yassl/src/socket_wrapper.cpp	2007-01-25 19:39:15 +01:00
> +++ b/extra/yassl/src/socket_wrapper.cpp	2007-08-24 16:59:23 +02:00
> @@ -114,8 +114,6 @@ uint Socket::send(const byte* buf, unsig
>      const byte* pos = buf;
>      const byte* end = pos + sz;
>  
> -    assert(socket_ != INVALID_SOCKET);
> -
>      while (pos != end) {
>          int sent = ::send(socket_, reinterpret_cast<const char *>(pos),
>                            static_cast<int>(end - pos), flags);
> @@ -132,7 +130,6 @@ uint Socket::send(const byte* buf, unsig
>  
>  uint Socket::receive(byte* buf, unsigned int sz, int flags)
>  {
> -    assert(socket_ != INVALID_SOCKET);
>      wouldBlock_ = false;
>  
>      int recvd = ::recv(socket_, reinterpret_cast<char *>(buf), sz, flags);
> @@ -163,7 +160,6 @@ bool Socket::wait()
>  
>  void Socket::shutDown(int how)
>  {
> -    assert(socket_ != INVALID_SOCKET);
>      shutdown(socket_, how);
>  }
>  
> diff -Nrup a/extra/yassl/src/ssl.cpp b/extra/yassl/src/ssl.cpp
> --- a/extra/yassl/src/ssl.cpp	2007-07-13 04:06:31 +02:00
> +++ b/extra/yassl/src/ssl.cpp	2007-08-24 16:59:23 +02:00
> @@ -238,6 +238,11 @@ int SSL_set_fd(SSL* ssl, YASSL_SOCKET_T 
>      return SSL_SUCCESS;
>  }
>  
> +YASSL_SOCKET_T SSL_get_fd(const SSL* ssl)
> +{
> +    return ssl->getSocket().get_fd();
> +}
> +
>  
>  int SSL_connect(SSL* ssl)
>  {
> diff -Nrup a/mysql-test/t/rpl_ssl.test b/mysql-test/t/rpl_ssl.test
> --- a/mysql-test/t/rpl_ssl.test	2007-06-15 13:09:26 +02:00
> +++ b/mysql-test/t/rpl_ssl.test	2007-08-24 16:59:23 +02:00
> @@ -41,24 +41,39 @@ select * from t1;
>  
>  # Do the same thing a number of times
>  disable_query_log;
> -let $i= 100;
> +disable_result_log;
> +let $i= 1000;
>  while ($i)
>  {
>    start slave;
>    connection master;
>    insert into t1 values (NULL);
> +  select * from t1; # Some variance
>    connection slave;
> +  select * from t1; # Some variance
>    stop slave;
>    dec $i;
>  }
>  start slave;
>  enable_query_log;
> +enable_result_log;
>  connection master;
>  insert into t1 values (NULL);
> +let $master_count= `select count(*) from t1`;
> +
>  sync_slave_with_master;
>  --source include/wait_for_slave_to_start.inc
>  --replace_result $MYSQL_TEST_DIR MYSQL_TEST_DIR $MASTER_MYPORT MASTER_MYPORT
>  --replace_column 1 # 7 # 8 # 9 # 22 # 23 # 33 #
>  query_vertical show slave status;
> +
> +let $slave_count= `select count(*) from t1`;
> +
> +if (`select $slave_count != $master_count`)
> +{
> +  echo master and slave differed in number of rows;
> +  echo master: $master_count;
> +  echo slave: $slave_count;
> +}
>  
>  --echo End of 5.0 tests
> diff -Nrup a/vio/viossl.c b/vio/viossl.c
> --- a/vio/viossl.c	2007-07-13 04:06:31 +02:00
> +++ b/vio/viossl.c	2007-08-24 16:59:23 +02:00
> @@ -172,78 +172,10 @@ void vio_ssl_delete(Vio *vio)
>    vio_delete(vio);
>  }
>  
> -
>  int sslaccept(struct st_VioSSLFd *ptr, Vio *vio, long timeout)
>  {
> -  SSL *ssl;
> -  my_bool unused;
> -  my_bool net_blocking;
> -  enum enum_vio_type old_type;
>    DBUG_ENTER("sslaccept");
> -  DBUG_PRINT("enter", ("sd: %d  ptr: 0x%lx, timeout: %ld",
> -                       vio->sd, (long) ptr, timeout));
> -
> -  old_type= vio->type;
> -  net_blocking= vio_is_blocking(vio);
> -  vio_blocking(vio, 1, &unused);	/* Must be called before reset */
> -  vio_reset(vio, VIO_TYPE_SSL, vio->sd, 0, FALSE);
> -
> -  if (!(ssl= SSL_new(ptr->ssl_context)))
> -  {
> -    DBUG_PRINT("error", ("SSL_new failure"));
> -    report_errors(ssl);
> -    vio_reset(vio, old_type,vio->sd,0,FALSE);
> -    vio_blocking(vio, net_blocking, &unused);
> -    DBUG_RETURN(1);
> -  }
> -  vio->ssl_arg= (void*)ssl;
> -  DBUG_PRINT("info", ("ssl: 0x%lx  timeout: %ld", (long) ssl, timeout));
> -  SSL_clear(ssl);
> -  SSL_SESSION_set_timeout(SSL_get_session(ssl), timeout);
> -  SSL_set_fd(ssl, vio->sd);
> -  if (SSL_accept(ssl) < 1)
> -  {
> -    DBUG_PRINT("error", ("SSL_accept failure"));
> -    report_errors(ssl);
> -    SSL_free(ssl);
> -    vio->ssl_arg= 0;
> -    vio_reset(vio, old_type,vio->sd,0,FALSE);
> -    vio_blocking(vio, net_blocking, &unused);
> -    DBUG_RETURN(1);
> -  }
> -
> -#ifndef DBUG_OFF
> -  {
> -    char buf[1024];
> -    X509 *client_cert;
> -    DBUG_PRINT("info",("cipher_name= '%s'", SSL_get_cipher_name(ssl)));
> -
> -    if ((client_cert= SSL_get_peer_certificate (ssl)))
> -    {
> -      DBUG_PRINT("info",("Client certificate:"));
> -      X509_NAME_oneline (X509_get_subject_name (client_cert),
> -                         buf, sizeof(buf));
> -      DBUG_PRINT("info",("\t subject: %s", buf));
> -
> -      X509_NAME_oneline (X509_get_issuer_name  (client_cert),
> -                         buf, sizeof(buf));
> -      DBUG_PRINT("info",("\t issuer: %s", buf));
> -
> -      X509_free (client_cert);
> -    }
> -    else
> -      DBUG_PRINT("info",("Client does not have certificate."));
> -
> -    if (SSL_get_shared_ciphers(ssl, buf, sizeof(buf)))
> -    {
> -      DBUG_PRINT("info",("shared_ciphers: '%s'", buf));
> -    }
> -    else
> -      DBUG_PRINT("info",("no shared ciphers!"));
> -  }
> -#endif
> -
> -  DBUG_RETURN(0);
> +  DBUG_RETURN(sslconnect(ptr, vio, timeout));
>  }
>  
>  
> @@ -251,57 +183,75 @@ int sslconnect(struct st_VioSSLFd *ptr, 
>  {
>    SSL *ssl;
>    my_bool unused;
> -  my_bool net_blocking;
> -  enum enum_vio_type old_type;
> +  my_bool was_blocking;
>  
>    DBUG_ENTER("sslconnect");
> -  DBUG_PRINT("enter", ("sd: %d  ptr: 0x%lx  ctx: 0x%lx",
> -                       vio->sd, (long) ptr, (long) ptr->ssl_context));
> +  DBUG_PRINT("enter", ("ptr: 0x%lx, sd: %d  ctx: 0x%lx",
> +                       (long) ptr, vio->sd, (long) ptr->ssl_context));
> +
> +  /* Set socket to blocking if not already set */
> +  vio_blocking(vio, 1, &was_blocking);
>  
> -  old_type= vio->type;
> -  net_blocking= vio_is_blocking(vio);
> -  vio_blocking(vio, 1, &unused);	/* Must be called before reset */
> -  vio_reset(vio, VIO_TYPE_SSL, vio->sd, 0, FALSE);
>    if (!(ssl= SSL_new(ptr->ssl_context)))
>    {
>      DBUG_PRINT("error", ("SSL_new failure"));
>      report_errors(ssl);
> -    vio_reset(vio, old_type, vio->sd, 0, FALSE);
> -    vio_blocking(vio, net_blocking, &unused);
> +    vio_blocking(vio, was_blocking, &unused);
>      DBUG_RETURN(1);
>    }
> -  vio->ssl_arg= (void*)ssl;
>    DBUG_PRINT("info", ("ssl: 0x%lx timeout: %ld", (long) ssl, timeout));
>    SSL_clear(ssl);
>    SSL_SESSION_set_timeout(SSL_get_session(ssl), timeout);
>    SSL_set_fd(ssl, vio->sd);
> -  if (SSL_connect(ssl) < 1)
> +
> +  /*
> +    SSL_do_handshake will select between SSL_connect
> +    or SSL_accept depending on server or client side
> +  */
> +  if (SSL_do_handshake(ssl) < 1)
>    {
> -    DBUG_PRINT("error", ("SSL_connect failure"));
> +    DBUG_PRINT("error", ("SSL_do_handshake failure"));
>      report_errors(ssl);
>      SSL_free(ssl);
> -    vio->ssl_arg= 0;
> -    vio_reset(vio, old_type, vio->sd, 0, FALSE);
> -    vio_blocking(vio, net_blocking, &unused);
> +    vio_blocking(vio, was_blocking, &unused);
>      DBUG_RETURN(1);
>    }
> +
> +  /*
> +    Connection suceeded. Install new function handlers,
> +    change type, set sd to the fd used when connecting
> +    and set pointer to the SSL structure
> +  */
> +  vio_reset(vio, VIO_TYPE_SSL, SSL_get_fd(ssl), 0, 0);
> +  vio->ssl_arg= (void*)ssl;
> +
>  #ifndef DBUG_OFF
>    {
> -    X509 *server_cert;
> -    DBUG_PRINT("info",("cipher_name: '%s'" , SSL_get_cipher_name(ssl)));
> +    /* Print some info about the peer */
> +    X509 *cert;
> +    char buf[512];
> +
> +    DBUG_PRINT("info",("SSL connection suceeded"));
> +    DBUG_PRINT("info",("Using cipher: '%s'" , SSL_get_cipher_name(ssl)));
> +
> +    if ((cert= SSL_get_peer_certificate (ssl)))
> +    {
> +      DBUG_PRINT("info",("Peer certificate:"));
> +      X509_NAME_oneline(X509_get_subject_name(cert), buf, sizeof(buf));
> +      DBUG_PRINT("info",("\t subject: '%s'", buf));
> +      X509_NAME_oneline(X509_get_issuer_name(cert), buf, sizeof(buf));
> +      DBUG_PRINT("info",("\t issuer: '%s'", buf));
> +      X509_free(cert);
> +    }
> +    else
> +      DBUG_PRINT("info",("Peer does not have certificate."));
>  
> -    if ((server_cert= SSL_get_peer_certificate (ssl)))
> +    if (SSL_get_shared_ciphers(ssl, buf, sizeof(buf)))
>      {
> -      char buf[256];
> -      DBUG_PRINT("info",("Server certificate:"));
> -      X509_NAME_oneline(X509_get_subject_name(server_cert), buf, sizeof(buf));
> -      DBUG_PRINT("info",("\t subject: %s", buf));
> -      X509_NAME_oneline (X509_get_issuer_name(server_cert), buf, sizeof(buf));
> -      DBUG_PRINT("info",("\t issuer: %s", buf));
> -      X509_free (server_cert);
> +      DBUG_PRINT("info",("shared_ciphers: '%s'", buf));
>      }
>      else
> -      DBUG_PRINT("info",("Server does not have certificate."));
> +      DBUG_PRINT("info",("no shared ciphers!"));
>    }
>  #endif
>  
>
> -- 
> MySQL Code Commits Mailing List
> For list archives: http://lists.mysql.com/commits
> To unsubscribe:    http://lists.mysql.com/commits?unsub=1
>