Simplify SSL BIO buffering logic

The write BIO for handshake messages is bufferred so that we only write
out to the network when we have a complete flight. There was some
complexity in the buffering logic so that we switched buffering on and
off at various points through out the handshake. The only real reason to
do this was historically it complicated the state machine when you wanted
to flush because you had to traverse through the "flush" state (in order
to cope with NBIO). Where we knew up front that there was only going to
be one message in the flight we switched off buffering to avoid that.

In the new state machine there is no longer a need for a flush state so
it is simpler just to have buffering on for the whole handshake. This
also gives us the added benefit that we can simply call flush after every
flight even if it only has one message in it. This means that BIO authors
can implement their own buffering strategies and not have to be aware of
the state of the SSL object (previously they would have to switch off
their own buffering during the handshake because they could not rely on
a flush being received when they really needed to write data out). This
last point addresses GitHub Issue #322.

Reviewed-by: Andy Polyakov <appro@openssl.org>

diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index 9fb6e89b3649d9daa04ca3db9f601e83e09d06a1..83ad9ef107ef046f489c2dcb5896a6fb6f2602ef 100644 (file)
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -3220,34 +3220,27 @@ const COMP_METHOD *SSL_get_current_expansion(SSL *s)
 #endif
 }
 
-int ssl_init_wbio_buffer(SSL *s, int push)
+int ssl_init_wbio_buffer(SSL *s)
 {
     BIO *bbio;
 
     if (s->bbio == NULL) {
         bbio = BIO_new(BIO_f_buffer());
         if (bbio == NULL)
-            return (0);
+            return 0;
         s->bbio = bbio;
+        s->wbio = BIO_push(bbio, s->wbio);
     } else {
         bbio = s->bbio;
-        if (s->bbio == s->wbio)
-            s->wbio = BIO_pop(s->wbio);
+        (void)BIO_reset(bbio);
     }
-    (void)BIO_reset(bbio);
-/*      if (!BIO_set_write_buffer_size(bbio,16*1024)) */
+
     if (!BIO_set_read_buffer_size(bbio, 1)) {
         SSLerr(SSL_F_SSL_INIT_WBIO_BUFFER, ERR_R_BUF_LIB);
-        return (0);
-    }
-    if (push) {
-        if (s->wbio != bbio)
-            s->wbio = BIO_push(bbio, s->wbio);
-    } else {
-        if (s->wbio == bbio)
-            s->wbio = BIO_pop(bbio);
+        return 0;
     }
-    return (1);
+
+    return 1;
 }
 
 void ssl_free_wbio_buffer(SSL *s)

diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h
index 968a2ec6cc90c0257c3af48969b63c7f78f341c2..a1f5774673e5a85e249694bee1b98be103dd14a2 100644 (file)
--- a/ssl/ssl_locl.h
+++ b/ssl/ssl_locl.h
@@ -1787,7 +1787,7 @@ const SSL_METHOD *func_name(void)  \
         }
 
 struct openssl_ssl_test_functions {
-    int (*p_ssl_init_wbio_buffer) (SSL *s, int push);
+    int (*p_ssl_init_wbio_buffer) (SSL *s);
     int (*p_ssl3_setup_buffers) (SSL *s);
 # ifndef OPENSSL_NO_HEARTBEATS
     int (*p_dtls1_process_heartbeat) (SSL *s,
@@ -1963,7 +1963,7 @@ __owur int dtls1_shutdown(SSL *s);
 
 __owur int dtls1_dispatch_alert(SSL *s);
 
-__owur int ssl_init_wbio_buffer(SSL *s, int push);
+__owur int ssl_init_wbio_buffer(SSL *s);
 void ssl_free_wbio_buffer(SSL *s);
 
 __owur int tls1_change_cipher_state(SSL *s, int which);

diff --git a/ssl/statem/statem.c b/ssl/statem/statem.c
index d0ea55f7bf74a8c90f14aa5baa5e933f9c369dbf..20353c305bcdfeb66fd99992c5141b2b3840ca7d 100644 (file)
--- a/ssl/statem/statem.c
+++ b/ssl/statem/statem.c
@@ -320,20 +320,20 @@ static int state_machine(SSL *s, int server)
          */
         s->s3->change_cipher_spec = 0;
 
-        if (!server || st->state != MSG_FLOW_RENEGOTIATE) {
-                /*
-                 * Ok, we now need to push on a buffering BIO ...but not with
-                 * SCTP
-                 */
+
+        /*
+         * Ok, we now need to push on a buffering BIO ...but not with
+         * SCTP
+         */
 #ifndef OPENSSL_NO_SCTP
-                if (!SSL_IS_DTLS(s) || !BIO_dgram_is_sctp(SSL_get_wbio(s)))
+        if (!SSL_IS_DTLS(s) || !BIO_dgram_is_sctp(SSL_get_wbio(s)))
 #endif
-                    if (!ssl_init_wbio_buffer(s, server ? 1 : 0)) {
-                        goto end;
-                    }
+            if (!ssl_init_wbio_buffer(s)) {
+                goto end;
+            }
 
+        if (!server || st->state != MSG_FLOW_RENEGOTIATE)
             ssl3_init_finished_mac(s);
-        }
 
         if (server) {
             if (st->state != MSG_FLOW_RENEGOTIATE) {

diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c
index 7591bb9d2286f693608483d90268c6e565a2adf5..ecbc43b3d09fae4b8957ccb8a6cdd835a06cdfd8 100644 (file)
--- a/ssl/statem/statem_clnt.c
+++ b/ssl/statem/statem_clnt.c
@@ -437,20 +437,9 @@ WORK_STATE ossl_statem_client_post_work(SSL *s, WORK_STATE wst)
 
     switch(st->hand_state) {
     case TLS_ST_CW_CLNT_HELLO:
-        if (SSL_IS_DTLS(s) && s->d1->cookie_len > 0 && statem_flush(s) != 1)
+        if (wst == WORK_MORE_A && statem_flush(s) != 1)
             return WORK_MORE_A;
-#ifndef OPENSSL_NO_SCTP
-        /* Disable buffering for SCTP */
-        if (!SSL_IS_DTLS(s) || !BIO_dgram_is_sctp(SSL_get_wbio(s))) {
-#endif
-            /*
-             * turn on buffering for the next lot of output
-             */
-            if (s->bbio != s->wbio)
-                s->wbio = BIO_push(s->bbio, s->wbio);
-#ifndef OPENSSL_NO_SCTP
-            }
-#endif
+
         if (SSL_IS_DTLS(s)) {
             /* Treat the next message as the first packet */
             s->first_packet = 1;

diff --git a/test/heartbeat_test.c b/test/heartbeat_test.c
index f92510ade8b487b95f81c7238ce5b48ae1e6978c..906736c37e560ac22fb013758d2817ed1ea1e658 100644 (file)
--- a/test/heartbeat_test.c
+++ b/test/heartbeat_test.c
@@ -101,7 +101,7 @@ static HEARTBEAT_TEST_FIXTURE set_up(const char *const test_case_name,
         goto fail;
     }
 
-    if (!ssl_init_wbio_buffer(fixture.s, 1)) {
+    if (!ssl_init_wbio_buffer(fixture.s)) {
         fprintf(stderr, "Failed to set up wbio buffer for test: %s\n",
                 test_case_name);
         setup_ok = 0;