Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix Channel.close() race condition in pipe.py #2274

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Fix Channel.close() race condition in pipe.py #2274

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
44cf229
Fix Channel.close() race condition in pipe.py
pettitpeon Jul 23, 2023
f4d7fec
fix typo
pettitpeon Jul 23, 2023
e4d9af9
Fix review issues
pettitpeon Aug 2, 2023
9b5776a
Fix typo
pettitpeon Aug 2, 2023
70741ac
Fix flake8
pettitpeon Aug 2, 2023
6a6f6fb
Use errno.EBADF
pettitpeon Aug 17, 2023
bf887b2
Update paramiko/pipe.py
pettitpeon Aug 30, 2023
a092146
Update paramiko/pipe.py
pettitpeon Aug 30, 2023
3cf92eb
Update changelog.rst
pettitpeon Aug 30, 2023
d9fd423
Update changelog.rst
pettitpeon Aug 30, 2023
917e1ec
Update sites/www/changelog.rst
pettitpeon Oct 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
34 changes: 30 additions & 4 deletions paramiko/pipe.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
will trigger as readable in `select <select.select>`.
"""

import errno
import sys
import os
import socket
Expand All @@ -46,25 +47,50 @@ def __init__(self):
self._closed = False

def close(self):
os.close(self._rfd)
os.close(self._wfd)
# used for unit tests:
self._closed = True

# We cannot do anything about closing errors. It is only a
# "best effort" approach
try:
os.close(self._rfd)
except Exception:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why OSError is not used rather than Exception?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case we want to be as broad as possible in the try/except. If any exception is raised during the closing of the rd file descriptor, we still want to guarantee the closing of the wr file descriptor. Catching only one exception type could be too narrow and leave the pipe in an invalid state (one descriptor open and the other one closed). But I won't be able to test my changes on Windows

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case we want to be as broad as possible in the try/except.

I'm unsure your comment. What I'm saying is:

        try:
            os.close(self._rfd)
        except Exception:
            pass
        try:
            os.close(self._wfd)
        except Exception:
            pass

should be

        try:
            os.close(self._rfd)
        except OSError:
            pass
        try:
            os.close(self._wfd)
        except OSError:
            pass

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the problem with catching OSError is that if os.close() raises a different exception for whatever unknow reason, the pipe could go into an invalid state and potentially leak opened file descriptors.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

        try:
            os.close(self._rfd)  # imagine this throws RuntimeError
        except OSError:
            pass
        try:
            os.close(self._wfd)  # in that case, the _wrd would not be closed
        except OSError:
            pass

Copy link
Contributor

@bskinn bskinn Oct 17, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll leave these excepts as a decision for bitprophet.

pass
try:
os.close(self._wfd)
except Exception:
pass

def fileno(self):
return self._rfd

def clear(self):
if not self._set or self._forever:
return

# .read() does not need to handle a race condition with .close()
# because the pipe is created, cleared and closed from the same
# "client thread". The "server thread" sets the pipe and only .set()
# suffers from a race condition with .close()
os.read(self._rfd, 1)
self._set = False

def set(self):
if self._set or self._closed:
return
self._set = True
os.write(self._wfd, b"*")

# This try fixes a race condition with .close()
# 1. The write/server thread sees ._closed == False and continues
# 2. The close/client thread closes the descriptors before the
# write/server thread writes.
# 3. The write/server fails to write() because the FD has been closed
try:
os.write(self._wfd, b"*")
except OSError as e:
if e.errno == errno.EBADF and self._closed:
# The pipe was closed, no need to do anything
return
raise e

def set_forever(self):
self._forever = True
Expand Down
3 changes: 3 additions & 0 deletions sites/www/changelog.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1619,3 +1619,6 @@ Changelog
- :feature:`80` Expose the internal "is closed" property of the file transfer
class ``BufferedFile`` as ``.closed``, better conforming to Python's file
interface. Thanks to ``@smunaut`` and James Hiscock for catch & patch.
- :bug:`2271` Fixes pipe ``write()`` after ``close()`` race condition in ``pipe.py``.
Ignores ``write()`` exception when the pipe was previously closed.
Thanks to Ivan Peon for the patch.