[Python-Dev] [Python-checkins] cpython (3.2): Fixes issue #8052: The posix subprocess module's close_fds behavior was (original) (raw)

Benjamin Peterson benjamin at python.org
Sun Jan 22 01:21:52 CET 2012

2012/1/21 gregory.p.smith <python-checkins at python.org>: ...

+/* Convert ASCII to a positive int, no libc call. no overflow. -1 on error. */

Is no libc call important?

+static int posintfromascii(char *name)

To be consistent with the rest of posixmodule.c, "static int" should be on a different line from the signature. This also applies to all other function declarations added by this.

+{ +    int num = 0; +    while (*name >= '0' && *name <= '9') { +        num = num * 10 + (*name - '0'); +        ++name; +    } +    if (*name) +        return -1;  /* Non digit found, not a number. */ +    return num; +} + + +/* Returns 1 if there is a problem with fdsequence, 0 otherwise. */ +static int sanitycheckpythonfdsequence(PyObject *fdsequence) +{ +    Pyssizet seqidx, seqlen = PySequenceLength(fdsequence);

PySequence_Length can fail.

+    long prevfd = -1; +    for (seqidx = 0; seqidx < seqlen; ++seqidx) {_ _+        PyObject* pyfd = PySequenceFastGETITEM(fdsequence, seqidx);_ _+        long iterfd = PyLongAsLong(pyfd);_ _+        if (iterfd < 0 || iterfd < prevfd || iterfd > INTMAX) { +            /* Negative, overflow, not a Long, unsorted, too big for a fd. */ +            return 1; +        } +    } +    return 0; +} + + +/* Is fd found in the sorted Python Sequence? */ +static int isfdinsortedfdsequence(int fd, PyObject *fdsequence) +{ +    /* Binary search. */ +    Pyssizet searchmin = 0; +    Pyssizet searchmax = PySequenceLength(fdsequence) - 1; +    if (searchmax < 0) +        return 0; +    do { +        long middle = (searchmin + searchmax) / 2; +        long middlefd = PyLongAsLong( +                PySequenceFastGETITEM(fdsequence, middle));

No check for error?

+        if (fd == middlefd) +            return 1; +        if (fd > middlefd) +            searchmin = middle + 1; +        else +            searchmax = middle - 1; +    } while (searchmin <= searchmax);_ _+    return 0;_ _+}_ _+_ _+_ _+/* Close all file descriptors in the range startfd inclusive to_ _+ * endfd exclusive except for those in pyfdstokeep.  If the_ _+ * range defined by [startfd, endfd) is large this will take a_ _+ * long time as it calls close() on EVERY possible fd._ _+ */_ _+static void closefdsbybruteforce(int startfd, int endfd,_ _+                                      PyObject *pyfdstokeep)_ _+{_ _+    Pyssizet numfdstokeep = PySequenceLength(pyfdstokeep);_ _+    Pyssizet keepseqidx;_ _+    int fdnum;_ _+    /* As pyfdstokeep is sorted we can loop through the list closing_ _+     * fds inbetween any in the keep list falling within our range. */_ _+    for (keepseqidx = 0; keepseqidx < numfdstokeep; ++keepseqidx) {_ _+        PyObject* pykeepfd = PySequenceFastGETITEM(pyfdstokeep,_ _+                                                        keepseqidx);_ _+        int keepfd = PyLongAsLong(pykeepfd);_ _+        if (keepfd < startfd)_ _+            continue;_ _+        for (fdnum = startfd; fdnum < keepfd; ++fdnum) {_ _+            while (close(fdnum) < 0 && errno == EINTR);_ _+        }_ _+        startfd = keepfd + 1;_ _+    }_ _+    if (startfd <= endfd) {_ _+        for (fdnum = startfd; fdnum < endfd; ++fdnum) {_ _+            while (close(fdnum) < 0 && errno == EINTR);_ _+        }_ _+    }_ _+}_ _+_ _+_ _+#if defined(_linux_) && defined(HAVESYSSYSCALLH)_ _+/* It doesn't matter if dname has room for NAMEMAX chars; we're using this_ _+ * only to read a directory of short file descriptor number names.  The kernel_ _+ * will return an error if we didn't give it enough space.  Highly Unlikely._ _+ * This structure is very old and stable: It will not change unless the kernel_ _+ * chooses to break compatibility with all existing binaries.  Highly Unlikely._ _+ */_ _+struct linuxdirent {_ _+   unsigned long  dino;        /* Inode number */_ _+   unsigned long  doff;        /* Offset to next linuxdirent */_ _+   unsigned short dreclen;     /* Length of this linuxdirent */_ _+   char           dname[256];  /* Filename (null-terminated) */_ _+};_ _+_ _+/* Close all open file descriptors in the range startfd inclusive to endfd_ _+ * exclusive. Do not close any in the sorted pyfdstokeep list._ _+ *_ _+ * This version is async signal safe as it does not make any unsafe C library_ _+ * calls, malloc calls or handle any locks.  It is unfortunate to be forced_ _+ * to resort to making a kernel system call directly but this is the ONLY api_ _+ * available that does no harm.  opendir/readdir/closedir perform memory_ _+ * allocation and locking so while they usually work they are not guaranteed_ _+ * to (especially if you have replaced your malloc implementation).  A version_ _+ * of this function that uses those can be found in the maybeunsafe variant._ _+ *_ _+ * This is Linux specific because that is all I am ready to test it on.  It_ _+ * should be easy to add OS specific dirent or dirent64 structures and modify_ _+ * it with some cpp #define magic to work on other OSes as well if you want._ _+ */_ _+static void closeopenfdrangesafe(int startfd, int endfd,_ _+                                      PyObject* pyfdstokeep)_ _+{_ _+    int fddirfd;_ _+    if (startfd >= endfd) +        return; +    fddirfd = open(LINUXSOLARISFDDIR, ORDONLY | OCLOEXEC, 0); +    /* Not trying to open the BSDOSX path as this is currently Linux only. */ +    if (fddirfd == -1) { +        /* No way to get a list of open fds. */ +        closefdsbybruteforce(startfd, endfd, pyfdstokeep); +        return; +    } else { +        char buffer[sizeof(struct linuxdirent)]; +        int bytes; +        while ((bytes = syscall(SYSgetdents, fddirfd, +                                (struct linuxdirent *)buffer, +                                sizeof(buffer))) > 0) { +            struct linuxdirent *entry; +            int offset; +            for (offset = 0; offset < bytes; offset += entry->dreclen) { +                int fd; +                entry = (struct linuxdirent *)(buffer + offset); +                if ((fd = posintfromascii(entry->dname)) < 0)_ _+                    continue;  /* Not a number. */_ _+                if (fd != fddirfd && fd >= startfd && fd < endfd &&_ _+                    !isfdinsortedfdsequence(fd, pyfdstokeep)) {_ _+                    while (close(fd) < 0 && errno == EINTR);_ _+                }_ _+            }_ _+        }_ _+        close(fddirfd);_ _+    }_ _+}_ _+_ _+#define closeopenfdrange closeopenfdrangesafe_ _+_ _+#else  /* NOT (defined(_linux_) && defined(HAVESYSSYSCALLH)) */_ _+_ _+_ _+/* Close all open file descriptors in the range startfd inclusive to endfd_ _+ * exclusive. Do not close any in the sorted pyfdstokeep list._ _+ *_ _+ * This function violates the strict use of async signal safe functions. :(_ _+ * It calls opendir(), readdir64() and closedir().  Of these, the one most_ _+ * likely to ever cause a problem is opendir() as it performs an internal_ _+ * malloc().  Practically this should not be a problem.  The Java VM makes the_ _+ * same calls between fork and exec in its own UNIXProcessmd.c implementation._ _+ *_ _+ * readdirr() is not used because it provides no benefit.  It is typically_ _+ * implemented as readdir() followed by memcpy().  See also:_ _+ *   http://womble.decadent.org.uk/readdirr-advisory.html_ _+ */_ _+static void closeopenfdrangemaybeunsafe(int startfd, int endfd,_ _+                                              PyObject* pyfdstokeep)_ _+{_ _+    DIR *procfddir;_ _+#ifndef HAVEDIRFD_ _+    while (isfdinsortedfdsequence(startfd, pyfdstokeep) &&_ _+           (startfd < endfd)) {_ _+        ++startfd;_ _+    }_ _+    if (startfd >= endfd) +        return; +    /* Close our lowest fd before we call opendir so that it is likely to +     * reuse that fd otherwise we might close opendir's file descriptor in +     * our loop.  This trick assumes that fd's are allocated on a lowest +     * available basis. */ +    while (close(startfd) < 0 && errno == EINTR);_ _+    ++startfd;_ _+#endif_ _+    if (startfd >= endfd) +        return; + +    procfddir = opendir(BSDOSXFDDIR); +    if (!procfddir) +        procfddir = opendir(LINUXSOLARISFDDIR); +    if (!procfddir) { +        /* No way to get a list of open fds. */ +        closefdsbybruteforce(startfd, endfd, pyfdstokeep); +    } else { +        struct dirent64 *direntry; +#ifdef HAVEDIRFD +        int fdusedbyopendir = DIRFD(procfddir); +#else +        int fdusedbyopendir = startfd - 1; +#endif +        errno = 0; +        /* readdir64 is used to work around Solaris 9 bug 6395699. */ +        while ((direntry = readdir64(procfddir))) { +            int fd; +            if ((fd = posintfromascii(direntry->dname)) < 0)_ _+                continue;  /* Not a number. */_ _+            if (fd != fdusedbyopendir && fd >= startfd && fd < endfd && +                !isfdinsortedfdsequence(fd, pyfdstokeep)) { +                while (close(fd) < 0 && errno == EINTR); +            } +            errno = 0; +        } +        if (errno) { +            /* readdir error, revert behavior. Highly Unlikely. */ +            closefdsbybruteforce(startfd, endfd, pyfdstokeep); +        } +        closedir(procfddir); +    } +} + +#define closeopenfdrange closeopenfdrangemaybeunsafe + +#endif  /* else NOT (defined(linux) && defined(HAVESYSSYSCALLH)) */ + +  /*  * This function is code executed in the child process immediately after fork  * to set things up and call exec(). @@ -46,12 +292,12 @@  int errread, int errwrite,  int errpiperead, int errpipewrite,  int closefds, int restoresignals, -                       int callsetsid, Pyssizet numfdstokeep, +                       int callsetsid,  PyObject *pyfdstokeep,  PyObject *preexecfn,  PyObject *preexecfnargstuple)  { -    int i, savederrno, fdnum, unused; +    int i, savederrno, unused; PyObject *result; const char* errmsg = ""; /* Buffer large enough to hold a hex integer.  We can't malloc. */ @@ -113,33 +359,8 @@ POSIXCALL(close(errwrite)); }

-    /* close() is intentionally not checked for errors here as we are closing */ -    /* a large range of fds, some of which may be invalid. */ -    if (closefds) { -        Pyssizet keepseqidx; -        int startfd = 3; -        for (keepseqidx = 0; keepseqidx < numfdstokeep; ++keepseqidx) {_ _-            PyObject* pykeepfd = PySequenceFastGETITEM(pyfdstokeep,_ _-                                                            keepseqidx);_ _-            int keepfd = PyLongAsLong(pykeepfd);_ _-            if (keepfd < 0) {  /* Negative number, overflow or not a Long. */_ _-                errmsg = "bad value in fdstokeep.";_ _-                errno = 0;  /* We don't want to report an OSError. */_ _-                goto error;_ _-            }_ _-            if (keepfd < startfd)_ _-                continue;_ _-            for (fdnum = startfd; fdnum < keepfd; ++fdnum) {_ _-                close(fdnum);_ _-            }_ _-            startfd = keepfd + 1;_ _-        }_ _-        if (startfd <= maxfd) {_ _-            for (fdnum = startfd; fdnum < maxfd; ++fdnum) {_ _-                close(fdnum);_ _-            }_ _-        }_ _-    }_ _+    if (closefds)_ _+        closeopenfdrange(3, maxfd, pyfdstokeep);_ _if (cwd)_ _POSIXCALL(chdir(cwd));_ _@@ -227,7 +448,7 @@_ _pidt pid;_ _int needtoreenablegc = 0;_ _char *const *execarray, *const *argv = NULL, *const *envp = NULL;_ _-    Pyssizet argnum, numfdstokeep;_ _+    Pyssizet argnum;_ _if (!PyArgParseTuple(_ _args, "OOOOOOiiiiiiiiiiO:forkexec",_ _@@ -243,9 +464,12 @@_ _PyErrSetString(PyExcValueError, "errpipewrite must be >= 3"); return NULL; } -    numfdstokeep = PySequenceLength(pyfdstokeep); -    if (numfdstokeep < 0) {_ _-        PyErrSetString(PyExcValueError, "bad fdstokeep");_ _+    if (PySequenceLength(pyfdstokeep) < 0) {_ _+        PyErrSetString(PyExcValueError, "cannot get length of fdstokeep");_ _+        return NULL;_ _+    }_ _+    if (sanitycheckpythonfdsequence(pyfdstokeep)) {_ _+        PyErrSetString(PyExcValueError, "bad value(s) in fdstokeep");_ _return NULL;_ _}_ _@@ -348,8 +572,7 @@_  _p2cread, p2cwrite, c2pread, c2pwrite,_  _errread, errwrite, errpiperead, errpipewrite,_  _closefds, restoresignals, callsetsid,_ _-                   numfdstokeep, pyfdstokeep,_ _-                   preexecfn, preexecfnargstuple);_ _+                   pyfdstokeep, preexecfn, preexecfnargstuple);_ _exit(255);_ _return NULL;  /* Dead code to avoid a potential compiler warning. */_ _}_ _diff --git a/configure b/configure_ _--- a/configure_ _+++ b/configure_ _@@ -6165,7 +6165,7 @@_  _sys/audioio.h sys/bsdtty.h sys/epoll.h sys/event.h sys/file.h sys/loadavg.h _  _sys/lock.h sys/mkdev.h sys/modem.h _  _sys/param.h sys/poll.h sys/select.h sys/socket.h sys/statvfs.h sys/stat.h _ _-sys/termio.h sys/time.h _ _+sys/syscall.h sys/termio.h sys/time.h _  _sys/times.h sys/types.h sys/un.h sys/utsname.h sys/wait.h pty.h libutil.h _  _sys/resource.h netpacket/packet.h sysexits.h bluetooth.h _  _bluetooth/bluetooth.h linux/tipc.h spawn.h util.h_ _diff --git a/configure.in b/configure.in_ _--- a/configure.in_ _+++ b/configure.in_ _@@ -1341,7 +1341,7 @@_  _sys/audioio.h sys/bsdtty.h sys/epoll.h sys/event.h sys/file.h sys/loadavg.h _  _sys/lock.h sys/mkdev.h sys/modem.h _  _sys/param.h sys/poll.h sys/select.h sys/socket.h sys/statvfs.h sys/stat.h _ _-sys/termio.h sys/time.h _ _+sys/syscall.h sys/termio.h sys/time.h _  _sys/times.h sys/types.h sys/un.h sys/utsname.h sys/wait.h pty.h libutil.h _  _sys/resource.h netpacket/packet.h sysexits.h bluetooth.h _  _bluetooth/bluetooth.h linux/tipc.h spawn.h util.h)_ _diff --git a/pyconfig.h.in b/pyconfig.h.in_ _--- a/pyconfig.h.in_ _+++ b/pyconfig.h.in_ _@@ -789,6 +789,9 @@_  _/* Define to 1 if you have the <sys/stat.h> header file. */  #undef HAVESYSSTATH +/* Define to 1 if you have the <sys/syscall.h> header file. */ +#undef HAVESYSSYSCALLH +  /* Define to 1 if you have the <sys/termio.h> header file. */  #undef HAVESYSTERMIOH

-- Regards, Benjamin

More information about the Python-Dev mailing list