Hello Nate, sorry that I forgot to follow up - the approach I proposed was accepted in July: https://cgit.git.savannah.gnu.org/cgit/tar.git/commit/?id=4e742fc8674064a9fa00d4483d06aca48d5b0463 (unfortunately the commit log credits only me and not you). Thank you for finding and reporting the problem.
Regards, Pavel On Fri, Jan 17, 2025 at 04:35:34PM -0600, Nate Simon wrote: > I would agree. I considered sharing a similar patch, but decided against it > only because it seemed improper to delete code that I didn't understand the > original purpose of. > > So if consensus is that the existing code needlessly complicates the way > this argument is handled, I'd gladly withdraw my patch in favor of Pavel's. > > Nate > > > On 1/16/25 15:57, Pavel Cahyna wrote: > > [ Please Cc me on replies, I am not subscribed to bug-tar@ ] > > > > Hello Nate, > > > > thank you for the observation and patch. To me the new behavior (introduced > > by > > http://git.savannah.gnu.org/cgit/tar.git/commit/?id=14d8fc718f0 > > in response to > > https://www.mail-archive.com/[email protected]/msg05838.html > > ) looks incorrect as well. But see below: > > > > On Tue, Nov 26, 2024 at 02:50:26PM -0600, Nate Simon wrote: > > > I went ahead and tested a patch for this issue. This is my first > > > contribution, so I hope it is formatted acceptably. > > > > > > When looking at the extractor source, I wasn't sure about this section > > > which > > > switches a target directory to a "safe" permission mode then changes it > > > back > > > afterward. I'm not sure this logic follows the documented behavior of > > > `--no-overwrite-dir` since it still updates the "Change"/"Modify" metadata > > > fields. It seems to me if a target directory is not traversable, > > > extraction > > > with no-overwrite-dir should fail where it currently does not. > > > Nonetheless, > > > I'm sure there's a reason I'm simply not aware of, so this patch does > > > preserve the existing behavior. > > > > I would like to know the reason, because when extracting to an existing > > directory that is not writable, tar does not make it writable, instead > > it fails: > > > > $ chmod a-w . > > $ tar -x -f ../test.tar > > tar: test: Cannot mkdir: Permission denied > > tar: Exiting with failure status due to previous errors > > > > so the user is responsible for having the permissions or appropriate > > privileges to write to the directory, and it does not seem to have been > > a problem, so why should be existing subdirectories given the > > --no-overwrite-dir option behave differently? I feel the code is a bit > > too smart. > > > > Therefore I would like to propose an alternative patch that simply > > removes the code to change the directory permissions - attached. WDYT? > > > > (Of course the associated test would need changing as well.) > > > > Best regards, Pavel > > > > > Thus my addition checks the euid to see if the current user is able to > > > perform this permissions swap; and if the change would fail, do nothing > > > instead. This is preferred for cases where users can have absolute paths > > > to > > > user-specific files/folders. For example, extracting a tar file containing > > > './home/nate/myfile', using the user 'nate' fails in the current tar > > > version, but passes with this patch. > > > > > > Nate Simon > > > > > > > > > On 11/11/24 08:43, Nate Simon wrote: > > > > Tar appears to be not preserving all metadata when --no-overwrite-dir is > > > > used for file extraction. I started seeing this regression when we > > > > updated from Ubuntu 20 to Ubuntu 22: tar 1.30 -> 1.34. > > > > > > > > This is the example that made me aware of the issue. These commands pass > > > > with tar 1.30. > > > > > > > > $ mkdir usr > > > > $ tar -cf test.tar usr > > > > $ tar --no-overwrite-dir -xf test.tar -C / > > > > tar: usr: Cannot change mode to rwxr-xr-x: Operation not permitted > > > > tar: Exiting with failure status due to previous errors > > > > > > > > > > > > And here is a similar example with folder metadata. I observed that even > > > > with '--no-overwrite-dir' passed to tar, the folder's "modify" metadata > > > > is rolled back to match the folder contained in the tarball. But based > > > > on the documentation, I expected neither Modify nor Change fields to > > > > update. > > > > > > > > $ mkdir test > > > > $ tar -cf test2.tar test > > > > $ rmdir test > > > > $ mkdir test > > > > $ stat test > > > > File: test > > > > Size: 4096 Blocks: 8 IO Block: 4096 directory > > > > Device: 259,2 Inode: 21561426 Links: 2 > > > > Access: (0755/drwxr-xr-x) Uid: ( 1000/ nate) Gid: ( 1000/ nate) > > > > Access: 2024-11-11 08:26:02.696280292 -0600 > > > > Modify: 2024-11-11 08:26:02.696280292 -0600 > > > > Change: 2024-11-11 08:26:02.696280292 -0600 > > > > Birth: 2024-11-11 08:26:02.696280292 -0600 > > > > > > > > $ tar --no-overwrite-dir -xf test2.tar > > > > $ stat test > > > > File: test > > > > Size: 4096 Blocks: 8 IO Block: 4096 directory > > > > Device: 259,2 Inode: 21561426 Links: 2 > > > > Access: (0755/drwxr-xr-x) Uid: ( 1000/ nate) Gid: ( 1000/ nate) > > > > Access: 2024-11-11 08:26:02.696280292 -0600 > > > > Modify: 2024-11-11 08:23:40.000000000 -0600 > > > > Change: 2024-11-11 08:26:17.222659660 -0600 > > > > Birth: 2024-11-11 08:26:02.696280292 -0600 > > > > > diff --git a/src/extract.c b/src/extract.c > > > index f741943f..d3205766 100644 > > > --- a/src/extract.c > > > +++ b/src/extract.c > > > @@ -1134,25 +1134,31 @@ extract_dir (char *file_name, char typeflag) > > > /* Temporarily change the directory mode to a > > > safe > > > value, to be able to create files in it, > > > should > > > the need be. > > > + If extracting as non-root user without permission to > > > + the target folder, the user assumes responsibility for > > > + ensuring workable permissions. > > > */ > > > - 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); > > > - } > > > + if (we_are_root || st.st_uid == geteuid ()) > > > + { > > > + 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; > > > } > > >
