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

[Piotr Grzybowski]

hello all.

 this 128 was just an example, i am using larger values.
 i cant produce the testcase since the problem is triggered by
a tool i do not acctually have.
 the effect is: tool triggers a lot of errors, including fatal errros like
GNUTLS_E_INTERNAL_ERROR, or GNUTLS_E_UNEXPECTED_PACKET
and others. while we libmicrohttpd retires for non-fatal E_AGAIN and
E_INTERRUPTED
it does nothing apparently when a fatal error apperas and no matter how many
non-fatal errors occur we do a retry. i think that:

 - we should close connection of fatal error during handshake
 - retry only a given number of times

and this patch does just that.
 i knew that Christian will not like the idea of "blocking" in
MHD_tls_connection_handle_read but it we have to retry only
finite number of times, maybe some other solution should be
proposed, the loop in this patch is only a demonstration.

sincerely,
pg


On Tue, Aug 9, 2011 at 9:26 AM, Christian Grothoff
<address@hidden> wrote:
> 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]
>