[issue15948] Unchecked return value of I/O functions

[Марк Коренберг]

Марк Коренберг added the comment:

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)

--
nosy: +mmarkk

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15948] Unchecked return value of I/O functions

[Berker Peksag]

Changes by Berker Peksag :


--
dependencies: +Missing sanity checks for various C library function calls...

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15948] Unchecked return value of I/O functions

[Berker Peksag]

Changes by Berker Peksag berker.pek...@gmail.com:


--
dependencies: +Failure to check return value from lseek() in 
Modules/mmapmodule.c

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15948
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15948] Unchecked return value of I/O functions

[Jesús Cea Avión]

Changes by Jesús Cea Avión j...@jcea.es:


--
nosy: +jcea

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15948
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15948] Unchecked return value of I/O functions

[Serhiy Storchaka]

Changes by Serhiy Storchaka storch...@gmail.com:


--
stage: needs patch - patch review

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15948
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15948] Unchecked return value of I/O functions

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

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.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15948
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15948] Unchecked return value of I/O functions

[Marek Šuppa]

Marek Šuppa added the comment:

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.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15948
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15948] Unchecked return value of I/O functions

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

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.

--
keywords:  -easy

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15948
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15948] Unchecked return value of I/O functions

[Marek Šuppa]

Marek Šuppa added the comment:

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!

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15948
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15948] Unchecked return value of I/O functions

[STINNER Victor]

STINNER Victor added the comment:

./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.

--
nosy: +haypo

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15948
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15948] Unchecked return value of I/O functions

[STINNER Victor]

STINNER Victor added the comment:

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.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15948
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15948] Unchecked return value of I/O functions

[John O'Connor]

Changes by John O'Connor tehj...@gmail.com:


--
nosy: +jcon

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15948
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15948] Unchecked return value of I/O functions

[Marek Šuppa]

Marek Šuppa added the comment:

Any update on this?

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15948
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15948] Unchecked return value of I/O functions

[Ezio Melotti]

Changes by Ezio Melotti ezio.melo...@gmail.com:


--
nosy: +serhiy.storchaka

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15948
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15948] Unchecked return value of I/O functions

[Marek Šuppa]

Marek Šuppa added the comment:

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!

--
Added file: http://bugs.python.org/file28253/unchecked_return_values.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15948
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15948] Unchecked return value of I/O functions

[Berker Peksag]

Berker Peksag added the comment:

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);

--
nosy: +berker.peksag

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15948
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15948] Unchecked return value of I/O functions

[Marek Šuppa]

Marek Šuppa added the comment:

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() ?

--
nosy: +mrshu

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15948
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15948] Unchecked return value of I/O functions

[Christian Heimes]

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 rep...@bugs.python.org
http://bugs.python.org/issue15948
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15948] Unchecked return value of I/O functions

[Marek Šuppa]

Marek Šuppa added the comment:

Thanks for a quick response. 

Should we also test this somewhere?

Christian Heimes rep...@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 rep...@bugs.python.org
http://bugs.python.org/issue15948
___

Marek, http://marek.suppa.co

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15948
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15948] Unchecked return value of I/O functions

[Marek Šuppa]

Marek Šuppa added the comment:

Appended is the patch for _cursesmodule.c 

Let me know if it's OK.

--
keywords: +patch
Added file: http://bugs.python.org/file27692/issue15948__cursesmodule.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15948
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15948] Unchecked return value of I/O functions

[Christian Heimes]

New submission from Christian Heimes:

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.

--
keywords: easy
messages: 170521
nosy: christian.heimes
priority: normal
severity: normal
status: open
title: Unchecked return value of I/O functions
type: behavior
versions: Python 2.7, Python 3.2, Python 3.3

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15948
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15948] Unchecked return value of I/O functions

[Felipe Cruz]

Felipe Cruz added the comment:

I can submit patches.. 

Is there any problem to send 1 patch per module?

--
nosy: +felipecruz

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15948
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15948] Unchecked return value of I/O functions

[Ezio Melotti]

Ezio Melotti added the comment:

I think that's OK.

--
nosy: +ezio.melotti
stage:  - needs patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15948
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com