[pve-devel] [PATCH common v2 2/3] daemon: refactor and cleanup

[Dietmar Maurer]

> As I deemed it unnecessary, we strongly control this environment
> variable passed on re-exec and the usage of the old_workers keys
> raise no perl tainting check, e.g., the following example:
> 
> # cat ./check-env-to-kill-taint.pl
> #!/usr/bin/perl -T
> 
> use strict;
> use warnings;
> 
> my $foo = $ENV{'FOO'};
> 
> my $h = {};
> $h->{$_} = 1 foreach (split(':', $foo));
> 
> 
> print "sending USR2 to " . join(' ', keys %$h) ."\n";
> kill 12, keys %$h;

I think this 'kill' should trigger the taint warning. If not, please can you
explain why not?

> can be run without problems:
> # FOO=1:2 ./check-env-to-kill-taint.pl
> 
> only when adding a line like:
> system "echo " . join(' ', keys %$h);

And that is why it is always a good idea to untaint values, even if
it just makes debugging easier.

Note: you do not know what we will do in future ...

> 
> I run into a tainting error.
> So as the regex provided no additional value it's safe to remove
> here as it provides no protection (kill refuses to do anything on
> non-integers). But yeah this was really a bit overhasty for a cleanup.
> 
> >> @@ -289,11 +285,7 @@ sub setup {
> >>  
> >>      if ($restart && $self->{max_workers}) {
> >>  	if (my $wpids = $ENV{PVE_DAEMON_WORKER_PIDS}) {
> >> -	    foreach my $pid (split(':', $wpids)) {
> >> -		if ($pid =~ m/^(\d+)$/) {
> >> -		    $self->{old_workers}->{$1} = 1;
> >> -		}
> >> -	    }
> >> +	    $self->{old_workers}->{$_} = 1 foreach (split(':', $wpids));

I still think the old code is better, because it 

 1.) checks the format. 
 2.) untaint the value
 3.) uses a more common loop construct

So please can we revert that change?