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?
> 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
