BIO_flush Segmentation Fault Issue

[Jay Foster]

Thanks.  I wrote some simple tests to exercise this and it worked 
correctly.  I was just not seeing how.
Jay

On 10/3/2022 11:26 PM, Tomas Mraz wrote:
> Your analysis is correct. However the library is still correct in
> regards to refcounting even for an SSL BIO in the chain. The reason is
> that the decrement of refcount of the BIOs underlying the SSL BIO is
> handled through the actual freeing of the SSL BIO. If the refcount for
> the SSL BIO in the chain freed by BIO_free_all() drops to zero, it
> will, in ssl_free() call in bio_ssl.c, free the underlying SSL *
> object. And if the refcount of that SSL object drops to zero it will
> call BIO_free_all() on the underlying wbio and rbio. Which means also
> the chained BIOs underlying the SSL BIO will have their refcount
> dropped and they will be properly freed.
>
> Tomas Mraz, OpenSSL
>
> On Mon, 2022-10-03 at 09:35 -0700, Jay Foster wrote:
>> Your response makes sense.  I am a bit puzzled by the BIO reference
>> counting.  For example
>>
>>       BIO_new() (or BIO_new_socket() which calls BIO_new()) produces a
>> BIO with a reference count of 1.
>>       BIO_free() drops 1 reference and if the reference count is 0,
>> frees
>> the BIO.
>>
>>       BIO_push() connects the BIO to the BIO chain.  No references are
>> directly changed.  However, the BIO_CTRL_PUSH command is sent to the
>> BIO.  For the SSL BIO, this results in a call to SSL_set_bio() and
>> adding 1 to the next_bio reference (socket BIO).
>>       BIO_pop() calls BIO_CTRL_POP and disconnects the BIO from the
>> BIO
>> chain.  For the SSL BIO, the BIO_CTRL_POP subtracts one from the
>> next_bio reference (undoing BIO_push()).
>>
>>       The problem case is calling BIO_free_all() on the BIO chain.
>> BIO_free_all() does not use BIO_pop().  It simply detaches each BIO
>> from
>> the chain, and calls BIO_free() for it.  If the next_bio reference
>> count
>> is > 1 (which it will be for the SSL BIO next_bio), it stops
>> unlinking
>> and freeing BIOs.  This seems asymmetrical with respect to the BIO
>> reference counting, and leaks the next_bio (socket BIO in this case).
>>
>> What am I missing here?
>> Jay
>>
>>
>> On 9/29/2022 11:50 PM, Tomas Mraz wrote:
>>> The SSL BIO should have the rbio from the SSL object as the next
>>> BIO.
>>> If you create the SSL BIO and then BIO_push() the TCP socket BIO
>>> into
>>> the SSL BIO, it will work correctly.
>>>
>>> Otherwise, you can just fix the next BIO of the SSL BIO by using
>>>
>>> BIO_up_ref(socketbio);
>>> BIO_set_next(sslbio, socketbio);
>>>
>>> The SSL BIO should always have a next BIO if properly initialized.
>>>
>>> Tomas Mraz, OpenSSL
>>>
>>> On Thu, 2022-09-29 at 13:02 -0700, Jay Foster wrote:
>>>> I have an application that constructs a chain of BIOs.  Sometimes
>>>> this
>>>> chain also includes an SSL BIO.  Years ago, I ran into a problem
>>>> that
>>>> caused BIO_flush() to segfault on the SSL BIO.  This turned out
>>>> to
>>>> happen because the SSL BIO is added using SSL_set_bio() instead
>>>> of
>>>> BIO_push().  SSL_set_bio() results in the SSL BIO always having a
>>>> NULL
>>>> bio_next value, so BIO_flush then crashes dereferencing this NULL
>>>> pointer when it calls BIO_copy_next_retry() on the SSL BIO (see
>>>> BIO_CTRL_FLUSH in ssl/bio_ssl.c).
>>>>
>>>> This was reported as ticket 2615 years ago.
>>>>
>>>> My question is, how could calling BIO_flush() on a BIO chain with
>>>> an
>>>> SSL
>>>> BIO ever work?  Is there a way to add the SSL BIO using
>>>> BIO_push()
>>>> instead of SSL_set_bio()?
>>>>
>>>> Jay
>>>>