Ah, right, so indeed I did misunderstand. So if I use SSL_set_bio without calling BIO_free myself, the refcount works out automatically and I don't need to up myself. That indeed works, and explains why the s_server and other places don't suffer from this. In that case, I would say that for having rbio == wbio, SSL_set_bio() would be the preferred function... In any case, thanks again for the clarifications!
Best wishes, Mischa On Mon, Jan 30, 2017 at 8:51 PM, Matt Caswell <m...@openssl.org> wrote: > > > 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 -- openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev