Support other types than dict for __builtins__

@mjpieters

BPO	14385
Nosy	@mjpieters, @pitrou, @vstinner
Files	builtins-3.patch builtins-4.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 = None
closed_at = <Date 2012-04-18.23:02:25.520>
created_at = <Date 2012-03-22.01:35:33.441>
labels = ['interpreter-core', 'type-feature']
title = 'Support other types than dict for __builtins__'
updated_at = <Date 2019-03-06.01:35:04.886>
user = 'https://github.com/vstinner'

bugs.python.org fields:

activity = <Date 2019-03-06.01:35:04.886>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2012-04-18.23:02:25.520>
closer = 'vstinner'
components = ['Interpreter Core']
creation = <Date 2012-03-22.01:35:33.441>
creator = 'vstinner'
dependencies = []
files = ['25230', '25257']
hgrepos = []
issue_num = 14385
keywords = ['patch']
message_count = 14.0
messages = ['156534', '156537', '156539', '156552', '156791', '157573', '158379', '158606', '158679', '158681', '169650', '169651', '337245', '337262']
nosy_count = 5.0
nosy_names = ['mjpieters', 'pitrou', 'vstinner', 'python-dev', 'Kevin Shweh']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = None
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue14385'
versions = ['Python 3.3']

CPython expects __builtins__ to be a dict, but it is interesting to be able to use another type. For example, my pysandbox project (sandbox to secure Python) requires a read-only mapping for __builtins__.

The PEP-416 was rejected, so there is no builtin frozendict type, but it looks like the dictproxy type will be exposed as a public type.

Attached patch uses PyDict_CheckExact() to check if __builtins__ is a dict and add a "slow-path" for other types. The overhead on runtime performance should be very low (near zero), PyDict_CheckExact() just dereference a pointer (to read the object type) and compare two pointers.

The patch depends on issue bpo-14383 patch (identifier.patch) for the __build_class__ identifier. I can write a new patch without this dependency if needed.

See the issue bpo-14386 which exposes dictproxy as a public type.

Example combining patches of bpo-14385 and bpo-14386 to run code with read-only __builtins__:
----------- test.py -------------

ns={'__builtins__': __builtins__.__dict__}
exec(compile("__builtins__['superglobal']=1; print(superglobal)", "test", "exec"), ns)
ns={'__builtins__': dictproxy(__builtins__.__dict__)}
exec(compile("__builtins__['superglobal']=2; print(superglobal)", "test", "exec"), ns)
----------- end of test.py

Output:
--------

$ ./python test.py
1
Traceback (most recent call last):
  File "x.py", line 4, in <module>
    exec(compile("__builtins__['superglobal']=1; print(superglobal)", "test", "exec"), ns)
  File "test", line 1, in <module>
TypeError: 'dictproxy' object does not support item assignment

Note: this protection is not enough to secure Python, but it is an important part of a Python sandbox.

Note: this protection is not enough to secure Python,
but it is an important part of a Python sandbox.

Oh, and by the way, I workaround the lack of read-only mapping in pysandbox by removing dict methods: dict.__init__(), dict.clear(), dict.update(), etc. This is a problem because these methods are useful in Python.

With my patch, Python doesn't check __builtins__ type whereas ceval.c replaces any lookup error by a NameError. Example:

$ ./python 
Python 3.3.0a1+ (default:f8d01c8baf6a+, Mar 26 2012, 01:44:48) 
>>> code=compile("print('Hello World!')", "", "exec")
>>> exec(code,{'__builtins__': {'print': print}})
Hello World!
>>> exec(code,{'__builtins__': {}})
NameError: name 'print' is not defined
>>> exec(code,{'__builtins__': 1})
NameError: name 'print' is not defined

It should only replace the current exception by NameError if the current exception is a LookupError.

And my patch on LOAD_GLOBAL is not correct, it does still call PyDict_GetItem. I'm waiting until bpo-14383 is done before writing a new patch.

New version:

if __build_class__ is missing, raise a NameError instead of surprising ImportError
add tests
if PyObject_GetItem on __builtins__ or globals fail, only raise NameError if the exception is a KeyError

Before my patch, a new dict was created for builtins if __builtins__ exists in global but is not a dict. With my patch, the __builtins__ is kept and the type is checked at runtime. If __builtins__ is not a mapping, an exception is raised on lookup in ceval.c.

We may check __builtins__ type in PyFrame_New() using:

PyDict_Check(builtins) || (PyMapping_Check(mapping) && !PyList_Check(mapping) && !PyTuple_Check(mapping))

(PyDict_Check(builtins) is checked first for performance)

Oops, patch version 2 was not correct: I forgot a { ... } in ceval.c.

New patch fixing this issue but leaves also the LOAD_GLOBAL code unchanged : keep the goto and don't try to factorize the 3 last instructions. LOAD_GLOBAL is really critical in performance.

With patch version 3, the overall overhead is +0.4% according to pybench.

           assert(!builtins || PyDict_Check(builtins));

+ assert(!builtins);

Oops, the assert must be replaced with assert(builtins != NULL) -> fixed in patch version 4.

This looks fine.

New changeset e3ab8aa0216c by Victor Stinner in branch 'default':
Issue bpo-14385: Support other types than dict for __builtins__
http://hg.python.org/cpython/rev/e3ab8aa0216c

I note that the documentation still states a dictionary is required for globals. Should that not be updated as well?

See http://docs.python.org/py3k/library/functions.html#exec

Apologies, I meant to link to the dev docs:

http://docs.python.org/dev/library/functions.html#exec

The patch for this issue changed LOAD_GLOBAL to use PyObject_GetItem when globals() is a dict subclass, but LOAD_NAME, STORE_GLOBAL, and DELETE_GLOBAL weren't changed. (LOAD_NAME uses PyObject_GetItem for builtins now, but not for globals.)

This means that global lookup doesn't respect overridden __getitem__ inside a class statement (unless you explicitly declare the name global with a global statement, in which case LOAD_GLOBAL gets used instead of LOAD_NAME).

I don't have a strong opinion on whether STORE_GLOBAL or DELETE_GLOBAL should respect overridden __setitem__ or __delitem__, but the inconsistency between LOAD_GLOBAL and LOAD_NAME seems like a bug that should be fixed.

For reference, in the following code, the first 3 exec calls successfully print 5, and the last exec call fails, due to the LOAD_GLOBAL/LOAD_NAME inconsistency:

class Foo(dict):
    def __getitem__(self, index):
        return 5 if index == 'y' else super().__getitem__(index)
 
exec('print(y)', Foo())
exec('global y; print(y)', Foo())
exec('''
class UsesLOAD_NAME:
    global y
    print(y)''', Foo())
exec('''
class UsesLOAD_NAME:
    print(y)''', Foo())

This issues is now closed. Please open a new issue.

Navigation Menu

Repository navigation

Support other types than dict for builtins #58593

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Support other types than dict for __builtins__ #58593

Description

Activity

vstinner commented on Mar 22, 2012

vstinner commented on Mar 22, 2012

vstinner commented on Mar 22, 2012

vstinner commented on Mar 22, 2012

vstinner commented on Mar 25, 2012

vstinner commented on Apr 5, 2012

vstinner commented on Apr 15, 2012

vstinner commented on Apr 18, 2012

pitrou commented on Apr 18, 2012

python-dev commented on Apr 18, 2012

mjpieters commented on Sep 1, 2012

mjpieters commented on Sep 1, 2012

KevinShweh commented on Mar 5, 2019

vstinner commented on Mar 6, 2019

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions

Support other types than dict for builtins #58593