remote: fix lock ordering mistake in event registration

If the event (un)registration methods are invoked while no connection is
open, they jump to a cleanup block which unlocks a mutex which is not
currently locked.

Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
index 90103f509350afba77c9e9c3cf8368a66380bce2..4a3312a9449d46b1cd305da77bde01e60f261235 100644 (file)
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -4212,13 +4212,13 @@ remoteDispatchConnectDomainEventRegister(virNetServerPtr server ATTRIBUTE_UNUSED
     struct daemonClientPrivate *priv =
         virNetServerClientGetPrivateData(client);
 
+    virMutexLock(&priv->lock);
+
     if (!priv->conn) {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
         goto cleanup;
     }
 
-    virMutexLock(&priv->lock);
-
     /* If we call register first, we could append a complete callback
      * to our array, but on OOM append failure, we'd have to then hope
      * deregister works to undo our register.  So instead we append an
@@ -4276,13 +4276,13 @@ remoteDispatchConnectDomainEventDeregister(virNetServerPtr server ATTRIBUTE_UNUS
     struct daemonClientPrivate *priv =
         virNetServerClientGetPrivateData(client);
 
+    virMutexLock(&priv->lock);
+
     if (!priv->conn) {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
         goto cleanup;
     }
 
-    virMutexLock(&priv->lock);
-
     for (i = 0; i < priv->ndomainEventCallbacks; i++) {
         if (priv->domainEventCallbacks[i]->eventID == VIR_DOMAIN_EVENT_ID_LIFECYCLE) {
             callbackID = priv->domainEventCallbacks[i]->callbackID;
@@ -4440,13 +4440,13 @@ remoteDispatchConnectDomainEventRegisterAny(virNetServerPtr server ATTRIBUTE_UNU
     struct daemonClientPrivate *priv =
         virNetServerClientGetPrivateData(client);
 
+    virMutexLock(&priv->lock);
+
     if (!priv->conn) {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
         goto cleanup;
     }
 
-    virMutexLock(&priv->lock);
-
     /* We intentionally do not use VIR_DOMAIN_EVENT_ID_LAST here; any
      * new domain events added after this point should only use the
      * modern callback style of RPC.  */
@@ -4516,13 +4516,13 @@ remoteDispatchConnectDomainEventCallbackRegisterAny(virNetServerPtr server ATTRI
         virNetServerClientGetPrivateData(client);
     virDomainPtr dom = NULL;
 
+    virMutexLock(&priv->lock);
+
     if (!priv->conn) {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
         goto cleanup;
     }
 
-    virMutexLock(&priv->lock);
-
     if (args->dom &&
         !(dom = get_nonnull_domain(priv->conn, *args->dom)))
         goto cleanup;
@@ -4590,13 +4590,13 @@ remoteDispatchConnectDomainEventDeregisterAny(virNetServerPtr server ATTRIBUTE_U
     struct daemonClientPrivate *priv =
         virNetServerClientGetPrivateData(client);
 
+    virMutexLock(&priv->lock);
+
     if (!priv->conn) {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
         goto cleanup;
     }
 
-    virMutexLock(&priv->lock);
-
     /* We intentionally do not use VIR_DOMAIN_EVENT_ID_LAST here; any
      * new domain events added after this point should only use the
      * modern callback style of RPC.  */
@@ -4647,13 +4647,13 @@ remoteDispatchConnectDomainEventCallbackDeregisterAny(virNetServerPtr server ATT
     struct daemonClientPrivate *priv =
         virNetServerClientGetPrivateData(client);
 
+    virMutexLock(&priv->lock);
+
     if (!priv->conn) {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
         goto cleanup;
     }
 
-    virMutexLock(&priv->lock);
-
     for (i = 0; i < priv->ndomainEventCallbacks; i++) {
         if (priv->domainEventCallbacks[i]->callbackID == args->callbackID)
             break;
@@ -6089,13 +6089,13 @@ remoteDispatchConnectNetworkEventRegisterAny(virNetServerPtr server ATTRIBUTE_UN
         virNetServerClientGetPrivateData(client);
     virNetworkPtr net = NULL;
 
+    virMutexLock(&priv->lock);
+
     if (!priv->networkConn) {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
         goto cleanup;
     }
 
-    virMutexLock(&priv->lock);
-
     if (args->net &&
         !(net = get_nonnull_network(priv->networkConn, *args->net)))
         goto cleanup;
@@ -6162,13 +6162,13 @@ remoteDispatchConnectNetworkEventDeregisterAny(virNetServerPtr server ATTRIBUTE_
     struct daemonClientPrivate *priv =
         virNetServerClientGetPrivateData(client);
 
+    virMutexLock(&priv->lock);
+
     if (!priv->networkConn) {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
         goto cleanup;
     }
 
-    virMutexLock(&priv->lock);
-
     for (i = 0; i < priv->nnetworkEventCallbacks; i++) {
         if (priv->networkEventCallbacks[i]->callbackID == args->callbackID)
             break;
@@ -6211,13 +6211,13 @@ remoteDispatchConnectStoragePoolEventRegisterAny(virNetServerPtr server ATTRIBUT
         virNetServerClientGetPrivateData(client);
     virStoragePoolPtr  pool = NULL;
 
+    virMutexLock(&priv->lock);
+
     if (!priv->storageConn) {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
         goto cleanup;
     }
 
-    virMutexLock(&priv->lock);
-
     if (args->pool &&
         !(pool = get_nonnull_storage_pool(priv->storageConn, *args->pool)))
         goto cleanup;
@@ -6283,13 +6283,13 @@ remoteDispatchConnectStoragePoolEventDeregisterAny(virNetServerPtr server ATTRIB
     struct daemonClientPrivate *priv =
         virNetServerClientGetPrivateData(client);
 
+    virMutexLock(&priv->lock);
+
     if (!priv->storageConn) {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
         goto cleanup;
     }
 
-    virMutexLock(&priv->lock);
-
     for (i = 0; i < priv->nstorageEventCallbacks; i++) {
         if (priv->storageEventCallbacks[i]->callbackID == args->callbackID)
             break;
@@ -6332,13 +6332,13 @@ remoteDispatchConnectNodeDeviceEventRegisterAny(virNetServerPtr server ATTRIBUTE
         virNetServerClientGetPrivateData(client);
     virNodeDevicePtr  dev = NULL;
 
+    virMutexLock(&priv->lock);
+
     if (!priv->nodedevConn) {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
         goto cleanup;
     }
 
-    virMutexLock(&priv->lock);
-
     if (args->dev &&
         !(dev = get_nonnull_node_device(priv->nodedevConn, *args->dev)))
         goto cleanup;
@@ -6404,13 +6404,13 @@ remoteDispatchConnectNodeDeviceEventDeregisterAny(virNetServerPtr server ATTRIBU
     struct daemonClientPrivate *priv =
         virNetServerClientGetPrivateData(client);
 
+    virMutexLock(&priv->lock);
+
     if (!priv->nodedevConn) {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
         goto cleanup;
     }
 
-    virMutexLock(&priv->lock);
-
     for (i = 0; i < priv->nnodeDeviceEventCallbacks; i++) {
         if (priv->nodeDeviceEventCallbacks[i]->callbackID == args->callbackID)
             break;
@@ -6453,13 +6453,13 @@ remoteDispatchConnectSecretEventRegisterAny(virNetServerPtr server ATTRIBUTE_UNU
         virNetServerClientGetPrivateData(client);
     virSecretPtr secret = NULL;
 
+    virMutexLock(&priv->lock);
+
     if (!priv->secretConn) {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
         goto cleanup;
     }
 
-    virMutexLock(&priv->lock);
-
     if (args->secret &&
         !(secret = get_nonnull_secret(priv->secretConn, *args->secret)))
         goto cleanup;
@@ -6525,13 +6525,13 @@ remoteDispatchConnectSecretEventDeregisterAny(virNetServerPtr server ATTRIBUTE_U
     struct daemonClientPrivate *priv =
         virNetServerClientGetPrivateData(client);
 
+    virMutexLock(&priv->lock);
+
     if (!priv->secretConn) {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
         goto cleanup;
     }
 
-    virMutexLock(&priv->lock);
-
     for (i = 0; i < priv->nsecretEventCallbacks; i++) {
         if (priv->secretEventCallbacks[i]->callbackID == args->callbackID)
             break;
@@ -6575,13 +6575,13 @@ qemuDispatchConnectDomainMonitorEventRegister(virNetServerPtr server ATTRIBUTE_U
     virDomainPtr dom = NULL;
     const char *event = args->event ? *args->event : NULL;
 
+    virMutexLock(&priv->lock);
+
     if (!priv->conn) {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
         goto cleanup;
     }
 
-    virMutexLock(&priv->lock);
-
     if (args->dom &&
         !(dom = get_nonnull_domain(priv->conn, *args->dom)))
         goto cleanup;
@@ -6643,13 +6643,13 @@ qemuDispatchConnectDomainMonitorEventDeregister(virNetServerPtr server ATTRIBUTE
     struct daemonClientPrivate *priv =
         virNetServerClientGetPrivateData(client);
 
+    virMutexLock(&priv->lock);
+
     if (!priv->conn) {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
         goto cleanup;
     }
 
-    virMutexLock(&priv->lock);
-
     for (i = 0; i < priv->nqemuEventCallbacks; i++) {
         if (priv->qemuEventCallbacks[i]->callbackID == args->callbackID)
             break;