[pve-devel] [PATCH storage v2 1/3] Recursive search for iso and vztmpl

Dominic Jäger d.jaeger at proxmox.com
Tue May 19 11:58:16 CEST 2020


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>
---
v2:
Adapt to Alwin's zstd patch
 - Adapt tests
 - Use file::stat
Dominik's Feedback
 - Regex: Fix rule for double dots, no useless ORs, ? instead of *
 - No full sub for recursion
 - No unfamiliar names like 'notdir'
 - Syntax: More parentheses, no $$, no 'not'
Additional
 - Syntax: More concise, stricter linewidth, clearer names
 - Add volid to the helper function parse_path and use it in both
   get_subdir_files and get_subdir_files_recursive.
   The comment might be a bit verbose, but having a "glossary" for newcomers
   did not seem too bad to me.

 PVE/API2/Storage/Content.pm  |   9 +++-
 PVE/API2/Storage/Status.pm   |  13 ++---
 PVE/Storage.pm               |  12 ++++-
 PVE/Storage/Plugin.pm        | 102 +++++++++++++++++++++++++++++++----
 test/filesystem_path_test.pm |  18 +++++++
 test/list_volumes_test.pm    |  56 ++++++++++++++++++-
 test/parse_volname_test.pm   |  42 ++++++++++++++-
 7 files changed, 230 insertions(+), 22 deletions(-)

diff --git a/PVE/API2/Storage/Content.pm b/PVE/API2/Storage/Content.pm
index f2e3e57..667be68 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..f3af879 100644
--- a/PVE/API2/Storage/Status.pm
+++ b/PVE/API2/Storage/Status.pm
@@ -403,25 +403,26 @@ __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 =~ $PVE::Storage::forbidden_double_dots_re;
+	$filename =~ s/^[\/\\]*//; # files are uploaded relative to storage path
+	$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 f1e3b19..3c3f24c 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -104,6 +104,14 @@ PVE::Storage::Plugin->init();
 my $UDEVADM = '/sbin/udevadm';
 
 our $iso_extension_re = qr/\.(?:iso|img)/i;
+{
+    # '..' is forbidden at the beginning, between two '/' and at the end
+    my $dots = quotemeta('..');
+    my $beginning = qr!^$dots/!;
+    my $between = qr!/$dots/!;
+    my $end = qr!/$dots$!;
+    our $forbidden_double_dots_re = qr!(?:$beginning|$between|$end)!;
+}
 
 #  PVE::Storage utility functions
 
@@ -937,7 +945,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);
 
@@ -951,7 +959,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 cec136e..3e09af9 100644
--- a/PVE/Storage/Plugin.pm
+++ b/PVE/Storage/Plugin.pm
@@ -418,6 +418,9 @@ sub parse_name_dir {
     die "unable to parse volume filename '$name'\n";
 }
 
+# for iso and vztmpl returns
+#   [0] format
+#   [1] basename+suffix
 sub parse_volname {
     my ($class, $volname) = @_;
 
@@ -431,9 +434,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)$!) {
+	die "volname must not contain two dots '..'\n" if $volname =~ $PVE::Storage::forbidden_double_dots_re;
 	return ('iso', $1);
-    } elsif ($volname =~ m!^vztmpl/([^/]+\.tar\.[gx]z)$!) {
+    } elsif ($volname =~ m!^vztmpl/(?:.+/)?([^/]+\.tar\.[gx]z)$!) {
+	die "volname must not contain two dots '..'\n" if $volname =~ $PVE::Storage::forbidden_double_dots_re;
 	return ('vztmpl', $1);
     } elsif ($volname =~ m!^rootdir/(\d+)$!) {
 	return ('rootdir', $1, $1);
@@ -492,7 +497,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;
 }
@@ -915,6 +927,72 @@ sub list_images {
     return $res;
 }
 
+
+# Input:
+#    $path ... a filesystem path (e.g. /root/iso/proxmox-ve.iso)
+#    $type ... <iso|vztmpl>
+#    $storage_path ... filesystem path of the storage (e.g. /root/iso)
+#    $storage_id ... identifier of the storage (e.g. local-iso)
+# Returns:
+#    If $path fits $type
+#	[0] format (e.g. iso)
+#	[1] volname (e.g. iso/proxmox-ve.iso)
+#	[2] volid (e.g. local-iso:iso/proxmox-ve.iso)
+#    undef otherwise
+sub parse_path {
+    my ($path, $type, $storage_path, $storage_id) = @_;
+    die "path must not contain two dots '..'\n"
+	if $path =~ $PVE::Storage::forbidden_double_dots_re;
+
+    if ($type eq 'iso') {
+	if ($path =~ m!$storage_path/(.+$PVE::Storage::iso_extension_re)$!i) {
+	    my $volname = "$type/$1";
+	    return [$type, $volname, "$storage_id:$volname"];
+	}
+	return undef;
+    } elsif ($type eq 'vztmpl') {
+	if ($path =~ m!$storage_path/(.+\.tar\.([gx]z))$!) {
+	    my $volname = "$type/$1";
+	    return ["t$2", $volname, "$storage_id:$volname"];
+	}
+	return undef;
+    } else {
+	die "Invalid parameter type=$type";
+    }
+}
+
+# $type = <iso|vztmpl>
+my $get_subdir_files_recursive;
+$get_subdir_files_recursive = sub {
+    my ($sid, $path, $type, $storage_path) = @_;
+    die "Invalid parameter" if !($type eq 'iso' || $type eq 'vztmpl');
+    die "Path must not contain two dots '..'\n"
+	if $path =~ $PVE::Storage::forbidden_double_dots_re;
+    # Each call needs to know the original storage path
+    $storage_path = $path if !defined($storage_path);
+
+    my $st = File::stat::stat($path);
+    die "Could not get status info for file '$path'" if !$st;
+
+    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 (S_ISDIR($st->mode)) {
+	foreach my $fn (<$path/*>) {
+	    push @result, $get_subdir_files_recursive->($sid, $fn, $type, $storage_path);
+	}
+    } else {
+	my $properties = parse_path($path, $type, $storage_path, $sid);
+	if ($properties) { # Ignore other files, e.g. with ending .iso.torrent
+	    my $file = { volid => $properties->[2], format => $properties->[0] };
+	    $file->{size} = $st->size;
+	    $file->{ctime} = $st->ctime;
+	    push @result, $file;
+	}
+    }
+    return @result;
+};
+
 # list templates ($tt = <iso|vztmpl|backup|snippets>)
 my $get_subdir_files = sub {
     my ($sid, $path, $tt, $vmid) = @_;
@@ -932,12 +1010,14 @@ my $get_subdir_files = sub {
 	if ($tt eq 'iso') {
 	    next if $fn !~ m!/([^/]+$PVE::Storage::iso_extension_re)$!i;
 
-	    $info = { volid => "$sid:iso/$1", format => 'iso' };
+	    my $properties = parse_path($fn, $tt, $path, $sid);
+	    $info = { volid => $properties->[2], format => $properties->[0] };
 
 	} elsif ($tt eq 'vztmpl') {
 	    next if $fn !~ m!/([^/]+\.tar\.([gx]z))$!;
 
-	    $info = { volid => "$sid:vztmpl/$1", format => "t$2" };
+	    my $properties = parse_path($fn, $tt, $path, $sid);
+	    $info = { volid => $properties->[2], format => $properties->[0] };
 
 	} elsif ($tt eq 'backup') {
 	    next if defined($vmid) && $fn !~  m/\S+-$vmid-\S+/;
@@ -975,7 +1055,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();
@@ -987,10 +1067,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_recursive->($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/filesystem_path_test.pm b/test/filesystem_path_test.pm
index c1b6d90..35a1d4e 100644
--- a/test/filesystem_path_test.pm
+++ b/test/filesystem_path_test.pm
@@ -56,6 +56,24 @@ my $tests = [
 	    'iso'
 	],
     },
+    {
+	volname  => 'iso/a/b/c/my-awesome-proxmox.iso',
+	snapname => undef,
+	expected => [
+	    "$path/template/iso/a/b/c/my-awesome-proxmox.iso",
+	    undef,
+	    'iso'
+	],
+    },
+    {
+	volname  => 'vztmpl/a/b/c/my-awesome-template.tar.gz',
+	snapname => undef,
+	expected => [
+	    "$path/template/cache/a/b/c/my-awesome-template.tar.gz",
+	    undef,
+	    'vztmpl'
+	],
+    },
     {
 	volname  => "backup/vzdump-qemu-1234-2020_03_30-21_12_40.vma",
 	snapname => undef,
diff --git a/test/list_volumes_test.pm b/test/list_volumes_test.pm
index 02edc35..cbf7ec3 100644
--- a/test/list_volumes_test.pm
+++ b/test/list_volumes_test.pm
@@ -395,6 +395,55 @@ my @tests = (
 	    },
 	],
     },
+    {
+	description => 'VMID: none, nested iso and vztmpl',
+	vmid => undef,
+	files => [
+	    "$storage_dir/template/iso/test.iso",
+	    "$storage_dir/template/iso/a/b/c/test.iso",
+	    "$storage_dir/template/iso/a/b.b/c/test.iso",
+	    "$storage_dir/template/cache/a/b/c/test.tar.gz",
+	    "$storage_dir/template/cache/a/b.b/c/test.tar.gz",
+	],
+	param => {recursive => 1},
+	expected => [
+	    {
+		'content' => 'vztmpl',
+		'format'  => 'tgz',
+		'size'    => DEFAULT_SIZE,
+		'ctime'   => DEFAULT_CTIME,
+		'volid'   => 'local:vztmpl/a/b/c/test.tar.gz',
+	    },
+	    {
+		'content' => 'vztmpl',
+		'format'  => 'tgz',
+		'size'    => DEFAULT_SIZE,
+		'ctime'   => DEFAULT_CTIME,
+		'volid'   => 'local:vztmpl/a/b.b/c/test.tar.gz',
+	    },
+	    {
+		'content' => 'iso',
+		'format'  => 'iso',
+		'size'    => DEFAULT_SIZE,
+		'ctime'   => DEFAULT_CTIME,
+		'volid'   => 'local:iso/a/b/c/test.iso',
+	    },
+	    {
+		'content' => 'iso',
+		'format'  => 'iso',
+		'size'    => DEFAULT_SIZE,
+		'ctime'   => DEFAULT_CTIME,
+		'volid'   => 'local:iso/a/b.b/c/test.iso',
+	    },
+	    {
+		'content' => 'iso',
+		'format'  => 'iso',
+		'size'    => DEFAULT_SIZE,
+		'ctime'   => DEFAULT_CTIME,
+		'volid'   => 'local:iso/test.iso',
+	    },
+	],
+    },
     {
 	description => 'VMID: none, parent, non-matching',
 	# string instead of vmid in folder
@@ -427,8 +476,10 @@ my @tests = (
 	    "$storage_dir/images/ssss/base-4321-disk-0.raw",
 	    "$storage_dir/images/ssss/vm-1234-disk-0.qcow2",
 	    "$storage_dir/template/iso/yet-again-a-installation-disk.dvd",
+	    "$storage_dir/template/iso/../../double_points_are_forbidden.iso",
 	    "$storage_dir/template/cache/debian-10.0-standard_10.0-1_amd64.zip.gz",
 	    "$storage_dir/template/cache/debian-10.0-standard_10.0-1_amd64.tar.bz2",
+	    "$storage_dir/template/cache/../../double_points_are_forbidden.tar.gz",
 	    "$storage_dir/private/subvol-19254-disk-0/19254",
 	    "$storage_dir/dump/vzdump-openvz-16112-2020_03_30-21_39_30.tar.bz2",
 	    "$storage_dir/dump/vzdump-openvz-16112-2020_03_30-21_39_30.zip.gz",
@@ -501,6 +552,7 @@ plan tests => $plan + 1;
 	my $expected = $tt->{expected};
 	my $description = $tt->{description};
 	my $parent = $tt->{parent};
+	my $param = $tt->{param};
 
 	# prepare environment
 	my $num = 0; #parent disks
@@ -521,10 +573,10 @@ plan tests => $plan + 1;
 	}
 
 	my $got;
-	eval { $got = PVE::Storage::Plugin->list_volumes($sid, $scfg, $vmid, $types) };
+	eval { $got = PVE::Storage::Plugin->list_volumes($sid, $scfg, $vmid, $types, $param) };
 	$got = $@ if $@;
 
-	is_deeply($got, $expected, $description) || diag(explain($got));
+	is_deeply($got, $expected, $description) || diag("Got\n", explain($got), "but expected\n", explain($expected));
 
 	# clean up after each test case, otherwise
 	# we get wrong results from leftover files
diff --git a/test/parse_volname_test.pm b/test/parse_volname_test.pm
index d6ac885..d6fb9ea 100644
--- a/test/parse_volname_test.pm
+++ b/test/parse_volname_test.pm
@@ -31,11 +31,26 @@ my $tests = [
 	volname     => 'iso/some-installation-disk.iso',
 	expected    => ['iso', 'some-installation-disk.iso'],
     },
+    {
+	description => 'ISO image, iso, nested',
+	volname     => 'iso/a/b/c/some-installation-disk.iso',
+	expected    => ['iso', 'some-installation-disk.iso'],
+    },
     {
 	description => 'ISO image, img',
 	volname     => 'iso/some-other-installation-disk.img',
 	expected    => ['iso', 'some-other-installation-disk.img'],
     },
+    {
+	description => 'ISO image, img, nested',
+	volname     => 'iso/a/b/c/some-other-installation-disk.img',
+	expected    => ['iso', 'some-other-installation-disk.img'],
+    },
+    {
+	description => 'ISO image, img, nested with dots',
+	volname     => 'iso/a/b.b/c/some-other-installation-disk.img',
+	expected    => ['iso', 'some-other-installation-disk.img'],
+    },
     #
     # container templates
     #
@@ -44,11 +59,21 @@ my $tests = [
 	volname     => 'vztmpl/debian-10.0-standard_10.0-1_amd64.tar.gz',
 	expected    => ['vztmpl', 'debian-10.0-standard_10.0-1_amd64.tar.gz'],
     },
+    {
+	description => 'Container template tar.gz nested',
+	volname     => 'vztmpl/a/b/c/debian-10.0-standard_10.0-1_amd64.tar.gz',
+	expected    => ['vztmpl', 'debian-10.0-standard_10.0-1_amd64.tar.gz'],
+    },
     {
 	description => 'Container template tar.xz',
 	volname     => 'vztmpl/debian-10.0-standard_10.0-1_amd64.tar.xz',
 	expected    => ['vztmpl', 'debian-10.0-standard_10.0-1_amd64.tar.xz'],
     },
+    {
+	description => 'Container template tar.xz nested',
+	volname     => 'vztmpl/a/b/c/debian-10.0-standard_10.0-1_amd64.tar.xz',
+	expected    => ['vztmpl', 'debian-10.0-standard_10.0-1_amd64.tar.xz'],
+    },
     #
     # container rootdir
     #
@@ -93,6 +118,21 @@ my $tests = [
 	volname     => 'iso/yet-again-a-installation-disk.dvd',
 	expected    => "unable to parse directory volume name 'iso/yet-again-a-installation-disk.dvd'\n",
     },
+    {
+	description => 'Failed match: ISO image, forbidden double dots between two /',
+	volname     => 'iso/a/../c/some-installation-disk.iso',
+	expected    => "volname must not contain two dots '..'\n",
+    },
+    {
+	description => 'Failed match: ISO image, forbidden double dots in the beginning',
+	volname     => '../iso/some-installation-disk.iso',
+	expected    => "unable to parse directory volume name '../iso/some-installation-disk.iso'\n",
+    },
+    {
+	description => 'Failed match: ISO image, forbidden double dots in the end',
+	volname     => 'iso/some-installation-disk.iso..',
+	expected    => "unable to parse directory volume name 'iso/some-installation-disk.iso..'\n",
+    },
     {
 	description => 'Failed match: Container template, zip.gz',
 	volname     => 'vztmpl/debian-10.0-standard_10.0-1_amd64.zip.gz',
@@ -239,7 +279,7 @@ foreach my $t (@$tests) {
     eval { $got = [ PVE::Storage::Plugin->parse_volname($volname) ] };
     $got = $@ if $@;
 
-    is_deeply($got, $expected, $description);
+    is_deeply($got, $expected, $description) || diag("Got\n", explain($got), "but expected\n", explain($expected));
 
     $seen_vtype->{@$expected[0]} = 1 if ref $expected eq 'ARRAY';
 }
-- 
2.20.1




More information about the pve-devel mailing list