classification
Title: zipimport.c doesn't check return value of fseek()
Type: behavior Stage: resolved
Components: Extension Modules Versions: Python 3.4, Python 3.2, Python 3.3, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: jcea Nosy List: christian.heimes, ezio.melotti, felipecruz, jcea, pitrou, python-dev
Priority: normal Keywords: patch

Created on 2012-09-10 00:08 by christian.heimes, last changed 2012-10-03 01:19 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
issue15897_v1.patch felipecruz, 2012-09-15 03:49 review
issue15897_v1.patch felipecruz, 2012-09-15 04:44 review
issue15897_v1.patch felipecruz, 2012-09-15 13:20 review
issue15897_v4.patch felipecruz, 2012-09-15 17:18 review
Messages (16)
msg170146 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2012-09-10 00:08
The code im zipimport.c doesn't check the return value of fseek() in at least four places. The missing checks may hide issues with the file or file system.

Solution:
Check for -1 and return with an appropriate call to PyErr_SetFromErrno()
msg170504 - (view) Author: Felipe Cruz (felipecruz) * Date: 2012-09-15 03:49
Hello!

This is one of my first patches.

Tests still OK! Let me know what you think!

Thanks!
msg170505 - (view) Author: Felipe Cruz (felipecruz) * Date: 2012-09-15 04:44
Patch updated - fseek errors messges will be "can't read Zip file' and not "can't Open Zip file"
msg170506 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2012-09-15 05:10
I suggested to Felipe on IRC to use "read" instead of "seek" because I don't think it's worth making a distinction between the two.  Moreover ISTM that in most of the cases, if the fseek fails, the error is detected in a following fread, so the message currently says "can't read", and changing it to "can't seek" is technically backward-incompatible.

We also checked if the test suite followed any of these new error paths, but apparently only the "if" added in the first chunk (line 880, in read_directory) is already covered.  Going through the following chunks (the ones with the goto fseek_error) is fairly difficult.  It should be possibly to trigger the error at line 1071, in get_data, by changing the value of self.archive, but alas self.archive is read-only.

The error generated by PyErr_SetFromErrno() doesn't seem too useful.  For example, with "PyErr_SetFromErrno(PyExc_IOError);" instead of "PyErr_Format(ZipImportError, "can't open Zip file: %R", archive);", the error becomes "OSError: [Errno 22] Invalid argument".  This alone is fairly useless, even though the message might be better in other situations.  I think either using PyErr_Format, or a combination of the two (something like "ZipImportError: can't read Zip file: 'ziptestmodule' ([Errno 22] Invalid argument)") would be better.
msg170516 - (view) Author: Felipe Cruz (felipecruz) * Date: 2012-09-15 13:20
I've updated the patch changing fseek_error goto block error return from PyErr_SetFromErrno to PyErr_Format(ZipImportError, "can't read Zip file: %R", archive); (returning NULL after).
msg170519 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2012-09-15 14:50
You can further compress the changes when you get rid of `rc` and check the return value inline, for example:

  if (fseek(...) == -1) {
    errorhandler;
  }
msg170526 - (view) Author: Felipe Cruz (felipecruz) * Date: 2012-09-15 17:18
v4 - inline fseek return code check - as Christian suggested
msg171687 - (view) Author: Jesús Cea Avión (jcea) * (Python committer) Date: 2012-10-01 01:15
Felipe, could you please submit a Contributor Agreement Form? http://www.python.org/psf/contrib/

Your patch looks good to me, although I would move almost all

"""
 fclose(fp);
 PyErr_Format(ZipImportError, "can't read Zip file: %R", archive);
 return NULL;
"""

To a "goto" label.
msg171724 - (view) Author: Felipe Cruz (felipecruz) * Date: 2012-10-01 14:58
Hello!

Just sent the Contributor Agreement Form.
msg171833 - (view) Author: Jesús Cea Avión (jcea) * (Python committer) Date: 2012-10-02 21:54
I think this patch must be applied too to 2.7 and 3.2.

Felipe, you are using "%R" modifier, but that modifier doesn't exist in Solaris, for instance. This seems to be a Linux specific option, I don't know what it does.

Also, you should be sure not to overflow the 500 bytes internal buffer. You can use a size limit in the format string.
msg171834 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-10-02 22:25
Jesus, please read the PyErr_Format() documentation.
msg171835 - (view) Author: Felipe Cruz (felipecruz) * Date: 2012-10-02 22:29
Should I send patches for 3.2 and 2.7?
msg171839 - (view) Author: Jesús Cea Avión (jcea) * (Python committer) Date: 2012-10-02 23:54
Antoine, I was reading the documentation of Python 2.7. It doesn't support %R.

Felipe, Python 2.7 has direct access to the path. You can print it directly, in this case. I will take care of this.
msg171843 - (view) Author: Roundup Robot (python-dev) Date: 2012-10-03 01:04
New changeset 39e608d462b6 by Jesus Cea in branch '2.7':
Closes #15897: zipimport.c doesn't check return value of fseek()
http://hg.python.org/cpython/rev/39e608d462b6

New changeset 4da48083aaab by Jesus Cea in branch '3.2':
Closes #15897: zipimport.c doesn't check return value of fseek()
http://hg.python.org/cpython/rev/4da48083aaab

New changeset 0f1637c4d673 by Jesus Cea in branch '3.3':
MERGE: Closes #15897: zipimport.c doesn't check return value of fseek()
http://hg.python.org/cpython/rev/0f1637c4d673

New changeset ad63e5246306 by Jesus Cea in branch 'default':
MERGE: Closes #15897: zipimport.c doesn't check return value of fseek()
http://hg.python.org/cpython/rev/ad63e5246306
msg171844 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2012-10-03 01:16
You broke the build of 3.2:

./Modules/zipimport.c: In function 'read_directory':
./Modules/zipimport.c:747:65: error: 'archive' undeclared (first use in this function)
./Modules/zipimport.c:747:65: note: each undeclared identifier is reported only once for each function it appears in
make: *** [Modules/zipimport.o] Error 1
msg171845 - (view) Author: Roundup Robot (python-dev) Date: 2012-10-03 01:19
New changeset 0f4d4f4db724 by Jesus Cea in branch '3.2':
Closes #15897: zipimport.c doesn't check return value of fseek(). Typo
http://hg.python.org/cpython/rev/0f4d4f4db724
History
Date User Action Args
2012-10-03 01:19:11python-devsetstatus: open -> closed
resolution: fixed
messages: + msg171845
2012-10-03 01:16:00christian.heimessetstatus: closed -> open
resolution: fixed -> (no value)
messages: + msg171844
2012-10-03 01:04:03python-devsetstatus: open -> closed

nosy: + python-dev
messages: + msg171843

resolution: fixed
stage: patch review -> resolved
2012-10-02 23:54:08jceasetassignee: jcea
messages: + msg171839
2012-10-02 22:29:38felipecruzsetmessages: + msg171835
2012-10-02 22:25:41pitrousetmessages: + msg171834
2012-10-02 21:54:19jceasetmessages: + msg171833
versions: + Python 2.7, Python 3.2
2012-10-01 14:58:52felipecruzsetmessages: + msg171724
2012-10-01 01:15:33jceasetmessages: + msg171687
2012-09-15 17:18:16felipecruzsetfiles: + issue15897_v4.patch

messages: + msg170526
2012-09-15 14:50:33christian.heimessetmessages: + msg170519
2012-09-15 13:20:40felipecruzsetfiles: + issue15897_v1.patch

messages: + msg170516
2012-09-15 05:10:12ezio.melottisetnosy: + ezio.melotti, pitrou
messages: + msg170506

keywords: - easy
stage: patch review
2012-09-15 04:44:49felipecruzsetfiles: + issue15897_v1.patch

messages: + msg170505
2012-09-15 03:49:03felipecruzsetfiles: + issue15897_v1.patch

nosy: + felipecruz
messages: + msg170504

keywords: + patch
2012-09-10 02:20:55jceasetnosy: + jcea
2012-09-10 00:08:37christian.heimescreate