[pve-devel] [PATCH pve-manager 1/1] Fix #2634: if hook-script without permission, prints message, that the script not executable.

[Thomas Lamprecht]

On 3/12/20 9:05 AM, Moayad Almalat wrote:
> Signed-off-by: Moayad Almalat <m.almalat at proxmox.com>

goes in the right direction, some style nits and comments below.

> ---
>  PVE/VZDump.pm | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
> index ada3681e..df6f8df9 100644
> --- a/PVE/VZDump.pm
> +++ b/PVE/VZDump.pm
> @@ -569,6 +569,10 @@ sub run_hook_script {
>  
>      my $script = $opts->{script};
>  
> +    unless (-X $script) {

1. We do not use "unless" in our code base, while it's valid perl we actively
   choose against that, see our perl style guide:
   https://pve.proxmox.com/wiki/Perl_Style_Guide#Perl_syntax_choices

2. Also, check out the difference between -X (upper case) and -x (lower case):
   -X  File is executable by real uid/gid.
   -x  File is executable by effective uid/gid.

   The real user id is who you really are (the one who owns the process), and the
   effective user id is what the operating system looks at to make a decision
   whether or not you are allowed to do something (most of the time, there are some
   exceptions).

   Please use lower case "-x".

3. This needs to be moved below the "return if !$script;" below, as else you test
   even if no script is set, which can give you "use of uninitialized $variable"
   messages, and is not good practice.

> +	    debugmsg('err', "$phase: The hook-script '$script' is not executable.");

There's no point in continuing as the run_command would die anyway - but without
clear error. So additionally please add a explicit die too


die "The hook-script '$script' is not executable.\n"

($phase is not relevant info here - it'd highly probably die on every phase)

> +    }
> +
>      return if !$script;


>  
>      my $cmd = "$script $phase";
>