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? 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
