[libvirt] [PATCH] remote: Don't leak gnutls session on negotiation error
--- 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(-) 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; int err; - gnutls_session_t session; + gnutls_session_t session = NULL; /* 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; } /* Use default priorities */ @@ -1350,7 +1351,7 @@ negotiate_gnutls_on_connection (virConnectPtr conn, remoteError(VIR_ERR_GNUTLS_ERROR, _("unable to set TLS algorithm priority: %s"), gnutls_strerror (err)); - return NULL; + goto cleanup; } err = gnutls_certificate_type_set_priority (session, @@ -1359,7 +1360,7 @@ negotiate_gnutls_on_connection (virConnectPtr conn, remoteError(VIR_ERR_GNUTLS_ERROR, _("unable to set certificate priority: %s"), gnutls_strerror (err)); - return NULL; + goto cleanup; } /* put the x509 credentials to the current session @@ -1369,7 +1370,7 @@ negotiate_gnutls_on_connection (virConnectPtr conn, remoteError(VIR_ERR_GNUTLS_ERROR, _("unable to set session credentials: %s"), gnutls_strerror (err)); - return NULL; + goto cleanup; } gnutls_transport_set_ptr (session, @@ -1391,13 +1392,14 @@ negotiate_gnutls_on_connection (virConnectPtr conn, remoteError(VIR_ERR_GNUTLS_ERROR, _("unable to complete TLS handshake: %s"), gnutls_strerror (err)); - return NULL; + goto cleanup; } /* Verify certificate. */ if (verify_certificate (conn, priv, session) == -1) { VIR_DEBUG0("failed to verify peer's certificate"); - if (!no_verify) return NULL; + if (!no_verify) + goto cleanup; } /* At this point, the server is verifying _our_ certificate, IP address, @@ -1413,13 +1415,13 @@ negotiate_gnutls_on_connection (virConnectPtr conn, remoteError(VIR_ERR_GNUTLS_ERROR, _("unable to complete TLS initialization: %s"), gnutls_strerror (len)); - return NULL; + goto cleanup; } if (len != 1 || buf[0] != '\1') { remoteError(VIR_ERR_RPC, "%s", _("server verification (of our certificate or IP " "address) failed")); - return NULL; + goto cleanup; } #if 0 @@ -1427,6 +1429,14 @@ negotiate_gnutls_on_connection (virConnectPtr conn, print_info (session); #endif + success = true; + +cleanup: + if (!success) { + gnutls_deinit(session); + session = NULL; + } + return session; } -- 1.7.0.4
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
2011/3/26 Eric Blake <eblake@redhat.com>:
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.
Well, we already rely on gnutls_session_t being assignment compatible with NULL as the negotiate_gnutls_on_connection has return type gnutls_session_t and we return NULL on error. But I agree that your version needs this assumption in less places.
/* 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.
Okay, not relying on gnutls_deinit being free-like is a good idea. I applied you suggested changes and pushed the result. Matthias
participants (2)
-
Eric Blake -
Matthias Bolte