From ad141d00ebcc274113b94fa1539d16fd6df6f9e2 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Mon, 31 Aug 2026 16:10:52 -0700 Subject: [PATCH 01/10] fix(util): correct off-by-one OOB write in strncopy() when src fills the buffer When strlen(src) was at least obuf the length clamped to obuf and the terminator went to dst[obuf], one past a buffer of exactly that size. The pragma that suppressed the compiler's warning about it is no longer needed. Signed-off-by: Thomas Vincent --- util.c | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/util.c b/util.c index 653fef62..9e14ba63 100644 --- a/util.c +++ b/util.c @@ -1683,26 +1683,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; + + /* 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); - len = (strlen(src) < obuf) ? strlen(src) : obuf; - if (len) { - strncpy(dst, src, len); + 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 From 189aee2ad4b3301f68fae2e46a20bffd4989b0be Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Mon, 31 Aug 2026 16:11:03 -0700 Subject: [PATCH 02/10] fix(php): reserve room for the terminator in php_readpipe() read() was handed the whole remaining buffer, so a script server result of exactly RESULTS_BUFFER bytes filled it and the terminator went one past the end. The loop also had no guard for the case where no capacity was left. Signed-off-by: Thomas Vincent --- php.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/php.c b/php.c index f57f6dc4..32752f43 100644 --- a/php.c +++ b/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); From 2d31fcbddf2e1ab01a4a8d7dee035bcb28ba0d69 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Mon, 31 Aug 2026 16:11:21 -0700 Subject: [PATCH 03/10] fix(ping): copy the hostname and tokenise reentrantly in get_namebyhost() The copy used strlen(stack) as its size, and stack had just been zeroed, so nothing was copied and strtok() saw an empty string. Every branch that parses a transport prefix or port was unreachable as a result. strtok() also keeps one process-wide save pointer while this runs on each poller thread. Signed-off-by: Thomas Vincent --- ping.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/ping.c b/ping.c index b38ef8f0..97670226 100644 --- a/ping.c +++ b/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) { From 744f1cc154d38be5e5880614743e0141dc39e206 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Mon, 31 Aug 2026 16:11:59 -0700 Subject: [PATCH 04/10] fix(php): bound the script server shutdown and reap the child php_close() sent SIGTERM and moved on, so a script server that ignores it or is stuck in uninterruptible I/O was left as an orphan. Shutdown now polls for the child over a bounded window, escalates to SIGKILL, and polls again. Signed-off-by: Thomas Vincent --- CHANGELOG | 6 ++++++ php.c | 46 ++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 48 insertions(+), 4 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 4afe52e8..05f4b9c9 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,5 +1,11 @@ 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 + 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 diff --git a/php.c b/php.c index 32752f43..67d07ab5 100644 --- a/php.c +++ b/php.c @@ -503,6 +503,44 @@ 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. */ + usleep(50000); + } + + 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 @@ -567,10 +605,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 */ From e2cf02164c7483f705990cd5cdc31e0ae95afa41 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Mon, 31 Aug 2026 16:20:33 -0700 Subject: [PATCH 05/10] fix(config): size the RDB SSL path fields like every other path rdb_ssl_key, rdb_ssl_cert and rdb_ssl_ca were BIG_BUFSIZE while the config parser reads at most 255 characters into a BUFSIZE scratch buffer, so the copy bound exceeded the source and the compiler said so. develop already carries these at BUFSIZE. Signed-off-by: Thomas Vincent --- spine.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/spine.h b/spine.h index f2716a19..b736fd56 100644 --- a/spine.h +++ b/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; From e5e1ad487c980b30b45f027de8a7b0bcea317e6c Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Mon, 31 Aug 2026 16:35:38 -0700 Subject: [PATCH 06/10] fix: check the four remaining calloc results in spine.c php_processes, debug_devices and both connection pools were dereferenced on the next statement. Backport of the develop fix for issue#564. Signed-off-by: Thomas Vincent --- spine.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/spine.c b/spine.c index 49c4535b..4a0dc765 100644 --- a/spine.c +++ b/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,7 +543,10 @@ 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) { @@ -546,7 +554,10 @@ int main(int argc, char *argv[]) { 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; From f06efd963315ebed835bcd2711821b3022b0ed83 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Mon, 31 Aug 2026 16:36:11 -0700 Subject: [PATCH 07/10] fix(sql): free the result set when the fetch returns no row Four settings helpers returned from the NULL-row branch without freeing while every other exit frees. Backport of the develop fix for issue#566. Signed-off-by: Thomas Vincent --- util.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/util.c b/util.c index 9e14ba63..04ae4a20 100644 --- a/util.c +++ b/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() && @@ -2060,6 +2074,7 @@ int get_cacti_version(MYSQL *psql, int mode) { return cacti_version; } }else{ + db_free_result(result); return 0; } }else{ From 752928052b97dfd865f038d7fdedd6d0eb6677ac Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Mon, 31 Aug 2026 16:36:11 -0700 Subject: [PATCH 08/10] fix(log): keep the appended newline inside flogmessage strcat() wrote the newline at LOGSIZE-1 and its terminator one past the end once the message filled the buffer. Backport of the develop fix for issue#565, with the changelog entries for this batch. Signed-off-by: Thomas Vincent --- CHANGELOG | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG b/CHANGELOG index 05f4b9c9..c5e82a9b 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -5,6 +5,9 @@ The Cacti Group | spine -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 From 1af1ec5c54352bf078c2fd6fea03350234b32e8e Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Fri, 4 Sep 2026 18:14:09 -0700 Subject: [PATCH 09/10] fix(php): drop the stale BUFSIZE bound in php_readpipe RESULTS_BUFFER defaults to 2048 and BUFSIZE is 1024, so the second guard rejected any reply over 1024 bytes that the read loop had already accepted, and it did not break, so the next read overwrote the buffer it had just declared out of range. The loop bound above it is the real limit. Signed-off-by: Thomas Vincent --- php.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/php.c b/php.c index 67d07ab5..7624b197 100644 --- a/php.c +++ b/php.c @@ -278,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)); From 3c6388af7d5c1881532214f6ab9fe7d24f26396a Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Fri, 4 Sep 2026 18:16:59 -0700 Subject: [PATCH 10/10] fix(php): guard the reap delay for Solaris like the other usleep calls Every other usleep() in php.c sits inside #ifndef SOLAR_THREAD. Signed-off-by: Thomas Vincent --- php.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/php.c b/php.c index 7624b197..a285e911 100644 --- a/php.c +++ b/php.c @@ -527,7 +527,9 @@ static void php_terminate_and_reap(pid_t pid) { /* 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;