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

[Thomas Lamprecht]

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 at 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.