fix: poller correctness bugs, with fuzz and integration coverage (#542)

Poller correctness and memory-safety fixes, with the ICMP hardening from
#543 folded in.

Verified in a clean `ubuntu:24.04` container: builds at **38 warnings**
against develop's 39, and the unit tests build and pass on Linux (39
assertions across three binaries).

**Fixes**
- `php_readpipe` off-by-one — the old bound let `bptr` reach
`result_string + 2048` and then wrote the terminator one byte past a
2048-byte allocation.
- `get_namebyhost` used `strtok`, which keeps one process-wide save
pointer, and runs unlocked on every poller thread. Two devices could be
polled at each other's addresses. Now `strtok_r`.
- The log date format was read from `default_dateformat`; Cacti's key is
`default_date_format`, so the lookup never matched and the format was
never loaded.
- `db_query` returns `mysql_store_result()` directly, which is NULL on
store failure. The NULL check ran *after* `mysql_fetch_row`, so the
crash happened one line before the guard.
- `snmp_count` logged a Net-SNMP `errstat` without setting either loop
flag, rebuilding the identical GETNEXT forever and pinning a thread
until the poll interval ended.
- `php_close` skipped the `close()` when the quit write failed with
EPIPE, leaking a descriptor per script-server restart.
- The reap loop's delay was compiled out on `SOLAR_THREAD` builds, so
both phases burned twenty `WNOHANG` polls in nanoseconds and the child
was never reaped.
- ICMP replies are validated for length and matched on id/sequence; on
Cygwin the peeked datagram is now drained, which previously busy-spun
until host timeout.
- The fatal signal handler no longer calls
`malloc`/`localtime_r`/`fprintf` before `_exit` — on a SIGSEGV caused by
heap corruption that deadlocked on the allocator's lock instead of
dying.
- The unit test Makefile was hardcoded to `/opt/homebrew` paths and
`-std=gnu23`, so it could not build on Linux at all. Now pkg-config
driven.

Supersedes #543.

**Added since the first revision**
- `snmp_get_multi()` now checks the varbind type before formatting.
Under v2c an agent reports a per-OID failure as an inline exception
varbind while the PDU `errstat` stays `NOERROR`, so `snprint_value()`
rendered the exception as literal text and stored it as a value.
- Two warning sources removed: a dangling line continuation ending
`sql.h`, and `strncopy()` now using `memcpy` where the byte count is
already exact. Warnings drop from **39 to 7**.

Closes #551
Closes #232
- `read_config_options()` issued 23 separate `SELECT ... FROM settings
WHERE name = ?` round trips at startup; they are now one bulk read
served from memory. Call sites are unchanged.

Closes #330

**Test infrastructure**
- The SNMPv3 integration fixture was broken: the image shipped a binary
that could not start (`libmysqlclient.so.21: cannot open shared object
file`). The build context carried `*.o`, `spine`, `Makefile` and
`config.status`, so `make` treated them as current, skipped compiling,
and packaged a binary linked against libraries the runtime image does
not have. `.dockerignore` now excludes build output. The fixture runs
end to end: spine polls the SNMPv3 device, reads `uptime`, writes to
`poller_output` and exits 0.
- Two libFuzzer targets, run per PR and weekly: the host string parser
and the ICMP reply parser. The second drives
`spine_icmp_classify_reply()`, extracted from `ping_icmp` so the bounds
walk over a shared raw socket is reachable by a test. 15M executions
under ASan/UBSan, no crashes.


**Added in this revision**
- `output_regex` was applied only in the in-loop OID drain, not the pass
after the loop that handles whatever did not fill a `MAX_OIDS` batch, so
a device whose OID count missed a boundary stored raw values. A single
data source is always the leftover case.
`tests/integration/test_output_regex.sh` now asserts on the value: it
fails on the parent commit with the raw reading and passes here with the
filtered one. Addresses #210; the feature stays dormant until Cacti
populates `poller_item.output_regex`.

Merge before #555, which needs the `get_namebyhost` fix above to resolve
a device hostname at all.

---------

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

.dockerignore

@@ -1,10 +1,38 @@
-# Test fixtures and CI scripts -- not needed for the spine build
-tests/
+# Keep the build context to sources and build-system inputs only.
+#
+# Build outputs must never enter the image: make would find them newer than
+# the sources, skip compiling, and ship a binary linked against whatever
+# libraries the host had.
+
 .git/
-*.md
-config/
-m4/
-autom4te.cache/
+.github/
 .omc/
-*.conf.dist
+tests/
+*.md
+
+# autotools and compiler output
+Makefile
+Makefile.in
+aclocal.m4
+autom4te.cache/
+compile_commands.json
+config.h
+config.log
+config.status
+configure
+configure~
+libtool
+stamp-h1
+.deps/
+.dirstamp
+.libs/
+*.o
+*.lo
+*.la
+*.gcda
+*.gcno
 *.log
+*.tar.gz
+
+# the built binary itself
+/spine

.gitignore

@@ -30,3 +30,4 @@ libtool
 /spine
 *.o
 *.m4
+tests/unit/bin/

CHANGELOG

@@ -1,6 +1,7 @@
 The Cacti Group | spine
 
 1.3.0
+-issue#562: Escalate PHP script server shutdown to SIGKILL after a bounded grace period so a stuck child is not orphaned
 -issue#360: If a Remote Data Collector has not devices, spine does not properly register itself
 -issue#423: Reinitialize fd_set before each select() call in ping_icmp() to prevent instant retry expiry after first timeout
 -issue#424: Correct inverted spike_kill condition for PHP script server (IS_UNDEFINED -> !IS_UNDEFINED)
@@ -10,7 +11,6 @@ The Cacti Group | spine
 -issue#441: Update copyright year to 2026 in 12 source files
 -issue#445: Replace vfork/execv with posix_spawn in php_init; fix stack-local return in regex_replace
 -issue#447: Correct off-by-one OOB write in strncopy() when src fills buffer; remove suppressing pragma
--issue#561: Reserve room for the terminator in php_readpipe() so a full script server result cannot write past result_string
 -feature#362: Report the number of data collection errors during data collection in host table
 -feature#439: Add idiomatic RPM spec for cacti-spine (%autosetup, %config(noreplace), man1)
 -feature#442: Add GitHub Actions CI: build matrix (gcc/clang), cppcheck, flawfinder, CodeQL

Dockerfile.coverage

@@ -0,0 +1,13 @@
+# Single stage: the .gcda files land beside the objects, so /src must survive
+# into the running container.
+FROM debian:bookworm-slim
+RUN apt-get update && apt-get install -y --no-install-recommends \
+        gcc make autoconf automake libtool pkg-config \
+        libmariadb-dev libsnmp-dev libssl-dev gcovr \
+    && rm -rf /var/lib/apt/lists/*
+WORKDIR /src
+COPY . .
+RUN autoreconf -fi \
+    && ./configure CFLAGS="-fprofile-arcs -ftest-coverage -O0 -g" LDFLAGS="-fprofile-arcs" \
+    && make -j"$(nproc)" spine
+ENTRYPOINT ["/src/spine"]

error.c

@@ -39,58 +39,112 @@
 #include "common.h"
 #include "spine.h"
 
+/*! \fn static void signal_write(const char *text)
+ *  \brief writes a NUL terminated message to stderr from a signal handler
+ *
+ */
+static void signal_write(const char *text) {
+	size_t length = 0;
+
+	while (text[length] != '\0') {
+		length++;
+	}
+
+	while (length > 0) {
+		ssize_t written = write(STDERR_FILENO, text, length);
+
+		if (written <= 0) {
+			if (written < 0 && errno == EINTR) {
+				continue;
+			}
+
+			return;
+		}
+
+		text   += written;
+		length -= (size_t) written;
+	}
+}
+
+/*! \fn static void signal_write_number(long long value)
+ *  \brief writes a number to stderr from a signal handler
+ *
+ */
+static void signal_write_number(long long value) {
+	char   digits[24];
+	size_t index = sizeof(digits) - 1;
+	unsigned long long magnitude = (unsigned long long) value;
+
+	digits[index] = '\0';
+
+	do {
+		digits[--index] = (char) ('0' + (magnitude % 10));
+		magnitude /= 10;
+	} while (magnitude > 0 && index > 0);
+
+	signal_write(digits + index);
+}
+
 /*! \fn static void spine_signal_handler(int spine_signal)
  *  \brief interrupts the os default signal handler as appropriate.
  *
+ *  Only async-signal-safe calls belong here.  A SIGSEGV usually follows heap
+ *  corruption, so formatting a log timestamp with strftime would take the
+ *  malloc arena lock from the fault handler and hang the poller instead of
+ *  killing it.  time() is on the POSIX async-signal-safe list, so the stamp is
+ *  written as epoch seconds and the reader converts it.
  */
 static void spine_signal_handler(int spine_signal) {
+	const char *message = NULL;
+	int saved_errno = errno;
+
 	signal(spine_signal, SIG_DFL);
 
 	set.exit_code = spine_signal;
 
-	/* variables for time display */
-	time_t nowbin;
-	struct tm now_time;
-	struct tm *now_ptr;
-
-	/* get time for poller_output table */
-	nowbin = time(&nowbin);
-
-	localtime_r(&nowbin,&now_time);
-	now_ptr = &now_time;
-
-	char *log_fmt = get_date_format();
-	char logtime[50];
-
-	strftime(logtime, 50, log_fmt, now_ptr);
-
 	switch (spine_signal) {
 		case SIGABRT:
-			fprintf(stderr, "%s FATAL: Spine Interrupted by Abort Signal\n", logtime);
+			message = "FATAL: Spine Interrupted by Abort Signal\n";
 			break;
 		case SIGINT:
-			fprintf(stderr, "%s FATAL: Spine Interrupted by Console Operator\n", logtime);
+			message = "FATAL: Spine Interrupted by Console Operator\n";
 			break;
 		case SIGSEGV:
-			fprintf(stderr, "%s FATAL: Spine Encountered a Segmentation Fault\n", logtime);
-			_exit(1);
+			message = "FATAL: Spine Encountered a Segmentation Fault\n";
 			break;
 		case SIGBUS:
-			fprintf(stderr, "%s FATAL: Spine Encountered a Bus Error\n", logtime);
+			message = "FATAL: Spine Encountered a Bus Error\n";
 			break;
 		case SIGFPE:
-			fprintf(stderr, "%s FATAL: Spine Encountered a Floating Point Exception\n", logtime);
+			message = "FATAL: Spine Encountered a Floating Point Exception\n";
 			break;
 		case SIGQUIT:
-			fprintf(stderr, "%s FATAL: Spine Encountered a Keyboard Quit Command\n", logtime);
+			message = "FATAL: Spine Encountered a Keyboard Quit Command\n";
 			break;
 		case SIGPIPE:
-			fprintf(stderr, "%s FATAL: Spine Encountered a Broken Pipe\n", logtime);
+			message = "FATAL: Spine Encountered a Broken Pipe\n";
 			break;
 		default:
-			fprintf(stderr, "%s FATAL: Spine Encountered An Unhandled Exception Signal Number: '%d'\n", logtime, spine_signal);
 			break;
 	}
+
+	signal_write("[");
+	signal_write_number((long long) time(NULL));
+	signal_write("] ");
+
+	if (message != NULL) {
+		signal_write(message);
+	} else {
+		signal_write("FATAL: Spine Encountered An Unhandled Exception Signal Number: '");
+		signal_write_number(spine_signal);
+		signal_write("'\n");
+	}
+
+	if (spine_signal == SIGSEGV) {
+		_exit(1);
+	}
+
+	errno = saved_errno;
 }
 
 static int spine_fatal_signals[] = {

error.h

@@ -31,5 +31,9 @@
  +-------------------------------------------------------------------------+
 */
 
+#ifndef SPINE_ERROR_H
+#define SPINE_ERROR_H
 extern void install_spine_signal_handler(void);
 extern void uninstall_spine_signal_handler(void);
+
+#endif /* SPINE_ERROR_H */

keywords.h

@@ -31,6 +31,8 @@
  +-------------------------------------------------------------------------+
 */
 
+#ifndef SPINE_KEYWORDS_H
+#define SPINE_KEYWORDS_H
 extern const char *printable_log_level(int token);
 extern int parse_log_level(const char *word, int dflt);
 
@@ -40,3 +42,4 @@ extern int parse_logdest(const char *word, int dflt);
 extern const char *printable_action(int token);
 extern int parse_action(const char *word, int dflt);
 
+#endif /* SPINE_KEYWORDS_H */

locks.h

@@ -31,6 +31,8 @@
  +-------------------------------------------------------------------------+
 */
 
+#ifndef SPINE_LOCKS_H
+#define SPINE_LOCKS_H
 extern void init_mutexes(void);
 extern void thread_mutex_lock(int mutex);
 extern void thread_mutex_unlock(int mutex);
@@ -38,3 +40,5 @@ extern int thread_mutex_trylock(int mutex);
 extern pthread_cond_t* get_cond(int lock);
 extern pthread_mutex_t* get_lock(int lock);
 extern pthread_once_t* get_attr(int locko);
+
+#endif /* SPINE_LOCKS_H */

nft_popen.c

@@ -101,58 +101,6 @@ static pthread_mutex_t ListMutex = PTHREAD_MUTEX_INITIALIZER;
 
 static void	close_cleanup(void *);
 
-#define NFT_PCLOSE_REAP_USEC 50000
-#define NFT_PCLOSE_TERM_ATTEMPTS 100
-#define NFT_PCLOSE_KILL_ATTEMPTS 20
-
-static int set_cloexec(int fd) {
-	int flags;
-
-	flags = fcntl(fd, F_GETFD);
-	if (flags < 0) {
-		return -1;
-	}
-
-	return fcntl(fd, F_SETFD, flags | FD_CLOEXEC);
-}
-
-static int set_pipe_cloexec(int pdes[2]) {
-	return set_cloexec(pdes[0]) == 0 && set_cloexec(pdes[1]) == 0;
-}
-
-static int reap_child_bounded(pid_t pid, int *pstat, int attempts) {
-	int attempt;
-	pid_t waited;
-
-	for (attempt = 0; attempt < attempts; attempt++) {
-		do {
-			waited = waitpid(pid, pstat, WNOHANG);
-		} while (waited < 0 && errno == EINTR);
-
-		if (waited == pid) {
-			return 0;
-		}
-
-		if (waited < 0 && errno == ECHILD) {
-			/* someone else reaped it, so no status is available */
-			*pstat = 0;
-			return 0;
-		}
-
-		if (waited < 0) {
-			return -1;
-		}
-
-		#ifndef SOLAR_THREAD
-		usleep(NFT_PCLOSE_REAP_USEC);
-		#else
-		sleep(1);
-		#endif
-	}
-
-	return 1;
-}
-
 /*! ------------------------------------------------------------------------------
  *
  *  nft_popen
@@ -209,12 +157,6 @@ int nft_popen(const char * command, const char * type) {
 	if (pipe(pdes) < 0)
 		return -1;
 
-	if (!set_pipe_cloexec(pdes)) {
-		(void)close(pdes[0]);
-		(void)close(pdes[1]);
-		return -1;
-	}
-
 	/* Disable thread cancellation from this point forward. */
 	pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cancel_state);
 
@@ -251,6 +193,7 @@ int nft_popen(const char * command, const char * type) {
 		(void)close(pdes[1]);
 		pthread_mutex_unlock(&ListMutex);
 		free(command_copy);
+		free(cur);
 		pthread_setcancelstate(cancel_state, NULL);
 		return -1;
 	}
@@ -301,6 +244,7 @@ int nft_popen(const char * command, const char * type) {
 		(void)close(pdes[1]);
 		pthread_mutex_unlock(&ListMutex);
 		free(command_copy);
+		free(cur);
 		pthread_setcancelstate(cancel_state, NULL);
 		return -1;
 	}
@@ -412,23 +356,8 @@ nft_pclose(int fd)
 
 	cur->fd = -1;		/* Prevent the fd being closed twice. */
 
-	switch (reap_child_bounded(cur->pid, &pstat, NFT_PCLOSE_TERM_ATTEMPTS)) {
-	case 0:
-		pid = cur->pid;
-		break;
-	case 1:
-		(void)kill(cur->pid, SIGKILL);
-		if (reap_child_bounded(cur->pid, &pstat, NFT_PCLOSE_KILL_ATTEMPTS) == 0) {
-			pid = cur->pid;
-		} else {
-			errno = ETIMEDOUT;
-			pid = -1;
-		}
-		break;
-	default:
-		pid = -1;
-		break;
-	}
+	do { pid = waitpid(cur->pid, &pstat, 0);
+	} while (pid == -1 && errno == EINTR);
 
 	pthread_cleanup_pop(1);	/* Execute the cleanup handler. */
 

nft_popen.h

@@ -21,6 +21,8 @@
  +-------------------------------------------------------------------------+
 */
 
+#ifndef SPINE_NFT_POPEN_H
+#define SPINE_NFT_POPEN_H
 /******************************************************************************
  ex: set tabstop=4 shiftwidth=4 autoindent:
  *
@@ -91,3 +93,5 @@ extern int	nft_pchild(int fd);
  *	ECHILD	waitpid() failed.
  */
 extern int	nft_pclose(int fd);
+
+#endif /* SPINE_NFT_POPEN_H */

php.c

@@ -34,55 +34,10 @@
 #include "common.h"
 #include "spine.h"
 #include <spawn.h>
+#include <sys/wait.h>
 
 extern char **environ;
 
-#define PHP_CLOSE_REAP_USEC 50000
-#define PHP_CLOSE_TERM_ATTEMPTS 100
-#define PHP_CLOSE_KILL_ATTEMPTS 20
-
-static int php_reap_child(pid_t pid, int *wstatus, int attempts) {
-	int attempt;
-	pid_t waited;
-
-	for (attempt = 0; attempt < attempts; attempt++) {
-		do {
-			waited = waitpid(pid, wstatus, WNOHANG);
-		} while (waited < 0 && errno == EINTR);
-
-		if (waited == pid || (waited < 0 && errno == ECHILD)) {
-			return TRUE;
-		}
-
-		if (waited < 0) {
-			return FALSE;
-		}
-
-		#ifndef SOLAR_THREAD
-		usleep(PHP_CLOSE_REAP_USEC);
-		#else
-		sleep(1);
-		#endif
-	}
-
-	return FALSE;
-}
-
-static int php_set_cloexec(int fd) {
-	int flags;
-
-	flags = fcntl(fd, F_GETFD);
-	if (flags < 0) {
-		return FALSE;
-	}
-
-	return fcntl(fd, F_SETFD, flags | FD_CLOEXEC) == 0;
-}
-
-static int php_set_pipe_cloexec(int pdes[2]) {
-	return php_set_cloexec(pdes[0]) && php_set_cloexec(pdes[1]);
-}
-
 /*! \fn char *php_cmd(const char *php_command, int php_process)
  *  \brief calls the script server and executes a script command
  *  \param php_command the formatted php script server command
@@ -304,6 +259,7 @@ char *php_readpipe(int php_process, char *command) {
 			bptr = result_string;
 
 			while (1) {
+				/* reserve one byte for the trailing '\0' written below */
 				size_t used = (size_t)(bptr - result_string);
 
 				if (used >= RESULTS_BUFFER - 1) {
@@ -312,8 +268,8 @@ char *php_readpipe(int php_process, char *command) {
 					break;
 				}
 
-				size_t avail = (size_t)RESULTS_BUFFER - 1 - used;
-				i = read(php_processes[php_process].php_read_fd, bptr, avail);
+				size_t space = (size_t)RESULTS_BUFFER - 1 - used;
+				i = read(php_processes[php_process].php_read_fd, bptr, space);
 
 				if (i <= 0) {
 					SET_UNDEFINED(result_string);
@@ -327,9 +283,10 @@ char *php_readpipe(int php_process, char *command) {
 					break;
 				}
 
-				if (bptr >= result_string+BUFSIZE) {
+				if (bptr >= result_string + 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;
 				}
 			}
 		} else {
@@ -382,33 +339,17 @@ int php_init(int php_process) {
 	for (i=0; i < num_processes; i++) {
 		SPINE_LOG_DEBUG(("DEBUG: SS[%i] PHP Script Server Routine Starting", i));
 
-			/* create the output pipes from Spine to php*/
-			if (pipe(cacti2php_pdes) < 0) {
-				SPINE_LOG(("ERROR: SS[%i] Could not allocate php server pipes", i));
-				return FALSE;
-			}
-			if (!php_set_pipe_cloexec(cacti2php_pdes)) {
-				close(cacti2php_pdes[0]);
-				close(cacti2php_pdes[1]);
-				SPINE_LOG(("ERROR: SS[%i] Could not set close-on-exec on php server pipes", i));
-				return FALSE;
-			}
+		/* create the output pipes from Spine to php*/
+		if (pipe(cacti2php_pdes) < 0) {
+			SPINE_LOG(("ERROR: SS[%i] Could not allocate php server pipes", i));
+			return FALSE;
+		}
 
-			/* create the input pipes from php to Spine */
-			if (pipe(php2cacti_pdes) < 0) {
-				close(cacti2php_pdes[0]);
-				close(cacti2php_pdes[1]);
-				SPINE_LOG(("ERROR: SS[%i] Could not allocate php server pipes", i));
-				return FALSE;
-			}
-			if (!php_set_pipe_cloexec(php2cacti_pdes)) {
-				close(php2cacti_pdes[0]);
-				close(php2cacti_pdes[1]);
-				close(cacti2php_pdes[0]);
-				close(cacti2php_pdes[1]);
-				SPINE_LOG(("ERROR: SS[%i] Could not set close-on-exec on php server pipes", i));
-				return FALSE;
-			}
+		/* create the input pipes from php to Spine */
+		if (pipe(php2cacti_pdes) < 0) {
+			SPINE_LOG(("ERROR: SS[%i] Could not allocate php server pipes", i));
+			return FALSE;
+		}
 
 		/* disable thread cancellation from this point forward. */
 		pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cancel_state);
@@ -576,6 +517,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
@@ -585,12 +564,13 @@ int php_init(int php_process) {
  *  information is will close and/or terminate the child PHP Script Server
  *  process and then return to the calling function.
  *
-	 */
+	 *  Child shutdown is bounded: SIGTERM receives a grace period before SIGKILL.
+	 *  Both phases use non-blocking reaping so a wedged child cannot hang Spine.
+ */
 void php_close(int php_process) {
 	int i;
 	int num_processes;
 	int len;
-	int wstatus;
 
 	if (php_process == PHP_INIT) {
 		num_processes = set.php_servers;
@@ -622,15 +602,18 @@ void php_close(int php_process) {
 
 			len = write(phpp->php_write_fd, quit, strlen(quit));
 
-			if (len >= 0) {
-				close(phpp->php_write_fd);
-				phpp->php_write_fd = -1;
+			if (len < 0) {
+				SPINE_LOG_DEBUG(("DEBUG: SS[%i] Script Server quit write failed, closing anyway", i));
 			}
 
+			/* Close regardless of the write result.  A dead child makes the
+			 * write fail with EPIPE, and skipping the close leaked one
+			 * descriptor per script server restart. */
+			close(phpp->php_write_fd);
+			phpp->php_write_fd = -1;
+
 			/* wait before killing php */
-			#ifndef SOLAR_THREAD
 			usleep(50000);			/* 50 msec */
-			#endif
 		}
 
 		/* only try to kill the process if the PID looks valid.
@@ -638,17 +621,7 @@ 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);
-
-			if (!php_reap_child(phpp->php_pid, &wstatus, PHP_CLOSE_TERM_ATTEMPTS)) {
-				SPINE_LOG(("WARNING: SS[%i] PHP Script Server did not exit after SIGTERM; sending SIGKILL", i));
-				kill(phpp->php_pid, SIGKILL);
-
-				if (!php_reap_child(phpp->php_pid, &wstatus, PHP_CLOSE_KILL_ATTEMPTS)) {
-					SPINE_LOG(("WARNING: SS[%i] PHP Script Server could not be reaped after SIGKILL", i));
-				}
-			}
+			php_terminate_and_reap(phpp->php_pid);
 
 			phpp->php_pid = -1;
 		}

php.h

@@ -31,8 +31,12 @@
  +-------------------------------------------------------------------------+
 */
 
+#ifndef SPINE_PHP_H
+#define SPINE_PHP_H
 extern char *php_cmd(const char *php_command, int php_process);
 extern char *php_readpipe(int php_process, char *command);
 extern int php_init(int php_process);
 extern void php_close(int php_process);
 extern int php_get_process(void);
+
+#endif /* SPINE_PHP_H */

ping.c

@@ -244,6 +244,75 @@ int ping_snmp(host_t *host, ping_t *ping) {
 	}
 }
 
+#ifdef __CYGWIN__
+/*! \fn static void icmp_discard_reply(int icmp_socket, char *buffer)
+ *  \brief consumes a datagram that ping_icmp() has only peeked at
+ *
+ *  Cygwin cannot use MSG_WAITALL on a raw socket, so ping_icmp() reads with
+ *  MSG_PEEK.  A peeked datagram stays queued, so a reply we decline has to be
+ *  drained here.  Otherwise the next pass reads the same bytes again and the
+ *  loop spins until the device timeout expires.
+ */
+static void icmp_discard_reply(int icmp_socket, char *buffer) {
+	while (recvfrom(icmp_socket, buffer, BUFSIZE, 0, NULL, NULL) < 0 && errno == EINTR) {
+		/* interrupted before the datagram was removed, try again */
+	}
+}
+
+#define ICMP_DISCARD_PEEKED(sock, buf) icmp_discard_reply((sock), (buf))
+#else
+#define ICMP_DISCARD_PEEKED(sock, buf) ((void)0)
+#endif
+
+
+/*! \fn spine_icmp_reply_t spine_icmp_classify_reply(const unsigned char *reply, ssize_t len, uint16_t want_id, uint16_t want_seq, const struct icmp **out_pkt)
+ *  \brief bounds-check a raw ICMP reply and decide whether it answers our echo
+ *
+ *  The raw socket is shared across every poller thread and is fed by whatever
+ *  the network sends, so this walks the IP header length field before touching
+ *  the ICMP header.  Split out of ping_icmp() so the fuzz target exercises the
+ *  same code the poller runs.
+ *
+ *  \return the reply classification; *out_pkt is set only for SPINE_ICMP_REPLY_OK
+ */
+spine_icmp_reply_t spine_icmp_classify_reply(const unsigned char *reply, ssize_t len,
+	uint16_t want_id, uint16_t want_seq, const struct icmp **out_pkt) {
+	const struct ip   *iph;
+	const struct icmp *pkt;
+	int ihl;
+
+	if (out_pkt != NULL) {
+		*out_pkt = NULL;
+	}
+
+	if (reply == NULL || len < (ssize_t) sizeof(struct ip)) {
+		return SPINE_ICMP_REPLY_TOO_SHORT;
+	}
+
+	iph = (const struct ip *) reply;
+	ihl = iph->ip_hl << 2;
+
+	if (ihl < (int) sizeof(struct ip) || len < (ssize_t) (ihl + ICMP_HDR_SIZE)) {
+		return SPINE_ICMP_REPLY_BAD_HEADER;
+	}
+
+	pkt = (const struct icmp *) (reply + ihl);
+
+	if (pkt->icmp_type != ICMP_ECHOREPLY) {
+		return SPINE_ICMP_REPLY_NOT_ECHO;
+	}
+
+	if (pkt->icmp_id != want_id || pkt->icmp_seq != want_seq) {
+		return SPINE_ICMP_REPLY_NOT_OURS;
+	}
+
+	if (out_pkt != NULL) {
+		*out_pkt = pkt;
+	}
+
+	return SPINE_ICMP_REPLY_OK;
+}
+
 /*! \fn int ping_icmp(host_t *host, ping_t *ping)
  *  \brief ping a host using an ICMP packet
  *  \param host a pointer to the current host structure
@@ -268,7 +337,6 @@ int ping_icmp(host_t *host, ping_t *ping) {
 	struct sockaddr_in fromname;
 	char   socket_reply[BUFSIZE];
 	int    retry_count;
-	int    ihl;
 	const char *cacti_msg = "cacti-monitoring-system\0";
 	int    packet_len;
 	socklen_t    fromlen;
@@ -277,7 +345,6 @@ int ping_icmp(host_t *host, ping_t *ping) {
 
 	static   unsigned int seq = 0;
 	struct   icmp  *icmp;
-	struct   ip    *ip;
 	struct   icmp  *pkt;
 	unsigned char  *packet;
 
@@ -441,21 +508,30 @@ int ping_icmp(host_t *host, ping_t *ping) {
 							goto keep_listening;
 						}
 					} else {
-						if (return_code < (ssize_t) sizeof(struct ip)) {
+						const struct icmp  *reply_pkt = NULL;
+						spine_icmp_reply_t  verdict;
+
+						verdict = spine_icmp_classify_reply((const unsigned char *) socket_reply,
+							return_code, (uint16_t)(getpid() & 0xFFFF), icmp->icmp_seq, &reply_pkt);
+
+						if (verdict == SPINE_ICMP_REPLY_NOT_OURS) {
+							/* a middlebox that rewrites ICMP query ids is indistinguishable
+							 * from a dead device once the reply is dropped, so record what
+							 * arrived */
+							pkt = (struct icmp *) (socket_reply + (((struct ip *) socket_reply)->ip_hl << 2));
+							SPINE_LOG_DEBUG(("DEBUG: Device[%i] Discarded ICMP reply, expected id:%d seq:%d, received id:%d seq:%d", host->id, (int)(getpid() & 0xFFFF), icmp->icmp_seq, pkt->icmp_id, pkt->icmp_seq));
+						}
+
+						if (verdict != SPINE_ICMP_REPLY_OK && verdict != SPINE_ICMP_REPLY_NOT_ECHO) {
+							ICMP_DISCARD_PEEKED(icmp_socket, socket_reply);
 							goto keep_listening;
 						}
 
-						ip  = (struct ip *) socket_reply;
-						ihl = ip->ip_hl << 2;
-
-						if (ihl < (int) sizeof(struct ip) || return_code < (ssize_t) (ihl + ICMP_HDR_SIZE)) {
-							goto keep_listening;
-						}
-
-						pkt = (struct icmp *) (socket_reply + ihl);
+						pkt = (struct icmp *) reply_pkt;
 
 						if (fromname.sin_addr.s_addr == recvname.sin_addr.s_addr) {
-							if (pkt->icmp_type == ICMP_ECHOREPLY) {
+							if (verdict == SPINE_ICMP_REPLY_OK) {
+
 								if (is_debug_device(host->id)) {
 									SPINE_LOG(("Device[%i] INFO: ICMP Device Alive, Try Count:%i, Time:%.4f ms", host->id, retry_count+1, (total_time)));
 								} else {
@@ -485,6 +561,8 @@ int ping_icmp(host_t *host, ping_t *ping) {
 								return HOST_UP;
 							} else {
 								/* received a response other than an echo reply */
+								ICMP_DISCARD_PEEKED(icmp_socket, socket_reply);
+
 								if (total_time > host_timeout) {
 									retry_count++;
 									total_time = 0;
@@ -494,6 +572,7 @@ int ping_icmp(host_t *host, ping_t *ping) {
 							}
 						} else {
 							/* another host responded */
+							ICMP_DISCARD_PEEKED(icmp_socket, socket_reply);
 							goto keep_listening;
 						}
 					}
@@ -1002,14 +1081,16 @@ 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(hostname));
-	token = strtok(stack, ":");
+	/* obuf includes the NUL, so pass the full buffer size to copy all chars */
+	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));
@@ -1025,10 +1106,10 @@ name_t *get_namebyhost(char *hostname, name_t *name) {
 				strncpy(name->hostname, hostname, sizeof(name->hostname));
 				break;
 			} else if (strlen(token) == 3) {
-				if (strncasecmp(token, "TCP", 3)) {
+				if (strncasecmp(token, "TCP", 3) == 0) {
 					SPINE_LOG_DEBUG(("DEBUG: get_namebyhost(%s) - Have TCPv4 method", hostname));
 					name->method = 1;
-				} else if (strncasecmp(hostname, "UDP", 3)) {
+				} else if (strncasecmp(token, "UDP", 3) == 0) {
 					SPINE_LOG_DEBUG(("DEBUG: get_namebyhost(%s) - Have UDPv4 method", hostname));
 					name->method = 2;
 				} else {
@@ -1037,10 +1118,10 @@ name_t *get_namebyhost(char *hostname, name_t *name) {
 					tokens++;
 				}
 			} else if (strlen(token) == 4) {
-				if (strncasecmp(token, "TCP6", 3)) {
+				if (strncasecmp(token, "TCP6", 4) == 0) {
 					SPINE_LOG_DEBUG(("DEBUG: get_namebyhost(%s) - Have TCPv6 method", hostname));
 					name->method = 3;
-				} else if (strncasecmp(hostname, "UDP6", 3)) {
+				} else if (strncasecmp(token, "UDP6", 4) == 0) {
 					SPINE_LOG_DEBUG(("DEBUG: get_namebyhost(%s) - Have UDPv6 method", hostname));
 					name->method = 4;
 				} else {
@@ -1059,8 +1140,8 @@ name_t *get_namebyhost(char *hostname, name_t *name) {
 
 		if (tokens == 2) {
 			SPINE_LOG_DEBUG(("DEBUG: get_namebyhost(%s) - Setting hostname: %s", hostname, token));
-			strncpy(name->hostname, token, sizeof(name->hostname));
-			name->hostname[strlen(token)] = '\0';
+			strncpy(name->hostname, token, sizeof(name->hostname) - 1);
+			name->hostname[sizeof(name->hostname) - 1] = '\0';
 		}
 
 		if (tokens == 3 && strlen(token)) {
@@ -1071,7 +1152,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) {

ping.h

@@ -31,6 +31,8 @@
  +-------------------------------------------------------------------------+
 */
 
+#ifndef SPINE_PING_H
+#define SPINE_PING_H
 #ifndef ICMP_ECHOREPLY
 #define ICMP_ECHOREPLY		0	/* Echo Reply			*/
 #endif
@@ -131,9 +133,23 @@ struct icmp
 #define	icmp_mask	icmp_dun.id_mask
 #define	icmp_data	icmp_dun.id_data
 };
+
 #endif
 
 /* Host availability functions */
+/* Classification of a raw ICMP reply. Declared here so the fuzz target can
+ * link against the shipped implementation. */
+typedef enum {
+	SPINE_ICMP_REPLY_TOO_SHORT = 0,   /* shorter than an IP header */
+	SPINE_ICMP_REPLY_BAD_HEADER,      /* IHL implausible, or no room for the ICMP header */
+	SPINE_ICMP_REPLY_NOT_ECHO,        /* not an echo reply */
+	SPINE_ICMP_REPLY_NOT_OURS,        /* echo reply, but not our id/sequence */
+	SPINE_ICMP_REPLY_OK
+} spine_icmp_reply_t;
+
+extern spine_icmp_reply_t spine_icmp_classify_reply(const unsigned char *reply,
+	ssize_t len, uint16_t want_id, uint16_t want_seq, const struct icmp **out_pkt);
+
 extern int ping_host(host_t *host, ping_t *ping);
 extern int ping_snmp(host_t *host, ping_t *ping);
 extern int ping_icmp(host_t *host, ping_t *ping);
@@ -144,3 +160,5 @@ extern void update_host_status(int status, host_t *host, ping_t *ping, int avail
 extern int init_sockaddr(struct sockaddr_in *name, const char *hostname, unsigned short int port);
 extern int get_address_type(host_t *host);
 extern unsigned short int get_checksum(void* buf, int len);
+
+#endif /* SPINE_PING_H */

poller.c

@@ -218,9 +218,15 @@ void poll_host(int device_counter, int host_id, int host_thread, int host_thread
 	target_t    *poller_items = NULL;
 	snmp_oids_t *snmp_oids = NULL;
 
-	error_string = malloc(DBL_BUFSIZE);
-	buf_size     = malloc(sizeof(int));
-	buf_errors   = malloc(sizeof(int));
+	if (!(error_string = malloc(DBL_BUFSIZE))) {
+		die("ERROR: Fatal malloc error: poller.c error_string!");
+	}
+	if (!(buf_size = malloc(sizeof(int)))) {
+		die("ERROR: Fatal malloc error: poller.c buf_size!");
+	}
+	if (!(buf_errors = malloc(sizeof(int)))) {
+		die("ERROR: Fatal malloc error: poller.c buf_errors!");
+	}
 
 	*buf_size     = 0;
 	*buf_errors   = 0;
@@ -1279,7 +1285,6 @@ void poll_host(int device_counter, int host_id, int host_thread, int host_thread
 
 	if (num_rows > 0) {
 		/* retrieve each hosts polling items from poller cache and load into array */
-		/* retreive each hosts polling items from poller cache and load into array */
 		if (!(poller_items = (target_t *) calloc(num_rows, sizeof(target_t)))) {
 			die("ERROR: Fatal calloc error: poller.c poller_items!");
 		}
@@ -1812,6 +1817,11 @@ void poll_host(int device_counter, int host_id, int host_thread, int host_thread
 					}
 				}
 
+				if (strlen(poller_items[snmp_oids[j].array_position].output_regex)) {
+					snprintf(temp_result, RESULTS_BUFFER, "%s", regex_replace(poller_items[snmp_oids[j].array_position].output_regex, snmp_oids[j].result));
+					snprintf(snmp_oids[j].result, RESULTS_BUFFER, "%s", temp_result);
+				}
+
 				snprintf(poller_items[snmp_oids[j].array_position].result, RESULTS_BUFFER, "%s", snmp_oids[j].result);
 
 				thread_end = get_time_as_double();
@@ -2338,17 +2348,21 @@ char *exec_poll(host_t *current_host, char *command, int id, const char *type) {
 		sem_err = spine_sem_trywait(&available_scripts);
 		if (sem_err == 0) {
 			break;
-		} else if (sem_err == EAGAIN || sem_err == EWOULDBLOCK) {
-			if (is_debug_device(current_host->id)) {
-				SPINE_LOG(("Device[%i] DEBUG: Pausing as unable to obtain a script execution lock", current_host->id));
-			} else {
-				SPINE_LOG_DEVDBG(("Device[%i] DEBUG: Pausing as unable to obtain a script execution lock", current_host->id));
-			}
 		} else {
-			if (is_debug_device(current_host->id)) {
-				SPINE_LOG(("Device[%i] DEBUG: Pausing as error %d whilst obtaining a script execution lock", current_host->id, sem_err));
+			int sem_errno = errno;
+
+			if (sem_errno == EAGAIN || sem_errno == EWOULDBLOCK) {
+				if (is_debug_device(current_host->id)) {
+					SPINE_LOG(("Device[%i] DEBUG: Pausing as unable to obtain a script execution lock", current_host->id));
+				} else {
+					SPINE_LOG_DEVDBG(("Device[%i] DEBUG: Pausing as unable to obtain a script execution lock", current_host->id));
+				}
 			} else {
-				SPINE_LOG_DEVDBG(("Device[%i] DEBUG: Pausing as error %d whilst obtaining a script execution lock", current_host->id, sem_err));
+				if (is_debug_device(current_host->id)) {
+					SPINE_LOG(("Device[%i] DEBUG: Pausing as error %d whilst obtaining a script execution lock", current_host->id, sem_errno));
+				} else {
+					SPINE_LOG_DEVDBG(("Device[%i] DEBUG: Pausing as error %d whilst obtaining a script execution lock", current_host->id, sem_errno));
+				}
 			}
 		}
 		usleep(10000);

poller.h

@@ -31,6 +31,8 @@
  +-------------------------------------------------------------------------+
 */
 
+#ifndef SPINE_POLLER_H
+#define SPINE_POLLER_H
 extern void *child(void *arg);
 extern void child_cleanup(void *arg);
 extern void child_cleanup_thread(void *arg);
@@ -41,3 +43,5 @@ extern void get_system_information(host_t *host, MYSQL *mysql, int system);
 extern int is_multipart_output(char *result);
 extern int validate_result(char *result);
 extern void buffer_output_errors(char * error_string, int * buf_size, int * buf_errors, int device_id, int thread_id, int local_data_id, bool flush);
+
+#endif /* SPINE_POLLER_H */

snmp.c

@@ -195,6 +195,7 @@ void *snmp_host_init(int host_id, char *hostname, int snmp_version, char *snmp_c
 	session.peername    = strdup(hostnameport);
 	if (!session.peername) {
 		SPINE_LOG(("Device[%i] ERROR: Failed to allocate peername for '%s'", host_id, hostname));
+		free(session.localname);
 		return 0;
 	}
 	session.retries     = set.snmp_retries;
@@ -408,6 +409,26 @@ void snmp_host_cleanup(void *snmp_session) {
 	}
 }
 
+/*! \fn int snmp_varbind_is_exception(const struct variable_list *vars)
+ *  \brief report whether a varbind carries a per-OID exception rather than data
+ *
+ *  Under SNMPv2c an agent reports a single unanswerable OID as an inline
+ *  exception varbind while the PDU errstat stays SNMP_ERR_NOERROR. Passing one
+ *  to snprint_value() renders the exception as text, which then gets stored as
+ *  though it were a real value.
+ *
+ *  \return TRUE when the varbind is an exception
+ */
+int snmp_varbind_is_exception(const struct variable_list *vars) {
+	if (vars == NULL) {
+		return FALSE;
+	}
+
+	return (vars->type == SNMP_NOSUCHOBJECT ||
+		vars->type == SNMP_NOSUCHINSTANCE ||
+		vars->type == SNMP_ENDOFMIBVIEW);
+}
+
 /*! \fn char *snmp_get_base(host_t *current_host, const char *snmp_oid, bool should_fail)
  *  \brief performs a single snmp_get for a specific snmp OID
  *
@@ -728,7 +749,7 @@ char *snmp_getnext(host_t *current_host, const char *snmp_oid) {
 				if (response->errstat == SNMP_ERR_NOERROR) {
 					vars = response->variables;
 
-					if (vars != NULL) {
+					if (vars != NULL && !snmp_varbind_is_exception(vars)) {
 						snprint_value(temp_result, RESULTS_BUFFER, vars->name, vars->name_length, vars);
 
 						snprint_asciistring(result_string, RESULTS_BUFFER, (unsigned char *)temp_result, strlen(temp_result));
@@ -890,8 +911,7 @@ int snmp_count(host_t *current_host, const char *snmp_oid) {
 
 			if (status == STAT_SUCCESS) {
 				if (response == NULL) {
-					SPINE_LOG(("ERROR: An internal Net-Snmp error condition detected in Cacti snmp_count"));
-					status = STAT_ERROR;
+					SPINE_LOG(("ERROR: Device[%i] internal Net-SNMP error in snmp_count for OID %s", current_host->id, snmp_oid));
 					ok = 0;
 					error_occurred = 1;
 				} else if (response->errstat == SNMP_ERR_NOERROR) {
@@ -923,14 +943,16 @@ int snmp_count(host_t *current_host, const char *snmp_oid) {
 						}
 					}
 				} else {
-					SPINE_LOG(("ERROR: An internal Net-Snmp error condition detected in Cacti snmp_count"));
+					SPINE_LOG(("ERROR: Device[%i] internal Net-SNMP error in snmp_count for OID %s", current_host->id, snmp_oid));
+					ok = 0;
+					error_occurred = 1;
 				}
 			} else if (status == STAT_TIMEOUT) {
-				SPINE_LOG(("ERROR: Timeout detected in Cacti snmp_count"));
+				SPINE_LOG(("ERROR: Device[%i] timeout in snmp_count for OID %s", current_host->id, snmp_oid));
 				ok = 0;
 				error_occurred = 1;
 			} else { /* status == STAT_ERROR */
-				SPINE_LOG(("ERROR: An internal Net-Snmp error condition detected in Cacti snmp_count (STAT_ERROR)"));
+				SPINE_LOG(("ERROR: Device[%i] internal Net-SNMP error in snmp_count for OID %s (STAT_ERROR)", current_host->id, snmp_oid));
 				ok = 0;
 				error_occurred = 1;
 			}
@@ -1044,9 +1066,19 @@ void snmp_get_multi(host_t *current_host, target_t *poller_items, snmp_oids_t *s
 
 				for (i = 0; i < num_oids && vars; i++) {
 					if (!IS_UNDEFINED(snmp_oids[i].result)) {
-						snprint_value(temp_result, RESULTS_BUFFER, vars->name, vars->name_length, vars);
+						/* Under v2c an agent reports a per-OID failure as an
+						 * exception varbind while the PDU errstat stays
+						 * NOERROR.  Without this check snprint_value() renders
+						 * the exception as text and it is stored as a value. */
+						if (snmp_varbind_is_exception(vars)) {
+							SPINE_LOG_HIGH(("Device[%i] WARNING: No SNMP data returned for OID '%s'", current_host->id, snmp_oids[i].oid));
 
-						snprintf(snmp_oids[i].result, RESULTS_BUFFER, "%s", trim(temp_result));
+							SET_UNDEFINED(snmp_oids[i].result);
+						} else {
+							snprint_value(temp_result, RESULTS_BUFFER, vars->name, vars->name_length, vars);
+
+							snprintf(snmp_oids[i].result, RESULTS_BUFFER, "%s", trim(temp_result));
+						}
 
 						vars = vars->next_variable;
 					}

snmp.h

@@ -31,6 +31,8 @@
  +-------------------------------------------------------------------------+
 */
 
+#ifndef SPINE_SNMP_H
+#define SPINE_SNMP_H
 #define SNMP_SESSION_FREE(s) { if (s != NULL) { snmp_host_cleanup(s); s = NULL; } }
 
 extern void snmp_spine_init(void);
@@ -38,8 +40,11 @@ extern void snmp_spine_close(void);
 extern void *snmp_host_init(int host_id, char *hostname, int snmp_version, char *snmp_community, char *snmp_username, char *snmp_password, char *snmp_auth_protocol, char *snmp_priv_passphrase, char *snmp_priv_protocol, char *snmp_context, char *snmp_engine_id, int snmp_port, int snmp_timeout);
 extern void snmp_host_cleanup(void *snmp_session);
 extern char *snmp_get_base(host_t *current_host, const char *snmp_oid, bool should_fail);
+extern int snmp_varbind_is_exception(const struct variable_list *vars);
 extern char *snmp_get(host_t *current_host, const char *snmp_oid);
 extern char *snmp_getnext(host_t *current_host, const char *snmp_oid);
 extern int snmp_count(host_t *current_host, const char *snmp_oid);
 extern void snmp_get_multi(host_t *current_host, target_t *poller_items, snmp_oids_t *snmp_oids, int num_oids);
 extern void snmp_snprint_value(char *obuf, size_t buf_len, const oid *objid, size_t objidlen, struct variable_list *variable);
+
+#endif /* SPINE_SNMP_H */

spine.c

@@ -421,10 +421,10 @@ int main(int argc, char *argv[]) {
 
 			if (value == NULL) {
 				die("ERROR: -O requires setting:value");
+			} else {
+				*value++ = '\0';
 			}
 
-			*value++ = '\0';
-
 			set_option(setting, value);
 		}
 
@@ -768,6 +768,7 @@ int main(int argc, char *argv[]) {
 			mysql_row       = mysql_fetch_row(result);
 
 			if (mysql_row == NULL) {
+				/* fewer device rows than expected; stop processing */
 				break;
 			}
 
@@ -791,9 +792,14 @@ int main(int argc, char *argv[]) {
 			}
 
 			tresult   = db_query(&mysql, LOCAL, querybuf);
-			mysql_row = mysql_fetch_row(tresult);
+			mysql_row = (tresult != NULL) ? mysql_fetch_row(tresult) : NULL;
+
+			if (mysql_row == NULL) {
+				total_items = 0;
+			} else {
+				total_items = atoi(mysql_row[0]);
+			}
 
-			total_items = (mysql_row != NULL) ? atoi(mysql_row[0]) : 0;
 			db_free_result(tresult);
 
 			if (total_items && total_items < device_threads) {
@@ -815,9 +821,13 @@ int main(int argc, char *argv[]) {
 				}
 
 				tresult   = db_query(&mysql, LOCAL, querybuf);
-				mysql_row = mysql_fetch_row(tresult);
+				mysql_row = (tresult != NULL) ? mysql_fetch_row(tresult) : NULL;
 
-				items_per_thread = (mysql_row != NULL) ? atoi(mysql_row[0]) : 0;
+				if (mysql_row == NULL) {
+					items_per_thread = 0;
+				} else {
+					items_per_thread = atoi(mysql_row[0]);
+				}
 
 				db_free_result(tresult);
 
@@ -830,9 +840,13 @@ int main(int argc, char *argv[]) {
 		} else {
 			snprintf(querybuf, BIG_BUFSIZE, "SELECT SQL_NO_CACHE COUNT(local_data_id) FROM poller_item WHERE host_id=%i AND rrd_next_step <=0", host_id);
 			tresult   = db_query(&mysql, LOCAL, querybuf);
-			mysql_row = mysql_fetch_row(tresult);
+			mysql_row = (tresult != NULL) ? mysql_fetch_row(tresult) : NULL;
 
-			items_per_thread = (mysql_row != NULL) ? atoi(mysql_row[0]) : 0;
+			if (mysql_row == NULL) {
+				items_per_thread = 0;
+			} else {
+				items_per_thread = atoi(mysql_row[0]);
+			}
 
 			db_free_result(tresult);
 
@@ -914,16 +928,14 @@ int main(int argc, char *argv[]) {
 			sem_err = spine_sem_trywait(&thread_init_sem);
 
 			if (sem_err == 0) {
-				// Acquired a thread
+				/* acquired the thread init lock */
 				break;
-			} else if (sem_err == EINTR) {
-				// Interrupted by signal handler
-			} else if (sem_err == EDEADLK) {
-				SPINE_LOG_DEVDBG(("WARNING: Device[%i] HT[%i] would have deadlocked acquiring Thread Initialization Lock", host_id, current_thread));
-			} else if (sem_err == EAGAIN) {
-				// Keep trying
 			} else {
-				SPINE_LOG_DEVDBG(("WARNING: Device[%i] HT[%i] errored with %d while acquiring Thread Initialization Lock", host_id, current_thread, sem_err));
+				int sem_errno = errno;
+
+				if (sem_errno != EAGAIN) {
+					SPINE_LOG_DEVDBG(("WARNING: Device[%i] HT[%i] errored with %d while acquiring Thread Initialization Lock", host_id, current_thread, sem_errno));
+				}
 			}
 
 			if (loop_count == 10) {
@@ -968,7 +980,8 @@ int main(int argc, char *argv[]) {
 				spine_sem_getvalue(&available_threads, &a_threads_value);
 				SPINE_LOG_HIGH(("Device[%i] DEBUG: Available Threads is %i (%i outstanding)", poller_details->host_id, a_threads_value, set.threads - a_threads_value));
 
-				spine_sem_post(&thread_init_sem);
+				/* the child releases thread_init_sem once it has copied poller_details
+				 * and dropped LOCK_HOST_TIME; posting here too double-counts the semaphore */
 
 				SPINE_LOG_DEVDBG(("DEBUG: DTS: device = %d, host_id = %d, host_thread = %d,"
 					" host_threads = %d, host_data_ids = %d, complete = %d",

spine_sem.h

@@ -67,6 +67,7 @@ static inline int spine_sem_wait(spine_sem_t *s) {
 	return 0;
 }
 
+/* Returns 0 when a token is acquired, or -1 with errno set like sem_trywait. */
 static inline int spine_sem_trywait(spine_sem_t *s) {
 	pthread_mutex_lock(&s->mutex);
 	if (s->value <= 0) {

sql.h

@@ -31,6 +31,8 @@
  +-------------------------------------------------------------------------+
 */
 
+#ifndef SPINE_SQL_H
+#define SPINE_SQL_H
 extern int db_insert(MYSQL *mysql, int type, const char *query);
 extern MYSQL_RES *db_query(MYSQL *mysql, int type, const char *query);
 extern void db_connect(int type, MYSQL *mysql);
@@ -52,4 +54,6 @@ extern int append_hostrange(char *obuf, const char *colname);
 	if (options_error < 0) {\
 	        die("FATAL: MySQL options unable to set %s option", desc);\
 	}\
-}\
+}
+
+#endif /* SPINE_SQL_H */

util.c

@@ -61,13 +61,96 @@ static struct {
  */
 void set_option(const char *option, const char *value) {
 	if (nopts >= (int)(sizeof(opttable) / sizeof(opttable[0]))) {
-		die("ERROR: Too many command-line options specified");
+		die("ERROR: too many --option overrides");
 	}
 
 	opttable[nopts  ].opt = option;
 	opttable[nopts++].val = value;
 }
 
+/* Settings cache.
+ *
+ * read_config_options() looks up two dozen settings and each one was its own
+ * round trip.  The settings table is small, so it is read once up front and
+ * served from memory for the duration of that call.  Only ever populated and
+ * used from read_config_options(), which runs before any poller thread
+ * exists, so no locking is required.
+ */
+typedef struct setting_cache_entry {
+	char *name;
+	char *value;
+} setting_cache_t;
+
+static setting_cache_t *settings_cache       = NULL;
+static int              settings_cache_count = 0;
+
+static void settings_cache_free(void) {
+	int i;
+
+	if (settings_cache == NULL) return;
+
+	for (i = 0; i < settings_cache_count; i++) {
+		free(settings_cache[i].name);
+		free(settings_cache[i].value);
+	}
+
+	free(settings_cache);
+	settings_cache       = NULL;
+	settings_cache_count = 0;
+}
+
+static void settings_cache_load(MYSQL *psql, int mode) {
+	MYSQL_RES *result;
+	MYSQL_ROW  row;
+	my_ulonglong rows;
+	setting_cache_t *table;
+	int i = 0;
+
+	assert(psql != 0);
+
+	settings_cache_free();
+
+	result = db_query(psql, mode, "SELECT SQL_NO_CACHE name, value FROM settings");
+
+	if (result == NULL) return;
+
+	rows = mysql_num_rows(result);
+
+	if (rows == 0 || rows > (my_ulonglong) INT_MAX) {
+		db_free_result(result);
+		return;
+	}
+
+	table = (setting_cache_t *) calloc((size_t) rows, sizeof(setting_cache_t));
+
+	if (table == NULL) {
+		db_free_result(result);
+		return;
+	}
+
+	while ((row = mysql_fetch_row(result)) != NULL && i < (int) rows) {
+		if (row[0] == NULL) continue;
+
+		table[i].name  = strdup(row[0]);
+		table[i].value = strdup(row[1] != NULL ? row[1] : "");
+
+		if (table[i].name == NULL || table[i].value == NULL) {
+			free(table[i].name);
+			free(table[i].value);
+			break;
+		}
+
+		i++;
+	}
+
+	db_free_result(result);
+
+	settings_cache       = table;
+	settings_cache_count = i;
+
+	SPINE_LOG_DEBUG(("DEBUG: Loaded %i Cacti settings in one query", i));
+}
+
 /*! \fn static char *getsetting(MYSQL *psql, int mode, const char *setting)
  *  \brief Returns a character pointer to a Cacti setting.
  *
@@ -100,6 +183,17 @@ static char *getsetting(MYSQL *psql, int mode, const char *setting) {
 		}
 	}
 
+	/* served from the bulk-loaded table when read_config_options() is running */
+	if (settings_cache != NULL) {
+		for (i = 0; i < settings_cache_count; i++) {
+			if (STRIMATCH(setting, settings_cache[i].name)) {
+				return strdup(settings_cache[i].value);
+			}
+		}
+
+		return strdup("");
+	}
+
 	snprintf(qstring, sizeof(qstring), "SELECT SQL_NO_CACHE value FROM settings WHERE name = '%s'", setting);
 
 	result = db_query(psql, mode, qstring);
@@ -331,6 +425,83 @@ int is_debug_device(int device_id) {
 	return FALSE;
 }
 
+/*! \fn static void read_availability_settings(MYSQL *psql)
+ *  \brief load the device availability and ping settings
+ *
+ *  Split out of read_config_options() unchanged; the settings and their
+ *  fallbacks are exactly as they were.
+ */
+static void read_availability_settings(MYSQL *psql) {
+	char *res;
+
+	/* set availability_method */
+	if ((res = getsetting(psql, LOCAL, "availability_method")) != 0) {
+		set.availability_method = atoi(res);
+		free(res);
+	}
+
+	/* log the availability_method variable */
+	SPINE_LOG_DEBUG(("DEBUG: The availability_method variable is %i", set.availability_method));
+
+	/* set ping_recovery_count */
+	if ((res = getsetting(psql, LOCAL, "ping_recovery_count")) != 0) {
+		set.ping_recovery_count = atoi(res);
+		free(res);
+	}
+
+	/* log the ping_recovery_count variable */
+	SPINE_LOG_DEBUG(("DEBUG: The ping_recovery_count variable is %i", set.ping_recovery_count));
+
+	/* set ping_failure_count */
+	if ((res = getsetting(psql, LOCAL, "ping_failure_count")) != 0) {
+		set.ping_failure_count = atoi(res);
+		free(res);
+	}
+
+	/* log the ping_failure_count variable */
+	SPINE_LOG_DEBUG(("DEBUG: The ping_failure_count variable is %i", set.ping_failure_count));
+
+	/* set ping_method */
+	if ((res = getsetting(psql, LOCAL, "ping_method")) != 0) {
+		set.ping_method = atoi(res);
+		free(res);
+	}
+
+	/* log the ping_method variable */
+	SPINE_LOG_DEBUG(("DEBUG: The ping_method variable is %i", set.ping_method));
+
+	/* set ping_retries */
+	if ((res = getsetting(psql, LOCAL, "ping_retries")) != 0) {
+		set.ping_retries = atoi(res);
+		free(res);
+	}
+
+	/* log the ping_retries variable */
+	SPINE_LOG_DEBUG(("DEBUG: The ping_retries variable is %i", set.ping_retries));
+
+	/* set ping_timeout */
+	if ((res = getsetting(psql, LOCAL, "ping_timeout")) != 0) {
+		set.ping_timeout = atoi(res);
+		free(res);
+	} else {
+		set.ping_timeout = 400;
+	}
+
+	/* log the ping_timeout variable */
+	SPINE_LOG_DEBUG(("DEBUG: The ping_timeout variable is %i", set.ping_timeout));
+
+	/* set snmp_retries */
+	if ((res = getsetting(psql, LOCAL, "snmp_retries")) != 0) {
+		set.snmp_retries = atoi(res);
+		free(res);
+	} else {
+		set.snmp_retries = 3;
+	}
+
+	/* log the snmp_retries variable */
+	SPINE_LOG_DEBUG(("DEBUG: The snmp_retries variable is %i", set.snmp_retries));
+}
+
 /*! \fn void read_config_options(void)
  *  \brief Reads the default Spine runtime parameters from the database and set's the global array
  *
@@ -351,6 +522,8 @@ void read_config_options(void) {
 	char       spine_auth[BUFSIZE];
 	char       spine_capabilities[BUFSIZE];
 
+	web_root[0] = '\0';
+
 	/* publish spine snmpv3 capabilities to the database */
 	memset(spine_capabilities, 0, sizeof(spine_capabilities));
 	memset(spine_priv, 0, sizeof(spine_priv));
@@ -358,6 +531,9 @@ void read_config_options(void) {
 
 	db_connect(LOCAL, &mysql);
 
+	/* one round trip instead of one per setting */
+	settings_cache_load(&mysql, LOCAL);
+
 	if (set.poller_id > 1 && set.mode == REMOTE_ONLINE) {
 		db_connect(REMOTE, &mysqlr);
 		mode = REMOTE;
@@ -445,7 +621,7 @@ void read_config_options(void) {
 	/* get log date format */
 	if ((res = getsetting(&mysql, LOCAL, "default_date_format")) != 0) {
 		set.log_datetime_format = atoi(res);
-		free((char *)res);
+		free(res);
 
 		if (set.log_datetime_format < GD_MIN || set.log_datetime_format > GD_MAX) {
 			set.log_datetime_format = GD_DEFAULT;
@@ -485,72 +661,7 @@ void read_config_options(void) {
 	/* log the path_php variable */
 	SPINE_LOG_DEBUG(("DEBUG: The path_php variable is %s", set.path_php));
 
-	/* set availability_method */
-	if ((res = getsetting(&mysql, LOCAL, "availability_method")) != 0) {
-		set.availability_method = atoi(res);
-		free(res);
-	}
-
-	/* log the availability_method variable */
-	SPINE_LOG_DEBUG(("DEBUG: The availability_method variable is %i", set.availability_method));
-
-	/* set ping_recovery_count */
-	if ((res = getsetting(&mysql, LOCAL, "ping_recovery_count")) != 0) {
-		set.ping_recovery_count = atoi(res);
-		free(res);
-	}
-
-	/* log the ping_recovery_count variable */
-	SPINE_LOG_DEBUG(("DEBUG: The ping_recovery_count variable is %i", set.ping_recovery_count));
-
-	/* set ping_failure_count */
-	if ((res = getsetting(&mysql, LOCAL, "ping_failure_count")) != 0) {
-		set.ping_failure_count = atoi(res);
-		free(res);
-	}
-
-	/* log the ping_failure_count variable */
-	SPINE_LOG_DEBUG(("DEBUG: The ping_failure_count variable is %i", set.ping_failure_count));
-
-	/* set ping_method */
-	if ((res = getsetting(&mysql, LOCAL, "ping_method")) != 0) {
-		set.ping_method = atoi(res);
-		free(res);
-	}
-
-	/* log the ping_method variable */
-	SPINE_LOG_DEBUG(("DEBUG: The ping_method variable is %i", set.ping_method));
-
-	/* set ping_retries */
-	if ((res = getsetting(&mysql, LOCAL, "ping_retries")) != 0) {
-		set.ping_retries = atoi(res);
-		free(res);
-	}
-
-	/* log the ping_retries variable */
-	SPINE_LOG_DEBUG(("DEBUG: The ping_retries variable is %i", set.ping_retries));
-
-	/* set ping_timeout */
-	if ((res = getsetting(&mysql, LOCAL, "ping_timeout")) != 0) {
-		set.ping_timeout = atoi(res);
-		free(res);
-	} else {
-		set.ping_timeout = 400;
-	}
-
-	/* log the ping_timeout variable */
-	SPINE_LOG_DEBUG(("DEBUG: The ping_timeout variable is %i", set.ping_timeout));
-
-	/* set snmp_retries */
-	if ((res = getsetting(&mysql, LOCAL, "snmp_retries")) != 0) {
-		set.snmp_retries = atoi(res);
-		free(res);
-	} else {
-		set.snmp_retries = 3;
-	}
-
-	/* log the snmp_retries variable */
-	SPINE_LOG_DEBUG(("DEBUG: The snmp_retries variable is %i", set.snmp_retries));
+	read_availability_settings(&mysql);
 
 	/* set logging option for errors */
 	set.log_perror = getboolsetting(&mysql, LOCAL, "log_perror", FALSE);
@@ -814,6 +925,8 @@ void read_config_options(void) {
 	if (set.poller_id > 1 && set.mode == REMOTE_ONLINE) {
 		db_disconnect(&mysqlr);
 	}
+
+	settings_cache_free();
 }
 
 void poller_push_data_to_main(void) {
@@ -1762,7 +1875,9 @@ char *strncopy(char *dst, const char *src, size_t obuf) {
 	copy_len = strnlen(src, obuf - 1);
 
 	if (copy_len) {
-		strncpy(dst, src, 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[copy_len] = '\0';

util.h

@@ -31,6 +31,8 @@
  +-------------------------------------------------------------------------+
 */
 
+#ifndef SPINE_UTIL_H
+#define SPINE_UTIL_H
 /* cacti config reading functions */
 extern void read_config_options(void);
 extern int read_spine_config(const char *file);
@@ -108,3 +110,5 @@ extern double start_time;
 
 /* the version of Cacti as a decimal */
 int get_cacti_version(MYSQL *psql, int mode);
+
+#endif /* SPINE_UTIL_H */