msg156534 - (view) |
Author: STINNER Victor (vstinner) * |
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 (vstinner) * |
Date: 2012-03-22 01:50 |
See the issue #14386 which exposes dictproxy as a public type.
|
msg156539 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2012-03-22 02:03 |
Example combining patches of #14385 and #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.
|
msg156552 - (view) |
Author: STINNER Victor (vstinner) * |
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 (vstinner) * |
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 (vstinner) * |
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 (vstinner) * |
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 (vstinner) * |
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) * |
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__
http://hg.python.org/cpython/rev/e3ab8aa0216c
|
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?
See http://docs.python.org/py3k/library/functions.html#exec
|
msg169651 - (view) |
Author: Martijn Pieters (mjpieters) * |
Date: 2012-09-01 17:37 |
Apologies, I meant to link to the dev docs:
http://docs.python.org/dev/library/functions.html#exec
|
msg337245 - (view) |
Author: Kevin Shweh (Kevin Shweh) |
Date: 2019-03-05 23:54 |
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())
|
msg337262 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2019-03-06 01:35 |
This issues is now closed. Please open a new issue.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:57:28 | admin | set | github: 58593 |
2019-03-06 01:35:04 | vstinner | set | messages:
+ msg337262 |
2019-03-05 23:54:37 | Kevin Shweh | set | nosy:
+ Kevin Shweh messages:
+ msg337245
|
2012-09-01 17:37:01 | mjpieters | set | messages:
+ msg169651 |
2012-09-01 17:35:55 | mjpieters | set | nosy:
+ mjpieters messages:
+ msg169650
|
2012-04-18 23:02:25 | vstinner | set | status: open -> closed resolution: fixed |
2012-04-18 23:00:20 | python-dev | set | nosy:
+ python-dev messages:
+ msg158681
|
2012-04-18 22:26:39 | pitrou | set | messages:
+ msg158679 |
2012-04-18 11:54:07 | vstinner | set | files:
+ builtins-4.patch
messages:
+ msg158606 |
2012-04-15 23:53:11 | vstinner | set | nosy:
+ pitrou
|
2012-04-15 23:49:45 | vstinner | set | files:
- builtins-2.patch |
2012-04-15 23:49:24 | vstinner | set | files:
+ builtins-3.patch
messages:
+ msg158379 |
2012-04-15 22:31:17 | vstinner | set | files:
- builtins.patch |
2012-04-05 12:09:41 | vstinner | set | files:
+ builtins-2.patch
messages:
+ msg157573 |
2012-03-25 23:59:41 | vstinner | set | messages:
+ msg156791 |
2012-03-22 12:38:15 | vstinner | set | messages:
+ msg156552 |
2012-03-22 02:03:35 | vstinner | set | messages:
+ msg156539 |
2012-03-22 01:50:27 | vstinner | set | messages:
+ msg156537 |
2012-03-22 01:35:34 | vstinner | create | |