[libvirt] [PATCH v2 2/2] remote: free client all event callbacks at remoteClientCloseFunc

[John Ferlan]

On 11/06/2017 12:47 AM, xinhua.Cao wrote:
> base on commit fe8f1c8b8650c8ab5e7d27338f4538582651bd14, we solve
> libvirt coredump problem, but it introduce a memory leak sense:
> 
> 1. one client register a domain event such as reboot event
> 2. and client was terminated unexpectly, such as kill -9,

unexpectedly

> then this client object will not free at libvirtd service program.
> 
> remoteDispatchConnectDomainEventCallbackRegisterAny reference the client,
> but when client was terminated before it call deRegisterAny,the reference
> of client will not reduced to zero. so the memory leak take place.

What's the difference between the path that would terminate normally and
the one that terminates abnormally. It seems to me there might be (an)
extra Ref(s) on the client which prevents virNetServerClientDispose from
calling it's @privateDataFreeFunc which would call remoteClientFreeFunc
since remoteClientInitHook worked properly.

That's what needs to be found.

> this patch will deRegister all event callbacks when client was close.
> ---

FWIW: This commit message was a bit difficult to follow - I know it's
primarily a language barrier thing - so maybe if you just construct some
function code flow for both initialization/open and destruction/close
it'd help... Paths that work and the path that you think is broken...
That can be done in this section after the "---" and before the changed
API's.

I assume you've done a lot of research, but the details of that research
can be difficult to just "pick up on".  Typically someone will provide
some sort of valgrind output as an example.

I just have a sense that this could boil down to a path (or two) that
are doing the virObjectRef without the expected virObjectUnref occurring
- something perhaps to do with how @wantClose is handled in the abnormal
termination case. Nothing jumps out at me though even after looking at
the code for a while.


>  daemon/remote.c | 46 ++++++++++++++++++++++++++++------------------
>  1 file changed, 28 insertions(+), 18 deletions(-)
> 
> diff --git a/daemon/remote.c b/daemon/remote.c
> index cbcb6e8..466b2e7 100644
> --- a/daemon/remote.c
> +++ b/daemon/remote.c
> @@ -1689,23 +1689,10 @@ void remoteRelayConnectionClosedEvent(virConnectPtr conn ATTRIBUTE_UNUSED, int r
>          neventCallbacks = 0; \
>      } while (0);
>  
> -/*
> - * You must hold lock for at least the client
> - * We don't free stuff here, merely disconnect the client's
> - * network socket & resources.
> - * We keep the libvirt connection open until any async
> - * jobs have finished, then clean it up elsewhere
> - */
> -void remoteClientFreeFunc(void *data)
> +static void
> +remoteClientFreePrivateCallbacks(struct daemonClientPrivate *priv)
>  {
> -    struct daemonClientPrivate *priv = data;
> -
> -    /* Deregister event delivery callback */
> -    if (priv->conn) {
> -        virIdentityPtr sysident = virIdentityGetSystem();
> -
> -        virIdentitySetCurrent(sysident);
> -
> +    if (priv && priv->conn) {

Well each of the callers has already dereferenced @priv so it's a bit
too late to check it now!

>          DEREG_CB(priv->conn, priv->domainEventCallbacks,
>                   priv->ndomainEventCallbacks,
>                   virConnectDomainEventDeregisterAny, "domain");
> @@ -1730,16 +1717,38 @@ void remoteClientFreeFunc(void *data)
>                                                    remoteRelayConnectionClosedEvent) < 0)
>                  VIR_WARN("unexpected close callback event deregister failure");
>          }
> +    }
> +}
> +#undef DEREG_CB
> +

There should be another blank line here.

> +/*
> + * You must hold lock for at least the client
> + * We don't free stuff here, merely disconnect the client's
> + * network socket & resources.
> + * We keep the libvirt connection open until any async
> + * jobs have finished, then clean it up elsewhere
> + */
> +void remoteClientFreeFunc(void *data)
> +{
> +    struct daemonClientPrivate *priv = data;
> +
> +    /* Deregister event delivery callback */
> +    if (priv->conn) {
> +        virIdentityPtr sysident = virIdentityGetSystem();
> +
> +        virIdentitySetCurrent(sysident);
> +
> +        remoteClientFreePrivateCallbacks(priv);
>  
>          virConnectClose(priv->conn);
>  
>          virIdentitySetCurrent(NULL);
>          virObjectUnref(sysident);
> -    }
>  
> +    }
>      VIR_FREE(priv);
> +
>  }
> -#undef DEREG_CB
>  
>  
>  static void remoteClientCloseFunc(virNetServerClientPtr client)
> @@ -1747,6 +1756,7 @@ static void remoteClientCloseFunc(virNetServerClientPtr client)
>      struct daemonClientPrivate *priv = virNetServerClientGetPrivateData(client);
>  
>      daemonRemoveAllClientStreams(priv->streams);
> +    remoteClientFreePrivateCallbacks(priv);

Perhaps this just gets a "if (priv->conn)" before calling...

Still if as I assume the missing Unref is found (someday), then
theoretically remoteClientFreeFunc would be called - of course the
nEventCallbacks = 0 will ensure the Dereg's dont' get called again. So
it shoudn't be a problem

John

>  }
>  
>  
>