[libvirt] [PATCH] Fix memory leak in virNetSocketNewConnectUNIX

[Michal Privoznik]

On 24.04.2015 12:03, Jiri Denemark wrote:
> ==26726==    by 0x673CD67: __vasprintf_chk (vasprintf_chk.c:80)
> ==26726==    by 0x5673605: UnknownInlinedFun (stdio2.h:210)
> ==26726==    by 0x5673605: virVasprintfInternal (virstring.c:476)
> ==26726==    by 0x56736EE: virAsprintfInternal (virstring.c:497)
> ==26726==    by 0x5680C37: virGetUserRuntimeDirectory (virutil.c:866)
> ==26726==    by 0x5783A89: virNetSocketNewConnectUNIX (virnetsocket.c:572)
> ==26726==    by 0x57751AF: virNetClientNewUNIX (virnetclient.c:344)
> ==26726==    by 0x57689B3: doRemoteOpen (remote_driver.c:895)
> ==26726==    by 0x5769F8E: remoteConnectOpen (remote_driver.c:1195)
> ==26726==    by 0x57092DF: do_open (libvirt.c:1189)
> ==26726==    by 0x570A7BF: virConnectOpenAuth (libvirt.c:1341)
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1215042
> Signed-off-by: Jiri Denemark <jdenemar at redhat.com>
> ---
>  src/rpc/virnetsocket.c | 44 +++++++++++++++++++++-----------------------
>  1 file changed, 21 insertions(+), 23 deletions(-)
> 
> diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
> index a59e3e1..7ae2796 100644
> --- a/src/rpc/virnetsocket.c
> +++ b/src/rpc/virnetsocket.c
> @@ -546,6 +546,7 @@ int virNetSocketNewConnectUNIX(const char *path,
>      virSocketAddr localAddr;
>      virSocketAddr remoteAddr;
>      char *rundir = NULL;
> +    int ret = -1;
>  
>      memset(&localAddr, 0, sizeof(localAddr));
>      memset(&remoteAddr, 0, sizeof(remoteAddr));
> @@ -559,50 +560,50 @@ int virNetSocketNewConnectUNIX(const char *path,
>              virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                             _("Auto-spawn of daemon requested, "
>                               "but no binary specified"));
> -            goto error;
> +            goto cleanup;
>          }
>  
>          if (!(binname = last_component(binary)) || binname[0] == '\0') {
>              virReportError(VIR_ERR_INTERNAL_ERROR,
>                             _("Cannot determine basename for binary '%s'"),
>                             binary);
> -            goto error;
> +            goto cleanup;
>          }
>  
>          if (!(rundir = virGetUserRuntimeDirectory()))
> -            goto error;
> +            goto cleanup;
>  
>          if (virFileMakePathWithMode(rundir, 0700) < 0) {
>              virReportSystemError(errno,
>                                   _("Cannot create user runtime directory '%s'"),
>                                   rundir);
> -            goto error;
> +            goto cleanup;
>          }
>  
>          if (virAsprintf(&lockpath, "%s/%s.lock", rundir, binname) < 0)
> -            goto error;
> +            goto cleanup;
>  
>          if ((lockfd = open(lockpath, O_RDWR | O_CREAT, 0600)) < 0 ||
>              virSetCloseExec(lockfd) < 0) {
>              virReportSystemError(errno, _("Unable to create lock '%s'"), lockpath);
> -            goto error;
> +            goto cleanup;
>          }
>  
>          if (virFileLock(lockfd, false, 0, 1, true) < 0) {
>              virReportSystemError(errno, _("Unable to lock '%s'"), lockpath);
> -            goto error;
> +            goto cleanup;
>          }
>      }
>  
>      if ((fd = socket(PF_UNIX, SOCK_STREAM, 0)) < 0) {
>          virReportSystemError(errno, "%s", _("Failed to create socket"));
> -        goto error;
> +        goto cleanup;
>      }
>  
>      remoteAddr.data.un.sun_family = AF_UNIX;
>      if (virStrcpyStatic(remoteAddr.data.un.sun_path, path) == NULL) {
>          virReportSystemError(ENOMEM, _("Path %s too long for unix socket"), path);
> -        goto error;
> +        goto cleanup;
>      }
>      if (remoteAddr.data.un.sun_path[0] == '@')
>          remoteAddr.data.un.sun_path[0] = '\0';
> @@ -612,42 +613,39 @@ int virNetSocketNewConnectUNIX(const char *path,
>          if (!(spawnDaemon && errno == ENOENT)) {
>              virReportSystemError(errno, _("Failed to connect socket to '%s'"),
>                                   path);
> -            goto error;
> +            goto cleanup;
>          }
>  
>          if (virNetSocketForkDaemon(binary) < 0)
> -            goto error;
> +            goto cleanup;
>  
>          retries--;
>          usleep(5000);
>      }
>  
> -    if (lockfd != -1) {
> -        unlink(lockpath);
> -        VIR_FORCE_CLOSE(lockfd);
> -        VIR_FREE(lockpath);
> -    }
> -

This extends the critical section, making others to wait a bit longer.
But I can live with that.

>      localAddr.len = sizeof(localAddr.data);
>      if (getsockname(fd, &localAddr.data.sa, &localAddr.len) < 0) {
>          virReportSystemError(errno, "%s", _("Unable to get local socket name"));
> -        goto error;
> +        goto cleanup;
>      }
>  
>      if (!(*retsock = virNetSocketNew(&localAddr, &remoteAddr, true, fd, -1, 0)))
> -        goto error;
> +        goto cleanup;
>  
> -    return 0;
> +    ret = 0;
>  
> - error:
> + cleanup:
>      if (lockfd != -1) {
>          unlink(lockpath);
>          VIR_FORCE_CLOSE(lockfd);
>      }
>      VIR_FREE(lockpath);
>      VIR_FREE(rundir);
> -    VIR_FORCE_CLOSE(fd);
> -    return -1;
> +
> +    if (ret == -1)

s/== -1/< 0/ please. It is more future proof IMO. For instance, if we
invent new return value to describe more specifically what went wrong,
and we still want the socket to close.

> +        VIR_FORCE_CLOSE(fd);
> +
> +    return ret;
>  }
>  #else
>  int virNetSocketNewConnectUNIX(const char *path ATTRIBUTE_UNUSED,
> 

ACK

Michal