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.
--
Eric Blake eblake@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list