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_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);