Bug [ 959379 ] Implicit close() should check for errors

I'd like to resolve bug 959379. Here's a naive fix. Comments? diff -u -r2.192 fileobject.c --- Objects/fileobject.c 11 Jun 2004 04:49:03 -0000 2.192 +++ Objects/fileobject.c 24 Oct 2004 20:58:49 -0000 @@ -304,7 +304,8 @@ PyObject_ClearWeakRefs((PyObject *) f); if (f->f_fp != NULL && f->f_close != NULL) { Py_BEGIN_ALLOW_THREADS - (*f->f_close)(f->f_fp); + if ((*f->f_close)(f->f_fp) != 0) + perror("close failed"); Py_END_ALLOW_THREADS } PyMem_Free(f->f_setbuf); /Peter Åstrand <astrand@lysator.liu.se>

Peter Astrand <astrand@lysator.liu.se> writes:
I'd like to resolve bug 959379. Here's a naive fix. Comments?
If you're going to match the normal close() processing, shouldn't that check the result against EOF and not just != 0? If for example, the file object is being used for a pipe, the result of the close might be the result code of the child process which need not be an error. I think that only EOF should signify a failure of the close(), and only in that case is errno probably appropriate (which perror will pick up). -- David

On Mon, 25 Oct 2004, David Bolen wrote:
Yes. Thanks. Here's the new patch. OK to commit? diff -u -r2.192 fileobject.c --- fileobject.c 11 Jun 2004 04:49:03 -0000 2.192 +++ fileobject.c 26 Oct 2004 05:50:12 -0000 @@ -304,7 +304,8 @@ PyObject_ClearWeakRefs((PyObject *) f); if (f->f_fp != NULL && f->f_close != NULL) { Py_BEGIN_ALLOW_THREADS - (*f->f_close)(f->f_fp); + if ((*f->f_close)(f->f_fp) == EOF) + perror("close failed"); Py_END_ALLOW_THREADS } PyMem_Free(f->f_setbuf); /Peter Åstrand <astrand@lysator.liu.se>

Peter Astrand wrote:
Yes. Thanks. Here's the new patch. OK to commit?
No. I feel that perror is inappropriate; PySys_WriteStderr might be slightly better. However, it might be that even raising an exception has some value. Regards, Martin

On Tue, 26 Oct 2004, "Martin v. Löwis" wrote:
Here's a patch with PySys_WriteStderr: diff -u -r2.192 fileobject.c --- Objects/fileobject.c 11 Jun 2004 04:49:03 -0000 2.192 +++ Objects/fileobject.c 26 Oct 2004 18:44:19 -0000 @@ -300,12 +300,15 @@ static void file_dealloc(PyFileObject *f) { + int sts = 0; if (f->weakreflist != NULL) PyObject_ClearWeakRefs((PyObject *) f); if (f->f_fp != NULL && f->f_close != NULL) { Py_BEGIN_ALLOW_THREADS - (*f->f_close)(f->f_fp); + sts = (*f->f_close)(f->f_fp); Py_END_ALLOW_THREADS + if (sts == EOF) + PySys_WriteStderr("close failed: %s\n", strerror(errno)); } PyMem_Free(f->f_setbuf); Py_XDECREF(f->f_name);
However, it might be that even raising an exception has some value.
Is it really possible to raise an exception in response to something triggered by the GC? /Peter Åstrand <astrand@lysator.liu.se>

[Peter Astrand] ...
Is it really possible to raise an exception in response to something triggered by the GC?
It's possible but wholly useless. If you try, gc will detect that an exception was raised, and kill the Python process ungracefully, via Py_FatalError("unexpected exception during garbage collection"); You don't want to do that <wink>.

On Tue, 26 Oct 2004, "Martin v. Löwis" wrote:
Like this?: diff -u -r2.192 fileobject.c --- dist/src/Objects/fileobject.c 11 Jun 2004 04:49:03 -0000 2.192 +++ dist/src/Objects/fileobject.c 7 Nov 2004 10:13:04 -0000 @@ -300,12 +300,19 @@ static void file_dealloc(PyFileObject *f) { + int sts = 0; if (f->weakreflist != NULL) PyObject_ClearWeakRefs((PyObject *) f); if (f->f_fp != NULL && f->f_close != NULL) { Py_BEGIN_ALLOW_THREADS - (*f->f_close)(f->f_fp); + sts = (*f->f_close)(f->f_fp); Py_END_ALLOW_THREADS + if (sts == EOF) +#ifdef HAVE_STRERROR + PySys_WriteStderr("close failed: [Errno %d] %s\n", errno, strerror(errno)); +#else + PySys_WriteStderr("close failed: [Errno %d]\n", errno); +#endif } PyMem_Free(f->f_setbuf); Py_XDECREF(f->f_name); /Peter Åstrand <astrand@lysator.liu.se>

Peter Astrand <astrand@lysator.liu.se> writes:
I'd like to resolve bug 959379. Here's a naive fix. Comments?
If you're going to match the normal close() processing, shouldn't that check the result against EOF and not just != 0? If for example, the file object is being used for a pipe, the result of the close might be the result code of the child process which need not be an error. I think that only EOF should signify a failure of the close(), and only in that case is errno probably appropriate (which perror will pick up). -- David

On Mon, 25 Oct 2004, David Bolen wrote:
Yes. Thanks. Here's the new patch. OK to commit? diff -u -r2.192 fileobject.c --- fileobject.c 11 Jun 2004 04:49:03 -0000 2.192 +++ fileobject.c 26 Oct 2004 05:50:12 -0000 @@ -304,7 +304,8 @@ PyObject_ClearWeakRefs((PyObject *) f); if (f->f_fp != NULL && f->f_close != NULL) { Py_BEGIN_ALLOW_THREADS - (*f->f_close)(f->f_fp); + if ((*f->f_close)(f->f_fp) == EOF) + perror("close failed"); Py_END_ALLOW_THREADS } PyMem_Free(f->f_setbuf); /Peter Åstrand <astrand@lysator.liu.se>

Peter Astrand wrote:
Yes. Thanks. Here's the new patch. OK to commit?
No. I feel that perror is inappropriate; PySys_WriteStderr might be slightly better. However, it might be that even raising an exception has some value. Regards, Martin

On Tue, 26 Oct 2004, "Martin v. Löwis" wrote:
Here's a patch with PySys_WriteStderr: diff -u -r2.192 fileobject.c --- Objects/fileobject.c 11 Jun 2004 04:49:03 -0000 2.192 +++ Objects/fileobject.c 26 Oct 2004 18:44:19 -0000 @@ -300,12 +300,15 @@ static void file_dealloc(PyFileObject *f) { + int sts = 0; if (f->weakreflist != NULL) PyObject_ClearWeakRefs((PyObject *) f); if (f->f_fp != NULL && f->f_close != NULL) { Py_BEGIN_ALLOW_THREADS - (*f->f_close)(f->f_fp); + sts = (*f->f_close)(f->f_fp); Py_END_ALLOW_THREADS + if (sts == EOF) + PySys_WriteStderr("close failed: %s\n", strerror(errno)); } PyMem_Free(f->f_setbuf); Py_XDECREF(f->f_name);
However, it might be that even raising an exception has some value.
Is it really possible to raise an exception in response to something triggered by the GC? /Peter Åstrand <astrand@lysator.liu.se>

[Peter Astrand] ...
Is it really possible to raise an exception in response to something triggered by the GC?
It's possible but wholly useless. If you try, gc will detect that an exception was raised, and kill the Python process ungracefully, via Py_FatalError("unexpected exception during garbage collection"); You don't want to do that <wink>.

On Tue, 26 Oct 2004, "Martin v. Löwis" wrote:
Like this?: diff -u -r2.192 fileobject.c --- dist/src/Objects/fileobject.c 11 Jun 2004 04:49:03 -0000 2.192 +++ dist/src/Objects/fileobject.c 7 Nov 2004 10:13:04 -0000 @@ -300,12 +300,19 @@ static void file_dealloc(PyFileObject *f) { + int sts = 0; if (f->weakreflist != NULL) PyObject_ClearWeakRefs((PyObject *) f); if (f->f_fp != NULL && f->f_close != NULL) { Py_BEGIN_ALLOW_THREADS - (*f->f_close)(f->f_fp); + sts = (*f->f_close)(f->f_fp); Py_END_ALLOW_THREADS + if (sts == EOF) +#ifdef HAVE_STRERROR + PySys_WriteStderr("close failed: [Errno %d] %s\n", errno, strerror(errno)); +#else + PySys_WriteStderr("close failed: [Errno %d]\n", errno); +#endif } PyMem_Free(f->f_setbuf); Py_XDECREF(f->f_name); /Peter Åstrand <astrand@lysator.liu.se>
participants (4)
-
"Martin v. Löwis"
-
David Bolen
-
Peter Astrand
-
Tim Peters