Re: [libvirt] [PATCH] remote: Don't leak gnutls session on negotiation error

On 03/26/2011 07:59 AM, Matthias Bolte wrote: > --- > > Found while trying to reproduce another reported memory leak in doRemoteOpen. > > This leaked 10kb per failing call to negotiate_gnutls_on_connection. > >  src/remote/remote_driver.c |   28 +++++++++++++++++++--------- >  1 files changed, 19 insertions(+), 9 deletions(-) ACK with two changes removed: > > diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c > index b05bbcb..0915548 100644 > --- a/src/remote/remote_driver.c > +++ b/src/remote/remote_driver.c > @@ -1331,8 +1331,9 @@ negotiate_gnutls_on_connection (virConnectPtr conn, >          GNUTLS_CRT_OPENPGP, >          0 >      }; > +    bool success = false; Good. >      int err; > -    gnutls_session_t session; > +    gnutls_session_t session = NULL; This line is not appropriate; gnutls_session_t is an opaque type and might be a structure (and thus incompatible with NULL).  It happens to be a pointer now, but we shouldn't rely on that.

> >      /* Initialize TLS session >       */ > @@ -1341,7 +1342,7 @@ negotiate_gnutls_on_connection (virConnectPtr conn, >          remoteError(VIR_ERR_GNUTLS_ERROR, >                      _("unable to initialize TLS client: %s"), >                      gnutls_strerror (err)); > -        return NULL; > +        goto cleanup; This is the one place where session is still undefined, because init failed.  Therefore, it is still safe to return NULL at this point rather than jumping to cleanup. > > +    success = true; > + > +cleanup: > +    if (!success) { > +        gnutls_deinit(session); If you make the above two changes, then all subsequent locations that goto cleanup will now be guaranteed that session was properly initialized, and we no longer have to worry about calling gnutls_deinit(<uninitialized>) and whether the cleanup function is free-like.