diff --git a/bin/plugin/open/scp b/bin/plugin/open/scp index 0d9860d..730b30a 100755 --- a/bin/plugin/open/scp +++ b/bin/plugin/open/scp @@ -103,11 +103,8 @@ if (not $host) { } if (not $ip) { - # use STDERR because stdout is eaten by scp itself - print STDERR "\nscp: Sorry, couldn't resolve the host you specified ('$host'), aborting.\n"; - # note that the calling-side scp will not passthrough this exit code, but most probably "1" instead. - exit(OVH::Bastion::EXIT_HOST_NOT_FOUND); + osh_exit 'ERR_HOST_NOT_FOUND', "Sorry, couldn't resolve the host you specified ('$host'), aborting."; } my $machine = $ip; @@ -138,9 +135,7 @@ $fnret = OVH::Bastion::is_access_granted( details => 1 ); if (not $fnret) { - my $msg = "Sorry, but you don't seem to have access to $machine"; - print STDERR ">>>" . $msg . "\n"; - osh_exit 'ERR_ACCESS_DENIED', $msg; + osh_exit 'ERR_ACCESS_DENIED', "Sorry, but you don't seem to have access to $machine"; } # get the keys we would try @@ -163,9 +158,8 @@ $fnret = OVH::Bastion::is_access_granted( details => 1 ); if (not $fnret) { - my $msg = "Sorry, but even if you have ssh access to $machine, you still need to be granted specifically for scp"; - print STDERR ">>>" . $msg . "\n"; - osh_exit 'ERR_ACCESS_DENIED', $msg; + osh_exit 'ERR_ACCESS_DENIED', + "Sorry, but even if you have ssh access to $machine, you still need to be granted specifically for scp"; } # get the keys we would try too diff --git a/lib/perl/OVH/Bastion/execute.inc b/lib/perl/OVH/Bastion/execute.inc index 7f197df..f240a47 100644 --- a/lib/perl/OVH/Bastion/execute.inc +++ b/lib/perl/OVH/Bastion/execute.inc @@ -4,7 +4,7 @@ package OVH::Bastion; use common::sense; use Config; -use Fcntl 'SEEK_CUR'; +use Fcntl qw{ :DEFAULT :seek }; use IO::Handle; use IO::Select; use IPC::Open3; @@ -43,6 +43,22 @@ sub sysret2human { } } +# utility function to set a filehandle to non-blocking +sub _set_non_blocking { + my $handle = shift; + + my $flags = fcntl($handle, F_GETFL, 0); + if (!$flags) { + return R('ERR_SYSCALL_FAILED', msg => "Couldn't set filehandle to non-blocking (F_GETFL failed)"); + } + + $flags |= O_NONBLOCK; + if (!fcntl($handle, F_SETFL, $flags)) { + return R('ERR_SYSCALL_FAILED', msg => "Couldn't set filehandle to non-blocking (F_SETFL failed)"); + } + return R('OK'); +} + ## no critic(ControlStructures::ProhibitDeepNests) sub execute { my %params = @_; @@ -60,6 +76,7 @@ sub execute { $noisy_stderr = $noisy_stdout = 1 if ($ENV{'PLUGIN_DEBUG'} or $is_binary); my $fnret; + # taint check require Scalar::Util; foreach (@$cmd) { if (Scalar::Util::tainted($_) && /(.+)/) { @@ -69,6 +86,7 @@ sub execute { } } + # if caller want us to use system(), just do it here and call it a day if ($system) { my $child_exit_status = system(@$cmd); $fnret = sysret2human($child_exit_status); @@ -84,6 +102,7 @@ sub execute { ); } + # otherwise, launch the command under open3() my ($child_stdin, $child_stdout, $child_stderr); $child_stderr = gensym; osh_debug("about to run_cmd ['" . join("','", @$cmd) . "']"); @@ -93,12 +112,31 @@ sub execute { chomp $@; return R('ERR_EXEC_FAILED', msg => "Couldn't exec requested command ($@)"); } + + # if some filehandles are already closed, binmode may fail, which is why we use eval{} here + eval { binmode $child_stdin; }; + eval { binmode $child_stdout; }; + eval { binmode $child_stderr; }; + eval { binmode STDIN; }; + eval { binmode STDOUT; }; + eval { binmode STDERR; }; + + # set our child's stdin to non-blocking, so that a syswrite to it is guaranteed to never block, + # otherwise we could get in an deadlock, where our child can't accept more data on its stdin + # before we read pending data from its stdout/stderr, which we will never do because we're busy + # waiting for the syswrite to its stdin to complete. + $fnret = _set_non_blocking($child_stdin); + $fnret or return $fnret; + osh_debug("waiting for child PID $pid to complete..."); + # declare vars we'll use below my %output = (); my $stderr_output; my $stdout_output; my $stdout_buffer; + my $child_stdin_to_write; + my $child_stdin_closing; my $current_fh; my $currently_in_json_block; my %bytesnb; @@ -106,107 +144,183 @@ sub execute { # maximum number of code_info() to call, to avoid flooding the logs my $info_limit = 5; - # always monitor our child stdout and stderr - my $select = IO::Select->new($child_stdout, $child_stderr); - binmode $child_stdin; - binmode $child_stdout; - binmode $child_stderr; + # maximum intermediate buffer size we want in $child_stdin_to_write, before we stop + # reading data from our own STDIN and wait our child to digest the data we send it, + # otherwise if we get a possibly infinite amount of data from our STDIN without ever + # being able to write it to our child, we'll end up in OOM + my $max_stdin_buf_size = 8 * 1024 * 1024; - # if some fd are closed, binmode may fail - eval { binmode STDIN; }; - eval { binmode STDOUT; }; - eval { binmode STDERR; }; - - if ($stdin_str) { - - # we have some stdin data to push, do it now - syswrite $child_stdin, $stdin_str; - close($child_stdin); - } - elsif ($expects_stdin) { - - # ... and also monitor our own stdin only if we expect it (we'll pipe it to our child's stdin) - $select->add(\*STDIN); - } - - # our own version of syswrite to handle auto-retry if interrupted by a signal + # define our own version of syswrite to handle auto-retry if interrupted by a signal, + # return the number of bytes actually written my $syswrite_ensure = sub { - my ($_nbread, $_FH, $_name, $_noisy_ref, $_buffer, $_info_limit) = @_; - return if (!$_nbread || !$_buffer); + my ($_bufsiz, $_FH, $_name, $_noisy_ref, $_buffer) = @_; + return 0 if (!$_bufsiz || !$_buffer); + + my $offset = 0; + my $totalwritten = 0; + while ($offset < $_bufsiz) { + my $written = eval { syswrite $_FH, $_buffer, 65535, $offset; }; + if ($@) { + if ($@ =~ m{on closed filehandle}) { + $child_stdin_to_write = ''; + return $totalwritten; + } + # don't ignore other errors + die($@); + } - my $offset = 0; - while ($offset < $_nbread) { - my $written = syswrite $_FH, $_buffer, 65535, $offset; if (not defined $written) { - # is the fd still open? (maybe we got a SIGPIPE or a SIGHUP) - # don't use tell() here, we use syseek() for unbuffered i/o, - # note that if we're at the position "0", it's still true (see doc). - my $previousError = $!; - if (!sysseek($_FH, 0, SEEK_CUR)) { - info_syslog("execute(): error while syswriting($previousError/$!) on $_name, " - . "the filehandle is closed, will no longer attempt to write to it") - if $$_info_limit-- > 0; - $$_noisy_ref = 0 if $_noisy_ref; + if ($!{'EAGAIN'} && $_name eq 'child_stdin') { + osh_debug("in syswrite_ensure, got EAGAIN on our child stdin, our caller will retry on next loop"); + return $totalwritten; } else { - # oww, abort writing for this cycle. as this might be user-induced, use info instead of warn - info_syslog("execute(): error while syswriting($previousError) on $_name, " . "aborting this cycle") - if $$_info_limit-- > 0; + # is the fd still open? (maybe we got a SIGPIPE or a SIGHUP) + # don't use tell() here, we use syseek() for unbuffered i/o, + # note that if we're at the position "0", it's still true (see doc). + my $previousError = $!; + if (!sysseek($_FH, 0, SEEK_CUR)) { + osh_debug("in syswrite_ensure, sysseek failed"); + info_syslog("execute(): error while syswriting($previousError/$!) on $_name, " + . "the filehandle is closed, will no longer attempt to write to it") + if $info_limit-- > 0; + $$_noisy_ref = 0 if $_noisy_ref; + } + else { + # oww, abort writing for this cycle. as this might be user-induced, use info instead of warn + info_syslog( + "execute(): error while syswriting($previousError) on $_name, " . "aborting this cycle") + if $info_limit-- > 0; + } } last; } - $offset += $written; + $offset += $written; + $totalwritten += $written; } + return $totalwritten; }; - # then, while we still have fh to monitor + # as we'll always monitor our child stdout and stderr, add those to our IO::Select + my $select = IO::Select->new($child_stdout, $child_stderr); + + if (length($stdin_str) > 0) { + # we have some stdin data to push to our child, so preinit our intermediate buffer with that data + $child_stdin_to_write = $stdin_str; + } + elsif ($expects_stdin) { + # monitor our own stdin only if we expect it (we'll pipe it to our child's stdin) + $select->add(\*STDIN); + } + + # then, while we still have at least two filehandles to monitor OR we have only one which is not our own STDIN: while ($select->count() > 1 || ($select->count() == 1 && !$select->exists(\*STDIN))) { - # block only for 50ms, before checking if child is dead - my @ready = $select->can_read(0.05); + # first, if we have data to push to our child's stdin that comes from our own stdin, + # try to do that before checking if we have pending data to read, as to ensure we don't get deadlocked + if (length($child_stdin_to_write) > 0) { + my $written2child = $syswrite_ensure->( + length($child_stdin_to_write), + $child_stdin, 'child_stdin', undef, $child_stdin_to_write + ); + + if ($written2child) { + # remove the data we've written from the intermediate buffer + $child_stdin_to_write = substr($child_stdin_to_write, $written2child); + + if (length($child_stdin_to_write) == 0) { + if (length($stdin_str) > 0) { + # we pushed all the data we wanted to push to our child stdin, we can close it now: + osh_debug("execute: closing child stdin as we wrote everything we wanted to it (stdin_str)"); + close($child_stdin); + } + elsif ($child_stdin_closing) { + # our own STDIN was already closed, and we just finished flushing the data to our child, + # so we can close it as well + osh_debug("execute: closing child stdin as we wrote everything we wanted to it"); + close($child_stdin); + } + } + } + } + + # then, wait until we have something to read. + # block only for 10ms, if there's nothing to read anywhere for this amount of time, + # it'll be when we want to check if our child is dead + my @ready = $select->can_read(0.01); # yep, we have something to read on at least one fh if (@ready) { - # guarantee we're still reading this fh while it has something to say + # guarantee we're still reading this fh while it has something to say, this helps avoiding + # mangling stdout/stderr on the console when noisy_* vars are set $current_fh = $ready[0]; + + # ...unless we have piled up a big buffer from our stdin, and child is still blocking on its own stdin, + # in which case we'll stop reading from our stdin and prioritize other filehandles + # in an attempt to unblock our child + if ($current_fh->fileno == STDIN->fileno && length($child_stdin_to_write) > $max_stdin_buf_size) { + osh_debug("main loop, changing current fh to avoid deadlocking"); + if (@ready > 1) { + $current_fh = $ready[1]; + } + else { + warn_syslog("possible deadlock while running ['" . join("','", @$cmd) . "']") if $info_limit-- > 0; + osh_debug("main loop, can't change current fh to avoid deadlock, refusing to read from it"); + # we have nothing to read/write from/to, except from our own STDIN but our buffer is already full, + # so let's sleep for a tiny bit to help ensuring our child is not CPU-starved which could make + # it incapable of accepting new data to its STDIN, hence deadlocking us in return + require Time::HiRes; + Time::HiRes::usleep(1000 * 15); + next; + } + } + my $sub_select = IO::Select->new($current_fh); - # can_read(0) because we don't need a timeout: we KNOW there's something to read on this fh + # can_read(0) because we don't need a timeout: we KNOW there's something to read on this fh, + # otherwise we wouldn't be here while ($sub_select->can_read(0)) { my $buffer; my $nbread = sysread $current_fh, $buffer, 65535; - # undef mears error, we'll log to syslog and close. as this might be user-induced, use info instead of warn + # undef==error, we log to syslog and close. as this might be user-induced, use info instead of warn if (not defined $nbread) { - # awwww, not cool at all info_syslog("execute(): error while sysreading($!), closing fh!"); } # if size 0, it means it's an EOF elsif ($nbread == 0) { # we got an EOF on this fh, remove it from the monitor list + osh_debug("main loop: removing current fh from select list"); $select->remove($current_fh); - # if this is an EOF on our own STDIN, we need to close our child's STDIN + # if this is an EOF on our own STDIN, we need to close our child's STDIN, + # but do this only if our intermediate buffer is empty, otherwise just mark it + # for close, we'll do it as soon as we empty the buffer if ($current_fh->fileno == STDIN->fileno) { - close(STDIN); # we got eof on it, so close it - close($child_stdin); # and close our child stdin + close(STDIN); # we got EOF on it, so close it + if (length($child_stdin_to_write) == 0) { + close($child_stdin); + } + else { + $child_stdin_closing = 1; # defer close to when our intermediate buffer is empty + } } else { - ; # eof on our child's stdout or stderr, nothing to do + ; # EOF on our child's stdout or stderr, nothing to do } last; } - # we got data, is this our child's stderr ? + # we got data, is this our child's stderr? elsif ($current_fh->fileno == $child_stderr->fileno) { $bytesnb{'stderr'} += $nbread; $stderr_output .= $buffer if !$is_binary; # syswrite on our own STDERR what we received if ($noisy_stderr) { - $syswrite_ensure->($nbread, *STDERR, 'stderr', \$noisy_stderr, $buffer, \$info_limit); + $syswrite_ensure->($nbread, *STDERR, 'stderr', \$noisy_stderr, $buffer); } } @@ -220,7 +334,7 @@ sub execute { # so handle that further below if ($noisy_stdout) { if (!$is_helper) { - $syswrite_ensure->($nbread, *STDOUT, 'stdout', \$noisy_stdout, $buffer, \$info_limit); + $syswrite_ensure->($nbread, *STDOUT, 'stdout', \$noisy_stdout, $buffer); } else { # if this is a helper, hide the HELPER_RESULT from noisy_stdout @@ -234,8 +348,7 @@ sub execute { if (not $currently_in_json_block) { $stdout_buffer .= $/; $syswrite_ensure->( - length($stdout_buffer), *STDOUT, 'stdout', \$noisy_stdout, $stdout_buffer, - \$info_limit + length($stdout_buffer), *STDOUT, 'stdout', \$noisy_stdout, $stdout_buffer ); } if ($currently_in_json_block and $stdout_buffer eq 'JSON_END') { @@ -249,7 +362,7 @@ sub execute { } # if we still have data in our local buffer, flush it $syswrite_ensure->( - length($stdout_buffer), *STDOUT, 'stdout', \$noisy_stdout, $stdout_buffer, \$info_limit + length($stdout_buffer), *STDOUT, 'stdout', \$noisy_stdout, $stdout_buffer ) if $stdout_buffer; } } @@ -270,15 +383,14 @@ sub execute { } } - # we got data, is this our stdin ? + # we got data, is this our stdin? elsif ($current_fh->fileno == STDIN->fileno) { $bytesnb{'stdin'} += $nbread; - - # we just write the data to our child's own stdin - $syswrite_ensure->($nbread, $child_stdin, 'child_stdin', undef, $buffer, \$info_limit); + # save this data for the next loop + $child_stdin_to_write .= $buffer; } - # wow, we got data from an unknown fh ... it's not possible + # wow, we got data from an unknown fh ... it's not possible ... theoretically else { warn_syslog("Got data from an unknown fh ($current_fh) with $nbread bytes of data"); last; diff --git a/tests/functional/tests.d/330-selfkeys.sh b/tests/functional/tests.d/330-selfkeys.sh index de9103b..28abb35 100644 --- a/tests/functional/tests.d/330-selfkeys.sh +++ b/tests/functional/tests.d/330-selfkeys.sh @@ -174,6 +174,7 @@ EOS ) account1key1fp=$(get_json | $jq '.value.keys[0].fingerprint') + ignorecodewarn "possible deadlock" script flood $a1 -osh selfAddIngressKey '<' /dev/urandom retvalshouldbe 0