[PATCH ustream] ustream-openssl: fix bio memory leak

[Eneas U de Queiroz]

Hi Petr

On Wed, Dec 9, 2020 at 6:59 PM Petr Štetiar <ynezz at true.cz> wrote:
>
> Eneas U de Queiroz <cotequeiroz at gmail.com> [2020-12-09 14:39:06]:
>
> Hi,
>
> > So the answer to your question is because you only allocate the table if
> > methods_ustream is NULL, and it will point to the created table then.
>
> I was referencing the missing freeing of allocated resources.
>
> > We could free it in s_ustream_free, but only to have to create it again
> > with the same data the next time ustream_bio_new is called. I wouldn't do
> > it, but if you'd rather, I can add it in a v2.
>
> Is this micro optimization worth it? You're adding global variable in the
> library, you're breaking API layer etc. I'm not supposed to study how is it
> implemented _now_, because it will likely change with the next release (either
> OpenSSL or wolfSSL) and it might be source of regressions. The API boundary is
> given so I'm just trying to use it as designed and as seen in the
> docs/examples/tests etc. And there is always new/free combo.
>
The purpose of BIO_METHOD struct is to hold a table of methods for a
BIO object to use.   In our case, it remains constant for the lifetime
of the process.
So, the maximum usable lifetime of methods_ustream is up to the
lifetime of the program--it does not mean that we can't set a shorter
lifetime.

In an ideal world, we would free the resource when the library is
cleaned up/deinitialized, but we don't have a function for that.
So a possible lifetime we can use is the lifetime of the BIO object
using it. One thing we need to be aware of is use after free.  We pass
the pointer to the BIO_new, and we must be sure that openssl will not
access that memory after we free it.  This would be after we call
BIO_free.  The thing is, we aren't making that call. so we are leaking
that resource as well.  That one can't have the lifetime of the
program, its lifetime is no larger than the underlying SSL connection,
apparently.  So we need to take care of that first.

After tackling BIO_free, my suggestion would be to determine where the
method table variable should go, and where to call BIO_meth_new and
BIO_meth_free.  I would add it to a defined struct
ustream_ssl_ctx--which is now just used with a cast to SSL_CTX--and
would create and free the object in __ustream_ssl_context_new and
__ustream_ssl_context_free, which would give it a possibly larger
lifetime than the ssl_session or the BIO object.

> > As for the WIP, you're perhaps doing too much work.

I was corrected by my own previous point.

> I'm spending time on this mainly because of FS#3465, perhaps mbedTLS has
> similar issues[1]. In the end I would like to have uclient/ustream-ssl CI
> tested (all 3 SSL libs combinations), with static analyzers, various
> sanitizers and Valgrind. So I have to fix all the issues those tools expose.
>
> Maybe it's too much work, but given the constraints (no globals, follow API),
> it's currently simplest working solution, but not fully tested yet.
>
> BTW I'm not discouraging you from v2, I've rejected the v1 patch, because it
> doesn't fix the memory leak as advertised in the subject :-) Thanks!

We should coordinate efforts.  You're the boss, so tell me what you
want me to do, if anything.

Cheers,

Eneas