Re: [pve-devel] [PATCH manager v2 13/21] api: backup/vzdump: add permission check for fleecing storage

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

On Wed, Apr 10, 2024 at 11:57:37AM +0200, Fiona Ebner wrote:
> Am 08.04.24 um 10:47 schrieb Wolfgang Bumiller:
> > On Fri, Mar 15, 2024 at 11:24:54AM +0100, Fiona Ebner wrote:
> >> @@ -52,6 +52,12 @@ sub assert_param_permission_common {
> >>      if (grep { defined($param->{$_}) } qw(bwlimit ionice performance)) {
> >>  	$rpcenv->check($user, "/", [ 'Sys.Modify' ]);
> >>      }
> >> +
> >> +    if ($param->{fleecing} && !$is_delete) {
> >> +	my $fleecing = PVE::VZDump::parse_fleecing($param);
> > 
> > ^ The parse_fleecing sub does not actually return the hash, at least not
> > explicitly, and when it is not set it returns undef, so the `if` guard
> > in the statement below tries to access `undef->{storage}`.
> > 
> 
> It can't be unset, because $param->{fleecing} is checked before entering
> the if branch here.
> 
> > If the parameter does exist then the first run through the function
> > which performs the actual string->hash conversion will *accidentally*
> > also return the hash implicitly, because there's no explicit return
> > statement for it.
> > Subsequent calls on the other hand will run into the
> >     return if ref($fleecing) eq 'HASH';
> > and thus return an empty list making `$fleecing` undef again.
> > 
> 
> Oh, good catch! It did work by chance in my testing, because of what you
> describe, the implicit return and because nobody else called
> parse_fleecing() before here. Will fix in v3!
> 
> >> +	$rpcenv->check($user, "/storage/$fleecing->{storage}", [ 'Datastore.AllocateSpace' ])
> >> +	    if $fleecing->{storage};
> >> +    }
> >>  }
> >>  
> >>  my sub assert_param_permission_create {
> 
> ---snip---
> 
> >> diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
> >> index 74eb0c83..88149d68 100644
> >> --- a/PVE/VZDump.pm
> >> +++ b/PVE/VZDump.pm
> >> @@ -130,7 +130,7 @@ my $generate_notes = sub {
> >>      return $notes_template;
> >>  };
> >>  
> >> -my sub parse_fleecing {
> >> +sub parse_fleecing {
> >>      my ($param) = @_;
> >>  
> >>      if (defined(my $fleecing = $param->{fleecing})) {
> > 
> > ^ So this should be updated to actually return the hash.
> 
> We also have parse_performance() and parse_prune_backups_maxfiles() with
> similar semantics. Their callers don't actually need any return value.
> If we change parse_fleecing() to return the result, we should change the
> others as well for consistency. Alternatively, I can fix the wrong
> caller of parse_fleecing() above and maybe add an explicit "return
> undef" to these parse_* functions to avoid something like this slipping
> through in the future. Which option do you prefer?

Having them all return the hash shouldn't hurt and makes sense to me.
Even if the others are "private" (`my sub`). A patch just dropping the
`my` usually does not include enough context lines in patch mails to
easily see that they don't return anything...

Given that we kind of need to always call them before using the hashes
anyway, always returning the hash makes sense anyway.