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 <[email protected]>.
open-power-host-os · Jun 25, 2017 · 2065499 · 2065499
1 parent 70c9b44
commit 2065499
Show file tree

Hide file tree

Showing 16 changed files with 52 additions and 40 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -2284,7 +2284,7 @@ xenUnifiedConnectDomainEventDeregisterAny(virConnectPtr conn,
 
     if (virObjectEventStateDeregisterID(conn,
                                         priv->domainEvents,
-                                        callbackID) < 0)
+                                        callbackID, true) < 0)
         ret = -1;
 
     xenUnifiedUnlock(priv);