On 30/01/17 10:13, Mischa Salle wrote: > Hi all, > > I noticed a doublefree when calling SSL_set_bio(ssl, bio, bio) followed > by either SSL_set_bio(ssl, NULL, NULL) or SSL_set_io_SSL_free(ssl). > Valgrind shows the double free, and I see the assert in > https://github.com/openssl/openssl/blob/master/crypto/bio/bio_lib.c#L122 > fail. This is all due to the same bio being using for read and write. > I found that in > https://github.com/openssl/openssl/blob/master/ssl/bio_ssl.c#L331-L332 > the ref-count is manually adjusted, which indeed also fixes my > doublefree. However, it seems that in a number of other places where > SSL_set_bio is called with equal rbio and wbio, this is not the case, > e.g. in apps/s_server.c (L2157, L2735, L3099) and also in > https://github.com/openssl/openssl/blob/master/ssl/ssl_lib.c#L1161 itself. > So the question is, when exactly is it necessary to manually adjust the > ref count, and couldn't this be done automatically in e.g. the > SSL_set_bio(ssl, bio, bio) ?
SSL_set_bio() is a curious beast and its memory management semantics are confusing at best. It's behaviour is retained for historical consistency. The man page now recommends using SSL_set0_rbio() and SSL_set0_wbio() in preference because of this. However they only exist in OpenSSL 1.1.0, so if you need to support 1.0.2 then you are stuck with it. The memory management rules are documented on the latest version of the man page, here: https://www.openssl.org/docs/man1.1.0/ssl/SSL_set_bio.html SSL_set_bio() passes ownership of the BIO's to the SSL object. They will get freed when the SSL object gets freed. Once called you should not then attempt to free them yourself directly, *unless* you have called BIO_up_ref(). If the rbio and wbio are different then ownership of both objects is transferred. If the rbio and the wbio are the same object then ownership is still transferred - but only one reference is consumed, i.e. you are not transferring ownership of two references even though you have passed the BIO to the function "twice" (once for the rbio and once for the wbio). You references a few places in the code: https://github.com/openssl/openssl/blob/master/ssl/bio_ssl.c#L331-L332 Here we up ref before passing the same bio in both arguments in a call to SSL_set_bio(). This is processing a BIO_ctrl call with a BIO_CTRL_PUSH operation. This operation is typically only used internally. It's semantics does *not* transfer ownership of its argument to the BIO_CTRL_PUSH code. However, we want to call SSL_set_bio() with it which will transfer an ownership that we don't currently hold! Therefore we need to up ref first. You also mention apps/s_server.c (L2157, L2735, L3099. In this case we just created the BIO and therefore own a reference to it. We then transfer that ownership to the SSL object in the SSL_set_bio() call. You will notice that after that call we never then attempt to free the BIO again...we no longer own it, so we don't need to. It will get freed when we free the SSL object. Finally you mention this code: https://github.com/openssl/openssl/blob/master/ssl/ssl_lib.c#L1161 Again, in this case, we just created the BIO object and therefore own a reference to it. We then transfer that ownership to the SSL object in the SSL_set_bio() call. You will note that, again, we never explicitly free the BIO object we just created. It will get freed when we free the SSL object. I hope that helps, Matt -- openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
