2011/3/26 Eric Blake <eblake@xxxxxxxxxx>:
> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list