From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Daniel P. Berrange" Subject: Re: [PATCH] Ensure FD_CLOEXEC is set on all XenD file handles Date: Tue, 15 Aug 2006 14:43:30 +0100 Message-ID: <20060815134330.GA7399@redhat.com> References: <20060815012357.GA28193@redhat.com> Reply-To: "Daniel P. Berrange" Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="AqsLC8rIMeq19msA" Return-path: Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Keir Fraser Cc: xen-devel@lists.xensource.com List-Id: xen-devel@lists.xenproject.org --AqsLC8rIMeq19msA Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Tue, Aug 15, 2006 at 10:53:01AM +0100, Keir Fraser wrote: > > On 15/8/06 2:23 am, "Daniel P. Berrange" 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 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 -=| --AqsLC8rIMeq19msA Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="xen-xc-cloexec.patch" 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 #include +#include +#include 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) --AqsLC8rIMeq19msA Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="xen-xs2-cloexec.patch" 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) --AqsLC8rIMeq19msA Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="xen-xend2-cloexec.patch" 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) --AqsLC8rIMeq19msA Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="xen-xend-logging-cloexec.patch" 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 --AqsLC8rIMeq19msA Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel --AqsLC8rIMeq19msA--