events: Avoid double free possibility on remote call failure

If a remote call fails during event registration (more than likely from
a network failure or remote libvirtd restart timed just right), then when
calling the virObjectEventStateDeregisterID we don't want to call the
registered @freecb function because that breaks our contract that we
would only call it after succesfully returning.  If the @freecb routine
were called, it could result in a double free from properly coded
applications that free their opaque data on failure to register, as seen
in the following details:

    Program terminated with signal 6, Aborted.
    #0  0x00007fc45cba15d7 in raise
    #1  0x00007fc45cba2cc8 in abort
    #2  0x00007fc45cbe12f7 in __libc_message
    #3  0x00007fc45cbe86d3 in _int_free
    #4  0x00007fc45d8d292c in PyDict_Fini
    #5  0x00007fc45d94f46a in Py_Finalize
    #6  0x00007fc45d960735 in Py_Main
    #7  0x00007fc45cb8daf5 in __libc_start_main
    #8  0x0000000000400721 in _start

The double dereference of 'pyobj_cbData' is triggered in the following way:

    (1) libvirt_virConnectDomainEventRegisterAny is invoked.
    (2) the event is successfully added to the event callback list
        (virDomainEventStateRegisterClient in
        remoteConnectDomainEventRegisterAny returns 1 which means ok).
    (3) when function remoteConnectDomainEventRegisterAny is hit,
        network connection disconnected coincidently (or libvirtd is
        restarted) in the context of function 'call' then the connection
        is lost and the function 'call' failed, the branch
        virObjectEventStateDeregisterID is therefore taken.
    (4) 'pyobj_conn' is dereferenced the 1st time in
        libvirt_virConnectDomainEventFreeFunc.
    (5) 'pyobj_cbData' (refered to pyobj_conn) is dereferenced the
         2nd time in libvirt_virConnectDomainEventRegisterAny.
    (6) the double free error is triggered.

Resolve this by adding a @doFreeCb boolean in order to avoid calling the
freeCb in virObjectEventStateDeregisterID for any remote call failure in
a remoteConnect*EventRegister* API. For remoteConnect*EventDeregister* calls,
the passed value would be true indicating they should run the freecb if it
exists; whereas, it's false for the remote call failure path.

Patch based on the investigation and initial patch posted by
fangying <fangying1@huawei.com>.

diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c
index ed2221a352e6dfc8d941a1f06fcf345b3d76a07a..550b257cd15a1b45b4a2cb19b28998e0426e5dc9 100644 (file)
--- a/src/bhyve/bhyve_driver.c
+++ b/src/bhyve/bhyve_driver.c
@@ -1503,7 +1503,7 @@ bhyveConnectDomainEventDeregisterAny(virConnectPtr conn,
 
     if (virObjectEventStateDeregisterID(conn,
                                         privconn->domainEventState,
-                                        callbackID) < 0)
+                                        callbackID, true) < 0)
         return -1;
 
     return 0;

diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c
index 6e471d7ddb83b4d4cad8d5d44062018862cfa3ab..7baccd5b5751f8366c17096943015957472f123a 100644 (file)
--- a/src/conf/domain_event.c
+++ b/src/conf/domain_event.c
@@ -2301,7 +2301,7 @@ virDomainEventStateDeregister(virConnectPtr conn,
                                                NULL);
     if (callbackID < 0)
         return -1;
-    return virObjectEventStateDeregisterID(conn, state, callbackID);
+    return virObjectEventStateDeregisterID(conn, state, callbackID, true);
 }
 
 

diff --git a/src/conf/object_event.c b/src/conf/object_event.c
index e5f942f0cca932bf62ae95d8257c53a2839b1c96..e8116b880ccf277650a49845d821b52c7d5fa582 100644 (file)
--- a/src/conf/object_event.c
+++ b/src/conf/object_event.c
@@ -234,13 +234,15 @@ virObjectEventCallbackListCount(virConnectPtr conn,
  * @conn: pointer to the connection
  * @cbList: the list
  * @callback: the callback to remove
+ * @doFreeCb: Inhibit calling the freecb
  *
  * Internal function to remove a callback from a virObjectEventCallbackListPtr
  */
 static int
 virObjectEventCallbackListRemoveID(virConnectPtr conn,
                                    virObjectEventCallbackListPtr cbList,
-                                   int callbackID)
+                                   int callbackID,
+                                   bool doFreeCb)
 {
     size_t i;
 
@@ -256,7 +258,10 @@ virObjectEventCallbackListRemoveID(virConnectPtr conn,
                                                  cb->key_filter ? cb->key : NULL,
                                                  cb->remoteID >= 0) - 1);
 
-            if (cb->freecb)
+            /* @doFreeCb inhibits calling @freecb from error paths in
+             * register functions to ensure the caller of a failed register
+             * function won't end up with a double free error */
+            if (doFreeCb && cb->freecb)
                 (*cb->freecb)(cb->opaque);
             virObjectEventCallbackFree(cb);
             VIR_DELETE_ELEMENT(cbList->callbacks, i, cbList->count);
@@ -927,16 +932,22 @@ virObjectEventStateRegisterID(virConnectPtr conn,
  * @conn: connection to associate with callback
  * @state: object event state
  * @callbackID: ID of the function to remove from event
+ * @doFreeCb: Allow the calling of a freecb
  *
  * Unregister the function @callbackID with connection @conn,
- * from @state, for events.
+ * from @state, for events. If @doFreeCb is false, then we
+ * are being called from a remote call failure path for the
+ * Event registration indicating a -1 return to the caller. The
+ * caller wouldn't expect us to run their freecb function if it
+ * exists, so we cannot do so.
  *
  * Returns: the number of callbacks still registered, or -1 on error
  */
 int
 virObjectEventStateDeregisterID(virConnectPtr conn,
                                 virObjectEventStatePtr state,
-                                int callbackID)
+                                int callbackID,
+                                bool doFreeCb)
 {
     int ret;
 
@@ -946,8 +957,8 @@ virObjectEventStateDeregisterID(virConnectPtr conn,
                                                      state->callbacks,
                                                      callbackID);
     else
-        ret = virObjectEventCallbackListRemoveID(conn,
-                                                 state->callbacks, callbackID);
+        ret = virObjectEventCallbackListRemoveID(conn, state->callbacks,
+                                                 callbackID, doFreeCb);
 
     virObjectEventStateCleanupTimer(state, true);
 

diff --git a/src/conf/object_event.h b/src/conf/object_event.h
index 7a9995e122e7089cf47fcda8631ea4e6e4d1cf3c..133f7ed914ec0b93439314362c46b1c0f700eb5e 100644 (file)
--- a/src/conf/object_event.h
+++ b/src/conf/object_event.h
@@ -73,7 +73,8 @@ virObjectEventStateQueueRemote(virObjectEventStatePtr state,
 int
 virObjectEventStateDeregisterID(virConnectPtr conn,
                                 virObjectEventStatePtr state,
-                                int callbackID)
+                                int callbackID,
+                                bool doFreeCb)
     ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
 
 int

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 0d6534206f06769fe5b02baa4a4efabd439c456a..907f1776fc8260801627bb5b0c89af13b4f70ba9 100644 (file)
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -5652,7 +5652,7 @@ libxlConnectDomainEventDeregisterAny(virConnectPtr conn, int callbackID)
 
     if (virObjectEventStateDeregisterID(conn,
                                         driver->domainEventState,
-                                        callbackID) < 0)
+                                        callbackID, true) < 0)
         return -1;
 
     return 0;

diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 22c8b58f961aea31fdc0a8646352169cdf563144..652e9cba031efaf51d05145bcdf1ca3fb1c9ad29 100644 (file)
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -1479,7 +1479,7 @@ lxcConnectDomainEventDeregisterAny(virConnectPtr conn,
 
     if (virObjectEventStateDeregisterID(conn,
                                         driver->domainEventState,
-                                        callbackID) < 0)
+                                        callbackID, true) < 0)
         return -1;
 
     return 0;

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 3ba70180b865a96693abc41cd32062c4ec263337..6b9aec86c59c50c5a5e38a151fa91e79699af560 100644 (file)
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -3001,7 +3001,7 @@ networkConnectNetworkEventDeregisterAny(virConnectPtr conn,
 
     if (virObjectEventStateDeregisterID(conn,
                                         driver->networkEventState,
-                                        callbackID) < 0)
+                                        callbackID, true) < 0)
         goto cleanup;
 
     ret = 0;

diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
index 760d73a4694966bc362e82f067ae156589c13b4f..bc5c0e50ec15b2448187176a63b0157542eb0b97 100644 (file)
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -690,7 +690,7 @@ nodeConnectNodeDeviceEventDeregisterAny(virConnectPtr conn,
 
     if (virObjectEventStateDeregisterID(conn,
                                         driver->nodeDeviceEventState,
-                                        callbackID) < 0)
+                                        callbackID, true) < 0)
         goto cleanup;
 
     ret = 0;

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index e91663ca9a0ae4cfd6ec4291d000e2b83e268353..cdb727b55452391fe87b9c3eb87823a3e2a8a6eb 100644 (file)
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11826,7 +11826,7 @@ qemuConnectDomainEventDeregisterAny(virConnectPtr conn,
 
     if (virObjectEventStateDeregisterID(conn,
                                         driver->domainEventState,
-                                        callbackID) < 0)
+                                        callbackID, true) < 0)
         goto cleanup;
 
     ret = 0;
@@ -18472,7 +18472,7 @@ qemuConnectDomainQemuMonitorEventDeregister(virConnectPtr conn,
         goto cleanup;
 
     if (virObjectEventStateDeregisterID(conn, driver->domainEventState,
-                                        callbackID) < 0)
+                                        callbackID, true) < 0)
         goto cleanup;
 
     ret = 0;

diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index b452e8b00928531d25bfd0756ff950ce5ac138c5..a57d25f994e99038828d22cfb3f7823aad3aca7a 100644 (file)
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -3066,7 +3066,7 @@ remoteConnectNetworkEventRegisterAny(virConnectPtr conn,
                  (xdrproc_t) xdr_remote_connect_network_event_register_any_args, (char *) &args,
                  (xdrproc_t) xdr_remote_connect_network_event_register_any_ret, (char *) &ret) == -1) {
             virObjectEventStateDeregisterID(conn, priv->eventState,
-                                            callbackID);
+                                            callbackID, false);
             goto done;
         }
         virObjectEventStateSetRemote(conn, priv->eventState, callbackID,
@@ -3099,7 +3099,7 @@ remoteConnectNetworkEventDeregisterAny(virConnectPtr conn,
         goto done;
 
     if ((count = virObjectEventStateDeregisterID(conn, priv->eventState,
-                                                 callbackID)) < 0)
+                                                 callbackID, true)) < 0)
         goto done;
 
     /* If that was the last callback for this eventID, we need to disable
@@ -3160,7 +3160,7 @@ remoteConnectStoragePoolEventRegisterAny(virConnectPtr conn,
                  (xdrproc_t) xdr_remote_connect_storage_pool_event_register_any_args, (char *) &args,
                  (xdrproc_t) xdr_remote_connect_storage_pool_event_register_any_ret, (char *) &ret) == -1) {
             virObjectEventStateDeregisterID(conn, priv->eventState,
-                                            callbackID);
+                                            callbackID, false);
             goto done;
         }
 
@@ -3193,7 +3193,7 @@ remoteConnectStoragePoolEventDeregisterAny(virConnectPtr conn,
         goto done;
 
     if ((count = virObjectEventStateDeregisterID(conn, priv->eventState,
-                                                 callbackID)) < 0)
+                                                 callbackID, true)) < 0)
         goto done;
 
     /* If that was the last callback for this eventID, we need to disable
@@ -3256,7 +3256,7 @@ remoteConnectNodeDeviceEventRegisterAny(virConnectPtr conn,
                  (xdrproc_t) xdr_remote_connect_node_device_event_register_any_args, (char *) &args,
                  (xdrproc_t) xdr_remote_connect_node_device_event_register_any_ret, (char *) &ret) == -1) {
             virObjectEventStateDeregisterID(conn, priv->eventState,
-                                            callbackID);
+                                            callbackID, false);
             goto done;
         }
 
@@ -3290,7 +3290,7 @@ remoteConnectNodeDeviceEventDeregisterAny(virConnectPtr conn,
         goto done;
 
     if ((count = virObjectEventStateDeregisterID(conn, priv->eventState,
-                                                 callbackID)) < 0)
+                                                 callbackID, true)) < 0)
         goto done;
 
     /* If that was the last callback for this eventID, we need to disable
@@ -3353,7 +3353,7 @@ remoteConnectSecretEventRegisterAny(virConnectPtr conn,
                  (xdrproc_t) xdr_remote_connect_secret_event_register_any_args, (char *) &args,
                  (xdrproc_t) xdr_remote_connect_secret_event_register_any_ret, (char *) &ret) == -1) {
             virObjectEventStateDeregisterID(conn, priv->eventState,
-                                            callbackID);
+                                            callbackID, false);
             goto done;
         }
 
@@ -3387,7 +3387,7 @@ remoteConnectSecretEventDeregisterAny(virConnectPtr conn,
         goto done;
 
     if ((count = virObjectEventStateDeregisterID(conn, priv->eventState,
-                                                 callbackID)) < 0)
+                                                 callbackID, true)) < 0)
         goto done;
 
     /* If that was the last callback for this eventID, we need to disable
@@ -3453,7 +3453,7 @@ remoteConnectDomainQemuMonitorEventRegister(virConnectPtr conn,
                  (xdrproc_t) xdr_qemu_connect_domain_monitor_event_register_args, (char *) &args,
                  (xdrproc_t) xdr_qemu_connect_domain_monitor_event_register_ret, (char *) &ret) == -1) {
             virObjectEventStateDeregisterID(conn, priv->eventState,
-                                            callbackID);
+                                            callbackID, false);
             goto done;
         }
         virObjectEventStateSetRemote(conn, priv->eventState, callbackID,
@@ -3485,7 +3485,7 @@ remoteConnectDomainQemuMonitorEventDeregister(virConnectPtr conn,
         goto done;
 
     if ((count = virObjectEventStateDeregisterID(conn, priv->eventState,
-                                                 callbackID)) < 0)
+                                                 callbackID, true)) < 0)
         goto done;
 
     /* If that was the last callback for this event, we need to disable
@@ -4409,7 +4409,7 @@ remoteConnectDomainEventRegister(virConnectPtr conn,
                      (xdrproc_t) xdr_remote_connect_domain_event_callback_register_any_args, (char *) &args,
                      (xdrproc_t) xdr_remote_connect_domain_event_callback_register_any_ret, (char *) &ret) == -1) {
                 virObjectEventStateDeregisterID(conn, priv->eventState,
-                                                callbackID);
+                                                callbackID, false);
                 goto done;
             }
             virObjectEventStateSetRemote(conn, priv->eventState, callbackID,
@@ -4419,7 +4419,7 @@ remoteConnectDomainEventRegister(virConnectPtr conn,
                      (xdrproc_t) xdr_void, (char *) NULL,
                      (xdrproc_t) xdr_void, (char *) NULL) == -1) {
                 virObjectEventStateDeregisterID(conn, priv->eventState,
-                                                callbackID);
+                                                callbackID, false);
                 goto done;
             }
         }
@@ -4452,7 +4452,7 @@ remoteConnectDomainEventDeregister(virConnectPtr conn,
         goto done;
 
     if ((count = virObjectEventStateDeregisterID(conn, priv->eventState,
-                                                 callbackID)) < 0)
+                                                 callbackID, true)) < 0)
         goto done;
 
     if (count == 0) {
@@ -5951,7 +5951,7 @@ remoteConnectDomainEventRegisterAny(virConnectPtr conn,
                      (xdrproc_t) xdr_remote_connect_domain_event_callback_register_any_args, (char *) &args,
                      (xdrproc_t) xdr_remote_connect_domain_event_callback_register_any_ret, (char *) &ret) == -1) {
                 virObjectEventStateDeregisterID(conn, priv->eventState,
-                                                callbackID);
+                                                callbackID, false);
                 goto done;
             }
             virObjectEventStateSetRemote(conn, priv->eventState, callbackID,
@@ -5965,7 +5965,7 @@ remoteConnectDomainEventRegisterAny(virConnectPtr conn,
                      (xdrproc_t) xdr_remote_connect_domain_event_register_any_args, (char *) &args,
                      (xdrproc_t) xdr_void, (char *)NULL) == -1) {
                 virObjectEventStateDeregisterID(conn, priv->eventState,
-                                                callbackID);
+                                                callbackID, false);
                 goto done;
             }
         }
@@ -5996,7 +5996,7 @@ remoteConnectDomainEventDeregisterAny(virConnectPtr conn,
         goto done;
 
     if ((count = virObjectEventStateDeregisterID(conn, priv->eventState,
-                                                 callbackID)) < 0)
+                                                 callbackID, true)) < 0)
         goto done;
 
     /* If that was the last callback for this eventID, we need to disable

diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c
index fc01e6d1b8fb46d312b638fb1e5a139c254f5929..7c8be814c9fcbe579b9b0250ca14a4c4c72413e0 100644 (file)
--- a/src/secret/secret_driver.c
+++ b/src/secret/secret_driver.c
@@ -532,7 +532,7 @@ secretConnectSecretEventDeregisterAny(virConnectPtr conn,
 
     if (virObjectEventStateDeregisterID(conn,
                                         driver->secretEventState,
-                                        callbackID) < 0)
+                                        callbackID, true) < 0)
         goto cleanup;
 
     ret = 0;

diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index ab1dc8f3648d14b3e2ff108f471e2d671b2c400d..0cc5c7cb08a84fe6dd3b6039aa23d10005e44eec 100644 (file)
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -2662,7 +2662,7 @@ storageConnectStoragePoolEventDeregisterAny(virConnectPtr conn,
 
     if (virObjectEventStateDeregisterID(conn,
                                         driver->storageEventState,
-                                        callbackID) < 0)
+                                        callbackID, true) < 0)
         goto cleanup;
 
     ret = 0;

diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 11e7fd88040ec7540dc735b9639ba0520f30fd4a..54e252c333b0bae95538779657fb99a22b3cb64e 100644 (file)
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -5697,7 +5697,7 @@ testConnectDomainEventDeregisterAny(virConnectPtr conn,
     int ret = 0;
 
     if (virObjectEventStateDeregisterID(conn, driver->eventState,
-                                        callbackID) < 0)
+                                        callbackID, true) < 0)
         ret = -1;
 
     return ret;
@@ -5731,7 +5731,7 @@ testConnectNetworkEventDeregisterAny(virConnectPtr conn,
     int ret = 0;
 
     if (virObjectEventStateDeregisterID(conn, driver->eventState,
-                                        callbackID) < 0)
+                                        callbackID, true) < 0)
         ret = -1;
 
     return ret;
@@ -5764,7 +5764,7 @@ testConnectStoragePoolEventDeregisterAny(virConnectPtr conn,
     int ret = 0;
 
     if (virObjectEventStateDeregisterID(conn, driver->eventState,
-                                        callbackID) < 0)
+                                        callbackID, true) < 0)
         ret = -1;
 
     return ret;
@@ -5797,7 +5797,7 @@ testConnectNodeDeviceEventDeregisterAny(virConnectPtr conn,
     int ret = 0;
 
     if (virObjectEventStateDeregisterID(conn, driver->eventState,
-                                        callbackID) < 0)
+                                        callbackID, true) < 0)
         ret = -1;
 
     return ret;

diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c
index 080fea47dad4e3ea34314ce0bf4a59d8dfc3d1b3..224b71984250e045da1ce83f33e346608ebcc8f3 100644 (file)
--- a/src/uml/uml_driver.c
+++ b/src/uml/uml_driver.c
@@ -2721,7 +2721,7 @@ umlConnectDomainEventDeregisterAny(virConnectPtr conn,
     umlDriverLock(driver);
     if (virObjectEventStateDeregisterID(conn,
                                         driver->domainEventState,
-                                        callbackID) < 0)
+                                        callbackID, true) < 0)
         ret = -1;
     umlDriverUnlock(driver);
 

diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c
index 7aa0c4c48f192b2c8f2b29976d74645bc491a85f..d47774c942343da44f9020fa3f9246e467575a2f 100644 (file)
--- a/src/vz/vz_driver.c
+++ b/src/vz/vz_driver.c
@@ -1053,7 +1053,7 @@ vzConnectDomainEventDeregisterAny(virConnectPtr conn,
 
     if (virObjectEventStateDeregisterID(conn,
                                         privconn->driver->domainEventState,
-                                        callbackID) < 0)
+                                        callbackID, true) < 0)
         return -1;
 
     return 0;

diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c
index 8e7bc350c8a7fe1cb367091fc053a2b7a4d367cc..b5cd47e02abdaed8d892f82d21e8ff1b2a180f3c 100644 (file)
--- a/src/xen/xen_driver.c
+++ b/src/xen/xen_driver.c
@@ -2284,7 +2284,7 @@ xenUnifiedConnectDomainEventDeregisterAny(virConnectPtr conn,
 
     if (virObjectEventStateDeregisterID(conn,
                                         priv->domainEvents,
-                                        callbackID) < 0)
+                                        callbackID, true) < 0)
         ret = -1;
 
     xenUnifiedUnlock(priv);