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

[Thomas Lamprecht]

On 11/11/2017 07:42 AM, Dietmar Maurer wrote:
>> 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?
> 

Hash keys are never tainted.
-- https://perldoc.perl.org/perlsec.html#Taint-mode

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

I was wrong here, this does not per se causes the tainting error,
I saw to late that perl complained that $ENV{'PATH'} was tainted,
not $h. Which makes sense as I do a system (calling external stuff
where PATH matters a lot). Adding:

delete $ENV{'PATH'};
or 
$ENV{'PATH'} = '/bin:/usr/bin';

on top fixes this.

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

True that.

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

While 2) is not completely true (Hash keys are never tainted) I have no
objection to reverting it back.