From 2065499b607a8df771761dc4249eff6ebc3adb5a Mon Sep 17 00:00:00 2001 From: John Ferlan Date: Wed, 14 Jun 2017 07:32:15 -0400 Subject: [PATCH] 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 . --- src/bhyve/bhyve_driver.c | 2 +- src/conf/domain_event.c | 2 +- src/conf/object_event.c | 23 ++++++++++++++------ src/conf/object_event.h | 3 ++- src/libxl/libxl_driver.c | 2 +- src/lxc/lxc_driver.c | 2 +- src/network/bridge_driver.c | 2 +- src/node_device/node_device_driver.c | 2 +- src/qemu/qemu_driver.c | 4 ++-- src/remote/remote_driver.c | 32 ++++++++++++++-------------- src/secret/secret_driver.c | 2 +- src/storage/storage_driver.c | 2 +- src/test/test_driver.c | 8 +++---- src/uml/uml_driver.c | 2 +- src/vz/vz_driver.c | 2 +- src/xen/xen_driver.c | 2 +- 16 files changed, 52 insertions(+), 40 deletions(-) diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index ed2221a352..550b257cd1 100644 --- 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 6e471d7ddb..7baccd5b57 100644 --- 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 e5f942f0cc..e8116b880c 100644 --- 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 7a9995e122..133f7ed914 100644 --- 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 0d6534206f..907f1776fc 100644 --- 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 22c8b58f96..652e9cba03 100644 --- 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 3ba70180b8..6b9aec86c5 100644 --- 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 760d73a469..bc5c0e50ec 100644 --- 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 e91663ca9a..cdb727b554 100644 --- 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 b452e8b009..a57d25f994 100644 --- 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 fc01e6d1b8..7c8be814c9 100644 --- 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 ab1dc8f364..0cc5c7cb08 100644 --- 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 11e7fd8804..54e252c333 100644 --- 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 080fea47da..224b719842 100644 --- 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 7aa0c4c48f..d47774c942 100644 --- 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 8e7bc350c8..b5cd47e02a 100644 --- 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);