Re: 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 <address@hidden> 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
> > address@hidden
> > http://lists.gnu.org/mailman/listinfo/gnutls-devel

pgpu5F3kFC9Ds.pgp
Description: PGP signature