From fb29a8ebcd076df9da51275adb45d90482f2e19f Mon Sep 17 00:00:00 2001
From: Martin Schwenke <martin@meltin.net>
Date: Wed, 13 Oct 2021 11:42:14 +1100
Subject: [PATCH] debug: Move header_str and hs_len to state

They'll need to be accessible by the backends.

Note that the snprintf() and strlcat() calls can result in
state.hs_len >= sizeof(state.header_str), so state.hs_len needs to be
sanitised before any potential use.  Previously this wasn't necessary
because this value was on the stack, so it couldn't be used after
dbghdrclass() returned.

Signed-off-by: Martin Schwenke <martin@meltin.net>
Reviewed-by: Volker Lendecke <vl@samba.org>
---
 lib/util/debug.c | 80 ++++++++++++++++++++++++++++++++----------------
 1 file changed, 54 insertions(+), 26 deletions(-)

diff --git a/lib/util/debug.c b/lib/util/debug.c
index 4fd17679227..51fd3d0641a 100644
--- a/lib/util/debug.c
+++ b/lib/util/debug.c
@@ -95,6 +95,8 @@ static struct {
 	struct debug_settings settings;
 	debug_callback_fn callback;
 	void *callback_private;
+	char header_str[300];
+	size_t hs_len;
 } state = {
 	.settings = {
 		.timestamp_logs = true
@@ -1543,11 +1545,17 @@ bool dbghdrclass(int level, int cls, const char *location, const char *func)
 	/* Ensure we don't lose any real errno value. */
 	int old_errno = errno;
 	bool verbose = false;
-	char header_str[300];
-	size_t hs_len;
 	struct timeval tv;
 	struct timeval_buf tvbuf;
 
+	/*
+	 * This might be overkill, but if another early return is
+	 * added later then initialising these avoids potential
+	 * problems
+	 */
+	state.hs_len = 0;
+	state.header_str[0] = '\0';
+
 	if( format_pos ) {
 		/* This is a fudge.  If there is stuff sitting in the format_bufr, then
 		 * the *right* thing to do is to call
@@ -1580,9 +1588,12 @@ bool dbghdrclass(int level, int cls, const char *location, const char *func)
 	timeval_str_buf(&tv, false, state.settings.debug_hires_timestamp,
 			&tvbuf);
 
-	hs_len = snprintf(header_str, sizeof(header_str), "[%s, %2d",
-			  tvbuf.buf, level);
-	if (hs_len >= sizeof(header_str)) {
+	state.hs_len = snprintf(state.header_str,
+				sizeof(state.header_str),
+				"[%s, %2d",
+				tvbuf.buf,
+				level);
+	if (state.hs_len >= sizeof(state.header_str)) {
 		goto full;
 	}
 
@@ -1591,31 +1602,35 @@ bool dbghdrclass(int level, int cls, const char *location, const char *func)
 	}
 
 	if (verbose || state.settings.debug_pid) {
-		hs_len += snprintf(
-			header_str + hs_len, sizeof(header_str) - hs_len,
-			", pid=%u", (unsigned int)getpid());
-		if (hs_len >= sizeof(header_str)) {
+		state.hs_len += snprintf(state.header_str + state.hs_len,
+					 sizeof(state.header_str) - state.hs_len,
+					 ", pid=%u",
+					 (unsigned int)getpid());
+		if (state.hs_len >= sizeof(state.header_str)) {
 			goto full;
 		}
 	}
 
 	if (verbose || state.settings.debug_uid) {
-		hs_len += snprintf(
-			header_str + hs_len, sizeof(header_str) - hs_len,
-			", effective(%u, %u), real(%u, %u)",
-			(unsigned int)geteuid(), (unsigned int)getegid(),
-			(unsigned int)getuid(), (unsigned int)getgid());
-		if (hs_len >= sizeof(header_str)) {
+		state.hs_len += snprintf(state.header_str + state.hs_len,
+					 sizeof(state.header_str) - state.hs_len,
+					 ", effective(%u, %u), real(%u, %u)",
+					 (unsigned int)geteuid(),
+					 (unsigned int)getegid(),
+					 (unsigned int)getuid(),
+					 (unsigned int)getgid());
+		if (state.hs_len >= sizeof(state.header_str)) {
 			goto full;
 		}
 	}
 
 	if ((verbose || state.settings.debug_class)
 	    && (cls != DBGC_ALL)) {
-		hs_len += snprintf(
-			header_str + hs_len, sizeof(header_str) - hs_len,
-			", class=%s", classname_table[cls]);
-		if (hs_len >= sizeof(header_str)) {
+		state.hs_len += snprintf(state.header_str + state.hs_len,
+					 sizeof(state.header_str) - state.hs_len,
+					 ", class=%s",
+					 classname_table[cls]);
+		if (state.hs_len >= sizeof(state.header_str)) {
 			goto full;
 		}
 	}
@@ -1623,22 +1638,35 @@ bool dbghdrclass(int level, int cls, const char *location, const char *func)
 	/*
 	 * No +=, see man man strlcat
 	 */
-	hs_len = strlcat(header_str, "] ", sizeof(header_str));
-	if (hs_len >= sizeof(header_str)) {
+	state.hs_len = strlcat(state.header_str, "] ", sizeof(state.header_str));
+	if (state.hs_len >= sizeof(state.header_str)) {
 		goto full;
 	}
 
 	if (!state.settings.debug_prefix_timestamp) {
-		hs_len += snprintf(
-			header_str + hs_len, sizeof(header_str) - hs_len,
-			"%s(%s)\n", location, func);
-		if (hs_len >= sizeof(header_str)) {
+		state.hs_len += snprintf(state.header_str + state.hs_len,
+					 sizeof(state.header_str) - state.hs_len,
+					 "%s(%s)\n",
+					 location,
+					 func);
+		if (state.hs_len >= sizeof(state.header_str)) {
 			goto full;
 		}
 	}
 
 full:
-	(void)Debug1(header_str);
+	/*
+	 * Above code never overflows state.header_str and always
+	 * NUL-terminates correctly.  However, state.hs_len can point
+	 * past the end of the buffer to indicate that truncation
+	 * occurred, so fix it if necessary, since state.hs_len is
+	 * expected to be used after return.
+	 */
+	if (state.hs_len >= sizeof(state.header_str)) {
+		state.hs_len = sizeof(state.header_str) - 1;
+	}
+
+	(void)Debug1(state.header_str);
 
 	errno = old_errno;
 	return( true );