From 1251728a3d855221d6d6066e1de0ffb50f1057f8 Mon Sep 17 00:00:00 2001 From: John Hood Date: Wed, 22 Apr 2020 04:35:41 -0400 Subject: [PATCH 1/4] mosh.pl: remove unnecessary block Early exit from a block is clearer. --- scripts/mosh.pl | 92 ++++++++++++++++++++++++------------------------- 1 file changed, 46 insertions(+), 46 deletions(-) diff --git a/scripts/mosh.pl b/scripts/mosh.pl index 56e96d76e..a5130e55a 100755 --- a/scripts/mosh.pl +++ b/scripts/mosh.pl @@ -409,62 +409,62 @@ sub predict_check { my @exec_argv = ( @ssh, @sshopts, $userhost, '--', $ssh_connection . "$server " . shell_quote( @server ) ); exec @exec_argv; die "Cannot exec ssh: $!\n"; -} else { # parent - my ( $sship, $port, $key ); - my $bad_udp_port_warning = 0; - LINE: while ( <$pipe> ) { - chomp; - if ( m{^MOSH IP } ) { - if ( defined $ip ) { - die "$0 error: detected attempt to redefine MOSH IP.\n"; - } - ( $ip ) = m{^MOSH IP (\S+)\s*$} or die "Bad MOSH IP string: $_\n"; - } elsif ( m{^MOSH SSH_CONNECTION } ) { - my @words = split; - if ( scalar @words == 6 ) { - $sship = $words[4]; - } else { - die "Bad MOSH SSH_CONNECTION string: $_\n"; - } - } elsif ( m{^MOSH CONNECT } ) { - if ( ( $port, $key ) = m{^MOSH CONNECT (\d+?) ([A-Za-z0-9/+]{22})\s*$} ) { - last LINE; - } else { - die "Bad MOSH CONNECT string: $_\n"; - } +} +# parent +my ( $sship, $port, $key ); +my $bad_udp_port_warning = 0; +LINE: while ( <$pipe> ) { + chomp; + if ( m{^MOSH IP } ) { + if ( defined $ip ) { + die "$0 error: detected attempt to redefine MOSH IP.\n"; + } + ( $ip ) = m{^MOSH IP (\S+)\s*$} or die "Bad MOSH IP string: $_\n"; + } elsif ( m{^MOSH SSH_CONNECTION } ) { + my @words = split; + if ( scalar @words == 6 ) { + $sship = $words[4]; } else { - if ( defined $port_request and $port_request =~ m{:} and m{Bad UDP port} ) { - $bad_udp_port_warning = 1; - } - print "$_\n"; + die "Bad MOSH SSH_CONNECTION string: $_\n"; } - } - waitpid $pid, 0; - close $pipe; - - if ( not defined $ip ) { - if ( defined $sship ) { - warn "$0: Using remote IP address ${sship} from \$SSH_CONNECTION for hostname ${userhost}\n"; - $ip = $sship; + } elsif ( m{^MOSH CONNECT } ) { + if ( ( $port, $key ) = m{^MOSH CONNECT (\d+?) ([A-Za-z0-9/+]{22})\s*$} ) { + last LINE; } else { - die "$0: Did not find remote IP address (is SSH ProxyCommand disabled?).\n"; + die "Bad MOSH CONNECT string: $_\n"; + } + } else { + if ( defined $port_request and $port_request =~ m{:} and m{Bad UDP port} ) { + $bad_udp_port_warning = 1; } + print "$_\n"; } +} +waitpid $pid, 0; +close $pipe; - if ( not defined $key or not defined $port ) { - if ( $bad_udp_port_warning ) { - die "$0: Server does not support UDP port range option.\n"; - } - die "$0: Did not find mosh server startup message. (Have you installed mosh on your server?)\n"; +if ( not defined $ip ) { + if ( defined $sship ) { + warn "$0: Using remote IP address ${sship} from \$SSH_CONNECTION for hostname ${userhost}\n"; + $ip = $sship; + } else { + die "$0: Did not find remote IP address (is SSH ProxyCommand disabled?).\n"; } +} - # Now start real mosh client - $ENV{ 'MOSH_KEY' } = $key; - $ENV{ 'MOSH_PREDICTION_DISPLAY' } = $predict; - $ENV{ 'MOSH_NO_TERM_INIT' } = '1' if !$term_init; - exec {$client} ("$client", "-# @cmdline |", $ip, $port); +if ( not defined $key or not defined $port ) { + if ( $bad_udp_port_warning ) { + die "$0: Server does not support UDP port range option.\n"; + } + die "$0: Did not find mosh server startup message. (Have you installed mosh on your server?)\n"; } +# Now start real mosh client +$ENV{ 'MOSH_KEY' } = $key; +$ENV{ 'MOSH_PREDICTION_DISPLAY' } = $predict; +$ENV{ 'MOSH_NO_TERM_INIT' } = '1' if !$term_init; +exec {$client} ("$client", "-# @cmdline |", $ip, $port); + sub shell_quote { join ' ', map {(my $a = $_) =~ s/'/'\\''/g; "'$a'"} @_ } sub locale_vars { From f39e1cc879ba880677fe5b6073064dccbd0d3bc2 Mon Sep 17 00:00:00 2001 From: John Hood Date: Wed, 22 Apr 2020 05:04:20 -0400 Subject: [PATCH 2/4] mosh.pl: Move server argv construction to parent from child. This allows its use for error messages in the parent-- used in the next commit. --- scripts/mosh.pl | 96 +++++++++++++++++++++++++++---------------------- 1 file changed, 53 insertions(+), 43 deletions(-) diff --git a/scripts/mosh.pl b/scripts/mosh.pl index a5130e55a..505381e72 100755 --- a/scripts/mosh.pl +++ b/scripts/mosh.pl @@ -350,65 +350,75 @@ sub predict_check { $userhost = "$user$ip"; } -my $pid = open(my $pipe, "-|"); -die "$0: fork: $!\n" unless ( defined $pid ); -if ( $pid == 0 ) { # child - open(STDERR, ">&STDOUT") or die; +# Construct exec arguments. - my @sshopts = ( '-n' ); - if ($ssh_pty) { - push @sshopts, '-tt'; - } +my @sshopts = ( '-n' ); +if ($ssh_pty) { + push @sshopts, '-tt'; +} - my $ssh_connection = ""; - if ( $use_remote_ip eq 'remote' ) { - # Ask the server for its IP. The user's shell may not be - # Posix-compatible so invoke sh explicitly. - $ssh_connection = "sh -c " . - shell_quote ( '[ -n "$SSH_CONNECTION" ] && printf "\nMOSH SSH_CONNECTION %s\n" "$SSH_CONNECTION"' ) . - " ; "; - # Only with 'remote', we may need to tell SSH which protocol to use. - if ( $family eq 'inet' ) { - push @sshopts, '-4'; - } elsif ( $family eq 'inet6' ) { - push @sshopts, '-6'; - } +my $ssh_connection = ""; +if ( $use_remote_ip eq 'remote' ) { + # Ask the server for its IP. The user's shell may not be + # Posix-compatible so invoke sh explicitly. + $ssh_connection = "sh -c " . + shell_quote ( '[ -n "$SSH_CONNECTION" ] && printf "\nMOSH SSH_CONNECTION %s\n" "$SSH_CONNECTION"' ) . + " ; "; + # Only with 'remote', we may need to tell SSH which protocol to use. + if ( $family eq 'inet' ) { + push @sshopts, '-4'; + } elsif ( $family eq 'inet6' ) { + push @sshopts, '-6'; } - my @server = ( 'new' ); +} +my @server = ( 'new' ); - push @server, ( '-c', $colors ); +push @server, ( '-c', $colors ); - push @server, @bind_arguments; +push @server, @bind_arguments; - if ( defined $port_request ) { - push @server, ( '-p', $port_request ); - } +if ( defined $port_request ) { + push @server, ( '-p', $port_request ); +} - for ( &locale_vars ) { - push @server, ( '-l', $_ ); - } +for ( &locale_vars ) { + push @server, ( '-l', $_ ); +} + +if ( scalar @command > 0 ) { + push @server, '--', @command; +} + +if ( $use_remote_ip eq 'proxy' ) { + my $quoted_proxy_command = shell_quote( $0, "--family=$family" ); + push @sshopts, ( '-S', 'none', '-o', "ProxyCommand=$quoted_proxy_command --fake-proxy -- %h %p" ); +} +my @exec_argv = ( @ssh, @sshopts, $userhost, '--', $ssh_connection . "$server " . shell_quote( @server ) ); + +# Override command line for local execution. +if ( defined( $localhost )) { + @exec_argv = ( "$server " . shell_quote( @server ) ); +} + +my $pid = open(my $pipe, "-|"); +die "$0: fork: $!\n" unless ( defined $pid ); +if ( $pid == 0 ) { # child + open(STDERR, ">&STDOUT") or die; - if ( scalar @command > 0 ) { - push @server, '--', @command; + if ( !defined( $localhost) && $use_remote_ip eq 'proxy' ) { + # Non-standard shells and broken shrc files cause the ssh + # proxy to break mysteriously. + $ENV{ 'SHELL' } = '/bin/sh'; } if ( defined( $localhost )) { delete $ENV{ 'SSH_CONNECTION' }; chdir; # $HOME print "MOSH IP ${userhost}\n"; - exec( "$server " . shell_quote( @server ) ); - die "Cannot exec $server: $!\n"; - } - if ( $use_remote_ip eq 'proxy' ) { - # Non-standard shells and broken shrc files cause the ssh - # proxy to break mysteriously. - $ENV{ 'SHELL' } = '/bin/sh'; - my $quoted_proxy_command = shell_quote( $0, "--family=$family" ); - push @sshopts, ( '-S', 'none', '-o', "ProxyCommand=$quoted_proxy_command --fake-proxy -- %h %p" ); } - my @exec_argv = ( @ssh, @sshopts, $userhost, '--', $ssh_connection . "$server " . shell_quote( @server ) ); + exec @exec_argv; - die "Cannot exec ssh: $!\n"; + die "Cannot exec child: $!\n"; } # parent my ( $sship, $port, $key ); From 00b3688fb33d36d53a726627a7b1af1c37030ee4 Mon Sep 17 00:00:00 2001 From: John Hood Date: Wed, 22 Apr 2020 05:23:04 -0400 Subject: [PATCH 3/4] mosh.pl: Report errors on server command failure. I think this will help with the papercut of "Did not find mosh server startup message". This message confuses people; often the real problem is that the command to start the remote mosh-server failed somehow. Fixes #1042, partially resolves #1005. Also relevant for #1042 and countless questions on IRC. --- scripts/mosh.pl | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/scripts/mosh.pl b/scripts/mosh.pl index 505381e72..249bbc370 100755 --- a/scripts/mosh.pl +++ b/scripts/mosh.pl @@ -350,8 +350,7 @@ sub predict_check { $userhost = "$user$ip"; } -# Construct exec arguments. - +# Construct server exec arguments. my @sshopts = ( '-n' ); if ($ssh_pty) { push @sshopts, '-tt'; @@ -450,8 +449,13 @@ sub predict_check { print "$_\n"; } } -waitpid $pid, 0; -close $pipe; +if ( not close $pipe ) { + if ( $! == 0 ) { + die_on_exitstatus($?, "server command", shell_quote(@exec_argv)); + } else { + die("$0: error closing server pipe: $!\n") + } +} if ( not defined $ip ) { if ( defined $sship ) { @@ -546,3 +550,20 @@ sub resolvename { } return @res; } + +sub die_on_exitstatus { + my ($exitstatus, $what_command, $exec_string) = @_; + + if (POSIX::WIFSIGNALED($exitstatus)) { + my $termsig = POSIX::WTERMSIG($exitstatus); + die("$0: $what_command exited on signal $termsig: $exec_string\n" ); + } + if (!POSIX::WIFEXITED($exitstatus)) { + die("$0: $what_command unexpectedly terminated with exit status $exitstatus: $exec_string\n"); + } + my $exitcode = POSIX::WEXITSTATUS($exitstatus); + if ($exitcode != 0) { + die("$0: $what_command failed with exitstatus $exitcode: $exec_string\n"); + } + return; +} From b6e7f3ec5e16b2dcf7a9ec2d63c1a54358530676 Mon Sep 17 00:00:00 2001 From: John Hood Date: Tue, 12 May 2020 20:32:21 -0400 Subject: [PATCH 4/4] server-failure.test: new test to check error reporting. --- src/tests/Makefile.am | 1 + src/tests/e2e-test | 28 +++++++------- src/tests/server-failure.test | 69 +++++++++++++++++++++++++++++++++++ 3 files changed, 84 insertions(+), 14 deletions(-) create mode 100755 src/tests/server-failure.test diff --git a/src/tests/Makefile.am b/src/tests/Makefile.am index 4863af61c..322842a81 100644 --- a/src/tests/Makefile.am +++ b/src/tests/Makefile.am @@ -31,6 +31,7 @@ displaytests = \ pty-deadlock.test \ repeat.test \ repeat-with-input.test \ + server-failure.test \ server-network-timeout.test \ server-signal-timeout.test \ window-resize.test \ diff --git a/src/tests/e2e-test b/src/tests/e2e-test index c0e9d27d0..ebfcfa69f 100755 --- a/src/tests/e2e-test +++ b/src/tests/e2e-test @@ -237,21 +237,21 @@ for run in $server_tests; do if [ "$server_rv" -ne 0 ]; then test_error "server harness exited with status %s\n" "$server_rv" fi - fi - if [ "${run}" != "direct" ]; then - # Check for "round-trip" failures - if grep -q "round-trip Instruction verification failed" "${test_dir}/${run}.server.stderr"; then - test_error "Round-trip Instruction verification failed on server during %s\n" "$run" - fi - # Check for 0-timeout select() issue - if egrep -q "(polls, rate limiting|consecutive polls)" "${test_dir}/${run}.server.stderr"; then - if [ "osx" != "${TRAVIS_OS_NAME}" ]; then - test_error "select() with zero timeout called too often on server during %s\n" "$run" + if [ "${run}" != "direct" ]; then + # Check for "round-trip" failures + if grep -q "round-trip Instruction verification failed" "${test_dir}/${run}.server.stderr"; then + test_error "Round-trip Instruction verification failed on server during %s\n" "$run" + fi + # Check for 0-timeout select() issue + if egrep -q "(polls, rate limiting|consecutive polls)" "${test_dir}/${run}.server.stderr"; then + if [ "osx" != "${TRAVIS_OS_NAME}" ]; then + test_error "select() with zero timeout called too often on server during %s\n" "$run" + fi + fi + # Check for assert() + if egrep -q "assertion.*failed" "${test_dir}/${run}.server.stderr"; then + test_error "assertion during %s\n" "$run" fi - fi - # Check for assert() - if egrep -q "assertion.*failed" "${test_dir}/${run}.server.stderr"; then - test_error "assertion during %s\n" "$run" fi fi # XXX We'd also like to check for "target state Instruction diff --git a/src/tests/server-failure.test b/src/tests/server-failure.test new file mode 100755 index 000000000..be6ec5150 --- /dev/null +++ b/src/tests/server-failure.test @@ -0,0 +1,69 @@ +#!/bin/sh + +# +# Run mosh with a bad server command, check that it reports this usefully. +# + +# shellcheck source=e2e-test-subrs +. "$(dirname "$0")/e2e-test-subrs" +PATH=$PATH:.:$srcdir +# Top-level wrapper. +if [ $# -eq 0 ]; then + e2e-test "$0" baseline client server mosh-args post + exit +fi + +# OK, we have arguments, we're one of the test hooks. +if [ $# -lt 1 ]; then + fail "bad arguments %s\n" "$@" +fi + +# The mosh session test command is never run. +baseline() +{ + printf "@@@ done\n" +} + +# Return inverted exitstatus from mosh-client. +client() +{ + ! "$@" + exit +} + +# Add a do-nothing server command, to disable the standard harness +# checks in e2e-test. +server() +{ + exit 0 +} + +# Make mosh.pl fail with a bad server. +mosh_args() +{ + printf "%s\n" "--server false" +} + +# Check for correct reporting of that failure. +post() +{ + if ! grep 'server command failed with exitstatus' "$(basename "$0").d/baseline.tmux.log"; then + exit 1 + fi +} + +case $1 in + baseline) + baseline;; + client) + shift + client "$@";; + mosh-args) + mosh_args;; + post) + post;; + server) + server;; + *) + fail "unknown test argument %s\n" "$1";; +esac