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

Changes from 2 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
11 changes: 8 additions & 3 deletions paramiko/pipe.py
View file
Open in desktop
Expand Up @@ -46,10 +46,9 @@ def __init__(self):
self._closed = False

def close(self):
self._closed = True
os.close(self._rfd)
os.close(self._wfd)
Copy link
Contributor

Choose a reason for hiding this comment

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

@pettitpeon, is there any chance of either of these two .close() calls failing, making self._closed == True a possibly inaccurate representation of the pipe state?

(I would think that a failure of either of these two calls would signal an error state for the pipe such that self._closed == True is a better state than == False... but I thought I'd raise the question.)

Copy link
Author

Choose a reason for hiding this comment

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

is there any chance of either of these two .close() calls failing?

Yes, it could happen. If either .close() fails, it would raise an OSError. In that case some FD might stay open, but the Pipe is not usable anyways. There is no use-case of using it after .close(). Worst case scenario, we leak resources until the application exits. The most probable scenario is that the close() exception is not handled and your program terminates anyways.
A small improvement would be to have single trys on each .close(). In that case if, the first fails and leaks, we might still close the second one correctly.
In any case, I see .close() errors as non-handable, we close() file descriptors as a "best-effort" to release the resource, but cannot really do anything if it fails.

making self._closed == True a possibly inaccurate representation of the pipe state?

Not really, after close() has been called, the object is disposed and no one should it anyways. The "broken state" is meaningless.

Last, the pipe is most probably a child of Channel and when the channel gets destroyed it tries to re-close, which fails if the pipe was previously closed, but it is disregarded by a catch-all try. This is OK, and aligns with the idea that it is a non-handable error.

paramiko/paramiko/channel.py

Line 136 in f4d7fec

def __del__(self):

# used for unit tests:
self._closed = True

def fileno(self):
return self._rfd
Expand All @@ -64,7 +63,13 @@ def set(self):
if self._set or self._closed:
return
self._set = True
os.write(self._wfd, b"*")
try:
os.write(self._wfd, b"*")
except OSError as e:
if e.errno == 9 and self._closed:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible for this ifcondition to ever be True?

We've already trapped for self._closed == True with the first if in the method, so it seems like we'd only ever hit this point with self._closed == False.

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 flowing (race condition) can happen

  1. The write thread sees ._closed == False and continues
  2. The close thread closes the descriptors before the write thread writes on line 67
  3. The write fails to write() because the FD has been closed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Aha, this is the crux of the race condition, got it.

For the benefit of future readers of the code, please add a comment here describing the race condition as you just described it.

Otherwise we risk someone thinking the same thing I did and removing this.

Copy link
Author

Choose a reason for hiding this comment

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

done

pettitpeon marked this conversation as resolved.
Show resolved Hide resolved
# The pipe was closed, no need to do anything
return
raise e
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we not also need something analogous to protect the os.read(self._rfd, 1) call above in clear()?

(We might not; again, just raising the question.)

Copy link
Author

@pettitpeon pettitpeon Aug 2, 2023 •
edited

Choose a reason for hiding this comment

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

From the pipe's perspective, yes. And it would be wise to protect it. However, the way the pipe is used, this does not happen. The same thread

  1. Opens,
  2. clears and
  3. closes

the pipe. So there is no race condition between .clear() and `.close().
A second thread sets it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same request, please add a comment here explaining why a similar race catch is not (currently) required here.

Copy link
Author

Choose a reason for hiding this comment

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

done


def set_forever(self):
self._forever = True
Expand Down