The type of cached objects is mutable

[serhiy-storchaka]

BPO	24912
Nosy	@malemburg, @gvanrossum, @brettcannon, @rhettinger, @ncoghlan, @pitrou, @vstinner, @larryhastings, @benjaminp, @ned-deily, @njsmith, @markshannon, @eltoder, @ericsnowcurrently, @serhiy-storchaka, @erlend-aasland
PRs	python/cpython#25714
Files	issue24912.patch issue24912-v2.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: ```python assignee = 'https://github.com/larryhastings' closed_at = created_at = labels = ['interpreter-core', 'type-bug', 'release-blocker'] title = 'The type of cached objects is mutable' updated_at = user = 'https://github.com/serhiy-storchaka' ``` bugs.python.org fields: ```python activity = actor = 'erlendaasland' assignee = 'larry' closed = True closed_date = closer = 'larry' components = ['Interpreter Core'] creation = creator = 'serhiy.storchaka' dependencies = [] files = ['40310', '40312'] hgrepos = [] issue_num = 24912 keywords = ['patch'] message_count = 69.0 messages = ['248981', '248982', '248983', '249019', '249180', '249189', '249198', '249345', '249350', '249351', '249354', '249357', '249360', '249363', '249369', '249388', '249392', '249393', '249396', '249397', '249398', '249438', '249446', '249450', '249455', '249456', '249457', '249467', '249468', '249470', '249471', '249473', '249483', '249484', '249489', '249495', '249499', '249500', '249502', '249504', '249505', '249506', '249509', '249510', '249511', '249604', '249607', '249618', '249623', '249685', '249854', '249869', '249871', '249888', '249889', '249890', '249891', '249893', '249930', '249931', '249934', '249938', '249940', '249943', '249944', '249948', '249954', '249979', '392411'] nosy_count = 18.0 nosy_names = ['lemburg', 'gvanrossum', 'brett.cannon', 'rhettinger', 'ncoghlan', 'pitrou', 'vstinner', 'larry', 'benjamin.peterson', 'ned.deily', 'Arfrever', 'njs', 'Mark.Shannon', 'python-dev', 'eltoder', 'eric.snow', 'serhiy.storchaka', 'erlendaasland'] pr_nums = ['25714'] priority = 'release blocker' resolution = 'fixed' stage = 'resolved' status = 'closed' superseder = None type = 'behavior' url = 'https://bugs.python.org/issue24912' versions = ['Python 3.5', 'Python 3.6'] ```

[serhiy-storchaka]

The type of non-heap types can be changed in 3.5. This means that the type of cached immutable objects such as small ints, empty or 1-character strings, etc can be changed.

>>> class I(int):
...     __slots__ = ()
...     def __repr__(self):
...         return 'Answer to The Ultimate Question of Life, the Universe, and Everything'
...     def __add__(self, other):
...         return self * other
... 
>>> (42).__class__ = I
>>> ord('*')
Answer to The Ultimate Question of Life, the Universe, and Everything
>>> x = 42; x + 2
84
>>> class S(str):
...     __slots__ = ()
...     def __len__(self):
...         return 123
... 
>>> 'a'.__class__ = S
>>> i = 1; len('abc'[:i])
123

[pitrou]

Isn't there already an issue open for this?

[serhiy-storchaka]

I don't remember. I noticed this once in comments on Rietveld, but don't remember the issue.

[markshannon]

If there is another issue for this, then it doesn't seem to be a release blocker. I think it should be.

[larryhastings]

As Python 3.5 Release Manager, my official statement is: Eek!

[markshannon]

Eek! indeed :)

I intend to track down the cause of this on Monday, and hopefully come up with a fix, unless anyone beats me to it.

Does anyone know if there is another issue for this?

[larryhastings]

This exciting new feature was added in checkin c0d25de5919e addressing issue bpo-22986. Perhaps the core devs who added it would like to chime in.

[njsmith]

Well, yeah, that indeed sucks.

Not sure what the best solution is. Some options:

1) "Don't do that then"

2) Explicitly add a "__class__" property to every immutable type, that unconditionally errors out on assignment.

3) Add a hack to typeobject.c checking for the important immutable types

4) Something cleverer...? A new type flag?

The immutable types are: int, float, str, tuple, bool, frozenset, complex, bytes, and... anything else?

[serhiy-storchaka]

Later I had got a crash.

>>> class S(str): __slots__ = ()
... 
>>> 'a'.__class__ = S
>>> 
>>> def f(a): pass
... 
Fatal Python error: non-string found in code slot

Current thread 0xb7583700 (most recent call first): Aborted (core dumped)

The stdlib is full of implicit caches. Virtually any hashable object can be cached and shared. Why __class assignment is allowed at all? There are only two uses of __class assignment in the stdlib besides tests (in Lib/importlib/util.py and in Lib/xml/sax/saxutils.py), and in both cases it looks as optimization trick.

[benjaminp]

Probably the patch on that bug should be reverted.

[njsmith]

Python goes to great lengths to make __class assignment work in general (I was surprised too). Historically the exception has been that instances of statically allocated C extension type objects could not have their __class reassigned. Semantically, this is totally arbitrary, and Guido even suggested that fixing this would be a good idea in this thread: https://mail.python.org/pipermail/python-dev/2014-December/137430.html The actual rule that's causing problems here is that *immutable* objects should not allow __class__ reassignment. (And specifically, objects which are assumed to be immutable by the interpreter. Most semantically immutable types in Python are defined by users and their immutability falls under the "consenting adults" rule; it's not enforced by the interpreter. Also this is very different from "hashable" -- even immutability isn't a requirement for being hashable, e.g., all user-defined types are mutable and hashable by default!)

By accident this category of "immutable-by-interpreter-invariant" has tended to be a subset of "defined in C using the old class definition API", so fixing the one issue uncovered the other.

This goal that motivated this patch was getting __class__ assignment to work on modules, which are mutable and uncacheable and totally safe. With this patch it becomes possible to e.g. issue deprecation warnings on module attribute access, which lets us finally deprecate some horrible global constants in numpy that have been confusing users for a decade now: http://mail.scipy.org/pipermail/numpy-discussion/2015-July/073205.html https://pypi.python.org/pypi/metamodule

I'd *really* prefer not to revert this totally, given the above. (Also, if you read that python-dev message above, you'll notice that the patch also caught and fixed a different really obscure interpreter-crashing bug.)

I think the Correct solution would be to disallow __class__ assignment specifically for the classes where it violates invariants.

If this is considered to be release critical -- and I'm not entirely sure it is, given that similar tricks have long been possible and are even referenced in the official docs? https://www.reddit.com/r/Python/comments/2441cv/can_you_change_the_value_of_1/ch3dwxt https://docs.python.org/3/c-api/long.html#c.PyLong_FromLong -- but if it is considered to be release critical, and it's considered too short notice to accomplish a proper fix, then a simple hack would be to add something like

if (!(oldto->tp_flags & Py_TPFLAGS_HEAPTYPE) && oldto != &PyModule_Type) {
    PyErr_Format(PyExc_TypeError, ...);
    return -1;
}

to typeobject.c:object_set_class. As compared to reverting the whole patch, this would preserve the most important case, which is one that we know is safe, and we could then progressively relax the check further in the future...

[markshannon]

Please revert c0d25de5919e. Breaking the interpreter in order to facilitate some obscure use case is unacceptable.

If you want to do fancy stuff with modules, fine. Take a look at the source of the six module https://bitbucket.org/gutworth/six/src/cd1e81d33eaf3d6944f859c2aa7d5d3f515013c8/six.py?at=default for some tips.

I think immutability is a fundamental property of an object. The "consenting" adults ideas is fine for accessibility. However, making previously immutable object mutable forces developers to use lots of defensive copying and causes obscure bugs (like this one).

I do not regard the immutable of numbers as an historical accident, but as vitally important for any sort of numerical reasoning. Just take a look at a language with mutable strings to see the problems that causes.

[njsmith]

Wow, Mark, why so hostile? What's so wrong with my proposed less-intrusive patch that you feel the need to call for reversion while condescendingly pointing me to a non-solution? Of course I know about six. There was a whole python-dev thread about how neither its tricks nor any of the other tricks that python 3.4 allows actually do what we need.

Let me try again.

We all agree that this bug is a bug and that numbers should be immutable.

Our old rules for __class__ assignment were also buggy; it was just an accident that they were buggy in a way that happened to prevent a different bug, ie this one. The proper way to enforce the immutability of immutable builtins is to enforce the immutability of immutable builtins, not to enforce the immutability of a bunch of random types based on an implementation detail (that happens to include the immutable builtins). Reverting the patch gives us the latter, which is why I don't think it's the proper fix.

Now maybe we don't have time for a proper fix. I'm not sure why not -- AFAICT there would not be any disaster if this fix waited for 3.5.1. This is a scary looking bug, but the effect is that it takes something that used to require 3 obscure lines involving ctypes and makes it into 1 obscure line not involving ctypes. Which is bad. But we're hardly going to see an epidemic of people using this loophole to change the type of 1 and then complaining to python-dev that they changed the type of 1, any more than we saw such an epidemic when ctypes was introduced. So we have time to take a deep breath and come up with a proper fix, is all I'm saying. But of course this is Larry's call.

If it is crucial to get a fix in for 3.5.0, then the least intrusive solution is not to revert the patch wholesale, but rather to add a (hacky but safe) guard to object_set_class. The guard I suggested above is stricter than necessary for correctness, but it catches all the problems described in this bug report, and the cases where it's over-strict are all cases where 3.4 was also over-strict, so there's minimal chance of it causing any regressions.

Like I said, I'm not sure that's what we want to do. But if it is then I can throw a patch together in a few minutes.

[markshannon]

Nathaniel, I'm hostile to this patch remaining in the code base. I'm not hostile to you, sorry that I came across as that way.

The proper way to deal with issues like this is to revert the change and then create a new patch, otherwise it becomes impossible to revert the change if other problems emerge.

I agree that the bug in __class__ assignment should be fixed, but such a fix should be separate from adding any new features.

Also, I'm surprised that you assert that you can't do what your metamodule does, without ctypes. Your metamodule package is almost there.

Your statement "Depending on the relative order of the assignment to sys.modules and imports of submodules, you can end up with different pieces of code in the same program thinking that mymodule refers to one or the other of these objects." is true.

So, just make sure that you insert the new object into sys.modules *before* doing any imports or calls to code that could import your module and it will all work fine.

[larryhastings]

I will not ship 3.5.0 with this bug.

Consider for a moment Google App Engine. If GAE updated to 3.5 with this bug, users would now have the ability to inject code into other people's programs, because interned ints (and a couple other types) are shared across interpreters.

Reverting the patch gives us back the old behavior. Dismissing the old behavior as "buggy" is unconvincing; it was acceptable enough that it shipped with many versions of Python, and its behavior is predictable and within the boundaries of the language spec.

Nathaniel, I'm willing to consider fixes for this bug, if the other devs in this thread are satisfied with the fix. But I am *absolutely* leaning towards backing it out for 3.5. My goal is to ship high-quality software, and that means balancing new features against regressions and new exploits. We almost didn't find this in time before 3.5 shipped--not to spread FUD, but what other ramifications of this code are lurking in the object model, waiting to be discovered?

p.s. I would love it if someone would add a regression test that tried mutating fields on a bunch of interned objects. Certainly such a test would be a precondition of keeping this change.

[malemburg]

I agree with Mark. This feature opens up a security hole large enough to drive a train through.

Looking at the python-dev thread, the only motivation appears to be making module look more like classes. I'd suggest to propose a PEP for making changes to module objects rather than creating a backdoor which let's you implement those changes at the expense of putting the whole interpreter at risk.

IMO, .__class of static types should not be mutable. I can understand why heap types need this feature (to e.g. be able to copy objects without invoking any .__init() methods of unknown objects as is needed for unpickle style operations), but for static types the set of supported objects is limited and the consequences of calling their constructor is known.

[njsmith]

I need to get to bed so I'll finish up tomorrow, but FYI I have a working patch -- I just want to add some __bases assignment test cases to make Larry happy. (Apparently there are no test cases for __bases assignment at all currently... :-(.)

Before anyone panics about security issues, do keep in mind that the patch you're talking about reverting fixed a buffer overflow which I strongly suspect could be used to accomplish arbitrary code execution. This is not a big deal, because all it does it let you turn the ability to execute arbitrary Python code into the ability to execute arbitrary machine code. If this were the JVM then this would be a big deal, but for CPython it isn't -- there are many "vulnerabilities" like this that are included in CPython by default as features, because CPython does not even attempt to provide a secure sandbox. The bug described in the current issue is bad, but security-wise AFAIK it's less bad than arbitrary code execution: it lets you mess with code in other subinterpreters (which is already possible through other means), and it lets you trigger assert checks that abort the interpreter, but AFAICT it doesn't violate memory safety or allow arbitrary code execution.

[malemburg]

On 31.08.2015 10:44, Nathaniel Smith wrote:

Before anyone panics about security issues, do keep in mind that the patch you're talking about reverting fixed a buffer overflow which I strongly suspect could be used to accomplish arbitrary code execution. ... it lets you trigger assert checks that abort the interpreter, but AFAICT it doesn't violate memory safety or allow arbitrary code execution.

I'm sure a buffer overflow can be fixed in other ways than allowing 42 to print out the Zen of Python when asked for a repr() ;-)

And if Serhiy can sneak in an os.system('rm -rf /') into a harmless operation such as 42 + 2, I do believe we can call this arbitrary code execution, even more so, since the patch only applies to a single integer object which happens to be a singleton in CPython.

The point is: Python code will generally assume that it can trust builtin types. It doesn't expect 42 + 2 to clear out the root dir, just because some package installed from PyPI happens to feel in the mood for Easter eggs :-)

[serhiy-storchaka]

I agree with Nathaniel, that this bug is not so critical to be release blocker. While it definitely should be fixed, it may wait for 3.5.1. Bug reproducing is not data driven, it needs executing special Python code, and when arbitrary Python code execution is available, there are a lot of other way to crash or compromise the interpreter. But I'm not sure that allowing __class assignment for larger domain of types is desirable. If we will desire that it is not, any enhancements to __class assignment should be withdrawn. May be __class assignment should be discouraged, deprecated and then disabled for all classes (in 3.6+), and other ways should be proposed to solve problems that are solved with __class assignment.

Nathaniel, can you provide a patch, that keeps the fix of a buffer overflow, but withdraws the ability to assign __class__ in cases that were disabled before?

[pitrou]

__class__ assignment can definitely be useful for monkey-patching, or other, purposes.

[malemburg]

On 31.08.2015 11:55, Antoine Pitrou wrote:

__class__ assignment can definitely be useful for monkey-patching, or other, purposes.

The feature certainly has its place for user defined types (see the unpickle example), but I don't think we should allow this for static types.

[njsmith]

Actually, I fail at grep, and actually there are already tons of good tests for __bases__ assignment in test_descr.py. So, patch attached, and analysis follows.

Background on __class__ assignment ----------------------------------

While it's certainly surprising, assignment to obj.__class and to type.__bases has been an explicitly supported feature ever since the advent of new-style classes back in 2.ancient, and typeobject.c goes to great lengths to handle all the weird resulting cases (including stuff like "what if __del changes the __class", "what if someone mutates __bases__ while we are in the middle of method lookup inside a metaclass's mro() method", etc.).

To make this safe, of course, we need to somehow check that after making the assignment then the resulting object will still be a valid, self-consistent Python object. The types in question need to have consistent in-memory representations, that their __dict__ slot (if present) is at the same offset, that their memory management callbacks are consistent (if we mutate an object from type A to type B, then A and B must be written in such a way that it's legal to pass an object allocated with A.tp_new to B.tp_dealloc), etc.

This compatibility checking occurs in two places: object_set_class and type_set_bases (and several utility functions that they share).

For historical reasons, there are two different C representations for type objects in Python: the original representation, which involves statically-allocated structs, and the newer representation, which involves heap-allocated structs. Aside from their allocation, there are a bunch of arcane little differences in things like reference counting, how you set them, etc., which are mostly motivated by the need to maintain compatibility with old code using the original representation -- all the places where they differ are places where the newer representation does something more sensible, it's not possible to create a statically-allocated type except by writing C code that *doesn't* use the stable ABI, etc. Otherwise they're supposed to work the same (eliminating differences between built-in and user-defined classes was the whole point of new-style classes!), but all these quirky implementation differences do leak out into little semantic differences in various places.

One of the places where they've historically leaked out is in __class and __bases assignment. When the compatibility-checking code for types was originally written, it punted on trying to handle the quirky differences between statically-allocated and heap-allocated type objects, and just declared that all statically-allocated types were incompatible with each other.

The previous patch ------------------

The patch that is causing all the debate here was reviewed in bpo-22986, after extensive discussion on python-dev, and the actual diff can be seen here: https://hg.python.org/cpython/rev/c0d25de5919e/

It did two things:

1) It cleaned up a bunch of nasty stuff in the compatibility-checking and assignment code. This fixed some real bugs (e.g., a case where incompatible classes could be considered compatible based on the contents of random memory found by following bogus pointers), and in the process made it robust enough to handle all types, both statically-allocated and heap-allocated.

2) It removed the check in object_set_class that protected the downstream code from ever seeing statically-allocated types - that's the 'if' check here: https://hg.python.org/cpython/rev/c0d25de5919e/#l3.97

As far as we know, this is all working exactly as designed! For example, Serhiy's int subclass at the beginning of the thread is compatible with the int class in the sense that the modified version of the '42' object is still a valid Python object, all its fields are valid, it won't crash the interpreter when deallocated, etc.

But of course what we didn't think of is that the interpreter assumes that instances of some particular built-in types are truly immutable, and the interpreter's internal invariants are violated if you can in any way modify an 'int' object in place. Doh.

This new patch --------------

So the attached patch modifies the original patch by leaving the cleanups in place, but puts back a guard in object_setclass that disallows \_class assignment for statically-allocated types EXCEPT that it still allows it specifically in the case of ModuleType subclasses. It also adds a big comment explaining the purpose of the check, and adds tests to make sure that __class assignment is disallowed for builtin immutable types.

Analysis of the mergeability of this patch:

• We still believe that the actual compatibility checking code is strictly better than it used to be, so in any case where these parts of the changes affect Python's semantics, the new results should be better (i.e. less likely to break memory safety and crash the interpreter).

• The new guard added by this patch is conservative, but allows a strict superset of what 3.4 and earlier allowed, so it won't break any existing code.

• The one way that the proposed new guard is less conservative than the one in 3.4 is that the new one allows ModuleType objects through. Since we believe that the compatibility-checking code is now correct for statically-allocated types, this should be fine from a memory-safety point of view, and because the interpreter definitely does not assume that ModuleType objects are immutable, then this should be safe from that perspective as well.

Larry also asked for a "regression test that tried mutating fields on a bunch of interned objects". I'm not sure which fields he was thinking of in particular, but the only entry points that lead to code touched by the original patch are attempts to set __class or to set __bases. *basesassignment has always been illegal on statically-allocated types, and has remained illegal through all of these patches*, plus it already has a bunch of tests, so I left it alone. This patch adds tests for __class__ assignment on all the potentially-interned types that I could think of.

So..... I think that covers all the bases about what this patch is and why it is appropriate for 3.5.0 (assuming that Larry sticks with treating this as a release-critical bug). I'll post a follow-up with replies to a few specific side comments that people made.

[njsmith]

Mark Shannon wrote:

So, just make sure that you insert the new object into sys.modules *before* doing any imports or calls to code that could import your module and it will all work fine.

The problem with doing this is that you're now stuck managing two diverging namespaces: the one associated with your new object that other modules can see, and the one where your __init.py code is doing all those imports and calls. So if you want this to work then you have to totally rewrite your package's startup sequence, OR you have to insert some code like sys.modules[name].__dict.update(origmodule.\_dict) after *every line* of your __init.py, OR you have to do some really complicated global analysis of every module inside your package to figure out exactly what the state of these two namespaces is at each possible point during the startup sequence and prove that the divergences don't matter...

The key feature of the metamodule approach is that sys.modules["modname"].__dict is always the same object as your __init.py globals(), so there's no change of divergence and it can guarantee that merely enabling metamodule for an existing package will always be safe and have no behavioural effect (until you start using the new metamodule features). This guarantee is hugely important given that the first user will probably be numpy, which is a giant crufty package with millions of users.

I promise, we went over all of this on python-dev last year :-)

Mark Lemburg wrote:

Python code will generally assume that it can trust builtin types. It doesn't expect 42 + 2 to clear out the root dir, just because some package installed from PyPI happens to feel in the mood for Easter eggs :-)

The only reason that'd be possible though is because you went and ran some untrusted code with permissions allowing it to clear out the root dir -- the only way to set up this "exploit" is to run untrusted Python code. Basically you've handed someone a gun, and now you're worried because this patch gives them a new and particularly rube-goldbergian method for pulling the trigger...

Except it isn't even a new method; your nasty PyPI package can trivially implement this "easter egg" using only fully-supported features from the stdlib, in any version of Python since 2.5. Here's some nice portable code to do __class__ assignment while dodging *all* the checks in object_set_class:

  from ctypes import *
  def set_class(obj, new_class):
      ob_type_offset = object.__basicsize__ - sizeof(c_void_p)
      c_void_p.from_address(id(obj) + ob_type_offset).value = id(new_class)

I mean, obviously ctypes is nasty and breaks the rules, I'm not saying this justifies making __class__ assignment broken as well. But this bug is no more a *security* problem than the existence of ctypes is.

Larry Hasting wrote:

Consider for a moment Google App Engine. If GAE updated to 3.5 with this bug, users would now have the ability to inject code into other people's programs, because interned ints (and a couple other types) are shared across interpreters.

Okay, fair enough :-). On GAE this *would* be a security bug because GAE I guess runs an extensively modified and audited fork of Python that implements a full sandbox. I assume this is also why it took them \~2 years to upgrade to 2.7, and why they're shipping 3 year old versions of all their libraries, and why they're now starting to move people to a new setup using OS-level sandboxing instead of interpreter-level sandboxing...

Python.org doesn't provide any sandbox guarantees, and this bug is a tiny drop in the bucket compared to what anyone will need to do to add a trustworthy sandbox to CPython 3.5, so for me I still wouldn't call this release critical. But you're the RM, so here's a patch if you want it :-).

Serhiy Storchaka wrote;

I'm not sure that allowing __class assignment for larger domain of types is desirable. If we will desire that it is not, any enhancements to __class assignment should be withdrawn. May be __class assignment should be discouraged, deprecated and then disabled for all classes (in 3.6+), and other ways should be proposed to solve problems that are solved with __class assignment.

I don't necessarily object to the idea of eventually removing __class__ assignment in some future version of Python. It kind of freaks me out too. (Though Guido seems to like it.)

I really, really, REALLY object to the idea of -- at this point in the release cycle! -- rejecting a new feature that has gone through review on python-dev, that solves a real problem that's impacting a bunch of people (see all the replies on the numpy-discussion thread linked above of people saying "oh ugh yes this has totally bitten me! please fix it!"), and to do this on the grounds that someone *might* later make an argument for it be removed again in 3.7 and that python-dev might eventually agree with that argument? I mean, c'mon.

If it were breaking everything, then that would be grounds for removing it, no question there. But the problems described in this bug report are well understood, and it's trivial to fix them in a conservative way without backing out the original feature.

[njsmith]

On further thought, here's a slightly improved version of the patch I posted above.

The difference is that the first version allowed through attempted __class__ assignments where either the old or new class was a subclass of ModuleType; the new version only allows through attempted assignments if both the old AND new class are a subclass of ModuleType.

In practice this doesn't make any difference, because the compatibility-checking code will reject any attempt to switch from a ModuleType subclass to a non ModuleType subclass or vice-versa. So both patches are correct. But the new patch is more obviously correct.

[serhiy-storchaka]

The first time this bug was discovered in bpo-23726.

[serhiy-storchaka]

What if just add settable __class__ property in ModuleType?

[njsmith]

Doh, apparently I did once know about bpo-23726 and then forgot. Good catch.

Technically we could add a __class field to ModuleType directly, but I think then the ModuleType __class setter would basically need to be an exact reimplementation of everything that object_setclass already does? (Since it would still need to check that this particular ModuleType subclass was layout-compatible etc. -- e.g. no new \_slots__.) And the end result would behave identically to the patch I just posted, except we'd replace a 2 line check in typeobject.c with some really complicated and bug-prone code in moduleobject.c? I don't think it buys us anything.

[malemburg]

On 01.09.2015 04:38, Nathaniel Smith wrote:

Mark Lemburg wrote: > Python code will generally assume that it can trust > builtin types. It doesn't expect 42 + 2 to clear out the root dir, > just because some package installed from PyPI happens to feel in the > mood for Easter eggs :-)

The only reason that'd be possible though is because you went and ran some untrusted code with permissions allowing it to clear out the root dir -- the only way to set up this "exploit" is to run untrusted Python code. Basically you've handed someone a gun, and now you're worried because this patch gives them a new and particularly rube-goldbergian method for pulling the trigger...

I think you're being overly optimistic here. People run unchecked and unverified code all the time; that's not the same as untrusted, since trust develops with time and doesn't get reset with each new package release.

Regardless, the above was only an example. The much more likely thing to happen is that some code replaces the .__class__ of some unrelated object by accident via a bug and causes all hell to break loose.

Except it isn't even a new method; your nasty PyPI package can trivially implement this "easter egg" using only fully-supported features from the stdlib, in any version of Python since 2.5. Here's some nice portable code to do __class__ assignment while dodging *all* the checks in object_set_class:

from ctypes import * def set_class(obj, new_class): ob_typeoffset = object.\_basicsize__ - sizeof(c_void_p) c_void_p.from_address(id(obj) + ob_type_offset).value = id(new_class)

I mean, obviously ctypes is nasty and breaks the rules, I'm not saying this justifies making __class__ assignment broken as well. But this bug is no more a *security* problem than the existence of ctypes is.

You can disable ctypes easily. OTOH, your patch is inherently changing the language and making it less secure. There's no way to disable it or even prevent using it from inside Python. IMO, that's a huge difference.

I also believe that the overall approach is wrong: if you want to add a feature to Python module objects, please stick to those instead of changing the overall interpreter semantics.

Some more background:

Replacing .__class__ of class instances is a known and useful Python feature. However, in the past this was only possible for regular instances, not for types. With new style classes, this differentiation got blurred and in Python 3 we only have new style classes, so it may look like we always wanted this feature to be available. Yet, I'm not sure whether this was ever intended, or a conscious design decision and because it creates serious problems for the interpreter, it definitely needs go through a PEP process first to make everyone aware of the consequences.

[rhettinger]

Guido, do you have any thoughts on this?

Several of us (me included) think http://hg.python.org/lookup/c0d25de5919e probably should not have been done. Mutating non-heap types crosses an implicit boundary that we've long resisted crossing because it opens a can worms and has potential to violate our expectations about how the language works.

[Mark Shannon]

Breaking the interpreter in order to facilitate some obscure use case is unacceptable.

[Marc-Andre Lemburg] I agree with Mark. This feature opens up a security hole large enough to drive a train through.

[Benjamin Peterson] Probably the patch on that bug should be reverted.

[Larry Hastings] As Python 3.5 Release Manager, my official statement is: Eek!

[njsmith]

Thanks Raymond. Hi Guido. Sorry about the mess.

My overview of the situation so far, and of the patch currently attached to this bug, is here (and a bit in the next post): https://bugs.python.org/issue24912#msg249438

From where I sit this all looks like a massive overreaction/misunderstanding: I introduced a bug, the cause is obvious, and it's straightforward to fix. If we want to fix the bug in the most general manner then that's straightforward but probably more work than we want to do in rc3. If we want to just apply a conservative fix and move on then we have an 8 line patch attached to this bug that does that. Even if we don't apply the patch then there's no security hole, that's just factually incorrect. But it doesn't even matter, because again, we have a patch. If we do apply this patch, then literally the only outcome of all of this will be that __class assignment in 3.5 will be (1) less buggy in general, and (2) be allowed, safely, on instances of ModuleType subclasses. I get that the whole concept of __class assignment is kinda freaky and unnerving, but I do not understand why some people are insisting that the above needs a PEP...

You know, in fact, note to everyone: I hereby withdraw any suggestion that we might want to "fix" the __class__ assignment code to handle the general case beyond ModuleType. Obviously this is super controversial and distracting for reasons I don't really understand, but I don't have to -- the only reason to brought it up in the first place is out of a vague desire to make Python "better", and frankly all I care about at this point is that I don't want to wait another 2 years before I can safely deprecate module-level constants. So let's just apply the attached patch and move on? Nothing broken, no security hole, nothing to eek about.

[pitrou]

From where I sit this all looks like a massive overreaction/misunderstanding: I introduced a bug, the cause is obvious, and it's straightforward to fix.

I agree with Nathaniel here. Let's just commit the fix instead of acting like irrational beings.

[malemburg]

FWIW: I still think that allowing to change .__class__ on instances of static types if wrong and should be undone. If we want to make this a feature of the language we should have a PEP and the associated discussion to be able to judge and document the possible consequences of such a change. This ticket is the wrong place for such a discussion.

Regarding you module object change: Why don't you propose a change on the object itself instead of trying to monkey patch it via a mod.__class__ replacement ? E.g. by defining a hook in the object to set which then permits whatever modification you'd like to make.

[9e803d50-6238-47c1-84e6-b1d134a28aa9]

Nathaniel, what if we allow creating module objects from an existing dict instead of always creating a new one. Does this solve your namespace diversion problem?

FWIW, when I discovered this change I was quite surprised this came through without a PEP. I agree that the old rules were somewhat arbitrary, but this is a significant change with non-trivial compatibility concerns.

[gvanrossum]

I don't want to own this, but this is absolutely a release blocker. I see three ways out:

• Fix it right (if possible) -- "system immutable" types such as int should not allow __class__ assignment. Risk: there might be other cases (the code being patched is clearly too complex for humans to comprehend).

• Roll back the patch; I'm unclear on why Nathaniel would be so heartbroken if he couldn't assign the __class of a module (since there are other approaches such as assignment to sys.module[name__].

• Roll back the patch but replace it with a narrower patch that specifically allows __class__ assignment for modules (but not for other types).

But first, why is it so important to assign the __class__ of a module? It seems somebody is trying to make modules into what they weren't meant to be.

[vstinner]

Sorry, I don't have time to read the whole discussion. Since we are *very* close to 3.5 final, I agree with Mark:

"Please revert c0d25de5919e. Breaking the interpreter in order to facilitate some obscure use case is unacceptable."

We can reintroduce the feature in Python 3.6 with a longer discussion how to implement it, or ensure that we really want it.

[serhiy-storchaka]

The c0d25de5919e changeset consists of three parts:

1) Fixes a bug (disallow an assignment of incompatible __class that happens to have the same __basesize). 2) Allows __class assignment for module type. 3) Allows __class assignment for other types, in particular non-heap builtin types.

I think we should keep (1) and disallow (3). (2) is questionable and I for disallowing it too (special cases aren't special enough to break the rules). It is too little time left to 3.5 release, we will have more time to discuss this for 3.6.

Nataniel's new patch without special casing ModuleType would be good to me.

[9e803d50-6238-47c1-84e6-b1d134a28aa9]

Guido: IIUC the general intention is to support @property and __getattr__ style hooks on the module level. Assigning to sys.modules[name] from the module itself is a bit too late -- there's already a global dict for this module, and no way to create a module from an existing dict, so you end up with two global namespaces which is annoying (but admittedly people live with that). One way around that is to set up import hooks, but this is also annoying, and leaks module implementation outside of its code.

[gvanrossum]

If we don't reach an agreement we should fall back to Serhiy's (1) only.

Eugene: Without more motivation that sounds like very questionable functionality to want to add to modules. I don't have time to read the discussion that led up to this, but modules are *intentionally* dumb. We already have classes and instances.

[njsmith]

Guido van Rossum wrote:

But first, why is it so important to assign the __class__ of a module? It seems somebody is trying to make modules into what they weren't meant to be.

Because you told me to do it this way on python-dev :-(

https://mail.python.org/pipermail/python-dev/2014-December/137430.html

The goal is to make it possible for projects (e.g. numpy) to safely issue a deprecation warning when people access certain module-level constants.

The reason this is difficult is that we have several almost-conflicting requirements:

1) we want to be able to override special methods like __getattr and __dir on modules. And it'd nice if we could have access to things like properties and __repr__ too.

2) we can't control the type used to originally construct the module, because the module object is constructed before the first line of our code is run.

3) we want sys.modules[ourmodule].\_dict to always refer to the namespace where __init.py is executing, for reasons described at the top of msg249446. This is not currently possible when replacing the module object in-place -- if you make a new module object, then now you have the old module's namespace and the new module's namespace, and are responsible for manually keeping them in sync. (This is not a case where "let's do more of those" applies ;-).)

Other solutions that were previously considered (in two threads on python-dev and python-ideas with 40+ messages each) include:

• tackling (1) directly by defining a new set of special-purpose hooks just for modules (e.g. make ModuleType.__getattr check for a special __module_getattr function in the module namespace and call it). This is what Marc-Andre is suggesting now (msg249473). OTOH it would be nice to re-use the existing class mechanism instead of reimplementing parts of it just for modules.

• tackling (2) directly via wacky stuff like preparsing the file to check for special markers that tell the import machinery what ModuleType subclass to instantiate before the module starts executing (similar to how __future__ imports work)

• tackling (3) by adding some new special machinery to module objects, like the ability to replace their __dict__ attribute. This is what Eugene Toder is suggesting now (msg249483).

The advantage of allowing __class__ assignment on ModuleType instances is that it solves all these problems by using an existing feature rather than adding any new ones.

(I also tried the strategy of switching ModuleType to just *be* a heap type and avoid all these issues, but unfortunately it turns out that this would have broken the stable ABI so I gave up on that.)

The diff from 3.4 to 3.5rc2+the attached patch consists of uncontroversial bug fixes, plus two lines of code in typeobject.c that cause module subtypes to be treated similarly to heap types with respect to __class__ assignment:

-    if (!(newto->tp_flags & Py_TPFLAGS_HEAPTYPE) ||
-        !(oldto->tp_flags & Py_TPFLAGS_HEAPTYPE)) {
+    if (!(PyType_IsSubtype(newto, &PyModule_Type) &&
+          PyType_IsSubtype(oldto, &PyModule_Type)) &&
+        (!(newto->tp_flags & Py_TPFLAGS_HEAPTYPE) ||
+         !(oldto->tp_flags & Py_TPFLAGS_HEAPTYPE))) {

These two lines of code solve an important user-facing issue for numpy, and are far simpler than any of the other proposed solutions. This approach was reviewed by python-dev, and has stood through the entire pre-release cycle so far without anyone producing a single argument yet for why it will cause any problem whatsoever.

I'm very sorry for introducing the bug with immutable types, and for not initially addressing it with the seriousness that it deserved. But that bug is fixed now, and unless someone can name an actual problem with the above two lines then I don't see why their association with a now-fixed bug is any reason to go rehash the entire discussion from scratch.

[markshannon]

I think Nathaniel and Eugene argument that you cannot replace the module in sys.modules safely in erroneous.

Immediately after the module is created by importlib it is inserted into sys.modules. The code object for the module is then executed. At that point, when the module code starts executing, no code outside the module has executed since the module was created and the import lock is held so no other thread will be able to import the module. Therefore the only code that can see the module object is the module's own code.

Given a custom subclass of module that overrides __getattr to fetch values from the original module's __dict, then an instance of that class can be inserted into sys.modules before any imports or calls to code that would access sys.modules, and the substitution of the module can be done safely.

[gvanrossum]

I don't think I told you to do it this way -- that message of mind sounded pretty noncommittal in all directions.

I do understand your predicament. Can you live with just a special case for modules? __class__ assignment is full of non-portable special cases already.

Given how close we are to the release we should tread with a lot of care here -- rolling back a diff is also a big liability.

Maybe the "right" solution is to allow __dict__ assignment for modules. But we're too close to the release to add that. Perhaps we can revisit the right way to do it for 3.6?

[gvanrossum]

PS. I have very little time this week or next to review everything -- if we don't find a reasonable compromise it *will* be rolled back.

[njsmith]

I do understand your predicament. Can you live with just a special case for modules? __class__ assignment is full of non-portable special cases already.

Not only can I live with it, but -- unless I misunderstand your meaning -- this is exactly the approach that I am advocating and have submitted a patch to implement :-).

[gvanrossum]

OK, then I think it's between you and Serhiy.

[njsmith]

Pull request version of patch is here:

https://bitbucket.org/larry/cpython350/pull-requests/9/fix-issue-24912-disallow-reassigning-the/diff

(identical to bpo-24912-v2.patch except for the addition of a NEWS entry)

[ncoghlan]

I've reviewed the patch, and it looks good to me. Key additions:

• truly immutable types (instances of which may be cached) are now explicitly checked in the test suite to ensure they don't support __class__ assignment, even to a subclass

• the check for non-heaptypes in __class__ assignment has been restored, with a hardcoded whitelist to bypass that check. The only type on the whitelist for 3.5 is PyModuleType.

Thus, this patch will also serve to protect any extension types that were relying on the non-heaptype check to prevent __class__ assignment.

The patch also includes a long block comment before the restored check for non-heap types that explains the history and links out to this issue.

[ncoghlan]

I've filed bpo-24991 to suggest reviewing the wider question of how we deal with identifying whether or not a type's instances are expected to be "truly immutable" in the Python 3.6 time frame.

For 3.5, I think Nathaniel's proposed patch restoring the assumption that non-heap types are immutable by default, and whitelisting __class__ assignment for PyModuleType specifically is a good way to resolve this with minimal code churn late in the release cycle.

[larryhastings]

Mark, Victor, Benjamin: how do you feel about v2 patch vs rolling back the change entirely?

[njsmith]

Since I know there's a lot of text here for people to process, here's an attempted TL;DR (with inspiration from Serhiy's msg249495):

There were three parts to the original change: 1) Bug fixes for typeobject.c 2) Enabling __class assignment for module objects 3) Enabling __class assignment for other objects, in particular all non-heap types, including 'int', 'str', etc.

Everyone agrees we want to revert (3), which is the bit that has caused all the problems. And I don't think anyone has argued for reverting (1) after learning that it is separable from the rest. So the main question still under discussion is whether we want to revert (2)+(3), or just revert (3) alone.

(Either of these is easy to do; the attached "v2" patch implements the revert (3) alone option.)

My position:

When making changes for rc3 (!), we should definitely revert any functionality that is breaking things, and should definitely not revert any functionality that isn't.

The (3) change obviously meets this bar, so we'll revert it. But when it comes to module objects specifically -- the (2) change -- then (AFAICT) no-one has identified any problem with the functionality itself, even though it's been specifically discussed and reviewed multiple times over the last year, and been enabled all the way through the pre-release with explicit tests provided, etc. The only argument I see raised against it here is that there might be some more-or-less-equivalent but maybe-more-aesthetic way of accomplishing the same thing, so maybe we should revert it now so we can get in another round of bikeshedding. And if this had been raised a few months ago I'd have been sympathetic... but IMO the rc3 release is too late to be churning functionality based purely on aesthetics, so I think we should revert (3) alone, while leaving (1)+(2) in place.

[markshannon]

Larry, of the two choices, I prefer rolling back the change entirely.

I would like to see the bug fixes for typeobject.c applied, but I see no reason why making the __class__ of module objects assignable should be included.