libreboot-utils: general code cleanup

Signed-off-by: Leah Rowe <leah@libreboot.org>

1 files changed, 72 insertions, 225 deletions

diff --git a/util/libreboot-utils/lib/mkhtemp.c b/util/libreboot-utils/lib/mkhtemp.c
index 8f54b045..b30f6587 100644
--- a/util/libreboot-utils/lib/mkhtemp.c
+++ b/util/libreboot-utils/lib/mkhtemp.c
@@ -168,10 +168,7 @@ err:
 	else
 		saved_errno = errno = EIO;
 
-	if (dest != NULL) {
-		free(dest);
-		dest = NULL;
-	}
+	free_if_null(&dest);
 
 	close_no_err(&dirfd);
 	close_no_err(fd);
@@ -543,76 +540,32 @@ mkhtemp(int *fd,
 	int r;
 	char *end;
 
-	if (fd == NULL ||
-	    template == NULL ||
-	    fname == NULL ||
-	    st_dir_initial == NULL) {
-
-		errno = EFAULT;
-		return -1;
-	}
-
-	/* we do not mess with an
-	   open descriptor.
-	 */
-	if (*fd >= 0) {
-		errno = EEXIST; /* leave their file alone */
-		return -1;
-	}
-
-	if (dirfd < 0) {
-		errno = EBADF;
-		return -1;
-	}
-
-	if (slen(template, max_len, &len) < 0)
-		return -1;
-
-	if (len >= max_len) {
-		errno = EMSGSIZE;
+	if (if_err(fd == NULL || template == NULL || fname == NULL ||
+	          st_dir_initial == NULL, EFAULT) ||
+	    if_err(*fd >= 0, EEXIST) ||
+	    if_err(dirfd < 0, EBADF)
+	      ||
+	    if_err_sys(slen(template, max_len, &len) < 0) ||
+	    if_err(len >= max_len, EMSGSIZE)
+	      ||
+	    if_err_sys(slen(fname, max_len, &fname_len)) ||
+	    if_err(fname == 0, EINVAL) ||
+	    if_err(strrchr(fname, '/') != NULL, EINVAL))
 		return -1;
-	}
-
-	if (slen(fname, max_len, &fname_len) < 0)
-		return -1;
-
-	if (fname_len == 0) {
-		errno = EINVAL;
-		return -1;
-	}
-
-	if (strrchr(fname, '/') != NULL) {
-		errno = EINVAL;
-		return -1;
-	}
 
-	/* count trailing X */
-	end = template + len;
-	while (end > template && *--end == 'X')
-		xc++;
+	for (end = template + len; /* count X */
+	    end > template && *--end == 'X'; xc++);
 
-	if (xc < 6 || xc > len) {
-		errno = EINVAL;
+	if (if_err(xc < 6 || xc > len, EINVAL) ||
+	    if_err(fname_len > len, EOVERFLOW))
 		return -1;
-	}
 
-	if (fname_len > len) {
-		errno = EOVERFLOW;
+	if (if_err(memcmp(fname, template + len - fname_len,
+	      fname_len) != 0, EINVAL))
 		return -1;
-	}
 
-	if (memcmp(fname,
-	    template + len - fname_len,
-	    fname_len) != 0) {
-		errno = EINVAL;
-		return -1;
-	}
-
-	fname_copy = malloc(fname_len + 1);
-	if (fname_copy == NULL) {
-		errno = ENOMEM;
+	if((fname_copy = malloc(fname_len + 1)) == NULL)
 		goto err;
-	}
 
 	/* fname_copy = suffix region only; p points to trailing XXXXXX */
 	memcpy(fname_copy,
@@ -622,15 +575,9 @@ mkhtemp(int *fd,
 
 	for (retries = 0; retries < MKHTEMP_RETRY_MAX; retries++) {
 
-		r = mkhtemp_try_create(
-		    dirfd,
-		    st_dir_initial,
-		    fname_copy,
-		    p,
-		    xc,
-		    fd,
-		    st,
-		    type);
+		r = mkhtemp_try_create(dirfd,
+		    st_dir_initial, fname_copy,
+		    p, xc, fd, st, type);
 
 		if (r == 0) {
 			if (retries >= MKHTEMP_SPIN_THRESHOLD) {
@@ -658,9 +605,7 @@ err:
 	close_no_err(fd);
 
 success:
-
-	if (fname_copy != NULL)
-		free(fname_copy);
+	free_if_null(&fname_copy);
 
 	return (*fd >= 0) ? *fd : -1;
 }
@@ -682,29 +627,21 @@ mkhtemp_try_create(int dirfd,
 	int file_created = 0;
 	int dir_created = 0;
 
-	if (fd == NULL || st == NULL || p == NULL || fname_copy == NULL ||
-	    st_dir_initial == NULL) {
-		errno = EFAULT;
-		goto err;
-	} else if (*fd >= 0) { /* do not mess with someone else's file */
-		errno = EEXIST;
-		goto err;
-	}
-
-	if (mkhtemp_fill_random(p, xc) < 0)
+	if (if_err(fd == NULL || st == NULL || p ==NULL || fname_copy ==NULL ||
+	      st_dir_initial == NULL, EFAULT) ||
+	    if_err(*fd >= 0, EEXIST))
 		goto err;
 
-	if (fd_verify_dir_identity(dirfd, st_dir_initial) < 0)
+	if (if_err_sys(mkhtemp_fill_random(p, xc) < 0) ||
+	    if_err_sys(fd_verify_dir_identity(dirfd, st_dir_initial) < 0))
 		goto err;
 
 	if (type == MKHTEMP_FILE) {
 		*fd = openat2p(dirfd, fname_copy,
 		    O_RDWR | O_CREAT | O_EXCL |
-		    O_NOFOLLOW | O_CLOEXEC | O_NOCTTY,
-		    0600);
+		    O_NOFOLLOW | O_CLOEXEC | O_NOCTTY, 0600);
 
-		/* O_CREAT and O_EXCL guarantees
-		 * creation upon success
+		/* O_CREAT and O_EXCL guarantees creation upon success
 		 */
 		if (*fd >= 0)
 			file_created = 1;
@@ -724,23 +661,15 @@ mkhtemp_try_create(int dirfd,
 		if (fd_verify_dir_identity(dirfd, st_dir_initial) < 0)
 			goto err;
 
-		*fd = openat2p(dirfd, fname_copy,
-		    O_RDONLY | O_DIRECTORY | O_CLOEXEC, 0);
-		if (*fd < 0)
+		if ((*fd = openat2p(dirfd, fname_copy,
+		    O_RDONLY | O_DIRECTORY | O_CLOEXEC, 0)) < 0)
 			goto err;
 
-		if (fstat(*fd, &st_open) < 0)
+		if (if_err_sys(fstat(*fd, &st_open) < 0) ||
+		    if_err(!S_ISDIR(st_open.st_mode), ENOTDIR))
 			goto err;
 
-		if (!S_ISDIR(st_open.st_mode)) {
-			errno = ENOTDIR;
-			goto err;
-		}
-
-		/* NOTE: could check nlink count here,
-		 *       but it's not very reliable here. skipped.
-		 */
-
+		/* NOTE: pointless to check nlink here (only just opened) */
 		if (fd_verify_dir_identity(dirfd, st_dir_initial) < 0)
 			goto err;
 
@@ -776,19 +705,10 @@ mkhtemp_try_create(int dirfd,
 		if (fd_verify_identity(*fd, &st_open, st_dir_initial) < 0)
 			goto err;
 
-		if (!S_ISDIR(st_open.st_mode)) {
-			errno = ENOTDIR;
-			goto err;
-		}
-
-		if (is_owner(&st_open) < 0)
-			goto err;
-
-		/* group/world writeable */
-		if (st_open.st_mode & (S_IWGRP | S_IWOTH)) {
-			errno = EPERM;
+		if (if_err(!S_ISDIR(st_open.st_mode), ENOTDIR) ||
+		    if_err_sys(is_owner(&st_open) < 0) ||
+		    if_err(st_open.st_mode & (S_IWGRP | S_IWOTH), EPERM))
 			goto err;
-		}
 	}
 
 	errno = saved_errno;
@@ -800,7 +720,6 @@ err:
 
 	if (file_created)
 		(void) unlinkat(dirfd, fname_copy, 0);
-
 	if (dir_created)
 		(void) unlinkat(dirfd, fname_copy, AT_REMOVEDIR);
 
@@ -879,6 +798,9 @@ err_mkhtemp_fill_random:
  *	!!! DO NOT RUN TWICE PER FILE. BEWARE OF THE DEMON !!!
  *	               watch out for spikes!
  */
+/* TODO: bad_flags can be negative, and is
+ *       ignored if it is. should we err instead?
+ */
 int secure_file(int *fd,
     struct stat *st,
     struct stat *expected,
@@ -890,93 +812,40 @@ int secure_file(int *fd,
 	int flags;
 	struct stat st_now;
 	int saved_errno = errno;
-	/* you have been warned */
-	if (fd == NULL) {
-		errno = EFAULT;
-		goto err_demons;
-	}
-	if (*fd < 0) {
-		errno = EBADF;
-		goto err_demons;
-	}
-
-	if (st == NULL) {
-		errno = EFAULT;
-		goto err_demons;
-	}
-
-	flags = fcntl(*fd, F_GETFL);
 
-	if (flags == -1)
+	if (if_err(fd == NULL || st == NULL, EFAULT) ||
+	    if_err(*fd < 0, EBADF) ||
+	    if_err_sys((flags = fcntl(*fd, F_GETFL)) == -1) ||
+	    if_err(bad_flags > 0 && (flags & bad_flags), EPERM))
 		goto err_demons;
 
-	if (bad_flags > 0) {
-
-		/* e.g. O_APPEND breaks pwrite/pread
-		 * by allowing offsets to be ignored */
-		if (flags & bad_flags) {
-			errno = EPERM;
-			goto err_demons;
-		}
-	}
-
 	if (expected != NULL) {
 		if (fd_verify_regular(*fd, expected, st) < 0)
 			goto err_demons;
-	} else {
-		if (fstat(*fd, &st_now) == -1)
-			goto err_demons;
-
-		if (!S_ISREG(st_now.st_mode)) {
-			errno = EBADF;
-			goto err_demons;
-		}
-
-		*st = st_now;
-	}
-
-	if (check_seek) {
-
-		/* check if it's seekable */
+	} else if (if_err_sys(fstat(*fd, &st_now) == -1) ||
+	    if_err(!S_ISREG(st_now.st_mode), EBADF)) {
+		goto err_demons; /***********/
+	} else                   /* ( >:3 ) */
+		*st = st_now;    /*  /| |\  */  /* don't let him out */
+				 /*   / \   */
+	if (check_seek) {        /***********/
 		if (lseek(*fd, 0, SEEK_CUR) == (off_t)-1)
 			goto err_demons;
-	}
-
-	/* don't release the demon
-	 */
-	if (st->st_nlink != 1) { /***********/
-		                 /* ( >:3 ) */
-		errno = ELOOP;   /*  /| |\  */  /* don't let him out */
-		goto err_demons; /*   / \   */
-	                         /***********/
-	}
-
-	if (st->st_uid != geteuid() && /* someone else's file */
-	    geteuid() != 0) { /* override for root */
+	} /* don't release the demon */
 
-		errno = EPERM;
-		goto err_demons;
-	}
-	if (is_owner(st) < 0)
+	if (if_err(st->st_nlink != 1, ELOOP) ||
+	    if_err(st->st_uid != geteuid() && geteuid() != 0, EPERM) ||
+	    if_err_sys(is_owner(st) < 0) ||
+	    if_err(st->st_mode & (S_IWGRP | S_IWOTH), EPERM))
 		goto err_demons;
 
-	/* world-writeable or group-ownership.
-	 * if these are set, then others could
-	 * modify the file (not secure)
-	 */
-	if (st->st_mode & (S_IWGRP | S_IWOTH)) {
-		errno = EPERM;
-		goto err_demons;
-	}
-
 	if (do_lock) {
 		if (lock_file(*fd, flags) == -1)
 			goto err_demons;
 
-		if (expected != NULL) {
+		if (expected != NULL)
 			if (fd_verify_identity(*fd, expected, &st_now) < 0)
 				goto err_demons;
-		}
 	}
 
 	if (fchmod(*fd, mode) == -1)
@@ -997,16 +866,12 @@ int
 fd_verify_regular(int fd,
     const struct stat *expected,
     struct stat *out)
-{
-	if (fd_verify_identity(fd, expected, out) < 0)
-		return -1;
-
-	if (!S_ISREG(out->st_mode)) {
-		errno = EBADF;
-		return -1;
-	}
-
-	return 0; /* regular file */
+{if (
+	if_err_sys(fd_verify_identity(fd, expected, out) < 0) ||
+	    if_err(!S_ISREG(out->st_mode), EBADF)
+	)	return -1;
+	else
+		return 0; /* regular file */
 }
 
 int
@@ -1017,20 +882,12 @@ fd_verify_identity(int fd,
 	struct stat st_now;
 	int saved_errno = errno;
 
-	if (fd < 0 || expected == NULL) {
-		errno = EFAULT;
-		return -1;
-	}
-
-	if (fstat(fd, &st_now) < 0)
+if(	if_err(fd < 0 || expected == NULL, EFAULT) ||
+	  if_err_sys(fstat(fd, &st_now)) ||
+	    if_err(st_now.st_dev != expected->st_dev ||
+	      st_now.st_ino != expected->st_ino, ESTALE))
 		return -1;
 
-	if (st_now.st_dev != expected->st_dev ||
-	    st_now.st_ino != expected->st_ino) {
-		errno = ESTALE;
-		return -1;
-	}
-
 	if (out != NULL)
 		*out = st_now;
 
@@ -1045,12 +902,8 @@ fd_verify_dir_identity(int fd,
 	struct stat st_now;
 	int saved_errno = errno;
 
-	if (fd < 0 || expected == NULL) {
-		errno = EFAULT;
-		return -1;
-	}
-
-	if (fstat(fd, &st_now) < 0)
+	if (if_err(fd < 0 || expected == NULL, EFAULT) ||
+	    if_err_sys(fstat(fd, &st_now) < 0))
 		return -1;
 
 	if (st_now.st_dev != expected->st_dev ||
@@ -1096,15 +949,9 @@ lock_file(int fd, int flags)
 	struct flock fl;
 	int saved_errno = errno;
 
-	if (fd < 0) {
-		errno = EBADF;
+	if (if_err(fd < 0, EBADF) ||
+	    if_err(flags < 0, EINVAL))
 		goto err_lock_file;
-	}
-
-	if (flags < 0) {
-		errno = EINVAL;
-		goto err_lock_file;
-	}
 
 	memset(&fl, 0, sizeof(fl));