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

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

[Daniel P. Berrange]

[-- 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

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

[Keir Fraser]

On 15/8/06 2:23 am, "Daniel P. Berrange" <berrange@redhat.com> wrote:

> 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.

The patches look okay to me. Please re-send with a signed-off-by line.

 -- Keir

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

[Daniel P. Berrange]

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

On Tue, Aug 15, 2006 at 10:53:01AM +0100, Keir Fraser wrote:
> 
> On 15/8/06 2:23 am, "Daniel P. Berrange" <berrange@redhat.com> wrote:
> 
> > 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.
> 
> The patches look okay to me. Please re-send with a signed-off-by line.

I'm also attaching one extra patch 'xen-xend-logging-cloexec.patch' which
sets the FD_CLOEXEC flag on the /var/log/xend.log  file. I'm not entirely
happy with this patch though because it accesses the private 'self.stream'
field in its superclass. Unfortunately the entire python logging class
hierarchy is 'designed'  on the principle of accessing  private class
members from superclasses, so I don't see any immediately obvious alternate
way to set FD_CLOEXEC on the log file.


A much more invasive patch to XenD would be to locate all places where we
call fork / exec and in between the forking & execing iterate over all
file handles explicitly setting FD_CLOEXEC, eg the equiv of this C code,
but in python

     pid = fork()
     if (pid == 0) {
        open_max = sysconf (_SC_OPEN_MAX);
        for (i = 0; i < open_max; i++)
            fcntl (i, F_SETFD, FD_CLOEXEC);

        exec(...)
     }

We'd also need to find all places where we call 'spawn' and replace this call
with a fork/exec pair.

Attached the 3 previous patches & the new one to this mail. I've tested that
they apply without trouble to latest xen-unstable.hg

  Signed-off-by: Daniel P. Berrange <berrange@redhat.com>

Regards
Dan.
-- 
|=- 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-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 #4: 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 #5: xen-xend-logging-cloexec.patch --]
[-- Type: text/plain, Size: 2248 bytes --]

diff -ruN xen-unstable-10712/tools/python/xen/xend/XendLogging.py xen-unstable-10712-cloexec/tools/python/xen/xend/XendLogging.py
--- xen-unstable-10712/tools/python/xen/xend/XendLogging.py	2006-07-21 13:31:22.000000000 -0400
+++ xen-unstable-10712-cloexec/tools/python/xen/xend/XendLogging.py	2006-08-15 09:19:09.000000000 -0400
@@ -21,6 +21,7 @@
 import types
 import logging
 import logging.handlers
+import fcntl
 
 from xen.xend.server import params
 
@@ -49,6 +50,27 @@
 
 logfilename = None
 
+class XendRotatingFileHandler(logging.handlers.RotatingFileHandler):
+
+    def __init__(self, fname, mode, maxBytes, backupCount):
+        logging.handlers.RotatingFileHandler.__init__(self, fname, mode, maxBytes, backupCount)
+        self.setCloseOnExec()
+
+    def doRollover(self):
+        logging.handlers.RotatingFileHandler.doRollover()
+        self.setCloseOnExec()
+
+    # NB yes accessing 'self.stream' violates OO encapsulation somewhat,
+    # but python logging API gives no other way to access the file handle
+    # and the entire python logging stack is already full of OO encapsulation
+    # violations. The other alternative is copy-and-paste duplicating the
+    # entire FileHandler, StreamHandler & RotatingFileHandler classes which
+    # is even worse
+    def setCloseOnExec(self):
+        flags = fcntl.fcntl(self.stream.fileno(), fcntl.F_GETFD)
+        flags |= fcntl.FD_CLOEXEC
+        fcntl.fcntl(self.stream.fileno(), fcntl.F_SETFD, flags)
+        
 
 def init(filename, level):
     """Initialise logging.  Logs to the given filename, and logs to stderr if
@@ -58,9 +80,9 @@
     global logfilename
 
     def openFileHandler(fname):
-        return logging.handlers.RotatingFileHandler(fname, mode = 'a',
-                                                    maxBytes = MAX_BYTES,
-                                                    backupCount = BACKUP_COUNT)
+        return XendRotatingFileHandler(fname, mode = 'a',
+                                       maxBytes = MAX_BYTES,
+                                       backupCount = BACKUP_COUNT)
 
     # Rather unintuitively, getLevelName will get the number corresponding to
     # a level name, as well as getting the name corresponding to a level

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

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