New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
imp.find_module crashes Python if there exists a directory named "__init__.py" #51981
Comments
Create a directory "__init__.py" and execute
to reproduce that issue. It will crash because Python tries to double-close a file pointer: |
Confirmed on trunk. ~ $ mkdir foo.py |
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 :-( |
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 |
AFAICT, in this case, if PyString_FromString gives NULL, then (o_name == NULL) and the function returns without calling "fill_file_fields". Do you suggest something else? |
I don't have a better idea, that's why I don't have provided a patch. |
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, |
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
|
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': New changeset 8c6fea5794b2 by Victor Stinner in branch 'default': |
New changeset 0f5b64630fda by Victor Stinner in branch '2.7': |
This broke the Windows buildbots in Python 2.7. |
Victor, can you fix the test failures on Windows and 2.7? |
New changeset 555871844962 by Victor Stinner in branch '2.7': |
New changeset bf882390713c by Brett Cannon in branch 'default': |
The test just failed on x86 Windows Server 2003 [SB] 3.x: ====================================================================== 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: ====================================================================== 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': |
Seems fixed now. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: