gh-112716: Fix SystemError when __builtins__ is not a dict #112770
Conversation
serhiy-storchaka
commented
Dec 5, 2023
•
- Issue: SystemError when builtins is not a dict + eval #112716
Lib/test/test_builtin.py
Outdated
| self.assertEqual(ns['foo'], ('foo.bar', ns, ns, None, 0)) | ||
|
|
||
| def test_eval_builtins_mapping_reduce(self): | ||
| code = compile("iter([1, 2]).__reduce__()", "test", "eval") |
There was a problem hiding this comment.
The test name is misleading. The test is more about getting a built-in function from custom builtins module (namespace), no?
I suggest testing that the code works as example. Such as:
code = compile("x = str(123)", "test", "exec")
ns = {}
globals = {'__builtins__': types.MappingProxyType({'str': str})}
exec(code, globals, ns)
self.assertEqual(ns['x'], "123")There was a problem hiding this comment.
It's testing __reduce__ on a list iterator specifically because the implementation of __reduce__ accesses PyEval_GetBuiltins. Your test wouldn't do that.
There was a problem hiding this comment.
The code already calls directly iter() explicitly. So it's not obvious if the expected error comes from this direct iter() call, or from the __reduce__() method call.
At least, it would be worth it to add a comment to explain these implementation details which are non obvious to me.
There was a problem hiding this comment.
That's a good point, might be better to pass iter([1, 2]) as one of the entries in the builtins.
| @@ -2568,7 +2568,7 @@ import_name(PyThreadState *tstate, _PyInterpreterFrame *frame, | |||
| PyObject *name, PyObject *fromlist, PyObject *level) | |||
| { | |||
| PyObject *import_func; | |||
| if (PyDict_GetItemRef(frame->f_builtins, &_Py_ID(__import__), &import_func) < 0) { | |||
| if (PyMapping_GetOptionalItem(frame->f_builtins, &_Py_ID(__import__), &import_func) < 0) { | |||
There was a problem hiding this comment.
Is it worth adding a fast path for the case where PyDict_CheckExact(builtins)?
There was a problem hiding this comment.
PyMapping_GetOptionalItem() already has a fast path for PyDict_CheckExact.
Backports will be more complicated.
There was a problem hiding this comment.
PyMapping_GetOptionalItem() has a fast-path for PyDict_CheckExact(). IMO it's enough.
Lib/test/test_builtin.py
Outdated
| @@ -832,6 +832,31 @@ class customdict(dict): # this one should not do anything fancy | |||
| self.assertRaisesRegex(NameError, "name 'superglobal' is not defined", | |||
| exec, code, {'__builtins__': customdict()}) | |||
|
|
|||
| def test_exec_builtins_mapping(self): | |||
There was a problem hiding this comment.
Could you also add some test cases where the builtins are a subclass of dict, ensuring that an overridden __getitem__ gets used?
There was a problem hiding this comment.
It is covered by test_exec_globals_error_on_get.
Lib/test/test_builtin.py
Outdated
| def test_exec_builtins_mapping(self): | ||
| code = compile("superglobal", "test", "exec") | ||
| # works correctly | ||
| exec(code, {'__builtins__': types.MappingProxyType({'superglobal': 1})}) |
There was a problem hiding this comment.
As I wrote previously, I would prefer to check for the expected superglobal value here.
There was a problem hiding this comment.
Do you want to change also existing tests?
There was a problem hiding this comment.
I'm only proposing to change this test.
|
Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12. |
|
Sorry, @serhiy-storchaka, I could not cleanly backport this to |
|
Sorry, @serhiy-storchaka, I could not cleanly backport this to |
…ct (pythonGH-112770) It was raised in two cases: * in the import statement when looking up __import__ * in pickling some builtin type when looking up built-ins iter, getattr, etc. (cherry picked from commit 1161c14) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
|
GH-113103 is a backport of this pull request to the 3.12 branch. |
…ct (pythonGH-112770) It was raised in two cases: * in the import statement when looking up __import__ * in pickling some builtin type when looking up built-ins iter, getattr, etc. (cherry picked from commit 1161c14) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
|
GH-113105 is a backport of this pull request to the 3.11 branch. |
…honGH-112770) It was raised in two cases: * in the import statement when looking up __import__ * in pickling some builtin type when looking up built-ins iter, getattr, etc.
…honGH-112770) It was raised in two cases: * in the import statement when looking up __import__ * in pickling some builtin type when looking up built-ins iter, getattr, etc.
…honGH-112770) It was raised in two cases: * in the import statement when looking up __import__ * in pickling some builtin type when looking up built-ins iter, getattr, etc.