Two memory leak remaining, with fixes

[Sam Varshavchik]

Nikos Mavrogiannopoulos writes:

> Hello,
>  Thank you, I applied it (the last one with some additional removals).
> I'm curious though how there could be a leak. Maybe it's too late for
> me anyway...

Both patches address leaks caused by the same underlying problem. The 
functions in question allocate some session-specific objects, during a 
handshake, and save the pointer in the session structure.

When a /rehandshake/ occurs, gnutls_handshake() gets called again, a second 
time. The existing code just allocates new objects and puts them into the 
gnutls_session_int structure again. Pointers to objects allocated by the 
first call to gnutls_handshake(), during the first handshake, never get 
freed.

All of these leaks occur when gnutls_handshake() gets called a second time, 
not the first time.

There is no test case for this, but I can confirm that GnuTLS seems to 
properly implement a rehandshake of an existing GnuTLS session. At least, 
when the GnuTLS client connects to a GnuTLS server. If the server requests a 
handshake, the client accepts, gnutls_handshake() -- on both the client and 
the server -- seems to do the right thing. Except for leaking memory, of 
course.

> 
> On Sun, Sep 28, 2008 at 1:40 AM, Sam Varshavchik <mrsam at courier-mta.com> wrote:
>> Rerunning my valgrind checks against the latest CVS, two memory leaks remain
>> that are triggered by a session rehandshake.
>>
>> First leak:
>>
>> ==6235== 719 bytes in 13 blocks are definitely lost in loss record 3 of 5
>> ==6235==    at 0x4A0739E: malloc (vg_replace_malloc.c:207)
>> ==6235==    by 0x4C59EA0: _gnutls_set_datum_m (gnutls_datum.c:80)
>> ==6235==    by 0x4C63899: _gnutls_copy_certificate_auth_info
>> (auth_cert.c:98)
>> ==6235==    by 0x4C65519: _gnutls_proc_x509_server_certificate
>> (auth_cert.c:1027)
>> ==6235==    by 0x4C660AA: _gnutls_proc_cert_server_certificate
>> (auth_cert.c:1262)
>> ==6235==    by 0x4C52DE4: _gnutls_recv_server_certificate (gnutls_kx.c:719)
>> ==6235==    by 0x4C4D6AD: _gnutls_handshake_client (gnutls_handshake.c:2359)
>> ==6235==    by 0x4C4D268: gnutls_handshake (gnutls_handshake.c:2262)
>>
>> Second leak:
>>
>> ==3435==    at 0x4A0739E: malloc (vg_replace_malloc.c:207)
>> ==3435==    by 0x4C59EA0: _gnutls_set_datum_m (gnutls_datum.c:80)
>> ==3435==    by 0x4C61352: _gnutls_set_keys (gnutls_constate.c:130)
>> ==3435==    by 0x4C61FA0: _gnutls_set_write_keys (gnutls_constate.c:424)
>> ==3435==    by 0x4C62AD4: _gnutls_write_connection_state_init
>> (gnutls_constate.c:721)
>> ==3435==    by 0x4C4DCF0: _gnutls_send_handshake_final
>> (gnutls_handshake.c:2462)
>> ==3435==    by 0x4C4E89C: _gnutls_handshake_common (gnutls_handshake.c:2675)
>> ==3435==    by 0x4C4D2B1: gnutls_handshake (gnutls_handshake.c:2279)
>>
>> Investigating the second leak also found a number of other potential leaks
>> in gnutls_session_int.cipher_specs. It's probably a good idea to audit all
>> other parts of gnutls_session_int that might need explicit cleanup before
>> they are allocated, given that gnutls_handshake() may be called again as a
>> result of a rehandshake.
>>
>>
>> --- lib/auth_cert.c~    2008-09-23 20:30:32.000000000 -0400
>> +++ lib/auth_cert.c     2008-09-27 17:55:28.000000000 -0400
>> @@ -73,6 +73,9 @@
>>
>>  if (info->raw_certificate_list != NULL)
>>    {
>> +      for (j = 0; j < info->ncerts; j++)
>> +        _gnutls_free_datum (&info->raw_certificate_list[j]);
>> +
>>      gnutls_free( info->raw_certificate_list);
>>    }
>>
>> --- lib/gnutls_constate.c~      2008-09-25 20:30:41.000000000 -0400
>> +++ lib/gnutls_constate.c       2008-09-27 18:18:15.000000000 -0400
>> @@ -124,6 +124,13 @@
>>                    _gnutls_bin2hex (key_block, block_size, buf,
>>                                     sizeof (buf)));
>>
>> +  _gnutls_free_datum (&session->cipher_specs.server_write_mac_secret);
>> +  _gnutls_free_datum (&session->cipher_specs.client_write_mac_secret);
>> +  _gnutls_free_datum (&session->cipher_specs.server_write_IV);
>> +  _gnutls_free_datum (&session->cipher_specs.client_write_IV);
>> +  _gnutls_free_datum (&session->cipher_specs.server_write_key);
>> +  _gnutls_free_datum (&session->cipher_specs.client_write_key);
>> +
>>  pos = 0;
>>  if (hash_size > 0)
>>    {
>>
>>
>>
>> _______________________________________________
>> Gnutls-devel mailing list
>> Gnutls-devel at gnu.org
>> http://lists.gnu.org/mailman/listinfo/gnutls-devel
>>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: </pipermail/attachments/20080927/b486c372/attachment.pgp>