[libvirt] [PATCH] remote: Fix libvirtd service memory leak when libvirt client was terminated unexpectly

[xinhua.Cao]

在 2017/11/3 1:29, Martin Kletzander 写道:
> On Tue, Oct 24, 2017 at 01:52:28PM +0800, xinhua.Cao wrote:
>> base on commit 2033e8cc119454bc4273e8a41e66c899c60ba58b and 
>> fe8f1c8b8650c8ab5e7d27338f4538582651bd14, we solve libvirt coredump 
>> problem, but it introduce a memory leak sense.
>
> The first one is just a syntax sugar, it introduces no functional change.
>
yes, this patch is OK. because first patch and second patch have same 
relationship, so I point those two patch.
>> the sense follow
>> 1. one client register a domain event such as reboot event
>> 2. and client was terminated unexpectly, then this client will not 
>> free at libvirtd service program.
>>
>> remoteDispatchConnectDomainEventCallbackRegisterAny reference the 
>> client, but when client was terminated before it call deRegisterAny, 
>> the reference of client will not reduced to zero. so the memory leak 
>> take place. this patch will deRegister all event when client was close.
>
> Can you elaborate more on how does the client get terminated? Maybe 
> the problem
> is that there is a way to terminate the client and not call the 
> FreeFunc on it
> and the fact that it doesn't go through the right cleanup procedure 
> should be
> what we should focus on?
>
such as kill -9 or client crash.
> Also please wrap the commit message as any other commit.  See `git 
> log` for
> reference.
OK, it will be correct at v2 patch
>
>> ---
>> daemon/remote.c | 47 +++++++++++++++++++++++++++++------------------
>> 1 file changed, 29 insertions(+), 18 deletions(-)
>>
>> diff --git a/daemon/remote.c b/daemon/remote.c
>> index 3f7d2d3..2b5a18b 100644
>> --- a/daemon/remote.c
>> +++ b/daemon/remote.c
>> @@ -1686,25 +1686,16 @@ void 
>> remoteRelayConnectionClosedEvent(virConnectPtr conn ATTRIBUTE_UNUSED, 
>> int r
>>                 VIR_WARN("unexpected %s event deregister failure", 
>> name);   \
>> } \
>> VIR_FREE(eventCallbacks); \
>> +        neventCallbacks = 
>> 0;                                                \
>
> This is OK, ACK to this hunk.  But I think this should be in a 
> separate patch,
> probably.
>
OK, it will be at v2 patch
>>     } while (0);
>>
>> -/*
>> - * You must hold lock for at least the client
>> - * We don't free stuff here, merely disconnect the client's
>> - * network socket & resources.
>> - * We keep the libvirt connection open until any async
>> - * jobs have finished, then clean it up elsewhere
>> - */
>> -void remoteClientFreeFunc(void *data)
>> +static void
>> +remoteFreePrivCallbacks(void *data)
>
> Why is it called Callbacks when it is not passed as a callback 
> anywhere?  Why
> does it take void *?  Why does it not have a 'Client' in the name when it
> clearly works with a daemonClientPrivate data?
>
so can we use remoteClientFreePrivateCallbacks?
>> {
>>     struct daemonClientPrivate *priv = data;
>>
>>     /* Deregister event delivery callback */
>> -    if (priv->conn) {
>> -        virIdentityPtr sysident = virIdentityGetSystem();
>> -
>> -        virIdentitySetCurrent(sysident);
>> -
>> +    if (priv && priv->conn) {
>>         DEREG_CB(priv->conn, priv->domainEventCallbacks,
>>                  priv->ndomainEventCallbacks,
>>                  virConnectDomainEventDeregisterAny, "domain");
>> @@ -1723,6 +1714,26 @@ void remoteClientFreeFunc(void *data)
>>         DEREG_CB(priv->conn, priv->qemuEventCallbacks,
>>                  priv->nqemuEventCallbacks,
>>                  virConnectDomainQemuMonitorEventDeregister, "qemu 
>> monitor");
>> +    }
>> +}
>> +#undef DEREG_CB
>> +
>> +/*
>> + * You must hold lock for at least the client
>> + * We don't free stuff here, merely disconnect the client's
>> + * network socket & resources.
>> + * We keep the libvirt connection open until any async
>> + * jobs have finished, then clean it up elsewhere
>> + */
>> +void remoteClientFreeFunc(void *data)
>> +{
>> +    struct daemonClientPrivate *priv = data;
>> +
>> +    if (priv) {
>> +        virIdentityPtr sysident = virIdentityGetSystem();
>> +
>> +        virIdentitySetCurrent(sysident);
>> +        remoteFreePrivCallbacks(priv);
>>
>>         if (priv->closeRegistered) {
>>             if (virConnectUnregisterCloseCallback(priv->conn,
>
> Why don't you also remove this callback in the new function?  Does the 
> close
> event not get propagated when you move it there?
>
OK, it will be at v2 patch
>> @@ -1734,18 +1745,18 @@ void remoteClientFreeFunc(void *data)
>>
>>         virIdentitySetCurrent(NULL);
>>         virObjectUnref(sysident);
>> +        VIR_FREE(priv);
>>     }
>> -
>> -    VIR_FREE(priv);
>> }
>> -#undef DEREG_CB
>> -
>>
>> static void remoteClientCloseFunc(virNetServerClientPtr client)
>> {
>>     struct daemonClientPrivate *priv = 
>> virNetServerClientGetPrivateData(client);
>>
>> -    daemonRemoveAllClientStreams(priv->streams);
>> +    if (priv) {
>
> Can it happen that priv is NULL?  It should only be freed when the 
> client is
> freed in which case this function should not be called at all. This is a
> warning light for me, if you encountered priv == NULL in this 
> function, then it
> signals that there is probably a problem somewhere else as well.
>
there have no way to take place "priv is NULL", I check it only because 
my habit. I will delete it at v2 patch.
>> + daemonRemoveAllClientStreams(priv->streams);
>> +        remoteFreePrivCallbacks(priv);
>> +    }
>> }
>>
>
> Generally, I'm OK with splitting the Free function to two of them, one 
> doing the
> closing part and one freeing the data (similarly to what I suggested 
> in another
> thread for virNetDaemonDispose() just now), but this patch does it in 
> a very
> weird way.