On Fri, 2011-04-29 at 12:08 +0100, Bastian Blank wrote:
> On Thu, Apr 28, 2011 at 03:20:58PM +0100, Ian Campbell wrote:
> > On Thu, 2011-04-28 at 14:27 +0200, Bastian Blank wrote:
> > > - Probably missing close-on-exit flags for several file handlers.
> > You mean close-on-exec?
>
> Yes.
>
> > the libxl interfaces for exec'ing takes care of closing file handles and
>
> It tries to, but it can't ever succeed (there is no practical upper
> limit for the fd value). Usually this is used to _hide_ other errors.
Agreed, and anyway libxl_exec is a generic is supposed to be a generic
interface so having it close file handles which the caller _wants_ to
pass to the child is a bit unhelpful.
Lets try the below to start with.
Ian.
8<-----------------------------------------
# HG changeset patch
# User Ian Campbell <ian.campbell@xxxxxxxxxx>
# Date 1304597796 -3600
# Node ID 158e845f0d2cc1d263da4ee5ea725a0d90a0e42d
# Parent 117680fafe0c901171e9d519be37072ed0b8a765
libxl: don't close file descriptors 4..255 in libxl__exec
It prevents callers from deliberately passing file descriptors to the child,
hides any callers who erroneously do so and doesn't deal with all file
descriptors in any case.
Rather than remove it all together replace it with some debug code
which checks for open file handles which do not have either O_CLOEXEC
or FD_CLOEXEC set. To enable the debug set _LIBXL_DEBUG_EXEC_FDS=1 to
print any open and non-CLOEXEC non-stdio FDs just before libxl__exec
actualy calls exec. Set _LIBXL_DEBUG_EXEC_FDS=2 to abort if any of
these exist.
On the basis of this debugging fix some leaked filehandles:
* The read end of the pipe used to wake the parent from the
intermediate process during libxl__spawn_spawn was leaked into the
intermediate process.
* The file descriptor representing the xl lock was not marked
O_CLOEXEC (the lock itself is already specified to not be
inherited across a fork).
* The file descriptors passed to libxl__exec to be dup'd as
std{in,out,err} were leaked at their original number. They are
harmless (an attacker can just as easily use fd 0..2) but close
anyway since it removes a case which a person evaluating open fd's
needs to consider.
* libxl_run_bootloader was leaking the xenconsole pty master into
the bootloader child process.
* If the bootloader fails to get as far as opening its end of the
FIFO then we can also hang, check that the process has not exited
as part of that loop. (we actually block opening the FIFO too so
this is only a partial fix for the case where the bootlader has
crashed quickly).
With these fixes I have tested that device models, bootloaders
(pygrub) and vncviewers which are spawned via libxl__exec with no
unexpected file descriptors open, at least in my configuration.
Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
diff -r 117680fafe0c -r 158e845f0d2c tools/libxl/libxl_bootloader.c
--- a/tools/libxl/libxl_bootloader.c Wed May 04 17:03:14 2011 +0100
+++ b/tools/libxl/libxl_bootloader.c Thu May 05 13:16:36 2011 +0100
@@ -115,6 +115,7 @@ static int open_xenconsoled_pty(int *mas
#endif
fcntl(*master, F_SETFL, O_NDELAY);
+ fcntl(*master, F_SETFD, FD_CLOEXEC);
return 0;
}
@@ -393,6 +394,9 @@ int libxl_run_bootloader(libxl_ctx *ctx,
}
while (1) {
+ if (waitpid(pid, &blrc, WNOHANG) == pid)
+ goto out_close;
+
fifo_fd = open(fifo, O_RDONLY);
if (fifo_fd > -1)
break;
diff -r 117680fafe0c -r 158e845f0d2c tools/libxl/libxl_exec.c
--- a/tools/libxl/libxl_exec.c Wed May 04 17:03:14 2011 +0100
+++ b/tools/libxl/libxl_exec.c Thu May 05 13:16:36 2011 +0100
@@ -36,20 +36,72 @@ static int call_waitpid(pid_t (*waitpid_
return (waitpid_cb) ? waitpid_cb(pid, status, options) : waitpid(pid,
status, options);
}
+static void check_open_fds(const char *what)
+{
+ const char *env_debug;
+ int debug;
+ int i, flags, badness = 0;
+ char path[PATH_MAX];
+ char link[PATH_MAX+1];
+
+ env_debug = getenv("_LIBXL_DEBUG_EXEC_FDS");
+ if (!env_debug) return;
+
+ debug = strtol(env_debug, (char **) NULL, 10);atoi(env_debug);
+ if (debug <= 0) return;
+
+ for (i = 4; i < 256; i++) {
+#ifdef __linux__
+ size_t len;
+#endif
+ flags = fcntl(i, F_GETFD);
+ if ( flags == -1 ) {
+ if ( errno != EBADF )
+ fprintf(stderr, "libxl: execing %s: fd %d flags returned %s
(%d)\n",
+ what, i, strerror(errno), errno);
+ continue;
+ }
+
+ if ( flags & FD_CLOEXEC )
+ continue;
+
+ badness++;
+
+#ifdef __linux__
+ snprintf(path, PATH_MAX, "/proc/%d/fd/%d", getpid(), i);
+ len = readlink(path, link, PATH_MAX);
+ if (len > 0) {
+ link[len] = '\0';
+ fprintf(stderr, "libxl: execing %s: fd %d is open to %s with flags
%#x\n",
+ what, i, link, flags);
+ } else
+#endif
+ fprintf(stderr, "libxl: execing %s: fd %d is open with flags
%#x\n",
+ what, i, flags);
+ }
+ if (debug < 2) return;
+ if (badness) abort();
+}
+
void libxl__exec(int stdinfd, int stdoutfd, int stderrfd, const char *arg0,
char **args)
/* call this in the child */
{
- int i;
-
if (stdinfd != -1)
dup2(stdinfd, STDIN_FILENO);
if (stdoutfd != -1)
dup2(stdoutfd, STDOUT_FILENO);
if (stderrfd != -1)
dup2(stderrfd, STDERR_FILENO);
- for (i = 4; i < 256; i++)
- close(i);
+
+ if (stdinfd != -1)
+ close(stdinfd);
+ if (stdoutfd != -1 && stdoutfd != stdinfd)
+ close(stdoutfd);
+ if (stderrfd != -1 && stderrfd != stdinfd && stderrfd != stdoutfd)
+ close(stderrfd);
+
+ check_open_fds(arg0);
signal(SIGPIPE, SIG_DFL);
/* in case our caller set it to IGN. subprocesses are entitled
@@ -148,6 +200,7 @@ int libxl__spawn_spawn(libxl__gc *gc,
}
/* we are now the intermediate process */
+ if (for_spawn) close(pipes[0]);
child = fork();
if (child == -1)
diff -r 117680fafe0c -r 158e845f0d2c tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c Wed May 04 17:03:14 2011 +0100
+++ b/tools/libxl/xl_cmdimpl.c Thu May 05 13:16:36 2011 +0100
@@ -199,7 +199,7 @@ static int acquire_lock(void)
fl.l_whence = SEEK_SET;
fl.l_start = 0;
fl.l_len = 0;
- fd_lock = open(lockfile, O_WRONLY|O_CREAT, S_IWUSR);
+ fd_lock = open(lockfile, O_WRONLY|O_CREAT|O_CLOEXEC, S_IWUSR);
if (fd_lock < 0) {
fprintf(stderr, "cannot open the lockfile %s errno=%d\n", lockfile,
errno);
return ERROR_FAIL;
--
Ian Campbell
Is a person who blows up banks an econoclast?
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home