On Fri Nov 20, 2015 at 09:59:00AM +0000, Dimitris Papastamos wrote: > Thanks for these patches, I've applied them. >
Shame on me, I have introduced a very stupid bug. I neglected to include a commit in the patches I sent and didn't notice; all was well for me, but I subtly broke tar. I only noticed after use of upstream with these patches for a while. chmod is being performed on HARDLINK type entities, but without the missing change, mode is uninitialized for that type leading to an unpredictable mode being set on those links. Any hard links and their targets may be affected. What I'd done is drop reading mode in each entity type, and read it unconditionally like mtime. An alternative would be to skip the mtime/chown/chmod altogether for hard links. It shouldn't really be necessary, though I think in the case of different permissions on a hard link to an earlier file (maybe a tar which has been added to), later mode should override any previous. Attached are patches for either fix. I prefer the first, I think it cleans things up a bit. Sorry for my mistake. I'm also including a patch that is a very slight clarity improvement over the earlier creation mode fix, if it's worth changing again. A new file's mode is available at creation time, so there's no reason not to use it directly (similar to what all the other types do). Hope this is useful, -Brad
>From cc9cc1d24c6f2efc1f884f81d778b609170f026f Mon Sep 17 00:00:00 2001 From: Brad Barden <[email protected]> Date: Sun, 6 Dec 2015 02:48:38 -0600 Subject: [PATCH 1/2] tar: read mode for all entity types --- tar.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/tar.c b/tar.c index e8dd26a..507a23d 100644 --- a/tar.c +++ b/tar.c @@ -258,6 +258,8 @@ unarchive(char *fname, ssize_t l, char b[BLKSIZ]) if (!mflag && ((mtime = strtol(h->mtime, &p, 8)) < 0 || *p != '\0')) eprintf("strtol %s: invalid number\n", h->mtime); + if ((mode = strtol(h->mode, &p, 8)) < 0 || *p != '\0') + eprintf("strtol %s: invalid number\n", h->mode); if (remove(fname) < 0 && errno != ENOENT) eprintf("remove %s:", fname); @@ -269,8 +271,6 @@ unarchive(char *fname, ssize_t l, char b[BLKSIZ]) case REG: case AREG: case RESERVED: - if ((mode = strtol(h->mode, &p, 8)) < 0 || *p != '\0') - eprintf("strtol %s: invalid number\n", h->mode); fd = open(fname, O_WRONLY | O_TRUNC | O_CREAT, 0600); if (fd < 0) eprintf("open %s:", fname); @@ -285,16 +285,12 @@ unarchive(char *fname, ssize_t l, char b[BLKSIZ]) fname, lname); break; case DIRECTORY: - if ((mode = strtol(h->mode, &p, 8)) < 0 || *p != '\0') - eprintf("strtol %s: invalid number\n", h->mode); if (mkdir(fname, (mode_t)mode) < 0 && errno != EEXIST) eprintf("mkdir %s:", fname); pushdirtime(fname, mtime); break; case CHARDEV: case BLOCKDEV: - if ((mode = strtol(h->mode, &p, 8)) < 0 || *p != '\0') - eprintf("strtol %s: invalid number\n", h->mode); if ((major = strtol(h->major, &p, 8)) < 0 || *p != '\0') eprintf("strtol %s: invalid number\n", h->major); if ((minor = strtol(h->minor, &p, 8)) < 0 || *p != '\0') @@ -304,8 +300,6 @@ unarchive(char *fname, ssize_t l, char b[BLKSIZ]) eprintf("mknod %s:", fname); break; case FIFO: - if ((mode = strtol(h->mode, &p, 8)) < 0 || *p != '\0') - eprintf("strtol %s: invalid number\n", h->mode); if (mknod(fname, S_IFIFO | mode, 0) < 0) eprintf("mknod %s:", fname); break; -- 2.3.6
>From ecad4b414a0d541c8db65f080d824a41dac50c3a Mon Sep 17 00:00:00 2001 From: Brad Barden <[email protected]> Date: Sun, 6 Dec 2015 02:49:31 -0600 Subject: [PATCH 2/2] tar: set mode directly on new files --- tar.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tar.c b/tar.c index 507a23d..e091b94 100644 --- a/tar.c +++ b/tar.c @@ -271,7 +271,7 @@ unarchive(char *fname, ssize_t l, char b[BLKSIZ]) case REG: case AREG: case RESERVED: - fd = open(fname, O_WRONLY | O_TRUNC | O_CREAT, 0600); + fd = open(fname, O_WRONLY | O_TRUNC | O_CREAT, (mode_t)mode); if (fd < 0) eprintf("open %s:", fname); break; -- 2.3.6
>From 13dc79043131bd0839720275d6e21081a36fc9bb Mon Sep 17 00:00:00 2001 From: Brad Barden <[email protected]> Date: Sun, 6 Dec 2015 02:43:32 -0600 Subject: [PATCH 1/1] tar: no mtime/chown/chmod on hard links --- tar.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/tar.c b/tar.c index e8dd26a..551585f 100644 --- a/tar.c +++ b/tar.c @@ -325,18 +325,20 @@ unarchive(char *fname, ssize_t l, char b[BLKSIZ]) close(fd); } - times[0].tv_sec = times[1].tv_sec = mtime; - times[0].tv_nsec = times[1].tv_nsec = 0; - if (!mflag && utimensat(AT_FDCWD, fname, times, AT_SYMLINK_NOFOLLOW) < 0) - weprintf("utimensat %s:\n", fname); - if (h->type == SYMLINK) { - if (!getuid() && lchown(fname, uid, gid)) - weprintf("lchown %s:\n", fname); - } else { - if (!getuid() && chown(fname, uid, gid)) - weprintf("chown %s:\n", fname); - if (chmod(fname, mode) < 0) - eprintf("fchmod %s:\n", fname); + if (h->type != HARDLINK) { + times[0].tv_sec = times[1].tv_sec = mtime; + times[0].tv_nsec = times[1].tv_nsec = 0; + if (!mflag && utimensat(AT_FDCWD, fname, times, AT_SYMLINK_NOFOLLOW) < 0) + weprintf("utimensat %s:\n", fname); + if (h->type == SYMLINK) { + if (!getuid() && lchown(fname, uid, gid)) + weprintf("lchown %s:\n", fname); + } else { + if (!getuid() && chown(fname, uid, gid)) + weprintf("chown %s:\n", fname); + if (chmod(fname, mode) < 0) + eprintf("fchmod %s:\n", fname); + } } return 0; -- 2.3.6
