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

Hello!

This is one of my first patches.

Tests still OK! Let me know what you think!

Thanks!

Patch updated - fseek errors messges will be "can't read Zip file' and not "can't Open Zip file"

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.

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

You can further compress the changes when you get rid of `rc` and check the return value inline, for example:

  if (fseek(...) == -1) {
    errorhandler;
  }

v4 - inline fseek return code check - as Christian suggested

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.

Hello!

Just sent the Contributor Agreement Form.

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.

Jesus, please read the PyErr_Format() documentation.

Should I send patches for 3.2 and 2.7?

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.

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

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

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