On Tue, Jun 27, 2017 at 11:40:49PM +0100, Nicholas Marriott wrote:
> Thanks. Comments inline.
>
> > [..]
>
> One member per line in structs please. Also you could reduce the amount
> of space here now to one tab.
>
> > [..]
>
> 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.
>
> > [..]
>
> 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.
>
> > [..]
>
> 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.
>
> > [..]
>
> This comment needs to be edited now we are not passing the file
> descriptors anywhere.
>
> > [..]
>
> I'd assign this in the loop which calls test_file? Everything else in
> input_file is set up outside this function.
>
Here's another stab at it with your suggestions, seems to work. Thanks!
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 23:19:35 -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 23:19:35 -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,45 +41,31 @@
#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;
+ struct magic *m;
- const char *path;
- int fd;
+ const char *path;
+ struct stat sb;
+ int fd;
+ int error;
- void *base;
- size_t size;
- int mapped;
- char *result;
+ char link_path[PATH_MAX];
+ int link_error;
+ int link_target;
+
+ void *base;
+ size_t size;
+ int mapped;
+ char *result;
};
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 read_link(struct input_msg *, const char *);
+static void prepare_input(struct input_file *, const char *);
-static __dead void child(int, pid_t, int, char **);
+static void read_link(struct input_file *, const char *);
static void test_file(struct input_file *, size_t);
@@ -120,14 +104,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,79 +175,75 @@ 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);
+ if (!cflag) {
+ inf = xcalloc(argc, sizeof *inf);
+ for (idx = 0; idx < argc; idx++) {
+ len = strlen(argv[idx]) + 1;
+ if (len > width)
+ width = len;
+ prepare_input(&inf[idx], argv[idx]);
+ }
}
- close(pair[1]);
- fclose(magicfp);
- magicfp = NULL;
+ if (pledge("stdio getpw id", NULL) == -1)
+ err(1, "pledge");
- if (cflag)
- goto wait_for_child;
+ if (geteuid() == 0) {
+ pw = getpwnam(FILE_USER);
+ if (pw == NULL)
+ errx(1, "unknown user %s", FILE_USER);
+ if (setgroups(1, &pw->pw_gid) != 0)
+ err(1, "setgroups");
+ if (setresgid(pw->pw_gid, pw->pw_gid, pw->pw_gid) != 0)
+ err(1, "setresgid");
+ if (setresuid(pw->pw_uid, pw->pw_uid, pw->pw_uid) != 0)
+ err(1, "setresuid");
+ }
- imsg_init(&ibuf, pair[0]);
- for (idx = 0; idx < argc; idx++) {
- fd = prepare_message(&msg, idx, argv[idx]);
- send_message(&ibuf, &msg, sizeof msg, fd);
+ if (pledge("stdio", NULL) == -1)
+ err(1, "pledge");
- 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);
+ m = magic_load(magicfp, magicpath, cflag || Wflag);
+ if (cflag) {
+ magic_dump(m);
+ exit(0);
}
+ fclose(magicfp);
+ magicfp = NULL;
-wait_for_child:
- close(pair[0]);
- while (wait(NULL) == -1 && errno != ECHILD) {
- if (errno != EINTR)
- err(1, "wait");
+ for (idx = 0; idx < argc; idx++) {
+ inf[idx].m = m;
+ test_file(&inf[idx], width);
}
- _exit(0); /* let the child flush */
+ exit(0);
}
-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;
}
/*
- * pledge(2) doesn't let us pass directory file descriptors around -
- * but in fact we don't need them, so just don't open directories or
- * symlinks (which could be to directories).
+ * Don't open directories or symlinks (which could be to directories).
*/
- 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 +251,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 +267,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,81 +295,7 @@ read_link(struct input_msg *msg, const c
}
if (!Lflag && stat(path, &sb) == -1)
- msg->link_target = errno;
-}
-
-static __dead void
-child(int fd, pid_t parent, int argc, char **argv)
-{
- 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)
- err(1, "pledge");
-
- if (geteuid() == 0) {
- pw = getpwnam(FILE_USER);
- if (pw == NULL)
- errx(1, "unknown user %s", FILE_USER);
- if (setgroups(1, &pw->pw_gid) != 0)
- err(1, "setgroups");
- if (setresgid(pw->pw_gid, pw->pw_gid, pw->pw_gid) != 0)
- err(1, "setresgid");
- if (setresuid(pw->pw_uid, pw->pw_uid, pw->pw_uid) != 0)
- err(1, "setresuid");
- }
-
- if (pledge("stdio recvfd", 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);
+ inf->link_target = errno;
}
static void *
@@ -461,14 +332,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 +362,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 +379,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 +409,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 +442,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);
