classification
Title: Unchecked return value of I/O functions
Type: behavior Stage: patch review
Components: Versions: Python 3.2, Python 3.3, Python 2.7
process
Status: open Resolution:
Dependencies: 23860 23878 Superseder:
Assigned To: Nosy List: berker.peksag, christian.heimes, ezio.melotti, felipecruz, jcea, jcon, mrshu, serhiy.storchaka, socketpair, vstinner
Priority: normal Keywords: patch

Created on 2012-09-15 15:29 by christian.heimes, last changed 2016-03-07 10:54 by socketpair.

Files
File name Uploaded Description Edit
issue15948__cursesmodule.patch mrshu, 2012-10-24 11:37 review
unchecked_return_values.patch mrshu, 2012-12-08 00:25 review
Messages (17)
msg170521 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2012-09-15 15:29
Python's C code contains more than 30 lines that don't check the return value of I/O functions like fseek(). A missing check can hide issues like a failing NFS connection.

I've created an (incomplete) list of missing checks with find and grep.

$ find -name '*.c' | sort | xargs egrep '^(\t|\ )*(fopen|fdopen|fread|fseek|fwite|open|read|write|readdir|readlink|lseek|dup|dup2|opendir|fdopendir|closedir|dirfd|readdir|seekdir|scandir|telldir|fcntl|ioctl)\ *\('

./Modules/_ctypes/libffi/src/dlmalloc.c:          read(fd, buf, sizeof(buf)) == sizeof(buf)) {
./Modules/_cursesmodule.c:    fseek(fp, 0, 0);
./Modules/_cursesmodule.c:    fseek(fp, 0, 0);
./Modules/faulthandler.c:        write(thread.fd, thread.header, thread.header_len);
./Modules/getpath.c:    fseek(env_file, 0, SEEK_SET);
./Modules/mmapmodule.c:        lseek(fileno, 0, SEEK_SET);
./Modules/ossaudiodev.c:     ioctl(fd, SNDCTL_DSP_cmd, &arg)
./Modules/posixmodule.c:    ioctl(slave_fd, I_PUSH, "ptem"); /* push ptem */
./Modules/posixmodule.c:    ioctl(slave_fd, I_PUSH, "ldterm"); /* push ldterm */
./Modules/posixmodule.c:    ioctl(slave_fd, I_PUSH, "ttcompat"); /* push ttcompat */
./Modules/_posixsubprocess.c:            fcntl(fd_dir_fd, F_SETFD, old | FD_CLOEXEC);
./Modules/_posixsubprocess.c:            fcntl(p2cread, F_SETFD, old & ~FD_CLOEXEC);
./Modules/_posixsubprocess.c:            fcntl(c2pwrite, F_SETFD, old & ~FD_CLOEXEC);
./Modules/_posixsubprocess.c:            fcntl(errwrite, F_SETFD, old & ~FD_CLOEXEC);
./Modules/signalmodule.c:        write(wakeup_fd, &byte, 1);
./Modules/socketmodule.c:    ioctl(s->sock_fd, FIONBIO, (caddr_t)&block, sizeof(block));
./Modules/socketmodule.c:    ioctl(s->sock_fd, FIONBIO, (unsigned int *)&block);
./Modules/socketmodule.c:    fcntl(s->sock_fd, F_SETFL, delay_flag);
./Modules/zipimport.c:    fseek(fp, -22, SEEK_END);
./Modules/zipimport.c:        fseek(fp, header_offset, 0);  /* Start of file header */
./Modules/zipimport.c:        fseek(fp, header_offset + 8, 0);
./Modules/zipimport.c:        fseek(fp, header_offset + 42, 0);
./Modules/zipimport.c:    fseek(fp, file_offset, 0);
./Modules/zipimport.c:    fseek(fp, file_offset + 26, 0);
./Modules/zlib/gzlib.c:        open(path,
./PC/getpathp.c:    fseek(env_file, 0, SEEK_SET);
./Python/traceback.c:    lseek(fd, 0, 0); /* Reset position */
./Python/traceback.c:    write(fd, buffer, len);
./Python/traceback.c:    write(fd, buffer, len);
./Python/traceback.c:            write(fd, &c, 1);
./Python/traceback.c:        write(fd, "\"", 1);
./Python/traceback.c:        write(fd, "\"", 1);
./Python/traceback.c:    write(fd, "\n", 1);
./Python/traceback.c:            write(fd, "\n", 1);

The missing checks for zipimport.c are already handles by ticket #15897.
msg170540 - (view) Author: Felipe Cruz (felipecruz) * Date: 2012-09-15 23:08
I can submit patches.. 

Is there any problem to send 1 patch per module?
msg170541 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2012-09-15 23:17
I think that's OK.
msg173662 - (view) Author: Marek Šuppa (mrshu) * Date: 2012-10-24 09:47
Since there is probably a lot to work on here I'd also like to participate.

I've got one question though. In case these function don't return 0 and the test fails what should happen then? What kind of error should be thrown for let's say fseek() ?
msg173663 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2012-10-24 09:55
The functions should set an appropriate errno so it's PyErr_SetFromErrno(PyExc_IOError). You should use the PyErr_SetFromErrnoWithFilename*() variants when a file name (PyObject*, char*, unicode) is available.
msg173664 - (view) Author: Marek Šuppa (mrshu) * Date: 2012-10-24 10:00
Thanks for a quick response. 

Should we also test this somewhere?

Christian Heimes <report@bugs.python.org> wrote:

>
>Christian Heimes added the comment:
>
>The functions should set an appropriate errno so it's
>PyErr_SetFromErrno(PyExc_IOError). You should use the
>PyErr_SetFromErrnoWithFilename*() variants when a file name (PyObject*,
>char*, unicode) is available.
>
>----------
>
>_______________________________________
>Python tracker <report@bugs.python.org>
><http://bugs.python.org/issue15948>
>_______________________________________

Marek, http://marek.suppa.co
msg173668 - (view) Author: Marek Šuppa (mrshu) * Date: 2012-10-24 11:37
Appended is the patch for _cursesmodule.c 

Let me know if it's OK.
msg174089 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2012-10-28 23:57
There is a typo in the command: s/fwite/fwrite/.

./Objects/object.c:                fwrite(PyBytes_AS_STRING(s), 1,
./Objects/object.c:                    fwrite(PyBytes_AS_STRING(t), 1,
./PC/bdist_wininst/install.c:        fwrite(arc_data, exe_size, 1, fp);
./Python/marshal.c:        fwrite(s, 1, n, p->fp);
msg177140 - (view) Author: Marek Šuppa (mrshu) * Date: 2012-12-08 00:25
Hi, 

Sorry for the long delay.

The attached patch should fix all the relevant occurrences of I/O functions I was able to find.

Please review.

Thanks!
msg178577 - (view) Author: Marek Šuppa (mrshu) * Date: 2012-12-30 14:11
Any update on this?
msg179584 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-01-10 19:18
Some general notes. Nitpick: check foo() < 0 is more used than foo() == 0. An exception raised after failed close() can hide original exception raised before. I left more specific comments on Rietveld.

Only a small part of the proposed changes may be approved by me. About the majority of changes only the module maintainer can say how they are safe and how to do correctly (they looks too risky for me). Some of the changes are obviously wrong.
msg179586 - (view) Author: Marek Šuppa (mrshu) * Date: 2013-01-10 19:34
Thanks for the review.

Do you think I should split it into multiple patches to make it easier to look through?

It might be that some changes are completely wrong. This is the first time I did something with cpython core code.
msg179597 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-01-10 20:56
Oh, I forgot press "Push All My Drafts" button.

Actually only patch for _cursesmodule.c looks safe at first glance (but curses maintainer can decide better). You can split the patch on several patches, but be very careful. You should research the entire module to understand to what effects the changes will lead and what changes should be. I would not recommend this task for beginners.
msg179598 - (view) Author: Marek Šuppa (mrshu) * Date: 2013-01-10 21:15
That is probably right.

I was way too foolish to start with this but won't stop now.

I'll try to iterate on your feedback.

Thanks!
msg179602 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2013-01-10 22:19
./Python/traceback.c:            write(fd, &c, 1);
./Python/traceback.c:        write(fd, "\"", 1);
./Python/traceback.c:        write(fd, "\"", 1);
./Python/traceback.c:    write(fd, "\n", 1);
./Python/traceback.c:            write(fd, "\n", 1);

Oh, I wrote these ones. It is code called by the faulthandle module, from a signal handler. So only async-safe functions can be called (no thread, no memory allocation, no lock!, etc.).

A simple fix would be to mark that we don't care if write() fails, but (void)write(fd, ...); doesn't make the warning quiet.
msg179603 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2013-01-10 22:25
diff -r 0acc5626a578 Modules/faulthandler.c
@@ -445,7 +445,10 @@
-        write(thread.fd, thread.header, thread.header_len);
+        if (write(thread.fd, thread.header, thread.header_len) == -1) {
+            PyErr_SetFromErrno(PyExc_IOError);
+            return; 
+        }
 
I wrote faulthandler to debug deadlocks, memory corruptions and other cases where Python internals are no consistency anymore.

faulthandler_thread() is not a Python thread, but a C thread. I don't know if it's legal to call PyErr_SetFromErrno(). And it would be really surprising to get a Python exception whereas it does not come from Python code.

I would prefer to just ignore if write() failed here.
msg261294 - (view) Author: Марк Коренберг (socketpair) * Date: 2016-03-07 10:54
In a common case,

if (write(thread.fd, thread.header, thread.header_len) == -1)

should be replaced with

if (write(thread.fd, thread.header, thread.header_len) != thread.header_len)
History
Date User Action Args
2016-03-07 10:54:24socketpairsetnosy: + socketpair
messages: + msg261294
2016-03-06 16:29:59berker.peksagsetdependencies: + Missing sanity checks for various C library function calls...
2015-04-04 08:32:47berker.peksagsetdependencies: + Windows: Failure to check return value from lseek() in Modules/mmapmodule.c
2013-01-15 09:17:58jceasetnosy: + jcea
2013-01-10 22:25:18vstinnersetmessages: + msg179603
2013-01-10 22:19:02vstinnersetnosy: + vstinner
messages: + msg179602
2013-01-10 21:15:02mrshusetmessages: + msg179598
2013-01-10 20:56:31serhiy.storchakasetkeywords: - easy

messages: + msg179597
2013-01-10 19:34:28mrshusetmessages: + msg179586
2013-01-10 19:18:47serhiy.storchakasetmessages: + msg179584
2013-01-10 09:12:17serhiy.storchakasetstage: needs patch -> patch review
2013-01-10 04:50:16jconsetnosy: + jcon
2012-12-30 15:23:47ezio.melottisetnosy: + serhiy.storchaka
2012-12-30 14:11:11mrshusetmessages: + msg178577
2012-12-08 00:25:10mrshusetfiles: + unchecked_return_values.patch

messages: + msg177140
2012-10-28 23:57:44berker.peksagsetnosy: + berker.peksag
messages: + msg174089
2012-10-24 11:37:35mrshusetfiles: + issue15948__cursesmodule.patch
keywords: + patch
messages: + msg173668
2012-10-24 10:00:09mrshusetmessages: + msg173664
2012-10-24 09:55:38christian.heimessetmessages: + msg173663
2012-10-24 09:47:15mrshusetnosy: + mrshu
messages: + msg173662
2012-09-15 23:17:13ezio.melottisetnosy: + ezio.melotti

messages: + msg170541
stage: needs patch
2012-09-15 23:08:52felipecruzsetnosy: + felipecruz
messages: + msg170540
2012-09-15 15:29:06christian.heimescreate