ovh/the-bastion
1
1
Fork 0
mirror of https://github.com/ovh/the-bastion.git synced 2025-09-08 05:54:12 +08:00
Code Issues Projects Releases Packages Wiki Activity

fix: rare race condition introduced by b7f4909

Browse source 
Under some specific conditions, the execute() call could get deadlocked with the program it started,
both waiting for each other to read or write data. This is easier to reproduce with the `scp` plugin,
where the transfer would just stall. Introduce an additional intermediate buffer to avoid this race condition.
This commit is contained in:
Stéphane Lesimple 2022-10-24 15:02:00 +00:00 • committed by Stéphane Lesimple
parent 21f29680b6
commit 521836b17b
3 changed files with 181 additions and 74 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

14
bin/plugin/open/scp
View file

@ -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

240
lib/perl/OVH/Bastion/execute.inc
View file

@ -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;

1
tests/functional/tests.d/330-selfkeys.sh
View file

@ -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