[PATCH] Ensure FD_CLOEXEC is set on all XenD file handles

["Daniel P. Berrange" <berrange@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2624 bytes --]

While debugging an issue with libvirt I discovered a serious problem
with XenD's management of file handles. Basically it does not set the
close-on-exec flag on any[1] of the file handles it has open. This means
that all XenD's file handles propagated to programs it spawns - eg, the
network device setup scripts, qemu-dm and others those in turn spawn.

The particular case that lead me to discover the problem was when I disabled
HTTP port on XenD and wondered why port 8000 was still in LISTEN state - it
turned out qemu-dm had a handle to the server socket. Aside from this it had
access to about 10 open handles on /proc/xen/privcmd, the HTTP *client*
connection which requested the domain creation[2], and any of the other
server sockets XenD happened to have open.

I'm attaching three prototype patches - I say prototype because there may
be other (better) places to set the FD_CLOEXEC flag - I've just picked
the place closest to the original socket()/accept() call.

 * xen-xend2-cloexec.patch - set the flag on the XMLRPC & HTTP server
   ports, both TCP & UNIX domain socket versions. Also ensure incoming
   clients have it set
 * xen-xc-cloexec.patch - set the flag on all handles to /proc/xen/privcmd
 * xen-xs2-cloexec.patch - set the flag on all connections to the
   xenstore daemon.

The only file handle I've not yet set the flag on is the one to the log
file /var/log/xend.log which (in my system) ends up on FD #9.

To test which file handles were being left open I created a dummy script:

  $ cat > /root/fake-qemu-dm.sh <<EOF
  #!/bin/sh

  lsof -P -n -p $$ >> /tmp/qemu-fds.log

  exec /usr/lib64/xen/bin/qemu-dm "$@"
  EOF
  $

And then in the domain's config file set 'device_model' to point to the
file '/root/fake-qemu-dm.sh'


BTW, the patches were prepared against the latest Xen userspace code in
Fedora Core 6, test2 - this is trailing xen-unstable by a couple of weeks
but I think they should still apply. If people agree with the approach
taken in the patch I'll re-diff against xen-unstable before posting again.

Regards,
Dan.

[1] Actually it turns out the relocation port does have the flag set
[2] This explains an issue discovered with libvirt where it would hang 
     forever after creating an HVM domain waiting for the server to close
    its end of the socket
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

[-- Attachment #2: xen-xc-cloexec.patch --]
[-- Type: text/plain, Size: 1345 bytes --]

diff -ruN xen-unstable-10712/tools/libxc/xc_linux.c xen-unstable-10712-cloexec/tools/libxc/xc_linux.c
--- xen-unstable-10712/tools/libxc/xc_linux.c	2006-07-21 13:31:22.000000000 -0400
+++ xen-unstable-10712-cloexec/tools/libxc/xc_linux.c	2006-08-14 20:24:15.000000000 -0400
@@ -13,13 +13,39 @@
 
 #include <xen/memory.h>
 #include <xen/sys/evtchn.h>
+#include <unistd.h>
+#include <fcntl.h>
 
 int xc_interface_open(void)
 {
+    int flags, saved_errno;
     int fd = open("/proc/xen/privcmd", O_RDWR);
-    if ( fd == -1 )
+    if ( fd == -1 ) {
         PERROR("Could not obtain handle on privileged command interface");
+        return -1;
+    }
+
+    /* Although we return the file handle as the 'xc handle' the API
+       does not specify / guarentee that this integer is in fact
+       a file handle. Thus we must take responsiblity to ensure
+       it doesn't propagate (ie leak) outside the process */
+    if ((flags = fcntl(fd, F_GETFD)) < 0) {
+        PERROR("Could not get file handle flags");
+        goto error;
+    }
+    flags |= FD_CLOEXEC;
+    if (fcntl(fd, F_SETFD, flags) < 0) {
+        PERROR("Could not set file handle flags");
+        goto error;
+    }
+
     return fd;
+
+ error:
+    saved_errno = errno;
+    close(fd);
+    errno = saved_errno;
+    return -1;
 }
 
 int xc_interface_close(int xc_handle)

[-- Attachment #3: xen-xend2-cloexec.patch --]
[-- Type: text/plain, Size: 2398 bytes --]

diff -ruN xen-unstable-10712/tools/python/xen/util/xmlrpclib2.py xen-unstable-10712-cloexec/tools/python/xen/util/xmlrpclib2.py
--- xen-unstable-10712/tools/python/xen/util/xmlrpclib2.py	2006-07-21 13:31:22.000000000 -0400
+++ xen-unstable-10712-cloexec/tools/python/xen/util/xmlrpclib2.py	2006-08-14 21:00:33.000000000 -0400
@@ -22,6 +22,7 @@
 
 import string
 import types
+import fcntl
 
 from httplib import HTTPConnection, HTTP
 from xmlrpclib import Transport
@@ -136,6 +137,17 @@
                  logRequests=1):
         SimpleXMLRPCServer.__init__(self, addr, requestHandler, logRequests)
 
+        flags = fcntl.fcntl(self.fileno(), fcntl.F_GETFD)
+        flags |= fcntl.FD_CLOEXEC
+        fcntl.fcntl(self.fileno(), fcntl.F_SETFD, flags)
+
+    def get_request(self):
+        (client, addr) = SimpleXMLRPCServer.get_request(self)
+        flags = fcntl.fcntl(client.fileno(), fcntl.F_GETFD)
+        flags |= fcntl.FD_CLOEXEC
+        fcntl.fcntl(client.fileno(), fcntl.F_SETFD, flags)
+        return (client, addr)
+                                                                                
     def _marshaled_dispatch(self, data, dispatch_method = None):
         params, method = xmlrpclib.loads(data)
         try:
diff -ruN xen-unstable-10712/tools/python/xen/web/httpserver.py xen-unstable-10712-cloexec/tools/python/xen/web/httpserver.py
--- xen-unstable-10712/tools/python/xen/web/httpserver.py	2006-07-21 13:31:22.000000000 -0400
+++ xen-unstable-10712-cloexec/tools/python/xen/web/httpserver.py	2006-08-14 21:00:54.000000000 -0400
@@ -24,6 +24,7 @@
 from urllib import quote, unquote
 import os
 import os.path
+import fcntl
 
 from xen.xend import sxp
 from xen.xend.Args import ArgError
@@ -294,6 +295,9 @@
 
     def bind(self):
         self.socket = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
+        flags = fcntl.fcntl(self.socket.fileno(), fcntl.F_GETFD)
+        flags |= fcntl.FD_CLOEXEC
+        fcntl.fcntl(self.socket.fileno(), fcntl.F_SETFD, flags)
         self.socket.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
         self.socket.bind((self.interface, self.port))
 
@@ -338,3 +342,6 @@
         
     def bind(self):
         self.socket = unix.bind(self.path)
+        flags = fcntl.fcntl(self.socket.fileno(), fcntl.F_GETFD)
+        flags |= fcntl.FD_CLOEXEC
+        fcntl.fcntl(self.socket.fileno(), fcntl.F_SETFD, flags)

[-- Attachment #4: xen-xs2-cloexec.patch --]
[-- Type: text/plain, Size: 1079 bytes --]

diff -ruN xen-unstable-10712/tools/xenstore/xs.c xen-unstable-10712-cloexec/tools/xenstore/xs.c
--- xen-unstable-10712/tools/xenstore/xs.c	2006-08-14 20:31:20.000000000 -0400
+++ xen-unstable-10712-cloexec/tools/xenstore/xs.c	2006-08-14 20:32:26.000000000 -0400
@@ -101,23 +101,31 @@
 static int get_socket(const char *connect_to)
 {
 	struct sockaddr_un addr;
-	int sock, saved_errno;
+	int sock, saved_errno, flags;
 
 	sock = socket(PF_UNIX, SOCK_STREAM, 0);
 	if (sock < 0)
 		return -1;
 
+	if ((flags = fcntl(sock, F_GETFD)) < 0)
+		goto error;
+	flags |= FD_CLOEXEC;
+	if (fcntl(sock, F_SETFD, flags) < 0)
+		goto error;
+
 	addr.sun_family = AF_UNIX;
 	strcpy(addr.sun_path, connect_to);
 
-	if (connect(sock, (struct sockaddr *)&addr, sizeof(addr)) != 0) {
-		saved_errno = errno;
-		close(sock);
-		errno = saved_errno;
-		return -1;
-	}
+	if (connect(sock, (struct sockaddr *)&addr, sizeof(addr)) != 0)
+		goto error;
 
 	return sock;
+
+error:
+	saved_errno = errno;
+	close(sock);
+	errno = saved_errno;
+	return -1;
 }
 
 static int get_dev(const char *connect_to)

[-- Attachment #5: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel