From 24906eb36ef0af70895d83536d4fa930ef8aca77 Mon Sep 17 00:00:00 2001 From: Lutz Justen Date: Mon, 6 Feb 2023 18:42:00 +0100 Subject: [PATCH] [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 | 1 + cdc_rsync/cdc_rsync_client.cc | 2 +- cdc_rsync/server_arch.cc | 16 ++++++++--- cdc_stream/cdc_fuse_manager.cc | 3 +- common/port_manager_win.cc | 2 +- common/remote_util.cc | 51 +++++++++++++++++++++++++--------- common/remote_util.h | 24 ++++++++++++---- common/remote_util_test.cc | 15 ++++++---- 8 files changed, 83 insertions(+), 31 deletions(-) diff --git a/cdc_rsync/BUILD b/cdc_rsync/BUILD index 4c648e0..6ac7f7f 100644 --- a/cdc_rsync/BUILD +++ b/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", diff --git a/cdc_rsync/cdc_rsync_client.cc b/cdc_rsync/cdc_rsync_client.cc index a002161..b45cbf7 100644 --- a/cdc_rsync/cdc_rsync_client.cc +++ b/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; diff --git a/cdc_rsync/server_arch.cc b/cdc_rsync/server_arch.cc index 694c231..27709a7 100644 --- a/cdc_rsync/server_arch.cc +++ b/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::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 type = GetArchTypeFromUname(uname_out); if (type.ok()) { @@ -142,8 +149,9 @@ absl::StatusOr 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 type = GetArchTypeFromWinProcArch(arch_out); diff --git a/cdc_stream/cdc_fuse_manager.cc b/cdc_stream/cdc_fuse_manager.cc index 5e411c9..8b5fefb 100644 --- a/cdc_stream/cdc_fuse_manager.cc +++ b/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. diff --git a/common/port_manager_win.cc b/common/port_manager_win.cc index 27f0565..0617507 100644 --- a/common/port_manager_win.cc +++ b/common/port_manager_win.cc @@ -280,7 +280,7 @@ absl::StatusOr> 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; diff --git a/common/remote_util.cc b/common/remote_util.cc index 4db64d1..05272af 100644 --- a/common/remote_util.cc +++ b/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; diff --git a/common/remote_util.h b/common/remote_util.h index 94cb300..3db22b4 100644 --- a/common/remote_util.h +++ b/common/remote_util.h @@ -21,6 +21,7 @@ #include #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_; diff --git a/common/remote_util_test.cc b/common/remote_util_test.cc index 3017e5b..c462c13 100644 --- a/common/remote_util_test.cc +++ b/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}); }