[pve-devel] [PATCH storage 1/2] don't bail on whitespaces in backing devices

[Wolfgang Bumiller]

On Tue, Apr 30, 2024 at 10:14:13AM +0200, Fiona Ebner wrote:
> Am 30.04.24 um 09:53 schrieb Wolfgang Bumiller:
> > This prevents importing from vmdks with whitespaces in file names.
> > Further, some operations that include file sizes (like listing disks)
> > would potentially fail entirely if a custom disk with a badly name
> > backing device exists in a VM images directory since they don't expect
> > this. Specifically, since we don't necessarily know the actual naming
> > scheme of the current storage in the plain Plugin.pm version, we don't
> > check the full name anyway, so why bother with whitespaces...
> > 
> > See-also: https://forum.proxmox.com/threads/new-import-wizard-available-for-migrating-vmware-esxi-based-virtual-machines.144023/page-16#post-658697
> > Signed-off-by: Wolfgang Bumiller <w.bumiller at proxmox.com>
> > ---
> >  src/PVE/Storage/Plugin.pm | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
> > index 22a9729..683190b 100644
> > --- a/src/PVE/Storage/Plugin.pm
> > +++ b/src/PVE/Storage/Plugin.pm
> > @@ -982,7 +982,7 @@ sub file_size_info {
> >      $used = int($used);
> >      ($format) = ($format =~ /^(\S+)$/) or die "format '$format' includes whitespace\n"; # untaint
> >      if (defined($parent)) {
> > -	($parent) = ($parent =~ /^(\S+)$/) or die "parent '$parent' includes whitespace\n"; # untaint
> > +	($parent) = ($parent =~ /^(\S+)$/); # untaint
> >      }
> >      return wantarray ? ($size, $format, $used, $parent, $st->ctime) : $size;
> >  }
> 
> So the returned $parent will now just be undef if it contains
> whitespaces, even though there is a parent. Can't that cause issues
> further down the line? If it's fine, a comment with the rationale would
> be nice.
> 
> Or should we rather allow whitespaces while matching and return it
> properly? Or are there any issues with proper escaping then?

I was a bit too quick on the send trigger there, but it should be fine
IMO:

- where we do run into this issue, we never use/need/care about the parent
- the parent info of file_size_info is usually discarded, or checked
  against whether the disk is a "base volume" according to the storage's
  idea of how such a volume has to be named (as in, it's created/managed
  by pve)
  or, eg. in Plugin.pm's `list_images` the parent is then checked
  against a more specific regex and if it does not matched it is simply
  discarded as if it was `undef`... (so we already have some logic
  around backing-devices which "discards" unexpected values...)
- technically users could add a disk with a "bad" parent to a storage
  *manually*, but given the list_images mentioned above, I'd argue the
  situation isn't really getting worse, as values that *do* match `\S+`
  don't necessarily match the regexes used *later* on the parent
  *anyway*...

So we could also just untaint with /^(.+)$/, since IMO if we end up with
actual whitespace issues anywhere *else*, then *that* could is the
broken one, not this one...

🤷