Bug in IMPORT_NAME

[serhiy-storchaka]

BPO	27352
Nosy	@brettcannon, @birkenfeld, @ncoghlan, @vstinner, @benjaminp, @ericsnowcurrently, @serhiy-storchaka, @1st1
Files	import_name_level.patch import_name_level_2.patch

^{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:

assignee = 'https://github.com/serhiy-storchaka'
closed_at = <Date 2016-06-27.20:54:17.536>
created_at = <Date 2016-06-19.11:53:12.714>
labels = ['interpreter-core', 'type-feature']
title = 'Bug in IMPORT_NAME'
updated_at = <Date 2016-06-27.20:54:17.511>
user = 'https://github.com/serhiy-storchaka'

bugs.python.org fields:

activity = <Date 2016-06-27.20:54:17.511>
actor = 'serhiy.storchaka'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2016-06-27.20:54:17.536>
closer = 'serhiy.storchaka'
components = ['Interpreter Core']
creation = <Date 2016-06-19.11:53:12.714>
creator = 'serhiy.storchaka'
dependencies = []
files = ['43470', '43545']
hgrepos = []
issue_num = 27352
keywords = ['patch']
message_count = 11.0
messages = ['268849', '268931', '268939', '268940', '269306', '269385', '269394', '269395', '269401', '269402', '269403']
nosy_count = 9.0
nosy_names = ['brett.cannon', 'georg.brandl', 'ncoghlan', 'vstinner', 'benjamin.peterson', 'python-dev', 'eric.snow', 'serhiy.storchaka', 'yselivanov']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue27352'
versions = ['Python 3.6']

[serhiy-storchaka]

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.

[brettcannon]

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?

[serhiy-storchaka]

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.

[brettcannon]

Then I say go ahead and commit it.

[serhiy-storchaka]

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.

[brettcannon]

LGTM

[python-dev]

New changeset e3164c9edb0b by Serhiy Storchaka in branch 'default':
Issue bpo-27352: Correct the validation of the ImportFrom AST node and simplify
https://hg.python.org/cpython/rev/e3164c9edb0b

[serhiy-storchaka]

Thanks Brett.

[vstinner]

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'