Re: [libvirt] [PATCH RESEND] 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. (virCommandKeepFD): Adjust helper to make this easier. --- This leak can only happen on OOM or other extreme error conditions, but ought to be plugged. When I originally posted this: https://www.redhat.com/archives/libvir-list/2011-July/msg00674.html DV was worried that callers might abuse things and use fd even after this function closed it; but I proved to myself in writing a (non-working) v2 that all callers were already safe, and that this v1 was indeed a smaller solution.

src/util/command.c | 17 +++++++++++------ 1 files changed, 11 insertions(+), 6 deletions(-) diff --git a/src/util/command.c b/src/util/command.c index 699ba2d..c3ce361 100644 --- a/src/util/command.c +++ b/src/util/command.c @@ -730,23 +730,26 @@ 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; } /** @@ -761,7 +764,7 @@ virCommandKeepFD(virCommandPtr cmd, int fd, bool transfer) void virCommandPreserveFD(virCommandPtr cmd, int fd) { - return virCommandKeepFD(cmd, fd, false); + virCommandKeepFD(cmd, fd, false); } /** @@ -771,12 +774,14 @@ virCommandPreserveFD(virCommandPtr cmd, int fd) * * Transfer the specified file descriptor * to the child, instead of closing it on exec. - * Close the fd in the parent during Run/RunAsync/Free. + * 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) { - return virCommandKeepFD(cmd, fd, true); + if (virCommandKeepFD(cmd, fd, true)) + VIR_FORCE_CLOSE(fd); }