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 25FEC93EFE for ; Wed, 10 Apr 2024 11:57:39 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id EE5C9B2E9 for ; Wed, 10 Apr 2024 11:57:38 +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) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Wed, 10 Apr 2024 11:57:38 +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 129A843BE9 for ; Wed, 10 Apr 2024 11:57:38 +0200 (CEST) Message-ID: Date: Wed, 10 Apr 2024 11:57:37 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Wolfgang Bumiller Cc: pve-devel@lists.proxmox.com References: <20240315102502.84163-1-f.ebner@proxmox.com> <20240315102502.84163-14-f.ebner@proxmox.com> Content-Language: en-US From: Fiona Ebner In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.073 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 09:57:39 -0000 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?