classification
Title: python RLock implementation unsafe with signals
Type: behavior Stage: needs patch
Components: Library (Lib) Versions: Python 3.3, Python 3.2, Python 2.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Mike.Meyer, alex, cvrebert, haypo, jcea, meador.inge, neologix, pitrou, rbcollins
Priority: normal Keywords:

Created on 2012-01-02 21:13 by rbcollins, last changed 2014-03-18 16:52 by cvrebert.

Files
File name Uploaded Description Edit
hang.py rbcollins, 2012-01-02 21:13 Demonstration script
Messages (13)
msg150477 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2012-01-02 21:13
This affects the python implementation of RLock only.

If a signal occurs during RLock.acquire() or release() and then operates on the same lock to acquire() or release() it, process hangs or assertions can be triggered.

The attached test script demonstrates the issue on Python 2.6 and 3.2, and code inspection suggests this is still valid for 2.7 and 3.4.

To use it, run the script and then 'kill -SIGUSR2' the process id it prints out. Keep sending SIGUSR2 until you get bored or:
 - a traceback occurs
 - the process stops printing out '.'s

We found this debugging application server hangs during log rotation (https://bugs.launchpad.net/launchpad/+bug/861742) where if the thread that the signal is received in was about to log a message the deadlock case would crop up from time to time. Of course, this wasn't ever safe code, and we're changing it (to have the signal handler merely set a integer flag that the logging handler can consult without locking).

However, I wanted to raise this upstream, and I note per issue #3618 that the 3.x IO module apparently includes a minimal version of the Python RLock implementation (or just uses RLock itself). Either way this bug probably exists for applications simply doing IO, either when there is no C RLock implementation available, or all the time (depending on exactly what the IO module is doing nowadays).
msg150483 - (view) Author: Jesús Cea Avión (jcea) * (Python committer) Date: 2012-01-02 23:10
Adding myself to the nosy list because I am interested in the issue, but I can't take care of this myself for a few weeks, at least.

Robert, would you consider to try to write a patch?
msg150488 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2012-01-03 00:52
I'm not sure it is sensibly implementable in pure python: the semantics of signal handling (AIUI) are that the vm is interrupted, sets a flag to say 'when the GIL is released or the next bytecode interpretation happens, please process signal X' [the former for C extensions, the latter for regular code], and then the OS level signal handler returns. The current C or bytecode execution completes and then we dispatch to the python signal handler.

Now, what we would want here is that attempts to acquire/release the RLock in a signal handler would behave as though the RLock.acquire/release methods were atomic. The general way to enforce such atomicity is via a lock or mutex. Now, because the call stack looks like this:
<signal>thread.acquire()
<non-signal-code>thread.acquire()

The other acquire, if it already holds this supposed mutex, would block the python signal handler acquire call from obtaining it; The inner acquire would have to error *whether or not a timeout was passed*, because the non-signal-code won't progress and complete until the python signal handler code returns. One could, in principle, use stack inspection to determine how far through a nested acquire/release the outer code is, but that seems, uhm, pathological to me :).

The crux of the problem is detecting whether the non-reentrant thread.lock is held because (a) another thread holds it or (b) this thread holds it. If we can inspect its internals, we could determine that this thread holds it - and thus we must be reentered into the acquire/release methods. With that tricky thing established, we could look at the remaining logic to make sure that each line is either atomic or conditional on reentrancy.

Simpler I think to require CRLocks always, and provide a portable C implementation which will always complete before returning to execution in the calling thread, avoiding this issue entirely.
msg150499 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-01-03 14:40
Yes, using synchronization primitives or doing I/O in Python signal handlers isn't a good idea. Perhaps the signal docs should be clearer about that.

> Of course, this wasn't ever safe code, and we're changing it (to have the signal handler merely set a integer flag that the logging handler can consult without locking)

Indeed, setting a variable (or using set_wakeup_fd()) is the right approach.
msg150517 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2012-01-03 17:20
The core of the problem is that we don't just want those methods to be atomic or thread-safe, but reentrant (or rather async-safe).
As such, protecting by a lock isn't enough (and it's not really feasible in Python).

Note that the RLock implementation already checks whether the lock is already acquire by the current thread, but there's a "race":
if self._owner == me:
    [...]
    self._count = self._count + 1
    return 1
rc = self._block.acquire(blocking, timeout)
if rc:
    self._owner = me

If the signal handler is called after _block has been acquired, but before self._owner is set, the next call to acquire from the signal handler won't "realize" that the block has already been acquired by the current thread, and will deadlock.

Now, the fun part: this affects not only RLock, but every Python code performing "atomic" actions: condition variables, barriers, etc. There are some constraints on what can be done from a signal handler, and it should probably be documented.

Note that another solution would be to use a dedicated thread for signal management (like Java does), but that's another story.

Also, this shouldn't be a problem for the buffered I/O code, since the code already accounts for this possibility (if the lock is already acquired by the current thread, an exception is raised).

Now, there's something I don't understand: I've just had a quick look, but AFAICT, there's no reason why the C version of RLock could not be available: am I missing something? Why do we even have a Python implementation?
msg150518 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-01-03 17:29
> Note that another solution would be to use a dedicated thread for
> signal management (like Java does), but that's another story.

That sounds like a good solution in the middle-term. Are there any
drawbacks? (apart from launching a thread)

> Also, this shouldn't be a problem for the buffered I/O code, since the
> code already accounts for this possibility (if the lock is already
> acquired by the current thread, an exception is raised).

Yes, but raising an exception is less helpful than doing what the user
asked for :)

> Now, there's something I don't understand: I've just had a quick look,
> but AFAICT, there's no reason why the C version of RLock could not be
> available: am I missing something? Why do we even have a Python
> implementation?

The C version is quite recent, and there's a school of thought that we
should always provide fallback Python implementations.
(also, arguably a Python implementation makes things easier to
prototype, although I don't think it's the case for an RLock)
msg150576 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2012-01-04 02:51
> That sounds like a good solution in the middle-term. Are there any
> drawbacks? (apart from launching a thread)

Just to be clear: the approach I was suggesting is to have a resident
thread dedicated to signal management, not to spawn a new one when
needed. Another advantage is that we could mask signals in all threads
except this one, and have a consistent cross-platform behavior with
respect to signals+threads.

However I see two drawbacks:
- it seems that we want to allow building Python without threads
support. In that case, this wouldn't work, or we would need the
current implementation as a fallback, but this would complicate the
code somewhat.
- the thread would have to be respawned after a fork()

> The C version is quite recent, and there's a school of thought that we
> should always provide fallback Python implementations.
> (also, arguably a Python implementation makes things easier to
> prototype, although I don't think it's the case for an RLock)

Hmmm, I'm not convinced by this argument in this specific case: since
the C version is now the default, is faster and more reliable, can't
we drop the Python version?
msg150578 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2012-01-04 03:15
> This affects the python implementation of RLock only.

In the issue #13550, it was discussed to remove completly the logging machinery from the threading module. If we remove it, we don't need the Python implementation of RLock.

We already removed the Python implementation of subprocess.Popen._execute_child() in Python 3.3 because of issues with fork, signals and threads.
msg150579 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2012-01-04 03:25
Normally I advocate very strongly for Python implementation of C accelerated modules, but when the two implementations are not equivalent, having a simpler Python one around does not help anyone (not users, other language implementors etc). True reentrancy is possible but quite hard to achieve. 

So, FWIW, +1 on just having a C implementation.

The dedicated signal thread sounds useful too - it would ease the issues for folk using other parts of the stdlib, and would in principle permit a pure-python RLock to hang around.
msg150600 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-01-04 10:52
> > That sounds like a good solution in the middle-term. Are there any
> > drawbacks? (apart from launching a thread)
> 
> Just to be clear: the approach I was suggesting is to have a resident
> thread dedicated to signal management, not to spawn a new one when
> needed. Another advantage is that we could mask signals in all threads
> except this one, and have a consistent cross-platform behavior with
> respect to signals+threads.

Hmm, but that would break single-threaded programs which expect their
select() (or other) to return EINTR when a signal is received (which is
a perfectly valid expectation in that case).

> However I see two drawbacks:
> - it seems that we want to allow building Python without threads
> support. In that case, this wouldn't work, or we would need the
> current implementation as a fallback, but this would complicate the
> code somewhat.

I don't know if that's still useful to build Python without threads. I
would expect most platforms to have a compatible threads implementation
(and Python probably can't run on very small embedded platforms).
Perhaps you can ask on python-dev.
msg150676 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2012-01-05 17:56
> Hmm, but that would break single-threaded programs which expect their
> select() (or other) to return EINTR when a signal is received (which is
> a perfectly valid expectation in that case).

Yes, that's why I said "that"s another story" ;-)
EINTR is really a pain, and relying on it to return from a syscall
upon signal reception is a bad idea (certain OS restart syscalls by
default - not select() though - and if the signal is received before
you call the syscall, you'll deadlock). This would IMHO be the best
way to go, but I know we can't reasonably change this now.

> I don't know if that's still useful to build Python without threads. I
> would expect most platforms to have a compatible threads implementation
> (and Python probably can't run on very small embedded platforms).
> Perhaps you can ask on python-dev.

There are another problems, for example it's very well known that
signals and threads don't mix well (so for example you'd have to block
all signals before spawning the new thread, and reenable them
afterwards).
I'm not sure it's worth the extra complication. I can still try to
write a quick patch to see if it gets somewhere (and doesn't break
test_threading and test_signals).

The first and most simple thing we could do would be to nuke the
Python version (and also the loggin hack at the same time): does that
sound reasonable ?
msg150688 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-01-05 20:54
> The first and most simple thing we could do would be to nuke the
> Python version (and also the loggin hack at the same time): does that
> sound reasonable ?

To me, yes, but I think it's better to ask on python-dev as for nuking
the Python version.
msg160131 - (view) Author: Mike Meyer (Mike.Meyer) Date: 2012-05-07 10:26
I just ran into this issue in the logging module using 2.7. Here's the TB in
case it sheds any light on the issue

Traceback (most recent call last):
  File "./crawler.py", line 531, in <module>
    main(argv[1:]1:)
  File "./crawler.py", line 522, in main
    MCP(config).run()
  File "./crawler.py", line 332, in run
    self.reaper()
  File "./crawler.py", line 359, in reaper
    logging.debug('MCP process alive: %s', state)
  File "/usr/local/lib/python2.7/logging/__init__.py", line 1600, in debug
    root.debug(msg, *args, **kwargs)
  File "/usr/local/lib/python2.7/logging/__init__.py", line 1120, in debug
    self._log(DEBUG, msg, args, **kwargs)
  File "/usr/local/lib/python2.7/logging/__init__.py", line 1250, in _log
    self.handle(record)
  File "/usr/local/lib/python2.7/logging/__init__.py", line 1260, in handle
    self.callHandlers(record)
  File "/usr/local/lib/python2.7/logging/__init__.py", line 1300, in callHandlers
    hdlr.handle(record)
  File "/usr/local/lib/python2.7/logging/__init__.py", line 746, in handle
    self.release()
  File "/usr/local/lib/python2.7/logging/__init__.py", line 700, in release
    self.lock.release()
  File "/usr/local/lib/python2.7/threading.py", line 147, in release
    self.__block.release()
thread.error: release unlocked lock

Since I'm not using threads, getting thread errors was a little bit of
a surprise. I guess trying to make the logging module thread safe
added a potential bug.
History
Date User Action Args
2014-03-18 16:52:41cvrebertsetnosy: + cvrebert
2012-05-07 10:26:51Mike.Meyersetnosy: + Mike.Meyer
messages: + msg160131
2012-01-05 20:54:39pitrousetmessages: + msg150688
2012-01-05 17:56:59neologixsetmessages: + msg150676
2012-01-04 10:52:14pitrousetmessages: + msg150600
2012-01-04 03:40:26alexsetnosy: + alex
2012-01-04 03:25:32rbcollinssetmessages: + msg150579
2012-01-04 03:15:52hayposetnosy: + haypo
messages: + msg150578
2012-01-04 02:51:11neologixsetmessages: + msg150576
2012-01-03 17:29:28pitrousetmessages: + msg150518
2012-01-03 17:20:57neologixsetnosy: + neologix
messages: + msg150517
2012-01-03 14:40:35pitrousetmessages: + msg150499
2012-01-03 03:43:02meador.ingesetnosy: + meador.inge
2012-01-03 00:52:43rbcollinssetmessages: + msg150488
2012-01-03 00:30:08pitrousetnosy: + pitrou
2012-01-03 00:30:04pitrousetstage: needs patch
type: behavior
versions: + Python 3.2
2012-01-02 23:10:16jceasetnosy: + jcea

messages: + msg150483
versions: + Python 3.3, - Python 3.4
2012-01-02 21:13:12rbcollinscreate