Re: [libmicrohttpd] another memory leak .. ;-)

[Christian Grothoff]

Dear Piotr,

I don't see how this fixes a memory leak, but it would seem to be incorrect as 
you repeatedly (without select/poll in the middle) calling gnutls_handshake 
may cause blocking IO, which is not acceptable.  The code will already re-try.

Are you trying to bound the number of times we call gnutls_handshake (hence 
the addition of attempted_number) so that the connection will go away 
eventually?  That's a hackish way to solve this as TLS may just take longer 
than you expect to establish the encrypted connection (i.e. TLS handshake 
happens to use a stuttering TCP which generates 128 1-byte messages; as each 
is processed individually, this might cause more than 128 times 
GNUTLS_E_AGAIN).

The correct approach (to avoid what might seem to be a leak to you, but 
technically is not since I don't see any memory truly being leaked) is likely 
for you to add a timeout in your call to MHD_daemon_start (so that connections 
that are "idle" are eventually removed).  But that depends on what the exact 
behavior of your 'leaky' application is, your e-mail does not give me enough 
context here.

So, I may be wrong here (does the SSL connection go idle? what does the leak 
really look like?), but I'd like to see a testcase demonstrating the issue 
first before I hack up anything. That way, I can be sure about what is going 
on.  Right now, the patch just seems to add more problems and I'm not 
convinced it solves any.

Happy hacking

Christian

On Tuesday, August 09, 2011 08:16:21 AM you wrote:
> hullo!
> 
>  hows life back there?
>  there exists a significant memory leak in libmicrohttpd. it occurs when
> a https client connects and triggers errors during tls handshake, by
> sending strange data, etc.
>  i am enclosing a patch that removes the leak, please take a look at it,
> probably the retry should be done on any !gnutls_error_is_fatal(ret)
> however i am not sure if there are any non-fatal errors besides
> "interrupted" and "again". and on fatal error i think we should close the
> connection (every project i know that uses gnutls closes the connection
> when
> fatal error during handshake occurs).
>  i am not sure about the acctual nature of a leak, but it seems that when
> a fatal error occurs, like GNUTLS_E_INTERNAL_ERROR,
> or GNUTLS_E_UNEXPECTED_PACKET, of which i have seen
> a couple, we are not freeing tls context correctly. anyway, please
> consider the patch.
> 
> cheers,
> pg

[original patch attached for the mailinglist]

libmicrohttpd_tls_handshake.patch
Description: Text Data