[libvirt] [PATCH] EventRegister: Fix double deference error in libvirt_virConnectDomainEventRegisterAny
7:25 a.m.
As descriped at https://www.spinics.net/linux/fedora/libvir/msg148562.html,
even when virConnectDomainEventRegisterAny returns -1 there is still a
scenario in which it will trigger the free callback. We fix the problem in the
following way:
(1) implement a function virObjectEventStateSetFreeCB to add freecallback.
(2) call virObjectEventStateSetFreeCB only if the event is successfully added.
Signed-off-by: fangying <fangying1(a)huawei.com>
---
src/conf/object_event.c | 34 ++++++++++++++++++++++++++++++++++
src/conf/object_event.h | 7 +++++++
src/libvirt_private.syms | 2 +-
src/remote/remote_driver.c | 4 ++--
4 files changed, 44 insertions(+), 3 deletions(-)
diff --git a/src/conf/object_event.c b/src/conf/object_event.c
index e5f942f..a770978 100644
--- a/src/conf/object_event.c
+++ b/src/conf/object_event.c
@@ -923,6 +923,40 @@ virObjectEventStateRegisterID(virConnectPtr conn,
/**
+ * virObjectEventStateSetFreeCB:
+ * @conn: connection to associate with callback
+ * @state: object event state
+ * @callbackID: ID of the function to remove from event
+ * @freecb: freecb to be added
+ *
+ * Add the callbalck function @freecb for @callbackID with connection @conn,
+ * from @state, for events.
+ */
+void
+virObjectEventStateSetFreeCB(virConnectPtr conn,
+ virObjectEventStatePtr state,
+ int callbackID,
+ virFreeCallback freecb)
+{
+ size_t i;
+ virObjectEventCallbackListPtr cbList;
+
+ virObjectEventStateLock(state);
+ cbList = state->callbacks;
+ for (i = 0; i < cbList->count; i++) {
+ virObjectEventCallbackPtr cb = cbList->callbacks[i];
+
+ if (cb->callbackID == callbackID && cb->conn == conn) {
+ cb->freecb = freecb;
+ break;
+ }
+ }
+
+ virObjectEventStateUnlock(state);
+}
+
+
+/**
* virObjectEventStateDeregisterID:
* @conn: connection to associate with callback
* @state: object event state
diff --git a/src/conf/object_event.h b/src/conf/object_event.h
index 7a9995e..a7d29a0 100644
--- a/src/conf/object_event.h
+++ b/src/conf/object_event.h
@@ -90,4 +90,11 @@ virObjectEventStateSetRemote(virConnectPtr conn,
int remoteID)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+void
+virObjectEventStateSetFreeCB(virConnectPtr conn,
+ virObjectEventStatePtr state,
+ int callbackID,
+ virFreeCallback freecb)
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4);
+
#endif
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 429b095..e9d9cb6 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -791,7 +791,7 @@ virObjectEventStateDeregisterID;
virObjectEventStateEventID;
virObjectEventStateNew;
virObjectEventStateQueue;
-
+virObjectEventStateSetFreeCB;
# conf/secret_conf.h
virSecretDefFormat;
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index d27e96f..71177bd 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -5947,7 +5947,7 @@ remoteConnectDomainEventRegisterAny(virConnectPtr conn,
if ((count = virDomainEventStateRegisterClient(conn, priv->eventState,
dom, eventID, callback,
- opaque, freecb, false,
+ opaque, NULL, false,
&callbackID,
priv->serverEventFilter)) < 0)
goto done;
@@ -5993,7 +5993,7 @@ remoteConnectDomainEventRegisterAny(virConnectPtr conn,
}
rv = callbackID;
-
+ virObjectEventStateSetFreeCB(conn, priv->eventState, callbackID, freecb);
done:
remoteDriverUnlock(priv);
return rv;
--
2.10.0.windows.1
6:15 a.m.
On 06/01/2017 08:25 AM, fangying wrote:
As descriped at
https://www.spinics.net/linux/fedora/libvir/msg148562.html,
even when virConnectDomainEventRegisterAny returns -1 there is still a
scenario in which it will trigger the free callback. We fix the problem in the
following way:
(1) implement a function virObjectEventStateSetFreeCB to add freecallback.
(2) call virObjectEventStateSetFreeCB only if the event is successfully added.
Rather than putting a link to some page that may not exist some time in
the future, please describe the issue in the patch. Like you did in your
first version:
https://www.redhat.com/archives/libvir-list/2017-May/msg01089.html
Signed-off-by: fangying <fangying1(a)huawei.com>
If you look at other commits in libvirt you'll typically see a "first"
and "last" name... IOW: We prefer a more legal name as opposed to your
current login/email short name.
---
If you need to ever place a reference to some web page, when using git
send-email and you can "edit" your patch before sending, you can add
"reviewer comments" right after the "---". That helps see the history
of
the patch. If you have a multiple patch series, that type of
information can go in the cover letter.
src/conf/object_event.c | 34 ++++++++++++++++++++++++++++++++++
src/conf/object_event.h | 7 +++++++
src/libvirt_private.syms | 2 +-
src/remote/remote_driver.c | 4 ++--
4 files changed, 44 insertions(+), 3 deletions(-)
So this patch doesn't compile using top of tree, please see and follow
the guidelines at:
http://libvirt.org/hacking.html
diff --git a/src/conf/object_event.c b/src/conf/object_event.c
index e5f942f..a770978 100644
--- a/src/conf/object_event.c
+++ b/src/conf/object_event.c
@@ -923,6 +923,40 @@ virObjectEventStateRegisterID(virConnectPtr conn,
/**
+ * virObjectEventStateSetFreeCB:
+ * @conn: connection to associate with callback
+ * @state: object event state
+ * @callbackID: ID of the function to remove from event
+ * @freecb: freecb to be added
+ *
+ * Add the callbalck function @freecb for @callbackID with connection @conn,
+ * from @state, for events.
+ */
+void
+virObjectEventStateSetFreeCB(virConnectPtr conn,
+ virObjectEventStatePtr state,
+ int callbackID,
+ virFreeCallback freecb)
+{
+ size_t i;
+ virObjectEventCallbackListPtr cbList;
+
+ virObjectEventStateLock(state);
In particular, this *Lock function no longer exists. It was removed in
libvirt 2.4 by commit id '1827f2ac5d', you should have used
'virObjectLock(state);' instead.
+ cbList = state->callbacks;
+ for (i = 0; i < cbList->count; i++) {
+ virObjectEventCallbackPtr cb = cbList->callbacks[i];
+
+ if (cb->callbackID == callbackID && cb->conn == conn) {
+ cb->freecb = freecb;
+ break;
+ }
+ }
+
+ virObjectEventStateUnlock(state);
Similarly virObjectUnlock(state);
+}
+
+
+/**
* virObjectEventStateDeregisterID:
* @conn: connection to associate with callback
* @state: object event state
diff --git a/src/conf/object_event.h b/src/conf/object_event.h
index 7a9995e..a7d29a0 100644
--- a/src/conf/object_event.h
+++ b/src/conf/object_event.h
@@ -90,4 +90,11 @@ virObjectEventStateSetRemote(virConnectPtr conn,
int remoteID)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+void
+virObjectEventStateSetFreeCB(virConnectPtr conn,
+ virObjectEventStatePtr state,
+ int callbackID,
+ virFreeCallback freecb)
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4);
+
#endif
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 429b095..e9d9cb6 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -791,7 +791,7 @@ virObjectEventStateDeregisterID;
virObjectEventStateEventID;
virObjectEventStateNew;
virObjectEventStateQueue;
-
Spurious deletion. Follow the model in the module. Each .h file has a
sorted list of function names followed by 2 blank lines followed by the
next .h file.
+virObjectEventStateSetFreeCB;
# conf/secret_conf.h
virSecretDefFormat;
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index d27e96f..71177bd 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -5947,7 +5947,7 @@ remoteConnectDomainEventRegisterAny(virConnectPtr conn,
if ((count = virDomainEventStateRegisterClient(conn, priv->eventState,
dom, eventID, callback,
- opaque, freecb, false,
+ opaque, NULL, false,
&callbackID,
priv->serverEventFilter)) < 0)
goto done;
@@ -5993,7 +5993,7 @@ remoteConnectDomainEventRegisterAny(virConnectPtr conn,
}
rv = callbackID;
-
+ virObjectEventStateSetFreeCB(conn, priv->eventState, callbackID, freecb);
After reading Daniel's response to your initial posting and doing just a
small amount of investigating - it would seem that perhaps this may fix
one instance, but there are multiple callers pass a @freecb to a
vir*EventStateRegisterClient API, make a remote "call" that could fail,
and then call virObjectEventStateDeregisterID which would conceivably
have the same problem.
So instead of adjusting each of those to have this type set the freecb,
I think altering virObjectEventStateDeregisterID to take a boolean that
would inhibit calling the cb->freecb function may be a better approach.
For callers to virObjectEventStateDeregisterID from some
remoteConnect*EventDeregister* function, the boolean would be false.
IOW: Change virObjectEventStateDeregisterID to add a 4th parameter "bool
inhibitFreeCb". It's "true" on *error* paths from the remote calls
because returning -1 to the caller indicates that the caller needs to
free their opaque data since the freecb wouldn't be run, while calls
from various removeConnect*EventDeregister* APIs the parameter would be
"false" and the logic prior to calling the cb->freecb function is
altered to be "if (!inhibitFreeCb && cb->freecb)", with perhaps a
comment before it indicating that inhibitFreeCb would be used on error
paths to ensure/avoid the double free problem.
HTH,
John
done:
remoteDriverUnlock(priv);
return rv;
2753
days inactive
2766
days old
1 comments
2 participants
participants (2)
-
fangying -
John Ferlan