[libvirt] [PATCH 1/8] remote: fix memory leak

[Matthias Bolte]

2011/3/26 Eric Blake <eblake at redhat.com>:
> Detected in this valgrind run:
> https://bugzilla.redhat.com/show_bug.cgi?id=690734
> ==13864== 10 bytes in 1 blocks are definitely lost in loss record 10 of 34
> ==13864==    at 0x4A05FDE: malloc (vg_replace_malloc.c:236)
> ==13864==    by 0x308587FD91: strdup (in /lib64/libc-2.12.so)
> ==13864==    by 0x3BB68BA0A3: doRemoteOpen (remote_driver.c:451)
> ==13864==    by 0x3BB68BCFCF: remoteOpen (remote_driver.c:1111)
> ==13864==    by 0x3BB689A0FF: do_open (libvirt.c:1290)
> ==13864==    by 0x3BB689ADD5: virConnectOpenAuth (libvirt.c:1545)
> ==13864==    by 0x41F659: main (virsh.c:11613)
>
> * src/remote/remote_driver.c (doRemoteClose): Move up.
> (remoteOpenSecondaryDriver, remoteOpen): Avoid leak on failure.
> ---
>
> Original post:
> https://www.redhat.com/archives/libvir-list/2011-March/msg01196.html
>
>  src/remote/remote_driver.c |  120 +++++++++++++++++++++++---------------------
>  1 files changed, 62 insertions(+), 58 deletions(-)
>
> diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
> index b05bbcb..4c7df17 100644
> --- a/src/remote/remote_driver.c
> +++ b/src/remote/remote_driver.c
> @@ -998,6 +998,64 @@ retry:
>     goto cleanup;
>  }
>
> +static int
> +doRemoteClose (virConnectPtr conn, struct private_data *priv)
> +{
> +    if (priv->eventFlushTimer >= 0) {
> +        /* Remove timeout */
> +        virEventRemoveTimeout(priv->eventFlushTimer);
> +        /* Remove handle for remote events */
> +        virEventRemoveHandle(priv->watch);
> +        priv->watch = -1;
> +    }
> +
> +    if (call (conn, priv, 0, REMOTE_PROC_CLOSE,
> +              (xdrproc_t) xdr_void, (char *) NULL,
> +              (xdrproc_t) xdr_void, (char *) NULL) == -1)
> +        return -1;

Preexisting problem, but when this remote call fails for some reason
we leak the gnutls and sasl sessions, leave FDs open and leak other
memory that would have been freed by the rest of the function.

> +    /* Close socket. */
> +    if (priv->uses_tls && priv->session) {
> +        gnutls_bye (priv->session, GNUTLS_SHUT_RDWR);
> +        gnutls_deinit (priv->session);
> +    }
> +#if HAVE_SASL
> +    if (priv->saslconn)
> +        sasl_dispose (&priv->saslconn);
> +#endif
> +    VIR_FORCE_CLOSE(priv->sock);
> +    VIR_FORCE_CLOSE(priv->errfd);
> +
> +#ifndef WIN32
> +    if (priv->pid > 0) {
> +        pid_t reap;
> +        do {
> +retry:
> +            reap = waitpid(priv->pid, NULL, 0);
> +            if (reap == -1 && errno == EINTR)
> +                goto retry;
> +        } while (reap != -1 && reap != priv->pid);
> +    }
> +#endif
> +    VIR_FORCE_CLOSE(priv->wakeupReadFD);
> +    VIR_FORCE_CLOSE(priv->wakeupSendFD);
> +
> +
> +    /* Free hostname copy */
> +    VIR_FREE(priv->hostname);
> +
> +    /* See comment for remoteType. */
> +    VIR_FREE(priv->type);
> +
> +    /* Free callback list */
> +    virDomainEventCallbackListFree(priv->callbackList);
> +
> +    /* Free queued events */
> +    virDomainEventQueueFree(priv->domainEvents);
> +
> +    return 0;
> +}
> +
>  static struct private_data *
>  remoteAllocPrivateData(void)
>  {
> @@ -1039,7 +1097,9 @@ remoteOpenSecondaryDriver(virConnectPtr conn,
>
>     ret = doRemoteOpen(conn, *priv, auth, rflags);
>     if (ret != VIR_DRV_OPEN_SUCCESS) {
> +        doRemoteClose(conn, *priv);

I'm not sure that this is the right thing to do here. I think we need
to make doRemoteOpen cleanup properly on a goto failure, as this is
the actual problem here.

doRemoteOpen can fail early before setting up the remote connection.
If we call doRemoteClose in such a situation the call of the call
function with REMOTE_PROC_CLOSE fail and we will leak more stuff.

Actually the valgrind log in the bug reports says "error: server
closed connection". In that case the call to the call function with
REMOTE_PROC_CLOSE will fail and the free functions for priv->hostname,
priv->callbackList and priv->domainEvents aren't called later in
doRemoteClose. Therefore you patch doesn't help here.

On the other hand I currently fail at reproducing the reported leaks.
I also can't understand how priv->domainEvents can leak when
doRemoteOpen returns != VIR_DRV_OPEN_SUCCESS, because when
priv->domainEvents got allocated then doRemoteOpen is guaranteed to
return VIR_DRV_OPEN_SUCCESS. That's another reason why I think adding
a call to doRemoteClose here is not correct, because the problem seems
to be somewhere else.

Were you actually able to reproduce the leaks and verify that this
patch fixes it?

>         remoteDriverUnlock(*priv);
> +        virMutexDestroy(&(*priv)->lock);

ACK, to this single addition.

>         VIR_FREE(*priv);
>     } else {
>         (*priv)->localUses = 1;
> @@ -1111,7 +1171,9 @@ remoteOpen (virConnectPtr conn,
>     ret = doRemoteOpen(conn, priv, auth, rflags);
>     if (ret != VIR_DRV_OPEN_SUCCESS) {
>         conn->privateData = NULL;
> +        doRemoteClose(conn, priv);

Here the same remarks apply as in remoteOpenSecondaryDriver.

>         remoteDriverUnlock(priv);
> +        virMutexDestroy(&priv->lock);

ACK, to this single addition too.

>         VIR_FREE(priv);
>     } else {
>         conn->privateData = priv;

Matthias