Memory leak in _gnutls_mpi_dprint_lz (possibly _gnutls_mpi_dprint)

[Sam Varshavchik]

I'm chasing a complaint from valgrind that I'm leaking memory.

Here's valgrind's complaint:

==26738== 257 bytes in 1 blocks are definitely lost in loss record 2 of 4
==26738==    at 0x4A0739E: malloc (vg_replace_malloc.c:207)
==26738==    by 0x35068328F6: _gnutls_mpi_dprint_lz (gnutls_mpi.c:146)
==26738==    by 0x350683E47C: _gnutls_dh_set_peer_public (gnutls_state.c:474)
==26738==    by 0x3506843819: _gnutls_proc_dh_common_server_kx 
(auth_dh_common.c:297)
==26738==    by 0x350683BB4F: proc_dhe_server_kx (auth_dhe.c:199)
==26738==    by 0x350682AF81: _gnutls_recv_server_kx_message (gnutls_kx.c:339)
==26738==    by 0x35068273DF: _gnutls_handshake_client (gnutls_handshake.c:2311)
==26738==    by 0x3506827F77: gnutls_handshake (gnutls_handshake.c:2193)


Here's what I've been able to figure out. I'm running gnutls 2.0.4, but I 
checked 2.4.0, and the affected bits have not changed, the following should 
still be applicable.

_gnutls_mpi_dprint_lz() allocates a buffer:

 if (bytes != 0)
   buf = gnutls_malloc (bytes);

. . . and puts it into its gnutls_datum_t parameter:

 if (!ret)
   {
     dest->data = buf;
     dest->size = bytes;
     return 0;
   }


If I set a breakpoint in _gnutls_dh_set_peer_public(), then I find that this 
breakpoint gets hit twice from gnutls_handshake():

#0  _gnutls_dh_set_peer_public (session=0x144fcc0, public=0x1431a70)
   at gnutls_state.c:474
#1  0x000000350684381a in _gnutls_proc_dh_common_server_kx (
   session=<value optimized out>, data=<value optimized out>, 
   _data_size=<value optimized out>, psk=<value optimized out>)
   at auth_dh_common.c:297
#2  0x000000350683bb50 in proc_dhe_server_kx (session=<value optimized out>, 
   data=<value optimized out>, _data_size=<value optimized out>)
   at auth_dhe.c:199
#3  0x000000350682af82 in _gnutls_recv_server_kx_message (
   session=<value optimized out>) at gnutls_kx.c:339
#4  0x00000035068273e0 in _gnutls_handshake_client (
   session=<value optimized out>) at gnutls_handshake.c:2311
#5  0x0000003506827f78 in gnutls_handshake (session=<value optimized out>)
   at gnutls_handshake.c:2193

Second breakpoint hit:

#0  _gnutls_dh_set_peer_public (session=0x144fcc0, public=0x1431a70)
   at gnutls_state.c:474
#1  0x0000003506843b8f in _gnutls_gen_dh_common_client_kx (
   session=<value optimized out>, data=<value optimized out>)
   at auth_dh_common.c:167
#2  0x000000350682aac1 in _gnutls_send_client_kx_message (
   session=<value optimized out>, again=<value optimized out>)
   at gnutls_kx.c:231
#3  0x000000350682736d in _gnutls_handshake_client (
   session=<value optimized out>) at gnutls_handshake.c:2352
#4  0x0000003506827f78 in gnutls_handshake (session=<value optimized out>)
   at gnutls_handshake.c:2193

Observe that _gnutls_dh_set_peer_public() invokes _gnutls_mpi_dprint_lz(), 
passing it the address of _gnutls_get_auth_info(session)->dh.public_key.

I hit the breakpoint the first time:

(gdb) p dh
$20 = (dh_info_st *) 0x8da8f8
(gdb) p $20->public_key
$21 = {data = 0x0, size = 0}

Empty unallocated buffer. Let me step into and out of the 
_gnutls_mpi_dprint_lz() function call:

(gdb) s
_gnutls_mpi_dprint_lz (dest=<value optimized out>, a=<value optimized out>)
   at gnutls_mpi.c:135
135     {
(gdb) finish
Run till exit from #0  _gnutls_mpi_dprint_lz (dest=<value optimized out>, 
   a=<value optimized out>) at gnutls_mpi.c:135
_gnutls_dh_set_peer_public (session=0x8d9bd0, public=0x8ad2b0)
   at gnutls_state.c:475
475       if (ret < 0)
Value returned is $22 = 0
(gdb) p $20->public_key
$23 = {data = 0x8e2cd0 "", size = 257}

_gnutls_mpi_dprint_lz() does its thing, and returns, leaving the public_key 
datum allocated. Fine.

I'm going to let it run now, and let it hit the breakpoint the 2nd time:

(gdb) c
Continuing.

Breakpoint 3, _gnutls_dh_set_peer_public (session=0x8d9bd0, public=0x8ad2b0)
   at gnutls_state.c:474
474       ret = _gnutls_mpi_dprint_lz (&dh→public_key, public);
(gdb) p dh
$24 = (dh_info_st *) 0x8da8f8
(gdb) p $24->public_key
$25 = {data = 0x8e2cd0 "", size = 257}

The buffer is still there, where it was before. Now you can see what's going 
to happen:

(gdb) s
_gnutls_mpi_dprint_lz (dest=<value optimized out>, a=<value optimized out>)
   at gnutls_mpi.c:135
135     {
(gdb) finish
Run till exit from #0  _gnutls_mpi_dprint_lz (dest=<value optimized out>, 
   a=<value optimized out>) at gnutls_mpi.c:135
_gnutls_dh_set_peer_public (session=0x8d9bd0, public=0x8ad2b0)
   at gnutls_state.c:475
475       if (ret < 0)
Value returned is $26 = 0
(gdb) p $24->public_key
$27 = {data = 0x8e7460 "", size = 257}

_gnutls_mpi_dprint_lz() did not check that its datum parameter was already 
initialized with a memory chunk, and allocated another memory block, 
overwriting the pointer, and leaking that memory block.

I cannot say what's the correct fix: have _gnutls_mpi_dprint_lz() clean up 
its gnutls_datum_t parameter, if it already points to some other memory 
chunk, or have _gnutls_dh_set_peer_public() check if dh->public_key is 
already initialized, and avoid calling _gnutls_mpi_dprint_lz() if so.

I also see the same logic in _gnutls_mpi_dprint() as well, there's a good 
possibility that there are execution paths that might trigger a leak there 
as well.