[pve-devel] [PATCH storage 2/4] Recursive search for iso and vztmpl

Dominik Csapak d.csapak at proxmox.com
Wed Apr 8 16:10:56 CEST 2020


comments inline

On 4/2/20 1:34 PM, Dominic Jäger wrote:
> Each content type has a predefined location in a storage where they can be
> found. iso and vztmpl files can now be found and and also uploaded in
> subdirectories of those locations, too.
> 
> Add a parameter "recursive" (default off) to the API at
> storage/{storage}/content, to perform this search.
> 
> Signed-off-by: Dominic Jäger <d.jaeger at proxmox.com>
> ---
> Changes since RFC:
> * Separate tests for existing and new stuff into 2 patches
> * Die when .. appears in a path
> * Make feature opt-in (relevant for symlinks)
> 
>   PVE/API2/Storage/Content.pm |  9 +++-
>   PVE/API2/Storage/Status.pm  | 12 +++---
>   PVE/Storage.pm              |  4 +-
>   PVE/Storage/Plugin.pm       | 85 +++++++++++++++++++++++++++++++++----
>   test/run_plugin_tests.pl    | 57 ++++++++++++++++++++++++-
>   5 files changed, 149 insertions(+), 18 deletions(-)
> 
> diff --git a/PVE/API2/Storage/Content.pm b/PVE/API2/Storage/Content.pm
> index 63fa4fc..cd302e9 100644
> --- a/PVE/API2/Storage/Content.pm
> +++ b/PVE/API2/Storage/Content.pm
> @@ -44,6 +44,12 @@ __PACKAGE__->register_method ({
>   		optional => 1,
>   		completion => \&PVE::Cluster::complete_vmid,
>   	    }),
> +	    recursive => {
> +	        description => "Search recursively.",
> +	        type => 'boolean',
> +	        optional => 1,
> +	        default => 0,
> +	    },
>   	},
>       },
>       returns => {
> @@ -102,7 +108,8 @@ __PACKAGE__->register_method ({
>   
>   	my $cfg = PVE::Storage::config();
>   
> -	my $vollist = PVE::Storage::volume_list($cfg, $storeid, $param->{vmid}, $param->{content});
> +	my $vollist = PVE::Storage::volume_list($cfg, $storeid, $param->{vmid},
> +	    $param->{content}, {recursive => $param->{recursive}});
>   
>   	my $res = [];
>   	foreach my $item (@$vollist) {
> diff --git a/PVE/API2/Storage/Status.pm b/PVE/API2/Storage/Status.pm
> index 14f5930..b8caf9c 100644
> --- a/PVE/API2/Storage/Status.pm
> +++ b/PVE/API2/Storage/Status.pm
> @@ -403,25 +403,25 @@ __PACKAGE__->register_method ({
>   	my $filename = $param->{filename};
>   
>   	chomp $filename;
> -	$filename =~ s/^.*[\/\\]//;
> -	$filename =~ s/[^-a-zA-Z0-9_.]/_/g;
> +	# Prevent entering other locations without permission
> +	die "Filename must not contain two dots '..'" if $filename =~ m/\.\./;

i'd argue that this check is wrong...
i get what you intend but what about files named like:

my.very.cool...iso.iso

would also be filtered out but is a very valid filename

'..' is only forbidden at the beginning, the end or between two '/'

so e.g.

m!(?:^\.\./|/\.\./|/\.\.$)!

(well, this is certainly not a readable regex^^; also i am
certainly missing a case, i think...)

> +	$filename =~ s/^[\/|\\]*//; # files are uploaded relative to storage path

are '|' intentionally removed here? in [] the characters are or'd anyway

> +	$filename =~ s/[^-a-zA-Z0-9_.\/\\]/_/g;
>   
>   	my $path;
> -
>   	if ($content eq 'iso') {
> -	    if ($filename !~ m![^/]+$PVE::Storage::iso_extension_re$!) {
> +	    if ($filename !~ m!$PVE::Storage::iso_extension_re$!) {
>   		raise_param_exc({ filename => "missing '.iso' or '.img' extension" });
>   	    }
>   	    $path = PVE::Storage::get_iso_dir($cfg, $param->{storage});
>   	} elsif ($content eq 'vztmpl') {
> -	    if ($filename !~ m![^/]+\.tar\.[gx]z$!) {
> +	    if ($filename !~ m!\.tar\.[gx]z$!) {
>   		raise_param_exc({ filename => "missing '.tar.gz' or '.tar.xz' extension" });
>   	    }
>   	    $path = PVE::Storage::get_vztmpl_dir($cfg, $param->{storage});
>   	} else {
>   	    raise_param_exc({ content => "upload content type '$content' not allowed" });
>   	}
> -
>   	die "storage '$param->{storage}' does not support '$content' content\n"
>   	    if !$scfg->{content}->{$content};
>   
> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
> index 60b8310..9ec40a5 100755
> --- a/PVE/Storage.pm
> +++ b/PVE/Storage.pm
> @@ -866,7 +866,7 @@ sub template_list {
>   }
>   
>   sub volume_list {
> -    my ($cfg, $storeid, $vmid, $content) = @_;
> +    my ($cfg, $storeid, $vmid, $content, $param) = @_;
>   
>       my @ctypes = qw(rootdir images vztmpl iso backup snippets);
>   
> @@ -880,7 +880,7 @@ sub volume_list {
>   
>       activate_storage($cfg, $storeid);
>   
> -    my $res = $plugin->list_volumes($storeid, $scfg, $vmid, $cts);
> +    my $res = $plugin->list_volumes($storeid, $scfg, $vmid, $cts, $param);
>   
>       @$res = sort {lc($a->{volid}) cmp lc ($b->{volid}) } @$res;
>   
> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
> index 8c0dae1..e81c89f 100644
> --- a/PVE/Storage/Plugin.pm
> +++ b/PVE/Storage/Plugin.pm
> @@ -415,6 +415,9 @@ sub parse_name_dir {
>       die "unable to parse volume filename '$name'\n";
>   }
>   
> +# for iso and vztmpl returns
> +#   [0] format
> +#   [1] notdir (=basename+suffix; no directories)

nit: again the 'notdir' here...

>   sub parse_volname {
>       my ($class, $volname) = @_;
>   
> @@ -428,9 +431,11 @@ sub parse_volname {
>   	my ($vmid, $name) = ($1, $2);
>   	my (undef, $format, $isBase) = parse_name_dir($name);
>   	return ('images', $name, $vmid, undef, undef, $isBase, $format);
> -    } elsif ($volname =~ m!^iso/([^/]+$PVE::Storage::iso_extension_re)$!) {
> +    } elsif ($volname =~ m!^iso/(?:.+/)*([^/]+$PVE::Storage::iso_extension_re)$!) {

nit:
(?:.+/)* could also be (?:.+/)?
since this will not repeat anyway,
since the .+ is greedy and will include '/',
the ? makes it clearer that this is just optional

> +	die "volname must not contain two dots '..'" if $volname =~ m!\.\.!;

see above

>   	return ('iso', $1);
> -    } elsif ($volname =~ m!^vztmpl/([^/]+\.tar\.[gx]z)$!) {
> +    } elsif ($volname =~ m!^vztmpl/(?:.+/)*([^/]+\.tar\.[gx]z)$!) {

same note about */?

> +	die "volname must not contain two dots '..'" if $volname =~ m!\.\.!;

see above

>   	return ('vztmpl', $1);
>       } elsif ($volname =~ m!^rootdir/(\d+)$!) {
>   	return ('rootdir', $1, $1);
> @@ -485,7 +490,14 @@ sub filesystem_path {
>   
>       $dir .= "/$vmid" if $vtype eq 'images';
>   
> -    my $path = "$dir/$name";
> +    my $path;
> +    if ($vtype eq 'iso' || $vtype eq 'vztmpl') {
> +	# remove vtype => file name relative to storage path
> +	$volname =~ m!^$vtype/(.+)!;
> +	$path = "$dir/$1";
> +    } else {
> +	$path = "$dir/$name";
> +    }
>   
>       return wantarray ? ($path, $vmid, $vtype) : $path;
>   }
> @@ -910,6 +922,61 @@ sub list_images {
>       return $res;
>   }
>   
> +
> +# returns if file name fits $type
> +#   [0] volname
> +#   [1] format
> +# undef otherwise
> +sub get_file_properties {
> +    my ($fn, $storage_path, $type) = @_;
> +    die "fn must not contain two dots '..'" if $fn =~ m/\.\./;

see above

> +    if ($type eq 'iso') {
> +	if ($fn =~ m!$storage_path/(.+$PVE::Storage::iso_extension_re)$!i) {
> +	    return ["$type/$1", $type];
> +	} else {
> +	    return undef;
> +	}
> +    } elsif ($type eq 'vztmpl') {
> +	if ($fn =~ m!$storage_path/(.+\.tar\.([gx]z))$!) {
> +	    return ["$type/$1", "t$2"];
> +	} else {
> +	    return undef;
> +	}
> +    } else {
> +	die "Invalid parameter type=$type";
> +    }
> +}
> +
> +# $type = <iso|vztmpl>
> +sub get_subdir_files_rec {

any special reason to make this a full-fledged sub
instead of a private my $foo = sub {}; ?
if it is about recursive access just do:

my $foo;

$foo = sub {
  $foo->();
};

> +    my ($sid, $path, $type, $storage_path) = @_;
> +    die "Invalid parameter" if not ($type eq 'iso' || $type eq 'vztmpl');

any reason for the 'not' vs '!' ? (not that it's wrong, just not our style)

> +    die "Path must not contain ../" if $path =~ m!.*\.\./.*!;

ha, its better here, but the .* front and back are useless and its also 
wrong: /myweirddirectory../

> +
> +    # Each call needs to know the original storage path
> +    if (!defined $storage_path) {

nit: please use parenthesis for defined

> +	$storage_path = $path;
> +    }
> +
> +    my @result = ();
> +
> +    # Symlinks have to be set up via shell. At least as long recursive scanning
> +    # is opt-in handling loops is out of scope. Instead, just follow the links.
> +    if (-d $path) {
> +	foreach my $fn (<$path/*>) {
> +	    push @result, get_subdir_files_rec($sid, $fn, $type, $storage_path);
> +	}
> +    } else {
> +	my $properties = get_file_properties($path, $storage_path, $type);
> +	if ($properties) {
> +	    my $file = { volid => "$sid:$$properties[0]", format => $$properties[1] };

please use $foo->[0]
see our style guideline:

https://pve.proxmox.com/wiki/Perl_Style_Guide#Perl_syntax_choices

> +	    $file->{size} = -s $path // 0;
> +	    push @result, $file;
> +	}
> +    }
> +    return @result;
> +}
> +
>   # list templates ($tt = <iso|vztmpl|backup|snippets>)
>   my $get_subdir_files = sub {
>       my ($sid, $path, $tt, $vmid) = @_;
> @@ -982,7 +1049,7 @@ my $get_subdir_files = sub {
>   };
>   
>   sub list_volumes {
> -    my ($class, $storeid, $scfg, $vmid, $content_types) = @_;
> +    my ($class, $storeid, $scfg, $vmid, $content_types, $param) = @_;
>   
>       my $res = [];
>       my $vmlist = PVE::Cluster::get_vmlist();
> @@ -994,10 +1061,12 @@ sub list_volumes {
>   	} elsif ($scfg->{path}) {
>   	    my $path = $class->get_subdir($scfg, $type);
>   
> -	    if ($type eq 'iso' && !defined($vmid)) {
> -		$data = $get_subdir_files->($storeid, $path, 'iso');
> -	    } elsif ($type eq 'vztmpl'&& !defined($vmid)) {
> -		$data = $get_subdir_files->($storeid, $path, 'vztmpl');
> +	    if (($type eq 'iso' || $type eq 'vztmpl') && !defined($vmid)) {
> +		if ($param->{recursive}) {
> +		    $data = [get_subdir_files_rec($storeid, $path, $type)];
> +		} else {
> +		    $data = $get_subdir_files->($storeid, $path, $type);
> +		}
>   	    } elsif ($type eq 'backup') {
>   		$data = $get_subdir_files->($storeid, $path, 'backup', $vmid);
>   	    } elsif ($type eq 'snippets') {
> diff --git a/test/run_plugin_tests.pl b/test/run_plugin_tests.pl
> index cd93430..7f627b1 100755
> --- a/test/run_plugin_tests.pl
> +++ b/test/run_plugin_tests.pl
> @@ -3,7 +3,7 @@ use strict;
>   use warnings;
>   
>   use lib ('.', '..');
> -use Test::More tests => 32;
> +use Test::More tests => 42;
>   use Test::MockModule qw(new);
>   use File::Temp qw(tempdir);
>   use File::Path qw(make_path);
> @@ -13,16 +13,23 @@ use PVE::Storage;
>   
>   my $plugin = 'PVE::Storage::Plugin';
>   my $basename = 'test';
> +my $subdir_regular = 'some/more/subdirectories/';
>   
>   my $iso_type = 'iso';
>   my $iso_suffix = '.iso';
>   my $iso_notdir = "$basename$iso_suffix";
>   my $iso_volname = "$iso_type/$iso_notdir";
> +my $iso_volname_with_subdirs = "$iso_type/$subdir_regular$iso_notdir";
> +# Subdirectories should be treated like normal folders, even if their name
> +# is equal to our predefined storage schema
> +my $subdirs_named_iso = 'a/template/iso/b/template/iso/c/template/iso/';
>   
>   my $vztmpl_type = 'vztmpl';
>   my $vztmpl_suffix = '.tar.gz';
>   my $vztmpl_notdir = "$basename$vztmpl_suffix";
>   my $vztmpl_volname = "$vztmpl_type/$vztmpl_notdir";
> +my $vztmpl_volname_with_subdirs = "$vztmpl_type/$subdir_regular$vztmpl_notdir";
> +my $subdir_named_cache = 'a/template/cache/b/template/cache/c/template/cache/';
>   
>   my $iso_with_dots = "$iso_type/../$iso_notdir";
>   my $vztmpl_with_dots = "$vztmpl_type/../$vztmpl_notdir";
> @@ -48,11 +55,25 @@ is (($plugin->parse_volname($iso_volname))[$type_index],
>       $iso_type, 'parse_volname: type for iso');
>   is (($plugin->parse_volname($iso_volname))[$notdir_index],
>       $iso_notdir, 'parse_volname: notdir for iso');
> +is (($plugin->parse_volname($iso_volname_with_subdirs))[$type_index],
> +    $iso_type, 'parse_volname: type for iso in subdir');
> +is (($plugin->parse_volname($iso_volname_with_subdirs))[$notdir_index],
> +    $iso_notdir, 'parse_volname: notdir for iso in subdir');
> +eval { $plugin->parse_volname($iso_with_dots) };
> +like ($@, qr/volname must not contain two dots/,
> +    'parse_volname: forbid two dots .. for iso');
>   
>   is (($plugin->parse_volname($vztmpl_volname))[$type_index],
>       $vztmpl_type, 'parse_volname: type for vztmpl');
>   is (($plugin->parse_volname($vztmpl_volname))[$notdir_index],
>       $vztmpl_notdir, 'parse_volname: notdir for vztmpl');
> +is (($plugin->parse_volname($vztmpl_volname_with_subdirs))[$type_index],
> +    $vztmpl_type, 'parse_volname: type for vztmpl in subdir');
> +is (($plugin->parse_volname($vztmpl_volname_with_subdirs))[$notdir_index],
> +    $vztmpl_notdir, 'parse_volname: notdir for vztmpl in subdir');
> +eval { $plugin->parse_volname($vztmpl_with_dots) };
> +like ($@, qr/volname must not contain two dots/,
> +    'parse_volname: forbid two dots .. for vztmpl');
>   
>   is (($plugin->parse_volname($raw_image_volname))[$type_index],
>       $image_type, 'parse_volname: type for raw image');
> @@ -91,9 +112,15 @@ is ($plugin->get_subdir($scfg_with_path, 'rootdir'),
>   is ($plugin->filesystem_path($scfg_with_path, $iso_volname),
>       "$scfg_with_path->{path}/template/$iso_volname",
>       'filesystem_path for iso');
> +is ($plugin->filesystem_path($scfg_with_path, $iso_volname_with_subdirs),
> +    "$scfg_with_path->{path}/template/$iso_volname_with_subdirs",
> +    'filesystem_path for iso in subdirs');
>   is ($plugin->filesystem_path($scfg_with_path, $vztmpl_volname),
>       "$scfg_with_path->{path}/template/cache/$vztmpl_notdir",
>       'filesystem_path for vztmpl');
> +is ($plugin->filesystem_path($scfg_with_path, $vztmpl_volname_with_subdirs),
> +    "$scfg_with_path->{path}/template/cache/$subdir_regular$vztmpl_notdir",
> +    'filesystem_path for vztmpl in subdirs');
>   is ($plugin->filesystem_path($scfg_with_path, $raw_image_volname),
>       "$scfg_with_path->{path}/images/$raw_image_volname",
>       'filesystem_path for raw images');
> @@ -142,12 +169,40 @@ my $paths = [
>   ];
>   add_test_files($paths);
>   
> +my $storage_dir_rec = File::Temp->newdir();
> +my $recursive_paths = [
> +    "$storage_dir_rec/template/cache",
> +    "$storage_dir_rec/template/iso",
> +    "$storage_dir_rec/images/$vmid",
> +    "$storage_dir_rec/template/cache/$subdir_regular",
> +    "$storage_dir_rec/template/cache/$subdirs_named_iso",
> +    "$storage_dir_rec/template/cache/$subdir_named_cache",
> +    "$storage_dir_rec/template/iso/$subdir_regular",
> +    "$storage_dir_rec/template/iso/$subdirs_named_iso",
> +    "$storage_dir_rec/template/iso/$subdir_named_cache",
> +];
> +add_test_files($recursive_paths);
> +
>   note 'Tests without recursion';
>   # note "Temp dir is:\n", `tree $storage_dir_no_rec`;
>   my $scfg_no_rec = { path => $storage_dir_no_rec };
>   test_list_volumes($scfg_no_rec, [$iso_type], [$iso_volname]);
>   test_list_volumes($scfg_no_rec, [$vztmpl_type], [$vztmpl_volname]);
>   
> +note 'Tests with recursion';
> +# note "Temp dir is:\n", `tree $storage_dir_rec`;
> +my $scfg_rec = { path => $storage_dir_rec };
> +my $expected_volnames_iso = [$iso_volname, $iso_volname_with_subdirs,
> +   "$iso_type/$subdir_named_cache$iso_notdir",
> +   "$iso_type/$subdirs_named_iso$iso_notdir"];
> +my $expected_volnames_vztmpl = [$vztmpl_volname, $vztmpl_volname_with_subdirs,
> +   "$vztmpl_type/$subdir_named_cache$vztmpl_notdir",
> +   "$vztmpl_type/$subdirs_named_iso$vztmpl_notdir"];
> +test_list_volumes($scfg_rec, [$iso_type], $expected_volnames_iso,
> +	{ recursive => 1 });
> +test_list_volumes($scfg_rec, [$vztmpl_type],$expected_volnames_vztmpl,
> +	{ recursive => 1 });
> +
>   my $scfg_with_type = { path => $storage_dir_no_rec, type => 'dir' };
>   my $plugin_mock = Test::MockModule->new('PVE::Cluster');
>   $plugin_mock->redefine('get_vmlist' => sub { return undef });
> 




More information about the pve-devel mailing list