[libvirt] [PATCHv2 0/4] double close() fixes
Updated version of Wen's and my patches. In this version, I fixed review comments, split Wen's patch into 3, and tracked down when each bug was introduced. Eric Blake (2): command: avoid double close bugs fdstream: avoid double close bug Wen Congyang (2): command: check for fork error before closing fd qemu: avoid closing fd more than once src/fdstream.c | 7 +++---- src/qemu/qemu_migration.c | 4 +++- src/util/command.c | 24 ++++++++++-------------- 3 files changed, 16 insertions(+), 19 deletions(-) -- 1.7.7.6
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. The bug has been present since the introduction of virCommand in commit f16ad06f. This is based on his first attempt at a patch, at https://bugzilla.redhat.com/show_bug.cgi?id=823716 * src/util/command.c (_virCommand): Drop inpipe member. (virCommandProcessIO): Add argument, to avoid closing caller's fd without informing caller. (virCommandRun, virCommandNewArgs): Adjust clients. --- v2: fix logic bug when not at end of input string src/util/command.c | 16 ++++++---------- 1 files changed, 6 insertions(+), 10 deletions(-) diff --git a/src/util/command.c b/src/util/command.c index 5b94f1e..1a22508 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 @@ -1808,10 +1806,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 (VIR_CLOSE(*inpipe) < 0) + VIR_DEBUG("ignoring failed close on fd %d", infd); + infd = -1; } } } @@ -1938,7 +1935,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 +1977,7 @@ virCommandRun(virCommandPtr cmd, int *exitstatus) } if (string_io) - ret = virCommandProcessIO(cmd); + ret = virCommandProcessIO(cmd, &infd[1]); if (virCommandWait(cmd, exitstatus) < 0) ret = -1; -- 1.7.7.6
Wen Congyang reported that we have a double-close bug if we fail virFDStreamOpenInternal, since childfd duplicated one of the fds[] array contents. In truth, since we always transfer both members of fds to other variables, we should close the fds through those other names, and just use fds[] for pipe(). Bug present since 0.9.0 (commit e886237a). * src/fdstream.c (virFDStreamOpenFileInternal): Swap scope of childfd and fds[], to avoid a double close. --- v2: my alternative to Wen's original proposal src/fdstream.c | 7 +++---- 1 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/fdstream.c b/src/fdstream.c index fca0f41..a4b41c0 100644 --- a/src/fdstream.c +++ b/src/fdstream.c @@ -577,7 +577,7 @@ virFDStreamOpenFileInternal(virStreamPtr st, int mode) { int fd = -1; - int fds[2] = { -1, -1 }; + int childfd = -1; struct stat sb; virCommandPtr cmd = NULL; int errfd = -1; @@ -619,7 +619,7 @@ virFDStreamOpenFileInternal(virStreamPtr st, if ((st->flags & VIR_STREAM_NONBLOCK) && (!S_ISCHR(sb.st_mode) && !S_ISFIFO(sb.st_mode))) { - int childfd; + int fds[2] = { -1, -1 }; if ((oflags & O_ACCMODE) == O_RDWR) { streamsReportError(VIR_ERR_INTERNAL_ERROR, @@ -665,9 +665,8 @@ virFDStreamOpenFileInternal(virStreamPtr st, error: virCommandFree(cmd); - VIR_FORCE_CLOSE(fds[0]); - VIR_FORCE_CLOSE(fds[1]); VIR_FORCE_CLOSE(fd); + VIR_FORCE_CLOSE(childfd); VIR_FORCE_CLOSE(errfd); if (oflags & O_CREAT) unlink(path); -- 1.7.7.6
From: Wen Congyang <wency@cn.fujitsu.com> We should not set *outfd or *errfd if virExecWithHook() failed because the caller may close these fds. Bug present since v0.4.5 (commit 60ed1d2a). --- v2: split into separate patch, track down origin src/util/command.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/util/command.c b/src/util/command.c index 1a22508..ba43078 100644 --- a/src/util/command.c +++ b/src/util/command.c @@ -492,6 +492,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]); @@ -502,10 +506,6 @@ virExecWithHook(const char *const*argv, *errfd = pipeerr[0]; } - if (forkRet < 0) { - goto cleanup; - } - *retpid = pid; if (binary != argv[0]) -- 1.7.7.6
From: Wen Congyang <wency@cn.fujitsu.com> 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(). Bug present since 0.9.5 (commit 326176179). --- v2: split into separate patch, track down origin src/qemu/qemu_migration.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-) 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); -- 1.7.7.6
On 05/30/2012 09:43 PM, Eric Blake wrote:
Updated version of Wen's and my patches. In this version, I fixed review comments, split Wen's patch into 3, and tracked down when each bug was introduced.
Eric Blake (2): command: avoid double close bugs fdstream: avoid double close bug
Wen Congyang (2): command: check for fork error before closing fd qemu: avoid closing fd more than once
I've gone ahead and pushed this series. My patch 2/4 differs slightly from Wen's 1/3 on his repost, but the principle is the same - transfer semantics means that fds[] no longer need to be closed because another variable tracks them. Meanwhile, my 1/4 fixed the only thing found wrong by Wen in v1, and my 3/4 and 4/4 are identical to Wen's latest repost with nicer commit messages. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (1)
-
Eric Blake