[pve-devel] [PATCH installer v2 6/6] fix #4872: run env: use run_command() for country detection

[Christoph Heiss]

This fixes a rather longstanding issue [0][1] with the country
detection, in that it might get completely stuck and thus hangs the
installation.

This is due how Perl, signals and line reading interacts.

A minimal reproducer, how the installer currently works, looks like
this:
```
    #!/usr/bin/env perl

    use strict;
    use warnings;

    open (my $fh, '-|', 'sleep', '1000') or die;

    my $prev = alarm(2);
    eval {
	local $SIG{ALRM} = sub { die "timed out!\n" };

	my $line;
	while (defined ($line = <$fh>)) {
	    print "line: $line";
	}
    };

    alarm($prev);
    close($fh);
```

One might expect that this times out after 2 seconds, as specified in
`alarm(2)`. The thruth is that `$line = <$fh>` apparently prevents the
signal to go through. This then causes the installer to hang there
indefinitely, if `traceroute` never progresses - which seems to happen
on lots of (weird) networks, as evidently can be seen in the forum [1].

Proxmox::Sys::Command::run_command() handles of these weird cases, takes
care of the nitty-gritty details and - most importantly - interacts
properly with SIGALRM, so just use that instead.

This _should_ really fix that issue, but reproducing it 1:1 as part of
the installation process is _very_ hard, basically pure luck. But
rewriting the reproducer using run_command (in the exact same way that
this patch rewrites detect_country_tracing_to()) fixes the issue there,
so it's the best we can probably do.

NB: This causes that the traceroute command is now printed to the log
(as run_command() logs that by default), which we could also hide e.g.
through another parameter if wanted.

[0] https://bugzilla.proxmox.com/show_bug.cgi?id=4872
[1] https://forum.proxmox.com/threads/proxmox-installation-trying-to-detect-country.134301/

Signed-off-by: Christoph Heiss <c.heiss at proxmox.com>
---
Changes since v1:
  * use new CMD_FINISHED constant

 Proxmox/Install/RunEnv.pm | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/Proxmox/Install/RunEnv.pm b/Proxmox/Install/RunEnv.pm
index c393f67..d7ccea6 100644
--- a/Proxmox/Install/RunEnv.pm
+++ b/Proxmox/Install/RunEnv.pm
@@ -7,6 +7,7 @@ use Carp;
 use JSON qw(from_json to_json);

 use Proxmox::Log;
+use Proxmox::Sys::Command qw(run_command CMD_FINISHED);
 use Proxmox::Sys::File qw(file_read_firstline);
 use Proxmox::Sys::Block;
 use Proxmox::Sys::Net;
@@ -188,35 +189,36 @@ my sub detect_country_tracing_to : prototype($$) {
     my ($ipver, $destination) = @_;

     print STDERR "trying to detect country...\n";
-    open(my $TRACEROUTE_FH, '-|', 'traceroute', "-$ipver", '-N', '1', '-q', '1', '-n', $destination)
-	or return undef;

+    my $traceroute_cmd = ['traceroute', "-$ipver", '-N', '1', '-q', '1', '-n', $destination];
     my $geoip_bin = ($ipver == 6) ? 'geoiplookup6' : 'geoiplookup';

     my $country;
-
-    my $previous_alarm = alarm (10);
+    my $previous_alarm;
     eval {
 	local $SIG{ALRM} = sub { die "timed out!\n" };
-	my $line;
-	while (defined ($line = <$TRACEROUTE_FH>)) {
+	$previous_alarm = alarm (10);
+
+	run_command($traceroute_cmd, sub {
+	    my $line = shift;
+
 	    log_debug("DC TRACEROUTE: $line");
 	    if ($line =~ m/^\s*\d+\s+(\S+)\s/) {
 		my $geoip = qx/$geoip_bin $1/;
 		log_debug("DC GEOIP: $geoip");
+
 		if ($geoip =~ m/GeoIP Country Edition:\s*([A-Z]+),/) {
 		    $country = lc ($1);
 		    log_info("DC FOUND: $country\n");
-		    last;
+		    return CMD_FINISHED;
 		}
 	    }
-	}
+	}, undef, undef, 1);
+
     };
     my $err = $@;
     alarm ($previous_alarm);

-    close($TRACEROUTE_FH);
-
     if ($err) {
 	die "unable to detect country - $err\n";
     } elsif ($country) {
--
2.43.0