[ python-Bugs-1069160 ] PyThreadState_SetAsyncExc segfault
SourceForge.net
noreply at sourceforge.net
Tue Feb 8 03:08:45 CET 2005
Bugs item #1069160, was opened at 2004-11-18 20:48
Message generated for change (Comment added) made by gvanrossum
You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=105470&aid=1069160&group_id=5470
Category: Threads
Group: Python 2.4
>Status: Closed
Resolution: None
Priority: 6
Submitted By: Tim Peters (tim_one)
>Assigned to: Guido van Rossum (gvanrossum)
Summary: PyThreadState_SetAsyncExc segfault
Initial Comment:
PyThreadState_SetAsyncExc() crawls over the list of
tstates without holding a mutex. Python implementation
code has tried to get away with this kind of thing before
(& more than once <wink>: and segfaults are always
eventually reported.
A common cause is that PyThreadState_DeleteCurrent()
is typically called *without* the GIL held, so any thread
can try to delete its own tstate *while*
PyThreadState_SetAsyncExc() is trying to muck with
that tstate. That the GIL is held by
PyThreadState_SetAsyncExc's caller is no protection.
Instead the code has to serialize against tstate chain
mutations by acquiring head_mutex for the duration.
Of course this is rare and can be virtually impossible to
reproduce, but that makes the bug worse, not "better".
----------------------------------------------------------------------
>Comment By: Guido van Rossum (gvanrossum)
Date: 2005-02-07 21:08
Message:
Logged In: YES
user_id=6380
OK, to prevent further waste of time I've checked in the
proposed fix.
I have no idea what the count>1 comment was referring to any
more; I'm sure I wrote it though.
----------------------------------------------------------------------
Comment By: Tim Peters (tim_one)
Date: 2005-02-07 15:24
Message:
Logged In: YES
user_id=31435
So we're peeing away time on a Mystery Function with no
known actual uses in the entire world. Cool <wink>.
----------------------------------------------------------------------
Comment By: Alex Martelli (aleax)
Date: 2005-02-07 15:11
Message:
Logged In: YES
user_id=60314
I've checked the current codebase of the client for whom I originally
wrote code using PyThreadState_SetAsyncExc, and that code is not
currently used in production. It did that call while holding the GIL, FWIW.
----------------------------------------------------------------------
Comment By: Just van Rossum (jvr)
Date: 2005-02-07 14:17
Message:
Logged In: YES
user_id=92689
To be honest: I don't yet have any code that relies on the feature, so
there's nothing to break. Don't know about Alex, I'll ask him.
I did a quick test (through ctypes), and the function works well with and
without patch. I've attached the test script; it's mostly due to Peter
Hansen: http://groups-beta.google.com/group/comp.lang.python/msg/
d310502f7c7133a9
----------------------------------------------------------------------
Comment By: Tim Peters (tim_one)
Date: 2005-02-07 10:41
Message:
Logged In: YES
user_id=31435
Just, this function isn't called anywhere in the Python
distribution, so Armin and Guido and I have no certain way to
know that it doesn't break the feature completely.
The presumption here is that you do have code that uses this
feature. If so, we're just asking that you verify your code
still works after applying the patch. OTOH, if you don't have
any code that uses this function, then there's no reason for
you to get sucked into this.
----------------------------------------------------------------------
Comment By: Armin Rigo (arigo)
Date: 2005-02-07 06:27
Message:
Logged In: YES
user_id=4771
I didn't worry too much about ZAP deadlocks precisely because this
problem was already there elsewhere. I guess that if this is a problem, it
should be discussed and fixed in another bug report, after we apply the
obvious two-liner patch of the current tracker.
----------------------------------------------------------------------
Comment By: Just van Rossum (jvr)
Date: 2005-02-07 04:04
Message:
Logged In: YES
user_id=92689
I'm not able to judge the patch. I'm only the one who had the feature
request (together with Alex Martelli), but I know next to nothing about
the implementation and all the subtle details. If you need a volunteer to
apply a patch, sure, I'll gladly help, but the discussion is way over my
head here.
----------------------------------------------------------------------
Comment By: Tim Peters (tim_one)
Date: 2005-02-06 20:51
Message:
Logged In: YES
user_id=31435
It's already documented that PyThreadState_SetAsyncExc()
must be called with the GIL held. head_mutex is only used to
protect traversal/mutation of the threadstate chain, so is
only held internally by functions that create, or destroy,
thread states or interpreter states. It's certainly possible to
get a deadlock then after the patch, not due to the GIL, but
due to head_mutex alone. I doubt that any __del__ method
would create/destroy thread/interpreter states on its own,
the bigger danger is that once a __del__ method is invoked,
other threads can run too, and they may do anything
(including calling PyThreadState_SetAsyncExc() again! that
seem the most likely way to deadlock).
It would be good to have a clue about "I have no idea why
this function warns about return values greater than 1". If in
fact it should be impossible to have a count greater than 1
given that concurrent mutations to the tstate chain are
locked out after the patch, then the patch could be rewritten
to release head_mutex when it found p->thread_id == id the
first time, before the ZAP. After the ZAP, the function would
just return:
if (p->thread_id == id) {
PyObject *temp = p->async_exc;
Py_XINCREF(exc);
p->async_exc = exc;
HEAD_UNLOCK();
Py_XDECREF(temp);
return 1;
}
Then deadlock couldn't occur.
I should note that PyInterpreterState_Clear() is prone to the
same kind of deadlocks on head_mutex (it calls
PyThreadState_Clear() with head_mutex held, and the latter
does a lot of ZAPping).
----------------------------------------------------------------------
Comment By: Guido van Rossum (gvanrossum)
Date: 2005-02-06 19:44
Message:
Logged In: YES
user_id=6380
But it was Just's idea, so assigning to him. Q for Just:
would applying the patch cause any problems in your code
that's using this feature?
(Q. for Tim: hoes does the head_mutex in this file relate to
the GIL? The ZAP() call would seem to require that the GIL
is already head when this fn. is called, because it could
invoke arbitrary Python __del__ code. Is there any other
call that could acquire these locks in the opposite order,
causing deadlocks?)
----------------------------------------------------------------------
Comment By: Tim Peters (tim_one)
Date: 2005-02-03 11:41
Message:
Logged In: YES
user_id=31435
Yup, that patch is all I had in mind. It's a shame that nobody
cares enough about this function to test it (which isn't a dig
at you, Armin -- I don't care enough either <0.5 wink>).
BTW, I have no idea why this function warns about return
values greater than 1. It's possible that the original author
realized the tstate chain could mutate while the loop was
running, and that's where "greater than 1" came from. If so,
locking head_mutex should make that impossible ... OK, some
quality time with CVS shows that Guido checked this function
in, so assigning the report to him.
----------------------------------------------------------------------
Comment By: Armin Rigo (arigo)
Date: 2005-02-02 06:19
Message:
Logged In: YES
user_id=4771
I guess that the patch you have in mind is just to protect the loop with HEAD_LOCK/HEAD_UNLOCK. I cannot test this patch (attached), though, as I don't have any code that uses this function. I can only tell that it also looks like a reasonable thing to do to me.
----------------------------------------------------------------------
Comment By: Tim Peters (tim_one)
Date: 2004-11-19 10:32
Message:
Logged In: YES
user_id=31435
Changed Category to Threads.
----------------------------------------------------------------------
You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=105470&aid=1069160&group_id=5470
More information about the Python-bugs-list
mailing list