classification
Title: imp.find_module crashes Python if there exists a directory named "__init__.py"
Type: crash Stage: resolved
Components: Library (Lib) Versions: Python 3.2, Python 3.3, Python 2.7, Python 2.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: haypo Nosy List: BreamoreBoy, Trundle, amaury.forgeotdarc, barry, brett.cannon, doko, flox, haypo, l0nwlf, ncoghlan, orsenthil, pitrou, python-dev, r.david.murray
Priority: high Keywords: patch

Created on 2010-01-18 12:07 by Trundle, last changed 2013-08-23 19:20 by pitrou. This issue is now closed.

Files
File name Uploaded Description Edit
issue7732_find_module_v2.diff flox, 2010-01-28 20:11 Patch, apply to trunk
import_directory-py3k.patch haypo, 2011-06-20 13:35 review
pyfile_fromfile_close.patch haypo, 2011-06-20 13:59
Messages (29)
msg98007 - (view) Author: Andreas Stührk (Trundle) Date: 2010-01-18 12:07
Create a directory "__init__.py" and execute

>>> import imp
>>> imp.find_module('__init__', ['.'])

to reproduce that issue. It will crash because Python tries to double-close a file pointer: `call_find_module` will call `PyFile_FromFile`, but `PyFile_FromFile` closes the file pointer if it's a directory (by decrefing the created file object) and ` call_find_module` then closes the already closed file.
msg98009 - (view) Author: Florent Xicluna (flox) * (Python committer) Date: 2010-01-18 12:42
Confirmed on trunk.

~ $ mkdir foo.py
~ $ ./python -c 'import imp;imp.find_module("foo", ["."])'
*** glibc detected *** ./python: double free or corruption (!prev): 0x00000000024a7390 ***
======= Backtrace: =========
(...)
msg98010 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-01-18 13:29
Confirmed that this does not affect py3k.
msg98011 - (view) Author: Florent Xicluna (flox) * (Python committer) Date: 2010-01-18 13:34
Patch proposed.
msg98012 - (view) Author: Florent Xicluna (flox) * (Python committer) Date: 2010-01-18 13:40
Removed the EnvironmentVarGuard from the test. It does not make sense.
msg98472 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2010-01-28 18:28
This is slightly incorrect: if PyFile_FromFile fails for another reason (PyString_FromString(name) runs out of memory), the fp is not closed and the caller is right to call fclose().

IMO PyFile_FromFile() should be changed to consistently leave the fp opened when NULL is returned. But then, many usages of this function are incorrect, e.g in posixmodule.c :-(
msg98474 - (view) Author: Florent Xicluna (flox) * (Python committer) Date: 2010-01-28 20:11
> if PyFile_FromFile fails for another reason (PyString_FromString(name)
> runs out of memory), the fp is not closed and the caller is right to
> call fclose().

As far as I understand, the fp is never left open, when PyFile_FromFile returns NULL. So there's no reason to call fclose on it.

However I found a reference leak in the case you describe (PyString_FromString(name) == NULL).

It is fixed with this last update.
msg98475 - (view) Author: Andreas Stührk (Trundle) Date: 2010-01-28 20:23
Note that the fp gets set with `fill_file_fields()` and that is called after the error return of `PyString_FromString()`. Hence, the fp is left open if `PyString_FromString()` returns NULL.
msg98476 - (view) Author: Florent Xicluna (flox) * (Python committer) Date: 2010-01-28 20:30
AFAICT, in this case, if PyString_FromString gives NULL, then (o_name == NULL) and the function returns without calling "fill_file_fields".
Hence, the fp is not opened.

Do you suggest something else?
msg98477 - (view) Author: Andreas Stührk (Trundle) Date: 2010-01-28 20:40
`fill_file_fields()` does not open the fp, the caller of `PyFile_FromFile()` opens the fp.

I don't have a better idea, that's why I don't have provided a patch.
msg102694 - (view) Author: Florent Xicluna (flox) * (Python committer) Date: 2010-04-09 07:59
see also http://bugs.python.org/issue8352#msg102662
msg102695 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2010-04-09 08:09
I can reproduce this locally. I believe it is relevant that a simple "import crash" (with crash.py as the directory) doesn't cause a problem - there must be something higher in the import machinery which avoids the issue.
msg102699 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2010-04-09 08:36
Ah, OK - the problem is confined solely to the wrapper for the Python imp module function. The normal import machinery doesn't go through the wrapper and hence doesn't have the problem.

The PyFile_FromFile logic is a little convoluted, but Florent's patch looks correct. Currently, if the dircheck call in fill_file_fields fails, the function returns NULL, but leaves the file object populated (included its f_fp field). The Py_DECREF call then implicitly closes the file, resulting in a double close when call_find_module does the same thing manually.

One other thing that is a little dubious in this code is the lack of error checking on the conversion of mode to a string object in fill_file_fields. That's fine for file_init (where mode came from a Python string object in the first place), but not valid for PyFile_FromFile (where mode is passed in as a char * instance).
msg102702 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2010-04-09 08:55
An interesting part of this story is *why* it doesn't crash in Py3k (despite explicitly closing the file descriptor in the same way as 2.x closes the C file pointer).

The reason is that PyFile_FromFd (the closest Py3k equivalent to PyFile_FromFile) will sometimes leave the file descriptor open, even if closefd is True. Specifically, this will happen if the raw file IO object fails to be created. Any subsequent failure while opening the file (e.g. while creating the line buffering or text wrapper) will trigger the same double close bug as occurs in 2.x.

io_open needs to be fixed so this behaviour is consistent: if creation of the raw file IO object fails and closefd is True, io_open should close the file descriptor so that the behaviour on error is consistent.
msg110328 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2010-07-14 20:51
Quote from ncoghlan on msg102699 "Florent's patch looks correct".  Does anyone else wish to comment or can this be taken forward?
msg113336 - (view) Author: Florent Xicluna (flox) * (Python committer) Date: 2010-08-08 22:00
According to message http://bugs.python.org/issue7732#msg102702,
someone should write a patch for 3.x too.
msg138707 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2011-06-20 13:35
import_directory-py3k.patch: find_module_path_list() ignores silently directories matching requested filename pattern (like module_name + ".py"). I don't think that it is useful to emit a warning (or raise an error) here, the code checks for various file extensions, not only .pyc: .so, .pyd, ...
msg138716 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2011-06-20 13:59
pyfile_fromfile_close.patch: patch based on issue7732_find_module_v2.diff, fixing this issue in Python 2.7

 - PyFile_FromFile() closes the file on PyString_FromString() failure (note: unlikely failure)
 - call_find_module() doesn't close the file anymore, PyFile_FromFile() closes already the file on failure (e.g. if the path is a directory)
 - update PyFile_FromFile() doc to simplify that the file is closed on error
msg144450 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2011-09-23 16:29
Note that Python 2.6 is also vulnerable to the crash.  While we do not have an exploit, we did get a report on security@ which led to this bug.  I could be convinced to allow the patch to 2.6 on grounds that if the crasher can be exploited, better to apply it now rather than wait.  Certainly if it's easier to apply 2.6 and forward port, I'm fine with that.

Victor's pyfile_fromfile_close.patch looks good to me and fixes the problem with no discernible ill effects.  On IRC, he said he'll apply it to 2.7, 3.2, and 3.3.  I will approve it for 2.6 if he wants to apply it there too.
msg144463 - (view) Author: Roundup Robot (python-dev) Date: 2011-09-23 16:59
New changeset 125887a41a6f by Victor Stinner in branch '3.2':
Issue #7732: Don't open a directory as a file anymore while importing a
http://hg.python.org/cpython/rev/125887a41a6f

New changeset 8c6fea5794b2 by Victor Stinner in branch 'default':
Merge 3.2: Issue #7732: Don't open a directory as a file anymore while
http://hg.python.org/cpython/rev/8c6fea5794b2
msg144465 - (view) Author: Roundup Robot (python-dev) Date: 2011-09-23 17:37
New changeset 0f5b64630fda by Victor Stinner in branch '2.7':
Issue #7732: Fix a crash on importing a module if a directory has the same name
http://hg.python.org/cpython/rev/0f5b64630fda
msg144977 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-10-06 00:09
This broke the Windows buildbots in Python 2.7.
msg147532 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-11-12 23:31
Victor, can you fix the test failures on Windows and 2.7?
Otherwise the commit should be reverted.
msg147628 - (view) Author: Roundup Robot (python-dev) Date: 2011-11-14 19:49
New changeset 555871844962 by Victor Stinner in branch '2.7':
Issue #7732: Try to fix the a failing test on Windows
http://hg.python.org/cpython/rev/555871844962
msg190762 - (view) Author: Roundup Robot (python-dev) Date: 2013-06-07 17:27
New changeset bf882390713c by Brett Cannon in branch 'default':
Issue #7732: Move an imp.find_module test from test_import to
http://hg.python.org/cpython/rev/bf882390713c
msg191820 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2013-06-24 22:52
The test just failed on x86 Windows Server 2003 [SB] 3.x:

http://buildbot.python.org/all/builders/x86%20Windows%20Server%202003%20%5BSB%5D%203.x/builds/1077/steps/test/logs/stdio

======================================================================
FAIL: test_bug7732 (test.test_imp.ImportTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "E:\Data\buildslave\cpython\3.x.snakebite-win2k3r2sp2-x86\build\lib\test\test_imp.py", line 285, in test_bug7732
    imp.find_module, support.TESTFN, ["."])
AssertionError: ImportError not raised by find_module
msg195668 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-08-19 21:24
The bug still fails on a regular basis on the Windows buildbots:

======================================================================
FAIL: test_bug7732 (test.test_imp.ImportTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "E:\Data\buildslave\cpython\3.x.snakebite-win2k3r2sp2-x86\build\lib\test\test_imp.py", line 285, in test_bug7732
    imp.find_module, support.TESTFN, ["."])
AssertionError: ImportError not raised by find_module
msg195672 - (view) Author: Roundup Robot (python-dev) Date: 2013-08-19 21:31
New changeset 4f7845be9e23 by Antoine Pitrou in branch 'default':
Issue #7732: try to fix test_bug7732's flakiness on Windows by executing it in a fresh temporary directory.
http://hg.python.org/cpython/rev/4f7845be9e23
msg196012 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-08-23 19:20
Seems fixed now.
History
Date User Action Args
2013-08-23 19:20:34pitrousetstatus: open -> closed
resolution: fixed
messages: + msg196012

stage: needs patch -> resolved
2013-08-19 21:31:30python-devsetmessages: + msg195672
2013-08-19 21:24:24pitrousetmessages: + msg195668
2013-06-24 22:52:55hayposetstatus: closed -> open
resolution: fixed -> (no value)
messages: + msg191820
2013-06-07 17:27:01python-devsetmessages: + msg190762
2012-02-20 20:36:46dmalcolmlinkissue14066 superseder
2011-11-16 22:23:56hayposetstatus: open -> closed
resolution: fixed
2011-11-14 19:49:27python-devsetmessages: + msg147628
2011-11-12 23:31:52pitrousetmessages: + msg147532
2011-10-06 00:09:49pitrousetassignee: haypo
messages: + msg144977
2011-09-23 17:37:22python-devsetmessages: + msg144465
2011-09-23 16:59:13python-devsetnosy: + python-dev
messages: + msg144463
2011-09-23 16:30:14barrysetversions: - Python 3.1
2011-09-23 16:29:42barrysetnosy: + barry

messages: + msg144450
versions: + Python 2.6, Python 3.1
2011-06-20 13:59:10hayposetfiles: + pyfile_fromfile_close.patch

messages: + msg138716
2011-06-20 13:35:22hayposetfiles: + import_directory-py3k.patch

messages: + msg138707
2011-06-12 19:21:04pitrousetnosy: + haypo
2011-06-12 18:42:40terry.reedysetversions: + Python 3.3, - Python 2.6, Python 3.1
2010-08-08 22:00:29floxsetassignee: flox -> (no value)
stage: patch review -> needs patch
messages: + msg113336
versions: + Python 3.1, Python 3.2
2010-07-14 20:51:40BreamoreBoysetnosy: + BreamoreBoy
messages: + msg110328
2010-04-09 09:16:23floxsetnosy: + pitrou
2010-04-09 08:55:30ncoghlansetmessages: + msg102702
2010-04-09 08:36:18ncoghlansetassignee: ncoghlan -> flox
messages: + msg102699
2010-04-09 08:09:41ncoghlansetassignee: ncoghlan
messages: + msg102695
2010-04-09 07:59:56floxsetnosy: + doko, ncoghlan, orsenthil, l0nwlf
messages: + msg102694
2010-04-09 07:57:25floxlinkissue8352 superseder
2010-01-28 20:40:50Trundlesetmessages: + msg98477
2010-01-28 20:30:53floxsetmessages: + msg98476
2010-01-28 20:23:58Trundlesetmessages: + msg98475
2010-01-28 20:11:26floxsetfiles: + issue7732_find_module_v2.diff

messages: + msg98474
2010-01-28 20:05:48floxsetfiles: - issue7732_find_module.diff
2010-01-28 18:28:13amaury.forgeotdarcsetnosy: + amaury.forgeotdarc
messages: + msg98472
2010-01-28 17:53:31floxsetstage: needs patch -> patch review
2010-01-18 13:40:47floxsetfiles: + issue7732_find_module.diff

messages: + msg98012
2010-01-18 13:40:28floxsetfiles: - issue7732_find_module.diff
2010-01-18 13:35:27floxsetfiles: + issue7732_find_module.diff
keywords: + patch
2010-01-18 13:34:46floxsetmessages: + msg98011
2010-01-18 13:29:45r.david.murraysetpriority: high
nosy: + r.david.murray, brett.cannon
messages: + msg98010

2010-01-18 12:42:44floxsetversions: + Python 2.7
nosy: + flox

messages: + msg98009

stage: needs patch
2010-01-18 12:07:04Trundlecreate