From: Arran Cudbard-Bell <a.cudbardb@freeradius.org>
Date: Mon, 2 Apr 2018 17:54:47 +0000 (+0100)
Subject: Fix use after free in fr_syserror and fr_strerror
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=17e8a186ef6b001fc2f2012c3280eb65ed906efb;p=thirdparty%2Ffreeradius-server.git

Fix use after free in fr_syserror and fr_strerror

This ocurred when something called fr_strerror_printf in another thread local destructor, after the memory had been freed.
---

diff --git a/src/lib/util/strerror.c b/src/lib/util/strerror.c
index 6cb76af0818..38032082ced 100644
--- a/src/lib/util/strerror.c
+++ b/src/lib/util/strerror.c
@@ -49,14 +49,19 @@ typedef struct {
 } fr_log_buffer_t;
 
 fr_thread_local_setup(fr_log_buffer_t *, fr_strerror_buffer)	/* macro */
+static _Thread_local bool logging_stop;	//!< Due to ordering issues we may get errors being
+					///< logged from within other thread local destructors
+					///< which cause a crash on exit if the logging buffer
+					///< has already been freed.
 
 /*
  *	Explicitly cleanup the memory allocated to the error buffer,
  *	just in case valgrind complains about it.
  */
-static void _fr_logging_free(void *arg)
+static void _fr_logging_free(UNUSED void *arg)
 {
-	talloc_free(arg);
+	TALLOC_FREE(fr_strerror_buffer);
+	logging_stop = true;
 }
 
 /** Reset cursor state
@@ -77,6 +82,8 @@ static inline fr_log_buffer_t *fr_strerror_init(void)
 {
 	fr_log_buffer_t *buffer;
 
+	if (logging_stop) return NULL;	/* No more logging */
+
 	buffer = fr_strerror_buffer;
 	if (!buffer) {
 		buffer = talloc(NULL, fr_log_buffer_t);	/* One byte extra for status */
diff --git a/src/lib/util/syserror.c b/src/lib/util/syserror.c
index cb17c75f8ea..d409c95bc3c 100644
--- a/src/lib/util/syserror.c
+++ b/src/lib/util/syserror.c
@@ -29,6 +29,12 @@ RCSID("$Id$")
 #define FR_SYSERROR_BUFSIZE (2048)
 
 fr_thread_local_setup(char *, fr_syserror_buffer)		/* macro */
+static _Thread_local bool logging_stop;	//!< Due to ordering issues we may get errors being
+					///< logged from within other thread local destructors
+					///< which cause a crash on exit if the logging buffer
+					///< has already been freed.
+
+#define HAVE_DEFINITION(_errno) ((_errno) < (int)(sizeof(fr_syserror_macro_names) / sizeof(*fr_syserror_macro_names)))
 
 /*
  *	Explicitly cleanup the memory allocated to the error buffer,
@@ -36,7 +42,8 @@ fr_thread_local_setup(char *, fr_syserror_buffer)		/* macro */
  */
 static void _fr_logging_free(UNUSED void *arg)
 {
-	TALLOC_FREE(fr_syserror_buffer);	/* Set to NULL in case the buffer needs to be recreated */
+	TALLOC_FREE(fr_syserror_buffer);
+	logging_stop = true;
 }
 
 /** POSIX-2008 errno macros
@@ -158,6 +165,17 @@ char const *fr_syserror(int num)
 
 	buffer = fr_syserror_buffer;
 	if (!buffer) {
+		/*
+		 *	Try and produce something useful,
+		 *	even if the thread is exiting.
+		 */
+		if (logging_stop) {
+			if (HAVE_DEFINITION(num)) {
+				return fr_syserror_macro_names[num];
+			}
+			return "";
+		}
+
 		buffer = talloc_array(NULL, char, FR_SYSERROR_BUFSIZE);
 		if (!buffer) {
 			fr_perror("Failed allocating memory for system error buffer");
@@ -175,7 +193,7 @@ char const *fr_syserror(int num)
 	 *	Prefix system errors with the macro name and number
 	 *	if we're debugging.
 	 */
-	if (num < (int)(sizeof(fr_syserror_macro_names) / sizeof(*fr_syserror_macro_names))) {
+	if (HAVE_DEFINITION(num)) {
 		p += snprintf(p, end - p, "%s: ", fr_syserror_macro_names[num]);
 	} else {
 		p += snprintf(p, end - p, "errno %i: ", num);