Lots of places checking malloc result for NULL pointer

[marekm72]

While trying to learn how accel-ppp works, I noticed lots of places where malloc is called and return value checked for NULL which means not enough memory. Are there current platforms where accel-ppp runs on which it makes sense to handle malloc failures gracefully, instead of simply terminating with fatal error? I bet these failure paths are not really much tested, as malloc returning NULL rarely (if ever) happens in practice, and lots of other bad things happen before the system is so low on memory (basically it slows down so much that it becomes unusable, swapping like mad).

See https://www.gnu.org/software/libc/manual/html_node/Malloc-Examples.html for a short xmalloc wrapper function which would be used instead, which never returns failure so all of these NULL checks can be removed. Many places also zero the allocated memory which could be handled by such a wrapper too, instead of separate memset calls. On current machines the performance impact of always zeroing even if not required is probably negligible, while leaving uninitialized data by mistake could lead to hard to detect bugs. What are your thoughts? Would pull requests cleaning up such things be accepted? Or has this been discussed before and decided that it should stay that way?

EDIT: looking more at the code, some places use malloc without checking for NULL, some places use malloc to allocate multiple objects where calloc would be safer (check for integer overflow), and many places zero the allocated memory (which calloc would do as well). A wrapper conventionally named xcalloc could take care of all this, and make the code more readable. Better simply abort() in the wrapper than risk undefined behaviour (like dereferencing NULL pointers), compilers sometimes do funny things making this a potential security issue too.

[marekm72]

The sound of silence... Really, no one interested in making the code less buggy and easier to understand at the same time?
I might do some work but don't want to waste time if it won't be accepted anyway, don't have the resources to maintain a long term fork.

[svlobanov]

Hi @marekm72 . I suggest the following way: please open draft PR with the fix in two-three places to see the code
Currently, most of developers are busy even for reviewing open PRs so I'm not sure how much time will be required to review your (potential) draft PR.

[marekm72]

Proposed patch (sorry not yet a PR as I'm still learning to use git properly, even though I learned C programming about 30 years ago from the first edition K&R book), minimally tested to compile with or without MEMDEBUG defined, just to show a possible way to do it re-using existing memdebug.[ch] - this avoids lots of changes to existing calls (they work as before except are now guaranteed to return non-NULL, so NULL checks are now redundant and can be slowly removed, and bugs where NULL check was missing are immediately fixed). For case without MEMDEBUG the md functions are still called but are simply wrappers for optimized libc functions that check return value and if failed, report file:line of the caller and abort. Also added calloc wrapper for cases where memory is allocated then zeroed and arrays of many elements, complete with size integer overflow check. As a side note, shouldn't all these printf calls be changed to fprintf to stderr?

diff --git a/accel-pppd/memdebug.c b/accel-pppd/memdebug.c
index 5353e57..0a9193c 100644
--- a/accel-pppd/memdebug.c
+++ b/accel-pppd/memdebug.c
@@ -1,5 +1,3 @@
-#undef MEMDEBUG
-
 #include <stdarg.h>
 #include <stdlib.h>
 #include <stdint.h>
@@ -11,9 +9,19 @@
 #include "spinlock.h"
 #include "list.h"
 
+static inline void _md_oom_check(void *ptr, const char *fname, int line)
+{
+	if (!ptr) {
+		printf("Out of memory %s:%i\n", fname, line);
+		abort();
+	}
+}
+
 #define __init __attribute__((constructor))
 #define __export __attribute__((visibility("default")))
 
+#ifdef MEMDEBUG
+
 #undef offsetof
 #ifdef __compiler_offsetof
 #define offsetof(TYPE,MEMBER) __compiler_offsetof(TYPE,MEMBER)
@@ -46,8 +54,7 @@ static struct mem_t *_md_malloc(size_t size, const char *fname, int line)
 {
 	struct mem_t *mem = malloc(sizeof(*mem) + size + 8);
 
-	if (mem == NULL)
-		return NULL;
+	_md_oom_check(mem, fname, line);
 
 	mem->fname = fname;
 	mem->line = line;
@@ -62,16 +69,49 @@ static struct mem_t *_md_malloc(size_t size, const char *fname, int line)
 
 	return mem;
 }
+#endif
 
 void __export *md_malloc(size_t size, const char *fname, int line)
 {
+#ifdef MEMDEBUG
 	struct mem_t *mem = _md_malloc(size, fname, line);
 
-	return mem ? mem->data : NULL;
+	return mem->data;
+#else
+	void *ptr = malloc(size);
+	if (size)
+		_md_oom_check(ptr, fname, line);
+	return ptr;
+#endif
+}
+
+void __export *md_calloc(size_t n, size_t size, const char *fname, int line)
+{
+	void *ptr;
+#ifdef MEMDEBUG
+	size_t bytes;
+
+	bytes = n * size;
+	if (n && size && bytes / size != n) {
+		printf("Size overflow calloc(%zu,%zu) %s:%i\n", n, size, fname, line);
+		abort();
+	}
+	ptr = md_malloc(bytes, fname, line);
+	if (bytes) {
+		_md_oom_check(ptr, fname, line);
+		memset(ptr, 0, bytes);
+	}
+#else
+	ptr = calloc(n, size);
+	if (n && size)
+		_md_oom_check(ptr, fname, line);
+#endif
+	return ptr;
 }
 
 void __export md_free(void *ptr, const char *fname, int line)
 {
+#ifdef MEMDEBUG
 	struct mem_t *mem;
 
 	if (!ptr)
@@ -96,13 +136,16 @@ void __export md_free(void *ptr, const char *fname, int line)
 	spin_lock(&mem_list_lock);
 	list_del(&mem->entry);
 	spin_unlock(&mem_list_lock);
-
 	free(mem);
-	return;
+#else
+	if (ptr)
+		free(ptr);
+#endif
 }
 
 void __export *md_realloc(void *ptr, size_t size, const char *fname, int line)
 {
+#ifdef MEMDEBUG
 	struct mem_t *mem = ptr ? container_of(ptr, typeof(*mem), data) : NULL;
 	struct mem_t *mem2;
 
@@ -126,8 +169,6 @@ void __export *md_realloc(void *ptr, size_t size, const char *fname, int line)
 	}
 
 	mem2 = _md_malloc(size, fname, line);
-	if (mem2 == NULL)
-		return NULL;
 
 	if (mem) {
 		memcpy(mem2->data, mem->data,
@@ -136,29 +177,42 @@ void __export *md_realloc(void *ptr, size_t size, const char *fname, int line)
 	}
 
 	return mem2->data;
+#else
+	void *p = realloc(ptr, size);
+	if (size)
+		_md_oom_check(p, fname, line);
+	return p;
+#endif
 }
 
 char __export *md_strdup(const char *ptr, const char *fname, int line)
 {
+#ifdef MEMDEBUG
 	size_t len = strlen(ptr);
 	char *str = md_malloc(len + 1, fname, line);
 
-	if (str)
-		memcpy(str, ptr, len + 1);
+	memcpy(str, ptr, len + 1);
+#else
+	char *str = strdup(ptr);
 
+	_md_oom_check(str, fname, line);
+#endif
 	return str;
 }
 
 char __export *md_strndup(const char *ptr, size_t n, const char *fname, int line)
 {
+#ifdef MEMDEBUG
 	size_t len = strnlen(ptr, n);
 	char *str = md_malloc(len + 1, fname, line);
 
-	if (str) {
-		memcpy(str, ptr, len);
-		str[len] = '\0';
-	}
+	memcpy(str, ptr, len);
+	str[len] = '\0';
+#else
+	char *str = strndup(ptr, n);
 
+	_md_oom_check(str, fname, line);
+#endif
 	return str;
 }
 
@@ -166,8 +220,9 @@ int __export md_asprintf(const char *fname, int line,
 			 char **strp, const char *fmt, ...)
 {
 	va_list ap;
-	va_list aq;
 	int len;
+#ifdef MEMDEBUG
+	va_list aq;
 
 	va_start(ap, fmt);
 	va_copy(aq, ap);
@@ -177,8 +232,6 @@ int __export md_asprintf(const char *fname, int line,
 		goto err;
 
 	*strp = md_malloc(len + 1, fname, line);
-	if (*strp == NULL)
-		goto err;
 
 	len = vsnprintf(*strp, len + 1, fmt, aq);
 	if (len < 0)
@@ -195,8 +248,17 @@ err:
 	va_end(aq);
 	va_end(ap);
 	return -1;
+#else
+	va_start(ap, fmt);
+	len = vasprintf(strp, fmt, ap);
+	va_end(ap);
+	if (len < 0)
+		_md_oom_check(NULL, fname, line);
+	return len;
+#endif
 }
 
+#ifdef MEMDEBUG
 static void siginfo(int num)
 {
 	struct mem_t *mem;
@@ -244,3 +306,10 @@ static void __init init(void)
 	signal(36, siginfo);
 	signal(37, siginfo2);
 }
+#else
+void __export md_check(void *ptr)
+{
+	if (!ptr)
+		abort();
+}
+#endif
diff --git a/accel-pppd/memdebug.h b/accel-pppd/memdebug.h
index 501b882..052aebc 100644
--- a/accel-pppd/memdebug.h
+++ b/accel-pppd/memdebug.h
@@ -1,11 +1,10 @@
 #ifndef __MEMDEBUG_H
 #define __MEMDEBUG_H
 
-#ifdef MEMDEBUG
-
 #include <sys/types.h>
 
 #define _malloc(size) md_malloc(size, __FILE__, __LINE__)
+#define _calloc(n, size) md_calloc(n, size, __FILE__, __LINE__)
 #define _realloc(ptr, size) md_realloc(ptr, size, __FILE__, __LINE__)
 #define _free(ptr) md_free(ptr, __FILE__, __LINE__)
 #define _strdup(str) md_strdup(str, __FILE__, __LINE__)
@@ -13,6 +12,7 @@
 #define _asprintf(strp, fmt, ...) md_asprintf(__FILE__, __LINE__, strp, fmt, ##__VA_ARGS__)
 
 void *md_malloc(size_t size, const char *fname, int line);
+void *md_calloc(size_t n, size_t size, const char *fname, int line);
 void *md_realloc(void *ptr, size_t size, const char *fname, int line);
 void md_free(void *ptr, const char *fname, int line);
 char* md_strdup(const char *ptr, const char *fname, int line);
@@ -20,18 +20,4 @@ char* md_strndup(const char *ptr, size_t size, const char *fname, int line);
 int md_asprintf(const char *fname, int line, char **strp, const char *fmt, ...) __attribute__((format(gnu_printf, 4, 5)));
 void md_check(void *ptr);
 
-#else
-
-#include <stdio.h>
-#include <stdlib.h>
-#include <string.h>
-
-#define _malloc(size) malloc(size)
-#define _realloc(ptr, size) realloc(ptr, size)
-#define _free(ptr) free(ptr)
-#define _strdup(str) strdup(str)
-#define _strndup(str, size) strndup(str, size)
-#define _asprintf(strp, fmt, ...) asprintf(strp, fmt, ##__VA_ARGS__)
-#endif
-
 #endif

[marekm72]

Hello @svlobanov just asking for a quick comment if you think it's worth to continue in this direction. Not asking for full review as this patch is not yet final, I have a few more updates in the works like adding function attributes, such as noreturn for the out of memory error. Also not sure if abort or exit is better for out of memory condition but it usually indicates the system in very bad state anyway (for example, in my setup there are two redundant pppoe servers so it's no big deal if only one of them stops working, but it's more important for BGP on the same box to keep running). I do understand everyone is busy (I'm too) but I want to help you, I'm in the process of moving from VyOS(tm) to roll-my-own-router-Debian-or-Alpine, but will likely continue using accel-ppp.