[libvirt] [PATCH 2/3] avoid closing fd more than once

[Eric Blake]

On 05/30/2012 03:20 AM, Wen Congyang wrote:
> fdstream:
>     If fd is fds[0] or fds[1], we should set to -1 if we meet
>     some error.
> 
>     childfd is fds[0] or fds[1], so we should close it only when
>     virFDStreamOpenFileInternal() successes.
> 
> qemu_migration:
>     If we migrate to fd, spec->fwdType is not MIGRATION_FWD_DIRECT,
>     we will close spec->dest.fd.local in qemuMigrationRun(). So we
>     should set spec->dest.fd.local to -1 in qemuMigrationRun(). 
> 
> command:
>     we should not set *outfd or *errfd if virExecWithHook() failed
>     because the caller may close these fds.

We should split this into three separate patches, to aid backporting
each patch across appropriate versions.  Needs a v2 for this reason; as
I want these bugs fixed sooner rather than later, I'll probably help by
reposting things myself.

> 
> ---
>  src/fdstream.c            |   15 ++++++++++-----
>  src/qemu/qemu_migration.c |    4 +++-
>  src/util/command.c        |    8 ++++----
>  3 files changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/src/fdstream.c b/src/fdstream.c
> index 32d386d..d0ea0ee 100644
> --- a/src/fdstream.c
> +++ b/src/fdstream.c
> @@ -581,6 +581,7 @@ virFDStreamOpenFileInternal(virStreamPtr st,
>      struct stat sb;
>      virCommandPtr cmd = NULL;
>      int errfd = -1;
> +    int childfd = -1;
>  
>      VIR_DEBUG("st=%p path=%s oflags=%x offset=%llu length=%llu mode=%o",
>                st, path, oflags, offset, length, mode);
> @@ -619,7 +620,6 @@ virFDStreamOpenFileInternal(virStreamPtr st,
>      if ((st->flags & VIR_STREAM_NONBLOCK) &&
>          (!S_ISCHR(sb.st_mode) &&
>           !S_ISFIFO(sb.st_mode))) {
> -        int childfd;
>  
>          if ((oflags & O_ACCMODE) == O_RDWR) {
>              streamsReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -652,15 +652,20 @@ virFDStreamOpenFileInternal(virStreamPtr st,
>          }
>          virCommandSetErrorFD(cmd, &errfd);
>  
> -        if (virCommandRunAsync(cmd, NULL) < 0)
> +        if (virCommandRunAsync(cmd, NULL) < 0) {
> +            /* donot close fd twice if we meet some error */

s/donot/don't/

> +            fd = -1;
>              goto error;
> -
> -        VIR_FORCE_CLOSE(childfd);
> +        }
>      }
>  
> -    if (virFDStreamOpenInternal(st, fd, cmd, errfd, length) < 0)
> +    if (virFDStreamOpenInternal(st, fd, cmd, errfd, length) < 0) {
> +        /* donot close fd twice if we meet some error */

and again.

> +        fd = -1;
>          goto error;
> +    }
>  
> +    VIR_FORCE_CLOSE(childfd);
>      return 0;

Doesn't this leak childfd on error?

Maybe a better solution here would be that when we assign fd and childfd
to elements of fds[], then we also assign fds to -1, as in:

diff --git i/src/fdstream.c w/src/fdstream.c
index 32d386d..b797a05 100644
--- i/src/fdstream.c
+++ w/src/fdstream.c
@@ -581,6 +581,7 @@ virFDStreamOpenFileInternal(virStreamPtr st,
     struct stat sb;
     virCommandPtr cmd = NULL;
     int errfd = -1;
+    int childfd = -1;

     VIR_DEBUG("st=%p path=%s oflags=%x offset=%llu length=%llu mode=%o",
               st, path, oflags, offset, length, mode);
@@ -619,7 +620,6 @@ virFDStreamOpenFileInternal(virStreamPtr st,
     if ((st->flags & VIR_STREAM_NONBLOCK) &&
         (!S_ISCHR(sb.st_mode) &&
          !S_ISFIFO(sb.st_mode))) {
-        int childfd;

         if ((oflags & O_ACCMODE) == O_RDWR) {
             streamsReportError(VIR_ERR_INTERNAL_ERROR,
@@ -650,6 +650,7 @@ virFDStreamOpenFileInternal(virStreamPtr st,
             fd = fds[1];
             virCommandSetInputFD(cmd, childfd);
         }
+        fds[0] = fds[1] = -1;
         virCommandSetErrorFD(cmd, &errfd);

         if (virCommandRunAsync(cmd, NULL) < 0)
@@ -668,6 +669,7 @@ error:
     VIR_FORCE_CLOSE(fds[0]);
     VIR_FORCE_CLOSE(fds[1]);
     VIR_FORCE_CLOSE(fd);
+    VIR_FORCE_CLOSE(childfd);
     if (oflags & O_CREAT)
         unlink(path);
     return -1;


> +++ b/src/qemu/qemu_migration.c
> @@ -1910,8 +1910,10 @@ qemuMigrationRun(struct qemud_driver *driver,
>          break;
>  
>      case MIGRATION_DEST_FD:
> -        if (spec->fwdType != MIGRATION_FWD_DIRECT)
> +        if (spec->fwdType != MIGRATION_FWD_DIRECT) {
>              fd = spec->dest.fd.local;
> +            spec->dest.fd.local = -1;
> +        }
>          ret = qemuMonitorMigrateToFd(priv->mon, migrate_flags,
>                                       spec->dest.fd.qemu);
>          VIR_FORCE_CLOSE(spec->dest.fd.qemu);

This hunk is probably okay on its own.

> diff --git a/src/util/command.c b/src/util/command.c
> index eaa9f16..2723fde 100644
> --- a/src/util/command.c
> +++ b/src/util/command.c
> @@ -493,6 +493,10 @@ virExecWithHook(const char *const*argv,
>      }
>  
>      if (pid) { /* parent */
> +        if (forkRet < 0) {
> +            goto cleanup;
> +        }
> +
>          VIR_FORCE_CLOSE(null);
>          if (outfd && *outfd == -1) {
>              VIR_FORCE_CLOSE(pipeout[1]);
> @@ -503,10 +507,6 @@ virExecWithHook(const char *const*argv,
>              *errfd = pipeerr[0];
>          }
>  
> -        if (forkRet < 0) {
> -            goto cleanup;
> -        }
> -
>          *retpid = pid;
>  
>          if (binary != argv[0])

And this one is probably okay as well (I'll have to spend more time
reading it); again a reason for splitting these into independent patches.

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120530/21aa8160/attachment-0001.sig>