comment

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

util/libreboot-utils/include/common.h

@@ -530,6 +530,9 @@ int fsync_on_eintr(int fd);
 int fs_rename_at(int olddirfd, const char *old,
              int newdirfd, const char *new);
 int fs_open(const char *path, int flags);
+void close_no_err(int *fd);
+void free_if_null(char **buf);
+int close_warn(int *fd, char *s);
 struct filesystem *rootfs(void);
 int fs_resolve_at(int dirfd, const char *path, int flags);
 int fs_next_component(const char **p,
@@ -542,6 +545,8 @@ int openat2p(int dirfd, const char *path,
     int flags, mode_t mode);
 int mkdirat_on_eintr(int dirfd, 
     const char *pathname, mode_t mode);
+int if_err(int condition, int errval);
+int if_err_sys(int condition);
 
 /* asserts */
 

util/libreboot-utils/lib/command.c

@@ -127,6 +127,7 @@ set_cmd(int argc, char *argv[])
 		    "Too few args on command '%s'", cmd);
 	}
 
+
 	x->no_cmd = 1;
 }
 

util/libreboot-utils/lib/file.c

@@ -1,11 +1,12 @@
 /* SPDX-License-Identifier: MIT
  * Copyright (c) 2026 Leah Rowe <leah@libreboot.org>
  *
- * Pathless i/o, and some stuff you probably never saw.
+ * Pathless i/o, and some stuff you
+ * probably never saw in userspace.
+ *
  * Be nice to the demon.
  */
 
-
 #include <sys/types.h>
 #include <sys/stat.h>
 
@@ -134,31 +135,13 @@ fsync_dir(const char *path)
 	maxlen = 4096;
 #endif
 
-	if (path == NULL) {
-		errno = EFAULT;
+	if (if_err(path == NULL, EFAULT) ||
+	    if_err_sys(slen(path, maxlen, &pathlen) < 0) ||
+	    if_err(pathlen >= maxlen || pathlen < 0, EMSGSIZE) ||
+	    if_err(pathlen == 0, EINVAL)
+	    ||
+	    if_err_sys((dirbuf = malloc(pathlen + 1)) == NULL))
 		goto err_fsync_dir;
-	}
-
-	if (slen(path, maxlen, &pathlen) < 0)
-		goto err_fsync_dir;
-
-	if (pathlen >= maxlen || pathlen < 0) {
-		errno = EMSGSIZE;
-		goto err_fsync_dir;
-	}
-
-	if (pathlen == 0)
-	{
-		errno = EINVAL;
-		goto err_fsync_dir;
-	}
-
-	dirbuf = malloc(pathlen + 1);
-	if (dirbuf == NULL) {
-
-		errno = ENOMEM;
-		goto err_fsync_dir;
-	}
 
 	memcpy(dirbuf, path, pathlen + 1);
 	slash = strrchr(dirbuf, '/');
@@ -183,20 +166,12 @@ fsync_dir(const char *path)
 	    | O_NOFOLLOW
 #endif
 );
-	if (dirfd < 0)
-		goto err_fsync_dir;
 
-	if (fstat(dirfd, &st) < 0)
-		goto err_fsync_dir;
-
-	if (!S_ISDIR(st.st_mode)) {
-
-		errno = ENOTDIR;
-		goto err_fsync_dir;
-	}
-
-	/* sync file on disk */
-	if (fsync_on_eintr(dirfd) == -1)
+	if (if_err_sys(dirfd < 0) ||
+	    if_err_sys(fstat(dirfd, &st) < 0) ||
+	    if_err(!S_ISDIR(st.st_mode), ENOTDIR)
+	    ||
+	    if_err_sys(fsync_on_eintr(dirfd) == -1))
 		goto err_fsync_dir;
 
 	if (close_on_eintr(dirfd) == -1) {
@@ -205,13 +180,7 @@ fsync_dir(const char *path)
 		goto err_fsync_dir;
 	}
 
-	if (dirbuf != NULL) {
-
-		free(dirbuf);
-		dirbuf = NULL;
-	}
-
-	dirbuf = NULL;
+	free_if_null(&dirbuf);
 
 	errno = saved_errno;
 	return 0;
@@ -221,19 +190,8 @@ err_fsync_dir:
 	if (errno == saved_errno)
 		errno = EIO;
 
-	if (dirbuf != NULL) {
-
-		free(dirbuf);
-		dirbuf = NULL;
-	}
-
-	if (dirfd >= 0) {
-
-		close_errno = errno;
-		(void) close_on_eintr(dirfd);
-		errno = close_errno;
-		dirfd = -1;
-	}
+	free_if_null(&dirbuf);
+	close_no_err(&dirfd);
 
 	return -1;
 }
@@ -545,57 +503,19 @@ io_args(int fd, void *mem, size_t nrw,
 {
 	int saved_errno = errno;
 
-	/* obviously */
-	if (mem == NULL) {
-
-		errno = EFAULT;
+	if (if_err(mem == NULL, EFAULT) ||
+	    if_err(fd < 0, EBADF) ||
+	    if_err(off < 0, ERANGE) ||
+	    if_err(!nrw, EPERM) || /* TODO: toggle zero-byte check */
+	    if_err(nrw > (size_t)SSIZE_MAX, ERANGE) ||
+	    if_err(((size_t)off + nrw) < (size_t)off, ERANGE) ||
+	    if_err(rw_type > IO_PWRITE, EINVAL))
 		goto err_io_args;
-	}
-
-	/* uninitialised fd */
-	if (fd < 0) {
-
-		errno = EBADF;
-		goto err_io_args;
-	}
-
-	/* negative offset */
-	if (off < 0) {
-
-		errno = ERANGE;
-		goto err_io_args;
-	}
-
-	/* prevent zero-byte rw */
-	if (!nrw)
-		goto err_io_args;
-
-	/* prevent overflow */
-	if (nrw > (size_t)SSIZE_MAX) {
-
-		errno = ERANGE;
-		goto err_io_args;
-	}
-
-	/* prevent overflow */
-	if (((size_t)off + nrw) < (size_t)off) {
-
-		errno = ERANGE;
-		goto err_io_args;
-	}
-
-	if (rw_type > IO_PWRITE) {
-
-		errno = EINVAL;
-		goto err_io_args;
-	}
 
 	errno = saved_errno;
-
 	return 0;
 
 err_io_args:
-
 	if (errno == saved_errno)
 		errno = EINVAL;
 
@@ -607,31 +527,16 @@ check_file(int fd, struct stat *st)
 {
 	int saved_errno = errno;
 
-	if (fd < 0) {
-		errno = EBADF;
+	if (if_err(fd < 0, EBADF) ||
+	    if_err(st == NULL, EFAULT) ||
+	    if_err(fstat(fd, st) == -1, 0) ||
+	    if_err(!S_ISREG(st->st_mode), EBADF))
 		goto err_is_file;
-	}
-
-	if (st == NULL) {
-		errno = EFAULT;
-		goto err_is_file;
-	}
-
-	if (fstat(fd, st) == -1)	
-		goto err_is_file;
-
-	if (!S_ISREG(st->st_mode)) {
-
-		errno = EBADF;
-		goto err_is_file;
-	}
 
 	errno = saved_errno;
-
 	return 0;
 
 err_is_file:
-
 	if (errno == saved_errno)
 		errno = EINVAL;
 
@@ -647,54 +552,16 @@ rw_over_nrw(ssize_t r, size_t nrw)
 {
 	int saved_errno = errno;
 
-	/* not a libc bug, but we
-	 * don't like the number zero
-	 */
-	if (!nrw)
+	if (if_err(!nrw, 0) ||
+	    if_err(r == -1, 0) ||
+	    if_err((size_t)r > SSIZE_MAX, ERANGE) ||
+	    if_err((size_t)r > nrw, ERANGE))
 		goto err_rw_over_nrw;
 
-	if (r == -1)
-		return r;
-
-	if ((size_t)
-	    r > SSIZE_MAX) {
-
-		/* Theoretical buggy libc
-		 * check. Extremely academic.
-		 *
-		 * Specifications never
-		 * allow this return value
-		 * to exceed SSIZE_T, but
-		 * spec != implementation
-		 *
-		 * Check this after using
-		 * [p]read() or [p]write()
-		 *
-		 * NOTE: here, we assume
-		 * ssize_t integers are the
-		 * same size as SSIZE_T
-		 */
-
-		errno = ERANGE;
-		goto err_rw_over_nrw;
-	}
-
-	/* Theoretical buggy libc:
-	 * Should never return a number of
-	 * bytes above the requested length.
-	 */
-	if ((size_t)r > nrw) {
-
-		errno = ERANGE;
-		goto err_rw_over_nrw;
-	}
-
 	errno = saved_errno;
-
 	return r;
 
 err_rw_over_nrw:
-
 	if (errno == saved_errno)
 		errno = EIO;
 
@@ -723,15 +590,102 @@ lseek_on_eintr(int fd, off_t off, int whence,
 }
 #endif
 
+/* two functions that reduce sloccount by
+ * two hundred lines... no, now three. */
+int
+if_err(int condition, int errval)
+{
+	if (!condition)
+		return 0;
+
+	if (errval)
+		errno = errval;
+
+	return 1;
+}
+/* technically pointless, but stylistically
+ * pleasing alongside if_err chains.
+ * use this one for syscalls that are
+ * expected to set errno
+ * also use it for non-system calls
+ * that act like them, e.g. prw() or
+ * rw_write_exact() */
+int
+if_err_sys(int condition)
+{
+	if (!condition)
+		return 0;
+	return 1;
+}
+/* errno can never be -1, so you can
+ * use this to conditionally set an integer
+ * for comparison. see example in lseek_on_eintr
+ */
 int
 try_err(int loop_err, int errval)
 {
 	if (loop_err)
 		return errval;
-
 	return -1;
 }
 
+void
+free_if_null(char **buf)
+{
+	if (buf == NULL || *buf == NULL)
+		return;
+
+	free(*buf);
+	*buf = NULL;
+}
+
+/* also returns error code */
+int
+close_warn(int *fd, char *s)
+{
+	int saved_errno = errno;
+
+	if (fd == NULL) {
+		if (s != NULL)
+			fprintf(stderr, "FAIL: %s: bad fd ptr\n", s);
+		return -1;
+	}
+
+	if (*fd < 0 && s != NULL) {
+		fprintf(stderr, "WARN: %s: already closed\n", s);
+	} else if (close(*fd) < 0) {
+		if (s != NULL)
+			fprintf(stderr, "FAIL: %s: close\n", s);
+		return -1;
+	}
+
+	*fd = -1;
+	errno = saved_errno;
+
+	return 0;
+}
+
+/* TODO: remove this. giant liability.
+   make close calls always err instead,
+   when they fail. otherwise we hide bugs!
+ */
+void
+close_no_err(int *fd)
+{
+	int saved_errno = errno;
+
+	if (fd == NULL || *fd < 0)
+		return;
+
+	(void) close_on_eintr(*fd);
+	*fd = -1;
+
+	errno = saved_errno;
+}
+
+/* TODO: make fd a pointer insttead
+	and automatically reset -1 here */
+/* BUT DO NOT reset -1 on error */
 int
 close_on_eintr(int fd)
 {
@@ -771,17 +725,9 @@ int
 fs_rename_at(int olddirfd, const char *old,
     int newdirfd, const char *new)
 {
-	if (new == NULL || old == NULL) {
-
-		errno = EFAULT;
+	if (if_err(new == NULL || old == NULL, EFAULT) ||
+	    if_err(olddirfd < 0 || newdirfd < 0, EBADF))
 		return -1;
-	}
-
-	if (olddirfd < 0 || newdirfd < 0) {
-
-		errno = EBADF;
-		return -1;
-	}
 
 	return renameat(olddirfd, old, newdirfd, new);
 }
@@ -796,25 +742,13 @@ int
 fs_open(const char *path, int flags)
 {
 	struct filesystem *fs;
-	const char *rel;
 
-	if (path == NULL) {
-		errno = EFAULT;
-		return -1;
-	}
-
-	if (path[0] != '/') {
-		errno = EINVAL;
-		return -1;
-	}
-
-	fs = rootfs();
-	if (fs == NULL)
+	if (if_err(path == NULL, EFAULT) ||
+	    if_err(path[0] != '/', EINVAL) ||
+	    if_err_sys((fs = rootfs()) == NULL))
 		return -1;
 
-	rel = path + 1;
-
-	return fs_resolve_at(fs->rootfd, rel, flags);
+	return fs_resolve_at(fs->rootfd, path + 1, flags);
 }
 
 /* singleton function
@@ -964,12 +898,8 @@ fs_open_component(int dirfd, const char *name,
 	 */
 	if (!is_last) {
 
-		if (fd < 0) {
-			errno = EBADF;
-			return -1;
-		}
-
-		if (fstat(fd, &st) < 0)
+		if (if_err(fd < 0, EBADF) ||
+		    if_err_sys(fstat(fd, &st) < 0))
 			return -1;
 
 		if (!S_ISDIR(st.st_mode)) {
@@ -999,14 +929,9 @@ fs_dirname_basename(const char *path,
 	size_t maxlen = 4096;
 #endif
 
-	if (path == NULL || dir == NULL || base == NULL)
-		return -1;
-
-	if (slen(path, maxlen, &len) < 0)
-		return -1;
-
-	buf = malloc(len + 1);
-	if (buf == NULL)
+	if (path == NULL || dir == NULL || base == NULL ||
+	    if_err_sys(slen(path, maxlen, &len) < 0) ||
+	    if_err_sys((buf = malloc(len + 1)) == NULL))
 		return -1;
 
 	memcpy(buf, path, len + 1);
@@ -1033,7 +958,7 @@ fs_dirname_basename(const char *path,
 		*base = buf;
 	} else {
 		errno = EINVAL;
-		free(buf);
+		free_if_null(&buf);
 		return -1;
 	}
 
@@ -1063,15 +988,9 @@ openat2p(int dirfd, const char *path,
 	int rval;
 #endif
 
-	if (dirfd < 0) {
-		errno = EBADF;
+	if (if_err(dirfd < 0, EBADF) ||
+	    if_err(path == NULL, EFAULT))
 		return -1;
-	}
-
-	if (path == NULL) {
-		errno = EFAULT;
-		return -1;
-	}
 
 retry:
 	errno = 0;
@@ -1108,15 +1027,9 @@ mkdirat_on_eintr( /* <-- say that 10 times to please the demon */
 	int saved_errno = errno;
 	int rval;
 
-	if (dirfd < 0) {
-		errno = EBADF;
+	if (if_err(dirfd < 0, EBADF) ||
+	    if_err(path == NULL, EFAULT))
 		return -1;
-	}
-
-	if (path == NULL) {
-		errno = EFAULT;
-		return -1;
-	}
 
 retry:
 	errno = 0;
@@ -1134,3 +1047,14 @@ retry:
 
 	return rval;
 }
+
+
+
+
+
+
+
+
+
+
+

util/libreboot-utils/lib/io.c

@@ -239,21 +239,8 @@ write_to_gbe_bin(void)
 
 	saved_errno = errno;
 
-	if (close_on_eintr(f->tmp_fd) == -1) {
-		f->tmp_fd = -1;
-
-		fprintf(stderr, "FAIL: %s: close\n", f->tname);
-		f->io_err_gbe_bin = 1;
-	}
-	f->tmp_fd = -1;
-
-	if (close_on_eintr(f->gbe_fd) == -1) {
-		f->gbe_fd = -1;
-
-		fprintf(stderr, "FAIL: %s: close\n", f->fname);
-		f->io_err_gbe_bin = 1;
-	}
-	f->gbe_fd = -1;
+	f->io_err_gbe_bin |= -close_warn(&f->tmp_fd, f->tname);
+	f->io_err_gbe_bin |= -close_warn(&f->gbe_fd, f->fname);
 
 	errno = saved_errno;
 
@@ -279,13 +266,7 @@ write_to_gbe_bin(void)
 
 			/* removed by rename
 			 */
-
-			if (f->tname != NULL) {
-				free(f->tname);
-				f->tname = NULL;
-			}
-
-			f->tname = NULL;
+			free_if_null(&f->tname);
 		}
 	}
 
@@ -513,12 +494,7 @@ gbe_mv(void)
 		goto ret_gbe_mv;
 	}
 
-	if (dest_tmp != NULL) {
-		free(dest_tmp);
-		dest_tmp = NULL;
-	}
-
-	dest_tmp = NULL;
+	free_if_null(&dest_tmp);
 
 ret_gbe_mv:
 

util/libreboot-utils/lib/mkhtemp.c

@@ -39,6 +39,21 @@ new_tmpdir(int *fd, char **path)
 	return new_tmp_common(fd, path, MKHTEMP_DIR);
 }
 
+/* WARNING:
+ * on error, *path (at **path) may be
+   NULL, or if the error pertains to
+   an actual TMPDIR, set. if set, it
+   will be using *static* memory and
+   must not be freed. on success,
+   a pointer to heap memory is set
+   instead.
+ * see:
+ * env_tmpdir()
+ * this is for error reports if e.g.
+ * TMPDIR isn't found (but is set)
+ * if TMPDIR isn't set, it will
+ * default to /tmp or /var/tmp
+ */
 int
 new_tmp_common(int *fd, char **path, int type)
 {
@@ -53,7 +68,6 @@ new_tmp_common(int *fd, char **path, int type)
 	char suffix[] = "tmp.XXXXXXXXXX";
 	char *tmpdir = NULL;
 
-	int close_errno;
 	size_t dirlen;
 	size_t destlen;
 	char *dest = NULL; /* final path (will be written into "path") */
@@ -140,12 +154,7 @@ new_tmp_common(int *fd, char **path, int type)
 	if (*fd < 0)
 		goto err;
 
-	if (dirfd >= 0) {
-		close_errno = errno;
-		(void) close_on_eintr(dirfd);
-		errno = close_errno;
-		dirfd = -1;
-	}
+	close_no_err(&dirfd);
 
 	errno = saved_errno;
 	*path = dest;
@@ -159,24 +168,10 @@ err:
 	else
 		saved_errno = errno = EIO;
 
-	if (dest != NULL) {
-		free(dest);
-		dest = NULL;
-	}
+	free_if_null(&dest);
 
-	if (dirfd >= 0) {
-		close_errno = errno;
-		(void) close_on_eintr(dirfd);
-		errno = close_errno;
-		dirfd = -1;
-	}
-
-	if (*fd >= 0) {
-		close_errno = errno;
-		(void) close_on_eintr(*fd);
-		errno = close_errno;
-		*fd = -1;
-	}
+	close_no_err(&dirfd);
+	close_no_err(fd);
 
 	/* where a TMPDIR isn't found, and we err,
 	 * we pass this back through for the
@@ -338,8 +333,8 @@ same_dir(const char *a, const char *b)
 	if (st_a.st_dev == st_b.st_dev &&
 	    st_a.st_ino == st_b.st_ino) {
 
-		(void) close_on_eintr(fd_a);
-		(void) close_on_eintr(fd_b);
+		close_no_err(&fd_a);
+		close_no_err(&fd_b);
 
 success_same_dir:
 
@@ -350,8 +345,8 @@ success_same_dir:
 		return 1;
 	}
 
-	(void) close_on_eintr(fd_a);
-	(void) close_on_eintr(fd_b);
+	close_no_err(&fd_a);
+	close_no_err(&fd_b);
 
 	/* FAILURE (logical)
 	 */
@@ -364,10 +359,8 @@ err_same_dir:
 	/* FAILURE (probably syscall)
 	 */
 
-	if (fd_a >= 0)
-		(void) close_on_eintr(fd_a);
-	if (fd_b >= 0)
-		(void) close_on_eintr(fd_b);
+	close_no_err(&fd_a);
+	close_no_err(&fd_b);
 
 	if (errno == saved_errno)
 		errno = EIO;
@@ -457,9 +450,7 @@ world_writeable_and_sticky(
 sticky_heaven:
 /* i like the one in hamburg better */
 
-	if (dirfd >= 0)
-		(void) close_on_eintr(dirfd);
-
+	close_no_err(&dirfd);
 	errno = saved_errno;
 
 	return 1;
@@ -471,8 +462,7 @@ sticky_hell:
 
 	saved_errno = errno;
 
-	if (dirfd >= 0)
-		(void) close_on_eintr(dirfd);
+	close_no_err(&dirfd);
 
 	errno = saved_errno;
 
@@ -550,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)
+	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 (len >= max_len) {
-		errno = EMSGSIZE;
-		return -1;
-	}
+	for (end = template + len; /* count X */
+	    end > template && *--end == 'X'; xc++);
 
-	if (slen(fname, max_len, &fname_len) < 0)
+	if (if_err(xc < 6 || xc > len, EINVAL) ||
+	    if_err(fname_len > len, EOVERFLOW))
 		return -1;
 
-	if (fname_len == 0) {
-		errno = EINVAL;
+	if (if_err(memcmp(fname, template + len - fname_len,
+	      fname_len) != 0, EINVAL))
 		return -1;
-	}
 
-	if (strrchr(fname, '/') != NULL) {
-		errno = EINVAL;
-		return -1;
-	}
-
-	/* count trailing X */
-	end = template + len;
-	while (end > template && *--end == 'X')
-		xc++;
-
-	if (xc < 6 || xc > len) {
-		errno = EINVAL;
-		return -1;
-	}
-
-	if (fname_len > len) {
-		errno = EOVERFLOW;
-		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,
@@ -629,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) {
@@ -662,17 +602,10 @@ mkhtemp(int *fd,
 	errno = EEXIST;
 
 err:
-	if (*fd >= 0) {
-		close_errno = errno;
-		(void)close_on_eintr(*fd);
-		errno = close_errno;
-		*fd = -1;
-	}
+	close_no_err(fd);
 
 success:
-
-	if (fname_copy != NULL)
-		free(fname_copy);
+	free_if_null(&fname_copy);
 
 	return (*fd >= 0) ? *fd : -1;
 }
@@ -689,35 +622,26 @@ mkhtemp_try_create(int dirfd,
 {
 	struct stat st_open;
 	int saved_errno = errno;
-	int close_errno;
 	int rval = -1;
 
 	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;
@@ -737,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;
 
@@ -789,40 +705,24 @@ 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;
+		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;
 	}
 
-		if (is_owner(&st_open) < 0)
-			goto err;
-
-		/* group/world writeable */
-		if (st_open.st_mode & (S_IWGRP | S_IWOTH)) {
-			errno = EPERM;
-			goto err;
-		}
-	}
-
 	errno = saved_errno;
 	rval = 1;
 	goto out;
 
 err:
-	close_errno = errno;
-
-	if (fd != NULL && *fd >= 0) {
-		(void) close_on_eintr(*fd);
-		*fd = -1;
-	}
+	close_no_err(fd);
 
 	if (file_created)
 		(void) unlinkat(dirfd, fname_copy, 0);
-
 	if (dir_created)
 		(void) unlinkat(dirfd, fname_copy, AT_REMOVEDIR);
 
-	errno = close_errno;
 	rval = -1;
 out:
 	return rval;
@@ -898,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,
@@ -909,94 +812,41 @@ 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;
+	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;
-	}
-
-	flags = fcntl(*fd, F_GETFL);
-
-	if (flags == -1)
-		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 */
 
-	/* 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 */
-
-		errno = EPERM;
+	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;
-	}
-	if (is_owner(st) < 0)
-		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)
 		goto err_demons;
@@ -1016,15 +866,11 @@ 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;
-	}
-
+{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 */
 }
 
@@ -1036,19 +882,11 @@ fd_verify_identity(int fd,
 	struct stat st_now;
 	int saved_errno = errno;
 
-	if (fd < 0 || expected == NULL) {
-		errno = EFAULT;
+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 (fstat(fd, &st_now) < 0)
-		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;
@@ -1064,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 ||
@@ -1115,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));
 

util/libreboot-utils/lib/num.c

@@ -257,12 +257,9 @@ retry_urandom_read:
 		nr = new_nr;
 		off = 0;
 
-		/* to mitigate file descriptor
-		 * injection, we do not re-use
-		 * the same descriptor each time
-		 */
-		(void) close_on_eintr(fd);
-		fd = -1;
+		/* don't re-use same fd, to mitaget
+		 * fd injection */
+		close_no_err(&fd);
 	}
 
 	fd = -1;

util/libreboot-utils/lib/state.c

@@ -26,11 +26,11 @@ struct xstate *
 xstart(int argc, char *argv[])
 {
 	static int first_run = 1;
-	static int pre_init = 0;
 
 	static struct xstate us = {
-	/* DO NOT MESS THIS UP, OR THERE WILL BE DEMONS */
 	{
+		/* be careful when modifying xstate. you
+		 * must set everything precisely */
 	{
 		CMD_DUMP, "dump", cmd_helper_dump, ARGC_3,
 		ARG_NOPART,
@@ -89,15 +89,8 @@ xstart(int argc, char *argv[])
 
 	};
 
-	if (!first_run) {
-		if (pre_init)
-			err_no_cleanup(ECANCELED,
-			    "Outside access to state during init");
-
-		first_run = 0;
-
+	if (!first_run)
 		return &us;
-	}
 
 	if (argc < 3)
 		err_no_cleanup(EINVAL, "xstart: Too few arguments");
@@ -105,20 +98,17 @@ xstart(int argc, char *argv[])
 		err_no_cleanup(EINVAL, "xstart: NULL argv");
 	
 	first_run = 0;
-	pre_init = 1;
 
 	us.f.buf = us.f.real_buf;
 
 	us.argv0 = argv[0];
 	us.f.fname = argv[1];
 
-	if (new_tmpfile(&us.f.tmp_fd, &us.f.tname) < 0)
-		err_no_cleanup(errno, "xstart: cannot create tmpfile");
+	us.f.tmp_fd = -1;
+	us.f.tname = NULL;
 
-	/* parse user command */
-/* TODO: CHECK ACCESSES VIA xstatus() */
-	set_cmd(argc, argv);
-	set_cmd_args(argc, argv);
+	if (new_tmpfile(&us.f.tmp_fd, &us.f.tname) < 0)
+		err_no_cleanup(errno, "%s", us.f.tname);
 
 	if (us.f.tname == NULL)
 		err_no_cleanup(errno, "x->f.tname null");
@@ -134,8 +124,6 @@ xstart(int argc, char *argv[])
 	/* for good measure */
 	memset(us.f.pad, 0, sizeof(us.f.pad));
 
-	pre_init = 0;
-
 	return &us;
 }
 
@@ -147,8 +135,6 @@ xstatus(void)
 	if (x == NULL)
 		err_no_cleanup(EACCES, "NULL pointer to xstate");
 
-	sanitize_command_list();
-
 	return x;
 }
 
@@ -189,10 +175,11 @@ getnvmprogname(void)
 	char *rval = fallback;
 
 	if (x != NULL) {
-		if (x->argv0 == NULL || *x->argv0 == '\0')
-			return "";
 
+		if (x->argv0 != NULL && *x->argv0 != '\0')
 			rval = x->argv0;
+		else
+			return fallback;
 	}
 
 	p = strrchr(rval, '/');
@@ -218,30 +205,15 @@ exit_cleanup(void)
 	if (x != NULL) {
 		f = &x->f;
 
-		if (f->gbe_fd > -1) {
-			if (close_on_eintr(f->gbe_fd) == -1) {
-				f->gbe_fd = -1;
-				close_err = 1;
-			}
-			f->gbe_fd = -1;
-		}
+		close_no_err(&f->gbe_fd);
+		close_no_err(&f->tmp_fd);
+		close_no_err(&f->tmp_fd);
 
-		if (f->tmp_fd > -1) {
-			if (close_on_eintr(f->tmp_fd) == -1) {
-				f->tmp_fd = -1;
-				close_err = 1;
-			}
-			f->tmp_fd = -1;
-		}
-
-		if (f->tname != NULL) {
+		if (f->tname != NULL)
 			if (unlink(f->tname) == -1)
 				close_err = 1;
 	}
 
-		f->tmp_fd = -1;
-	}
-
 	if (saved_errno)
 		errno = saved_errno;
 

util/libreboot-utils/nvmutil.c

@@ -62,6 +62,11 @@ main(int argc, char *argv[])
 	if (x == NULL)
 		err_no_cleanup(ECANCELED, "NULL state on init");
 
+	/* parse user command */
+/* TODO: CHECK ACCESSES VIA xstatus() */
+	set_cmd(argc, argv);
+	set_cmd_args(argc, argv);
+
 	cmd = &x->cmd[x->i];
 	f = &x->f;
 
@@ -96,6 +101,9 @@ main(int argc, char *argv[])
 	if (cmd->run == NULL)
 		err(errno, "Command not set");
 
+
+	sanitize_command_list();
+
 	open_gbe_file();
 
 	copy_gbe();
@@ -115,10 +123,7 @@ main(int argc, char *argv[])
 	if (f->io_err_gbe_bin)
 		err(EIO, "%s: error writing final file");
 
-	if (f->tname != NULL) {
-		free(f->tname);
-		f->tname = NULL;
-	}
+	free_if_null(&f->tname);
 
 	return EXIT_SUCCESS;
 }