From: Christian Ebner <c.ebner@proxmox.com>
To: "Fabian Grünbichler" <f.gruenbichler@proxmox.com>,
pbs-devel@lists.proxmox.com
Subject: Re: [pbs-devel] [PATCH proxmox-backup] client: catalog shell: avoid navigating below archive root
Date: Wed, 7 Aug 2024 12:54:50 +0200 [thread overview]
Message-ID: <202bfa01-f571-4343-a432-ac9d08f5d87d@proxmox.com> (raw)
In-Reply-To: <172301911740.107519.13980702778008805321@yuna.proxmox.com>
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
next prev parent reply other threads:[~2024-08-07 10:55 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-07-22 10:07 Christian Ebner
2024-08-07 8:25 ` Fabian Grünbichler
2024-08-07 10:54 ` Christian Ebner [this message]
2024-08-07 11:24 ` Fabian Grünbichler
2024-08-08 11:44 ` Christian Ebner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=202bfa01-f571-4343-a432-ac9d08f5d87d@proxmox.com \
--to=c.ebner@proxmox.com \
--cc=f.gruenbichler@proxmox.com \
--cc=pbs-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox