gh-112716: Fix SystemError when __builtins__ is not a dict by serhiy-storchaka · Pull Request #112770 · python/cpython

serhiy-storchaka · 2023-12-05T18:44:11Z

Issue: SystemError when builtins is not a dict + eval #112716

Eclips4

LGTM

vstinner · 2023-12-05T23:32:27Z

Lib/test/test_builtin.py

+        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")


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")

It's testing __reduce__ on a list iterator specifically because the implementation of __reduce__ accesses PyEval_GetBuiltins. Your test wouldn't do that.

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.

That's a good point, might be better to pass iter([1, 2]) as one of the entries in the builtins.

JelleZijlstra · 2023-12-06T03:30:56Z

Python/ceval.c

 {
    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) {


Is it worth adding a fast path for the case where PyDict_CheckExact(builtins)?

PyMapping_GetOptionalItem() already has a fast path for PyDict_CheckExact.

Backports will be more complicated.

PyMapping_GetOptionalItem() has a fast-path for PyDict_CheckExact(). IMO it's enough.

JelleZijlstra · 2023-12-06T03:31:30Z

Lib/test/test_builtin.py

        self.assertRaisesRegex(NameError, "name 'superglobal' is not defined",
                               exec, code, {'__builtins__': customdict()})

+    def test_exec_builtins_mapping(self):


Could you also add some test cases where the builtins are a subclass of dict, ensuring that an overridden __getitem__ gets used?

It is covered by test_exec_globals_error_on_get.

vstinner

LGTM. I just suggested to enhance test_exec_builtins_mapping() to ensure that "superglobal" value is correct.

Lib/test/test_builtin.py

vstinner · 2023-12-06T08:37:18Z

Lib/test/test_builtin.py

+    def test_exec_builtins_mapping(self):
+        code = compile("superglobal", "test", "exec")
+        # works correctly
+        exec(code, {'__builtins__': types.MappingProxyType({'superglobal': 1})})


As I wrote previously, I would prefer to check for the expected superglobal value here.

Do you want to change also existing tests?

I'm only proposing to change this test.

miss-islington-app · 2023-12-14T12:24:28Z

Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖

miss-islington-app · 2023-12-14T12:24:31Z

Sorry, @serhiy-storchaka, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 1161c14e8c68296fc465cd48970b32be9bee012e 3.12

miss-islington-app · 2023-12-14T12:24:45Z

Sorry, @serhiy-storchaka, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 1161c14e8c68296fc465cd48970b32be9bee012e 3.11

…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>

bedevere-app · 2023-12-14T12:36:40Z

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>

bedevere-app · 2023-12-14T12:44:18Z

GH-113105 is a backport of this pull request to the 3.11 branch.

…-112770) (GH-113103) 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)

…-112770) (GH-113105) 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)

…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.

pythongh-112716: Fix SystemError when __builtins__ is not a dict

ae68655

serhiy-storchaka added needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Dec 5, 2023

serhiy-storchaka requested a review from JelleZijlstra December 5, 2023 18:44

serhiy-storchaka requested review from gvanrossum and markshannon as code owners December 5, 2023 18:44

bedevere-app bot added the awaiting core review label Dec 5, 2023

bedevere-app bot mentioned this pull request Dec 5, 2023

SystemError when builtins is not a dict + eval #112716

Closed

Eclips4 approved these changes Dec 5, 2023

View reviewed changes

vstinner reviewed Dec 5, 2023

View reviewed changes

JelleZijlstra reviewed Dec 6, 2023

View reviewed changes

Fix test_eval_builtins_mapping_reduce.

734811c

vstinner reviewed Dec 6, 2023

View reviewed changes

serhiy-storchaka added 2 commits December 6, 2023 10:45

Add a comment.

cdbf8e1

Use eval().

e0834e0

serhiy-storchaka merged commit 1161c14 into python:main Dec 14, 2023

bedevere-app bot removed the awaiting core review label Dec 14, 2023

serhiy-storchaka deleted the builtins-mapping branch December 14, 2023 12:24

miss-islington-app bot assigned serhiy-storchaka Dec 14, 2023

bedevere-app bot removed the needs backport to 3.12 only security fixes label Dec 14, 2023

bedevere-app bot removed the needs backport to 3.11 only security fixes label Dec 14, 2023

serhiy-storchaka removed their assignment Dec 14, 2023

Uh oh!

Conversation

serhiy-storchaka commented Dec 5, 2023 • edited by bedevere-app bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Eclips4 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miss-islington-app bot commented Dec 14, 2023

Uh oh!

miss-islington-app bot commented Dec 14, 2023

Uh oh!

miss-islington-app bot commented Dec 14, 2023

Uh oh!

bedevere-app bot commented Dec 14, 2023

Uh oh!

bedevere-app bot commented Dec 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

serhiy-storchaka commented Dec 5, 2023 •

edited by bedevere-app bot

Loading