On 11/15/24 11:05, Fiona Ebner wrote:
On 15.11.24 10:55 AM, Dominik Csapak wrote:
On 11/15/24 10:49, Fiona Ebner wrote:
On 15.11.24 10:42 AM, Fiona Ebner wrote:
On 04.11.24 11:42 AM, Fabian Grünbichler wrote:
creating non-raw disk images with arbitrary content is only possible
with raw
access to the storage, but checking for references to external files
doesn't
hurt.
Signed-off-by: Fabian Grünbichler <f.gruenbich...@proxmox.com>
---
Notes:
requires pve-storage change to actually have an effect
v2:
- re-order code to improve readability
- extract $path into variable to avoid scalar vs array context
issue
PVE/API2/Qemu.pm | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 848001b6..ae0c39bf 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -412,18 +412,29 @@ my sub create_disks : prototype($$$$$$$$$$) {
$needs_creation = $live_import;
- if (PVE::Storage::parse_volume_id($source, 1)) { # PVE-
managed volume
+ my ($source_storage, $source_volid) =
PVE::Storage::parse_volume_id($source, 1);
+
+ if ($source_storage) { # PVE-managed volume
if ($live_import && $ds ne 'efidisk0') {
my $path = PVE::Storage::path($storecfg, $source)
or die "failed to get a path for '$source'\n";
$source = $path;
- ($size, my $source_format) =
PVE::Storage::file_size_info($source);
+ # check potentially untrusted image file!
+ ($size, my $source_format) =
PVE::Storage::file_size_info($source, undef, 1);
+
die "could not get file size of $source\n" if !$size;
$live_import_mapping->{$ds} = {
path => $source,
format => $source_format,
};
} else {
+ # check potentially untrusted image file!
+ my $scfg = PVE::Storage::storage_config($storecfg,
$source_storage);
+ if ($scfg->{path}) {
Is there a special reason this is made conditional on the presence of
$scfg->{path}? Note that the above check for live import isn't.
Shouldn't matter much right now (except if there is a weird third-party
plugin that has qcow2 and doesn't use $scfg->{path}), but as long as we
get a result for PVE::Storage::path(), we should be able to call
file_size_info() too, right?
Or maybe not, but then live-import should've used volume_size_info() too
I guess? I think it would be good to add an 'untrusted' flag to
volume_size_info() too and have the storage layer do the right thing,
rather than manually checking $scfg->{path} here.
my 2 cents:
the checking of untrusted images is mainly helpful for file based volumes
(such as qcow2/vmdk/etc.) where we can detect backing images etc.
Yes, currently. But like this, third-party plugins without a path won't
be able to have checks and maybe we'd like to add different checks in
the future that won't require file-based volumes.
My point is about the mismatch between the check here and the
live-import one. Either we can call file_size_info() for any result of
path() (not sure, because e.g. ISCSIDirectPlugin's path() will return an
'iscsi://' protocol path for exmplae) or we should use
volume_size_info() instead.
true, this should be consistent
just want to note that if path() does not return an actual
path for the filesystem, we'll fail to 'stat' it and die here
while previously we returned undef in that case und died later down
because size wasn't checked
so AFAIU it importing non file based volumes was broken anyway?
But I guess that's orthogonal and can still be improved later.
the check for 'path' is how we ususually determine if a storage is
file-based or not (would probably better to have that check
in storage and it should probably check for qcow2/vmdk/etc. support)
but we already use that check in some places
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
