Thanks. Comments inline.
On Tue, Jun 27, 2017 at 06:28:57AM -0400, Bryan Steele wrote:
> On Tue, Jun 27, 2017 at 01:20:59AM -0400, Bryan Steele wrote:
> > On Tue, Jun 27, 2017 at 12:26:08AM -0400, Bryan Steele wrote:
> > Some unintentional changes crept in, here's another diff..
>
> Sorry, last diff broke width calculation.. 3rd times the charm.
>
> Index: Makefile
> ===================================================================
> RCS file: /cvs/src/usr.bin/file/Makefile,v
> retrieving revision 1.16
> diff -u -p -u -r1.16 Makefile
> --- Makefile 4 Oct 2015 07:25:59 -0000 1.16
> +++ Makefile 27 Jun 2017 10:05:39 -0000
> @@ -5,9 +5,6 @@ SRCS= file.c magic-dump.c magic-load.c
> text.c xmalloc.c
> MAN= file.1 magic.5
>
> -LDADD= -lutil
> -DPADD= ${LIBUTIL}
> -
> CDIAGFLAGS+= -Wno-long-long -Wall -W -Wnested-externs -Wformat=2
> CDIAGFLAGS+= -Wmissing-prototypes -Wstrict-prototypes -Wmissing-declarations
> CDIAGFLAGS+= -Wwrite-strings -Wshadow -Wpointer-arith -Wsign-compare
> Index: file.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/file/file.c,v
> retrieving revision 1.59
> diff -u -p -u -r1.59 file.c
> --- file.c 18 Apr 2017 14:16:48 -0000 1.59
> +++ file.c 27 Jun 2017 10:05:39 -0000
> @@ -29,12 +29,10 @@
> #include <errno.h>
> #include <fcntl.h>
> #include <getopt.h>
> -#include <imsg.h>
> #include <libgen.h>
> #include <limits.h>
> #include <pwd.h>
> #include <stdlib.h>
> -#include <stdlib.h>
> #include <string.h>
> #include <time.h>
> #include <unistd.h>
> @@ -43,27 +41,16 @@
> #include "magic.h"
> #include "xmalloc.h"
>
> -struct input_msg {
> - int idx;
> -
> - struct stat sb;
> - int error;
> -
> - char link_path[PATH_MAX];
> - int link_error;
> - int link_target;
> -};
> -
> -struct input_ack {
> - int idx;
> -};
> -
> struct input_file {
> struct magic *m;
> - struct input_msg *msg;
>
> const char *path;
> - int fd;
> + struct stat sb;
> + int fd, error;
One member per line in structs please. Also you could reduce the amount
of space here now to one tab.
> +
> + char link_path[PATH_MAX];
> + int link_error;
> + int link_target;
>
> void *base;
> size_t size;
> @@ -75,15 +62,13 @@ extern char *__progname;
>
> __dead void usage(void);
>
> -static int prepare_message(struct input_msg *, int, const char *);
> -static void send_message(struct imsgbuf *, void *, size_t, int);
> -static int read_message(struct imsgbuf *, struct imsg *, pid_t);
> +static void prepare_input(struct input_file *, const char *);
>
> -static void read_link(struct input_msg *, const char *);
> +static void read_link(struct input_file *, const char *);
>
> -static __dead void child(int, pid_t, int, char **);
> +static void privdrop(void);
>
> -static void test_file(struct input_file *, size_t);
> +static void test_file(struct input_file *, struct magic *, size_t);
>
> static int try_stat(struct input_file *);
> static int try_empty(struct input_file *);
> @@ -120,14 +105,12 @@ usage(void)
> int
> main(int argc, char **argv)
> {
> - int opt, pair[2], fd, idx;
> + int opt, idx;
> char *home;
> struct passwd *pw;
> - struct imsgbuf ibuf;
> - struct imsg imsg;
> - struct input_msg msg;
> - struct input_ack *ack;
> - pid_t pid, parent;
> + struct magic *m;
> + struct input_file *inf;
> + size_t len, width = 0;
>
> tzset();
>
> @@ -193,71 +176,48 @@ main(int argc, char **argv)
> if (magicfp == NULL)
> err(1, "%s", magicpath);
>
> - parent = getpid();
> - if (socketpair(AF_UNIX, SOCK_STREAM, PF_UNSPEC, pair) != 0)
> - err(1, "socketpair");
> - switch (pid = fork()) {
> - case -1:
> - err(1, "fork");
> - case 0:
> - close(pair[0]);
> - child(pair[1], parent, argc, argv);
> + m = magic_load(magicfp, magicpath, cflag || Wflag);
> + if (cflag) {
> + magic_dump(m);
> + exit(0);
magic_load (which parses the magic file) is now before pledge and
privdrop which is wrong. You need to drop privs and pledge before
magic_load - if needed you can reduce the pledge further after
magic_load, but I think it only needs stdio anyway.
> }
> - close(pair[1]);
> -
> - fclose(magicfp);
> - magicfp = NULL;
>
> - if (cflag)
> - goto wait_for_child;
> -
> - imsg_init(&ibuf, pair[0]);
> + inf = xcalloc(argc, sizeof *inf);
> for (idx = 0; idx < argc; idx++) {
> - fd = prepare_message(&msg, idx, argv[idx]);
> - send_message(&ibuf, &msg, sizeof msg, fd);
> -
> - if (read_message(&ibuf, &imsg, pid) == 0)
> - break;
> - if (imsg.hdr.len != IMSG_HEADER_SIZE + sizeof *ack)
> - errx(1, "message too small");
> - ack = imsg.data;
> - if (ack->idx != idx)
> - errx(1, "index not expected");
> - imsg_free(&imsg);
> + len = strlen(argv[idx]) + 1;
> + if (len > width)
> + width = len;
> + prepare_input(&inf[idx], argv[idx]);
> }
>
> -wait_for_child:
> - close(pair[0]);
> - while (wait(NULL) == -1 && errno != ECHILD) {
> - if (errno != EINTR)
> - err(1, "wait");
> - }
> - _exit(0); /* let the child flush */
> + privdrop();
The privdrop was easier to see inline than in a separate function. I
think it should all be up front in main() not hidden away.
> +
> + for (idx = 0; idx < argc; idx++)
> + test_file(&inf[idx], m, width);
> + fclose(magicfp);
> + magicfp = NULL;
This fclose() is pointless here but it can be done much earlier (after
magic_load).
And you need to exit(0) rather than dropping out of the bottom of main.
> }
>
> -static int
> -prepare_message(struct input_msg *msg, int idx, const char *path)
> +static void
> +prepare_input(struct input_file *inf, const char *path)
> {
> int fd, mode, error;
>
> - memset(msg, 0, sizeof *msg);
> - msg->idx = idx;
> -
> if (strcmp(path, "-") == 0) {
> - if (fstat(STDIN_FILENO, &msg->sb) == -1) {
> - msg->error = errno;
> - return (-1);
> + if (fstat(STDIN_FILENO, &inf->sb) == -1) {
> + inf->error = errno;
> + inf->fd = -1;
> }
> - return (STDIN_FILENO);
> + inf->fd = STDIN_FILENO;
> }
>
> if (Lflag)
> - error = stat(path, &msg->sb);
> + error = stat(path, &inf->sb);
> else
> - error = lstat(path, &msg->sb);
> + error = lstat(path, &inf->sb);
> if (error == -1) {
> - msg->error = errno;
> - return (-1);
> + inf->error = errno;
> + inf->fd = -1;
> }
>
> /*
> @@ -265,7 +225,7 @@ prepare_message(struct input_msg *msg, i
> * but in fact we don't need them, so just don't open directories or
> * symlinks (which could be to directories).
> */
This comment needs to be edited now we are not passing the file
descriptors anywhere.
> - mode = msg->sb.st_mode;
> + mode = inf->sb.st_mode;
> if (!S_ISDIR(mode) && !S_ISLNK(mode)) {
> fd = open(path, O_RDONLY|O_NONBLOCK);
> if (fd == -1 && (errno == ENFILE || errno == EMFILE))
> @@ -273,46 +233,13 @@ prepare_message(struct input_msg *msg, i
> } else
> fd = -1;
> if (S_ISLNK(mode))
> - read_link(msg, path);
> - return (fd);
> -
> -}
> -
> -static void
> -send_message(struct imsgbuf *ibuf, void *msg, size_t msglen, int fd)
> -{
> - if (imsg_compose(ibuf, -1, -1, 0, fd, msg, msglen) != 1)
> - err(1, "imsg_compose");
> - if (imsg_flush(ibuf) != 0)
> - err(1, "imsg_flush");
> -}
> -
> -static int
> -read_message(struct imsgbuf *ibuf, struct imsg *imsg, pid_t from)
> -{
> - int n;
> -
> - while ((n = imsg_read(ibuf)) == -1 && errno == EAGAIN)
> - /* nothing */ ;
> - if (n == -1)
> - err(1, "imsg_read");
> - if (n == 0)
> - return (0);
> -
> - if ((n = imsg_get(ibuf, imsg)) == -1)
> - err(1, "imsg_get");
> - if (n == 0)
> - return (0);
> -
> - if ((pid_t)imsg->hdr.pid != from)
> - errx(1, "PIDs don't match");
> -
> - return (n);
> -
> + read_link(inf, path);
> + inf->fd = fd;
> + inf->path = path;
> }
>
> static void
> -read_link(struct input_msg *msg, const char *path)
> +read_link(struct input_file *inf, const char *path)
> {
> struct stat sb;
> char lpath[PATH_MAX];
> @@ -322,25 +249,25 @@ read_link(struct input_msg *msg, const c
>
> size = readlink(path, lpath, sizeof lpath - 1);
> if (size == -1) {
> - msg->link_error = errno;
> + inf->link_error = errno;
> return;
> }
> lpath[size] = '\0';
>
> if (*lpath == '/')
> - strlcpy(msg->link_path, lpath, sizeof msg->link_path);
> + strlcpy(inf->link_path, lpath, sizeof inf->link_path);
> else {
> copy = xstrdup(path);
>
> root = dirname(copy);
> if (*root == '\0' || strcmp(root, ".") == 0 ||
> strcmp (root, "/") == 0)
> - strlcpy(msg->link_path, lpath, sizeof msg->link_path);
> + strlcpy(inf->link_path, lpath, sizeof inf->link_path);
> else {
> - used = snprintf(msg->link_path, sizeof msg->link_path,
> + used = snprintf(inf->link_path, sizeof inf->link_path,
> "%s/%s", root, lpath);
> - if (used < 0 || (size_t)used >= sizeof msg->link_path) {
> - msg->link_error = ENAMETOOLONG;
> + if (used < 0 || (size_t)used >= sizeof inf->link_path) {
> + inf->link_error = ENAMETOOLONG;
> free(copy);
> return;
> }
> @@ -350,23 +277,15 @@ read_link(struct input_msg *msg, const c
> }
>
> if (!Lflag && stat(path, &sb) == -1)
> - msg->link_target = errno;
> + inf->link_target = errno;
> }
>
> -static __dead void
> -child(int fd, pid_t parent, int argc, char **argv)
> +static void
> +privdrop(void)
> {
> struct passwd *pw;
> - struct magic *m;
> - struct imsgbuf ibuf;
> - struct imsg imsg;
> - struct input_msg *msg;
> - struct input_ack ack;
> - struct input_file inf;
> - int i, idx;
> - size_t len, width = 0;
>
> - if (pledge("stdio getpw recvfd id", NULL) == -1)
> + if (pledge("stdio getpw id", NULL) == -1)
> err(1, "pledge");
>
> if (geteuid() == 0) {
> @@ -381,50 +300,9 @@ child(int fd, pid_t parent, int argc, ch
> err(1, "setresuid");
> }
>
> - if (pledge("stdio recvfd", NULL) == -1)
> + if (pledge("stdio", NULL) == -1)
> err(1, "pledge");
>
> - m = magic_load(magicfp, magicpath, cflag || Wflag);
> - if (cflag) {
> - magic_dump(m);
> - exit(0);
> - }
> -
> - for (i = 0; i < argc; i++) {
> - len = strlen(argv[i]) + 1;
> - if (len > width)
> - width = len;
> - }
> -
> - imsg_init(&ibuf, fd);
> - for (;;) {
> - if (read_message(&ibuf, &imsg, parent) == 0)
> - break;
> - if (imsg.hdr.len != IMSG_HEADER_SIZE + sizeof *msg)
> - errx(1, "message too small");
> - msg = imsg.data;
> -
> - idx = msg->idx;
> - if (idx < 0 || idx >= argc)
> - errx(1, "index out of range");
> -
> - memset(&inf, 0, sizeof inf);
> - inf.m = m;
> - inf.msg = msg;
> -
> - inf.path = argv[idx];
> - inf.fd = imsg.fd;
> -
> - test_file(&inf, width);
> -
> - if (imsg.fd != -1)
> - close(imsg.fd);
> - imsg_free(&imsg);
> -
> - ack.idx = idx;
> - send_message(&ibuf, &ack, sizeof ack, -1);
> - }
> - exit(0);
> }
>
> static void *
> @@ -461,14 +339,14 @@ load_file(struct input_file *inf)
> {
> size_t used;
>
> - if (inf->msg->sb.st_size == 0 && S_ISREG(inf->msg->sb.st_mode))
> + if (inf->sb.st_size == 0 && S_ISREG(inf->sb.st_mode))
> return (0); /* empty file */
> - if (inf->msg->sb.st_size == 0 || inf->msg->sb.st_size > FILE_READ_SIZE)
> + if (inf->sb.st_size == 0 || inf->sb.st_size > FILE_READ_SIZE)
> inf->size = FILE_READ_SIZE;
> else
> - inf->size = inf->msg->sb.st_size;
> + inf->size = inf->sb.st_size;
>
> - if (!S_ISREG(inf->msg->sb.st_mode))
> + if (!S_ISREG(inf->sb.st_mode))
> goto try_read;
>
> inf->base = mmap(NULL, inf->size, PROT_READ, MAP_PRIVATE, inf->fd, 0);
> @@ -491,13 +369,13 @@ try_read:
> static int
> try_stat(struct input_file *inf)
> {
> - if (inf->msg->error != 0) {
> + if (inf->error != 0) {
> xasprintf(&inf->result, "cannot stat '%s' (%s)", inf->path,
> - strerror(inf->msg->error));
> + strerror(inf->error));
> return (1);
> }
> if (sflag || strcmp(inf->path, "-") == 0) {
> - switch (inf->msg->sb.st_mode & S_IFMT) {
> + switch (inf->sb.st_mode & S_IFMT) {
> case S_IFIFO:
> if (strcmp(inf->path, "-") != 0)
> break;
> @@ -508,29 +386,29 @@ try_stat(struct input_file *inf)
> }
> }
>
> - if (iflag && (inf->msg->sb.st_mode & S_IFMT) != S_IFREG) {
> + if (iflag && (inf->sb.st_mode & S_IFMT) != S_IFREG) {
> xasprintf(&inf->result, "application/x-not-regular-file");
> return (1);
> }
>
> - switch (inf->msg->sb.st_mode & S_IFMT) {
> + switch (inf->sb.st_mode & S_IFMT) {
> case S_IFDIR:
> xasprintf(&inf->result, "directory");
> return (1);
> case S_IFLNK:
> - if (inf->msg->link_error != 0) {
> + if (inf->link_error != 0) {
> xasprintf(&inf->result, "unreadable symlink '%s' (%s)",
> - inf->path, strerror(inf->msg->link_error));
> + inf->path, strerror(inf->link_error));
> return (1);
> }
> - if (inf->msg->link_target == ELOOP)
> + if (inf->link_target == ELOOP)
> xasprintf(&inf->result, "symbolic link in a loop");
> - else if (inf->msg->link_target != 0) {
> + else if (inf->link_target != 0) {
> xasprintf(&inf->result, "broken symbolic link to '%s'",
> - inf->msg->link_path);
> + inf->link_path);
> } else {
> xasprintf(&inf->result, "symbolic link to '%s'",
> - inf->msg->link_path);
> + inf->link_path);
> }
> return (1);
> case S_IFSOCK:
> @@ -538,13 +416,13 @@ try_stat(struct input_file *inf)
> return (1);
> case S_IFBLK:
> xasprintf(&inf->result, "block special (%ld/%ld)",
> - (long)major(inf->msg->sb.st_rdev),
> - (long)minor(inf->msg->sb.st_rdev));
> + (long)major(inf->sb.st_rdev),
> + (long)minor(inf->sb.st_rdev));
> return (1);
> case S_IFCHR:
> xasprintf(&inf->result, "character special (%ld/%ld)",
> - (long)major(inf->msg->sb.st_rdev),
> - (long)minor(inf->msg->sb.st_rdev));
> + (long)major(inf->sb.st_rdev),
> + (long)minor(inf->sb.st_rdev));
> return (1);
> case S_IFIFO:
> xasprintf(&inf->result, "fifo (named pipe)");
> @@ -571,16 +449,16 @@ try_access(struct input_file *inf)
> {
> char tmp[256] = "";
>
> - if (inf->msg->sb.st_size == 0 && S_ISREG(inf->msg->sb.st_mode))
> + if (inf->sb.st_size == 0 && S_ISREG(inf->sb.st_mode))
> return (0); /* empty file */
> if (inf->fd != -1)
> return (0);
>
> - if (inf->msg->sb.st_mode & (S_IWUSR|S_IWGRP|S_IWOTH))
> + if (inf->sb.st_mode & (S_IWUSR|S_IWGRP|S_IWOTH))
> strlcat(tmp, "writable, ", sizeof tmp);
> - if (inf->msg->sb.st_mode & (S_IXUSR|S_IXGRP|S_IXOTH))
> + if (inf->sb.st_mode & (S_IXUSR|S_IXGRP|S_IXOTH))
> strlcat(tmp, "executable, ", sizeof tmp);
> - if (S_ISREG(inf->msg->sb.st_mode))
> + if (S_ISREG(inf->sb.st_mode))
> strlcat(tmp, "regular file, ", sizeof tmp);
> strlcat(tmp, "no read permission", sizeof tmp);
>
> @@ -653,10 +531,12 @@ try_unknown(struct input_file *inf)
> }
>
> static void
> -test_file(struct input_file *inf, size_t width)
> +test_file(struct input_file *inf, struct magic *m, size_t width)
> {
> char *label;
> int stop;
> +
> + inf->m = m;
I'd assign this in the loop which calls test_file? Everything else in
input_file is set up outside this function.
>
> stop = 0;
> if (!stop)
>
