classification
Title: selectors incorrectly retain invalid file descriptors
Type: Stage: resolved
Components: asyncio Versions: Python 3.6, Python 3.4, Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Mark.Williams, gvanrossum, python-dev, vstinner, yselivanov
Priority: normal Keywords: patch

Created on 2016-08-14 00:26 by Mark.Williams, last changed 2017-03-31 16:36 by dstufft. This issue is now closed.

Files
File name Uploaded Description Edit
selectors.patch Mark.Williams, 2016-08-14 00:26 selectors.patch
selectors.patch vstinner, 2016-08-14 08:21 review
Pull Requests
URL Status Linked Edit
PR 552 closed dstufft, 2017-03-31 16:36
Messages (10)
msg272626 - (view) Author: Mark Williams (Mark.Williams) * Date: 2016-08-14 00:26
Registering a file descriptor with EpollSelector.register that epoll(7) refuses results in the selector retaining a SelectorKey for that file descriptor, even though it's not monitored.

The following program attempts to register a file on the filesystem with an EpollSelector.  epoll_ctl(2) returns EPERM when given a such a file descriptor, so it shouldn't be registered with the selector, but it is registered.


import selectors
import tempfile
import traceback

sel = selectors.EpollSelector()

with tempfile.TemporaryFile() as f:
    try:
        sel.register(f, selectors.EVENT_READ)
    except PermissionError:
        traceback.print_exc()
    print("This should have raised a KeyError:", sel.get_key(f))


It produces this output on Pythons 3.4-3.6:

Traceback (most recent call last):
  File "/tmp/sel.py", line 9, in <module>
    sel.register(f, selectors.EVENT_READ)
  File "/usr/lib/python3.4/selectors.py", line 402, in register
    self._epoll.register(key.fd, epoll_events)
PermissionError: [Errno 1] Operation not permitted
This should have raised a KeyError: SelectorKey(fileobj=<_io.BufferedRandom name=8>, fd=8, events=1, data=None)

A similar problem exists with KqueueSelector.  Consider the following program:


import selectors
import tempfile
import traceback

sel = selectors.KqueueSelector()

f = tempfile.TemporaryFile()
filedescriptor = f.fileno()
f.close()

try:
    sel.register(filedescriptor, selectors.EVENT_READ)
except OSError:
    traceback.print_exc()

print("This should have raised a KeyError:", sel.get_key(filedescriptor))


In this case selectors._fileobj_to_fd doesn't detect that the integer file descriptor is closed.  

Note that _fileobj_to_fd should not be changed! Attempting to use, say, fcntl(fd, fcntl.GET_FD) to detect a closed file descriptor introduces a TOCTTOU bug.  Another thread (or another process, if the file descriptor refers to a socket shared between two or more processes and one calls shutdown(2) on it) can change the state of the file descriptor between the time it's checked and the time it's registered.  A new file might even be opened and given the previous file descriptor.

The attached patch catches any exception raised by EpollSelector.register or KqueueSelector.register, removes the SelectorKey from BaseSelector's table, and then re-raises the exception.

Note that I've included asyncio as an affected component, because asyncio wraps the selectors module.
msg272629 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2016-08-14 01:03
Your patch doesn't apply cleanly to the 3.4 or 3.5 branches -- how did you generate it?
msg272630 - (view) Author: Mark Williams (Mark.Williams) * Date: 2016-08-14 01:09
Whoops!  I pulled from https://github.com/python/cpython and branched off of master.  The previous commit was https://github.com/python/cpython/tree/043152b8da83105ff5cba88d4e6ef1d5c64b3602

I can generate new patches using hg.python.org's 3.4 and 3.5 branches.  Please stand by.
msg272631 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2016-08-14 01:14
We only need 3.5; 3.4 is no longer supported.
msg272632 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2016-08-14 01:17
Also don't spend more time on this, I found a way to apply the patch
cleanly.
msg272633 - (view) Author: Mark Williams (Mark.Williams) * Date: 2016-08-14 01:17
OK!  Sorry for the trouble.

On Sat, Aug 13, 2016 at 6:17 PM, Guido van Rossum <report@bugs.python.org>
wrote:

>
> Guido van Rossum added the comment:
>
> Also don't spend more time on this, I found a way to apply the patch
> cleanly.
>
> ----------
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue27759>
> _______________________________________
>
msg272657 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-08-14 08:21
Regenerated patch to get a review button.
msg276639 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-09-15 23:32
New changeset 8944fd09b9ca by Yury Selivanov in branch '3.5':
Issue #27759: Fix selectors incorrectly retain invalid file descriptors.
https://hg.python.org/cpython/rev/8944fd09b9ca

New changeset 08a75f380699 by Yury Selivanov in branch '3.6':
Merge 3.5 (issue #27759)
https://hg.python.org/cpython/rev/08a75f380699

New changeset ded64b96a4e7 by Yury Selivanov in branch 'default':
Merge 3.6 (issue #27759)
https://hg.python.org/cpython/rev/ded64b96a4e7
msg276640 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-09-15 23:33
I've fixed the remaining review comments & committed the patch.  Thank you Mark!

BTW, this also fixes http://bugs.python.org/issue27386.
msg278199 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-10-06 18:06
New changeset 8cc1fca83fb8 by Yury Selivanov in branch '3.4':
Issue #27759: Fix selectors incorrectly retain invalid file descriptors.
https://hg.python.org/cpython/rev/8cc1fca83fb8
History
Date User Action Args
2017-03-31 16:36:37dstufftsetpull_requests: + pull_request1095
2016-10-06 18:06:39python-devsetmessages: + msg278199
2016-09-15 23:33:00yselivanovsetstatus: open -> closed
resolution: fixed
messages: + msg276640

stage: resolved
2016-09-15 23:32:00python-devsetnosy: + python-dev
messages: + msg276639
2016-08-14 08:21:20vstinnersetfiles: + selectors.patch

messages: + msg272657
2016-08-14 01:17:36Mark.Williamssetmessages: + msg272633
2016-08-14 01:17:14gvanrossumsetmessages: + msg272632
2016-08-14 01:14:45gvanrossumsetmessages: + msg272631
2016-08-14 01:09:25Mark.Williamssetmessages: + msg272630
2016-08-14 01:03:06gvanrossumsetmessages: + msg272629
2016-08-14 00:26:49Mark.Williamscreate