[libvirt] [PATCH v2 2/2] remote: free client all event callbacks at remoteClientCloseFunc

[Martin Kletzander]

On Mon, Nov 13, 2017 at 09:07:12PM +0800, xinhua.Cao wrote:
>
>
>在 2017/11/12 22:45, John Ferlan 写道:
>>
>> On 11/11/2017 03:20 AM, xinhua.Cao wrote:
>>>
>>> 在 2017/11/11 5:43, John Ferlan 写道:
>>>> On 11/06/2017 12:47 AM, xinhua.Cao wrote:
>>>>> base on commit fe8f1c8b8650c8ab5e7d27338f4538582651bd14, we solve
>>>>> libvirt coredump problem, but it introduce a memory leak sense:
>>>>>
>>>>> 1. one client register a domain event such as reboot event
>>>>> 2. and client was terminated unexpectly, such as kill -9,
>>>> unexpectedly
>>>>
>>>>> then this client object 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.
>>>> What's the difference between the path that would terminate normally and
>>>> the one that terminates abnormally. It seems to me there might be (an)
>>>> extra Ref(s) on the client which prevents virNetServerClientDispose from
>>>> calling it's @privateDataFreeFunc which would call remoteClientFreeFunc
>>>> since remoteClientInitHook worked properly.
>>>>
>>>> That's what needs to be found.
>>> If client terminate normally, client would call
>>> virConnectDomainEventDeregisterAny to
>>> clear eventCallbacks. Then libvirt daemon will call
>>> remoteDispatchConnectDomainEventDeregisterAny
>>> -> remoteEventCallbackFree -> virObjectUnref(callback->client) to free
>>> eventCallbacks and unRef client.
>>> just think if client don't call  virConnectDomainEventDeregisterAny and
>>> be killed. then there is no way to call call
>>> remoteDispatchConnectDomainEventDeregisterAny -> remoteEventCallbackFree
>>> -> virObjectUnref(callback->client)
>>> so client's ref can't down to zero. so remoteClientFreeFunc can't be
>>> called. then client object memory leak.
>> So the client/sample code is not calling Deregister because it has no
>> "handling" of an abnormal exit. That is no signal handler to call
>> Deregister in the event some outside force kills the client/sample code
>> while it's processing.
>>
>> So a different solution would be to add signal handling to the code in
>> order to handle the kill and make the Deregister (and close) calls.
>
>signal handler can handle such as kill -15 signal, But can't handle kill
>-9 signal.
>so we also need to resolve it in libvirt

Basically we need to handle bad behaving clients in the daemon anyway.  I tried
running libvirtd under valgrind and there was no extra leak with a client which
didn't deregister the callback.  How did you encounter this?  I really want to
find the place that I think needs to be changed, but without a reproducer this
is just a guessing game.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20171113/10641bff/attachment-0001.sig>