[PATCH] fix #7828: avoid parsing absolute paths as volume ids

[PATCH] fix #7828: avoid parsing absolute paths as volume ids

[Thomas Ellmenreich]

Just like in the case of cdroms, absolute paths to storage drives can be
skipped when beeing marked as unused as they cannot be owned by the vm.

Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
---
I'm not 100% sure that I have understood the procedure of registering
unused drives correctly, but skipping this branch in the case of an
absolute path as 'volid' seems to be the correct action.

Alternatively, placing the condition next to the `storage_config` check
could make sense, but would just achieve the same thing, as the
subsequent ownership check will never succeed with an absolute path.

 src/PVE/QemuServer.pm | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index 191ae549..782d2e7b 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -1570,12 +1570,14 @@ sub vm_is_volid_owner {
 sub vmconfig_register_unused_drive {
     my ($storecfg, $vmid, $conf, $drive) = @_;
 
+    my $volid = $drive->{file};
+
     if (drive_is_cloudinit($drive)) {
-        eval { PVE::Storage::vdisk_free($storecfg, $drive->{file}) };
+        eval { PVE::Storage::vdisk_free($storecfg, $volid) };
         warn $@ if $@;
         delete $conf->{'special-sections'}->{cloudinit};
-    } elsif (!drive_is_cdrom($drive)) {
-        my $volid = $drive->{file};
+    } elsif (!drive_is_cdrom($drive) && $volid !~ m|^/|) {
+
         my ($storeid, undef) = PVE::Storage::parse_volume_id($volid);
         if (PVE::Storage::storage_config($storecfg, $storeid, 1)) {
             if (vm_is_volid_owner($storecfg, $vmid, $volid)) {
-- 
2.47.3

Re: [PATCH] fix #7828: avoid parsing absolute paths as volume ids

[Daniel Kral]

Thanks for sending in a patch for this!

On Tue Jul 21, 2026 at 2:20 PM CEST, Thomas Ellmenreich wrote:
> Just like in the case of cdroms, absolute paths to storage drives can be
> skipped when beeing marked as unused as they cannot be owned by the vm.
>
> Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>

This should have a

Fixes: 5b5c5768 ("display warnings for storage errors or if storage no longer exists")

as the added PVE::Storage::parse_volume_id() in that commit introduced
this issue as it assumed that there would never be a absolute path
storage here in $drive->{file}.

> ---
> I'm not 100% sure that I have understood the procedure of registering
> unused drives correctly, but skipping this branch in the case of an
> absolute path as 'volid' seems to be the correct action.
>
> Alternatively, placing the condition next to the `storage_config` check
> could make sense, but would just achieve the same thing, as the
> subsequent ownership check will never succeed with an absolute path.

I think the placement is fine here.

I think it would be nice to add a patch before this to add a helper
subroutine PVE::QemuServer::Drive::drive_is_absolute_path() or something
similar:

sub drive_is_absolute_path {
    my ($drive) = @_;
    return $drive->{file} =~ m|^/|;
}

and replace all current checks =~ m|^/| and !~ m|^/| appropriately in
qemu-server similar to the drive_is_cdrom() and drive_is_cloudinit() to
make it easier to understand the special case here.

>
>  src/PVE/QemuServer.pm | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
> index 191ae549..782d2e7b 100644
> --- a/src/PVE/QemuServer.pm
> +++ b/src/PVE/QemuServer.pm
> @@ -1570,12 +1570,14 @@ sub vm_is_volid_owner {
>  sub vmconfig_register_unused_drive {
>      my ($storecfg, $vmid, $conf, $drive) = @_;
>  
> +    my $volid = $drive->{file};
> +
>      if (drive_is_cloudinit($drive)) {
> -        eval { PVE::Storage::vdisk_free($storecfg, $drive->{file}) };
> +        eval { PVE::Storage::vdisk_free($storecfg, $volid) };

It is best to do such changes in their own patch either preceding this
or succeding this patch to make the fixing patch only about fixing the
patch and nothing else ;).

>          warn $@ if $@;
>          delete $conf->{'special-sections'}->{cloudinit};
> -    } elsif (!drive_is_cdrom($drive)) {
> -        my $volid = $drive->{file};
> +    } elsif (!drive_is_cdrom($drive) && $volid !~ m|^/|) {
> +
>          my ($storeid, undef) = PVE::Storage::parse_volume_id($volid);
>          if (PVE::Storage::storage_config($storecfg, $storeid, 1)) {
>              if (vm_is_volid_owner($storecfg, $vmid, $volid)) {