[RemoteUtil] Fix output from Windows SSH commands (#90)

Adds an ArchType argument to many RemoteUtil methods, which is used to
replace -tt (forced pseudo-TTY allocation) by -T (no pseudo-TTY
allocation). The -tt option adds tons of ANSI escape sequences to the
output and makes it unparsable, even after removing the sequences, as
some sequences like "delete the last X characters" are not honoured.

An exception is BuildProcessStartInfoForSshPortForward, where
replacing -tt by -T would make the port forwarding process exit
immediately.

cdc_rsync/BUILD

@@ -179,6 +179,7 @@ cc_library(
     srcs = ["server_arch.cc"],
     hdrs = ["server_arch.h"],
     deps = [
+        "//common:ansi_filter",
         "//common:arch_type",
         "//common:path",
         "//common:remote_util",

cdc_rsync/cdc_rsync_client.cc

@@ -267,7 +267,7 @@ absl::Status CdcRsyncClient::StartServer(int port, const ServerArch& arch) {
     std::string remote_command = arch.GetStartServerCommand(
         kExitCodeNotFound, absl::StrFormat("%i %s", port, component_args));
     start_info = remote_util_->BuildProcessStartInfoForSshPortForwardAndCommand(
-        port, port, /*reverse=*/false, remote_command);
+        port, port, /*reverse=*/false, remote_command, arch.GetType());
   } else {
     // Run cdc_rsync_server locally.
     std::string exe_dir;

cdc_rsync/server_arch.cc

@@ -19,6 +19,7 @@
 #include "absl/strings/match.h"
 #include "absl/strings/str_format.h"
 #include "absl/strings/str_split.h"
+#include "common/ansi_filter.h"
 #include "common/path.h"
 #include "common/remote_util.h"
 #include "common/status_macros.h"
@@ -119,9 +120,15 @@ absl::StatusOr<ServerArch> ServerArch::DetectFromRemoteDevice(
   // Run uname, assuming it's a Linux machine.
   std::string uname_out;
   std::string linux_cmd = "uname -sm";
-  absl::Status status =
-      remote_util->RunWithCapture(linux_cmd, "uname", &uname_out, nullptr);
+  absl::Status status = remote_util->RunWithCapture(
+      linux_cmd, "uname", &uname_out, nullptr, ArchType::kLinux_x86_64);
   if (status.ok()) {
+    // Running uname on Windows, assuming it's Linux, leads to tons of ANSI
+    // escape sequences in the output. Remove them to at least get some readable
+    // output.
+    uname_out = absl::StripAsciiWhitespace(
+        ansi_filter::RemoveEscapeSequences(uname_out));
+
     LOG_DEBUG("Uname returned '%s'", uname_out);
     absl::StatusOr<ArchType> type = GetArchTypeFromUname(uname_out);
     if (type.ok()) {
@@ -142,8 +149,9 @@ absl::StatusOr<ServerArch> ServerArch::DetectFromRemoteDevice(
   std::string arch_out;
   std::string windows_cmd =
       RemoteUtil::QuoteForSsh("cmd /C set PROCESSOR_ARCHITECTURE ");
-  status = remote_util->RunWithCapture(
-      windows_cmd, "set PROCESSOR_ARCHITECTURE", &arch_out, nullptr);
+  status = remote_util->RunWithCapture(windows_cmd,
+                                       "set PROCESSOR_ARCHITECTURE", &arch_out,
+                                       nullptr, ArchType::kWindows_x86_64);
   if (status.ok()) {
     LOG_DEBUG("PROCESSOR_ARCHITECTURE is '%s'", arch_out);
     absl::StatusOr<ArchType> type = GetArchTypeFromWinProcArch(arch_out);

cdc_stream/cdc_fuse_manager.cc

@@ -143,7 +143,8 @@ absl::Status CdcFuseManager::RunFuseProcess(uint16_t local_port,
   LOG_DEBUG("Running FUSE process");
   ProcessStartInfo start_info =
       remote_util_->BuildProcessStartInfoForSshPortForwardAndCommand(
-          local_port, remote_port, true, remote_command);
+          local_port, remote_port, true, remote_command,
+          ArchType::kLinux_x86_64);
   start_info.name = kFuseFilename;
 
   // Capture stdout to determine whether a deploy is required.

common/port_manager_win.cc

@@ -280,7 +280,7 @@ absl::StatusOr<std::unordered_set<int>> PortManager::FindAvailableRemotePorts(
     SteadyClock* steady_clock) {
   std::string remote_command = GetNetstatCommand(arch_type);
   ProcessStartInfo start_info =
-      remote_util->BuildProcessStartInfoForSsh(remote_command);
+      remote_util->BuildProcessStartInfoForSsh(remote_command, arch_type);
   start_info.name = "netstat";
   start_info.flags = ProcessFlags::kNoWindow;
 

common/remote_util.cc

@@ -34,6 +34,22 @@ std::string GetPortForwardingArg(int local_port, int remote_port,
   return absl::StrFormat("-L%i:localhost:%i ", local_port, remote_port);
 }
 
+const char* GetFlagsForArch(ArchType remote_arch_type) {
+  if (IsWindowsArchType(remote_arch_type)) {
+    // Disable pseudo-TTY. Otherwise, the output is riddled by Ansi control
+    // sequences and almost impossible to parse without handling them.
+    return "-T";
+  }
+
+  if (IsLinuxArchType(remote_arch_type)) {
+    // Force pseudo-TTY.
+    return "-tt";
+  }
+
+  assert(!"Unhandled arch type");
+  return "";
+}
+
 }  // namespace
 
 RemoteUtil::RemoteUtil(std::string user_host, int verbosity, bool quiet,
@@ -136,12 +152,13 @@ absl::Status RemoteUtil::Chmod(const std::string& mode,
       absl::StrFormat("chmod %s %s %s", QuoteForSsh(mode),
                       QuoteForSsh(remote_path), quiet ? "-f" : "");
 
-  return Run(remote_command, "chmod");
+  return Run(remote_command, "chmod", ArchType::kLinux_x86_64);
 }
 
-absl::Status RemoteUtil::Run(std::string remote_command, std::string name) {
+absl::Status RemoteUtil::Run(std::string remote_command, std::string name,
+                             ArchType remote_arch_type) {
   ProcessStartInfo start_info =
-      BuildProcessStartInfoForSsh(std::move(remote_command));
+      BuildProcessStartInfoForSsh(std::move(remote_command), remote_arch_type);
   start_info.name = std::move(name);
   start_info.forward_output_to_log = forward_output_to_log_;
 
@@ -150,9 +167,10 @@ absl::Status RemoteUtil::Run(std::string remote_command, std::string name) {
 
 absl::Status RemoteUtil::RunWithCapture(std::string remote_command,
                                         std::string name, std::string* std_out,
-                                        std::string* std_err) {
+                                        std::string* std_err,
+                                        ArchType remote_arch_type) {
   ProcessStartInfo start_info =
-      BuildProcessStartInfoForSsh(std::move(remote_command));
+      BuildProcessStartInfoForSsh(std::move(remote_command), remote_arch_type);
   start_info.name = std::move(name);
   start_info.forward_output_to_log = forward_output_to_log_;
 
@@ -174,8 +192,9 @@ absl::Status RemoteUtil::RunWithCapture(std::string remote_command,
 }
 
 ProcessStartInfo RemoteUtil::BuildProcessStartInfoForSsh(
-    std::string remote_command) {
-  return BuildProcessStartInfoForSshInternal("", "-- " + remote_command);
+    std::string remote_command, ArchType remote_arch_type) {
+  return BuildProcessStartInfoForSshInternal("", "-- " + remote_command,
+                                             remote_arch_type);
 }
 
 ProcessStartInfo RemoteUtil::BuildProcessStartInfoForSshPortForward(
@@ -183,28 +202,34 @@ ProcessStartInfo RemoteUtil::BuildProcessStartInfoForSshPortForward(
   // Usually, one would pass in -N here, but this makes the connection terribly
   // slow! As a workaround, don't use -N (will open a shell), but simply eat the
   // output.
+  // Note: Don't use the Windows args now. It implies -T instead of -tt, which
+  // will make the process exit immediately.
   ProcessStartInfo si = BuildProcessStartInfoForSshInternal(
-      GetPortForwardingArg(local_port, remote_port, reverse) + "-n ", "");
+      GetPortForwardingArg(local_port, remote_port, reverse) + "-n ", "",
+      ArchType::kLinux_x86_64);
   si.stdout_handler = [](const void*, size_t) { return absl::OkStatus(); };
   return si;
 }
 
 ProcessStartInfo RemoteUtil::BuildProcessStartInfoForSshPortForwardAndCommand(
-    int local_port, int remote_port, bool reverse, std::string remote_command) {
+    int local_port, int remote_port, bool reverse, std::string remote_command,
+    ArchType remote_arch_type) {
   return BuildProcessStartInfoForSshInternal(
       GetPortForwardingArg(local_port, remote_port, reverse),
-      "-- " + remote_command);
+      "-- " + remote_command, remote_arch_type);
 }
 
 ProcessStartInfo RemoteUtil::BuildProcessStartInfoForSshInternal(
-    std::string forward_arg, std::string remote_command_arg) {
+    std::string forward_arg, std::string remote_command_arg,
+    ArchType remote_arch_type) {
   ProcessStartInfo start_info;
   start_info.command = absl::StrFormat(
-      "%s %s -tt %s "
+      "%s %s %s %s "
       "-oServerAliveCountMax=6 "  // Number of lost msgs before ssh terminates
       "-oServerAliveInterval=5 "  // Time interval between alive msgs
       "%s %s",
-      ssh_command_, quiet_ || verbosity_ < 2 ? "-q" : "", forward_arg,
+      ssh_command_, GetFlagsForArch(remote_arch_type),
+      quiet_ || verbosity_ < 2 ? "-q" : "", forward_arg,
       QuoteForWindows(user_host_), remote_command_arg);
   start_info.forward_output_to_log = forward_output_to_log_;
   start_info.flags = ProcessFlags::kNoWindow;

common/remote_util.h

@@ -21,6 +21,7 @@
 #include <vector>
 
 #include "absl/status/status.h"
+#include "common/arch_type.h"
 #include "common/process.h"
 
 namespace cdc_ft {
@@ -92,15 +93,24 @@ class RemoteUtil {
 
   // Runs |remote_command| on the remote device. The command must be properly
   // escaped. |name| is the name of the command displayed in the logs.
-  absl::Status Run(std::string remote_command, std::string name);
+  // |remote_arch_type| is the arch type of the remote device. It determines
+  // which type of pseudo console is used (-T on Windows, -tt on Linux). If the
+  // wrong arch type is passed, output might be corrupted, but otherwise the
+  // command will work.
+  absl::Status Run(std::string remote_command, std::string name,
+                   ArchType remote_arch_type);
 
   // Same as Run(), but captures both stdout and stderr.
   // If |std_out| or |std_err| are nullptr, the output is not captured.
+  // |remote_arch_type| is the arch type of the remote device, see Run().
   absl::Status RunWithCapture(std::string remote_command, std::string name,
-                              std::string* std_out, std::string* std_err);
+                              std::string* std_out, std::string* std_err,
+                              ArchType remote_arch_type);
 
   // Builds an SSH command that executes |remote_command| on the remote device.
-  ProcessStartInfo BuildProcessStartInfoForSsh(std::string remote_command);
+  // |remote_arch_type| is the arch type of the remote device, see Run().
+  ProcessStartInfo BuildProcessStartInfoForSsh(std::string remote_command,
+                                               ArchType remote_arch_type);
 
   // Builds an SSH command that runs SSH port forwarding to the remote device,
   // using the given |local_port| and |remote_port|. If |reverse| is true, sets
@@ -112,9 +122,10 @@ class RemoteUtil {
   // Builds an SSH command that executes |remote_command| on the remote device,
   // using port forwarding with given |local_port| and |remote_port|. If
   // |reverse| is true, sets up reverse port forwarding.
+  // |remote_arch_type| is the arch type of the remote device, see Run().
   ProcessStartInfo BuildProcessStartInfoForSshPortForwardAndCommand(
-      int local_port, int remote_port, bool reverse,
-      std::string remote_command);
+      int local_port, int remote_port, bool reverse, std::string remote_command,
+      ArchType remote_arch_type);
 
   // Returns whether output is suppressed.
   bool Quiet() const { return quiet_; }
@@ -145,7 +156,8 @@ class RemoteUtil {
  private:
   // Common code for BuildProcessStartInfoForSsh*.
   ProcessStartInfo BuildProcessStartInfoForSshInternal(
-      std::string forward_arg, std::string remote_command);
+      std::string forward_arg, std::string remote_command,
+      ArchType remote_arch_type);
 
   const int verbosity_;
   const bool quiet_;

common/remote_util_test.cc

@@ -59,8 +59,12 @@ class RemoteUtilTest : public ::testing::Test {
 };
 
 TEST_F(RemoteUtilTest, BuildProcessStartInfoForSsh) {
-  ProcessStartInfo si = util_.BuildProcessStartInfoForSsh(kCommand);
-  ExpectContains(si.command, {"ssh", kUserHostArg, kCommand});
+  ProcessStartInfo si =
+      util_.BuildProcessStartInfoForSsh(kCommand, ArchType::kLinux_x86_64);
+  ExpectContains(si.command, {"ssh", "-tt", kUserHostArg, kCommand});
+
+  si = util_.BuildProcessStartInfoForSsh(kCommand, ArchType::kWindows_x86_64);
+  ExpectContains(si.command, {"ssh", "-T", kUserHostArg, kCommand});
 }
 
 TEST_F(RemoteUtilTest, BuildProcessStartInfoForSshPortForward) {
@@ -75,19 +79,20 @@ TEST_F(RemoteUtilTest, BuildProcessStartInfoForSshPortForward) {
 
 TEST_F(RemoteUtilTest, BuildProcessStartInfoForSshPortForwardAndCommand) {
   ProcessStartInfo si = util_.BuildProcessStartInfoForSshPortForwardAndCommand(
-      kLocalPort, kRemotePort, kRegular, kCommand);
+      kLocalPort, kRemotePort, kRegular, kCommand, ArchType::kLinux_x86_64);
   ExpectContains(si.command,
                  {"ssh", kUserHostArg, kPortForwardingArg, kCommand});
 
   si = util_.BuildProcessStartInfoForSshPortForwardAndCommand(
-      kLocalPort, kRemotePort, kReverse, kCommand);
+      kLocalPort, kRemotePort, kReverse, kCommand, ArchType::kLinux_x86_64);
   ExpectContains(si.command,
                  {"ssh", kUserHostArg, kReversePortForwardingArg, kCommand});
 }
 TEST_F(RemoteUtilTest, BuildProcessStartInfoForSshWithCustomCommand) {
   constexpr char kCustomSshCmd[] = "C:\\path\\to\\ssh.exe --fooarg --bararg=42";
   util_.SetSshCommand(kCustomSshCmd);
-  ProcessStartInfo si = util_.BuildProcessStartInfoForSsh(kCommand);
+  ProcessStartInfo si =
+      util_.BuildProcessStartInfoForSsh(kCommand, ArchType::kLinux_x86_64);
   ExpectContains(si.command, {kCustomSshCmd});
 }