kris/boringssl
1
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

Make SSL_set_bio's ownership easier to reason about.

Browse Source 
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>
This commit is contained in:
David Benjamin 2016-06-11 19:01:56 -04:00 committed by Adam Langley
parent 5c0fb889a1
commit f715c42322
3 changed files with 37 additions and 16 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
include/openssl/ssl.h
View File

@ -237,7 +237,10 @@ OPENSSL_EXPORT int SSL_is_server(SSL *ssl);
* 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. */

45
ssl/ssl_lib.c
View File

@ -477,11 +477,8 @@ void SSL_free(SSL *ssl) {
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 @@ void SSL_set_accept_state(SSL *ssl) {
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) {
}
}
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) {

3
ssl/ssl_test.cc
View File

@ -1309,8 +1309,6 @@ static bool TestSetFD() {
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 @@ static bool TestSetFD() {
!ExpectFDs(ssl.get(), 2, 1)) {
return false;
}
#endif
// Test changing the write FD partway through.
ssl.reset(SSL_new(ctx.get()));