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) * |
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) * |
Date: 2010-01-18 13:29 |
Confirmed that this does not affect py3k.
|
msg98011 - (view) |
Author: Florent Xicluna (flox) * |
Date: 2010-01-18 13:34 |
Patch proposed.
|
msg98012 - (view) |
Author: Florent Xicluna (flox) * |
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) * |
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) * |
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) * |
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) * |
Date: 2010-04-09 07:59 |
see also http://bugs.python.org/issue8352#msg102662
|
msg102695 - (view) |
Author: Alyssa Coghlan (ncoghlan) * |
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: Alyssa Coghlan (ncoghlan) * |
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: Alyssa Coghlan (ncoghlan) * |
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) * |
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 (vstinner) * |
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 (vstinner) * |
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) * |
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) * |
Date: 2011-10-06 00:09 |
This broke the Windows buildbots in Python 2.7.
|
msg147532 - (view) |
Author: Antoine Pitrou (pitrou) * |
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 (vstinner) * |
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) * |
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) * |
Date: 2013-08-23 19:20 |
Seems fixed now.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:56:56 | admin | set | github: 51981 |
2013-08-23 19:20:34 | pitrou | set | status: open -> closed resolution: fixed messages:
+ msg196012
stage: needs patch -> resolved |
2013-08-19 21:31:30 | python-dev | set | messages:
+ msg195672 |
2013-08-19 21:24:24 | pitrou | set | messages:
+ msg195668 |
2013-06-24 22:52:55 | vstinner | set | status: closed -> open resolution: fixed -> (no value) messages:
+ msg191820
|
2013-06-07 17:27:01 | python-dev | set | messages:
+ msg190762 |
2012-02-20 20:36:46 | dmalcolm | link | issue14066 superseder |
2011-11-16 22:23:56 | vstinner | set | status: open -> closed resolution: fixed |
2011-11-14 19:49:27 | python-dev | set | messages:
+ msg147628 |
2011-11-12 23:31:52 | pitrou | set | messages:
+ msg147532 |
2011-10-06 00:09:49 | pitrou | set | assignee: vstinner messages:
+ msg144977 |
2011-09-23 17:37:22 | python-dev | set | messages:
+ msg144465 |
2011-09-23 16:59:13 | python-dev | set | nosy:
+ python-dev messages:
+ msg144463
|
2011-09-23 16:30:14 | barry | set | versions:
- Python 3.1 |
2011-09-23 16:29:42 | barry | set | nosy:
+ barry
messages:
+ msg144450 versions:
+ Python 2.6, Python 3.1 |
2011-06-20 13:59:10 | vstinner | set | files:
+ pyfile_fromfile_close.patch
messages:
+ msg138716 |
2011-06-20 13:35:22 | vstinner | set | files:
+ import_directory-py3k.patch
messages:
+ msg138707 |
2011-06-12 19:21:04 | pitrou | set | nosy:
+ vstinner
|
2011-06-12 18:42:40 | terry.reedy | set | versions:
+ Python 3.3, - Python 2.6, Python 3.1 |
2010-08-08 22:00:29 | flox | set | assignee: flox -> (no value) stage: patch review -> needs patch messages:
+ msg113336 versions:
+ Python 3.1, Python 3.2 |
2010-07-14 20:51:40 | BreamoreBoy | set | nosy:
+ BreamoreBoy messages:
+ msg110328
|
2010-04-09 09:16:23 | flox | set | nosy:
+ pitrou
|
2010-04-09 08:55:30 | ncoghlan | set | messages:
+ msg102702 |
2010-04-09 08:36:18 | ncoghlan | set | assignee: ncoghlan -> flox messages:
+ msg102699 |
2010-04-09 08:09:41 | ncoghlan | set | assignee: ncoghlan messages:
+ msg102695 |
2010-04-09 07:59:56 | flox | set | nosy:
+ doko, ncoghlan, orsenthil, l0nwlf messages:
+ msg102694
|
2010-04-09 07:57:25 | flox | link | issue8352 superseder |
2010-01-28 20:40:50 | Trundle | set | messages:
+ msg98477 |
2010-01-28 20:30:53 | flox | set | messages:
+ msg98476 |
2010-01-28 20:23:58 | Trundle | set | messages:
+ msg98475 |
2010-01-28 20:11:26 | flox | set | files:
+ issue7732_find_module_v2.diff
messages:
+ msg98474 |
2010-01-28 20:05:48 | flox | set | files:
- issue7732_find_module.diff |
2010-01-28 18:28:13 | amaury.forgeotdarc | set | nosy:
+ amaury.forgeotdarc messages:
+ msg98472
|
2010-01-28 17:53:31 | flox | set | stage: needs patch -> patch review |
2010-01-18 13:40:47 | flox | set | files:
+ issue7732_find_module.diff
messages:
+ msg98012 |
2010-01-18 13:40:28 | flox | set | files:
- issue7732_find_module.diff |
2010-01-18 13:35:27 | flox | set | files:
+ issue7732_find_module.diff keywords:
+ patch |
2010-01-18 13:34:46 | flox | set | messages:
+ msg98011 |
2010-01-18 13:29:45 | r.david.murray | set | priority: high nosy:
+ r.david.murray, brett.cannon messages:
+ msg98010
|
2010-01-18 12:42:44 | flox | set | versions:
+ Python 2.7 nosy:
+ flox
messages:
+ msg98009
stage: needs patch |
2010-01-18 12:07:04 | Trundle | create | |