This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: Bypassing __dict__ readonlyness
Type: Stage:
Components: Interpreter Core Versions: Python 2.6, Python 2.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Rhamphoryncus, arigo, brett.cannon, gvanrossum, jhylton, mwh, nnorwitz, zseil
Priority: critical Keywords:

Created on 2005-09-24 23:40 by arigo, last changed 2022-04-11 14:56 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
test66.py arigo, 2005-09-24 23:40 obscure?
test67.py arigo, 2005-09-25 08:55 changing __dict__
deldict.py arigo, 2005-12-14 14:49
NULL_in_ref_forgetting.diff brett.cannon, 2006-06-29 17:41 Check for NULLs when forgetting a reference
modify_dict_bug.diff zseil, 2007-04-17 00:42 deldict fix
modify_dict_bug2.diff zseil, 2007-04-19 16:40 updated patch against trunk revision 54876
deldict.diff gvanrossum, 2008-01-23 21:45 Here's the second fix
Messages (25)
msg26375 - (view) Author: Armin Rigo (arigo) * (Python committer) Date: 2005-09-24 23:40
The __dict__ attribute of some objects is read-only,
e.g. for type objects.  However, there is a generic
way to still access and modify it (without hacking with
gc.get_referrents(), that is).  This can lead to
obscure crashes.  Attached is an example that shows
a potential "problem" involving putting strange keys
in the __dict__ of a type.

This is probably very minor anyway.  If we wanted to
fix this, we would need a __dict__ descriptor that
looks more cleverly at the object to which it is
applied.

BTW the first person who understand why the attached
program crashes gets a free coffee.
msg26376 - (view) Author: Armin Rigo (arigo) * (Python committer) Date: 2005-09-25 08:55
Logged In: YES 
user_id=4771

The bug is related to code like PyObject_GenericGetAttribute and _PyType_Lookup which are not careful about the reference counters of the objects they operate on.  This allows a much simpler crash (test67.py): the __dict__ of an object can be decrefed away while lookdict() is looking at it.  This has a simple fix -- drop some amount of Py_INCREF/Py_DECREF in core routines like PyObject_GenericGetAttr.  We probably need to measure if it has a performance impact, though.
msg26377 - (view) Author: Armin Rigo (arigo) * (Python committer) Date: 2005-12-14 14:49
Logged In: YES 
user_id=4771

Uh oh.  There is a much simpler crash.  The technique outlined in deldict.py can be used to rebind or even delete the __dict__ attribute of instances where it should normally not be allowed.
msg26378 - (view) Author: Armin Rigo (arigo) * (Python committer) Date: 2005-12-14 15:32
Logged In: YES 
user_id=4771

A proposed fix for the later crash: the __dict__ descriptor of user-defined classes should verify if there is another __dict__ descriptor along the solid path of the type (i.e. following "tp_base" pointers).  If so, it should delegate to it.
msg26379 - (view) Author: Michael Hudson (mwh) (Python committer) Date: 2005-12-14 15:36
Logged In: YES 
user_id=6656

Yikes!
msg26380 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2006-06-29 17:41
Logged In: YES 
user_id=357491

Simple patch for the loosing_dict_ref.py crasher is
attached.  Just checked for possible NULL values in what is
needed by _Py_ForgetReference().  Let me know what you
think, Armin.
msg26381 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2006-06-29 17:45
Logged In: YES 
user_id=357491

For the deldict.py crasher, if you look at the traceback
there is no good place to do a check that tp_dict is sane
except in type_module() or PyDict_GetItem().  Now adding the
NULL check in type_module() will fix this specific problem,
but that seems like an ad-hoc patch.

Why don't we check for NULL in PyDict_GetItem() and return
NULL just like the PyDict_Check() test?  I am sure the
answer is "performance", but this is not PyDict_GETITEM()and
thus already is not the performance-critical version anyway.
 So why shouldn't we check for NULL there and possibly catch
other errors?
msg26382 - (view) Author: Armin Rigo (arigo) * (Python committer) Date: 2006-06-29 21:19
Logged In: YES 
user_id=4771

Brett: I think you're approaching the problem from the wrong
angle.  The problem is being allowed to freely tamper with
the dict stored in objects.  Getting NULL errors here and
there is only a symptom.  As I explain, the '__dict__'
descriptor object needs to do some more checks, and to be
fully safe some Py_INCREF/Py_DECREF are needed in some
critical places.
msg26383 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2006-06-30 01:14
Logged In: YES 
user_id=357491

OK, then I am going to need a little guidance to dive into
this more since this is going into some murky waters for me.  =)

For the normal, non-evil case of an empty Python class
(``class Foo(object): pass``), I would think that accessing
__dict__ would fall through to the tp_getset for object, and
then fall to the same slot for type.  Now that is obviously
not happening since you get a straight dict back for that
attribute access and not a dict proxy as would be suggested
based on my logic listed above.

So, my question is, where is the code that handles fetching
Foo().__dict__?  And if you have an inkling of where I
should be looking in terms of possible refcounting additions
that would be great as well.
msg26384 - (view) Author: Armin Rigo (arigo) * (Python committer) Date: 2006-06-30 07:05
Logged In: YES 
user_id=4771

Well, try making an "empty" class Foo(object): pass, and see
what magically shows up in Foo.__dict__.keys().  Here is the
__dict__ descriptor used for instances of Foo.  This
descriptor is connected to subtype_dict() and
subtype_setdict() in typeobject.c.

INCREF/DECREFs are in theory missing around every use of the
dictionary returned by _PyObject_GetDictPtr(), because more
or less any such use could remove the dict from the object
from which _PyObject_GetDictPtr() returned from, and so
decref the dict - while it's used.  This might require some
timing, to check the performance impact.
msg26385 - (view) Author: Jeremy Hylton (jhylton) (Python triager) Date: 2007-02-25 22:23
I tried test67.py in IronPython.  It reports that x.hello has the value 456.  Is that the correct behavior?  It seems incorrect to me.  If the __eq__ method is called, then the object should no longer have either the Strange() or hello attributes.  They are cleared by reseting __dict__.  Is that right?
msg26386 - (view) Author: Jeremy Hylton (jhylton) (Python triager) Date: 2007-02-25 22:36
I tried test67.py in IronPython.  It reports that x.hello has the value 456.  Is that the correct behavior?  It seems incorrect to me.  If the __eq__ method is called, then the object should no longer have either the Strange() or hello attributes.  They are cleared by reseting __dict__.  Is that right?
msg26387 - (view) Author: Armin Rigo (arigo) * (Python committer) Date: 2007-02-26 08:23
456?  Now that's interesting.  Not 579?

I'm not sure if I ever thought about what the
expected answer should be, but I'd say that
you are correct: 'x.__dict__' should be empty
in the end.  Actually I don't really see how
it manages *not* to be empty in IronPython;
looks like either a very minor detail or a
deeper bug...
msg26388 - (view) Author: Ziga Seilnacht (zseil) * (Python committer) Date: 2007-04-17 00:42
Here is a patch that shold fix the deldict bug.  It also
removes the new condition for metaclasses, because it
isn't needed anymore, and fixes bug #1174712.  These two
changes were needed so that the patch could be properly
tested.

The patch does what Armin suggested; the __dict__ descriptor
looks for a builtin base with tp_dictoffset != 0, and if one
is found, it delegates to that base's __dict__ descriptor.

File Added: modify_dict_bug.diff
msg26389 - (view) Author: Armin Rigo (arigo) * (Python committer) Date: 2007-04-18 09:22
I'm skeptical about the whole revision 53997
which introduced not only the unneeded metaclass
condition, but also the strange check when
assigning to __bases__.  I don't think that this
check about __bases__ is correct or relevant or
really fixes any crash.  The link between the
bases and the metaclass of a class is tenuous
anyway: the bases are just used to compute the
metaclass if none is specified explicitly.

As I said on python-dev (with no answer) I am
thinking about reverting r53997 completely.
I'm going to check what I said above a bit more
in-depth first.
msg26390 - (view) Author: Armin Rigo (arigo) * (Python committer) Date: 2007-04-19 14:46
Reverted r53997 in revision 54875.

zseil, could you update your patch for the new
svn head?  Thanks!  It should mostly be a matter
of simplifying it.
msg26391 - (view) Author: Ziga Seilnacht (zseil) * (Python committer) Date: 2007-04-19 16:40
Here is the updated patch.  I also added a few more
tests, and removed the news entry for revision 53997.


File Added: modify_dict_bug2.diff
msg26392 - (view) Author: Armin Rigo (arigo) * (Python committer) Date: 2007-05-02 19:23
Thanks zseil for the patch, which looks really good to me.
For reference, it also contains the fix for #1174712.

Checked in as r55080.
msg61603 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2008-01-23 20:49
Reopening, since test67.py still causes a segfault in the trunk.  It is
represented as Lib/test/crashers/loosing_dict_ref.py [sic].
msg61608 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2008-01-23 21:33
Armin, what do you think of the attached patch (deldict.diff)?
msg61610 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2008-01-23 21:45
Here's a better fix, which also fixes the similar code path in
GenericSetAttr().  As a bonus, I reviewed all uses of
_PyObject_GetDictPtr() and found one questionable place where the dict
at *dictptr was DECREF'ed before that location was set to NULL.  I think
we're better off setting it to NULL and *then* DECREF'ing the dict.
msg61616 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2008-01-23 23:02
The more i think about it, I don't think the issue in typeobject.c can
ever occur, so I'm skipping that part of the fix.
msg61620 - (view) Author: Neal Norwitz (nnorwitz) * (Python committer) Date: 2008-01-24 07:12
I looked at Guido's latest deldict.diff patch--the one to
Objects/object.c only.  It seems good.  I can't convince myself either
way about the change to Objects/typeobject.c.  I can't think of a way
to cause a problem.  It seems safer to use Py_CLEAR in this case
though.

There are several other uses of _PyObject_GetDictPtr in
Objects/typeobject.c.  It was pretty much the same--I can't convince
myself either way.  Can Py_VISIT cause any Python code to execute that
might lead to a problem?  The other uses of _PyObject_GetDictPtr in
Objects/typeobject.c seemed safer.  Not a very useful review.
msg61640 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2008-01-24 17:05
On Jan 23, 2008 11:12 PM, Neal Norwitz <report@bugs.python.org> wrote:
>
> Neal Norwitz added the comment:
>
> I looked at Guido's latest deldict.diff patch--the one to
> Objects/object.c only.  It seems good.  I can't convince myself either
> way about the change to Objects/typeobject.c.  I can't think of a way
> to cause a problem.  It seems safer to use Py_CLEAR in this case
> though.

Here's my reasoning: the object whose dict is being cleared itself has
a refcount of zero at this point. So it is truly unreachable from
Python code. So I'm not going to submit that part of the change.

> There are several other uses of _PyObject_GetDictPtr in
> Objects/typeobject.c.  It was pretty much the same--I can't convince
> myself either way.  Can Py_VISIT cause any Python code to execute that
> might lead to a problem?

The answer lies in the gc module which does all the visiting. A quick
scan of all the traverse() calls there indicates that none of them
call back into Python -- not a DECREF in sight.

> The other uses of _PyObject_GetDictPtr in
> Objects/typeobject.c seemed safer.  Not a very useful review.

I reviewed those too and found them safe.

I'll submit the change now.
msg61649 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2008-01-24 18:57
r60247 (2.5.2 branch), r60246 (2.6 trunk).
History
Date User Action Args
2022-04-11 14:56:13adminsetgithub: 42413
2008-01-24 18:57:54gvanrossumsetstatus: open -> closed
resolution: fixed
messages: + msg61649
2008-01-24 17:05:13gvanrossumsetmessages: + msg61640
2008-01-24 07:12:10nnorwitzsetmessages: + msg61620
2008-01-24 02:57:22Rhamphoryncussetnosy: + Rhamphoryncus
2008-01-23 23:02:12gvanrossumsetmessages: + msg61616
2008-01-23 22:14:10nnorwitzsetnosy: + nnorwitz
2008-01-23 21:45:52gvanrossumsetfiles: - deldict.diff
2008-01-23 21:45:35gvanrossumsetfiles: + deldict.diff
2008-01-23 21:45:11gvanrossumsetresolution: fixed -> (no value)
messages: + msg61610
versions: + Python 2.6, Python 2.5
2008-01-23 21:33:53gvanrossumsetfiles: + deldict.diff
messages: + msg61608
2008-01-23 20:49:56gvanrossumsetstatus: closed -> open
nosy: + gvanrossum
messages: + msg61603
priority: normal -> critical
2005-09-24 23:40:40arigocreate