Title: Support other types than dict for __builtins__
Type: enhancement Stage:
Components: Interpreter Core Versions: Python 3.3
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: haypo, mjpieters, pitrou, python-dev
Priority: normal Keywords: patch

Created on 2012-03-22 01:35 by haypo, last changed 2012-09-01 17:37 by mjpieters. This issue is now closed.

File name Uploaded Description Edit
builtins-3.patch haypo, 2012-04-15 23:49 review
builtins-4.patch haypo, 2012-04-18 11:54 review
Messages (12)
msg156534 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2012-03-22 01:35
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 #14383 patch (identifier.patch) for the __build_class__ identifier. I can write a new patch without this dependency if needed.
msg156537 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2012-03-22 01:50
See the issue #14386 which exposes dictproxy as a public type.
msg156539 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2012-03-22 02:03
Example combining patches of #14385 and #14386 to run code with read-only __builtins__:
----------- -------------
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 -----

$ ./python
Traceback (most recent call last):
  File "", 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.
msg156552 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2012-03-22 12:38
> 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.
msg156791 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2012-03-25 23:59
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 #14383 is done before writing a new patch.
msg157573 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2012-04-05 12:09
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)
msg158379 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2012-04-15 23:49
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.
msg158606 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2012-04-18 11:54
-                assert(!builtins || PyDict_Check(builtins));
+                assert(!builtins);

Oops, the assert must be replaced with assert(builtins != NULL) -> fixed in patch version 4.
msg158679 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-04-18 22:26
This looks fine.
msg158681 - (view) Author: Roundup Robot (python-dev) Date: 2012-04-18 23:00
New changeset e3ab8aa0216c by Victor Stinner in branch 'default':
Issue #14385: Support other types than dict for __builtins__
msg169650 - (view) Author: Martijn Pieters (mjpieters) Date: 2012-09-01 17:35
I note that the documentation still states a dictionary is required for globals. Should that not be updated as well?

msg169651 - (view) Author: Martijn Pieters (mjpieters) Date: 2012-09-01 17:37
Apologies, I meant to link to the dev docs:
Date User Action Args
2012-09-01 17:37:01mjpieterssetmessages: + msg169651
2012-09-01 17:35:55mjpieterssetnosy: + mjpieters
messages: + msg169650
2012-04-18 23:02:25hayposetstatus: open -> closed
resolution: fixed
2012-04-18 23:00:20python-devsetnosy: + python-dev
messages: + msg158681
2012-04-18 22:26:39pitrousetmessages: + msg158679
2012-04-18 11:54:07hayposetfiles: + builtins-4.patch

messages: + msg158606
2012-04-15 23:53:11hayposetnosy: + pitrou
2012-04-15 23:49:45hayposetfiles: - builtins-2.patch
2012-04-15 23:49:24hayposetfiles: + builtins-3.patch

messages: + msg158379
2012-04-15 22:31:17hayposetfiles: - builtins.patch
2012-04-05 12:09:41hayposetfiles: + builtins-2.patch

messages: + msg157573
2012-03-25 23:59:41hayposetmessages: + msg156791
2012-03-22 12:38:15hayposetmessages: + msg156552
2012-03-22 02:03:35hayposetmessages: + msg156539
2012-03-22 01:50:27hayposetmessages: + msg156537
2012-03-22 01:35:34haypocreate