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(-)
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 an @inhibitFreeCb boolean in order to avoid calling
freecb in virObjectEventStateDeregisterID for any remote call failure in
a remoteConnect*EventRegister* API. For remoteConnect*EventDeregister* calls,
the passed value would be false indicating they should run the freecb if it
exists.
Patch based on the investigation and initial patch posted by:
fangying <fangying1@huawei.com>.
Signed-off-by: John Ferlan <jferlan@redhat.com>
---
Initial patch found at:
https://www.redhat.com/archives/libvir-list/2017-June/msg00039.html
based on feedback from Daniel Berrange to a previous posting:
https://www.redhat.com/archives/libvir-list/2017-May/msg01089.html
Since no new patch was posted, I posted my idea from review for
consideration.
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 ed2221a..6722dc4 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, false) < 0)
return -1;
return 0;
diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c
index 6e471d7..ff4c602 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, false);
}
diff --git a/src/conf/object_event.c b/src/conf/object_event.c
index e5f942f..5e944af 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
+ * @inhibitFreeCb: 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 inhibitFreeCb)
{
size_t i;
@@ -256,7 +258,10 @@ virObjectEventCallbackListRemoveID(virConnectPtr conn,
cb->key_filter ? cb->key : NULL,
cb->remoteID >= 0) - 1);
- if (cb->freecb)
+ /* inhibiting calling @freecb from error paths in register
+ * functions ensures the caller of the register function won't
+ * end up with a double free error */
+ if (!inhibitFreeCb && 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
+ * @inhibitFreeCb: Inhibit the calling of a freecb
*
* Unregister the function @callbackID with connection @conn,
- * from @state, for events.
+ * from @state, for events. If @inhibitFreeCb is true, 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 inhibitFreeCb)
{
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, inhibitFreeCb);
virObjectEventStateCleanupTimer(state, true);
diff --git a/src/conf/object_event.h b/src/conf/object_event.h
index 7a9995e..b36fcc9 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 inhibitFreeCb)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
int
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 0d65342..2995324 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, false) < 0)
return -1;
return 0;
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 22c8b58..ec357c6 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, false) < 0)
return -1;
return 0;
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 3ba7018..e9ad77e 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, false) < 0)
goto cleanup;
ret = 0;
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
index 760d73a..37e4e19 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, false) < 0)
goto cleanup;
ret = 0;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index e91663c..8c10599 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, false) < 0)
goto cleanup;
ret = 0;
@@ -18472,7 +18472,7 @@ qemuConnectDomainQemuMonitorEventDeregister(virConnectPtr conn,
goto cleanup;
if (virObjectEventStateDeregisterID(conn, driver->domainEventState,
- callbackID) < 0)
+ callbackID, false) < 0)
goto cleanup;
ret = 0;
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index b452e8b..6ebf910 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, true);
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, false)) < 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, true);
goto done;
}
@@ -3193,7 +3193,7 @@ remoteConnectStoragePoolEventDeregisterAny(virConnectPtr conn,
goto done;
if ((count = virObjectEventStateDeregisterID(conn, priv->eventState,
- callbackID)) < 0)
+ callbackID, false)) < 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, true);
goto done;
}
@@ -3290,7 +3290,7 @@ remoteConnectNodeDeviceEventDeregisterAny(virConnectPtr conn,
goto done;
if ((count = virObjectEventStateDeregisterID(conn, priv->eventState,
- callbackID)) < 0)
+ callbackID, false)) < 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, true);
goto done;
}
@@ -3387,7 +3387,7 @@ remoteConnectSecretEventDeregisterAny(virConnectPtr conn,
goto done;
if ((count = virObjectEventStateDeregisterID(conn, priv->eventState,
- callbackID)) < 0)
+ callbackID, false)) < 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, true);
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, false)) < 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, true);
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, true);
goto done;
}
}
@@ -4452,7 +4452,7 @@ remoteConnectDomainEventDeregister(virConnectPtr conn,
goto done;
if ((count = virObjectEventStateDeregisterID(conn, priv->eventState,
- callbackID)) < 0)
+ callbackID, false)) < 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, true);
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, true);
goto done;
}
}
@@ -5996,7 +5996,7 @@ remoteConnectDomainEventDeregisterAny(virConnectPtr conn,
goto done;
if ((count = virObjectEventStateDeregisterID(conn, priv->eventState,
- callbackID)) < 0)
+ callbackID, false)) < 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 fc01e6d..7d7ded8 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, false) < 0)
goto cleanup;
ret = 0;
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index ab1dc8f..698654c 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, false) < 0)
goto cleanup;
ret = 0;
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 11e7fd8..fec4d83 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, false) < 0)
ret = -1;
return ret;
@@ -5731,7 +5731,7 @@ testConnectNetworkEventDeregisterAny(virConnectPtr conn,
int ret = 0;
if (virObjectEventStateDeregisterID(conn, driver->eventState,
- callbackID) < 0)
+ callbackID, false) < 0)
ret = -1;
return ret;
@@ -5764,7 +5764,7 @@ testConnectStoragePoolEventDeregisterAny(virConnectPtr conn,
int ret = 0;
if (virObjectEventStateDeregisterID(conn, driver->eventState,
- callbackID) < 0)
+ callbackID, false) < 0)
ret = -1;
return ret;
@@ -5797,7 +5797,7 @@ testConnectNodeDeviceEventDeregisterAny(virConnectPtr conn,
int ret = 0;
if (virObjectEventStateDeregisterID(conn, driver->eventState,
- callbackID) < 0)
+ callbackID, false) < 0)
ret = -1;
return ret;
diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c
index 080fea4..c3dc9d2 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, false) < 0)
ret = -1;
umlDriverUnlock(driver);
diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c
index 7aa0c4c..1fa2e2f 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, false) < 0)
return -1;
return 0;
diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c
index 8e7bc35..f6d7fdc 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, false) < 0)
ret = -1;
xenUnifiedUnlock(priv);
--
2.9.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 06/23/2017 12:10 AM, John Ferlan wrote: > 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 an @inhibitFreeCb boolean in order to avoid calling > freecb in virObjectEventStateDeregisterID for any remote call failure in > a remoteConnect*EventRegister* API. For remoteConnect*EventDeregister* calls, > the passed value would be false indicating they should run the freecb if it > exists. > > Patch based on the investigation and initial patch posted by: > fangying <fangying1@huawei.com>. > > Signed-off-by: John Ferlan <jferlan@redhat.com> > --- > Initial patch found at: > > https://www.redhat.com/archives/libvir-list/2017-June/msg00039.html > > based on feedback from Daniel Berrange to a previous posting: > > https://www.redhat.com/archives/libvir-list/2017-May/msg01089.html > > Since no new patch was posted, I posted my idea from review for > consideration. > > 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 ed2221a..6722dc4 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, false) < 0) > return -1; > > return 0; > diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c > index 6e471d7..ff4c602 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, false); Why is this false? virDomainEventStateDeregister is called directly from public API implementations. For instance from qemuConnectDomainEventDeregister(). Don't we want to run free callback then? > } > Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 06/23/2017 05:39 AM, Michal Privoznik wrote:
> On 06/23/2017 12:10 AM, John Ferlan wrote:
>> 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 an @inhibitFreeCb boolean in order to avoid calling
>> freecb in virObjectEventStateDeregisterID for any remote call failure in
>> a remoteConnect*EventRegister* API. For remoteConnect*EventDeregister* calls,
>> the passed value would be false indicating they should run the freecb if it
>> exists.
>>
>> Patch based on the investigation and initial patch posted by:
>> fangying <fangying1@huawei.com>.
>>
>> Signed-off-by: John Ferlan <jferlan@redhat.com>
>> ---
>> Initial patch found at:
>>
>> https://www.redhat.com/archives/libvir-list/2017-June/msg00039.html
>>
>> based on feedback from Daniel Berrange to a previous posting:
>>
>> https://www.redhat.com/archives/libvir-list/2017-May/msg01089.html
>>
>> Since no new patch was posted, I posted my idea from review for
>> consideration.
>>
>> 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 ed2221a..6722dc4 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, false) < 0)
>> return -1;
>>
>> return 0;
>> diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c
>> index 6e471d7..ff4c602 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, false);
>
> Why is this false? virDomainEventStateDeregister is called directly from
> public API implementations. For instance from
> qemuConnectDomainEventDeregister(). Don't we want to run free callback then?
>
and the eventual check in virObjectEventCallbackListRemoveID is:
/* inhibiting calling @freecb from error paths in register
* functions ensures the caller of the register function won't
* end up with a double free error */
if (!inhibitFreeCb && cb->freecb)
IOW: Any time called from a Deregister path, we call the free callback;
however, when called from a RPC call REGISTER path, then we don't want
to call the free callback because we're returning -1 to the caller and
the caller will free things.
Or am I misreading your question. For a Deregister path if the
cb->freecb exists, then we want to call it like we have.
John
>> }
>>
>
> Michal
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.