[pbs-devel] [PATCH proxmox-backup] client: catalog shell: avoid navigating below archive root

[pbs-devel] [PATCH proxmox-backup] client: catalog shell: avoid navigating below archive root

[Christian Ebner]

Avoid to underflow the catalogs shell position stack by navigating
below the archives root directory into the catalog root. Otherwise
the shell will panic, as the root entry is always expected to be
present.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
Encountered while implementing the catalog shell for the split pxar
archive case.

Without this additional check, a `cd ..` in the pxar archive root of
the catalog shell will panic.

 pbs-client/src/catalog_shell.rs | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/pbs-client/src/catalog_shell.rs b/pbs-client/src/catalog_shell.rs
index 349bb7cbc..88045db5d 100644
--- a/pbs-client/src/catalog_shell.rs
+++ b/pbs-client/src/catalog_shell.rs
@@ -720,6 +720,12 @@ impl Shell {
                     &mut None,
                 )
                 .await?;
+
+                if new_position.len() < 1 {
+                    // Avoid moving below archive root into catalog root
+                    return Ok(());
+                }
+
                 if !new_position.last().unwrap().catalog.is_directory() {
                     bail!("not a directory");
                 }
-- 
2.39.2



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox-backup] client: catalog shell: avoid navigating below archive root

[Fabian Grünbichler]

Quoting Christian Ebner (2024-07-22 12:07:05)
> Avoid to underflow the catalogs shell position stack by navigating
> below the archives root directory into the catalog root. Otherwise
> the shell will panic, as the root entry is always expected to be
> present.
> 
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
> Encountered while implementing the catalog shell for the split pxar
> archive case.
> 
> Without this additional check, a `cd ..` in the pxar archive root of
> the catalog shell will panic.
> 
>  pbs-client/src/catalog_shell.rs | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/pbs-client/src/catalog_shell.rs b/pbs-client/src/catalog_shell.rs
> index 349bb7cbc..88045db5d 100644
> --- a/pbs-client/src/catalog_shell.rs
> +++ b/pbs-client/src/catalog_shell.rs
> @@ -720,6 +720,12 @@ impl Shell {
>                      &mut None,
>                  )
>                  .await?;
> +
> +                if new_position.len() < 1 {
> +                    // Avoid moving below archive root into catalog root
> +                    return Ok(());
> +                }

since new_position is a Vec, wouldn't `new_position.is_empty()` be the right
fit?

should this print a warning? I guess since regular shells pretend `/` is the
parent of `/`, we can do the same here?

> +
>                  if !new_position.last().unwrap().catalog.is_directory() {
>                      bail!("not a directory");
>                  }
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
>


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox-backup] client: catalog shell: avoid navigating below archive root

[Christian Ebner]

On 8/7/24 10:25, Fabian Grünbichler wrote:
> Quoting Christian Ebner (2024-07-22 12:07:05)
>> Avoid to underflow the catalogs shell position stack by navigating
>> below the archives root directory into the catalog root. Otherwise
>> the shell will panic, as the root entry is always expected to be
>> present.
>>
>> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
>> ---
>> Encountered while implementing the catalog shell for the split pxar
>> archive case.
>>
>> Without this additional check, a `cd ..` in the pxar archive root of
>> the catalog shell will panic.
>>
>>   pbs-client/src/catalog_shell.rs | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/pbs-client/src/catalog_shell.rs b/pbs-client/src/catalog_shell.rs
>> index 349bb7cbc..88045db5d 100644
>> --- a/pbs-client/src/catalog_shell.rs
>> +++ b/pbs-client/src/catalog_shell.rs
>> @@ -720,6 +720,12 @@ impl Shell {
>>                       &mut None,
>>                   )
>>                   .await?;
>> +
>> +                if new_position.len() < 1 {
>> +                    // Avoid moving below archive root into catalog root
>> +                    return Ok(());
>> +                }
> 
> since new_position is a Vec, wouldn't `new_position.is_empty()` be the right
> fit?

Yes, this is even a clippy warning I overlooked, will send a new version 
for this.

> 
> should this print a warning? I guess since regular shells pretend `/` is the
> parent of `/`, we can do the same here?

I'm not sure a warning would bring much benefit here, unless we plan to 
allow to navigate the whole catalog at some point, not just the archive 
given as parameter when invoking the shell.

Also, could you clarify what do you mean exactly with the latter 
question. With the patch applied the shell should treat '/' as parent of 
itself, or am I missing something.



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox-backup] client: catalog shell: avoid navigating below archive root

[Fabian Grünbichler]

> Christian Ebner <c.ebner@proxmox.com> hat am 07.08.2024 12:54 CEST geschrieben:
> 
>  
> On 8/7/24 10:25, Fabian Grünbichler wrote:
> > Quoting Christian Ebner (2024-07-22 12:07:05)
> >> Avoid to underflow the catalogs shell position stack by navigating
> >> below the archives root directory into the catalog root. Otherwise
> >> the shell will panic, as the root entry is always expected to be
> >> present.
> >>
> >> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> >> ---
> >> Encountered while implementing the catalog shell for the split pxar
> >> archive case.
> >>
> >> Without this additional check, a `cd ..` in the pxar archive root of
> >> the catalog shell will panic.
> >>
> >>   pbs-client/src/catalog_shell.rs | 6 ++++++
> >>   1 file changed, 6 insertions(+)
> >>
> >> diff --git a/pbs-client/src/catalog_shell.rs b/pbs-client/src/catalog_shell.rs
> >> index 349bb7cbc..88045db5d 100644
> >> --- a/pbs-client/src/catalog_shell.rs
> >> +++ b/pbs-client/src/catalog_shell.rs
> >> @@ -720,6 +720,12 @@ impl Shell {
> >>                       &mut None,
> >>                   )
> >>                   .await?;
> >> +
> >> +                if new_position.len() < 1 {
> >> +                    // Avoid moving below archive root into catalog root
> >> +                    return Ok(());
> >> +                }
> > 
> > since new_position is a Vec, wouldn't `new_position.is_empty()` be the right
> > fit?
> 
> Yes, this is even a clippy warning I overlooked, will send a new version 
> for this.
> 
> > 
> > should this print a warning? I guess since regular shells pretend `/` is the
> > parent of `/`, we can do the same here?
> 
> I'm not sure a warning would bring much benefit here, unless we plan to 
> allow to navigate the whole catalog at some point, not just the archive 
> given as parameter when invoking the shell.
> 
> Also, could you clarify what do you mean exactly with the latter 
> question. With the patch applied the shell should treat '/' as parent of 
> itself, or am I missing something.

sorry, yes, that was not phrased clearly. 

I just wanted to make sure that this is intentional, and "shell" was referring to a regular shell not our catalog shell. as in - is it intentional that this is silent (like it is in regular shells when doing "cd .." while already being in "/"), or do we want to have a warning there?


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox-backup] client: catalog shell: avoid navigating below archive root

[Christian Ebner]

superseded-by version 2:
https://lists.proxmox.com/pipermail/pbs-devel/2024-August/010479.html


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel