[issue24912] The type of cached objects is mutable

Change by Erlend Egeberg Aasland : -- nosy: +erlendaasland nosy_count: 17.0 -> 18.0 pull_requests: +24435 pull_request: https://github.com/python/cpython/pull/25714 ___ Python tracker

[issue24912] The type of cached objects is mutable

STINNER Victor added the comment: Update: a new Py_TPFLAGS_IMMUTABLETYPE flag was added in bpo-43908, and it's now checked rather than relying on Py_TPFLAGS_HEAPTYPE to decide if a type is "mutable" or not. -- nosy: +ned.deily ___ Python tracker

[issue24912] The type of cached objects is mutable

Changes by Arfrever Frehtes Taifersar Arahesis : -- nosy: +Arfrever ___ Python tracker ___

[issue24912] The type of cached objects is mutable

Larry Hastings added the comment: Because the BDFL asked that it be so. -- ___ Python tracker ___ ___

[issue24912] The type of cached objects is mutable

Mark Shannon added the comment: Why has the change allowing the __class__ attribute of modules been merged? It is not necessary (as I have stated repeatedly) and not known to be safe. -- ___ Python tracker

[issue24912] The type of cached objects is mutable

Mark Shannon added the comment: Well, there are important reasons why not to make the __class__ attribute of modules mutable. First of all, there is no valid rationale. How do know for sure that modifying the class of the sys or builtins module is not going to cause much the same problems

[issue24912] The type of cached objects is mutable

Mark Shannon added the comment: Guido, I just think this change is misguided. The original rationale for the change just doesn't hold water. If you want the change anyway, then fine, but this seems an odd way to introduce it. -- status: closed -> open

[issue24912] The type of cached objects is mutable

Nathaniel Smith added the comment: I didn't want to get further into the weeds of debating basic design points on the tracker while blocking rc3, but, for the record -- the proposal in msg249504 that one could keep the namespace of an old and new module object effectively in sync by defining

[issue24912] The type of cached objects is mutable

Eric Snow added the comment: While I recognize the practicality/purity argument here, I somewhat agree with Mark. Assigning to module.__class__ makes sense for the use case, but it does open up a number of negative possible side effects (e.g. changing sys.__class__). Ideally it would be

[issue24912] The type of cached objects is mutable

Mark Shannon added the comment: Oh, and has anyone considered the potential impact this might have on Jython or PyPy? Modules are quite fundamental to the way that Python works. -- ___ Python tracker

[issue24912] The type of cached objects is mutable

Guido van Rossum added the comment: Mark, please calm down. Modules are incredibly simple objects compared to classes -- and yet you can change the __class__ of a class. You can also directly modify the __dict__ of a module. You can shoot yourself in the foot phenomenally this way, but that's

[issue24912] The type of cached objects is mutable

Roundup Robot added the comment: New changeset 27cc5cce0292 by Guido van Rossum in branch '3.5': Issue #24912: Prevent __class__ assignment to immutable built-in objects. https://hg.python.org/cpython/rev/27cc5cce0292 New changeset 1c55f169f4ee by Guido van Rossum in branch '3.5': Issue #24912:

[issue24912] The type of cached objects is mutable

Guido van Rossum added the comment: Oh. I feel dumb now. I guess I'll let Larry choose. If it's just a matter of comment text we can always improve it in 3.5.1. -- ___ Python tracker

[issue24912] The type of cached objects is mutable

Guido van Rossum added the comment: I'm creating the PR for Larry. -- ___ Python tracker ___ ___

[issue24912] The type of cached objects is mutable

Nathaniel Smith added the comment: There's already a PR on bitbucket that I made, in case that's helpful. The only difference from the v2 patch are that I rephrased some of the comment in a more neutral way (based on feedback from Mark posted on the PR itself), and added a NEWS entry. It

[issue24912] The type of cached objects is mutable

Guido van Rossum added the comment: PR: https://bitbucket.org/larry/cpython350/pull-requests/15/issue-24912-prevent-__class__-assignment/diff -- assignee: -> larry resolution: -> fixed ___ Python tracker

[issue24912] The type of cached objects is mutable

Nathaniel Smith added the comment: Sorry, you were too quick for me :-) -- ___ Python tracker ___ ___

[issue24912] The type of cached objects is mutable

Guido van Rossum added the comment: We should have something for rc3, which is imminent. I vote for (1) and (2) if Serhiy thinks the patch is ready. -- ___ Python tracker

[issue24912] The type of cached objects is mutable

Mark Shannon added the comment: 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. --

[issue24912] The type of cached objects is mutable

Nick Coghlan added the comment: I've filed issue 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

[issue24912] The type of cached objects is mutable

Larry Hastings added the comment: Mark, Victor, Benjamin: how do you feel about v2 patch vs rolling back the change entirely? -- ___ Python tracker ___

[issue24912] The type of cached objects is mutable

Nathaniel Smith added the comment: 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

[issue24912] The type of cached objects is mutable

Nick Coghlan added the comment: 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

[issue24912] The type of cached objects is mutable

Nathaniel Smith added the comment: > 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

[issue24912] The type of cached objects is mutable

Nathaniel Smith added the comment: Pull request version of patch is here: https://bitbucket.org/larry/cpython350/pull-requests/9/fix-issue-24912-disallow-reassigning-the/diff (identical to issue24912-v2.patch except for the addition of a NEWS entry) --

[issue24912] The type of cached objects is mutable

Guido van Rossum added the comment: OK, then I think it's between you and Serhiy. -- ___ Python tracker ___

[issue24912] The type of cached objects is mutable

Guido van Rossum added the comment: 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

[issue24912] The type of cached objects is mutable

Mark Shannon added the comment: 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,

[issue24912] The type of cached objects is mutable

Guido van Rossum added the comment: 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

[issue24912] The type of cached objects is mutable

Guido van Rossum added the comment: 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. -- ___ Python tracker

[issue24912] The type of cached objects is mutable

Serhiy Storchaka added the comment: 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,

[issue24912] The type of cached objects is mutable

Eugene Toder added the comment: 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

[issue24912] The type of cached objects is mutable

Nathaniel Smith added the comment: 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 :-(

[issue24912] The type of cached objects is mutable

Nathaniel Smith added the comment: Doh, apparently I did once know about issue23726 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

[issue24912] The type of cached objects is mutable

Marc-Andre Lemburg added the comment: 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

[issue24912] The type of cached objects is mutable

Marc-Andre Lemburg added the comment: 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

[issue24912] The type of cached objects is mutable

Antoine Pitrou added the comment: > 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.

[issue24912] The type of cached objects is mutable

Raymond Hettinger added the comment: 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

[issue24912] The type of cached objects is mutable

Nathaniel Smith added the comment: 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

[issue24912] The type of cached objects is mutable

STINNER Victor added the comment: 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

[issue24912] The type of cached objects is mutable

Eugene Toder added the comment: 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

[issue24912] The type of cached objects is mutable

Guido van Rossum added the comment: 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

[issue24912] The type of cached objects is mutable

Nathaniel Smith added the comment: 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

[issue24912] The type of cached objects is mutable

Nathaniel Smith added the comment: 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

[issue24912] The type of cached objects is mutable

Marc-Andre Lemburg added the comment: 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.

[issue24912] The type of cached objects is mutable

Marc-Andre Lemburg added the comment: 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

[issue24912] The type of cached objects is mutable

Serhiy Storchaka added the comment: 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

[issue24912] The type of cached objects is mutable

Marc-Andre Lemburg added the comment: 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

[issue24912] The type of cached objects is mutable

Nathaniel Smith added the comment: 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

[issue24912] The type of cached objects is mutable

Antoine Pitrou added the comment: __class__ assignment can definitely be useful for monkey-patching, or other, purposes. -- ___ Python tracker ___

[issue24912] The type of cached objects is mutable

Nathaniel Smith added the comment: 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

[issue24912] The type of cached objects is mutable

Serhiy Storchaka added the comment: What if just add settable __class__ property in ModuleType? -- ___ Python tracker ___

[issue24912] The type of cached objects is mutable

Serhiy Storchaka added the comment: The first time this bug was discovered in issue23726. -- ___ Python tracker ___

[issue24912] The type of cached objects is mutable

Changes by Serhiy Storchaka : -- nosy: +eltoder ___ Python tracker ___ ___

[issue24912] The type of cached objects is mutable

Benjamin Peterson added the comment: Probably the patch on that bug should be reverted. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue24912 ___

[issue24912] The type of cached objects is mutable

Nathaniel Smith added the comment: 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

[issue24912] The type of cached objects is mutable

Serhiy Storchaka added the comment: 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

[issue24912] The type of cached objects is mutable

Mark Shannon added the comment: 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

[issue24912] The type of cached objects is mutable

Mark Shannon added the comment: 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

[issue24912] The type of cached objects is mutable

Nathaniel Smith added the comment: 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

[issue24912] The type of cached objects is mutable

Nathaniel Smith added the comment: 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

[issue24912] The type of cached objects is mutable

Mark Shannon added the comment: 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? -- ___ Python tracker

[issue24912] The type of cached objects is mutable

Larry Hastings added the comment: This exciting new feature was added in checkin c0d25de5919e addressing issue #22986. Perhaps the core devs who added it would like to chime in. -- nosy: +benjamin.peterson, njs ___ Python tracker

[issue24912] The type of cached objects is mutable

Changes by Ned Deily n...@acm.org: -- nosy: +larry ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue24912 ___ ___ Python-bugs-list mailing list

[issue24912] The type of cached objects is mutable

Larry Hastings added the comment: As Python 3.5 Release Manager, my official statement is: Eek! -- priority: high - release blocker ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue24912 ___

[issue24912] The type of cached objects is mutable

Mark Shannon added the comment: If there is another issue for this, then it doesn't seem to be a release blocker. I think it should be. -- nosy: +Mark.Shannon ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue24912