Align with OpenSSL on SSL_set_bio behavior.
SSL_set_bio is a nightmare.
In f715c423224a292d79ba0e3df373c828fbae29f7, we noticed that, among
other problems, SSL_set_bio's actual behavior did not match how
SSL_set_rfd was calling it due to an asymmetry in the rbio/wbio
handling. This resulted in SSL_set_fd/SSL_set_rfd calls to crash. We
decided that SSL_set_rfd's believed semantics were definitive and
changed SSL_set_bio.
Upstream, in 65e2d672548e7c4bcb28f1c5c835362830b1745b, decided that
SSL_set_bio's behavior, asymmetry and all, was definitive and that the
SSL_set_rfd crash was a bug in SSL_set_rfd. Accordingly, they switched
the fd callers to use the side-specific setters, new in 1.1.0.
Align with upstream's behavior and add tests for all of SSL_set_bio's
insanity. Also export the new side-specific setters in anticipation of
wanting to be mostly compatible with OpenSSL 1.1.0.
Change-Id: Iceac9508711f79750a3cc2ded081b2bb2cbf54d8
Reviewed-on: https://boringssl-review.googlesource.com/9064
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index 21699c4..294f95d 100644
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -528,12 +528,12 @@
ssl->handshake_func = ssl3_accept;
}
-static void ssl_set_rbio(SSL *ssl, BIO *rbio) {
+void SSL_set0_rbio(SSL *ssl, BIO *rbio) {
BIO_free_all(ssl->rbio);
ssl->rbio = rbio;
}
-static void ssl_set_wbio(SSL *ssl, BIO *wbio) {
+void SSL_set0_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);
@@ -552,25 +552,34 @@
/* For historical reasons, this function has many different cases in ownership
* handling. */
+ /* If nothing has changed, do nothing */
+ if (rbio == SSL_get_rbio(ssl) && wbio == SSL_get_wbio(ssl)) {
+ return;
+ }
+
/* 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 only the wbio is changed, adopt only one reference. */
if (rbio == SSL_get_rbio(ssl)) {
- ssl_set_wbio(ssl, wbio);
+ SSL_set0_wbio(ssl, wbio);
return;
}
- if (wbio == SSL_get_wbio(ssl)) {
- ssl_set_rbio(ssl, rbio);
+
+ /* 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(ssl) && SSL_get_rbio(ssl) != SSL_get_wbio(ssl)) {
+ SSL_set0_rbio(ssl, rbio);
return;
}
/* Otherwise, adopt both references. */
- ssl_set_rbio(ssl, rbio);
- ssl_set_wbio(ssl, wbio);
+ SSL_set0_rbio(ssl, rbio);
+ SSL_set0_wbio(ssl, wbio);
}
BIO *SSL_get_rbio(const SSL *ssl) { return ssl->rbio; }
@@ -1158,9 +1167,11 @@
return 0;
}
BIO_set_fd(bio, fd, BIO_NOCLOSE);
- SSL_set_bio(ssl, rbio, bio);
+ SSL_set0_wbio(ssl, bio);
} else {
- SSL_set_bio(ssl, rbio, rbio);
+ /* Copy the rbio over to the wbio. */
+ BIO_up_ref(rbio);
+ SSL_set0_wbio(ssl, rbio);
}
return 1;
@@ -1176,9 +1187,11 @@
return 0;
}
BIO_set_fd(bio, fd, BIO_NOCLOSE);
- SSL_set_bio(ssl, bio, wbio);
+ SSL_set0_rbio(ssl, bio);
} else {
- SSL_set_bio(ssl, wbio, wbio);
+ /* Copy the wbio over to the rbio. */
+ BIO_up_ref(wbio);
+ SSL_set0_rbio(ssl, wbio);
}
return 1;
}
diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc
index 5dd689b..86e7942 100644
--- a/ssl/ssl_test.cc
+++ b/ssl/ssl_test.cc
@@ -1278,9 +1278,9 @@
SSL_SESSION *session0 = SSL_get_session(client.get());
ScopedSSL_SESSION session1(SSL_SESSION_dup(session0, 1));
if (!session1) {
- return false;
+ return false;
}
-
+
uint8_t *s0_bytes, *s1_bytes;
size_t s0_len, s1_len;
@@ -1397,6 +1397,63 @@
return true;
}
+static bool TestSetBIO() {
+ ScopedSSL_CTX ctx(SSL_CTX_new(TLS_method()));
+ if (!ctx) {
+ return false;
+ }
+
+ ScopedSSL ssl(SSL_new(ctx.get()));
+ ScopedBIO bio1(BIO_new(BIO_s_mem())), bio2(BIO_new(BIO_s_mem())),
+ bio3(BIO_new(BIO_s_mem()));
+ if (!ssl || !bio1 || !bio2 || !bio3) {
+ return false;
+ }
+
+ // SSL_set_bio takes one reference when the parameters are the same.
+ BIO_up_ref(bio1.get());
+ SSL_set_bio(ssl.get(), bio1.get(), bio1.get());
+
+ // Repeating the call does nothing.
+ SSL_set_bio(ssl.get(), bio1.get(), bio1.get());
+
+ // It takes one reference each when the parameters are different.
+ BIO_up_ref(bio2.get());
+ BIO_up_ref(bio3.get());
+ SSL_set_bio(ssl.get(), bio2.get(), bio3.get());
+
+ // Repeating the call does nothing.
+ SSL_set_bio(ssl.get(), bio2.get(), bio3.get());
+
+ // It takes one reference when changing only wbio.
+ BIO_up_ref(bio1.get());
+ SSL_set_bio(ssl.get(), bio2.get(), bio1.get());
+
+ // It takes one reference when changing only rbio and the two are different.
+ BIO_up_ref(bio3.get());
+ SSL_set_bio(ssl.get(), bio3.get(), bio1.get());
+
+ // If setting wbio to rbio, it takes no additional references.
+ SSL_set_bio(ssl.get(), bio3.get(), bio3.get());
+
+ // From there, wbio may be switched to something else.
+ BIO_up_ref(bio1.get());
+ SSL_set_bio(ssl.get(), bio3.get(), bio1.get());
+
+ // If setting rbio to wbio, it takes no additional references.
+ SSL_set_bio(ssl.get(), bio1.get(), bio1.get());
+
+ // From there, rbio may be switched to something else, but, for historical
+ // reasons, it takes a reference to both parameters.
+ BIO_up_ref(bio1.get());
+ BIO_up_ref(bio2.get());
+ SSL_set_bio(ssl.get(), bio2.get(), bio1.get());
+
+ // ASAN builds will implicitly test that the internal |BIO| reference-counting
+ // is correct.
+ return true;
+}
+
static uint16_t kVersions[] = {
SSL3_VERSION, TLS1_VERSION, TLS1_1_VERSION, TLS1_2_VERSION, TLS1_3_VERSION,
};
@@ -1546,6 +1603,7 @@
!TestOneSidedShutdown() ||
!TestSessionDuplication() ||
!TestSetFD() ||
+ !TestSetBIO() ||
!TestGetPeerCertificate() ||
!TestRetainOnlySHA256OfCerts()) {
ERR_print_errors_fp(stderr);
