From 9b5d91185bbbc73ee8a01c35be3549493f0dd87a Mon Sep 17 00:00:00 2001 From: Nir Soffer Date: Thu, 30 Nov 2017 19:03:55 +0200 Subject: [PATCH 1/3] bpo-32186: Release the GIL during fstat and lseek calls In fileio, there were 3 fstat() calls and one lseek() call that did not release the GIL during the call. This can cause all threads to hang for unlimited time when using io.FileIO with inaccessible NFS server. --- Modules/_io/fileio.c | 55 +++++++++++++++++++++++++++++++++++--------- 1 file changed, 44 insertions(+), 11 deletions(-) diff --git a/Modules/_io/fileio.c b/Modules/_io/fileio.c index 4a71a57ec0dea4c..2b40ada195a1f5f 100644 --- a/Modules/_io/fileio.c +++ b/Modules/_io/fileio.c @@ -146,9 +146,15 @@ dircheck(fileio* self, PyObject *nameobj) { #if defined(HAVE_FSTAT) && defined(S_IFDIR) && defined(EISDIR) struct stat buf; + int res; if (self->fd < 0) return 0; - if (fstat(self->fd, &buf) == 0 && S_ISDIR(buf.st_mode)) { + + Py_BEGIN_ALLOW_THREADS + res = fstat(self->fd, &buf); + Py_END_ALLOW_THREADS + + if (res == 0 && S_ISDIR(buf.st_mode)) { errno = EISDIR; PyErr_SetFromErrnoWithFilenameObject(PyExc_IOError, nameobj); return -1; @@ -162,17 +168,34 @@ check_fd(int fd) { #if defined(HAVE_FSTAT) struct stat buf; - if (!_PyVerify_fd(fd) || (fstat(fd, &buf) < 0 && errno == EBADF)) { - PyObject *exc; - char *msg = strerror(EBADF); - exc = PyObject_CallFunction(PyExc_OSError, "(is)", - EBADF, msg); - PyErr_SetObject(PyExc_OSError, exc); - Py_XDECREF(exc); - return -1; + int res; + PyObject *exc; + char *msg; + + if (!_PyVerify_fd(fd)) { + goto badfd; } -#endif + + Py_BEGIN_ALLOW_THREADS + res = fstat(fd, &buf); + Py_END_ALLOW_THREADS + + if (res < 0 && errno == EBADF) { + goto badfd; + } + return 0; + +badfd: + msg = strerror(EBADF); + exc = PyObject_CallFunction(PyExc_OSError, "(is)", + EBADF, msg); + PyErr_SetObject(PyExc_OSError, exc); + Py_XDECREF(exc); + return -1; +#else + return 0; +#endif } @@ -519,9 +542,19 @@ new_buffersize(fileio *self, size_t currentsize) #ifdef HAVE_FSTAT off_t pos, end; struct stat st; - if (fstat(self->fd, &st) == 0) { + int res; + + Py_BEGIN_ALLOW_THREADS + res = fstat(self->fd, &st); + Py_END_ALLOW_THREADS + + if (res == 0) { end = st.st_size; + + Py_BEGIN_ALLOW_THREADS pos = lseek(self->fd, 0L, SEEK_CUR); + Py_END_ALLOW_THREADS + /* Files claiming a size smaller than SMALLCHUNK may actually be streaming pseudo-files. In this case, we apply the more aggressive algorithm below. From 5b8bc4f1e22b32b5bad4043e13303d1b99305a91 Mon Sep 17 00:00:00 2001 From: Nir Soffer Date: Thu, 30 Nov 2017 20:35:28 +0200 Subject: [PATCH 2/3] Add news entry --- .../next/Library/2017-11-30-20-33-22.bpo-32186.O42bVe.rst | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2017-11-30-20-33-22.bpo-32186.O42bVe.rst diff --git a/Misc/NEWS.d/next/Library/2017-11-30-20-33-22.bpo-32186.O42bVe.rst b/Misc/NEWS.d/next/Library/2017-11-30-20-33-22.bpo-32186.O42bVe.rst new file mode 100644 index 000000000000000..98fe3c4863387fe --- /dev/null +++ b/Misc/NEWS.d/next/Library/2017-11-30-20-33-22.bpo-32186.O42bVe.rst @@ -0,0 +1,4 @@ +Creating io.FileIO() object now releases the GIL when checking the file +descriptor. io.FileIO.readall() and io.FileIO.read() now release the GIL +when getting the file size. Fixed hang of all threads with inaccessible +NFS server. Patch by Nir Soffer. From bebc7bc065175ee1d4a912c26da1599afc07d348 Mon Sep 17 00:00:00 2001 From: Nir Soffer Date: Thu, 7 Dec 2017 18:11:13 +0200 Subject: [PATCH 3/3] Fix fstat and lseek calls in fileobject Same issue seen in fileio exists also in fileobject, fixed in the same way. --- .../2017-11-30-20-33-22.bpo-32186.O42bVe.rst | 8 +++---- Objects/fileobject.c | 21 ++++++++++++++++--- 2 files changed, 22 insertions(+), 7 deletions(-) diff --git a/Misc/NEWS.d/next/Library/2017-11-30-20-33-22.bpo-32186.O42bVe.rst b/Misc/NEWS.d/next/Library/2017-11-30-20-33-22.bpo-32186.O42bVe.rst index 98fe3c4863387fe..66c4468e80c0b64 100644 --- a/Misc/NEWS.d/next/Library/2017-11-30-20-33-22.bpo-32186.O42bVe.rst +++ b/Misc/NEWS.d/next/Library/2017-11-30-20-33-22.bpo-32186.O42bVe.rst @@ -1,4 +1,4 @@ -Creating io.FileIO() object now releases the GIL when checking the file -descriptor. io.FileIO.readall() and io.FileIO.read() now release the GIL -when getting the file size. Fixed hang of all threads with inaccessible -NFS server. Patch by Nir Soffer. +Creating io.FileIO() and builtin file() objects now release the GIL when +checking the file descriptor. io.FileIO.readall(), io.FileIO.read(), and +file.read() now release the GIL when getting the file size. Fixed hang of all +threads with inaccessible NFS server. Patch by Nir Soffer. diff --git a/Objects/fileobject.c b/Objects/fileobject.c index 2f63c374d1e2421..8d1c5812f0d213b 100644 --- a/Objects/fileobject.c +++ b/Objects/fileobject.c @@ -121,10 +121,15 @@ dircheck(PyFileObject* f) { #if defined(HAVE_FSTAT) && defined(S_IFDIR) && defined(EISDIR) struct stat buf; + int res; if (f->f_fp == NULL) return f; - if (fstat(fileno(f->f_fp), &buf) == 0 && - S_ISDIR(buf.st_mode)) { + + Py_BEGIN_ALLOW_THREADS + res = fstat(fileno(f->f_fp), &buf); + Py_END_ALLOW_THREADS + + if (res == 0 && S_ISDIR(buf.st_mode)) { char *msg = strerror(EISDIR); PyObject *exc = PyObject_CallFunction(PyExc_IOError, "(isO)", EISDIR, msg, f->f_name); @@ -1010,7 +1015,13 @@ new_buffersize(PyFileObject *f, size_t currentsize) #ifdef HAVE_FSTAT off_t pos, end; struct stat st; - if (fstat(fileno(f->f_fp), &st) == 0) { + int res; + + Py_BEGIN_ALLOW_THREADS + res = fstat(fileno(f->f_fp), &st); + Py_END_ALLOW_THREADS + + if (res == 0) { end = st.st_size; /* The following is not a bug: we really need to call lseek() *and* ftell(). The reason is that some stdio libraries @@ -1021,7 +1032,11 @@ new_buffersize(PyFileObject *f, size_t currentsize) works. We can't use the lseek() value either, because we need to take the amount of buffered data into account. (Yet another reason why stdio stinks. :-) */ + + Py_BEGIN_ALLOW_THREADS pos = lseek(fileno(f->f_fp), 0L, SEEK_CUR); + Py_END_ALLOW_THREADS + if (pos >= 0) { pos = ftell(f->f_fp); }