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

[Mischa Salle]

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 <matt at 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 <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
> --
> openssl-dev mailing list
> To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev