On 8/25/22 07:23, Lukas Javorsky wrote:
[1] https://www.mail-archive.com/bug-tar@gnu.org/msg05904.html
[2] https://bugzilla.redhat.com/show_bug.cgi?id=1886540
Thanks for reminding me about that. Although that patch helps, I don't
think it's a complete fix. I installed the attached instead; please give
these patches a try. The first one's more important.
It seems odd that we must use mknodat here. Why can't we create the file
using openat with O_CREAT|O_TRUNC|O_EXCL instead? That would be simpler.
Is it some weird limitation of xattr? Perhaps should be a comment if so.
From 0b74885e81b90d6ab4890b195dce99ca9109fe59 Mon Sep 17 00:00:00 2001
From: Paul Eggert <egg...@cs.ucla.edu>
Date: Fri, 26 Aug 2022 15:23:23 -0500
Subject: [PATCH 1/2] Fix bug with -x --xattr read-only files
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Problem reported by Kevin Raymond in:
https://bugzilla.redhat.com/show_bug.cgi?id=1886540
* src/extract.c (open_output_file): If we already created the
empty file, do not open with O_EXCL, or with O_CREAT or O_TRUNC
for that matter. Instead, use only O_NOFOLLOW to avoid some
races. When estimating current mode, use openflag & O_EXCL rather
than overwriting_old_files.
(extract_file): Also invert S_IWUSR if it’s not set.
* tests/xattr08.at: New test.
* tests/Makefile.am, tests/testsuite.at: Add it.
---
src/extract.c | 31 +++++++++++++++----------------
tests/Makefile.am | 1 +
tests/testsuite.at | 1 +
tests/xattr08.at | 41 +++++++++++++++++++++++++++++++++++++++++
4 files changed, 58 insertions(+), 16 deletions(-)
create mode 100644 tests/xattr08.at
diff --git a/src/extract.c b/src/extract.c
index 2813c961..c1f5731b 100644
--- a/src/extract.c
+++ b/src/extract.c
@@ -1189,14 +1189,12 @@ open_output_file (char const *file_name, int typeflag, mode_t mode,
int fd;
bool overwriting_old_files = old_files_option == OVERWRITE_OLD_FILES;
int openflag = (O_WRONLY | O_BINARY | O_CLOEXEC | O_NOCTTY | O_NONBLOCK
- | O_CREAT
- | (overwriting_old_files
- ? O_TRUNC | (dereference_option ? 0 : O_NOFOLLOW)
- : O_EXCL));
-
- /* File might be created in set_xattr. So clear O_EXCL to avoid open() fail */
- if (file_created)
- openflag = openflag & ~O_EXCL;
+ | (file_created
+ ? O_NOFOLLOW
+ : (O_CREAT
+ | (overwriting_old_files
+ ? O_TRUNC | (dereference_option ? 0 : O_NOFOLLOW)
+ : O_EXCL))));
if (typeflag == CONTTYPE)
{
@@ -1227,7 +1225,12 @@ open_output_file (char const *file_name, int typeflag, mode_t mode,
fd = openat (chdir_fd, file_name, openflag, mode);
if (0 <= fd)
{
- if (overwriting_old_files)
+ if (openflag & O_EXCL)
+ {
+ *current_mode = mode & ~ current_umask;
+ *current_mode_mask = MODE_RWX;
+ }
+ else
{
struct stat st;
if (fstat (fd, &st) != 0)
@@ -1246,11 +1249,6 @@ open_output_file (char const *file_name, int typeflag, mode_t mode,
*current_mode = st.st_mode;
*current_mode_mask = ALL_MODE_BITS;
}
- else
- {
- *current_mode = mode & ~ current_umask;
- *current_mode_mask = MODE_RWX;
- }
}
return fd;
@@ -1268,8 +1266,9 @@ extract_file (char *file_name, int typeflag)
bool interdir_made = false;
mode_t mode = (current_stat_info.stat.st_mode & MODE_RWX
& ~ (0 < same_owner_option ? S_IRWXG | S_IRWXO : 0));
- mode_t invert_permissions = 0 < same_owner_option ? mode & (S_IRWXG | S_IRWXO)
- : 0;
+ mode_t invert_permissions
+ = ((0 < same_owner_option ? mode & (S_IRWXG | S_IRWXO) : 0)
+ | ((mode & S_IWUSR) ^ S_IWUSR));
mode_t current_mode = 0;
mode_t current_mode_mask = 0;
diff --git a/tests/Makefile.am b/tests/Makefile.am
index eff8a3bf..a0ce690b 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -275,6 +275,7 @@ TESTSUITE_AT = \
xattr05.at\
xattr06.at\
xattr07.at\
+ xattr08.at\
acls01.at\
acls02.at\
acls03.at\
diff --git a/tests/testsuite.at b/tests/testsuite.at
index 0769e71b..a99cdeee 100644
--- a/tests/testsuite.at
+++ b/tests/testsuite.at
@@ -502,6 +502,7 @@ m4_include([xattr04.at])
m4_include([xattr05.at])
m4_include([xattr06.at])
m4_include([xattr07.at])
+m4_include([xattr08.at])
m4_include([acls01.at])
m4_include([acls02.at])
diff --git a/tests/xattr08.at b/tests/xattr08.at
new file mode 100644
index 00000000..2beef235
--- /dev/null
+++ b/tests/xattr08.at
@@ -0,0 +1,41 @@
+# Process this file with autom4te to create testsuite. -*- Autotest -*-
+#
+# Test suite for GNU tar.
+# Copyright 2022 Free Software Foundation, Inc.
+
+# This file is part of GNU tar.
+
+# GNU tar is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+
+# GNU tar is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+# Test description:
+# Test read-only files can be extracted with --xattr.
+# Per report:
+# https://lists.gnu.org/r/bug-tar/2020-10/msg00001.html
+
+AT_SETUP([xattrs: xattrs and read-only files])
+AT_KEYWORDS([xattrs xattr08])
+
+AT_TAR_CHECK([
+AT_XATTRS_PREREQ
+mkdir dir dir2
+genfile --file dir/file
+
+setfattr -n user.test -v OurDirValue dir/file
+chmod a-w dir/file
+
+tar --xattrs -C dir -cf archive.tar file
+tar --xattrs -C dir2 -xf archive.tar
+])
+
+AT_CLEANUP
--
2.37.2
From 35d9845d5d410237e418113ec79d8f9e4b0b2bb8 Mon Sep 17 00:00:00 2001
From: Paul Eggert <egg...@cs.ucla.edu>
Date: Fri, 26 Aug 2022 16:38:29 -0500
Subject: [PATCH 2/2] Do not diagnose same xattr file twice
* src/extract.c (set_xattr): Simplify, by having it do only
the mknodat and xattrs_xattrs_set, rather than also
trying to recover from failure. Caller simplified too.
* tests/xattr07.at (xattrs: xattrs and --skip-old-files):
Adjust test to match fixed behavior.
---
src/extract.c | 59 ++++++++++++++----------------------------------
tests/xattr07.at | 2 --
2 files changed, 17 insertions(+), 44 deletions(-)
diff --git a/src/extract.c b/src/extract.c
index c1f5731b..7696d5e4 100644
--- a/src/extract.c
+++ b/src/extract.c
@@ -903,42 +903,21 @@ maybe_recoverable (char *file_name, bool regular, bool *interdir_made)
in advance dramatically improves the following performance of reading and
writing a file). If not restoring permissions, invert the INVERT_PERMISSIONS
bits from the file's current permissions. TYPEFLAG specifies the type of the
- file. Returns non-zero when error occurs (while un-available xattrs is not
- an error, rather no-op). Non-zero FILE_CREATED indicates set_xattr has
- created the file. */
+ file. Return a negative number (setting errno) on failure, zero if
+ successful but FILE_NAME was not created (e.g., xattrs not
+ available), and a positive number if FILE_NAME was created. */
static int
set_xattr (char const *file_name, struct tar_stat_info const *st,
- mode_t invert_permissions, char typeflag, int *file_created)
+ mode_t mode, char typeflag)
{
#ifdef HAVE_XATTRS
- bool interdir_made = false;
-
if ((xattrs_option > 0) && st->xattr_map.xm_size)
{
- mode_t mode = current_stat_info.stat.st_mode & MODE_RWX & ~ current_umask;
-
- for (;;)
- {
- if (!mknodat (chdir_fd, file_name, mode ^ invert_permissions, 0))
- {
- /* Successfully created file */
- xattrs_xattrs_set (st, file_name, typeflag, 0);
- *file_created = 1;
- return 0;
- }
-
- switch (maybe_recoverable ((char *)file_name, false, &interdir_made))
- {
- case RECOVER_OK:
- continue;
- case RECOVER_NO:
- skip_member ();
- open_error (file_name);
- return 1;
- case RECOVER_SKIP:
- return 0;
- }
- }
+ int r = mknodat (chdir_fd, file_name, mode, 0);
+ if (r < 0)
+ return r;
+ xattrs_xattrs_set (st, file_name, typeflag, 0);
+ return 1;
}
#endif
@@ -1266,9 +1245,6 @@ extract_file (char *file_name, int typeflag)
bool interdir_made = false;
mode_t mode = (current_stat_info.stat.st_mode & MODE_RWX
& ~ (0 < same_owner_option ? S_IRWXG | S_IRWXO : 0));
- mode_t invert_permissions
- = ((0 < same_owner_option ? mode & (S_IRWXG | S_IRWXO) : 0)
- | ((mode & S_IWUSR) ^ S_IWUSR));
mode_t current_mode = 0;
mode_t current_mode_mask = 0;
@@ -1285,15 +1261,14 @@ extract_file (char *file_name, int typeflag)
}
else
{
- int file_created = 0;
- if (set_xattr (file_name, ¤t_stat_info, invert_permissions,
- typeflag, &file_created))
- return 1;
-
- while ((fd = open_output_file (file_name, typeflag, mode,
- file_created, ¤t_mode,
- ¤t_mode_mask))
- < 0)
+ int file_created;
+ while (((file_created = set_xattr (file_name, ¤t_stat_info,
+ mode | S_IWUSR, typeflag))
+ < 0)
+ || ((fd = open_output_file (file_name, typeflag, mode,
+ file_created, ¤t_mode,
+ ¤t_mode_mask))
+ < 0))
{
int recover = maybe_recoverable (file_name, true, &interdir_made);
if (recover != RECOVER_OK)
diff --git a/tests/xattr07.at b/tests/xattr07.at
index dcc3aabe..fa6797d8 100644
--- a/tests/xattr07.at
+++ b/tests/xattr07.at
@@ -62,8 +62,6 @@ user.test="OurFileValue2"
user.test="OurFileValue2"
], [tar: dir: skipping existing file
tar: dir/file: skipping existing file
-tar: dir/file: skipping existing file
-tar: dir/file2: skipping existing file
tar: dir/file2: skipping existing file
tar: dir/file: Cannot open: File exists
tar: dir/file2: Cannot open: File exists
--
2.37.2
