8:20 p.m.
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;
--
1.7.7.6
10:51 p.m.
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;
8:05 a.m.
On 05/29/2012 09:51 PM, Wen Congyang wrote:
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.
Thanks for helping on that front.
> + 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.
Oh, good catch. I'll post a v2.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
4:20 a.m.
New subject: [libvirt] [PATCH 1/3] avoid closing uninitialized fd
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
8:06 a.m.
New subject: [libvirt] [PATCH 1/3] avoid closing uninitialized fd
On 05/30/2012 03:20 AM, Wen Congyang wrote:
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;
ACK.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
4:20 a.m.
New subject: [libvirt] [PATCH 2/3] avoid closing fd more than once
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
8:17 a.m.
New subject: [libvirt] [PATCH 2/3] avoid closing fd more than once
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
10:03 p.m.
New subject: [libvirt] [PATCH 2/3] avoid closing fd more than once
At 05/30/2012 09:17 PM, Eric Blake Wrote:
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?
No, childfd is fds[0] or fds[1]. We have closed fds[0] and fds[1] on error,
so we should not close 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:
Good idea.
Thanks
Wen Congyang
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.
10:45 p.m.
New subject: [libvirt] [PATCH 2/3] avoid closing fd more than once
On 05/30/2012 09:03 PM, Wen Congyang wrote:
At 05/30/2012 09:17 PM, Eric Blake Wrote:
> 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.
>
>
> 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:
Good idea.
Looks like our email is crossing paths; I just reposted 4 patches as a
v2 series.
I like my version better than your repost, if only because I spent time
in 'git gui blame' tracking down which commit introduced each bug.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
4:20 a.m.
New subject: [libvirt] [PATCH 3/3] avoid fd leak
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
8:08 a.m.
New subject: [libvirt] [PATCH 3/3] avoid fd leak
On 05/30/2012 03:20 AM, Wen Congyang wrote:
virCommandRunAsync() will set errfd if it successes. We should
s/successes/succeeds/
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);
ACK.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
3:51 p.m.
New subject: [libvirt] [PATCH 3/3] avoid fd leak
On 05/30/2012 07:08 AM, Eric Blake wrote:
On 05/30/2012 03:20 AM, Wen Congyang wrote:
> virCommandRunAsync() will set errfd if it successes. We should
s/successes/succeeds/
> 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);
ACK.
I've pushed 1 and 3, and will post a v2 series with my patch and the
split-up version of your patch 2 once I complete another round of testing.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
4560
days inactive
4561
days old
11 comments
2 participants
participants (2)
-
Eric Blake -
Wen Congyang