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

[Matthias Bolte]

2011/3/26 Eric Blake <eblake at 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