Implement #322 - new ChannelFile subclass for stdin that calls shutdown_write on close()

6 files changed, 61 insertions, 9 deletions

diff --git a/paramiko/__init__.py b/paramiko/__init__.py
index 6afc7a61..2b9d8895 100644
--- a/paramiko/__init__.py
+++ b/paramiko/__init__.py
@@ -29,7 +29,7 @@ from paramiko.client import (
 )
 from paramiko.auth_handler import AuthHandler
 from paramiko.ssh_gss import GSSAuth, GSS_AUTH_AVAILABLE, GSS_EXCEPTIONS
-from paramiko.channel import Channel, ChannelFile, ChannelStderrFile
+from paramiko.channel import Channel, ChannelFile, ChannelStderrFile, ChannelStdinFile
 from paramiko.ssh_exception import (
     SSHException,
     PasswordRequiredException,
diff --git a/paramiko/channel.py b/paramiko/channel.py
index 2687e6fc..d7938b19 100644
--- a/paramiko/channel.py
+++ b/paramiko/channel.py
@@ -896,6 +896,23 @@ class Channel(ClosingContextManager):
         """
         return ChannelStderrFile(*([self] + list(params)))
 
+    def makefile_stdin(self, *params):
+        """
+        Return a file-like object associated with this channel's stdin
+        stream.
+
+        The optional ``mode`` and ``bufsize`` arguments are interpreted the
+        same way as by the built-in ``file()`` function in Python.  For a
+        client, it only makes sense to open this file for writing.  For a
+        server, it only makes sense to open this file for reading.
+
+        :returns:
+            `.ChannelStdinFile` object which can be used for Python file I/O.
+
+        .. versionadded:: 2.6
+        """
+        return ChannelStdinFile(*([self] + list(params)))
+
     def fileno(self):
         """
         Returns an OS-level file descriptor which can be used for polling, but
@@ -1355,3 +1372,9 @@ class ChannelStderrFile(ChannelFile):
     def _write(self, data):
         self.channel.sendall_stderr(data)
         return len(data)
+
+
+class ChannelStdinFile(ChannelFile):
+    def close(self):
+        super(ChannelFile, self).close()
+        self.channel.shutdown_write()
diff --git a/paramiko/client.py b/paramiko/client.py
index 6bf479d4..a47efbfe 100644
--- a/paramiko/client.py
+++ b/paramiko/client.py
@@ -503,7 +503,7 @@ class SSHClient(ClosingContextManager):
         if environment:
             chan.update_environment(environment)
         chan.exec_command(command)
-        stdin = chan.makefile("wb", bufsize)
+        stdin = chan.makefile_stdin("wb", bufsize)
         stdout = chan.makefile("r", bufsize)
         stderr = chan.makefile_stderr("r", bufsize)
         return stdin, stdout, stderr
diff --git a/sites/www/changelog.rst b/sites/www/changelog.rst
index 41c50ce5..abd73b06 100644
--- a/sites/www/changelog.rst
+++ b/sites/www/changelog.rst
@@ -2,6 +2,17 @@
 Changelog
 =========
 
+- :bug:`322 major` `SSHClient.exec_command
+  <paramiko.client.SSHClient.exec_command>` previously returned a naive
+  `~paramiko.channel.ChannelFile` object for its ``stdin`` value; such objects
+  don't know to properly shut down the remote end's stdin when they
+  ``.close()``. This leads to issues when running remote commands that read
+  from stdin.
+
+  A new subclass, `~paramiko.channel.ChannelStdinFile`, has been created which
+  closes remote stdin when it itself is closed.
+  `~paramiko.client.SSHClient.exec_command` has been updated to use that class
+  for its ``stdin`` return value.
 - :release:`2.5.0 <2019-06-09>`
 - :feature:`1233` (also :issue:`1229`, :issue:`1332`) Add support for
   encrypt-then-MAC (ETM) schemes (``hmac-sha2-256-etm@openssh.com``,
diff --git a/tests/test_channelfile.py b/tests/test_channelfile.py
index 6faf8bf8..707a49c0 100644
--- a/tests/test_channelfile.py
+++ b/tests/test_channelfile.py
@@ -1,32 +1,36 @@
 from mock import patch, MagicMock
 
-from paramiko import Channel, ChannelFile, ChannelStderrFile
+from paramiko import Channel, ChannelFile, ChannelStderrFile, ChannelStdinFile
 
 
-class TestChannelFile(object):
+class ChannelFileBase(object):
     @patch("paramiko.channel.ChannelFile._set_mode")
     def test_defaults_to_unbuffered_reading(self, setmode):
-        cf = ChannelFile(Channel(None))
+        cf = self.klass(Channel(None))
         setmode.assert_called_once_with("r", -1)
 
     @patch("paramiko.channel.ChannelFile._set_mode")
     def test_can_override_mode_and_bufsize(self, setmode):
-        cf = ChannelFile(Channel(None), mode="w", bufsize=25)
+        cf = self.klass(Channel(None), mode="w", bufsize=25)
         setmode.assert_called_once_with("w", 25)
 
     def test_read_recvs_from_channel(self):
         chan = MagicMock()
-        cf = ChannelFile(chan)
+        cf = self.klass(chan)
         cf.read(100)
         chan.recv.assert_called_once_with(100)
 
     def test_write_calls_channel_sendall(self):
         chan = MagicMock()
-        cf = ChannelFile(chan, mode="w")
+        cf = self.klass(chan, mode="w")
         cf.write("ohai")
         chan.sendall.assert_called_once_with(b"ohai")
 
 
+class TestChannelFile(ChannelFileBase):
+    klass = ChannelFile
+
+
 class TestChannelStderrFile(object):
     def test_read_calls_channel_recv_stderr(self):
         chan = MagicMock()
@@ -40,3 +44,17 @@ class TestChannelStderrFile(object):
         cf.write("ohai")
         chan.sendall_stderr.assert_called_once_with(b"ohai")
 
+
+class TestChannelStdinFile(ChannelFileBase):
+    klass = ChannelStdinFile
+
+    def test_close_calls_channel_shutdown_write(self):
+        chan = MagicMock()
+        cf = ChannelStdinFile(chan, mode="wb")
+        cf.flush = MagicMock()
+        cf.close()
+        # Sanity check that we still call BufferedFile.close()
+        cf.flush.assert_called_once_with()
+        assert cf._closed is True
+        # Actual point of test
+        chan.shutdown_write.assert_called_once_with()
diff --git a/tests/test_client.py b/tests/test_client.py
index 26de2d37..c46c383b 100644
--- a/tests/test_client.py
+++ b/tests/test_client.py
@@ -213,7 +213,7 @@ class ClientTest(unittest.TestCase):
 
         # Nobody else tests the API of exec_command so let's do it here for
         # now. :weary:
-        assert isinstance(stdin, paramiko.ChannelFile)
+        assert isinstance(stdin, paramiko.ChannelStdinFile)
         assert isinstance(stdout, paramiko.ChannelFile)
         assert isinstance(stderr, paramiko.ChannelStderrFile)