Re: [pve-devel] applied: [PATCH storage] plugins: untaint volume_size_info retuns

[Stoiko Ivanov <s.ivanov@proxmox.com>]

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)