fix: memory-safety and correctness backports for 1.2.x

CHANGELOG

@@ -1,5 +1,14 @@
 The Cacti Group | spine
 
+1.2.32
+-issue#447: Correct off-by-one OOB write in strncopy() when src fills the buffer
+-issue#561: Reserve room for the terminator in php_readpipe() so a full script server result cannot write past result_string
+-issue#562: Escalate PHP script server shutdown to SIGKILL after a bounded grace period so a stuck child is not orphaned
+-issue#573: Copy the hostname in get_namebyhost() so transport and port parsing runs, and tokenise reentrantly
+-issue#564: Check the remaining calloc() results in spine.c before they are dereferenced
+-issue#565: Keep the appended newline inside flogmessage when the log line fills LOGSIZE
+-issue#566: Free the result set when the settings helpers fetch no row
+
 1.2.31
 -issue#365: Removed Backtrace Support due to lack of OS support
 -issue#366: Prevent polling from stopping when sysDescr OID returns noSuchObject

php.c

@@ -255,7 +255,17 @@ char *php_readpipe(int php_process, char *command) {
 			bptr = result_string;
 
 			while (1) {
-				i = read(php_processes[php_process].php_read_fd, bptr, RESULTS_BUFFER-(bptr-result_string));
+				/* reserve one byte for the trailing '\0' written below */
+				size_t used = (size_t)(bptr - result_string);
+
+				if (used >= RESULTS_BUFFER - 1) {
+					SPINE_LOG(("ERROR: SS[%i] The Script Server result was longer than the acceptable range", php_process));
+					SET_UNDEFINED(result_string);
+					break;
+				}
+
+				size_t avail = (size_t)RESULTS_BUFFER - 1 - used;
+				i = read(php_processes[php_process].php_read_fd, bptr, avail);
 
 				if (i <= 0) {
 					SET_UNDEFINED(result_string);
@@ -268,11 +278,6 @@ char *php_readpipe(int php_process, char *command) {
 				if ((cp = strstr(result_string,"\n")) != 0) {
 					break;
 				}
-
-				if (bptr >= result_string+BUFSIZE) {
-					SPINE_LOG(("ERROR: SS[%i] The Script Server result was longer than the acceptable range", php_process));
-					SET_UNDEFINED(result_string);
-				}
 			}
 		} else {
 			SPINE_LOG(("ERROR: SS[%i] The FD was not set as expected", php_process));
@@ -493,6 +498,46 @@ int php_init(int php_process) {
 	return TRUE;
 }
 
+static void php_terminate_and_reap(pid_t pid) {
+	int attempts;
+	int phase;
+	int status;
+	int signal_number = SIGTERM;
+	pid_t waited;
+
+	for (phase = 0; phase < 2; phase++) {
+		if (kill(pid, signal_number) < 0 && errno != ESRCH) {
+			SPINE_LOG(("WARNING: Unable to signal PHP Script Server PID[%ld]: %s", (long)pid, strerror(errno)));
+		}
+
+		for (attempts = 0; attempts < 20; attempts++) {
+			do {
+				waited = waitpid(pid, &status, WNOHANG);
+			} while (waited < 0 && errno == EINTR);
+
+			if (waited == pid || (waited < 0 && errno == ECHILD)) {
+				return;
+			}
+
+			if (waited < 0) {
+				SPINE_LOG(("WARNING: Unable to reap PHP Script Server PID[%ld]: %s", (long)pid, strerror(errno)));
+				return;
+			}
+
+			/* The delay is load-bearing: without it both phases burn twenty
+			 * WNOHANG polls in nanoseconds, so SIGKILL lands immediately and
+			 * the child is never reaped. */
+			#ifndef SOLAR_THREAD
+			usleep(50000);
+			#endif
+		}
+
+		signal_number = SIGKILL;
+	}
+
+	SPINE_LOG(("WARNING: PHP Script Server PID[%ld] did not exit after SIGKILL", (long)pid));
+}
+
 /*! \fn void php_close(int php_process)
  *  \brief close the php script server process
  *  \param php_process the process to close or PHP_INIT
@@ -557,10 +602,10 @@ void php_close(int php_process) {
 	 	 * a process group leader), and PID 1 is "init".
 	  	 */
 		if (phpp->php_pid > 1) {
-			/* end the php script server process */
-			kill(phpp->php_pid, SIGTERM);
-
-			/* reset this PID variable? */
+			/* end the php script server process, escalating if it ignores
+			 * SIGTERM, and reap it so it cannot linger as an orphan */
+			php_terminate_and_reap(phpp->php_pid);
+			phpp->php_pid = -1;
 		}
 
 		/* close file descriptors */

ping.c

@@ -987,14 +987,15 @@ name_t *get_namebyhost(char *hostname, name_t *name) {
 	int tokens = 0;
 	char *stack = NULL;
 	char *token = NULL;
+	char *saveptr = NULL;
 
 	if (!(stack = (char *) malloc(strlen(hostname)+1))) {
 		die("ERROR: Fatal malloc error: ping.c get_namebyhost->stack");
 	}
 
 	memset(stack, '\0', strlen(hostname)+1);
-	strncopy(stack, hostname, strlen(stack));
-	token = strtok(stack, ":");
+	strncopy(stack, hostname, strlen(hostname)+1);
+	token = strtok_r(stack, ":", &saveptr);
 
 	if (token == NULL) {
 		SPINE_LOG_DEBUG(("DEBUG: get_namebyhost(%s) - No delimiter, assume full hostname", hostname));
@@ -1056,7 +1057,7 @@ name_t *get_namebyhost(char *hostname, name_t *name) {
 		if (tokens > 3) {
 			SPINE_LOG_DEBUG(("DEBUG: get_namebyhost(%s) - Unexpected token: %i", hostname, tokens));
 		}
-		token = strtok(NULL, ":");
+		token = strtok_r(NULL, ":", &saveptr);
 	}
 
 	if (stack != NULL) {

spine.c

@@ -244,13 +244,18 @@ int main(int argc, char *argv[]) {
 	install_spine_signal_handler();
 
 	/* establish php processes and initialize space */
-	php_processes = (php_t*) calloc(MAX_PHP_SERVERS, sizeof(php_t));
+	if (!(php_processes = (php_t*) calloc(MAX_PHP_SERVERS, sizeof(php_t)))) {
+		die("ERROR: Fatal malloc error: spine.c php_processes!");
+	}
+
 	for (i = 0; i < MAX_PHP_SERVERS; i++) {
 		php_processes[i].php_state = PHP_BUSY;
 	}
 
 	/* create the array of debug devices */
-	debug_devices = calloc(100, sizeof(int));
+	if (!(debug_devices = calloc(100, sizeof(int)))) {
+		die("ERROR: Fatal malloc error: spine.c debug_devices!");
+	}
 
 	/* initialize icmp_avail */
 	set.icmp_avail = TRUE;
@@ -538,15 +543,21 @@ int main(int argc, char *argv[]) {
 	db_connect(LOCAL, &mysql);
 
 	/* setup local connection pool for hosts */
-	db_pool_local = (pool_t *) calloc(set.threads, sizeof(pool_t));
+	if (!(db_pool_local = (pool_t *) calloc(set.threads, sizeof(pool_t)))) {
+		die("ERROR: Fatal malloc error: spine.c db_pool_local!");
+	}
+
 	db_create_connection_pool(LOCAL);
 
 	if (set.poller_id > 1 && set.mode == REMOTE_ONLINE) {
 		db_connect(REMOTE, &mysqlr);
 		mode = REMOTE;
 
 		/* setup remote connection pool for hosts */
-		db_pool_remote = (pool_t *) calloc(set.threads, sizeof(pool_t));
+		if (!(db_pool_remote = (pool_t *) calloc(set.threads, sizeof(pool_t)))) {
+			die("ERROR: Fatal malloc error: spine.c db_pool_remote!");
+		}
+
 		db_create_connection_pool(REMOTE);
 	} else {
 		mode = LOCAL;

spine.h

@@ -424,9 +424,9 @@ typedef struct config_struct {
 	char   rdb_user[BUFSIZE];
 	char   rdb_pass[BUFSIZE];
 	int    rdb_ssl;
-	char   rdb_ssl_key[BIG_BUFSIZE];
-	char   rdb_ssl_cert[BIG_BUFSIZE];
-	char   rdb_ssl_ca[BIG_BUFSIZE];
+	char   rdb_ssl_key[BUFSIZE];
+	char   rdb_ssl_cert[BUFSIZE];
+	char   rdb_ssl_ca[BUFSIZE];
 	unsigned int rdb_port;
 	char   rdbversion[BUFSIZE];
 	int    rdbonupdate;

util.c

@@ -108,6 +108,7 @@ static const char *getsetting(MYSQL *psql, int mode, const char *setting) {
 				db_free_result(result);
 				return retval;
 			}else{
+				db_free_result(result);
 				return strdup("");
 			}
 		}else{
@@ -200,6 +201,7 @@ static const char *getpsetting(MYSQL *psql, int mode, const char *setting) {
 				db_free_result(result);
 				return retval;
 			} else {
+				db_free_result(result);
 				return 0;
 			}
 		} else {
@@ -294,6 +296,7 @@ static const char *getglobalvariable(MYSQL *psql, int mode, const char *setting)
 				db_free_result(result);
 				return retval;
 			} else {
+				db_free_result(result);
 				return 0;
 			}
 		} else {
@@ -1364,9 +1367,20 @@ int spine_log(const char *format, ...) {
 		closelog();
 	}
 
-	/* append a line feed to the log message if needed */
+	/* append a line feed to the log message if needed.  The strncat() calls
+	 * above are allowed to fill flogmessage exactly, so the newline only fits
+	 * when a byte is free; otherwise it replaces the last character rather
+	 * than running past the end. */
 	if (!strstr(flogmessage, "\n")) {
-		strcat(flogmessage, "\n");
+		size_t flog_used = strlen(flogmessage);
+
+		if (flog_used < LOGSIZE - 1) {
+			flogmessage[flog_used]     = '\n';
+			flogmessage[flog_used + 1] = '\0';
+		} else {
+			flogmessage[LOGSIZE - 2] = '\n';
+			flogmessage[LOGSIZE - 1] = '\0';
+		}
 	}
 
 	if ((IS_LOGGING_TO_FILE() &&
@@ -1683,26 +1697,27 @@ char *add_slashes(char *string) {
  *  \return pointer to destination string
  *
 */
-#pragma GCC diagnostic push
-#if (defined(__GNUC__) && (__GNUC__ > 7)) || (__GNUC__ == 7 && defined(__GNUC_MINOR__) && __GNUC_MINOR__ > 1)
-#pragma GCC diagnostic ignored "-Wstringop-overflow"
-#pragma GCC diagnostic ignored "-Wstringop-truncation"
-#endif
 char *strncopy(char *dst, const char *src, size_t obuf) {
+	size_t copy_len;
+
 	assert(dst != 0);
 	assert(src != 0);
 
-	size_t len;
+	if (obuf == 0) return dst;
 
-	len = (strlen(src) < obuf) ? strlen(src) : obuf;
-	if (len) {
-		strncpy(dst, src, len);
+	/* Cap the scan at obuf-1: no need to walk past the usable copy capacity,
+	 * and avoids a full strlen when src is large or unterminated near obuf. */
+	copy_len = strnlen(src, obuf - 1);
+
+	if (copy_len) {
+		/* copy_len is the exact byte count and dst is terminated below, so
+		 * memcpy avoids the strncpy truncation diagnostic. */
+		memcpy(dst, src, copy_len);
 	}
 
-	dst[len] = '\0';
+	dst[copy_len] = '\0';
 	return dst;
 }
-#pragma GCC diagnostic pop
 
 /*! \fn double get_time_as_double()
  *  \brief fetches system time as a double-precison value
@@ -2059,6 +2074,7 @@ int get_cacti_version(MYSQL *psql, int mode) {
 					return cacti_version;
 				}
 			}else{
+				db_free_result(result);
 				return 0;
 			}
 		}else{