At 05/30/2012 09:20 AM, Eric Blake Wrote:

KAMEZAWA Hiroyuki reported a nasty double-free bug when virCommand is used to convert a string into input to a child command. The problem is that the poll() loop of virCommandProcessIO would close() the write end of the pipe in order to let the child see EOF, then the caller virCommandRun() would also close the same fd number, with the second close possibly nuking an fd opened by some other thread in the meantime. This in turn can have all sorts of bad effects.

This is based on his first attempt at a patch, at https://bugzilla.redhat.com/show_bug.cgi?id=823716

close fd more twice is the cause of this bug. But there are some other codes that have the same problem. I am searching all such codes recent days.

* src/util/command.c (_virCommand): Drop inpipe member. (virCommandProcessIO): Add argument, to avoid closing caller's fd without informing caller. (virCommandRun, virCommandNewArgs): Adjust clients. --- src/util/command.c | 18 ++++++------------ 1 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/src/util/command.c b/src/util/command.c index eaa9f16..bf219e4 100644 --- a/src/util/command.c +++ b/src/util/command.c @@ -83,7 +83,6 @@ struct _virCommand { char **errbuf;

int infd; - int inpipe; int outfd; int errfd; int *outfdptr; @@ -788,7 +787,6 @@ virCommandNewArgs(const char *const*args) cmd->handshakeNotify[1] = -1;

cmd->infd = cmd->outfd = cmd->errfd = -1; - cmd->inpipe = -1; cmd->pid = -1;

virCommandAddArgSet(cmd, args); @@ -1676,7 +1674,7 @@ virCommandTranslateStatus(int status) * Manage input and output to the child process. */ static int -virCommandProcessIO(virCommandPtr cmd) +virCommandProcessIO(virCommandPtr cmd, int *inpipe) { int infd = -1, outfd = -1, errfd = -1; size_t inlen = 0, outlen = 0, errlen = 0; @@ -1687,7 +1685,7 @@ virCommandProcessIO(virCommandPtr cmd) * via pipe */ if (cmd->inbuf) { inlen = strlen(cmd->inbuf); - infd = cmd->inpipe; + infd = *inpipe; }

/* With out/err buffer, the outfd/errfd have been filled with an @@ -1807,12 +1805,9 @@ virCommandProcessIO(virCommandPtr cmd) } } else { inoff += done; - if (inoff == inlen) { - int tmpfd ATTRIBUTE_UNUSED; - tmpfd = infd; - if (VIR_CLOSE(infd) < 0) - VIR_DEBUG("ignoring failed close on fd %d", tmpfd); - } + if (inoff == inlen && VIR_CLOSE(*inpipe) < 0) + VIR_DEBUG("ignoring failed close on fd %d", infd); + infd = -1;

if inoff != inlen, we should not set infd to -1.

} } } @@ -1938,7 +1933,6 @@ virCommandRun(virCommandPtr cmd, int *exitstatus) return -1; } cmd->infd = infd[0]; - cmd->inpipe = infd[1]; }

/* If caller requested the same string for stdout and stderr, then @@ -1981,7 +1975,7 @@ virCommandRun(virCommandPtr cmd, int *exitstatus) }

if (string_io) - ret = virCommandProcessIO(cmd); + ret = virCommandProcessIO(cmd, &infd[1]);

if (virCommandWait(cmd, exitstatus) < 0) ret = -1;

If the system does not support bypass cache, we will close fd, but it is uninitialized. --- src/qemu/qemu_driver.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index fea9c24..1b3391b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4010,7 +4010,7 @@ qemuDomainSaveImageOpen(struct qemud_driver *driver, const char *xmlin, int state, bool edit, bool unlink_corrupt) { - int fd; + int fd = -1; struct qemud_save_header header; char *xml = NULL; virDomainDefPtr def = NULL; -- 1.7.1

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. --- 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 */ + 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 */ + fd = -1; goto error; + } + VIR_FORCE_CLOSE(childfd); return 0; error: diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 6f42823..b58380b 100644 --- a/src/qemu/qemu_migration.c +++ 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); 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]) -- 1.7.1

virCommandRunAsync() will set errfd if it successes. We should close it if virFDStreamOpenInternal() fails. --- src/fdstream.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/src/fdstream.c b/src/fdstream.c index d0ea0ee..f068439 100644 --- a/src/fdstream.c +++ b/src/fdstream.c @@ -673,6 +673,7 @@ error: VIR_FORCE_CLOSE(fds[0]); VIR_FORCE_CLOSE(fds[1]); VIR_FORCE_CLOSE(fd); + VIR_FORCE_CLOSE(errfd); if (oflags & O_CREAT) unlink(path); return -1; -- 1.7.1