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.

Confirmed on trunk.

~ $ mkdir foo.py
~ $ ./python -c 'import imp;imp.find_module("foo", ["."])'
*** glibc detected *** ./python: double free or corruption (!prev): 0x00000000024a7390 ***
======= Backtrace: =========
(...)

Confirmed that this does not affect py3k.

Patch proposed.

Removed the EnvironmentVarGuard from the test. It does not make sense.

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

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

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.

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?

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

see also http://bugs.python.org/issue8352#msg102662

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.

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

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.

Quote from ncoghlan on msg102699 "Florent's patch looks correct".  Does anyone else wish to comment or can this be taken forward?

According to message http://bugs.python.org/issue7732#msg102702,
someone should write a patch for 3.x too.

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

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

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.

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

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

This broke the Windows buildbots in Python 2.7.

Victor, can you fix the test failures on Windows and 2.7?
Otherwise the commit should be reverted.

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

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

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

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

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

Seems fixed now.