Issue6990
Created on 2009-09-25 03:27 by pjenvey, last changed 2009-09-29 18:16 by pitrou.
|
msg93099 - (view) |
Author: Philip Jenvey (pjenvey) |
Date: 2009-09-25 03:27 |
|
When threading.local subclasses are cleared during a reference cycle the
local's internal key is nulled before the local is deallocated. That's a
problem because local only deletes its state (ldicts) from threads
during deallocation, and doesn't do so at all when its key is null.
So leaving ldicts around is one thing, but what's worse is they can be
recycled by new local objects later -- since ldicts are mapped to
threadstates by said key, and said key is based on the local's pointer.
If a new local is malloced at the old one's address it can end up with
the original's ldicts (depending on which thread it's allocated from).
Attached is a test against trunk showing this. Should we delete the
ldicts during clear, recreate the key during dealloc, or something else?
|
|
msg93101 - (view) |
Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) |
Date: 2009-09-25 07:32 |
|
Thanks for the excellent test case!
Is seems enough to remove the statement "Py_CLEAR(self->key);" from
local_clear(). self->key is a string which cannot cause cycles (and is
not visited in local_traverse()); now local_dealloc() does its job.
Index: threadmodule.c
===================================================================
--- threadmodule.c (revision 74229)
+++ threadmodule.c (working copy)
@@ -239,7 +239,6 @@
static int
local_clear(localobject *self)
{
- Py_CLEAR(self->key);
Py_CLEAR(self->args);
Py_CLEAR(self->kw);
Py_CLEAR(self->dict);
|
|
msg93115 - (view) |
Author: Philip Jenvey (pjenvey) |
Date: 2009-09-25 17:23 |
|
Great, though I think it still needs to deallocated:
Index: Modules/threadmodule.c
===================================================================
--- Modules/threadmodule.c (revision 75050)
+++ Modules/threadmodule.c (working copy)
@@ -244,7 +244,6 @@
static int
local_clear(localobject *self)
{
- Py_CLEAR(self->key);
Py_CLEAR(self->args);
Py_CLEAR(self->kw);
Py_CLEAR(self->dict);
@@ -266,6 +265,7 @@
PyDict_DelItem(tstate->dict, self->key);
}
+ Py_XDECREF(self->key);
local_clear(self);
Py_TYPE(self)->tp_free((PyObject*)self);
}
|
|
msg93116 - (view) |
Author: Philip Jenvey (pjenvey) |
Date: 2009-09-25 17:26 |
|
Also I've tagged this as a (potential) security issue. E.g. if a web app
were affected, one user could potentially access another's data.
I actually noticed it in the Beaker sessioning/caching middleware (used by
Pylons and other web frameworks). Though it only manifested itself as an
exception in Beaker, others may not be so lucky
So I'd like to apply the ultimate fix all the way down to at least the 2.5
branch
|
|
msg93224 - (view) |
Author: Antoine Pitrou (pitrou) |
Date: 2009-09-28 18:42 |
|
The latest patch + tests looks ok to me.
|
|
msg93252 - (view) |
Author: Philip Jenvey (pjenvey) |
Date: 2009-09-29 05:32 |
|
applied in r75123-r75127, including a backport to release25-maint
|
|
msg93311 - (view) |
Author: Antoine Pitrou (pitrou) |
Date: 2009-09-29 18:12 |
|
I get the following failure on py3k:
test test_threading_local failed -- Traceback (most recent call last):
File "/home/antoine/py3k/__svn__/Lib/test/test_threading_local.py",
line 107, in test_derived_cycle_dealloc
self.assertTrue(passed)
AssertionError: False is not True
|
|
msg93312 - (view) |
Author: Antoine Pitrou (pitrou) |
Date: 2009-09-29 18:15 |
|
Of course, I had forgotten to rebuild :-/ Sorry for the noise.
|
|
| Date |
User |
Action |
Args |
| 2009-09-29 18:16:00 | pitrou | set | status: open -> closed resolution: accepted -> fixed messages:
+ msg93312
|
| 2009-09-29 18:12:34 | pitrou | set | status: closed -> open resolution: fixed -> accepted messages:
+ msg93311
|
| 2009-09-29 16:35:44 | pitrou | set | status: open -> closed resolution: fixed |
| 2009-09-29 05:32:05 | pjenvey | set | messages:
+ msg93252 |
| 2009-09-28 18:42:52 | pitrou | set | nosy:
+ pitrou messages:
+ msg93224
|
| 2009-09-25 20:23:09 | barry | set | priority: release blocker |
| 2009-09-25 17:26:06 | pjenvey | set | messages:
+ msg93116 |
| 2009-09-25 17:23:40 | pjenvey | set | messages:
+ msg93115 |
| 2009-09-25 07:32:16 | amaury.forgeotdarc | set | keywords:
+ needs review
messages:
+ msg93101 stage: patch review |
| 2009-09-25 04:04:55 | bbangert | set | nosy:
+ bbangert
|
| 2009-09-25 03:27:58 | pjenvey | create | |
|