Re: [pve-devel] [PATCH] Check only first part of script parameter for executability

["Phillipp Röll" <pr@lima-city.de>]

Hi Thomas,
I'll skip the legal stuff for now, until we figure out if the patch has 
a chance
to be merged, ok?

That would break existing configuration that use a script with 
whitespace in its
path name.

There could be two checks, check if either the full script name is 
executable OR
just the first part (split by whitespaces).

Can you please provide your usecase where the arguments Proxmox VE sets 
for the
script aren't enough? So that we've a better picture about possible 
options.

We have a backup & restore management system, where the state, retention 
period etc.
of a backup is stored. All proxmox backup states are updated by a hook 
script calling
a REST API. We need to pass the ID of the backup from the management 
system to the
hook script, which is no longer possible since the check was introduced.

We comment/patch these checks out automatically on all systems, but 
that's a bit
annoying.

So actually the (much) earlier introduction of the check already broke 
our use case,
but before today the pain to report it/change something simply wasn't 
great enough.

---
Mit freundlichen Grüßen
Phillipp Röll

Am 2021-10-21 11:05, schrieb Thomas Lamprecht:
> Hi,
> 
> thanks for your effort in contributing to Proxmox VE!
> 
> A few things inline
> 
> On 21.10.21 10:46, Phillipp Röll wrote:
>> When giving a script parameter including arguments to the dump 
>> process,
>> the full script parameter including the arguments is checked for
>> executablility. The following will always abort with a "not 
>> executable"
>> error:
>> 
>> vzdump 100 --script "/usr/local/bin/myscript.pl myarg"
>> 
>> If we split the script argument and check only the first part, the
>> check works as expected.
> 
> your sign-off is missing here and it doesn't seems like we have a 
> signed
> CLA from you or your company here, and to be able to take in a patch 
> you'd
> need to send one to <office@proxmox.com>. For details please check:
> https://pve.proxmox.com/wiki/Developer_Documentation#Software_License_and_Copyright
> 
>> ---
>>  PVE/VZDump.pm | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>> 
>> diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
>> index 88ac11b3..fe1a7f23 100644
>> --- a/PVE/VZDump.pm
>> +++ b/PVE/VZDump.pm
>> @@ -630,7 +630,9 @@ sub run_hook_script {
>>      my $script = $opts->{script};
>>      return if !$script;
>> 
>> -    if (!-x $script) {
>> +    my @script_args = split /\s+/, $script;
> 
> That would break existing configuration that use a script with 
> whitespace in its
> path name.
> 
> Can you please provide your usecase where the arguments Proxmox VE sets 
> for the
> script aren't enough? So that we've a better picture about possible 
> options.
> 
> cheers,
> Thomas