Make SSL_set_bio's ownership easier to reason about.

SSL_set_bio has some rather complex ownership story because whether rbio/wbio
are both owning depends on whether they are equal. Moreover, whether
SSL_set_bio(ssl, rbio, wbio) frees ssl->rbio depends on whether rbio is the
existing rbio or not. The current logic doesn't even get it right; see tests.

Simplify this. First, rbio and wbio are always owning. All the weird ownership
cases which we're stuck with for compatibility will live in SSL_set_bio. It
will internally BIO_up_ref if necessary and appropriately no-op the left or
right side as needed. It will then call more well-behaved ssl_set_rbio or
ssl_set_wbio functions as necessary.

Change-Id: I6b4b34e23ed01561a8c0aead8bb905363ee413bb
Reviewed-on: https://boringssl-review.googlesource.com/8240
Reviewed-by: Adam Langley <agl@google.com>

include/openssl/ssl.h

diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index 03a4ea5..086aa9d 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -237,7 +237,10 @@
  * In DTLS, if |rbio| is blocking, it must handle
  * |BIO_CTRL_DGRAM_SET_NEXT_TIMEOUT| control requests to set read timeouts.
  *
- * Calling this function on an already-configured |ssl| is deprecated. */
+ * If |rbio| (respectively, |wbio|) is the same as the currently configured
+ * |BIO| for reading (respectivly, writing), that side is left untouched and is
+ * not freed. Using this behavior and calling this function if |ssl| already has
+ * |BIO|s configured is deprecated. */
 OPENSSL_EXPORT void SSL_set_bio(SSL *ssl, BIO *rbio, BIO *wbio);
 
 /* SSL_get_rbio returns the |BIO| that |ssl| reads from. */

ssl/ssl_lib.c

diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index 7f7bf5a..0d95695 100644
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -477,11 +477,8 @@
   ssl_free_wbio_buffer(ssl);
   assert(ssl->bbio == NULL);
 
-  int free_wbio = ssl->wbio != ssl->rbio;
   BIO_free_all(ssl->rbio);
-  if (free_wbio) {
-    BIO_free_all(ssl->wbio);
-  }
+  BIO_free_all(ssl->wbio);
 
   BUF_MEM_free(ssl->init_buf);
 
@@ -523,19 +520,18 @@
   ssl->handshake_func = ssl3_accept;
 }
 
-void SSL_set_bio(SSL *ssl, BIO *rbio, BIO *wbio) {
+static void ssl_set_rbio(SSL *ssl, BIO *rbio) {
+  BIO_free_all(ssl->rbio);
+  ssl->rbio = rbio;
+}
+
+static void ssl_set_wbio(SSL *ssl, BIO *wbio) {
   /* If the output buffering BIO is still in place, remove it. */
   if (ssl->bbio != NULL) {
     ssl->wbio = BIO_pop(ssl->wbio);
   }
 
-  if (ssl->rbio != rbio) {
-    BIO_free_all(ssl->rbio);
-  }
-  if (ssl->wbio != wbio && ssl->rbio != ssl->wbio) {
-    BIO_free_all(ssl->wbio);
-  }
-  ssl->rbio = rbio;
+  BIO_free_all(ssl->wbio);
   ssl->wbio = wbio;
 
   /* Re-attach |bbio| to the new |wbio|. */
@@ -544,6 +540,31 @@
   }
 }
 
+void SSL_set_bio(SSL *ssl, BIO *rbio, BIO *wbio) {
+  /* For historical reasons, this function has many different cases in ownership
+   * handling. */
+
+  /* If the two arguments are equal, one fewer reference is granted than
+   * taken. */
+  if (rbio != NULL && rbio == wbio) {
+    BIO_up_ref(rbio);
+  }
+
+  /* If at most one of rbio or wbio is changed, only adopt one reference. */
+  if (rbio == SSL_get_rbio(ssl)) {
+    ssl_set_wbio(ssl, wbio);
+    return;
+  }
+  if (wbio == SSL_get_wbio(ssl)) {
+    ssl_set_rbio(ssl, rbio);
+    return;
+  }
+
+  /* Otherwise, adopt both references. */
+  ssl_set_rbio(ssl, rbio);
+  ssl_set_wbio(ssl, wbio);
+}
+
 BIO *SSL_get_rbio(const SSL *ssl) { return ssl->rbio; }
 
 BIO *SSL_get_wbio(const SSL *ssl) {

ssl/ssl_test.cc

diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc
index 619b4b2..3e9cd1e 100644
--- a/ssl/ssl_test.cc
+++ b/ssl/ssl_test.cc
@@ -1309,8 +1309,6 @@
     return false;
   }
 
-  // TODO(davidben): This one is broken.
-#if 0
   // Test changing the read FD partway through.
   ssl.reset(SSL_new(ctx.get()));
   if (!ssl ||
@@ -1319,7 +1317,6 @@
       !ExpectFDs(ssl.get(), 2, 1)) {
     return false;
   }
-#endif
 
   // Test changing the write FD partway through.
   ssl.reset(SSL_new(ctx.get()));