commit ba7c5de18757cbbf429cb5fe419937a4c650be13
Author: FRIGN <[email protected]>
Date:   Sat Mar 21 01:03:35 2015 +0100

    Audit tar(1), add DIRFIRST-flag to recurse()
    
    I've been wanting to do this for a while now, as tar(1) used to
    be one of messiest and cruftiest tools.
    First off, before walking through the audit, I'll talk about
    what the DIRFIRST-flag for recurse() does.
    It basically calls fn() on the first-level-dir before calling
    it's subentries. It's necessary here, because else the order
    of the tar-files would've been wrong (it would try to create
    dir/file before creating dir/).
    
    Now, to the audit:
    1)  Update manpage, fix mistake that compression is also available
        for compressing. It's only available for extracting.
    2)  Define the major, minor and makedev macros from glibc by ourselves.
        No need to rely on them, as they are common sense.
    
    decomp()
    3)  Simple refactorization.
    
    putoctal()
    4)  Add a truncation check for snprintf().
    
    archive()
    5)  BUGFIX: Add checks to any checkable function, don't blindly call
        them, this is harmful and there are 100 ways to exploit that.
    6)  Use estrlcpy() instead of snprintf() wherever possible, fix
        alignment.
    7)  BUGFIX: Terminate the result-buffer of readlink(), check if
        it even succeeded.
    8)  Fix sizeof()-formatting.
    
    unarchive()
    9)  BUGFIX: Add checks to any checkable function, don't blindly call
        them, this is harmful and there are 100 ways to exploit that.
    10) BUGFIX: strtoul can happily return negative numbers. Add checks
        for that and also if the full string has been processed.
    11) Remove calls to perror(). We have eprintf, use it.
    12) BUGFIX: "minor = strtoul(h->mode, 0, 8);". We need h->minor of
        course.
    13) Fix typo "usupported", remove fprintf-call.
    
    print()
    14) Check fread().
    
    xt()
    15) Get rid of snprintf-magic. Use estrlcat().
    16) BUGFIX: check for ferror() on the tarfile.
    
    usage()
    17) Update it. The old usage() was like 1000 years old.
    
    main()
    18) Add DIRFIRST-flag to the recursor.
    19) Don't print usage() when a mode is re-set. We allow this in
        general.
    20) Add function checks and fix error messages.
    21) Add tarfilename-global for proper error-messages.

diff --git a/README b/README
index 1d9186d..4d97f00 100644
--- a/README
+++ b/README
@@ -72,7 +72,7 @@ The following tools are implemented ('*' == finished, '#' == 
UTF-8 support,
 #*| strings         yes                          none
 =*| sync            non-posix                    none
 =*| tail            yes                          none
-=*  tar             non-posix                    none
+=*| tar             non-posix                    none
 =*| tee             yes                          none
 =*| test            yes                          none
 =*| time            yes                          none
diff --git a/fs.h b/fs.h
index e7590f8..27ceb52 100644
--- a/fs.h
+++ b/fs.h
@@ -17,7 +17,8 @@ struct recursor {
 };
 
 enum {
-        SAMEDEV = 1 << 0,
+       SAMEDEV  = 1 << 0,
+       DIRFIRST = 1 << 1,
 };
 
 extern int cp_aflag;
diff --git a/libutil/recurse.c b/libutil/recurse.c
index c7f67d3..8a37c2f 100644
--- a/libutil/recurse.c
+++ b/libutil/recurse.c
@@ -58,6 +58,9 @@ recurse(const char *path, void *data, struct recursor *r)
                return;
        }
 
+       if (!r->depth && (r->flags & DIRFIRST))
+               (r->fn)(path, &st, data, r);
+
        while ((d = readdir(dp))) {
                if (r->follow == 'H') {
                        statf_name = "lstat";
@@ -82,7 +85,8 @@ recurse(const char *path, void *data, struct recursor *r)
        }
 
        if (!r->depth) {
-               (r->fn)(path, &st, data, r);
+               if (!(r->flags & DIRFIRST))
+                       (r->fn)(path, &st, data, r);
 
                for (; r->hist; ) {
                        h = r->hist;
diff --git a/tar.1 b/tar.1
index 6e0ec56..a9e438b 100644
--- a/tar.1
+++ b/tar.1
@@ -1,4 +1,4 @@
-.Dd February 8, 2015
+.Dd March 21, 2015
 .Dt TAR 1
 .Os sbase
 .Sh NAME
@@ -13,15 +13,8 @@
 .Nm
 .Op Fl C Ar dir
 .Op Fl h
-.Op Fl j | Fl z
 .Fl c Ar dir
 .Op Fl f Ar file
-.Nm
-.Op Fl C Ar dir
-.Op Fl h
-.Op Fl j | Fl z
-.Fl cf
-.Ar file Ar dir
 .Sh DESCRIPTION
 .Nm
 is the standard file archiver.
@@ -47,7 +40,7 @@ Extract archive.
 .It Fl h
 Always dereference symbolic links while recursively traversing directories.
 .It Fl j | Fl z
-Use bzip2 | gzip compression. The
+Use bzip2 | gzip decompression. The
 .Xr bzip2 1 |
 .Xr gzip 1
 utilities must be installed separately.
diff --git a/tar.c b/tar.c
index 965f87e..9606515 100644
--- a/tar.c
+++ b/tar.c
@@ -2,6 +2,7 @@
 #include <sys/stat.h>
 #include <sys/time.h>
 
+#include <errno.h>
 #include <grp.h>
 #include <pwd.h>
 #include <stdio.h>
@@ -33,12 +34,20 @@ struct header {
 
 #define BLKSIZ 512
 
+#undef major
+#define major(dev) ((int)(((unsigned int)(dev) >> 8) & 0xff))
+#undef minor
+#define minor(dev) ((int)((dev) & 0xff))
+#undef makedev
+#define makedev(major, minor) (((major) << 8) | (minor))
+
 enum Type {
        REG = '0', AREG = '\0', HARDLINK = '1', SYMLINK = '2', CHARDEV = '3',
        BLOCKDEV = '4', DIRECTORY = '5', FIFO = '6'
 };
 
 static FILE *tarfile;
+static char *tarfilename;
 static ino_t tarinode;
 static dev_t tardev;
 
@@ -48,7 +57,8 @@ static char filtermode;
 static FILE *
 decomp(FILE *fp)
 {
-       int   fds[2];
+       int fds[2];
+       char *tool;
 
        if (pipe(fds) < 0)
                eprintf("pipe:");
@@ -62,175 +72,202 @@ decomp(FILE *fp)
                close(fds[0]);
                close(fds[1]);
 
-               switch (filtermode) {
-               case 'j':
-                       execlp("bzip2", "bzip2", "-cd", NULL);
-                       weprintf("execlp bzip2:");
-                       _exit(1);
-               case 'z':
-                       execlp("gzip", "gzip", "-cd", NULL);
-                       weprintf("execlp gzip:");
-                       _exit(1);
-               }
+               tool = (filtermode == 'j') ? "bzip2" : "gzip";
+               execlp(tool, tool, "-cd", NULL);
+               weprintf("execlp %s:", tool);
+               _exit(1);
        }
        close(fds[1]);
+
        return fdopen(fds[0], "r");
 }
 
 static void
-putoctal(char *dst, unsigned num, int n)
+putoctal(char *dst, unsigned num, int size)
 {
-       snprintf(dst, n, "%.*o", n - 1, num);
+       if (snprintf(dst, size, "%.*o", size - 1, num) >= size)
+               eprintf("snprintf: input number too large\n");
 }
 
 static int
-archive(const char* path)
+archive(const char *path)
 {
        FILE *f = NULL;
-       mode_t mode;
        struct group *gr;
        struct header *h;
        struct passwd *pw;
        struct stat st;
        size_t chksum, x;
-       ssize_t l;
+       ssize_t l, r;
        unsigned char b[BLKSIZ];
 
-       lstat(path, &st);
-       if (st.st_ino == tarinode && st.st_dev == tardev) {
-               fprintf(stderr, "ignoring '%s'\n", path);
+       if (lstat(path, &st) < 0) {
+               weprintf("lstat %s:", path);
+               return 0;
+       } else if (st.st_ino == tarinode && st.st_dev == tardev) {
+               weprintf("ignoring %s\n", path);
+               return 0;
+       }
+       errno = 0;
+       if (!(pw = getpwuid(st.st_uid)) && errno) {
+               weprintf("getpwuid:");
+               return 0;
+       }
+       errno = 0;
+       if (!(gr = getgrgid(st.st_gid)) && errno) {
+               weprintf("getgrgid:");
                return 0;
        }
-       pw = getpwuid(st.st_uid);
-       gr = getgrgid(st.st_gid);
 
-       h = (void*)b;
+       h = (void *)b;
        memset(b, 0, sizeof(b));
-       snprintf(h->name, sizeof(h->name), "%s", path);
-       putoctal(h->mode,  (unsigned)st.st_mode & 0777, sizeof(h->mode));
-       putoctal(h->uid,   (unsigned)st.st_uid,         sizeof(h->uid));
-       putoctal(h->gid,   (unsigned)st.st_gid,         sizeof(h->gid));
-       putoctal(h->size,  0,                           sizeof(h->size));
-       putoctal(h->mtime, (unsigned)st.st_mtime,       sizeof(h->mtime));
-       memcpy(h->magic,   "ustar",                     sizeof(h->magic));
-       memcpy(h->version, "00",                        sizeof(h->version));
-       snprintf(h->uname, sizeof h->uname, "%s", pw ? pw->pw_name : "");
-       snprintf(h->gname, sizeof h->gname, "%s", gr ? gr->gr_name : "");
-
-       mode = st.st_mode;
-       if (S_ISREG(mode)) {
+       estrlcpy(h->name,    path,                        sizeof(h->name));
+       putoctal(h->mode,    (unsigned)st.st_mode & 0777, sizeof(h->mode));
+       putoctal(h->uid,     (unsigned)st.st_uid,         sizeof(h->uid));
+       putoctal(h->gid,     (unsigned)st.st_gid,         sizeof(h->gid));
+       putoctal(h->size,    0,                           sizeof(h->size));
+       putoctal(h->mtime,   (unsigned)st.st_mtime,       sizeof(h->mtime));
+       memcpy(  h->magic,   "ustar",                     sizeof(h->magic));
+       memcpy(  h->version, "00",                        sizeof(h->version));
+       estrlcpy(h->uname,   pw ? pw->pw_name : "",       sizeof(h->uname));
+       estrlcpy(h->gname,   gr ? gr->gr_name : "",       sizeof(h->gname));
+
+       if (S_ISREG(st.st_mode)) {
                h->type = REG;
-               putoctal(h->size, (unsigned)st.st_size,  sizeof h->size);
+               putoctal(h->size, (unsigned)st.st_size,  sizeof(h->size));
                f = fopen(path, "r");
-       } else if (S_ISDIR(mode)) {
+       } else if (S_ISDIR(st.st_mode)) {
                h->type = DIRECTORY;
-       } else if (S_ISLNK(mode)) {
+       } else if (S_ISLNK(st.st_mode)) {
                h->type = SYMLINK;
-               readlink(path, h->link, (sizeof h->link)-1);
-       } else if (S_ISCHR(mode) || S_ISBLK(mode)) {
-               h->type = S_ISCHR(mode) ? CHARDEV : BLOCKDEV;
-#if defined(major) && defined(minor)
-               putoctal(h->major, (unsigned)major(st.st_dev), sizeof h->major);
-               putoctal(h->minor, (unsigned)minor(st.st_dev), sizeof h->minor);
-#else
-               return 0;
-#endif
-       } else if (S_ISFIFO(mode)) {
+               if ((r = readlink(path, h->link, sizeof(h->link) - 1)) < 0)
+                       eprintf("readlink %s:", path);
+               h->link[r] = '\0';
+       } else if (S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode)) {
+               h->type = S_ISCHR(st.st_mode) ? CHARDEV : BLOCKDEV;
+               putoctal(h->major, (unsigned)major(st.st_dev), 
sizeof(h->major));
+               putoctal(h->minor, (unsigned)minor(st.st_dev), 
sizeof(h->minor));
+       } else if (S_ISFIFO(st.st_mode)) {
                h->type = FIFO;
        }
 
-       memset(h->chksum, ' ', sizeof h->chksum);
-       for (x = 0, chksum = 0; x < sizeof *h; x++)
+       memset(h->chksum, ' ', sizeof(h->chksum));
+       for (x = 0, chksum = 0; x < sizeof(*h); x++)
                chksum += b[x];
-       putoctal(h->chksum, chksum, sizeof h->chksum);
+       putoctal(h->chksum, chksum, sizeof(h->chksum));
 
-       fwrite(b, BLKSIZ, 1, tarfile);
-       if (!f)
-               return 0;
-       while ((l = fread(b, 1, BLKSIZ, f)) > 0) {
-               if (l < BLKSIZ)
-                       memset(b+l, 0, BLKSIZ-l);
-               fwrite(b, BLKSIZ, 1, tarfile);
+       if (fwrite(b, BLKSIZ, 1, tarfile) != 1)
+               eprintf("fwrite:");
+
+       if (f) {
+               while ((l = fread(b, 1, BLKSIZ, f)) > 0) {
+                       if (l < BLKSIZ)
+                               memset(b + l, 0, BLKSIZ - l);
+                       if (fwrite(b, BLKSIZ, 1, tarfile) != 1)
+                               eprintf("fwrite:");
+               }
+               if (fclose(f) == EOF)
+                       eprintf("fclose %s:", path);
        }
-       fclose(f);
+
        return 0;
 }
 
 static int
-unarchive(char *fname, int l, char b[BLKSIZ])
+unarchive(char *fname, ssize_t l, char b[BLKSIZ])
 {
        FILE *f = NULL;
        struct timeval times[2];
-       struct header *h = (void*)b;
-       unsigned long mode, major, minor, type, mtime;
-       char lname[101];
+       struct header *h = (void *)b;
+       long mode, major, minor, type, mtime, uid, gid;
+       char lname[101], *p;
+
+       if (!mflag && ((mtime = strtoul(h->mtime, &p, 8)) < 0 || *p != '\0'))
+               eprintf("strtoul %s: invalid number\n", h->mtime);
+       if (unlink(fname) < 0 && errno != ENOENT && errno != EISDIR)
+               eprintf("unlink %s:", fname);
 
-       if (!mflag)
-               mtime = strtoul(h->mtime, 0, 8);
-       unlink(fname);
        switch (h->type) {
        case REG:
        case AREG:
-               mode = strtoul(h->mode, 0, 8);
-               if (!(f = fopen(fname, "w")) || chmod(fname, mode))
-                       perror(fname);
+               if ((mode = strtoul(h->mode, &p, 8)) < 0 || *p != '\0')
+                       eprintf("strtoul %s: invalid number\n", h->mode);
+               if (!(f = fopen(fname, "w")))
+                       eprintf("fopen %s:", fname);
+               if (chmod(fname, mode) < 0)
+                       eprintf("chmod %s:", fname);
                break;
        case HARDLINK:
        case SYMLINK:
-               snprintf(lname, sizeof lname, "%s", h->link);
-               if (!((h->type == HARDLINK) ? link : symlink)(lname, fname))
-                       perror(fname);
+               estrlcpy(lname, h->link, sizeof(lname));
+               if (((h->type == HARDLINK) ? link : symlink)(lname, fname) < 0)
+                       eprintf("%s %s -> %s:",
+                               (h->type == HARDLINK) ? "link" : "symlink",
+                               fname, lname);
                break;
        case DIRECTORY:
-               mode = strtoul(h->mode, 0, 8);
-               if (mkdir(fname, (mode_t)mode))
-                       perror(fname);
+               if ((mode = strtoul(h->mode, &p, 8)) < 0 || *p != '\0')
+                       eprintf("strtoul %s: invalid number\n", h->mode);
+               if (mkdir(fname, (mode_t)mode) < 0 && errno != EEXIST)
+                       eprintf("mkdir %s:", fname);
                break;
        case CHARDEV:
        case BLOCKDEV:
-#ifdef makedev
-               mode = strtoul(h->mode, 0, 8);
-               major = strtoul(h->major, 0, 8);
-               minor = strtoul(h->mode, 0, 8);
+               if ((mode = strtoul(h->mode, &p, 8)) < 0 || *p != '\0')
+                       eprintf("strtoul %s: invalid number\n", h->mode);
+               if ((major = strtoul(h->major, &p, 8)) < 0 || *p != '\0')
+                       eprintf("strtoul %s: invalid number\n", h->major);
+               if ((minor = strtoul(h->minor, &p, 8)) < 0 || *p != '\0')
+                       eprintf("strtoul %s: invalid number\n", h->minor);
                type = (h->type == CHARDEV) ? S_IFCHR : S_IFBLK;
-               if (mknod(fname, type | mode, makedev(major, minor)))
-                       perror(fname);
-#endif
+               if (mknod(fname, type | mode, makedev(major, minor)) < 0)
+                       eprintf("mknod %s:", fname);
                break;
        case FIFO:
-               mode = strtoul(h->mode, 0, 8);
-               if (mknod(fname, S_IFIFO | mode, 0))
-                       perror(fname);
+               if ((mode = strtoul(h->mode, &p, 8)) < 0 || *p != '\0')
+                       eprintf("strtoul %s: invalid number\n", h->mode);
+               if (mknod(fname, S_IFIFO | mode, 0) < 0)
+                       eprintf("mknod %s:", fname);
                break;
        default:
-               fprintf(stderr, "usupported tarfiletype %c\n", h->type);
+               eprintf("unsupported tar-filetype %c\n", h->type);
        }
-       if (getuid() == 0 && chown(fname, strtoul(h->uid, 0, 8), 
strtoul(h->gid, 0, 8)))
-               perror(fname);
+
+       if ((uid = strtoul(h->uid, &p, 8)) < 0 || *p != '\0')
+               eprintf("strtoul %s: invalid number\n", h->uid);
+       if ((gid = strtoul(h->gid, &p, 8)) < 0 || *p != '\0')
+               eprintf("strtoul %s: invalid number\n", h->gid);
+       if (!getuid() && chown(fname, uid, gid))
+               eprintf("chown %s:", fname);
 
        for (; l > 0; l -= BLKSIZ) {
-               fread(b, BLKSIZ, 1, tarfile);
-               if (f)
-                       fwrite(b, MIN(l, 512), 1, f);
+               if (fread(b, BLKSIZ, 1, tarfile) != 1)
+                       eprintf("fread %s:", tarfilename);
+               if (f && fwrite(b, MIN(l, BLKSIZ), 1, f) != 1)
+                       eprintf("fwrite %s:", fname);
        }
-       if (f)
-               fclose(f);
+       if (f && fclose(f) == EOF)
+               eprintf("fclose %s:", fname);
 
        if (!mflag) {
                times[0].tv_sec = times[1].tv_sec = mtime;
                times[0].tv_usec = times[1].tv_usec = 0;
-               if (utimes(fname, times))
-                       perror(fname);
+               if (utimes(fname, times) < 0)
+                       eprintf("utimes %s:", fname);
        }
+
        return 0;
 }
 
 static int
-print(char * fname, int l, char b[BLKSIZ])
+print(char *fname, ssize_t l, char b[BLKSIZ])
 {
        puts(fname);
+
        for (; l > 0; l -= BLKSIZ)
-               fread(b, BLKSIZ, 1, tarfile);
+               if (fread(b, BLKSIZ, 1, tarfile) != 1)
+                       eprintf("fread %s:", tarfilename);
+
        return 0;
 }
 
@@ -244,32 +281,42 @@ c(const char *path, struct stat *st, void *data, struct 
recursor *r)
 }
 
 static void
-xt(int (*fn)(char *, int, char[BLKSIZ]))
+xt(int (*fn)(char *, ssize_t, char[BLKSIZ]))
 {
-       char b[BLKSIZ], fname[257], *s;
-       struct header *h = (void*)b;
-
-       while (fread(b, BLKSIZ, 1, tarfile) && h->name[0] != '\0') {
-               s = fname;
-               if (h->prefix[0] != '\0')
-                       s += sprintf(s, "%.*s/", (int)sizeof h->prefix, 
h->prefix);
-               sprintf(s, "%.*s", (int)sizeof h->name, h->name);
-               fn(fname, strtol(h->size, 0, 8), b);
+       struct header *h;
+       long size;
+       char b[BLKSIZ], fname[256 + 1], *p;
+
+       h = (void *)b;
+
+       while (fread(b, BLKSIZ, 1, tarfile) == 1 && *(h->name)) {
+               fname[0] = '\0';
+               if (*(h->prefix)) {
+                       estrlcat(fname, h->prefix, sizeof(fname));
+                       estrlcat(fname, "/", sizeof(fname));
+               }
+               estrlcat(fname, h->name, sizeof(fname));
+               if ((size = strtoul(h->size, &p, 8)) < 0 || *p != '\0')
+                       eprintf("strtoul %s: invalid number\n", h->size);
+
+               fn(fname, size, b);
        }
+       if (ferror(tarfile))
+               eprintf("fread %s:", tarfilename);
 }
 
 static void
 usage(void)
 {
-       eprintf("usage: tar [-f tarfile] [-C dir] -j|z -x[m]|t\n"
-               "       tar [-f tarfile] [-C dir] -c dir\n");
+       eprintf("usage: %s [-C dir] [-j | -z] -x [-m | -t] [-f file]\n"
+               "       %s [-C dir] [-h] -c dir [-f file]\n", argv0, argv0);
 }
 
 int
 main(int argc, char *argv[])
 {
        FILE *fp;
-       struct recursor r = { .fn = c, .hist = NULL, .depth = 0, .follow = 'P', 
.flags = 0};
+       struct recursor r = { .fn = c, .hist = NULL, .depth = 0, .follow = 'P', 
.flags = DIRFIRST};
        struct stat st;
        char *file = NULL, *dir = ".", mode = '\0';
 
@@ -277,8 +324,6 @@ main(int argc, char *argv[])
        case 'x':
        case 'c':
        case 't':
-               if (mode)
-                       usage();
                mode = ARGC();
                break;
        case 'C':
@@ -292,8 +337,6 @@ main(int argc, char *argv[])
                break;
        case 'j':
        case 'z':
-               if (filtermode)
-                       usage();
                filtermode = ARGC();
                break;
        case 'h':
@@ -312,14 +355,17 @@ main(int argc, char *argv[])
                        if (!(fp = fopen(file, "w")))
                                eprintf("fopen %s:", file);
                        if (lstat(file, &st) < 0)
-                               eprintf("tar: stat '%s':", file);
+                               eprintf("lstat %s:", file);
                        tarinode = st.st_ino;
                        tardev = st.st_dev;
                        tarfile = fp;
+                       tarfilename = file;
                } else {
                        tarfile = stdout;
+                       tarfilename = "<stdout>";
                }
-               chdir(dir);
+               if (chdir(dir) < 0)
+                       eprintf("chdir %s:", dir);
                recurse(argv[0], NULL, &r);
                break;
        case 't':
@@ -331,6 +377,8 @@ main(int argc, char *argv[])
                        fp = stdin;
                }
 
+               tarfilename = file;
+
                switch (filtermode) {
                case 'j':
                case 'z':
@@ -341,8 +389,9 @@ main(int argc, char *argv[])
                        break;
                }
 
-               chdir(dir);
-               xt(mode == 'x' ? unarchive : print);
+               if (chdir(dir) < 0)
+                       eprintf("chdir %s:", dir);
+               xt((mode == 'x') ? unarchive : print);
                break;
        }
 

Reply via email to