[libvirt] [PATCH] EventRegister: Fix double deference error in libvirt_virConnectDomainEventRegisterAny

[John Ferlan]

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 at 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;
>