From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <g.goller@proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
key-exchange X25519 server-signature RSA-PSS (2048 bits))
(No client certificate requested)
by lists.proxmox.com (Postfix) with ESMTPS id 4E8EAC611
for <pbs-devel@lists.proxmox.com>; Mon, 14 Aug 2023 09:42:21 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 2863B1F56E
for <pbs-devel@lists.proxmox.com>; Mon, 14 Aug 2023 09:41:51 +0200 (CEST)
Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com
[94.136.29.106])
(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256)
(No client certificate requested)
by firstgate.proxmox.com (Proxmox) with ESMTPS
for <pbs-devel@lists.proxmox.com>; Mon, 14 Aug 2023 09:41:50 +0200 (CEST)
Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1])
by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 377364730A
for <pbs-devel@lists.proxmox.com>; Mon, 14 Aug 2023 09:41:50 +0200 (CEST)
Message-ID: <56ca2e7e-00ca-d565-dcad-add9a06788c8@proxmox.com>
Date: Mon, 14 Aug 2023 09:41:49 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
Thunderbird/102.13.1
Content-Language: en-US
To: Wolfgang Bumiller <w.bumiller@proxmox.com>
Cc: pbs-devel@lists.proxmox.com
References: <20230809101913.81818-1-g.goller@proxmox.com>
<20230809101913.81818-2-g.goller@proxmox.com>
<ff3ue3ab5uscsatexjmo7sahc6vtotqe4jcdzmjc7d22msnzg5@a35jf4thexka>
From: Gabriel Goller <g.goller@proxmox.com>
In-Reply-To: <ff3ue3ab5uscsatexjmo7sahc6vtotqe4jcdzmjc7d22msnzg5@a35jf4thexka>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.697 Adjusted score from AWL reputation of From: address
BAYES_00 -1.9 Bayes spam probability is 0 to 1%
DMARC_MISSING 0.1 Missing DMARC policy
KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
NICE_REPLY_A -2.265 Looks like a legit reply (A)
SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record
SPF_PASS -0.001 SPF: sender matches SPF record
URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See
http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more
information. [create.rs]
Subject: Re: [pbs-devel] [PATCH proxmox-backup v3 1/1] fix #4380: check if
file is excluded before running `stat()`
X-BeenThere: pbs-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox Backup Server development discussion
<pbs-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pbs-devel>,
<mailto:pbs-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pbs-devel/>
List-Post: <mailto:pbs-devel@lists.proxmox.com>
List-Help: <mailto:pbs-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel>,
<mailto:pbs-devel-request@lists.proxmox.com?subject=subscribe>
X-List-Received-Date: Mon, 14 Aug 2023 07:42:21 -0000
On 8/11/23 10:51, Wolfgang Bumiller wrote:
> On Wed, Aug 09, 2023 at 12:19:13PM +0200, Gabriel Goller wrote:
>> Before running `stat()` we now make a `matches_path()` call, which checks
> I was more thinking to use `matches_path()` only if `stat()` actually
> fails and then propagate `stat()`'s error (instead of the error that a
> file mode is required... ;-) )
> That way we wouldn't match twice in the other case.
I used this approach so that we don't `stat` every excluded files as
well (I think `matches_path` is
probably faster than `stat`). Also we will very rarely make the matching
twice, because most of the
excluded paths come from the `--exclude` parameter or the `.pxarexclude`
file, which both don't
allow to set the `file_mode`.
>> if the file/directory is excluded (`matches_path()` tries to match using
>> only the path, without the file mode). If the file is not excluded,
>> we `stat()` and then check again if the path matches using the file_mode and
>> the `matches()` function.
>> Added `pathpatterns` crate to local overrides in cargo.toml.
>>
>> changes v2:
>> - checking for `read` and `execute` permissions before entering directory,
>> doesn't work because there are a lot of side-effects (executed by
>> different user, AppArmor, SELinux, ...).
>> changes v1:
>> - checking for excluded files with `matches()` before executing `stat()`,
>> this doesn't work because we get the file_mode from `stat()` and don't
>> want to ignore it when matching.
>>
>> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
>> ---
>> Cargo.toml | 1 +
>> pbs-client/src/pxar/create.rs | 45 ++++++++++++++++++++++++++---------
>> 2 files changed, 35 insertions(+), 11 deletions(-)
>>
>> diff --git a/Cargo.toml b/Cargo.toml
>> index 2a05c471..c54bc28b 100644
>> --- a/Cargo.toml
>> +++ b/Cargo.toml
>> @@ -262,6 +262,7 @@ proxmox-rrd.workspace = true
>>
>> #proxmox-apt = { path = "../proxmox-apt" }
>> #proxmox-openid = { path = "../proxmox-openid-rs" }
>> +#pathpatterns = {path = "../pathpatterns" }
>>
>> #pxar = { path = "../pxar" }
>>
>> diff --git a/pbs-client/src/pxar/create.rs b/pbs-client/src/pxar/create.rs
>> index 2577cf98..a6d608e7 100644
>> --- a/pbs-client/src/pxar/create.rs
>> +++ b/pbs-client/src/pxar/create.rs
>> @@ -232,7 +232,7 @@ impl Archiver {
>> let entry_counter = self.entry_counter;
>>
>> let old_patterns_count = self.patterns.len();
>> - self.read_pxar_excludes(dir.as_raw_fd())?;
>> + self.read_pxar_excludes(&mut dir)?;
>>
>> let mut file_list = self.generate_directory_file_list(&mut dir, is_root)?;
>>
>> @@ -315,8 +315,23 @@ impl Archiver {
>> }
>> }
>>
>> - fn read_pxar_excludes(&mut self, parent: RawFd) -> Result<(), Error> {
>> - let fd = match self.open_file(parent, c_str!(".pxarexclude"), OFlag::O_RDONLY, false)? {
>> + fn read_pxar_excludes(&mut self, parent: &mut Dir) -> Result<(), Error> {
>> + let mut exclude_file_found = false;
>> + for file in parent.iter() {
>> + if file?.file_name().to_str()? == ".pxarexclude" {
>> + exclude_file_found = true;
>> + break;
>> + }
>> + }
>> + if !exclude_file_found {
> Why are we adding this check?
> It's not necessary.
It is not strictly necessary, but the problem is that the function tries
to open `.pxarexclude` in every directory.
Also directories were we have no access to, which will obviously fail.
So if we have a directory with limited permissions,
(regardless if a `.pxarexclude` lives in it or not) we will get an error
`failed to open .pxarexclude file` (even though it
doesn't even exist).
>> + return Ok(());
>> + }
>> + let fd = match self.open_file(
>> + parent.as_raw_fd(),
>> + c_str!(".pxarexclude"),
>> + OFlag::O_RDONLY,
>> + false,
>> + )? {
>> Some(fd) => fd,
>> None => return Ok(()),
> ^ This already means the file does not exist.
>
>> };
>> @@ -420,7 +435,7 @@ impl Archiver {
>> for file in dir.iter() {
>> let file = file?;
>>
>> - let file_name = file.file_name().to_owned();
>> + let file_name = file.file_name();
>> let file_name_bytes = file_name.to_bytes();
>> if file_name_bytes == b"." || file_name_bytes == b".." {
>> continue;
>> @@ -434,9 +449,17 @@ impl Archiver {
>> assert_single_path_component(os_file_name)?;
>> let full_path = self.path.join(os_file_name);
>>
>> + let match_path = PathBuf::from("/").join(full_path.clone());
>> + let pre_stat_match = self
>> + .patterns
>> + .matches_path(match_path.as_os_str().as_bytes());
> As said above, I'd like this to instead happen in stat's `Err() => {}`
> case.
>
>> + if pre_stat_match == Ok(Some(MatchType::Exclude)) {
>> + continue;
>> + }
>> +
>> let stat = match nix::sys::stat::fstatat(
>> dir_fd,
>> - file_name.as_c_str(),
>> + file_name.to_owned().as_c_str(),
>> nix::fcntl::AtFlags::AT_SYMLINK_NOFOLLOW,
>> ) {
>> Ok(stat) => stat,
>> @@ -444,11 +467,11 @@ impl Archiver {
>> Err(err) => return Err(err).context(format!("stat failed on {:?}", full_path)),
>> };
>>
>> - let match_path = PathBuf::from("/").join(full_path.clone());
>> - if self
>> - .patterns
>> - .matches(match_path.as_os_str().as_bytes(), Some(stat.st_mode))
>> - == Some(MatchType::Exclude)
>> + if pre_stat_match.is_err()
> And then this won't be needed and AFAICT isn't correct with the current
> implementation of `matches_path()` of this series since it might have
> found a match *after* one of a higher precedence would have already
> matched potentially differently.
I'll fix the implementation of `matches_path()` so that it actually
fails (returns `Err`) when a pattern
with a `file_mode` is encountered (as it was intended). Otherwise this
should work, as I didn't change
anything (except the preceding `matches_path()`).
>> + && self
>> + .patterns
>> + .matches(match_path.as_os_str().as_bytes(), Some(stat.st_mode))
>> + == Some(MatchType::Exclude)
>> {
>> continue;
>> }
>> @@ -462,7 +485,7 @@ impl Archiver {
>> }
>>
>> file_list.push(FileListEntry {
>> - name: file_name,
>> + name: file_name.to_owned(),
>> path: full_path,
>> stat,
>> });
>> --
>> 2.39.2