[PATCH proxmox-backup] tape: virtual drive: fix endless recursion in changer status

[PATCH proxmox-backup] tape: virtual drive: fix endless recursion in changer status

[Jakob-Tobias Winter]

The virtual tape drive, which exists for tests and debugging only, also
acts as its own changer and reports every virtual tape in its directory
as a slot. To build that status it asked for the list of online media,
but got the generic variant of that list, which is derived from the
changer status. The two called each other until the stack overflowed, so
every operation that needs the changer status of a virtual drive, like
loading or unloading a tape, crashed the process.

Use the list of tapes in the directory of the drive instead, as it was
intended, for the status and for loading a tape from a slot.

Signed-off-by: Jakob-Tobias Winter <tobias@stuttgart.de>
---
The recursion has been there since 46a1863f8 ("tape: improve MediaChange
trait").

Reproducer, with a virtual drive 'vtape' in tape.cfg and an empty file
'tape-demo1.json' in its directory:

  proxmox-backup-debug api create /tape/drive/vtape/load-slot \
      --source-slot 1

Tested with debug builds of current master and of 4.2.8, calling the API
in-process (PROXMOX_DEBUG_API_CODE=1): the call aborts with a stack
overflow without the patch and loads the tape with it.

 src/tape/drive/virtual_tape.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/tape/drive/virtual_tape.rs b/src/tape/drive/virtual_tape.rs
index a17916bab..b2febaed5 100644
--- a/src/tape/drive/virtual_tape.rs
+++ b/src/tape/drive/virtual_tape.rs
@@ -499,7 +499,7 @@ impl MediaChange for VirtualTapeHandle {
         // slot-assignment here.
 
         let mut slots = Vec::new();
-        let label_texts = self.online_media_label_texts()?;
+        let label_texts = VirtualTapeHandle::online_media_label_texts(self)?;
         let max_slots = label_texts.len().div_ceil(8) * 8;
 
         for i in 0..max_slots {
@@ -535,7 +535,7 @@ impl MediaChange for VirtualTapeHandle {
             bail!("invalid slot ID {}", slot);
         }
 
-        let label_texts = self.online_media_label_texts()?;
+        let label_texts = VirtualTapeHandle::online_media_label_texts(self)?;
 
         if slot > label_texts.len() as u64 {
             bail!("slot {} is empty", slot);
-- 
2.53.0

Re: [PATCH proxmox-backup] tape: virtual drive: fix endless recursion in changer status

[Christian Ebner]

On 11/9/26 11:07 PM, Jakob-Tobias Winter wrote:
> The virtual tape drive, which exists for tests and debugging only, also
> acts as its own changer and reports every virtual tape in its directory
> as a slot. To build that status it asked for the list of online media,
> but got the generic variant of that list, which is derived from the
> changer status. The two called each other until the stack overflowed, so
> every operation that needs the changer status of a virtual drive, like
> loading or unloading a tape, crashed the process.
> 
> Use the list of tapes in the directory of the drive instead, as it was
> intended, for the status and for loading a tape from a slot.

Thanks for the patch, I would however prefer a different approach for 
fixing this:

The issue is that VirtualTapeHandle does not implement the MediaChange 
traits online_media_label_texts() method with signature

fn online_media_label_texts(&mut self) -> Result<Vec<String>, Error>>

but has it's own impl with a slightly different method signature

fn online_media_label_texts(&self) -> Result<Vec<String>, Error>>

which leads to the incorrect method being called and therefore the 
recursion in the first place.

So the better fix would be to move the VirtualTapeHandle's custom impl 
to actually override the MediaChange trait method.
> Signed-off-by: Jakob-Tobias Winter <tobias@stuttgart.de>
> ---
> The recursion has been there since 46a1863f8 ("tape: improve MediaChange
> trait").

nit: please include this as Fixes: ... trailer with the patch.

> Reproducer, with a virtual drive 'vtape' in tape.cfg and an empty file
> 'tape-demo1.json' in its directory:
> 
>    proxmox-backup-debug api create /tape/drive/vtape/load-slot \
>        --source-slot 1
> 
> Tested with debug builds of current master and of 4.2.8, calling the API
> in-process (PROXMOX_DEBUG_API_CODE=1): the call aborts with a stack
> overflow without the patch and loads the tape with it.
> 
>   src/tape/drive/virtual_tape.rs | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/tape/drive/virtual_tape.rs b/src/tape/drive/virtual_tape.rs
> index a17916bab..b2febaed5 100644
> --- a/src/tape/drive/virtual_tape.rs
> +++ b/src/tape/drive/virtual_tape.rs
> @@ -499,7 +499,7 @@ impl MediaChange for VirtualTapeHandle {
>           // slot-assignment here.
>   
>           let mut slots = Vec::new();
> -        let label_texts = self.online_media_label_texts()?;
> +        let label_texts = VirtualTapeHandle::online_media_label_texts(self)?;
>           let max_slots = label_texts.len().div_ceil(8) * 8;
>   
>           for i in 0..max_slots {
> @@ -535,7 +535,7 @@ impl MediaChange for VirtualTapeHandle {
>               bail!("invalid slot ID {}", slot);
>           }
>   
> -        let label_texts = self.online_media_label_texts()?;
> +        let label_texts = VirtualTapeHandle::online_media_label_texts(self)?;
>   
>           if slot > label_texts.len() as u64 {
>               bail!("slot {} is empty", slot);