[openssl-dev] SSL_set_bio(ssl, bio, bio) and BIO_up_ref(bio)

[Mischa Salle]

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 <matt at 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