Issue python#23694: Enhance _Py_open(), it now raises exceptions

vstinner · vstinner · commit a555cfcb73cf · 2015-03-18T00:22:14.000+01:00
* _Py_open() now raises exceptions on error. If open() fails, it raises an
  OSError with the filename.
* _Py_open() now releases the GIL while calling open()
* Add _Py_open_noraise() when _Py_open() cannot be used because the GIL is not
  held
diff --git a/Include/fileutils.h b/Include/fileutils.h
@@ -62,6 +62,10 @@ PyAPI_FUNC(int) _Py_stat(
 PyAPI_FUNC(int) _Py_open(
     const char *pathname,
     int flags);
+
+PyAPI_FUNC(int) _Py_open_noraise(
+    const char *pathname,
+    int flags);
 #endif
 
 PyAPI_FUNC(FILE *) _Py_wfopen(
diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c
@@ -257,6 +257,7 @@ _close_open_fds_safe(int start_fd, PyObject* py_fds_to_keep)
     fd_dir_fd = _Py_open(FD_DIR, O_RDONLY);
     if (fd_dir_fd == -1) {
         /* No way to get a list of open fds. */
+        PyErr_Clear();
         _close_fds_by_brute_force(start_fd, py_fds_to_keep);
         return;
     } else {
diff --git a/Modules/mmapmodule.c b/Modules/mmapmodule.c
@@ -1221,7 +1221,6 @@ new_mmap_object(PyTypeObject *type, PyObject *args, PyObject *kwdict)
         fd = devzero = _Py_open("/dev/zero", O_RDWR);
         if (devzero == -1) {
             Py_DECREF(m_obj);
-            PyErr_SetFromErrno(PyExc_OSError);
             return NULL;
         }
 #endif
diff --git a/Modules/ossaudiodev.c b/Modules/ossaudiodev.c
@@ -116,11 +116,8 @@ newossobject(PyObject *arg)
        provides a special ioctl() for non-blocking read/write, which is
        exposed via oss_nonblock() below. */
     fd = _Py_open(devicename, imode|O_NONBLOCK);
-
-    if (fd == -1) {
-        PyErr_SetFromErrnoWithFilename(PyExc_IOError, devicename);
+    if (fd == -1)
         return NULL;
-    }
 
     /* And (try to) put it back in blocking mode so we get the
        expected write() semantics. */
@@ -180,10 +177,8 @@ newossmixerobject(PyObject *arg)
     }
 
     fd = _Py_open(devicename, O_RDWR);
-    if (fd == -1) {
-        PyErr_SetFromErrnoWithFilename(PyExc_IOError, devicename);
+    if (fd == -1)
         return NULL;
-    }
 
     if ((self = PyObject_New(oss_mixer_t, &OSSMixerType)) == NULL) {
         close(fd);
diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c
@@ -7930,7 +7930,7 @@ os_openpty_impl(PyModuleDef *module)
 
     slave_fd = _Py_open(slave_name, O_RDWR);
     if (slave_fd < 0)
-        goto posix_error;
+        goto error;
 
 #else
     master_fd = open(DEV_PTY_FILE, O_RDWR | O_NOCTTY); /* open master */
@@ -7958,8 +7958,8 @@ os_openpty_impl(PyModuleDef *module)
         goto posix_error;
 
     slave_fd = _Py_open(slave_name, O_RDWR | O_NOCTTY); /* open slave */
-    if (slave_fd < 0)
-        goto posix_error;
+    if (slave_fd == -1)
+        goto error;
 
     if (_Py_set_inheritable(master_fd, 0, NULL) < 0)
         goto posix_error;
@@ -7977,9 +7977,7 @@ os_openpty_impl(PyModuleDef *module)
 
 posix_error:
     posix_error();
-#if defined(HAVE_OPENPTY) || defined(HAVE__GETPTY)
 error:
-#endif
     if (master_fd != -1)
         close(master_fd);
     if (slave_fd != -1)
diff --git a/Modules/selectmodule.c b/Modules/selectmodule.c
@@ -1013,26 +1013,21 @@ newDevPollObject(void)
     struct pollfd *fds;
     struct rlimit limit;
 
-    Py_BEGIN_ALLOW_THREADS
     /*
     ** If we try to process more that getrlimit()
     ** fds, the kernel will give an error, so
     ** we set the limit here. It is a dynamic
     ** value, because we can change rlimit() anytime.
     */
     limit_result = getrlimit(RLIMIT_NOFILE, &limit);
-    if (limit_result != -1)
-        fd_devpoll = _Py_open("/dev/poll", O_RDWR);
-    Py_END_ALLOW_THREADS
-
     if (limit_result == -1) {
         PyErr_SetFromErrno(PyExc_OSError);
         return NULL;
     }
-    if (fd_devpoll == -1) {
-        PyErr_SetFromErrnoWithFilename(PyExc_IOError, "/dev/poll");
+
+    fd_devpoll = _Py_open("/dev/poll", O_RDWR);
+    if (fd_devpoll == -1)
         return NULL;
-    }
 
     fds = PyMem_NEW(struct pollfd, limit.rlim_cur);
     if (fds == NULL) {
diff --git a/Python/fileutils.c b/Python/fileutils.c
@@ -30,7 +30,8 @@ extern wchar_t* _Py_DecodeUTF8_surrogateescape(const char *s, Py_ssize_t size);
     0: open() ignores O_CLOEXEC flag, ex: Linux kernel older than 2.6.23
     1: open() supports O_CLOEXEC flag, close-on-exec is set
 
-   The flag is used by _Py_open(), io.FileIO and os.open() */
+   The flag is used by _Py_open(), _Py_open_noraise(), io.FileIO
+   and os.open(). */
 int _Py_open_cloexec_works = -1;
 #endif
 
@@ -907,37 +908,74 @@ _Py_set_inheritable(int fd, int inheritable, int *atomic_flag_works)
     return set_inheritable(fd, inheritable, 1, atomic_flag_works);
 }
 
-/* Open a file with the specified flags (wrapper to open() function).
-   The file descriptor is created non-inheritable. */
-int
-_Py_open(const char *pathname, int flags)
+static int
+_Py_open_impl(const char *pathname, int flags, int gil_held)
 {
     int fd;
-#ifdef MS_WINDOWS
-    fd = open(pathname, flags | O_NOINHERIT);
-    if (fd < 0)
-        return fd;
-#else
-
+#ifndef MS_WINDOWS
     int *atomic_flag_works;
-#ifdef O_CLOEXEC
+#endif
+
+#ifdef MS_WINDOWS
+    flags |= O_NOINHERIT;
+#elif defined(O_CLOEXEC)
     atomic_flag_works = &_Py_open_cloexec_works;
     flags |= O_CLOEXEC;
 #else
     atomic_flag_works = NULL;
 #endif
-    fd = open(pathname, flags);
-    if (fd < 0)
-        return fd;
 
-    if (set_inheritable(fd, 0, 0, atomic_flag_works) < 0) {
+    if (gil_held) {
+        Py_BEGIN_ALLOW_THREADS
+        fd = open(pathname, flags);
+        Py_END_ALLOW_THREADS
+
+        if (fd < 0) {
+            PyErr_SetFromErrnoWithFilename(PyExc_OSError, pathname);
+            return -1;
+        }
+    }
+    else {
+        fd = open(pathname, flags);
+        if (fd < 0)
+            return -1;
+    }
+
+#ifndef MS_WINDOWS
+    if (set_inheritable(fd, 0, gil_held, atomic_flag_works) < 0) {
         close(fd);
         return -1;
     }
-#endif   /* !MS_WINDOWS */
+#endif
+
     return fd;
 }
 
+/* Open a file with the specified flags (wrapper to open() function).
+   Return a file descriptor on success. Raise an exception and return -1 on
+   error.
+
+   The file descriptor is created non-inheritable.
+
+   The GIL must be held. Use _Py_open_noraise() if the GIL cannot be held. */
+int
+_Py_open(const char *pathname, int flags)
+{
+    /* _Py_open() must be called with the GIL held. */
+    assert(PyGILState_Check());
+    return _Py_open_impl(pathname, flags, 1);
+}
+
+/* Open a file with the specified flags (wrapper to open() function).
+   Return a file descriptor on success. Set errno and return -1 on error.
+
+   The file descriptor is created non-inheritable. */
+int
+_Py_open_noraise(const char *pathname, int flags)
+{
+    return _Py_open_impl(pathname, flags, 0);
+}
+
 /* Open a file. Use _wfopen() on Windows, encode the path to the locale
    encoding and use fopen() otherwise. The file descriptor is created
    non-inheritable. */
diff --git a/Python/random.c b/Python/random.c
@@ -111,7 +111,7 @@ dev_urandom_noraise(unsigned char *buffer, Py_ssize_t size)
 
     assert (0 < size);
 
-    fd = _Py_open("/dev/urandom", O_RDONLY);
+    fd = _Py_open_noraise("/dev/urandom", O_RDONLY);
     if (fd < 0)
         Py_FatalError("Failed to open /dev/urandom");
 
@@ -158,17 +158,13 @@ dev_urandom_python(char *buffer, Py_ssize_t size)
     if (urandom_cache.fd >= 0)
         fd = urandom_cache.fd;
     else {
-        Py_BEGIN_ALLOW_THREADS
         fd = _Py_open("/dev/urandom", O_RDONLY);
-        Py_END_ALLOW_THREADS
-        if (fd < 0)
-        {
+        if (fd < 0) {
             if (errno == ENOENT || errno == ENXIO ||
                 errno == ENODEV || errno == EACCES)
                 PyErr_SetString(PyExc_NotImplementedError,
                                 "/dev/urandom (or equivalent) not found");
-            else
-                PyErr_SetFromErrno(PyExc_OSError);
+            /* otherwise, keep the OSError exception raised by _Py_open() */
             return -1;
         }
         if (urandom_cache.fd >= 0) {

Original file line number	Diff line number	Diff line change
`@@ -1221,7 +1221,6 @@ new_mmap_object(PyTypeObject type, PyObject args, PyObject *kwdict)`
`1221`	`1221`	`fd = devzero = _Py_open("/dev/zero", O_RDWR);`
`1222`	`1222`	`if (devzero == -1) {`
`1223`	`1223`	`Py_DECREF(m_obj);`
`1224`		`- PyErr_SetFromErrno(PyExc_OSError);`
`1225`	`1224`	`return NULL;`
`1226`	`1225`	`}`
`1227`	`1226`	`#endif`