On 04/26/2018 12:27 PM, Marc Hartmayer wrote:
> On Thu, Apr 26, 2018 at 05:07 PM +0200, John Ferlan <jferlan@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
>>> Reviewed-by: Boris Fiuczynski <fiuczy@xxxxxxxxxxxxxxxxxx>
>>> ---
>>> 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
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
