On Thu, Mar 15, 2018 at 07:56 PM +0100, John Ferlan <jferlan@xxxxxxxxxx> wrote: > On 03/08/2018 07:20 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(-) >> > > Reviewed-by: John Ferlan <jferlan@xxxxxxxxxx> > > John > > (see my note below, I imagine you agree...) > >> diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c >> index 9580e854efbe..95940ffdeefe 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); >> @@ -3814,6 +3814,7 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server ATTRIBUTE_UNUS >> virNetMessageErrorPtr rerr) >> { >> int rv = -1; >> + daemonClientEventCallbackPtr callback = NULL; >> struct daemonClientPrivate *priv = >> virNetServerClientGetPrivateData(client); >> >> @@ -3824,9 +3825,16 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server ATTRIBUTE_UNUS >> goto cleanup; >> } >> >> + if (VIR_ALLOC(callback) < 0) >> + goto cleanup; >> + callback->client = virObjectRef(client); > > This one's scary because currently that means we currently call > virConnectRegisterCloseCallback, but haven't been doing the > virObjectRef(client) prior to using it as an opaque... Meaning > remoteRelayConnectionClosedEvent could be called with client already free'd. > > >> + 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; This is where I produced a memory leak, relying on virConnectRegisterCloseCallback to return < 0 if something failed. >> >> priv->closeRegistered = true; >> @@ -3834,8 +3842,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