From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id B1865942EF for ; Wed, 10 Apr 2024 13:38:18 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 8A677E5AE for ; Wed, 10 Apr 2024 13:37:48 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Wed, 10 Apr 2024 13:37:47 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 8EBDA43AB8 for ; Wed, 10 Apr 2024 13:37:47 +0200 (CEST) Date: Wed, 10 Apr 2024 13:37:46 +0200 From: Wolfgang Bumiller To: Fiona Ebner Cc: pve-devel@lists.proxmox.com Message-ID: <2pg74avsvdfnyruwd5hnwscxeivujilsf66uir4epekrthsbqw@bvc4wi5noppt> References: <20240315102502.84163-1-f.ebner@proxmox.com> <20240315102502.84163-14-f.ebner@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-SPAM-LEVEL: Spam detection results: 0 AWL 0.087 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH manager v2 13/21] api: backup/vzdump: add permission check for fleecing storage X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 10 Apr 2024 11:38:18 -0000 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.