Revert "sshdriver: Prevent timeout from deadlock"

This reverts commit b8f059f.

OpenSSH < 8.5 called with -f waits for the stderr pipe to be closed
before forking. [1] fixes this behavior.
The labgrid commit to be reverted calls communicate(), relying on a
timely fork. Affected OpenSSH versions now run into a TimeoutExpired
exception on communicate() leading to an ExecutionError being raised in
labgrid.

A wait() with timeout was used since the initial implementation [2].
We wanted to make sure that we don't depend on the state of the pipes, so
use of wait() was intentional, as it directly covers the interesting cases:

- immediate abort (due to a config error or similar)
- normal startup (parent process exits after fork)
- hang (via wait() timeout)

Reverting the problematic commit avoids the complexity of having to
maintain two different ways to start SSH as suggested in [3].

If timeouts still occur after the revert, we should implement this
suggestion [4].

Discussion that lead to this patch can be found in the PR introducing
the problematic commit [5] as well as in [3].

The other commit [6] in the PR [5] has a similar approach for
`labgrid.util.SSHConnection._start_own_master()`. It shouldn't
be problematic though: The ssh process is not expected to fork as it is
not called with -f.

[1] OpenSSH: 396d32f3a ("upstream: There are lots of place where we want to redirect stdin,")
[2] e4862fa ("SSHDriver: ControlMaster & Driver")
[3] labgrid-project#1278
[4] labgrid-project#1265 (comment)
[5] labgrid-project#1265
[6] f9ca024 ("ssh: Prevent timeout from deadlock")

Signed-off-by: Bastian Krause <[email protected]>

labgrid/driver/sshdriver.py

@@ -138,8 +138,9 @@ def _start_own_master_once(self, timeout):
 
         try:
             subprocess_timeout = timeout + 5
-            stdout, _ = self.process.communicate(timeout=subprocess_timeout)
-            if self.process.returncode != 0:
+            return_value = self.process.wait(timeout=subprocess_timeout)
+            if return_value != 0:
+                stdout, _ = self.process.communicate(timeout=subprocess_timeout)
                 stdout = stdout.split(b"\n")
                 for line in stdout:
                     self.logger.warning("ssh: %s", line.rstrip().decode(encoding="utf-8", errors="replace"))
@@ -156,14 +157,12 @@ def _start_own_master_once(self, timeout):
                     pass
 
                 raise ExecutionError(
-                    f"Failed to connect to {self.networkservice.address} with {' '.join(args)}: return code {self.process.returncode}",  # pylint: disable=line-too-long
+                    f"Failed to connect to {self.networkservice.address} with {' '.join(args)}: return code {return_value}",  # pylint: disable=line-too-long
                     stdout=stdout,
                 )
         except subprocess.TimeoutExpired:
-            self.process.kill()
-            stdout, _ = self.process.communicate()
             raise ExecutionError(
-                f"Subprocess timed out [{subprocess_timeout}s] while executing {args}: {stdout}",
+                f"Subprocess timed out [{subprocess_timeout}s] while executing {args}",
             )
         finally:
             if self.networkservice.password and os.path.exists(pass_file):

tests/test_sshdriver.py

@@ -17,8 +17,6 @@ def ssh_driver_mocked_and_activated(target, mocker):
     instance_mock = mocker.MagicMock()
     popen.return_value = instance_mock
     instance_mock.wait = mocker.MagicMock(return_value=0)
-    instance_mock.communicate = mocker.MagicMock(return_value=(b"", b""))
-    instance_mock.returncode = 0
     SSHDriver(target, "ssh")
     s = target.get_driver("SSHDriver")
     return s
@@ -37,8 +35,6 @@ def test_create(target, mocker):
     instance_mock = mocker.MagicMock()
     popen.return_value = instance_mock
     instance_mock.wait = mocker.MagicMock(return_value=0)
-    instance_mock.communicate = mocker.MagicMock(return_value=(b"", b""))
-    instance_mock.returncode = 0
     s = SSHDriver(target, "ssh")
     assert isinstance(s, SSHDriver)