libreboot-utils: extremely safe(ish) malloc usage

yes, a common thing in C programs is one or all
of the following:

* use after frees
* double free (on non-NULL pointer)
* over-writing currently used pointer (mem leak)

i try to reduce the chance of this in my software,
by running free() through a filter function,
free_if_not_null, that returns if a function
is being freed twice - because it sets NULL
after freeing, but will only free if it's not
null already.

this patch adds two functions: smalloc and vmalloc,
for strings and voids. using these makes the program
abort if:

* non-null pointer given for initialisation
* pointer to pointer is null (of course)
* size of zero given, for malloc (zero bytes)

i myself was caught out by this change, prompting
me to make the following fix in fs_dirname_basename()
inside lib/file.c:

-       char *buf;
+       char *buf = NULL;

Yes.

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

5 files changed, 75 insertions, 60 deletions

diff --git a/util/libreboot-utils/include/common.h b/util/libreboot-utils/include/common.h
index 77672846..652b8c08 100644
--- a/util/libreboot-utils/include/common.h
+++ b/util/libreboot-utils/include/common.h
@@ -375,6 +375,8 @@ void write_mac_part(size_t partnum);
 
 int xunveilx(const char *path, const char *permissions);
 int xpledgex(const char *promises, const char *execpromises);
+char *smalloc(char **buf, size_t size);
+void *vmalloc(void **buf, size_t size);
 int slen(const char *scmp, size_t maxlen,
     size_t *rval);
 int scmp(const char *a, const char *b,
@@ -393,7 +395,6 @@ int dcat(const char *s, size_t n,
 
 unsigned short hextonum(char ch_s);
 void *mkrbuf(size_t n);
-void *rmalloc(size_t *size); /* don't ever use this */
 void rset(void *buf, size_t n);
 void *mkrbuf(size_t n);
 char *mkrstr(size_t n);
diff --git a/util/libreboot-utils/lib/file.c b/util/libreboot-utils/lib/file.c
index 3ca50889..3b3e57d8 100644
--- a/util/libreboot-utils/lib/file.c
+++ b/util/libreboot-utils/lib/file.c
@@ -111,12 +111,11 @@ fsync_dir(const char *path)
 	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))
+	    if_err(pathlen == 0, EINVAL))
 		goto err_fsync_dir;
 
-	memcpy(dirbuf, path, pathlen + 1);
+	memcpy(smalloc(&dirbuf, pathlen + 1),
+	    path, pathlen + 1);
 	slash = strrchr(dirbuf, '/');
 
 	if (slash != NULL) {
@@ -862,7 +861,7 @@ fs_dirname_basename(const char *path,
     char **dir, char **base,
     int allow_relative)
 {
-	char *buf;
+	char *buf = NULL;
 	char *slash;
 	size_t len;
 	int rval;
@@ -874,11 +873,10 @@ fs_dirname_basename(const char *path,
 #endif
 
 	if (path == NULL || dir == NULL || base == NULL ||
-	    if_err_sys(slen(path, maxlen, &len) < 0) ||
-	    if_err_sys((buf = malloc(len + 1)) == NULL))
+	    if_err_sys(slen(path, maxlen, &len) < 0))
 		return -1;
 
-	memcpy(buf, path, len + 1);
+	memcpy(smalloc(&buf, len + 1), path, len + 1);
 
 	/* strip trailing slashes */
 	while (len > 1 && buf[len - 1] == '/')
diff --git a/util/libreboot-utils/lib/mkhtemp.c b/util/libreboot-utils/lib/mkhtemp.c
index c913ce6c..e499de34 100644
--- a/util/libreboot-utils/lib/mkhtemp.c
+++ b/util/libreboot-utils/lib/mkhtemp.c
@@ -144,13 +144,7 @@ new_tmp_common(int *fd, char **path, int type,
 		goto err;
 	}
 
-	dest = malloc(destlen + 1);
-	if (dest == NULL) {
-		errno = ENOMEM;
-		goto err;
-	}
-
-	memcpy(dest, tmpdir, dirlen);
+	memcpy(smalloc(&dest, destlen + 1), tmpdir, dirlen);
 	*(dest + dirlen) = '/';
 	memcpy(dest + dirlen + 1, templatestr, templatestr_len);
 	*(dest + destlen) = '\0';
@@ -585,11 +579,8 @@ mkhtemp(int *fd,
 	      fname_len) != 0, EINVAL))
 		return -1;
 
-	if((fname_copy = malloc(fname_len + 1)) == NULL)
-		goto err;
-
 	/* fname_copy = templatestr region only; p points to trailing XXXXXX */
-	memcpy(fname_copy,
+	memcpy(smalloc(&fname_copy, fname_len + 1),
 	    template + len - fname_len,
 	    fname_len + 1);
 	p = fname_copy + fname_len - xc;
diff --git a/util/libreboot-utils/lib/rand.c b/util/libreboot-utils/lib/rand.c
index 10831e44..863ace17 100644
--- a/util/libreboot-utils/lib/rand.c
+++ b/util/libreboot-utils/lib/rand.c
@@ -72,13 +72,6 @@
  * or your program dies.
  */
 
-void *
-rmalloc(size_t *rval)
-{
-	return if_err(rval == NULL, EFAULT) ?
-	    NULL : mkrstr(*rval = rsize(BUFSIZ));
-}
-
 size_t
 rsize(size_t n)
 {
@@ -120,17 +113,7 @@ void *
 mkrbuf(size_t n)
 {
 	void *buf = NULL;
-
-	if (n == 0)
-		err_no_cleanup(0, EPERM, "mkrbuf: zero-byte request");
-
-	if (n >= SIZE_MAX - 1)
-		err_no_cleanup(0, EOVERFLOW, "integer overflow in mkrbuf");
-
-	if ((buf = malloc(n)) == NULL)
-		err_no_cleanup(0, ENOMEM, "mkrbuf: malloc");
-
-	rset(buf, n);
+	rset(vmalloc(&buf, n), n);
 	return buf; /* basically malloc() but with rand */
 }
 
diff --git a/util/libreboot-utils/lib/string.c b/util/libreboot-utils/lib/string.c
index c6e09752..986c7b8e 100644
--- a/util/libreboot-utils/lib/string.c
+++ b/util/libreboot-utils/lib/string.c
@@ -19,6 +19,51 @@
 
 #include "../include/common.h"
 
+/* safe(ish) malloc.
+
+   use this and free_and_set_null()
+   in your program, to reduce the
+   chance of use after frees!
+
+   if you use these functions in the
+   intended way, you will greatly reduce
+   the number of bugs in your code
+ */
+char *
+smalloc(char **buf, size_t size)
+{
+	return (char *)vmalloc((void **)buf, size);
+}
+void *
+vmalloc(void **buf, size_t size)
+{
+	void *rval = NULL;
+
+	if (size >= SIZE_MAX - 1)
+		err_no_cleanup(0, EOVERFLOW, "integer overflow in vmalloc");
+	if (buf == NULL)
+		err_no_cleanup(0, EFAULT, "Bad pointer passed to vmalloc");
+
+	/* lots of programs will
+	 * re-initialise a buffer
+	 * that was allocated, without
+	 * freeing or NULLing it. this
+	 * is here intentionally, to
+	 * force the programmer to behave
+	 */
+	if (*buf != NULL)
+		err_no_cleanup(0, EFAULT, "Non-null pointer given to vmalloc");
+
+	if (!size)
+		err_no_cleanup(0, EFAULT,
+		   "Tried to vmalloc(0) and that is very bad. Fix it now");
+
+	if ((rval = malloc(size)) == NULL)
+		err_no_cleanup(0, errno, "malloc fail in vmalloc");
+
+	return *buf = rval;
+}
+
 /* strict strcmp */
 int
 scmp(const char *a,
@@ -98,19 +143,16 @@ sdup(const char *s,
     size_t n, char **dest)
 {
 	size_t size;
-	char *rval;
+	char *rval = NULL;
 
 	if (dest == NULL ||
-	    slen(s, n, &size) < 0 ||
-	    if_err(size == SIZE_MAX, EOVERFLOW) ||
-	    (rval = malloc(size + 1)) == NULL) {
-
+	    slen(s, n, &size) < 0) {
 		if (dest != NULL)
 			*dest = NULL;
 		return -1;
 	}
 
-	memcpy(rval, s, size);
+	memcpy(smalloc(&rval, size + 1), s, size);
 	*(rval + size) = '\0';
 
 	*dest = rval;
@@ -133,10 +175,11 @@ scatn(ssize_t sc, const char **sv,
 
 	if (if_err(sc <= 0, EINVAL) ||
 	    if_err(sc > SIZE_MAX / sizeof(size_t), EOVERFLOW) ||
-	    if_err(sv == NULL, EINVAL) ||
-	    if_err((size = malloc(sizeof(size_t) * sc)) == NULL, ENOMEM))
+	    if_err(sv == NULL, EINVAL))
 		goto err;
 
+	vmalloc((void **)&size, sizeof(size_t) * sc);
+
 	for (i = 0; i < sc; i++, ts += size[i])
 		if (if_err(sv[i] == NULL, EINVAL) ||
 		    slen(sv[i], max, &size[i]) < 0 ||
@@ -145,10 +188,10 @@ scatn(ssize_t sc, const char **sv,
 			goto err;
 
 	if (if_err(ts > SIZE_MAX - 1, EOVERFLOW) ||
-	    if_err(ts > max - 1, EOVERFLOW) ||
-	    if_err((ct = malloc(ts + 1)) == NULL, ENOMEM))
+	    if_err(ts > max - 1, EOVERFLOW))
 		goto err;
 
+	smalloc(&ct, ts + 1);
 	for (ts = i = 0; i < sc; i++, ts += size[i])
 		memcpy(ct + ts, sv[i], size[i]);
 
@@ -175,20 +218,20 @@ scat(const char *s1, const char *s2,
 {
 	size_t size1;
 	size_t size2;
-	char *rval;
+	char *rval = NULL;
 
 	if (dest == NULL ||
 	    slen(s1, n, &size1) < 0 ||
 	    slen(s2, n, &size2) < 0 ||
-	    if_err(size1 > SIZE_MAX - size2 - 1, EOVERFLOW) ||
-	    (rval = malloc(size1 + size2 + 1)) == NULL) {
+	    if_err(size1 > SIZE_MAX - size2 - 1, EOVERFLOW)) {
 
 		if (dest != NULL)
 			*dest = NULL;
 		return -1;
 	}
 
-	memcpy(rval, s1, size1);
+	memcpy(smalloc(&rval, size1 + size2 + 1),
+	    s1, size1);
 	memcpy(rval + size1, s2, size2);
 	*(rval + size1 + size2) = '\0';
 
@@ -210,17 +253,17 @@ dcat(const char *s, size_t n,
 	if (dest1 == NULL || dest2 == NULL ||
 	    slen(s, n, &size) < 0 ||
 	    if_err(size == SIZE_MAX, EOVERFLOW) ||
-	    if_err(off >= size, EOVERFLOW) ||
-	    (rval1 = malloc(off + 1)) == NULL ||
-	    (rval2 = malloc(size - off + 1)) == NULL) {
+	    if_err(off >= size, EOVERFLOW)) {
 
 		goto err;
 	}
 
-	memcpy(rval1, s, off);
+	memcpy(smalloc(&rval1, off + 1),
+	    s, off);
 	*(rval1 + off) = '\0';
 
-	memcpy(rval2, s + off, size - off);
+	memcpy(smalloc(&rval2, size - off +1),
+	    s + off, size - off);
 	*(rval2 + size - off) = '\0';
 
 	*dest1 = rval1;
@@ -310,11 +353,10 @@ lbgetprogname(char *argv0)
 
 	if (!setname) {
 		if (if_err(argv0 == NULL || *argv0 == '\0', EFAULT) ||
-		    slen(argv0, 4096, &len) < 0 ||
-		    (progname = malloc(len + 1)) == NULL)
+		    slen(argv0, 4096, &len) < 0)
 			return NULL;
 
-		memcpy(progname, argv0, len + 1);
+		memcpy(smalloc(&progname, len + 1), argv0, len + 1);
 		setname = 1;
 	}