Re: [libvirt] [PATCH] command: avoid double close bugs

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

* 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;

} } } @@ -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;