From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <w.bumiller@proxmox.com>
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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; Wed, 10 Apr 2024 13:37:47 +0200 (CEST)
Date: Wed, 10 Apr 2024 13:37:46 +0200
From: Wolfgang Bumiller <w.bumiller@proxmox.com>
To: Fiona Ebner <f.ebner@proxmox.com>
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>
<q5t7ju4byueriaicrfqpusazj5ehhugunxod4ihmi2fpakw5ns@mhc3ougfz2bn>
<aa295e42-2286-4e57-9523-cace64ef7aa6@proxmox.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <aa295e42-2286-4e57-9523-cace64ef7aa6@proxmox.com>
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 <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=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.