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 2017-01-06 12:37 by Cornelius Diekmann.

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
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