diff --git a/CHANGELOG b/CHANGELOG index 4afe52e8..c5e82a9b 100644 --- a/CHANGELOG +++ b/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 diff --git a/php.c b/php.c index f57f6dc4..a285e911 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); @@ -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 */ 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) { 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; 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; diff --git a/util.c b/util.c index 653fef62..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() && @@ -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{