Re: [PATCH] Don't pre-create read-only files with --extract --xattrs
Applied (commit e7987b72). Thank you. Regards, Sergey
Re: [PATCH] Don't pre-create read-only files with --extract --xattrs
Thank you for the fix in 35d9845d5d41023 and 0b74885e81b90d6ab4. The
patches simplify the code a lot. I just think two in-code doc fixes are
worth it, patch attached. Per https://github.com/praiskup/tar/pull/2
Pavel
On středa 24. listopadu 2021 15:56:05 CEST Pavel Raiskup wrote:
> Gently pinging on this again. This causes invalid extraction failures.
> I'm ready for the review cycles (patch attached :-)!
>
> Pavel
>
> On Monday, February 15, 2021 10:00:20 AM CET Pavel Raiskup wrote:
> > Ping, I can see there's 1.34 release now, thank you!
> >
> > But the git isn't updated yet so I'm not sure this has been fixed or not.
> >
> > Pavel
> >
> > On Tuesday, February 9, 2021 5:06:31 PM CET Pavel Raiskup wrote:
> > > Gently ping on this. This is easily reproducible, so I am re-attaching
> > > the patch including a new test-case that fails with the current code:
> > >
> > > $ tar --xattrs -xf tar
> > > tar: setxattrat: Cannot set 'user.attr' extended attribute for file
> > > 'file': Permission denied
> > > tar: file: Cannot open: Permission denied
> > > tar: Exiting with failure status due to previous errors
> > >
> > > Pavel
> > >
> > > On Friday, October 9, 2020 1:39:11 PM CET Pavel Raiskup wrote:
> > > > We used to respect the target file mode when pre-creating files in
> > > > set_xattr, so we also pre-created read-only files that we were not able
> > > > to open later for writing. This is now fixed, and we always create the
> > > > file with S_IWUSR.
> > > >
> > > > Fixes the original bug report https://bugzilla.redhat.com/1886540
> > > >
> > > > * src/extract.c (set_xattr): Blindly add S_IWUSR flag to pre-created
> > > > files, to avoid openat failures later.
> > > > ---
> > > > src/extract.c | 8 +++-
> > > > 1 file changed, 7 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/src/extract.c b/src/extract.c
> > > > index b73a591..2e650ad 100644
> > > > --- a/src/extract.c
> > > > +++ b/src/extract.c
> > > > @@ -865,7 +865,13 @@ set_xattr (char const *file_name, struct
> > > > tar_stat_info const *st,
> > > >
> > > >for (;;)
> > > > {
> > > > - if (!mknodat (chdir_fd, file_name, mode ^
> > > > invert_permissions, 0))
> > > > + /* We'll open the file with O_WRONLY later by
> > > > open_output_file,
> > > > + therefore we need to give us the S_IWUSR bit. If the
> > > > file was
> > > > + meant to be user-read-only, the permissions will be
> > > > corrected by
> > > > + the set_stat call. */
> > > > + mode_t initial_mode = mode ^ invert_permissions | S_IWUSR;
> > > > +
> > > > + if (!mknodat (chdir_fd, file_name, initial_mode, 0))
> > > > {
> > > >/* Successfully created file */
> > > >xattrs_xattrs_set (st, file_name, typeflag, 0);
> > > >
> > >
> > >
> >
> >
> >
> >
> >
>
>
>From 4fc376ec43c8a235306ef719db3e157a67969598 Mon Sep 17 00:00:00 2001
From: Pavel Raiskup
Date: Thu, 18 May 2023 14:30:08 +0200
Subject: [PATCH] Comment a bit on the xattr extraction logic
* src/extract.c (extract_file): Document why we pre-create with S_IWUSR.
(set_xattr): Drop the INVERT_PERMISSIONS doc leftover.
---
src/extract.c | 13 -
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/src/extract.c b/src/extract.c
index aec5de69..7adc7aff 100644
--- a/src/extract.c
+++ b/src/extract.c
@@ -902,11 +902,10 @@ maybe_recoverable (char *file_name, bool regular, bool *interdir_made)
(e.g. on Lustre distributed parallel filesystem - setting info about how many
servers is this file striped over, stripe size, mirror copies, etc.
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. 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. */
+ writing a file). TYPEFLAG specifies the type of the 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 mode, char typeflag)
@@ -1271,6 +1270,10 @@ extract_file (char *file_name, int typeflag)
else
{
int file_created;
+ /* Either we pre-create the file in set_xattr(), or we just directly open
+ the file in open_output_file() with O_CREAT. If pre-creating, we need
+ to use S_IWUSR so we can open the file O_WRONLY in open_output_file().
+ The additional mode bit is cleared later by set_stat()->set_mode(). */
while (((file_created = set_xattr
Re: [PATCH] Don't pre-create read-only files with --extract --xattrs
Gently pinging on this again. This causes invalid extraction failures.
I'm ready for the review cycles (patch attached :-)!
Pavel
On Monday, February 15, 2021 10:00:20 AM CET Pavel Raiskup wrote:
> Ping, I can see there's 1.34 release now, thank you!
>
> But the git isn't updated yet so I'm not sure this has been fixed or not.
>
> Pavel
>
> On Tuesday, February 9, 2021 5:06:31 PM CET Pavel Raiskup wrote:
> > Gently ping on this. This is easily reproducible, so I am re-attaching
> > the patch including a new test-case that fails with the current code:
> >
> > $ tar --xattrs -xf tar
> > tar: setxattrat: Cannot set 'user.attr' extended attribute for file
> > 'file': Permission denied
> > tar: file: Cannot open: Permission denied
> > tar: Exiting with failure status due to previous errors
> >
> > Pavel
> >
> > On Friday, October 9, 2020 1:39:11 PM CET Pavel Raiskup wrote:
> > > We used to respect the target file mode when pre-creating files in
> > > set_xattr, so we also pre-created read-only files that we were not able
> > > to open later for writing. This is now fixed, and we always create the
> > > file with S_IWUSR.
> > >
> > > Fixes the original bug report https://bugzilla.redhat.com/1886540
> > >
> > > * src/extract.c (set_xattr): Blindly add S_IWUSR flag to pre-created
> > > files, to avoid openat failures later.
> > > ---
> > > src/extract.c | 8 +++-
> > > 1 file changed, 7 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/extract.c b/src/extract.c
> > > index b73a591..2e650ad 100644
> > > --- a/src/extract.c
> > > +++ b/src/extract.c
> > > @@ -865,7 +865,13 @@ set_xattr (char const *file_name, struct
> > > tar_stat_info const *st,
> > >
> > >for (;;)
> > > {
> > > - if (!mknodat (chdir_fd, file_name, mode ^ invert_permissions,
> > > 0))
> > > + /* We'll open the file with O_WRONLY later by open_output_file,
> > > + therefore we need to give us the S_IWUSR bit. If the file
> > > was
> > > + meant to be user-read-only, the permissions will be
> > > corrected by
> > > + the set_stat call. */
> > > + mode_t initial_mode = mode ^ invert_permissions | S_IWUSR;
> > > +
> > > + if (!mknodat (chdir_fd, file_name, initial_mode, 0))
> > > {
> > >/* Successfully created file */
> > >xattrs_xattrs_set (st, file_name, typeflag, 0);
> > >
> >
> >
>
>
>
>
>
>From 5f9283cd00273115097920da7ece8e97ccf1902b Mon Sep 17 00:00:00 2001
From: Pavel Raiskup
Date: Fri, 9 Oct 2020 12:03:22 +0200
Subject: [PATCH] Don't pre-create read-only files with --extract --xattrs
We used to respect the target file mode when pre-creating files in
set_xattr, so we also pre-created read-only files that we were not able
to open later for writing. This is now fixed, and we always create the
file with S_IWUSR.
Fixes the original bug report https://bugzilla.redhat.com/1886540
* src/extract.c (set_xattr): Blindly add S_IWUSR flag to pre-created
files, to avoid openat failures later.
* tests/xattr08.at: New test-case file.
---
src/extract.c| 8 +++-
tests/xattr08.at | 45 +
2 files changed, 52 insertions(+), 1 deletion(-)
create mode 100644 tests/xattr08.at
diff --git a/src/extract.c b/src/extract.c
index 850a08a2..1bfe36a6 100644
--- a/src/extract.c
+++ b/src/extract.c
@@ -860,7 +860,13 @@ set_xattr (char const *file_name, struct tar_stat_info const *st,
for (;;)
{
- if (!mknodat (chdir_fd, file_name, mode ^ invert_permissions, 0))
+ /* We'll open the file with O_WRONLY later by open_output_file,
+ therefore we need to give us the S_IWUSR bit. If the file was
+ meant to be user-read-only, the permissions will be corrected by
+ the set_stat call. */
+ mode_t initial_mode = mode ^ invert_permissions | S_IWUSR;
+
+ if (!mknodat (chdir_fd, file_name, initial_mode, 0))
{
/* Successfully created file */
xattrs_xattrs_set (st, file_name, typeflag, 0);
diff --git a/tests/xattr08.at b/tests/xattr08.at
new file mode 100644
index ..0dd3ed34
--- /dev/null
+++ b/tests/xattr08.at
@@ -0,0 +1,45 @@
+# Process this file with autom4te to create testsuite. -*- Autotest -*-
+#
+# Test suite for GNU tar.
+# Copyright 2021 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
Re: [PATCH] Don't pre-create read-only files with --extract --xattrs
Ping, I can see there's 1.34 release now, thank you!
But the git isn't updated yet so I'm not sure this has been fixed or not.
Pavel
On Tuesday, February 9, 2021 5:06:31 PM CET Pavel Raiskup wrote:
> Gently ping on this. This is easily reproducible, so I am re-attaching
> the patch including a new test-case that fails with the current code:
>
> $ tar --xattrs -xf tar
> tar: setxattrat: Cannot set 'user.attr' extended attribute for file
> 'file': Permission denied
> tar: file: Cannot open: Permission denied
> tar: Exiting with failure status due to previous errors
>
> Pavel
>
> On Friday, October 9, 2020 1:39:11 PM CET Pavel Raiskup wrote:
> > We used to respect the target file mode when pre-creating files in
> > set_xattr, so we also pre-created read-only files that we were not able
> > to open later for writing. This is now fixed, and we always create the
> > file with S_IWUSR.
> >
> > Fixes the original bug report https://bugzilla.redhat.com/1886540
> >
> > * src/extract.c (set_xattr): Blindly add S_IWUSR flag to pre-created
> > files, to avoid openat failures later.
> > ---
> > src/extract.c | 8 +++-
> > 1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/extract.c b/src/extract.c
> > index b73a591..2e650ad 100644
> > --- a/src/extract.c
> > +++ b/src/extract.c
> > @@ -865,7 +865,13 @@ set_xattr (char const *file_name, struct tar_stat_info
> > const *st,
> >
> >for (;;)
> > {
> > - if (!mknodat (chdir_fd, file_name, mode ^ invert_permissions, 0))
> > + /* We'll open the file with O_WRONLY later by open_output_file,
> > + therefore we need to give us the S_IWUSR bit. If the file was
> > + meant to be user-read-only, the permissions will be corrected
> > by
> > + the set_stat call. */
> > + mode_t initial_mode = mode ^ invert_permissions | S_IWUSR;
> > +
> > + if (!mknodat (chdir_fd, file_name, initial_mode, 0))
> > {
> >/* Successfully created file */
> >xattrs_xattrs_set (st, file_name, typeflag, 0);
> >
>
>
Re: [PATCH] Don't pre-create read-only files with --extract --xattrs
Gently ping on this. This is easily reproducible, so I am re-attaching
the patch including a new test-case that fails with the current code:
$ tar --xattrs -xf tar
tar: setxattrat: Cannot set 'user.attr' extended attribute for file 'file':
Permission denied
tar: file: Cannot open: Permission denied
tar: Exiting with failure status due to previous errors
Pavel
On Friday, October 9, 2020 1:39:11 PM CET Pavel Raiskup wrote:
> We used to respect the target file mode when pre-creating files in
> set_xattr, so we also pre-created read-only files that we were not able
> to open later for writing. This is now fixed, and we always create the
> file with S_IWUSR.
>
> Fixes the original bug report https://bugzilla.redhat.com/1886540
>
> * src/extract.c (set_xattr): Blindly add S_IWUSR flag to pre-created
> files, to avoid openat failures later.
> ---
> src/extract.c | 8 +++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/src/extract.c b/src/extract.c
> index b73a591..2e650ad 100644
> --- a/src/extract.c
> +++ b/src/extract.c
> @@ -865,7 +865,13 @@ set_xattr (char const *file_name, struct tar_stat_info
> const *st,
>
>for (;;)
> {
> - if (!mknodat (chdir_fd, file_name, mode ^ invert_permissions, 0))
> + /* We'll open the file with O_WRONLY later by open_output_file,
> + therefore we need to give us the S_IWUSR bit. If the file was
> + meant to be user-read-only, the permissions will be corrected by
> + the set_stat call. */
> + mode_t initial_mode = mode ^ invert_permissions | S_IWUSR;
> +
> + if (!mknodat (chdir_fd, file_name, initial_mode, 0))
> {
>/* Successfully created file */
>xattrs_xattrs_set (st, file_name, typeflag, 0);
>
>From 42201a0ce560846f770cd5305d04287b13e4bd2b Mon Sep 17 00:00:00 2001
From: Pavel Raiskup
Date: Fri, 9 Oct 2020 12:03:22 +0200
Subject: [PATCH] Don't pre-create read-only files with --extract --xattrs
We used to respect the target file mode when pre-creating files in
set_xattr, so we also pre-created read-only files that we were not able
to open later for writing. This is now fixed, and we always create the
file with S_IWUSR.
Fixes the original bug report https://bugzilla.redhat.com/1886540
* src/extract.c (set_xattr): Blindly add S_IWUSR flag to pre-created
files, to avoid openat failures later.
* tests/xattr08.at: New test-case file.
---
src/extract.c| 8 +++-
tests/xattr08.at | 45 +
2 files changed, 52 insertions(+), 1 deletion(-)
create mode 100644 tests/xattr08.at
diff --git a/src/extract.c b/src/extract.c
index 80009a54..358603fe 100644
--- a/src/extract.c
+++ b/src/extract.c
@@ -865,7 +865,13 @@ set_xattr (char const *file_name, struct tar_stat_info const *st,
for (;;)
{
- if (!mknodat (chdir_fd, file_name, mode ^ invert_permissions, 0))
+ /* We'll open the file with O_WRONLY later by open_output_file,
+ therefore we need to give us the S_IWUSR bit. If the file was
+ meant to be user-read-only, the permissions will be corrected by
+ the set_stat call. */
+ mode_t initial_mode = mode ^ invert_permissions | S_IWUSR;
+
+ if (!mknodat (chdir_fd, file_name, initial_mode, 0))
{
/* Successfully created file */
xattrs_xattrs_set (st, file_name, typeflag, 0);
diff --git a/tests/xattr08.at b/tests/xattr08.at
new file mode 100644
index ..0dd3ed34
--- /dev/null
+++ b/tests/xattr08.at
@@ -0,0 +1,45 @@
+# Process this file with autom4te to create testsuite. -*- Autotest -*-
+#
+# Test suite for GNU tar.
+# Copyright 2021 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
