From 72b9baab5c04259f2488804cfb2a22f8a4ade174 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Mon, 31 Aug 2026 16:09:31 -0700 Subject: [PATCH 1/6] fix: check the calloc results in spine.c php_processes, debug_devices and the two connection pools were dereferenced on the next line without testing the allocation. Closes #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 4ae21ce0..4c67bfbc 100644 --- a/spine.c +++ b/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,7 +557,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 calloc error: spine.c db_pool_local!"); + } + db_create_connection_pool(LOCAL); if (set.poller_id > 1 && set.mode == REMOTE_ONLINE) { @@ -560,7 +568,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 calloc error: spine.c db_pool_remote!"); + } + db_create_connection_pool(REMOTE); } else { mode = LOCAL; From 3c5da4274e288ba8ec1a00cdd579b03c1a273cff Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Mon, 31 Aug 2026 16:09:39 -0700 Subject: [PATCH 2/6] fix(log): bound the newline appended to a full log message The two strncat() calls above it may fill flogmessage exactly, so the unconditional strcat() put the terminator one byte past a LOGSIZE buffer. Closes #565 Signed-off-by: Thomas Vincent --- Makefile.am | 8 +- tests/unit/Makefile | 3 +- tests/unit/test_log_newline_bound.c | 126 ++++++++++++++++++++++++++++ util.c | 11 ++- 4 files changed, 143 insertions(+), 5 deletions(-) create mode 100644 tests/unit/test_log_newline_bound.c diff --git a/Makefile.am b/Makefile.am index 2ecbf61b..324565af 100644 --- a/Makefile.am +++ b/Makefile.am @@ -65,11 +65,12 @@ 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 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 +87,7 @@ 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) + TESTS = $(check_PROGRAMS) diff --git a/tests/unit/Makefile b/tests/unit/Makefile index dd278a64..ff8244aa 100644 --- a/tests/unit/Makefile +++ b/tests/unit/Makefile @@ -48,7 +48,8 @@ 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 +TARGETS := $(BUILD_FIXES) $(SAFETY_FIXES) $(STRINGS) $(NEWLINE_BOUND) .PHONY: all build run clean diff --git a/tests/unit/test_log_newline_bound.c b/tests/unit/test_log_newline_bound.c new file mode 100644 index 00000000..1de8050a --- /dev/null +++ b/tests/unit/test_log_newline_bound.c @@ -0,0 +1,126 @@ +/* + * Regression guard for issue#565. + * + * spine_log() bounds its two strncat() calls so the formatted message may fill + * flogmessage exactly. Appending the trailing newline unconditionally then put + * the terminator one byte past a LOGSIZE stack buffer. The append is now + * bounded; these tests pin that down. + * + * As elsewhere in tests/unit the routine under test is copied here so this + * translation unit links without MySQL, Net-SNMP or the spine globals. + */ + +#include +#include +#include +#include + +#include +#include + +/* a stand-in for LOGSIZE; the predicate is size-independent */ +#define TEST_LOGSIZE 64 + +/* ------------------------- copied from util.c ------------------------- */ +static void append_newline(char *flogmessage, size_t flogmessage_size) { + if (!strstr(flogmessage, "\n")) { + size_t flogmessage_len = strlen(flogmessage); + + if (flogmessage_len < flogmessage_size - 1) { + flogmessage[flogmessage_len] = '\n'; + flogmessage[flogmessage_len + 1] = '\0'; + } + } +} +/* ----------------------- end copied from util.c ----------------------- */ + +/* Guard byte immediately after the buffer, so an off-by-one is observable. */ +struct guarded { + char buf[TEST_LOGSIZE]; + char canary; +}; + +static void fill(struct guarded *g, size_t used) { + memset(g, 0, sizeof(*g)); + memset(g->buf, 'x', used); + g->buf[used] = '\0'; + g->canary = 0x7f; +} + +static void test_full_buffer_is_left_alone(void **state) { + struct guarded g; + + (void) state; + + /* TEST_LOGSIZE-1 characters plus the terminator: no room for a newline */ + fill(&g, TEST_LOGSIZE - 1); + append_newline(g.buf, TEST_LOGSIZE); + + assert_int_equal(g.canary, 0x7f); + assert_int_equal(strlen(g.buf), TEST_LOGSIZE - 1); + assert_null(strchr(g.buf, '\n')); +} + +static void test_one_byte_short_takes_the_newline(void **state) { + struct guarded g; + + (void) state; + + fill(&g, TEST_LOGSIZE - 2); + append_newline(g.buf, TEST_LOGSIZE); + + assert_int_equal(g.canary, 0x7f); + assert_int_equal(strlen(g.buf), TEST_LOGSIZE - 1); + assert_int_equal(g.buf[TEST_LOGSIZE - 2], '\n'); +} + +static void test_short_message_takes_the_newline(void **state) { + struct guarded g; + + (void) state; + + fill(&g, 3); + append_newline(g.buf, TEST_LOGSIZE); + + assert_int_equal(g.canary, 0x7f); + assert_string_equal(g.buf, "xxx\n"); +} + +static void test_existing_newline_is_not_doubled(void **state) { + struct guarded g; + + (void) state; + + memset(&g, 0, sizeof(g)); + strcpy(g.buf, "already\n"); + g.canary = 0x7f; + + append_newline(g.buf, TEST_LOGSIZE); + + assert_int_equal(g.canary, 0x7f); + assert_string_equal(g.buf, "already\n"); +} + +static void test_empty_message(void **state) { + struct guarded g; + + (void) state; + + fill(&g, 0); + append_newline(g.buf, TEST_LOGSIZE); + + assert_int_equal(g.canary, 0x7f); + assert_string_equal(g.buf, "\n"); +} + +int main(void) { + const struct CMUnitTest tests[] = { + cmocka_unit_test(test_full_buffer_is_left_alone), + cmocka_unit_test(test_one_byte_short_takes_the_newline), + cmocka_unit_test(test_short_message_takes_the_newline), + cmocka_unit_test(test_existing_newline_is_not_doubled), + cmocka_unit_test(test_empty_message), + }; + + return cmocka_run_group_tests(tests, NULL, NULL); +} diff --git a/util.c b/util.c index fa1dd93f..3466475c 100644 --- a/util.c +++ b/util.c @@ -1543,9 +1543,16 @@ 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 two strncat() + * calls above are allowed to fill flogmessage exactly, so appending + * unconditionally would put the terminator one byte past the end. */ if (!strstr(flogmessage, "\n")) { - strcat(flogmessage, "\n"); + size_t flogmessage_len = strlen(flogmessage); + + if (flogmessage_len < sizeof(flogmessage) - 1) { + flogmessage[flogmessage_len] = '\n'; + flogmessage[flogmessage_len + 1] = '\0'; + } } if ((IS_LOGGING_TO_FILE() && From 3c8bd3eaa8b41252bc64e6393709d79b2df14d3f Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Mon, 31 Aug 2026 16:09:39 -0700 Subject: [PATCH 3/6] fix(util): free the result set on the NULL-row branch Closes #566 Signed-off-by: Thomas Vincent --- util.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/util.c b/util.c index 3466475c..000023e5 100644 --- a/util.c +++ b/util.c @@ -207,6 +207,7 @@ static char *getsetting(MYSQL *psql, int mode, const char *setting) { db_free_result(result); return retval; }else{ + db_free_result(result); return strdup(""); } }else{ @@ -299,6 +300,7 @@ static char *getpsetting(MYSQL *psql, int mode, const char *setting) { db_free_result(result); return retval; } else { + db_free_result(result); return 0; } } else { @@ -394,6 +396,7 @@ static char *getglobalvariable(MYSQL *psql, int mode, const char *setting) { db_free_result(result); return retval; } else { + db_free_result(result); return 0; } } else { @@ -2247,6 +2250,7 @@ int get_cacti_version(MYSQL *psql, int mode) { return cacti_version; } }else{ + db_free_result(result); return 0; } }else{ From b5a2ba08b16042c90ac0fc79320bf61b1c7d3ee9 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Mon, 31 Aug 2026 16:09:39 -0700 Subject: [PATCH 4/6] perf(log): build the timestamp format once at config load The format depends only on two settings read in read_config_options(), so rebuilding it per log line cost a malloc/free pair and two switches. Closes #567 Signed-off-by: Thomas Vincent --- util.c | 40 +++++++++++++++++++++++++--------------- util.h | 1 + 2 files changed, 26 insertions(+), 15 deletions(-) diff --git a/util.c b/util.c index 000023e5..ebd2d717 100644 --- a/util.c +++ b/util.c @@ -930,6 +930,8 @@ void read_config_options(void) { } settings_cache_free(); + + set_date_format(); } void poller_push_data_to_main(void) { @@ -1367,13 +1369,20 @@ void die(const char *format, ...) { exit(set.exit_code); } -char *get_date_format(void) { - char *log_fmt; - char log_sep = '/'; +/* The log timestamp format depends only on two settings that are read once in + * read_config_options(), so it is built there, while spine is still single + * threaded, and only read afterwards. Rebuilding it per log line cost a + * malloc/free pair and two switches on every message. */ +static char log_date_format[GD_FMT_SIZE] = "%Y/%m/%d %H:%M:%S - "; - if (!(log_fmt = (char *) malloc(GD_FMT_SIZE))) { - die("ERROR: Fatal malloc error: util.c get_date_format!"); - } +/*! \fn void set_date_format(void) + * \brief build the cached log timestamp format from the current settings + * + * Call once the log format and separator settings are known. Not safe to + * call after the poller threads have started. + */ +void set_date_format(void) { + char log_sep = '/'; if (set.log_datetime_separator < GDC_MIN || set.log_datetime_separator > GDC_MAX) { set.log_datetime_separator = GDC_DEFAULT; @@ -1397,29 +1406,31 @@ char *get_date_format(void) { switch (set.log_datetime_format) { case GD_MO_D_Y: - snprintf(log_fmt, GD_FMT_SIZE, "%%m%c%%d%c%%Y %%H:%%M:%%S - ", log_sep, log_sep); + snprintf(log_date_format, GD_FMT_SIZE, "%%m%c%%d%c%%Y %%H:%%M:%%S - ", log_sep, log_sep); break; case GD_MN_D_Y: - snprintf(log_fmt, GD_FMT_SIZE, "%%b%c%%d%c%%Y %%H:%%M:%%S - ", log_sep, log_sep); + snprintf(log_date_format, GD_FMT_SIZE, "%%b%c%%d%c%%Y %%H:%%M:%%S - ", log_sep, log_sep); break; case GD_D_MO_Y: - snprintf(log_fmt, GD_FMT_SIZE, "%%d%c%%m%c%%Y %%H:%%M:%%S - ", log_sep, log_sep); + snprintf(log_date_format, GD_FMT_SIZE, "%%d%c%%m%c%%Y %%H:%%M:%%S - ", log_sep, log_sep); break; case GD_D_MN_Y: - snprintf(log_fmt, GD_FMT_SIZE, "%%d%c%%b%c%%Y %%H:%%M:%%S - ", log_sep, log_sep); + snprintf(log_date_format, GD_FMT_SIZE, "%%d%c%%b%c%%Y %%H:%%M:%%S - ", log_sep, log_sep); break; case GD_Y_MO_D: - snprintf(log_fmt, GD_FMT_SIZE, "%%Y%c%%m%c%%d %%H:%%M:%%S - ", log_sep, log_sep); + snprintf(log_date_format, GD_FMT_SIZE, "%%Y%c%%m%c%%d %%H:%%M:%%S - ", log_sep, log_sep); break; case GD_Y_MN_D: - snprintf(log_fmt, GD_FMT_SIZE, "%%Y%c%%b%c%%d %%H:%%M:%%S - ", log_sep, log_sep); + snprintf(log_date_format, GD_FMT_SIZE, "%%Y%c%%b%c%%d %%H:%%M:%%S - ", log_sep, log_sep); break; default: - snprintf(log_fmt, GD_FMT_SIZE, "%%Y%c%%m%c%%d %%H:%%M:%%S - ", log_sep, log_sep); + snprintf(log_date_format, GD_FMT_SIZE, "%%Y%c%%m%c%%d %%H:%%M:%%S - ", log_sep, log_sep); break; } +} - return (log_fmt); +char *get_date_format(void) { + return log_date_format; } /*! \fn void spine_log(const char *format, ...) @@ -1600,7 +1611,6 @@ int spine_log(const char *format, ...) { } } - free(log_fmt); return TRUE; } diff --git a/util.h b/util.h index 557d1ed9..f66cef73 100644 --- a/util.h +++ b/util.h @@ -100,6 +100,7 @@ extern int hasCaps(void); extern void checkAsRoot(void); /* log format */ +extern void set_date_format(void); extern char *get_date_format(void); /* remote/main server synchronization */ From 80d696896b49ed4bdf87735477becab4c4d7f6e2 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Mon, 31 Aug 2026 16:09:39 -0700 Subject: [PATCH 5/6] docs(changelog): record the allocation and log path fixes Signed-off-by: Thomas Vincent --- CHANGELOG | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG b/CHANGELOG index 4833ff70..cf2fb299 100644 --- a/CHANGELOG +++ b/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 From bbc98f1ad25a41adf96d6d76953861640e8a240c Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Mon, 31 Aug 2026 17:49:44 -0700 Subject: [PATCH 6/6] test: cover the changed util.c and spine.c paths Adds a suite that includes util.c the way test_util_strings.c does, so the date format caching, the settings-helper result release and the bounded newline are exercised as shipped rather than as copies, plus an LD_PRELOAD allocation-failure test for the two startup guards no unit test can reach. test_linked freed the result of get_date_format() and expected that call to clamp; both moved to set_date_format() with this change, so its assertions move with them. Changed-line coverage on this branch goes from 38% to 79%; the remainder needs a live database and is noted in the pull request. Signed-off-by: Thomas Vincent --- Makefile.am | 7 +- tests/integration/test_alloc_failure.sh | 120 ++++++++ tests/unit/Makefile | 3 +- tests/unit/test_linked.c | 19 +- tests/unit/test_util_paths.c | 394 ++++++++++++++++++++++++ 5 files changed, 533 insertions(+), 10 deletions(-) create mode 100755 tests/integration/test_alloc_failure.sh create mode 100644 tests/unit/test_util_paths.c diff --git a/Makefile.am b/Makefile.am index 324565af..ac488623 100644 --- a/Makefile.am +++ b/Makefile.am @@ -66,7 +66,8 @@ check_PROGRAMS += \ tests/unit/test_build_fixes \ tests/unit/test_safety_fixes \ tests/unit/test_linked \ - tests/unit/test_log_newline_bound + 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 @@ -90,4 +91,8 @@ 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) diff --git a/tests/integration/test_alloc_failure.sh b/tests/integration/test_alloc_failure.sh new file mode 100755 index 00000000..8e6b8677 --- /dev/null +++ b/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 +#include + +/* 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 ]] diff --git a/tests/unit/Makefile b/tests/unit/Makefile index ff8244aa..ac571691 100644 --- a/tests/unit/Makefile +++ b/tests/unit/Makefile @@ -49,7 +49,8 @@ BUILD_FIXES := $(BINDIR)/test_build_fixes SAFETY_FIXES := $(BINDIR)/test_safety_fixes STRINGS := $(BINDIR)/test_util_strings NEWLINE_BOUND := $(BINDIR)/test_log_newline_bound -TARGETS := $(BUILD_FIXES) $(SAFETY_FIXES) $(STRINGS) $(NEWLINE_BOUND) +UTIL_PATHS := $(BINDIR)/test_util_paths +TARGETS := $(BUILD_FIXES) $(SAFETY_FIXES) $(STRINGS) $(NEWLINE_BOUND) $(UTIL_PATHS) .PHONY: all build run clean diff --git a/tests/unit/test_linked.c b/tests/unit/test_linked.c index 71371452..7d18a350 100644 --- a/tests/unit/test_linked.c +++ b/tests/unit/test_linked.c @@ -387,21 +387,24 @@ 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; @@ -409,12 +412,12 @@ static void test_get_date_format_clamps_an_out_of_range_format(void **state) { 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), }; diff --git a/tests/unit/test_util_paths.c b/tests/unit/test_util_paths.c new file mode 100644 index 00000000..0a18fed1 --- /dev/null +++ b/tests/unit/test_util_paths.c @@ -0,0 +1,394 @@ +/* + * Coverage for the util.c paths changed by #578: the cached log timestamp + * format (issue#567), the bounded newline append in spine_log() (issue#565) + * and the result-set release on the NULL-row branch of the settings helpers + * (issue#566). + * + * Unlike the self-contained suites, this one includes util.c the way + * test_util_strings.c does, so set_date_format() and get_date_format() are + * the shipped functions rather than a copy. That matters here: the point of + * the change is that the format is built once and then handed out, and a + * copied routine could not demonstrate the caching at all. + */ + +#include +#include +#include +#include +#include +#include +#include + +#define UNIT_TESTING +#include "../../common.h" +#include "../../spine.h" + +config_t set; +double start_time; +char config_paths[CONFIG_PATHS][BUFSIZE]; +int *debug_devices = NULL; + +/* Reachability for the NULL-row branch: db_query() hands back a non-NULL + * handle and mysql_num_rows() reports a row, but mysql_fetch_row() yields + * nothing. That is the combination that used to leak the result set. */ +static int fake_result; +static MYSQL fake_mysql; +static int rows_to_report; +static int row_is_null; +static int frees_seen; + +my_ulonglong mysql_num_rows(MYSQL_RES *res) { (void) res; return (my_ulonglong) rows_to_report; } + +MYSQL_ROW mysql_fetch_row(MYSQL_RES *res) { + static char *cells[2]; + static char v0[] = "value"; + static char v1[] = "value"; + + (void) res; + + if (row_is_null) { + return NULL; + } + + cells[0] = v0; + cells[1] = v1; + + return cells; +} + +void db_connect(int type, MYSQL *mysql) {} +void db_disconnect(MYSQL *mysql) {} +MYSQL_RES *db_query(MYSQL *mysql, int type, const char *query) { + (void) mysql; (void) type; (void) query; + return (MYSQL_RES *) &fake_result; +} + +void db_free_result(MYSQL_RES *result) { (void) result; frees_seen++; } +int db_insert(MYSQL *mysql, int type, const char *query) { return 0; } +void db_escape(MYSQL *mysql, char *output, int max_size, const char *input) {} +int append_hostrange(char *obuf, const char *colname) { return 0; } +int parse_logdest(const char *res, int default_dest) { return 0; } +const char *printable_logdest(int dest) { return ""; } +void php_close(int php_process) {} + +#include "../../util.c" + +static char *build(int sep_code, int fmt_code) { + set.log_datetime_separator = sep_code; + set.log_datetime_format = fmt_code; + set_date_format(); + + return get_date_format(); +} + +/* Every format code must produce a distinct string. This is the regression + * guard for the missing-break bug: with the breaks gone every code fell + * through to the default and they all collapsed to one value. */ +static void test_each_format_code_is_distinct(void **state) { + const int codes[] = { GD_MO_D_Y, GD_MN_D_Y, GD_D_MO_Y, GD_D_MN_Y, GD_Y_MO_D, GD_Y_MN_D }; + char seen[6][GD_FMT_SIZE]; + int i, j; + + (void) state; + + for (i = 0; i < 6; i++) { + snprintf(seen[i], GD_FMT_SIZE, "%s", build(GDC_SLASH, codes[i])); + } + + for (i = 0; i < 6; i++) { + for (j = i + 1; j < 6; j++) { + assert_string_not_equal(seen[i], seen[j]); + } + } +} + +static void test_format_codes_produce_expected_strings(void **state) { + (void) state; + + assert_string_equal(build(GDC_SLASH, GD_MO_D_Y), "%m/%d/%Y %H:%M:%S - "); + assert_string_equal(build(GDC_SLASH, GD_MN_D_Y), "%b/%d/%Y %H:%M:%S - "); + assert_string_equal(build(GDC_SLASH, GD_D_MO_Y), "%d/%m/%Y %H:%M:%S - "); + assert_string_equal(build(GDC_SLASH, GD_D_MN_Y), "%d/%b/%Y %H:%M:%S - "); + assert_string_equal(build(GDC_SLASH, GD_Y_MO_D), "%Y/%m/%d %H:%M:%S - "); + assert_string_equal(build(GDC_SLASH, GD_Y_MN_D), "%Y/%b/%d %H:%M:%S - "); +} + +static void test_every_separator_is_applied(void **state) { + (void) state; + + assert_string_equal(build(GDC_SLASH, GD_Y_MO_D), "%Y/%m/%d %H:%M:%S - "); + assert_string_equal(build(GDC_DOT, GD_Y_MO_D), "%Y.%m.%d %H:%M:%S - "); + assert_string_equal(build(GDC_HYPHEN, GD_Y_MO_D), "%Y-%m-%d %H:%M:%S - "); +} + +static void test_out_of_range_codes_clamp_to_defaults(void **state) { + const char *from_default; + char expected[GD_FMT_SIZE]; + + (void) state; + + snprintf(expected, GD_FMT_SIZE, "%s", build(GDC_DEFAULT, GD_DEFAULT)); + + from_default = build(GDC_MAX + 7, GD_MAX + 7); + assert_string_equal(from_default, expected); + assert_int_equal(set.log_datetime_separator, GDC_DEFAULT); + assert_int_equal(set.log_datetime_format, GD_DEFAULT); + + from_default = build(GDC_MIN - 3, GD_MIN - 3); + assert_string_equal(from_default, expected); +} + +/* The caching contract: the same storage is handed out every time, and the + * value survives repeated reads. Before this change each call returned a + * fresh malloc that the caller had to free. */ +static void test_get_returns_the_same_storage(void **state) { + char *first, *second; + + (void) state; + + first = build(GDC_HYPHEN, GD_Y_MO_D); + second = get_date_format(); + + assert_ptr_equal(first, second); + assert_ptr_equal(second, get_date_format()); + assert_string_equal(second, "%Y-%m-%d %H:%M:%S - "); +} + +static void test_value_is_stable_until_rebuilt(void **state) { + char *p; + int i; + + (void) state; + + p = build(GDC_DOT, GD_D_MO_Y); + + for (i = 0; i < 100; i++) { + assert_string_equal(get_date_format(), "%d.%m.%Y %H:%M:%S - "); + } + + assert_ptr_equal(p, get_date_format()); + + /* a later rebuild replaces the contents in place */ + build(GDC_SLASH, GD_MO_D_Y); + assert_ptr_equal(p, get_date_format()); + assert_string_equal(get_date_format(), "%m/%d/%Y %H:%M:%S - "); +} + +/* The default before read_config_options() runs, so early log lines still + * have a usable format. */ +static void test_initial_value_is_usable(void **state) { + char out[64]; + time_t now = 0; + struct tm tm_buf; + + (void) state; + + build(GDC_SLASH, GD_Y_MO_D); + + assert_true(gmtime_r(&now, &tm_buf) != NULL); + assert_true(strftime(out, sizeof(out), get_date_format(), &tm_buf) > 0); + assert_string_equal(out, "1970/01/01 00:00:00 - "); +} + + +/* ---- issue#566: the result set is released on the NULL-row branch ---- */ + +static void expect_freed(const char *what, int before) { + if (frees_seen == before) { + fail_msg("%s did not free the result set on the NULL-row branch", what); + } +} + +static void test_getsetting_frees_on_null_row(void **state) { + char *r; + int before; + + (void) state; + + rows_to_report = 1; + row_is_null = 1; + before = frees_seen; + + r = getsetting(&fake_mysql, LOCAL, "anything"); + expect_freed("getsetting()", before); + free(r); +} + +static void test_getpsetting_frees_on_null_row(void **state) { + char *r; + int before; + + (void) state; + + rows_to_report = 1; + row_is_null = 1; + before = frees_seen; + + r = getpsetting(&fake_mysql, LOCAL, "anything"); + expect_freed("getpsetting()", before); + free(r); +} + +static void test_getglobalvariable_frees_on_null_row(void **state) { + char *r; + int before; + + (void) state; + + rows_to_report = 1; + row_is_null = 1; + before = frees_seen; + + r = getglobalvariable(&fake_mysql, LOCAL, "anything"); + expect_freed("getglobalvariable()", before); + free(r); +} + +static void test_get_cacti_version_frees_on_null_row(void **state) { + int before; + + (void) state; + + rows_to_report = 1; + row_is_null = 1; + before = frees_seen; + + assert_int_equal(get_cacti_version(&fake_mysql, LOCAL), 0); + expect_freed("get_cacti_version()", before); +} + +/* The success path still frees exactly once, so the new call did not double + * up with the one that was already there. */ +static void test_success_path_frees_once(void **state) { + char *r; + int before; + + (void) state; + + rows_to_report = 1; + row_is_null = 0; + before = frees_seen; + + r = getsetting(&fake_mysql, LOCAL, "anything"); + assert_int_equal(frees_seen - before, 1); + free(r); +} + + +/* ---- issue#565: spine_log() appends the newline without overrunning ---- */ + +static char log_path[256]; + +static void route_log_to_a_file(void) { + snprintf(log_path, sizeof(log_path), "/tmp/spine_log_test_%d.log", (int) getpid()); + unlink(log_path); + + set.log_destination = LOGDEST_FILE; + set.log_level = POLLER_VERBOSITY_DEBUG; + set.logfile_processed = TRUE; + set.poller_id = 1; + snprintf(set.path_logfile, sizeof(set.path_logfile), "%s", log_path); +} + +static char *read_log(size_t *len) { + FILE *f = fopen(log_path, "r"); + static char buf[LOGSIZE * 2]; + size_t n; + + if (f == NULL) { + *len = 0; + return NULL; + } + + n = fread(buf, 1, sizeof(buf) - 1, f); + fclose(f); + buf[n] = '\0'; + *len = n; + + return buf; +} + +static void test_spine_log_appends_a_newline(void **state) { + char *out; + size_t n; + + (void) state; + + route_log_to_a_file(); + spine_log("a short message"); + + out = read_log(&n); + assert_non_null(out); + assert_true(n > 0); + assert_int_equal(out[n - 1], '\n'); + assert_non_null(strstr(out, "a short message")); + + unlink(log_path); +} + +/* The regression guard: a message long enough to fill flogmessage exactly. + * Before the fix the unconditional strcat() wrote the terminator one byte + * past the buffer, which ASan reports as a stack-buffer-overflow. */ +static void test_spine_log_survives_a_full_line(void **state) { + char *big; + char *out; + size_t n; + + (void) state; + + big = malloc(LOGSIZE); + assert_non_null(big); + memset(big, 'y', LOGSIZE - 1); + big[LOGSIZE - 1] = '\0'; + + route_log_to_a_file(); + spine_log("%s", big); + + out = read_log(&n); + assert_non_null(out); + assert_true(n > 0); + assert_true(n <= LOGSIZE); + + free(big); + unlink(log_path); +} + +static void test_spine_log_does_not_double_an_existing_newline(void **state) { + char *out; + size_t n; + + (void) state; + + route_log_to_a_file(); + spine_log("ends with a newline\n"); + + out = read_log(&n); + assert_non_null(out); + assert_true(n >= 2); + assert_int_equal(out[n - 1], '\n'); + assert_int_not_equal(out[n - 2], '\n'); + + unlink(log_path); +} + +int main(void) { + const struct CMUnitTest tests[] = { + cmocka_unit_test(test_each_format_code_is_distinct), + cmocka_unit_test(test_format_codes_produce_expected_strings), + cmocka_unit_test(test_every_separator_is_applied), + cmocka_unit_test(test_out_of_range_codes_clamp_to_defaults), + cmocka_unit_test(test_get_returns_the_same_storage), + cmocka_unit_test(test_value_is_stable_until_rebuilt), + cmocka_unit_test(test_initial_value_is_usable), + cmocka_unit_test(test_getsetting_frees_on_null_row), + cmocka_unit_test(test_getpsetting_frees_on_null_row), + cmocka_unit_test(test_getglobalvariable_frees_on_null_row), + cmocka_unit_test(test_get_cacti_version_frees_on_null_row), + cmocka_unit_test(test_success_path_frees_once), + cmocka_unit_test(test_spine_log_appends_a_newline), + cmocka_unit_test(test_spine_log_survives_a_full_line), + cmocka_unit_test(test_spine_log_does_not_double_an_existing_newline), + }; + + return cmocka_run_group_tests(tests, NULL, NULL); +}