Re: [PATCH 2/3] virnetsocket: Resolve Coverity RESOURCE_LEAK

[Michal Privoznik <mprivozn@xxxxxxxxxx>]

On 22.08.2014 17:28, John Ferlan wrote:
> Since '1b807f92d' - Coverity complains that in the error paths of
> both virFork() and virProcessWait() that the 'passfd' will not be closed
> 
> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
> ---
>   src/rpc/virnetsocket.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
> index f913365..ce908fa 100644
> --- a/src/rpc/virnetsocket.c
> +++ b/src/rpc/virnetsocket.c
> @@ -593,8 +593,10 @@ int virNetSocketNewConnectUNIX(const char *path,
>            * behaviour on sockets according to POSIX, so it doesn't
>            * work outside Linux.
>            */
> -        if ((pid = virFork()) < 0)
> +        if ((pid = virFork()) < 0) {
> +            VIR_FORCE_CLOSE(passfd);
>               goto error;
> +        }
>   
>           if (pid == 0) {
>               umask(0077);
> @@ -604,8 +606,10 @@ int virNetSocketNewConnectUNIX(const char *path,
>               _exit(EXIT_SUCCESS);
>           }
>   
> -        if (virProcessWait(pid, &status, false) < 0)
> +        if (virProcessWait(pid, &status, false) < 0) {
> +            VIR_FORCE_CLOSE(passfd);
>               goto error;
> +        }
>   
>           if (status != EXIT_SUCCESS) {
>               /*
> 

Unfortunately not enough context is shown to express myself, so I'll paste interesting knobs from the function:

        if (listen(passfd, 0) < 0) {
             virReportSystemError(errno, "%s",
                                  _("Failed to listen on socket that's about "
                                    "to be passed to the daemon"));
             goto error;
         }
 
         if (connect(fd, &remoteAddr.data.sa, remoteAddr.len) < 0) {
             virReportSystemError(errno, _("Failed to connect socket to '%s'"),
                                  path);
             goto error;
         }
 
         if (virNetSocketForkDaemon(binary, passfd) < 0)
             goto error;
     }
 
     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;
     }
 
     if (!(*retsock = virNetSocketNew(&localAddr, &remoteAddr, true, fd, -1, 0)))
         goto error;
 
     return 0;
 
  error:
     VIR_FREE(buf);
     VIR_FORCE_CLOSE(fd);
     if (spawnDaemon)
         unlink(path);
     return -1;
 }


Here, if any of listen() or connect() fail, the control continues on the error label and the @passfd is leaked again.
virNetSocketForkDaemon() is different - it closes passfd on failure. So I suggest moving VIR_FORCE_CLOSE() under the error label.

Michal

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list