Repository navigation
select.poll is not thread safe #53111
Description
| BPO | 8865 |
|---|---|
| Nosy | @gpshead, @pitrou, @vstinner, @asvetlov, @serhiy-storchaka |
| Files |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = 'https://github.com/serhiy-storchaka'
closed_at = <Date 2013-08-20.18:23:29.244>
created_at = <Date 2010-05-31.22:20:13.881>
labels = ['library', 'type-crash']
title = 'select.poll is not thread safe'
updated_at = <Date 2013-08-20.18:23:29.243>
user = 'https://bugs.python.org/apexo'bugs.python.org fields:
activity = <Date 2013-08-20.18:23:29.243>
actor = 'serhiy.storchaka'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2013-08-20.18:23:29.244>
closer = 'serhiy.storchaka'
components = ['Library (Lib)']
creation = <Date 2010-05-31.22:20:13.881>
creator = 'apexo'
dependencies = []
files = ['17510', '17511', '17675', '27961', '27963', '27964', '27967', '27968', '28687', '28855']
hgrepos = []
issue_num = 8865
keywords = ['patch']
message_count = 31.0
messages = ['106815', '106816', '106827', '106829', '107863', '175387', '175389', '175390', '175394', '175395', '175397', '175400', '175409', '175429', '175431', '175432', '175501', '175525', '176894', '176895', '176969', '176998', '179657', '179659', '180696', '181822', '181823', '186736', '194334', '195705', '195707']
nosy_count = 9.0
nosy_names = ['gregory.p.smith', 'exarkun', 'pitrou', 'vstinner', 'asvetlov', 'neologix', 'apexo', 'python-dev', 'serhiy.storchaka']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue8865'
versions = ['Python 2.7', 'Python 3.3', 'Python 3.4']
Activity
apexo commentedon May 31, 2010
invoking select.poll.poll() concurrently from multiple threads frequently yields garbage in one thread:
while poll_poll in thread 1 is parsing its result, another thread 2 calling poll may overwrite revents; assuming poll_result was 1 in thread 1 and thread 2 managed to clear all revents before thread 1 started scanning ufds, thread 1 would iterate straight through all ufds, past its bounds (no bound checks there), and return the first out-of-bounds entry that happens to have revents != 0
this issue needs at least documentation (although bounds-checking to prevent garbage in the result wouldn't hurt)
also, since there doesn't seem to be any locking w/ regards to ufds, it might be possible to corrupt python's heap, by concurrently invoking poll_register and poll_poll. poll_register could move the ufds array to another location while resizing it and poll_poll would subsequently overwrite memory that is not allocated anymore or allocated by someone else (did not test that)
python 2.5.5
pitrou commentedon May 31, 2010
Do you have a script that reproduces it?
apexo commentedon Jun 1, 2010
okay, I've managed to put together a script that fairly consistently reproduces the problem on my (core2 duo 2,something GHz) machine
some parameters (sleep times) are randomized in each iteration, the script runs for 30 iterations, each for 1 second, and counts the number of events where the fd looks bogus (max 1 per iteration); some other events (fd correct, revents bogus are also reported, but not counted)
example output:
poll returned [(-3, 65535)], we were looking for fd 3
poll returned [(-3, 65535)], we were looking for fd 3
poll returned [(-3, 65535)], we were looking for fd 3
poll returned [(-3, 65535)], we were looking for fd 3
poll returned [(-3, 65535)], we were looking for fd 3
poll returned [(-1, 65535)], we were looking for fd 3
poll returned [(-1, 65535)], we were looking for fd 3
poll returned [(-1627358347, 60497)], we were looking for fd 3
should not get here; poll returned [(3, 0)]
poll returned [(-3, 65535)], we were looking for fd 3
9 events in 30 iterations
apexo commentedon Jun 1, 2010
the other issue (poll_register freeing data structures that poll_poll currently uses) can be reproduced by the attached script (at least I can)
depending on the parameters/circumstances it either segfaults or prints garbage results from poll, e.g. [(1684955474, 28192)]; the expected result would be [(3, 1)]
apexo commentedon Jun 15, 2010
added a patch which fixes both issues
before releasing the GIL we take a copy of the ufds pointer and its len, erasing the ufds pointer in the poll object (to make sure nobody else fiddles with it); when we're done we either but it back into the object (when it's still NULL) or we free it
the update logic is modified as well, since the uptodate flag is not sufficient anymore, we now count up a tag for each modification of the poll dictionary and remember that tag together with the ufds pointer
the result is:
there still is potential for incorrect poll results (but not for memory corruption): when poll_register/poll_unregister are called exactly 2**32 times (or multiples thereof) while poll_poll is running, it will happily put back its outdated ufds pointer into the poll object when its done, this could be alleviated by changing tag to long long ... which is unlikely to wrap around anytime soon.
serhiy-storchaka commentedon Nov 11, 2012
I was unable to apply the patch automatically, so I had to do it manually. Here is an updated patch for review. I did not consider it in detail yet, but it seems to correct these errors.
17 remaining items
serhiy-storchaka commentedon Dec 5, 2012
Test should check changed behavior. I.e. it should test that calling poll concurrently raises an exception.
neologix commentedon Jan 11, 2013
This patch should be updated to also fix epoll().
Also, is it right to raise an exception in case of concurrent invocation?
Here's a simple script that crashes systematically on my Linux box.
serhiy-storchaka commentedon Jan 11, 2013
It is right for poll() because it was not concurrently usable in previous versions in any case. For epoll() it is an another issue.
serhiy-storchaka commentedon Jan 26, 2013
Here is a test made from Charles-François's crasher. Let's go.
serhiy-storchaka commentedon Feb 10, 2013
Ping.
pitrou commentedon Feb 10, 2013
Patches look good to me.
serhiy-storchaka commentedon Apr 13, 2013
Gregory?
serhiy-storchaka commentedon Aug 4, 2013
Gregory, could you commit the patch or allow me to do this?
python-dev commentedon Aug 20, 2013
New changeset 072ba5df77e4 by Serhiy Storchaka in branch '3.3':
Issue bpo-8865: Concurrent invocation of select.poll.poll() now raises a
http://hg.python.org/cpython/rev/072ba5df77e4
New changeset 4543408e2ba6 by Serhiy Storchaka in branch 'default':
Issue bpo-8865: Concurrent invocation of select.poll.poll() now raises a
http://hg.python.org/cpython/rev/4543408e2ba6
New changeset a4091c1de27a by Serhiy Storchaka in branch '2.7':
Issue bpo-8865: Concurrent invocation of select.poll.poll() now raises a
http://hg.python.org/cpython/rev/a4091c1de27a
serhiy-storchaka commentedon Aug 20, 2013
Thank you for the report and the patch Christian.
wsapoll: add lock