On 30/01/17 17:19, Mischa Salle wrote: > Hi Matt, > > thanks for the quick and extensive answer! > > I've tried by replacing all SSL_set_bio(ssl, bio, bio) with a separate > SSL_set0_rbio(ssl, bio) and SSL_set0_wbio(ssl, bio). > I've also removed all BIO_free statements and if I understand you correctly, > I should then *not* need to call BIO_up_ref() manually, or did I > misunderstand?
Well, SSL_set0_rbio() and SSL_set0_wbio() still transfer the ownership of memory - but their semantics are a lot simpler. There is no special-casing for instances where the rbio and wbio are the same. SSL_set0_rbio() *always* transfers ownership of the rbio, and SSL_set0_wbio() *always* transfers ownership of the wbio. If you call SSL_set0_rbio() followed immediately by SSL_set0_wbio() with the *same* BIO for both then that means you are transferring *2* references (once for the rbio and once for the wbio) - so you better make sure you own 2 refs before you start. That will most likely require a BIO_up_ref() call. Matt > > However, I still seem to need to do need it (as also indicated in the > man-page) > otherwise I get a double free and a ref-counter assertion failure from: > https://github.com/openssl/openssl/blob/master/ssl/ssl_lib.c#L977-L978 > The only other thing could be that the code (which I inherited) is calling a > SSL_shutdown() beforehand which does something I have missed...? > > Best wishes, > Mischa > > > On Mon, Jan 30, 2017 at 12:13 PM, Matt Caswell <m...@openssl.org> wrote: >> >> >> >> 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 -- openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
