homepage

This issue tracker has been migrated to GitHub , and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: pty.py: pty.spawn hangs after client disconnect over nc (netcat)
Type: enhancement Stage:
Components: Library (Lib) Versions: Python 3.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Cornelius Diekmann, martin.panter, vstinner
Priority: normal Keywords: patch

Created on 2016年12月23日 14:17 by Cornelius Diekmann, last changed 2022年04月11日 14:58 by admin.

Files
File name Uploaded Description Edit
pty.patch Cornelius Diekmann, 2016年12月23日 14:17 Hacky patch with placeholder
pty.patch Cornelius Diekmann, 2016年12月23日 19:40 Patch with test cases. If the tests are not red on other obscure UNIXes, consider merging. review
pty.patch Cornelius Diekmann, 2016年12月23日 22:30 Same, but without git patch header review
pty.patch Cornelius Diekmann, 2016年12月23日 22:37 Same, but without git patch header. Why does the review tool not show test_pty.py? review
pty_and_tests.patch Cornelius Diekmann, 2016年12月23日 23:35 Same, but less broken diff format. review
test_pty_and_doc.patch Cornelius Diekmann, 2016年12月24日 13:59 Document and test existing behavior, no change in behavior. review
Messages (9)
msg283880 - (view) Author: Cornelius Diekmann (Cornelius Diekmann) * Date: 2016年12月23日 14:17
My OS: Debian GNU/Linux 8.6 (jessie)
Python 3.4.2
pty.py from Python-3.5.2/Lib (pty.py is just a tiny, portable python file which did not see many changes)
Bug Report
Steps to Reproduce:
I wrote a very simple python remote shell:
#!/usr/bin/env python3
import pty
pty.spawn('/bin/sh')
It can be run in a terminal (call it TermA) with `nc -e ./myfancypythonremoteshell.py -l -p 6699`
In a different terminal (call it TermB), I connect to my fancy remote shell with `nc 127.0.0.1 6699`.
The shell works fine. In TermB, I quit by pressing ctrl+c.
Observed Behavior: In TermA, the nc process, the python process, and the spawned /bin/sh still exist. They still occupy TermA.
Expected Behavior: The client in TermB has disconnected, /bin/sh in TermA can no longer read anything from stdin and it should close down. Ultimately, in TermA, nc should have exited successfully.
Fix: End the _copy loop in pty.py once EOF in STDIN is reached. Everything shuts itself down automatically. I included a small patch to demonstrate this behavior.
This patch is not meant to go straight into production. I have not verified if this behavior somehow breaks other use cases. This bug report is meant to document exactly one specific use case and supply exactly one line of code change for it.
This issue is related to issue26228. Actually, it is complementary. issue26228 wants to return if master_fd is EOF, this issue wants to return if stdin is EOF. Both behaviors together looks good to me. By the way, I hacked a hacky `assert False` into my patch as a placeholder for issue26228's proper handling of exec failures at that part of the code.
I suggest to combine the patches of this issue and issue26228.
msg283899 - (view) Author: Cornelius Diekmann (Cornelius Diekmann) * Date: 2016年12月23日 19:40
I wrote a proper patch for the issue of handling EOF in STDIN, including tests. My patch is against the github mirror head, but don't worry, the files I touch haven't been touched in recent years ;-)
I only tested on Linux. My patch only addresses the issue in this thread. It does not include the patch for issue26228. I still recommend to also merge the patch for issue26228 (but I don't have a FreeBSD box to test).
msg283904 - (view) Author: Cornelius Diekmann (Cornelius Diekmann) * Date: 2016年12月23日 22:30
Removed git patch header from pty.patch to make python code review tool happy. Sorry, this is my first contribution.
msg283905 - (view) Author: Cornelius Diekmann (Cornelius Diekmann) * Date: 2016年12月23日 22:37
Review tool still did not show the test_pty.py file. Sry.
msg283909 - (view) Author: Cornelius Diekmann (Cornelius Diekmann) * Date: 2016年12月23日 23:35
Make review tool happy by giving it less broken patch format :)
`make patchcheck` is already happy.
Sorry for the noise :(
msg283912 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016年12月24日 01:25
This is a change in behaviour of the _copy() loop: it will stop as soon as EOF is read from the parent’s input, and immediately close the terminal master. Unpatched, the loop continues to read output from the child, until the child closes the terminal slave.
I agree that your new behaviour may be desired in some cases, but you need to respect backwards compatibility. With your patch, things will no longer work robustly when the child "has the last word", i.e. it writes output and exits without reading any (extra) input. Simple example: the child prints a message, but the parent has no input:
python -c 'import pty; pty.spawn("./message.py")' < /dev/null
Any new functionality would also need documenting. (If you want to suggest some wording to document the existing behaviour better, that would also be welcome :)
msg283952 - (view) Author: Cornelius Diekmann (Cornelius Diekmann) * Date: 2016年12月24日 13:59
Thank you Martin very much. To resolve this issue, I decided to document the current behavior and add test cases for it. No change in behavior is introduced. This hopefully allows to close this issue. The test cases for the current behavior ensure that we can (at some point in the future) add some different behavior without breaking backwards compatibility.
Fixed: Observed behavior is now expected+documented behavior. Improved test cases.
Happy Holidays!
msg284494 - (view) Author: Cornelius Diekmann (Cornelius Diekmann) * Date: 2017年01月02日 18:43
Status change: I proposed a generic test suite for pty.spawn() in issue29070. Once we have agreed on the current behavior of pty.spawn() and the test suite is merged, I would like to come back to this issue which requests for a change in behavior of pty.spawn(). Currently, I would like to document that this issue is waiting for issue29070 and this issue doesn't need any attention.
msg284816 - (view) Author: Cornelius Diekmann (Cornelius Diekmann) * Date: 2017年01月06日 12:37
[no status change, this issue currently does NOT need any attention]
To keep issues separate, I just wanted to document a comment about this issue mentioned in issue29070. It refers to the _copy loop.
 if STDIN_FILENO in rfds:
 data = stdin_read(STDIN_FILENO)
 if not data:
 fds.remove(STDIN_FILENO)
+ # Proposal for future behavior change: Signal EOF to
+ # slave if STDIN of master is gone. Solves issue29054.
+ # os.write(master_fd, b'\x04')
 else:
 _writen(master_fd, data)
> vadmium 2017年01月04日 21:50:26
> I suggest leaving this for the other [issue29054, i.e. this] bug. Another option may be to send SIGHUP
> (though I am far from an expert on Unix terminals :).
http://bugs.python.org/review/29070/diff/19626/Lib/pty.py 
History
Date User Action Args
2022年04月11日 14:58:41adminsetgithub: 73240
2017年01月06日 12:37:37Cornelius Diekmannsetmessages: + msg284816
2017年01月03日 12:24:01vstinnersetnosy: + vstinner
2017年01月02日 18:43:10Cornelius Diekmannsetmessages: + msg284494
2016年12月24日 13:59:24Cornelius Diekmannsetfiles: + test_pty_and_doc.patch

messages: + msg283952
2016年12月24日 01:25:23martin.pantersetmessages: + msg283912
versions: - Python 3.4, Python 3.5, Python 3.6
2016年12月23日 23:35:28Cornelius Diekmannsetfiles: + pty_and_tests.patch

messages: + msg283909
2016年12月23日 22:37:13Cornelius Diekmannsetfiles: + pty.patch

messages: + msg283905
2016年12月23日 22:30:43Cornelius Diekmannsetfiles: + pty.patch

messages: + msg283904
2016年12月23日 19:40:39Cornelius Diekmannsetfiles: + pty.patch

messages: + msg283899
versions: + Python 3.6, Python 3.7
2016年12月23日 14:17:17Cornelius Diekmanncreate

AltStyle によって変換されたページ (->オリジナル) /