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.

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

channel: prevent infinite loop sending to closed channel #140

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jaysoffian
Copy link

Both sendall and sendall_stderr call send inside a loop and use its result to advance their buffer. If the channel is closed, send returns 0 which causes both sendall methods to go into an infinite loop. Prevent this from occurring by using the open_only decorator.

I'm not sure this is the best fix since I haven't verified whether the channel can be closed after the sending begins. If so, then both sendall routines should instead raise an exception if send returns 0.

Both sendall and sendall_stderr call send inside a loop and use its result
to advance their buffer. If the channel is closed, send returns 0 which
causes both sendall methods to go into an infinite loop. Prevent this from
occurring by using the open_only decorator.

I'm not sure this is the best fix since I haven't verified whether the channel
can be closed after the sending begins. If so, then both sendall routines should
instead raise an exception if send returns 0.
@jaysoffian jaysoffian changed the title channel: prevent infinite loop sending closed channel channel: prevent infinite loop sending to closed channel Sep 18, 2022
jaysoffian added a commit to jaysoffian/fab-classic that referenced this pull request Sep 18, 2022
When attempting to use parallel mode along with sudo, fabric was causing
paramiko-ng to go into an infinite loop. The reason was due to a combination of
fabric calling sendall() along with it misdetecting EOF on the input and closing
the output channel. The fix for paramiko-ng going into an infinite loop is here:

ploxiln/paramiko-ng#140

This commit fixes the root cause in fabric where it (apparently) erroneously 
detects the input channel as being closed when it is not.
@ploxiln
Copy link
Owner

ploxiln commented Oct 24, 2022

I'm unsure of this particular solution, because:

  • @open_only checks for both eof_sent and eof_received. stdin and stdout are independent. These send methods should probably only check eof_sent?
  • These methods call send() which calls _send(), which raises an exception if the socket is closed (another thing @open_only checks). But it does seem that _send() can just return 0 if eof_sent, maybe just that should be checked.
  • Maybe the check should be inside the while loop in these send_all methods, instead of on the outside, in case the EOF occurs after starting the method?

@jaysoffian
Copy link
Author

I'm unsure of this particular solution, because:

These are all good questions, but I was stuck with an infinite loop and this seemed like a minimally invasive fix at the time. I'll work on a more targeted fix that just avoids the infinite loop and hopefully has no other side-effects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants