On Thu, Apr 26, 2018 at 05:07 PM +0200, John Ferlan <jferlan@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@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@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?
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