[libvirt] [PATCH libvirt v2 4/9] remote: Use domainClientEventCallbacks for remoteReplayConnectionClosedEvent

[John Ferlan]

On 04/26/2018 12:27 PM, Marc Hartmayer wrote:
> On Thu, Apr 26, 2018 at 05:07 PM +0200, John Ferlan <jferlan at redhat.com> wrote:
>> On 04/12/2018 08:40 AM, Marc Hartmayer wrote:
>>> This allows us to get rid of another usage of the global variable
>>> remoteProgram.
>>>
>>> Signed-off-by: Marc Hartmayer <mhartmay at linux.vnet.ibm.com>
>>> Reviewed-by: Boris Fiuczynski <fiuczy at linux.vnet.ibm.com>
>>> ---
>>>  src/remote/remote_daemon_dispatch.c | 18 ++++++++++++++----
>>>  1 file changed, 14 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
>>> index b4c0e01b0832..cf2cd0add7d6 100644
>>> --- a/src/remote/remote_daemon_dispatch.c
>>> +++ b/src/remote/remote_daemon_dispatch.c
>>> @@ -1661,12 +1661,12 @@ remoteRelayDomainQemuMonitorEvent(virConnectPtr conn,
>>>  static
>>>  void remoteRelayConnectionClosedEvent(virConnectPtr conn ATTRIBUTE_UNUSED, int reason, void *opaque)
>>>  {
>>> -    virNetServerClientPtr client = opaque;
>>> +    daemonClientEventCallbackPtr callback = opaque;
>>>
>>>      VIR_DEBUG("Relaying connection closed event, reason %d", reason);
>>>
>>>      remote_connect_event_connection_closed_msg msg = { reason };
>>> -    remoteDispatchObjectEventSend(client, remoteProgram,
>>> +    remoteDispatchObjectEventSend(callback->client, callback->program,
>>>                                    REMOTE_PROC_CONNECT_EVENT_CONNECTION_CLOSED,
>>>                                    (xdrproc_t)xdr_remote_connect_event_connection_closed_msg,
>>>                                    &msg);
>>> @@ -3836,6 +3836,7 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server ATTRIBUTE_UNUS
>>>                                             virNetMessageErrorPtr rerr)
>>>  {
>>>      int rv = -1;
>>> +    daemonClientEventCallbackPtr callback = NULL;
>>>      struct daemonClientPrivate *priv =
>>>          virNetServerClientGetPrivateData(client);
>>>
>>> @@ -3846,9 +3847,16 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server ATTRIBUTE_UNUS
>>>          goto cleanup;
>>>      }
>>>
>>> +    if (VIR_ALLOC(callback) < 0)
>>> +        goto cleanup;
>>> +    callback->client = virObjectRef(client);
>>
>> Oh and this would seem to fix a problem with the callback possibly using
>> @client after it had been freed!
> 
> The problem is more of a theoretical nature as Nikolay had explained:
> 
> “Refcounting was here originally but then removed in [1] as it conflicts with
> virConnectRegisterCloseCallback returns 0 in case connectRegisterCloseCallback
> is not implemented. This is safe though (at least nobody touch this place :).
> 
> [1] ce35122cfe: "daemon: fixup refcounting in close callback handling"”
> 
>>
>>> +    callback->program = virObjectRef(remoteProgram);
>>> +    /* eventID, callbackID, and legacy are not used */
>>> +    callback->eventID = -1;
>>> +    callback->callbackID = -1;
>>>      if (virConnectRegisterCloseCallback(priv->conn,
>>>                                          remoteRelayConnectionClosedEvent,
>>> -                                        client, NULL) < 0)
>>> +                                        callback, remoteEventCallbackFree) < 0)
>>>          goto cleanup;
>>>
>>
>> @callback would be leaked in the normal path...
> 
> By normal path, you mean without the first patch?
> 

I was following how the other register functions proceeded and they
saved the callback in a list to be freed at unregister.  So if Register
succeeds, rv == 0 and the removeEventCallbackFree(callback) isn't called
and we leak callback.  At least that's how I read it

John

>> If you consider all the
>> other callbacks will load @callback onto some list that gets run during
>> remoteClientFreePrivateCallbacks via DEREG_CB, this code would need to
>> do something similar. Since there's only 1 it's perhaps easier at
>> least.
>>
>>>      priv->closeRegistered = true;
>>
>> Perhaps change closeRegistered from bool to daemonClientEventCallbackPtr
>> and handle it that way similar to how other callback processing is
>> handled.
> 
> This would be one way how to deal with it (even without the first
> patch). But a double free error must be prevented.
> 
>>
>> John
>>
>>> @@ -3856,8 +3864,10 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server ATTRIBUTE_UNUS
>>>
>>>   cleanup:
>>>      virMutexUnlock(&priv->lock);
>>> -    if (rv < 0)
>>> +    if (rv < 0) {
>>> +        remoteEventCallbackFree(callback);
>>>          virNetMessageSaveError(rerr);
>>> +    }
>>>      return rv;
>>>  }
>>>
>>>
>>
> --
> Beste Grüße / Kind regards
>    Marc Hartmayer
> 
> IBM Deutschland Research & Development GmbH
> Vorsitzende des Aufsichtsrats: Martina Koederitz
> Geschäftsführung: Dirk Wittkopp
> Sitz der Gesellschaft: Böblingen
> Registergericht: Amtsgericht Stuttgart, HRB 243294
>