Hello tar maintainers,

the patch below is an attempt to fix issues reported in the thread
"no-overwrite-dir does not preserve metadata of existing directories"
( https://www.mail-archive.com/bug-tar@gnu.org/msg06431.html ). One
issue is the non-preservation of some metadata, other issue is the
inability to extract an archive which contains an empty directory that
already exists and is not writable and we are not allowed to make it
writable. This used to succeed, as we are not supposed to overwrite
the directory, and we don't need to, as we don't need to create any
files in it - but since 14d8fc718f0c872274b90991ee634b0cd8e1a6f0 it
has been failing. In addition to the cases demonstrated in the thread,
I found another reproducer (/mnt/qa is a filesystem mounted read-only
and it contains var/):
# cd
# mkdir -p mnt/qa/var
# tar cf qa.tar mnt
# cd /
# tar -x --no-overwrite-dir -f ~/qa.tar
tar errors out with:
tar: mnt/qa: Cannot change mode to rwx------: Read-only file system
tar: mnt/qa/var: Cannot change mode to rwx------: Read-only file system
tar: Exiting with failure status due to previous errors

The patch removes the ability to extract with --no-overwrite-dir when
there are files in the directory to be extracted and the existing
directory is not writable. I think this is acceptable, as the original
bug report that led to the change was about something else and the fix
for that issue ("Option "--no-overwrite-dir" does not work for empty
directories") remains. See the thread for more details, discussion,
and alternative approaches.

The patch adds only a test for the changing of directory permission
bits issue. I suppose that testing the issue with directories not
being writable could be possible with AT_PRIVILEGED_PREREQ.

Best regards, Pavel

-- >8 --
From: Pavel Cahyna <pcah...@redhat.com>
Subject: [PATCH 1/1] Don't set directory mode with --no-overwrite-dir

Since 14d8fc718f0c872274b90991ee634b0cd8e1a6f0, with --no-overwrite-dir,
tar attempted to temporarily change the existing target directory mode
to be able to create files in it, and then restore it back in order to
preserve the directory metadata.

This approach created several problems:
- The directory metadata were not restored accurately. The operation led
  to a change of mtime and ctime, and sometimes to a loss of some mode
  bits (they got masked by the current umask).
- If it was not possible to change the target directory mode, tar
  errored out, even if the directory being extracted was empty and there
  was actually no need to create files in it.

Fix by removing code that makes the existing target directory writable
with --no-overwrite-dir.

This reverts part of commit 14d8fc718f0c872274b90991ee634b0cd8e1a6f0
"Fix the --no-overwrite-dir option", but the fix for the original
problem ("Option "--no-overwrite-dir" does not work for empty
directories") is preserved.

As a result, extracting a directory with files when the directory
already exists and is not writable now fails with --no-overwrite-dir
(just as it did before 14d8fc718f0c872274b90991ee634b0cd8e1a6f0).
Ensuring that the target directory is writable is now a responsibility
of the user.

See the thread "no-overwrite-dir does not preserve metadata of existing
directories"
(https://lists.gnu.org/archive/html/bug-tar/2024-11/msg00006.html)
for the original bug report, discussion and details.

* src/extract.c (extract_dir): Remove setting and restoring of directory
permissions in --no-overwrite-dir mode.
* tests/extrac23.at: Remove the test for the now-unsupported case, add a
a test case for the problem with loss of mode bits.
---
 src/extract.c     | 25 -------------------------
 tests/extrac23.at | 30 ++++++++++++++++--------------
 2 files changed, 16 insertions(+), 39 deletions(-)

diff --git a/src/extract.c b/src/extract.c
index 5d89458a..0e92abe0 100644
--- a/src/extract.c
+++ b/src/extract.c
@@ -1158,31 +1158,6 @@ extract_dir (char *file_name, char typeflag)
                          repair_delayed_set_stat (file_name, &st);
                          return true;
                        }
-                     else if (old_files_option == NO_OVERWRITE_DIR_OLD_FILES)
-                       {
-                         /* Temporarily change the directory mode to a safe
-                            value, to be able to create files in it, should
-                            the need be.
-                         */
-                         mode = safe_dir_mode (&st);
-                         status = fd_chmod (-1, file_name, mode,
-                                            AT_SYMLINK_NOFOLLOW, DIRTYPE);
-                         if (status == 0)
-                           {
-                             /* Store the actual directory mode, to be restored
-                                later.
-                             */
-                             current_stat_info.stat = st;
-                             current_mode = mode & ~ current_umask;
-                             current_mode_mask = MODE_RWX;
-                             atflag = AT_SYMLINK_NOFOLLOW;
-                             break;
-                           }
-                         else
-                           {
-                             chmod_error_details (file_name, mode);
-                           }
-                       }
                      break;
                    }
                }
diff --git a/tests/extrac23.at b/tests/extrac23.at
index c8943a73..1f9ea2fc 100644
--- a/tests/extrac23.at
+++ b/tests/extrac23.at
@@ -21,16 +21,23 @@ AT_KEYWORDS([extract extrac23 no-overwrite-dir])
 # Description: Implementation of the --no-overwrite-dir option was flawed in
 # tar versions up to 1.32.90.  This option is intended to preserve metadata
 # of existing directories.  In fact it worked only for non-empty directories.
-# Moreover, if the actual directory was owned by the user tar runs as and the
-# S_IWUSR bit was not set in its actual permissions, tar failed to create files
-# in it.
+# Moreover, the first fix caused some permissions (depending on the current
+# umask) and modification times to get overwritten when the directory had not
+# the S_IWUSR bit set in its actual permissions, and also caused failure
+# to extract an empty directory if the already existing directory had not
+# the S_IWUSR bit set in its actual permissions and changing the permissions
+# was not possible.
 #
 # Reported by: Michael Kaufmann <m...@michael-kaufmann.ch>
 # References: 
<20200207112934.horde.anxzyhaj2chiwurw5cut...@webmail.michael-kaufmann.ch>,
 #   https://lists.gnu.org/archive/html/bug-tar/2020-02/msg00003.html
+#
+# Reported by: Nate Simon <njsimo...@gmail.com>
+# References: <e09b5362-548a-40b6-beec-99add9a88...@gmail.com>,
+#   https://lists.gnu.org/archive/html/bug-tar/2024-11/msg00006.html
 
 AT_TAR_CHECK([
-# Test if the directory permissions are restored properly.
+# Test if the directory permissions are preserved properly.
 mkdir dir
 chmod 755 dir
 tar cf a.tar dir
@@ -38,21 +45,16 @@ chmod 777 dir
 tar -xf a.tar --no-overwrite-dir
 genfile --stat=mode.777 dir
 
-# Test if temporary permissions are set correctly to allow the owner
-# to write to the directory.
-genfile --file dir/file
+# Test if the directory permissions are preserved even if it is not writable.
+chmod 755 dir
 tar cf a.tar dir
-rm dir/file
-chmod 400 dir
+chmod 444 dir
+umask 066
 tar -xf a.tar --no-overwrite-dir
 genfile --stat=mode.777 dir
-chmod 700 dir
-find dir
 ],
 [0],
 [777
-400
-dir
-dir/file
+444
 ])
 AT_CLEANUP
-- 
2.39.5


Reply via email to