fix: allocation checks, a log buffer overflow, result leaks, and a per-line rebuild

CHANGELOG

@@ -26,6 +26,10 @@ The Cacti Group | spine
 -issue#552: Terminate die() output so consecutive fatal messages no longer run together
 -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#564: Check the calloc() results in spine.c so an allocation failure dies instead of dereferencing NULL
+-issue#565: Bound the newline appended by spine_log() so a full log line cannot write past flogmessage
+-issue#566: Free the result set on the NULL-row branch of the util.c settings helpers
+-issue#567: Build the log timestamp format once at config load instead of rebuilding it on every log line
 -issue: Correct signed and unsigned printf format specifiers in poller.c, free session.localname on the unknown-version return in snmp.c, and quote shell variables in the build scripts
 -issue: Escape the SNMP result and RRD name before the poller_output INSERT, bound the buffer_output_errors write to the space left in error_string, and validate the --hostlist argument before it reaches SQL
 -issue: Restore the twelve headers and spine.conf.dist missing from the dist tarball so a release tarball can be compiled from

Makefile.am

@@ -65,11 +65,13 @@ check_PROGRAMS += \
 	tests/unit/test_util_strings \
 	tests/unit/test_build_fixes \
 	tests/unit/test_safety_fixes \
-	tests/unit/test_linked
+	tests/unit/test_linked \
+	tests/unit/test_log_newline_bound \
+	tests/unit/test_util_paths
 endif
 
 # test_util_strings pulls in common.h and the sources under test, so it needs
-# the same libraries spine links against.  The other two are self-contained.
+# the same libraries spine links against.  The others are self-contained.
 tests_unit_test_util_strings_SOURCES = tests/unit/test_util_strings.c
 tests_unit_test_util_strings_LDADD   = $(CMOCKA_LIBS) $(LIBS)
 
@@ -86,4 +88,11 @@ tests_unit_test_linked_SOURCES = tests/unit/test_linked.c tests/fuzz/stubs.c \
 	sql.c util.c snmp.c locks.c poller.c nft_popen.c php.c ping.c keywords.c error.c
 tests_unit_test_linked_LDADD   = $(CMOCKA_LIBS) $(LIBS)
 
+tests_unit_test_log_newline_bound_SOURCES = tests/unit/test_log_newline_bound.c
+tests_unit_test_log_newline_bound_LDADD   = $(CMOCKA_LIBS)
+
+# includes util.c, so it needs the same libraries spine links against
+tests_unit_test_util_paths_SOURCES = tests/unit/test_util_paths.c
+tests_unit_test_util_paths_LDADD   = $(CMOCKA_LIBS) $(LIBS)
+
 TESTS = $(check_PROGRAMS)

spine.c

@@ -245,13 +245,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 calloc 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(MAX_DEBUG_DEVICES, sizeof(int));
+	if (!(debug_devices = calloc(MAX_DEBUG_DEVICES, sizeof(int)))) {
+		die("ERROR: Fatal calloc error: spine.c debug_devices!");
+	}
 
 	/* initialize icmp_avail */
 	set.icmp_avail = TRUE;
@@ -552,15 +557,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 calloc 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 calloc error: spine.c db_pool_remote!");
+		}
+
 		db_create_connection_pool(REMOTE);
 	} else {
 		mode = LOCAL;

tests/integration/test_alloc_failure.sh

@@ -0,0 +1,120 @@
+#!/usr/bin/env bash
+# Integration test for the startup allocation guards (issue#564).
+#
+# php_processes and debug_devices are dereferenced on the line after their
+# calloc(), so an allocation failure there used to be a NULL deref rather than
+# a diagnosable exit.  A unit test cannot reach them: they live in main().
+# This injects the failure into the real binary with an LD_PRELOAD calloc that
+# returns NULL on the Nth call, and asserts spine dies with its own message.
+#
+# The db_pool_local and db_pool_remote guards are not covered here.  They run
+# after the database connection, so reaching them needs the SNMPv3 harness's
+# MySQL container rather than a bare binary.
+#
+# Skips with exit 77 (automake and prove read that as "skipped") when the
+# binary, a compiler, or LD_PRELOAD interposition is unavailable, so the
+# suite stays green on platforms where this technique does not apply.
+#
+# Usage: ./tests/integration/test_alloc_failure.sh
+set -euo pipefail
+
+SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)"
+REPO_ROOT="$(cd "$SCRIPT_DIR/../.." && pwd)"
+PASS=0
+FAIL=0
+
+pass() { echo "  PASS: $*"; PASS=$((PASS+1)); }
+fail() { echo "  FAIL: $*"; FAIL=$((FAIL+1)); }
+
+SPINE=""
+if [[ -x "$REPO_ROOT/spine" ]]; then
+	SPINE="$REPO_ROOT/spine"
+elif command -v spine >/dev/null 2>&1; then
+	SPINE="$(command -v spine)"
+fi
+
+if [[ -z "$SPINE" ]]; then
+	echo "no spine binary found; skipping"
+	exit 77
+fi
+
+if ! command -v cc >/dev/null 2>&1; then
+	echo "no compiler available for the interposer; skipping"
+	exit 77
+fi
+
+WORK="$(mktemp -d)"
+trap 'rm -rf "$WORK"' EXIT
+
+cat > "$WORK/failcalloc.c" <<'INTERPOSER'
+#define _GNU_SOURCE
+#include <dlfcn.h>
+#include <stdlib.h>
+
+/* Return NULL from the Nth calloc() of the process, pass the rest through. */
+static long seen;
+static long fail_at = -1;
+
+void *calloc(size_t nmemb, size_t size) {
+	static void *(*real_calloc)(size_t, size_t);
+
+	if (real_calloc == NULL) {
+		const char *at;
+
+		real_calloc = dlsym(RTLD_NEXT, "calloc");
+		at = getenv("SPINE_FAIL_CALLOC_AT");
+
+		if (at != NULL) {
+			fail_at = atol(at);
+		}
+	}
+
+	if (++seen == fail_at) {
+		return NULL;
+	}
+
+	return real_calloc(nmemb, size);
+}
+INTERPOSER
+
+if ! cc -shared -fPIC -o "$WORK/failcalloc.so" "$WORK/failcalloc.c" -ldl 2>/dev/null; then
+	echo "could not build the LD_PRELOAD interposer; skipping"
+	exit 77
+fi
+
+# Sanity check: without injection --help must still work, otherwise the
+# interposer itself is broken and every assertion below would be meaningless.
+if ! LD_PRELOAD="$WORK/failcalloc.so" "$SPINE" --help >/dev/null 2>&1; then
+	echo "LD_PRELOAD interposition not supported here; skipping"
+	exit 77
+fi
+
+echo "Allocation failure guards:"
+
+# --help exits before the database connection, so only the two allocations at
+# the top of main() are reachable.  They are the first and second calloc() the
+# process makes.
+check_guard() {
+	local nth="$1" want="$2" out
+
+	out="$(SPINE_FAIL_CALLOC_AT="$nth" LD_PRELOAD="$WORK/failcalloc.so" "$SPINE" --help 2>&1 || true)"
+
+	if grep -q "Fatal calloc error: spine.c $want" <<<"$out"; then
+		pass "calloc #$nth failing reports $want"
+	else
+		fail "calloc #$nth failing did not report $want (got: ${out:0:120})"
+	fi
+
+	if grep -qiE 'segmentation fault|signal 11' <<<"$out"; then
+		fail "calloc #$nth failing crashed instead of exiting cleanly"
+	else
+		pass "calloc #$nth failing did not crash"
+	fi
+}
+
+check_guard 1 php_processes
+check_guard 2 debug_devices
+
+echo
+echo "passed: $PASS, failed: $FAIL"
+[[ "$FAIL" -eq 0 ]]

tests/unit/Makefile

@@ -48,7 +48,9 @@ LDLIBS := $(CMOCKA_LIBS)
 BUILD_FIXES   := $(BINDIR)/test_build_fixes
 SAFETY_FIXES  := $(BINDIR)/test_safety_fixes
 STRINGS       := $(BINDIR)/test_util_strings
-TARGETS       := $(BUILD_FIXES) $(SAFETY_FIXES) $(STRINGS)
+NEWLINE_BOUND := $(BINDIR)/test_log_newline_bound
+UTIL_PATHS    := $(BINDIR)/test_util_paths
+TARGETS       := $(BUILD_FIXES) $(SAFETY_FIXES) $(STRINGS) $(NEWLINE_BOUND) $(UTIL_PATHS)
 
 .PHONY: all build run clean
 

tests/unit/test_linked.c

@@ -387,34 +387,37 @@ static void test_read_spine_config_reads_settings(void **state) {
 	remove(path);
 }
 
-/* --- get_date_format(): every format and separator is owned by the caller -- */
+/* --- get_date_format(): cached storage, rebuilt by set_date_format() ------ */
 
-static void test_get_date_format_returns_owned_memory(void **state) {
+static void test_get_date_format_returns_cached_storage(void **state) {
 	char *fmt;
 	(void) state;
 
 	config_defaults();
+	set_date_format();
 	fmt = get_date_format();
 
 	assert_non_null(fmt);
 	assert_true(strlen(fmt) > 0);
-	free(fmt);
+
+	/* the buffer belongs to util.c and is handed out, not owned by us */
+	assert_ptr_equal(fmt, get_date_format());
 }
 
-static void test_get_date_format_clamps_an_out_of_range_format(void **state) {
+static void test_set_date_format_clamps_an_out_of_range_format(void **state) {
 	char *fmt;
 	(void) state;
 
 	config_defaults();
 	set.log_datetime_format    = GD_MAX + 10;
 	set.log_datetime_separator = GDC_MAX + 10;
 
+	set_date_format();
 	fmt = get_date_format();
 
 	assert_non_null(fmt);
 	assert_int_equal(set.log_datetime_format, GD_DEFAULT);
 	assert_int_equal(set.log_datetime_separator, GDC_DEFAULT);
-	free(fmt);
 }
 
 static void test_get_date_format_covers_each_supported_format(void **state) {
@@ -430,10 +433,10 @@ static void test_get_date_format_covers_each_supported_format(void **state) {
 			set.log_datetime_format    = fmt_value;
 			set.log_datetime_separator = sep_value;
 
+			set_date_format();
 			fmt = get_date_format();
 			assert_non_null(fmt);
 			assert_true(strlen(fmt) > 0);
-			free(fmt);
 		}
 	}
 }
@@ -492,8 +495,8 @@ int main(void) {
 		cmocka_unit_test(test_config_defaults_populates_the_set),
 		cmocka_unit_test(test_read_spine_config_rejects_a_missing_file),
 		cmocka_unit_test(test_read_spine_config_reads_settings),
-		cmocka_unit_test(test_get_date_format_returns_owned_memory),
-		cmocka_unit_test(test_get_date_format_clamps_an_out_of_range_format),
+		cmocka_unit_test(test_get_date_format_returns_cached_storage),
+		cmocka_unit_test(test_set_date_format_clamps_an_out_of_range_format),
 		cmocka_unit_test(test_get_date_format_covers_each_supported_format),
 		cmocka_unit_test(test_is_debug_device_matches_only_listed_ids),
 	};