[libvirt] [PATCH] command: avoid fd leak on failure
virCommandTransferFD promises that the fd is no longer owned by the caller. Normally, we want the fd to remain open until the child runs, but in error situations, we must close it earlier. * src/util/command.c (virCommandTransferFD): Close fd now if we can't track it to close later. --- src/util/command.c | 10 ++++++++-- 1 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/util/command.c b/src/util/command.c index 3c516ec..83d4e88 100644 --- a/src/util/command.c +++ b/src/util/command.c @@ -738,7 +738,7 @@ virCommandKeepFD(virCommandPtr cmd, int fd, bool transfer) void virCommandPreserveFD(virCommandPtr cmd, int fd) { - return virCommandKeepFD(cmd, fd, false); + virCommandKeepFD(cmd, fd, false); } /* @@ -749,7 +749,13 @@ virCommandPreserveFD(virCommandPtr cmd, int fd) void virCommandTransferFD(virCommandPtr cmd, int fd) { - return virCommandKeepFD(cmd, fd, true); + virCommandKeepFD(cmd, fd, true); + if ((!cmd || cmd->has_error) && fd > STDERR_FILENO) { + /* We must close the fd now, instead of waiting for + * virCommandRun, if there was an error that prevented us from + * adding the fd to cmd. */ + VIR_FORCE_CLOSE(fd); + } } -- 1.7.4.4
On Tue, Jul 12, 2011 at 02:09:34PM -0600, Eric Blake wrote:
virCommandTransferFD promises that the fd is no longer owned by the caller. Normally, we want the fd to remain open until the child runs, but in error situations, we must close it earlier.
* src/util/command.c (virCommandTransferFD): Close fd now if we can't track it to close later. --- src/util/command.c | 10 ++++++++-- 1 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/src/util/command.c b/src/util/command.c index 3c516ec..83d4e88 100644 --- a/src/util/command.c +++ b/src/util/command.c @@ -738,7 +738,7 @@ virCommandKeepFD(virCommandPtr cmd, int fd, bool transfer) void virCommandPreserveFD(virCommandPtr cmd, int fd) { - return virCommandKeepFD(cmd, fd, false); + virCommandKeepFD(cmd, fd, false); }
I must admit I'm surprized, the compiler really doesn't warn if one does "return f()" if the caller is a void function. I.e. void is actually considered a value ??? I guess my brain still follows Pascal where procedure and functions were not the same...
/* @@ -749,7 +749,13 @@ virCommandPreserveFD(virCommandPtr cmd, int fd) void virCommandTransferFD(virCommandPtr cmd, int fd) { - return virCommandKeepFD(cmd, fd, true); + virCommandKeepFD(cmd, fd, true); + if ((!cmd || cmd->has_error) && fd > STDERR_FILENO) { + /* We must close the fd now, instead of waiting for + * virCommandRun, if there was an error that prevented us from + * adding the fd to cmd. */ + VIR_FORCE_CLOSE(fd); + } }
Hum, it's a bit like memory allocation, it's better to have the one who allocates it to free it to avoid double frees. Maybe we could instead raise and error in the caller chain, and have it freed at teh right place (unless it really get messy). opinion ? Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
At 07/13/2011 10:43 AM, Daniel Veillard Write:
On Tue, Jul 12, 2011 at 02:09:34PM -0600, Eric Blake wrote:
virCommandTransferFD promises that the fd is no longer owned by the caller. Normally, we want the fd to remain open until the child runs, but in error situations, we must close it earlier.
* src/util/command.c (virCommandTransferFD): Close fd now if we can't track it to close later. --- src/util/command.c | 10 ++++++++-- 1 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/src/util/command.c b/src/util/command.c index 3c516ec..83d4e88 100644 --- a/src/util/command.c +++ b/src/util/command.c @@ -738,7 +738,7 @@ virCommandKeepFD(virCommandPtr cmd, int fd, bool transfer) void virCommandPreserveFD(virCommandPtr cmd, int fd) { - return virCommandKeepFD(cmd, fd, false); + virCommandKeepFD(cmd, fd, false); }
I must admit I'm surprized, the compiler really doesn't warn if one does "return f()" if the caller is a void function. I.e. void
If f() and caller are void functions, gcc does not warn 'return f()'. If f() is not a void function, and caller is a void function, gcc will warn 'return f()' I guess gcc check the caller's return type and f()'s return type. If they are the same type, gcc does not warn.
is actually considered a value ??? I guess my brain still follows Pascal where procedure and functions were not the same...
/* @@ -749,7 +749,13 @@ virCommandPreserveFD(virCommandPtr cmd, int fd) void virCommandTransferFD(virCommandPtr cmd, int fd) { - return virCommandKeepFD(cmd, fd, true); + virCommandKeepFD(cmd, fd, true); + if ((!cmd || cmd->has_error) && fd > STDERR_FILENO) { + /* We must close the fd now, instead of waiting for + * virCommandRun, if there was an error that prevented us from + * adding the fd to cmd. */ + VIR_FORCE_CLOSE(fd); + } }
Hum, it's a bit like memory allocation, it's better to have the one who allocates it to free it to avoid double frees. Maybe we could instead raise and error in the caller chain, and have it freed at teh right place (unless it really get messy).
opinion ?
Daniel
On 07/12/2011 08:43 PM, Daniel Veillard wrote:
On Tue, Jul 12, 2011 at 02:09:34PM -0600, Eric Blake wrote:
virCommandTransferFD promises that the fd is no longer owned by the caller. Normally, we want the fd to remain open until the child runs, but in error situations, we must close it earlier.
* src/util/command.c (virCommandTransferFD): Close fd now if we can't track it to close later. ---
@@ -749,7 +749,13 @@ virCommandPreserveFD(virCommandPtr cmd, int fd) void virCommandTransferFD(virCommandPtr cmd, int fd) { - return virCommandKeepFD(cmd, fd, true); + virCommandKeepFD(cmd, fd, true); + if ((!cmd || cmd->has_error) && fd > STDERR_FILENO) { + /* We must close the fd now, instead of waiting for + * virCommandRun, if there was an error that prevented us from + * adding the fd to cmd. */ + VIR_FORCE_CLOSE(fd); + } }
Hum, it's a bit like memory allocation, it's better to have the one who allocates it to free it to avoid double frees. Maybe we could instead raise and error in the caller chain, and have it freed at teh right place (unless it really get messy).
That was the whole reason we introduced virCommandPreserveFD vs. virCommandTransferFD. With preserve, the caller both opens and closes fd. With transfer, the caller opens fd, then transfers it to virCommand and must not touch it again; virCommand then guarantees that it will be closed after the child runs. The problem was that we were not closing the fd in the few cases where either the child could not be run (due to a previous error, or because the fd was out of range of fdset). But I'm open to the idea of changing the signature: virCommandPreserveFD(virCommandPtr, int) - remains as-is virCommandTransferFD(virCommandPtr, int *) - change to passing the address of an fd, so that fd in the caller is set to -1 as part of the transfer process I'll post a v2 along those lines, so you can compare the difference (and also so that you'll see that existing callers are already abandoning fd after calling transfer, so setting it to -1 is only a safety valve). -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
virCommandTransferFD promises that the fd is no longer owned by the caller. Normally, we want the fd to remain open until the child runs, but in error situations, we must close it earlier. To avoid any risks of double-close due to misunderstanding about this interface, change it to take a pointer which gets set to -1 in the caller to record that the transfer was successful. * src/util/command.h (virCommandTransferFD): Alter signature. * src/util/command.c (virCommandTransferFD): Close fd now if we can't track it to close later. (virCommandKeepFD): Adjust helper to make this easier. (virCommandRequireHandshake): Adjust callers. * src/qemu/qemu_command.c (qemuBuildCommandLine): Likewise. * src/uml/uml_conf.c (umlBuildCommandLineChr): Likewise. * tests/commandtest.c (test3): Likewise. * docs/internals/command.html.in: Update the docs. --- v2: alter the signature, and adjust all callers, to make it foolproof at avoiding a double-close docs/internals/command.html.in | 2 +- src/qemu/qemu_command.c | 8 ++++---- src/uml/uml_conf.c | 2 +- src/util/command.c | 28 +++++++++++++++++----------- src/util/command.h | 9 +++++---- tests/commandtest.c | 6 +++++- 6 files changed, 33 insertions(+), 22 deletions(-) diff --git a/docs/internals/command.html.in b/docs/internals/command.html.in index 8a194ec..21536c4 100644 --- a/docs/internals/command.html.in +++ b/docs/internals/command.html.in @@ -257,7 +257,7 @@ int sharedfd = open("cmd.log", "w+"); int childfd = open("conf.txt", "r"); virCommandPreserveFD(cmd, sharedfd); - virCommandTransferFD(cmd, childfd); + virCommandTransferFD(cmd, &childfd); if (VIR_CLOSE(sharedfd) < 0) goto cleanup; </pre> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index a3bce4a..b413339 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3698,7 +3698,7 @@ qemuBuildCommandLine(virConnectPtr conn, goto error; last_good_net = i; - virCommandTransferFD(cmd, tapfd); + virCommandTransferFD(cmd, &tapfd); if (snprintf(tapfd_name, sizeof(tapfd_name), "%d", tapfd) >= sizeof(tapfd_name)) @@ -3710,7 +3710,7 @@ qemuBuildCommandLine(virConnectPtr conn, goto error; last_good_net = i; - virCommandTransferFD(cmd, tapfd); + virCommandTransferFD(cmd, &tapfd); if (snprintf(tapfd_name, sizeof(tapfd_name), "%d", tapfd) >= sizeof(tapfd_name)) @@ -3727,7 +3727,7 @@ qemuBuildCommandLine(virConnectPtr conn, if (qemuOpenVhostNet(def, net, qemuCaps, &vhostfd) < 0) goto error; if (vhostfd >= 0) { - virCommandTransferFD(cmd, vhostfd); + virCommandTransferFD(cmd, &vhostfd); if (snprintf(vhostfd_name, sizeof(vhostfd_name), "%d", vhostfd) >= sizeof(vhostfd_name)) @@ -4535,7 +4535,7 @@ qemuBuildCommandLine(virConnectPtr conn, goto no_memory; } - virCommandTransferFD(cmd, configfd); + virCommandTransferFD(cmd, &configfd); } } virCommandAddArg(cmd, "-device"); diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c index 0122472..4c2c2f1 100644 --- a/src/uml/uml_conf.c +++ b/src/uml/uml_conf.c @@ -376,7 +376,7 @@ umlBuildCommandLineChr(virDomainChrDefPtr def, VIR_FORCE_CLOSE(fd_out); return NULL; } - virCommandTransferFD(cmd, fd_out); + virCommandTransferFD(cmd, &fd_out); } break; case VIR_DOMAIN_CHR_TYPE_PIPE: diff --git a/src/util/command.c b/src/util/command.c index 3c516ec..f583b37 100644 --- a/src/util/command.c +++ b/src/util/command.c @@ -711,45 +711,51 @@ virCommandNewArgList(const char *binary, ...) * closing it. FD must not be one of the three standard streams. If * transfer is true, then fd will be closed in the parent after a call * to Run/RunAsync/Free, otherwise caller is still responsible for fd. + * Returns true if a transferring caller should close FD now, and + * false if the transfer is successfully recorded. */ -static void +static bool virCommandKeepFD(virCommandPtr cmd, int fd, bool transfer) { if (!cmd) - return; + return fd > STDERR_FILENO; if (fd <= STDERR_FILENO || FD_SETSIZE <= fd) { if (!cmd->has_error) cmd->has_error = -1; VIR_DEBUG("cannot preserve %d", fd); - return; + return fd > STDERR_FILENO; } FD_SET(fd, &cmd->preserve); if (transfer) FD_SET(fd, &cmd->transfer); + return false; } /* * Preserve the specified file descriptor - * in the child, instead of closing it. + * in the child, instead of closing it on exec. * The parent is still responsible for managing fd. */ void virCommandPreserveFD(virCommandPtr cmd, int fd) { - return virCommandKeepFD(cmd, fd, false); + virCommandKeepFD(cmd, fd, false); } /* * Transfer the specified file descriptor - * to the child, instead of closing it. - * Close the fd in the parent during Run/RunAsync/Free. + * to the child, instead of closing it on exec. + * The parent should no longer use fd, and the parent's copy will + * be automatically closed no later than during Run/RunAsync/Free. */ void -virCommandTransferFD(virCommandPtr cmd, int fd) +virCommandTransferFD(virCommandPtr cmd, int *fd) { - return virCommandKeepFD(cmd, fd, true); + if (virCommandKeepFD(cmd, *fd, true)) + VIR_FORCE_CLOSE(*fd); + *fd = -1; } @@ -2109,8 +2115,8 @@ void virCommandRequireHandshake(virCommandPtr cmd) VIR_DEBUG("Transfer handshake wait=%d notify=%d", cmd->handshakeWait[1], cmd->handshakeNotify[0]); - virCommandTransferFD(cmd, cmd->handshakeWait[1]); - virCommandTransferFD(cmd, cmd->handshakeNotify[0]); + virCommandTransferFD(cmd, &cmd->handshakeWait[1]); + virCommandTransferFD(cmd, &cmd->handshakeNotify[0]); cmd->handshake = true; } diff --git a/src/util/command.h b/src/util/command.h index c8a04f1..9e14215 100644 --- a/src/util/command.h +++ b/src/util/command.h @@ -67,7 +67,7 @@ virCommandPtr virCommandNewArgList(const char *binary, ...) /* * Preserve the specified file descriptor - * in the child, instead of closing it. + * in the child, instead of closing it on exec. * The parent is still responsible for managing fd. */ void virCommandPreserveFD(virCommandPtr cmd, @@ -75,11 +75,12 @@ void virCommandPreserveFD(virCommandPtr cmd, /* * Transfer the specified file descriptor - * to the child, instead of closing it. - * Close the fd in the parent during Run/RunAsync/Free. + * to the child, instead of closing it on exec. + * The parent should no longer use fd, and the parent's copy will + * be automatically closed no later than during Run/RunAsync/Free. */ void virCommandTransferFD(virCommandPtr cmd, - int fd); + int *fd); /* * Save the child PID in a pidfile diff --git a/tests/commandtest.c b/tests/commandtest.c index 6757253..ee8e7c3 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -181,7 +181,11 @@ static int test3(const void *unused ATTRIBUTE_UNUSED) int ret = -1; virCommandPreserveFD(cmd, newfd1); - virCommandTransferFD(cmd, newfd3); + virCommandTransferFD(cmd, &newfd3); + if (newfd3 != -1) { + puts("transfer should clear parent's fd"); + goto cleanup; + } if (virCommandRun(cmd, NULL) < 0) { virErrorPtr err = virGetLastError(); -- 1.7.4.4
On 07/14/2011 11:04 AM, Eric Blake wrote:
virCommandTransferFD promises that the fd is no longer owned by the caller. Normally, we want the fd to remain open until the child runs, but in error situations, we must close it earlier.
To avoid any risks of double-close due to misunderstanding about this interface, change it to take a pointer which gets set to -1 in the caller to record that the transfer was successful.
@@ -2109,8 +2115,8 @@ void virCommandRequireHandshake(virCommandPtr cmd)
VIR_DEBUG("Transfer handshake wait=%d notify=%d", cmd->handshakeWait[1], cmd->handshakeNotify[0]); - virCommandTransferFD(cmd, cmd->handshakeWait[1]); - virCommandTransferFD(cmd, cmd->handshakeNotify[0]); + virCommandTransferFD(cmd,&cmd->handshakeWait[1]); + virCommandTransferFD(cmd,&cmd->handshakeNotify[0]);
Unfortunately, resetting the caller's fd to -1 breaks handshake; here, cmd->handshakeWait[1] need to be closed in the parent, but not set to -1 until after the child has been forked (setting to -1 here was too soon). Squash this in: diff --git i/src/util/command.c w/src/util/command.c index 25494cf..3fda2cd 100644 --- i/src/util/command.c +++ w/src/util/command.c @@ -2292,6 +2292,8 @@ virCommandAbort(virCommandPtr cmd ATTRIBUTE_UNUSED) */ void virCommandRequireHandshake(virCommandPtr cmd) { + int tmp; + if (!cmd || cmd->has_error) return; @@ -2314,8 +2316,12 @@ void virCommandRequireHandshake(virCommandPtr cmd) VIR_DEBUG("Transfer handshake wait=%d notify=%d", cmd->handshakeWait[1], cmd->handshakeNotify[0]); - virCommandTransferFD(cmd, &cmd->handshakeWait[1]); - virCommandTransferFD(cmd, &cmd->handshakeNotify[0]); + /* Transfer the fds now, but don't change the handshake arrays, + * because the child must see what fd it inherited. */ + tmp = cmd->handshakeWait[1]; + virCommandTransferFD(cmd, &tmp); + tmp = cmd->handshakeNotify[0]; + virCommandTransferFD(cmd, &tmp); cmd->handshake = true; } -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 07/14/2011 11:04 AM, Eric Blake wrote:
virCommandTransferFD promises that the fd is no longer owned by the caller. Normally, we want the fd to remain open until the child runs, but in error situations, we must close it earlier.
To avoid any risks of double-close due to misunderstanding about this interface, change it to take a pointer which gets set to -1 in the caller to record that the transfer was successful.
* src/util/command.h (virCommandTransferFD): Alter signature. * src/util/command.c (virCommandTransferFD): Close fd now if we can't track it to close later. (virCommandKeepFD): Adjust helper to make this easier. (virCommandRequireHandshake): Adjust callers. * src/qemu/qemu_command.c (qemuBuildCommandLine): Likewise. * src/uml/uml_conf.c (umlBuildCommandLineChr): Likewise. * tests/commandtest.c (test3): Likewise. * docs/internals/command.html.in: Update the docs. ---
v2: alter the signature, and adjust all callers, to make it foolproof at avoiding a double-close
Another flaw in v2, that v1 did not have:
@@ -3698,7 +3698,7 @@ qemuBuildCommandLine(virConnectPtr conn, goto error;
last_good_net = i; - virCommandTransferFD(cmd, tapfd); + virCommandTransferFD(cmd,&tapfd);
if (snprintf(tapfd_name, sizeof(tapfd_name), "%d", tapfd)>= sizeof(tapfd_name))
Oops - this prints tapfd_name as -1 instead of the fd that the child will be inheriting. But rearranging the lines, and doing snprintf prior to virCommandTransferFD, is also problematic - if the snprintf fails, then we go to error without doing the virCommandTransferFD, then the virCommandPtr no longer closes the fd and we have a leak. I'm starting to think that resetting fd to -1 as a safety valve causes more harm than good, and hope that we can go with v1 after all. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (3)
-
Daniel Veillard -
Eric Blake -
Wen Congyang