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>
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. */
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) {
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()));
