8:59 a.m.
---
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
10:10 a.m.
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
10:44 a.m.
2011/3/26 Eric Blake <eblake(a)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
4990
days inactive
4990
days old
2 comments
2 participants
participants (2)
-
Eric Blake -
Matthias Bolte