classification
Title: Bug in IMPORT_NAME
Type: enhancement Stage: resolved
Components: Interpreter Core Versions: Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: benjamin.peterson, brett.cannon, eric.snow, georg.brandl, ncoghlan, python-dev, serhiy.storchaka, vstinner, yselivanov
Priority: normal Keywords: patch

Created on 2016-06-19 11:53 by serhiy.storchaka, last changed 2016-06-27 20:54 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
import_name_level.patch serhiy.storchaka, 2016-06-19 11:53
import_name_level_2.patch serhiy.storchaka, 2016-06-26 19:51 review
Messages (11)
msg268849 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-06-19 11:53
Seems there is a bug in the implementation of the IMPORT_NAME opcode in ceval.c.

If the level argument is -1, it is not passed to __import__ (should never happen, but looks correct). If it is an integer != -1 in C long range, it is passed to __import__ (this is correct). But if it is not integer (e.g. None) or can't be converted to C long, an exception is set and __import__ is called with level and not cleared error (this is wrong).

In correct bytecode the level argument can be either integer or None. Default __import__ accepts only integers as the level argument and checks the range. Proposed patch makes the code always passing the level argument to __import__ unless it is None.
msg268931 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2016-06-20 20:57
My suspicion is the -1 aspect is a hold-over from that being the default level value in Python 2.7 that no one removed.

The patch LGTM. Do we need a test for this for some reason?
msg268939 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-06-20 21:35
Thak you for the explanation Brett.

Since current "import" without "from" is not broken, I suppose that the raised exception is cleared somewhere later. The patch doesn't have visible effect (unless may be using nonstandard loaders or __import__, don't know). It just cleans up the code, avoid raising unneeded exception (small performance boost) and may be fix some hidden non-reproducible bug.
msg268940 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2016-06-20 21:36
Then I say go ahead and commit it.
msg269306 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-06-26 19:51
Sorry, I was wrong. The level argument is always non-negative integer (if a bytecode is not corrupted). The second branch can be just removed.

Following patch also fixes the validation of the ImportFrom AST node.
msg269385 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2016-06-27 16:33
LGTM
msg269394 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-06-27 18:39
New changeset e3164c9edb0b by Serhiy Storchaka in branch 'default':
Issue #27352: Correct the validation of the ImportFrom AST node and simplify
https://hg.python.org/cpython/rev/e3164c9edb0b
msg269395 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-06-27 18:40
Thanks Brett.
msg269401 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-06-27 20:33
You broke Python! Example:

http://buildbot.python.org/all/builders/AMD64%20Snow%20Leop%203.x/builds/4853/steps/test/logs/stdio


======================================================================
FAIL: test_importfrom (test.test_ast.ASTValidatorTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/buildbot/buildarea/3.x.murray-snowleopard/build/Lib/test/test_ast.py", line 757, in test_importfrom
    self.stmt(imp, "level less than -1")
  File "/Users/buildbot/buildarea/3.x.murray-snowleopard/build/Lib/test/test_ast.py", line 579, in stmt
    self.mod(mod, msg)
  File "/Users/buildbot/buildarea/3.x.murray-snowleopard/build/Lib/test/test_ast.py", line 571, in mod
    self.assertIn(msg, str(cm.exception))
AssertionError: 'level less than -1' not found in 'Negative ImportFrom level'
msg269402 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-06-27 20:41
New changeset e5063a82f490 by Serhiy Storchaka in branch 'default':
Issue #27352: Fixed an error message in a test.
https://hg.python.org/cpython/rev/e5063a82f490
msg269403 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-06-27 20:54
Thanks Victor!
History
Date User Action Args
2016-06-27 20:54:17serhiy.storchakasetstatus: open -> closed
resolution: fixed
messages: + msg269403
2016-06-27 20:41:03python-devsetmessages: + msg269402
2016-06-27 20:33:04vstinnersetstatus: closed -> open

nosy: + vstinner
messages: + msg269401

resolution: fixed -> (no value)
2016-06-27 18:40:51serhiy.storchakasetstatus: open -> closed
resolution: fixed
messages: + msg269395

stage: commit review -> resolved
2016-06-27 18:39:38python-devsetnosy: + python-dev
messages: + msg269394
2016-06-27 16:33:17brett.cannonsetmessages: + msg269385
stage: patch review -> commit review
2016-06-26 19:51:42serhiy.storchakasetfiles: + import_name_level_2.patch

type: behavior -> enhancement
versions: - Python 2.7, Python 3.5
nosy: + georg.brandl, benjamin.peterson, yselivanov

messages: + msg269306
stage: commit review -> patch review
2016-06-20 21:36:30brett.cannonsetmessages: + msg268940
2016-06-20 21:35:38serhiy.storchakasetmessages: + msg268939
2016-06-20 20:57:41brett.cannonsetassignee: serhiy.storchaka
messages: + msg268931
stage: patch review -> commit review
2016-06-19 11:53:12serhiy.storchakacreate