Simplify and rename SSL_set_rbio() and SSL_set_wbio()

SSL_set_rbio() and SSL_set_wbio() are new functions in 1.1.0 and really
should be called SSL_set0_rbio() and SSL_set0_wbio(). The old
implementation was not consistent with what "set0" means though as there
were special cases around what happens if the rbio and wbio are the same.
We were only ever taking one reference on the BIO, and checking everywhere
whether the rbio and wbio are the same so as not to double free.

A better approach is to rename the functions to SSL_set0_rbio() and
SSL_set0_wbio(). If an existing BIO is present it is *always* freed
regardless of whether the rbio and wbio are the same or not. It is
therefore the callers responsibility to ensure that a reference is taken
for *each* usage, i.e. one for the rbio and one for the wbio.

The legacy function SSL_set_bio() takes both the rbio and wbio in one go
and sets them both. We can wrap up the old behaviour in the implementation
of that function, i.e. previously if the rbio and wbio are the same in the
call to this function then the caller only needed to ensure one reference
was passed. This behaviour is retained by internally upping the ref count.

This commit was inspired by BoringSSL commit f715c423224.

RT#4572

Reviewed-by: Rich Salz <rsalz@openssl.org>

diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index 3628cd588190ac17d0ba3dc8f8373ff69fb79c34..2aca2f94d50e8e02525da570e47d6aa47397be59 100644 (file)
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -1326,8 +1326,8 @@ __owur int SSL_set_fd(SSL *s, int fd);
 __owur int SSL_set_rfd(SSL *s, int fd);
 __owur int SSL_set_wfd(SSL *s, int fd);
 # endif
-void SSL_set_rbio(SSL *s, BIO *rbio);
-void SSL_set_wbio(SSL *s, BIO *wbio);
+void SSL_set0_rbio(SSL *s, BIO *rbio);
+void SSL_set0_wbio(SSL *s, BIO *wbio);
 void SSL_set_bio(SSL *s, BIO *rbio, BIO *wbio);
 __owur BIO *SSL_get_rbio(const SSL *s);
 __owur BIO *SSL_get_wbio(const SSL *s);

diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index c49fc5c704b4569130c1e2e0dfeab33fdb0eb3b6..df71f7b0dc5a1dcc61d984c564ce2fc278be332d 100644 (file)
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -979,8 +979,7 @@ void SSL_free(SSL *s)
 
     ssl_free_wbio_buffer(s);
 
-    if (s->wbio != s->rbio)
-        BIO_free_all(s->wbio);
+    BIO_free_all(s->wbio);
     BIO_free_all(s->rbio);
 
     BUF_MEM_free(s->init_buf);
@@ -1043,14 +1042,13 @@ void SSL_free(SSL *s)
     OPENSSL_free(s);
 }
 
-void SSL_set_rbio(SSL *s, BIO *rbio)
+void SSL_set0_rbio(SSL *s, BIO *rbio)
 {
-    if (s->rbio != rbio && s->rbio != s->wbio)
-        BIO_free_all(s->rbio);
+    BIO_free_all(s->rbio);
     s->rbio = rbio;
 }
 
-void SSL_set_wbio(SSL *s, BIO *wbio)
+void SSL_set0_wbio(SSL *s, BIO *wbio)
 {
     /*
      * If the output buffering BIO is still in place, remove it
@@ -1058,8 +1056,7 @@ void SSL_set_wbio(SSL *s, BIO *wbio)
     if (s->bbio != NULL)
         s->wbio = BIO_pop(s->wbio);
 
-    if (s->wbio != wbio && s->rbio != s->wbio)
-        BIO_free_all(s->wbio);
+    BIO_free_all(s->wbio);
     s->wbio = wbio;
 
     /* Re-attach |bbio| to the new |wbio|. */
@@ -1069,8 +1066,42 @@ void SSL_set_wbio(SSL *s, BIO *wbio)
 
 void SSL_set_bio(SSL *s, BIO *rbio, BIO *wbio)
 {
-    SSL_set_wbio(s, wbio);
-    SSL_set_rbio(s, rbio);
+    /*
+     * For historical reasons, this function has many different cases in
+     * ownership handling.
+     */
+
+    /* If nothing has changed, do nothing */
+    if (rbio == SSL_get_rbio(s) && wbio == SSL_get_wbio(s))
+        return;
+
+    /*
+     * If the two arguments are equal then one fewer reference is granted by the
+     * caller than we want to take
+     */
+    if (rbio != NULL && rbio == wbio)
+        BIO_up_ref(rbio);
+
+    /*
+     * If only the wbio is changed only adopt one reference.
+     */
+    if (rbio == SSL_get_rbio(s)) {
+        SSL_set0_wbio(s, wbio);
+        return;
+    }
+    /*
+     * There is an asymmetry here for historical reasons. If only the rbio is
+     * changed AND the rbio and wbio were originally different, then we only
+     * adopt one reference.
+     */
+    if (wbio == SSL_get_wbio(s) && SSL_get_rbio(s) != SSL_get_wbio(s)) {
+        SSL_set0_rbio(s, rbio);
+        return;
+    }
+
+    /* Otherwise, adopt both references. */
+    SSL_set0_rbio(s, rbio);
+    SSL_set0_wbio(s, wbio);
 }
 
 BIO *SSL_get_rbio(const SSL *s)
@@ -1151,9 +1182,10 @@ int SSL_set_wfd(SSL *s, int fd)
             return 0;
         }
         BIO_set_fd(bio, fd, BIO_NOCLOSE);
-        SSL_set_wbio(s, bio);
+        SSL_set0_wbio(s, bio);
     } else {
-        SSL_set_wbio(s, rbio);
+        BIO_up_ref(rbio);
+        SSL_set0_wbio(s, rbio);
     }
     return 1;
 }
@@ -1171,9 +1203,10 @@ int SSL_set_rfd(SSL *s, int fd)
             return 0;
         }
         BIO_set_fd(bio, fd, BIO_NOCLOSE);
-        SSL_set_rbio(s, bio);
+        SSL_set0_rbio(s, bio);
     } else {
-        SSL_set_rbio(s, wbio);
+        BIO_up_ref(wbio);
+        SSL_set0_rbio(s, wbio);
     }
 
     return 1;
@@ -3141,8 +3174,10 @@ SSL *SSL_dup(SSL *s)
         if (s->wbio != s->rbio) {
             if (!BIO_dup_state(s->wbio, (char *)&ret->wbio))
                 goto err;
-        } else
+        } else {
+            BIO_up_ref(ret->rbio);
             ret->wbio = ret->rbio;
+        }
     }
 
     ret->server = s->server;

diff --git a/test/dtlsv1listentest.c b/test/dtlsv1listentest.c
index cc7e5f788923c66d550dc271a82890295d4749e4..91d78e1301a7781df412e81b8d574861369c4739 100644 (file)
--- a/test/dtlsv1listentest.c
+++ b/test/dtlsv1listentest.c
@@ -352,7 +352,7 @@ int main(void)
     outbio = BIO_new(BIO_s_mem());
     if (outbio == NULL)
         goto err;
-    SSL_set_wbio(ssl, outbio);
+    SSL_set0_wbio(ssl, outbio);
 
     success = 1;
     for (i = 0; i < (long)OSSL_NELEM(testpackets) && success; i++) {
@@ -365,7 +365,7 @@ int main(void)
         /* Set Non-blocking IO behaviour */
         BIO_set_mem_eof_return(inbio, -1);
 
-        SSL_set_rbio(ssl, inbio);
+        SSL_set0_rbio(ssl, inbio);
 
         /* Process the incoming packet */
         ret = DTLSv1_listen(ssl, peer);
@@ -404,7 +404,7 @@ int main(void)
         (void)BIO_reset(outbio);
         inbio = NULL;
         /* Frees up inbio */
-        SSL_set_rbio(ssl, NULL);
+        SSL_set0_rbio(ssl, NULL);
     }
 
  err:

diff --git a/test/sslapitest.c b/test/sslapitest.c
index 5fdbe2a03dea82eb78dbf4e9f8ce0880a3768466..5fc552d13ce7458a2f73708caebad345f0f65f45 100644 (file)
--- a/test/sslapitest.c
+++ b/test/sslapitest.c
@@ -503,9 +503,9 @@ static int execute_test_ssl_bio(SSL_BIO_TEST_FIXTURE fix)
             goto end;
         }
         if (fix.change_bio == CHANGE_RBIO)
-            SSL_set_rbio(ssl, membio2);
+            SSL_set0_rbio(ssl, membio2);
         else
-            SSL_set_wbio(ssl, membio2);
+            SSL_set0_wbio(ssl, membio2);
     }
     ssl = NULL;
 

diff --git a/util/libssl.num b/util/libssl.num
index f19ee4c83e14cc880ee3e0c3dd715d487fe4e9d6..1041d791725e68adbcec3b1ebe3d9af4c28b9f2e 100644 (file)
--- a/util/libssl.num
+++ b/util/libssl.num
@@ -156,7 +156,7 @@ SSL_CTX_set_tmp_dh_callback             156 1_1_0   EXIST::FUNCTION:DH
 SSL_CTX_get_default_passwd_cb           157    1_1_0   EXIST::FUNCTION:
 TLSv1_server_method                     158    1_1_0   EXIST::FUNCTION:DEPRECATEDIN_1_1_0,TLS1_METHOD
 DTLS_server_method                      159    1_1_0   EXIST::FUNCTION:
-SSL_set_rbio                            160    1_1_0   EXIST::FUNCTION:
+SSL_set0_rbio                           160    1_1_0   EXIST::FUNCTION:
 SSL_CTX_set_options                     161    1_1_0   EXIST::FUNCTION:
 SSL_set_msg_callback                    162    1_1_0   EXIST::FUNCTION:
 SSL_CONF_CTX_free                       163    1_1_0   EXIST::FUNCTION:
@@ -236,7 +236,7 @@ DTLSv1_server_method                    236 1_1_0   EXIST::FUNCTION:DEPRECATEDIN_1
 SSL_set_fd                              237    1_1_0   EXIST::FUNCTION:SOCK
 SSL_use_certificate                     238    1_1_0   EXIST::FUNCTION:
 DTLSv1_method                           239    1_1_0   EXIST::FUNCTION:DEPRECATEDIN_1_1_0,DTLS1_METHOD
-SSL_set_wbio                            240    1_1_0   EXIST::FUNCTION:
+SSL_set0_wbio                           240    1_1_0   EXIST::FUNCTION:
 SSL_read                                241    1_1_0   EXIST::FUNCTION:
 SSL_CTX_get_options                     242    1_1_0   EXIST::FUNCTION:
 SSL_CTX_set_ssl_version                 243    1_1_0   EXIST::FUNCTION: