From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <s.ivanov@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 93640751BA
 for <pve-devel@lists.proxmox.com>; Wed, 23 Jun 2021 15:18:27 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
 by firstgate.proxmox.com (Proxmox) with ESMTP id 88D2EB38A
 for <pve-devel@lists.proxmox.com>; Wed, 23 Jun 2021 15:18:27 +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 id E5F9CB378
 for <pve-devel@lists.proxmox.com>; Wed, 23 Jun 2021 15:18:23 +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 AB689457EA
 for <pve-devel@lists.proxmox.com>; Wed, 23 Jun 2021 15:18:23 +0200 (CEST)
Date: Wed, 23 Jun 2021 15:18:21 +0200
From: Stoiko Ivanov <s.ivanov@proxmox.com>
To: Thomas Lamprecht <t.lamprecht@proxmox.com>
Cc: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Message-ID: <20210623151821.1cbabf05@rosa.proxmox.com>
In-Reply-To: <49e4b2cc-5c15-b343-e374-e7d78f6c69ac@proxmox.com>
References: <20210622163954.5580-1-s.ivanov@proxmox.com>
 <49e4b2cc-5c15-b343-e374-e7d78f6c69ac@proxmox.com>
X-Mailer: Claws Mail 3.17.8 (GTK+ 2.24.33; x86_64-pc-linux-gnu)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
X-SPAM-LEVEL: Spam detection results:  0
 AWL 0.641 Adjusted score from AWL reputation of From: address
 BAYES_00                 -1.9 Bayes spam probability is 0 to 1%
 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] applied: [PATCH storage] plugins: untaint
 volume_size_info retuns
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, 23 Jun 2021 13:18:27 -0000

On Wed, 23 Jun 2021 09:13:40 +0200
Thomas Lamprecht <t.lamprecht@proxmox.com> wrote:

> On 22.06.21 18:39, Stoiko Ivanov wrote:
> > the size returned by volume_size_info is used for creating the new
> > destination image in PVE::QemuServer::clone_disk (and probably
> > elsewhere). In certain cases the return values are tainted - they are
> > obtained by a run_command call and depending on the format and length
> > of the parsed output can still have their tainted attribute.
> > 
> > One example of a tainted return has been reported in our
> > community-forum:
> > https://forum.proxmox.com/threads/cannot-clone-vm-or-move-disk-with-more-than-13-snapshots.89628/
> > 
> > A qcow2 image with 13 snapshots generates a output > 4k in length from
> > `qemu-img info --output=json`, which in turn causes the output to be
> > considered tainted.
> > 
> > This patch untaints the returns where applicable. The other
> > storage-plugins are not affected:
> > * LVMPlugin returns a single number and a newline (thus gets untainted
> >   by run_command)
> > * RBDPlugin untaints the complete json before decoding
> > * ZFSPoolplugin and ISCSIDirectPlugin explicitly untaint their
> >   returns.
> > 
> > Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> > ---
> > 
> > Note:
> > Not really a v2, since it's a different patch, but addresses the same issue
> > as in https://lists.proxmox.com/pipermail/pve-devel/2021-June/048910.html  
> 
> Aren't the version rather tied to the issue they try to solve, as implementations
> and approaches can always significantly change? So I'd be OK with this being
> marked as v2, but no hard feelings.
> 
> > 
> >  PVE/Storage/PBSPlugin.pm | 4 +++-
> >  PVE/Storage/Plugin.pm    | 6 ++++++
> >  2 files changed, 9 insertions(+), 1 deletion(-)
> > 
> >  
> 
> applied to master and a newly branched stable-6, thanks!
> 
> I made two followups:
> 
> 1) - fix nit that we have a space in comments after the #
>    - actually die with error message if untaint fails
> 
> 2) return early if decode JSON fails, compensates issues where the qemu-img
>    command somehow fails (e.g., file was removed since the stat check) and
>    the semantic change from 1)
> 
> Please check if this still looks alright to you.

Thanks for the improvements - Look fine to me! (and survived a few quick
tests of disk-moving)