Open and own the required ICMP sockets before permanently dropping
setuid root, then parse input without elevated identity. Close-on-exec
socket ownership, shared IPv4/IPv6 dispatch and fault-injected
identity/capability calls are covered. The owned fixture tests check
deny/allow behavior without contacting external devices.
Step 2/14 of the complete C17/native-correctness series. Base: the
preceding series branch; merge/review in order so this PR shows only its
own step.
Earlier validation, before this review follow-up, in isolated copied
snapshots on Linux arm64:
- Clean GCC build and make check: 13 passed, 1 existing prerequisite
skips, 0 failures.
- ASan/UBSan: 12 passed, 2 existing privilege/allocator-interposition
prerequisite skips, 0 failures; no sanitizer findings.
- Production source/profile integrity verification against disposable
MariaDB and loopback SNMP fixtures passed.
- Release archive extracted into a separate source tree and built/tested
out of tree; fuzz checks passed.
Earlier local profiles retain source hashes for the copied validation
tree; GitHub checks validate the published commit. No production
database/device, host installation or live privilege change is part of
validation. Existing prerequisite skips were retained; no test or
finding was suppressed to obtain a pass.
Series navigation: [Series
index](https://github.com/Cacti/spine/pull/638); [previous
step](https://github.com/Cacti/spine/pull/638). [Next
step](https://github.com/Cacti/spine/pull/640).
Review follow-up (2026-10-09): Restacked on the corrected preceding
branches. The SNMP status fix is introduced in #638, the
reindex/completion fixes in #640, and focused regression coverage in
#650.
Restacked tip validation: isolated Linux arm64 build and make check
passed (18 passed, 1 setuid prerequisite skip, no failures).
GitHub checks validate the updated published commits; earlier
profile/archive/sanitizer evidence above belongs to the previous
revisions.
---------
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Introduce bounded native interfaces and grouped runtime state, then link
regression and fault contracts against the real poller implementations.
The contracts capture CLI/configuration input, availability
combinations, SNMP request/response ownership, script framing,
reindexing, output persistence, worker completion and shutdown. This
foundational change is larger because the shared interfaces and their
existing production contracts must move together; later changes have
separate review boundaries.
Step 1/14 of the complete C17/native-correctness series. Base: develop.
Earlier validation, before this review follow-up, in isolated copied
snapshots on Linux arm64:
- Clean GCC build and make check: 13 passed, 0 existing prerequisite
skips, 0 failures.
- ASan/UBSan: 12 passed, 1 existing privilege/allocator-interposition
prerequisite skips, 0 failures; no sanitizer findings.
- Production source/profile integrity verification against disposable
MariaDB and loopback SNMP fixtures passed.
- Release archive extracted into a separate source tree and built/tested
out of tree; fuzz checks passed.
Earlier local profiles retain source hashes for the copied validation
tree; GitHub checks validate the published commit. No production
database/device, host installation or live privilege change is part of
validation. Existing prerequisite skips were retained; no test or
finding was suppressed to obtain a pass.
The worker contracts publish owned heap state and release it only after
joining workers and restoring the runtime table. Coverage counters use
atomic updates; cppcheck receives an explicit model of the fatal exit
contract. Fixture images use public mirrors to avoid shared Docker Hub
pull limits.
Series navigation: review and merge in order. Every PR targets the
preceding branch so its diff contains one step.
1. [refactor(core): add bounded native interfaces and polling
contracts](https://github.com/Cacti/spine/pull/638)
2. [fix(privileges): drop root before input and retain owned ICMP
sockets](https://github.com/Cacti/spine/pull/639)
3. [fix(poller): isolate worker failures and bound database
reconnects](https://github.com/Cacti/spine/pull/640)
4. [fix(results): normalize output and unify SNMP batch
persistence](https://github.com/Cacti/spine/pull/641)
5. [fix(runtime): bound probe retries and validate CLI and
configuration](https://github.com/Cacti/spine/pull/642)
6. [build: select GNU C17 explicitly and probe hardening
support](https://github.com/Cacti/spine/pull/643)
7. [fix(threads): size worker stacks and guard size
arithmetic](https://github.com/Cacti/spine/pull/644)
8. [refactor(c17): simplify ownership guards and share test
support](https://github.com/Cacti/spine/pull/645)
9. [refactor(core): extract configuration polling and runtime
modules](https://github.com/Cacti/spine/pull/646)
10. [fix(boundaries): validate script slots and make cleanup
portable](https://github.com/Cacti/spine/pull/647)
11. [refactor(layout): organize responsibilities and separate main from
runtime](https://github.com/Cacti/spine/pull/648)
12. [docs(license): adopt SPDX notices and retain contributor
credits](https://github.com/Cacti/spine/pull/649)
13. [test(runtime): measure all suites and cover protocol
boundaries](https://github.com/Cacti/spine/pull/650)
14. [build(ci): validate connectors time width and native C17
modules](https://github.com/Cacti/spine/pull/651)
Review follow-up (2026-10-09): Publish the decoded GET/GETNEXT status
back to the host so availability checks observe exception and
missing-response failures, while preserving the legacy transport-success
behavior for agent PDU errors.
Updated step 1 snapshot: isolated Linux arm64 build and make check
passed (13 passed, no skips or failures).
The restacked tip passes all five clang-tidy gate regression tests and
real clang-tidy analysis (26 reviewed baseline findings, no new
findings). Actionlint and matching DCO trailers pass. The setuid test
also passes when run in the separate disposable root container.
GitHub checks validate the updated published commits; earlier
profile/archive/sanitizer evidence above belongs to the previous
revisions.
---------
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
## Summary
A batch of small, independent, unaddressed bugs from the open-issue
backlog. None of these overlapped with any currently open PR's actual
diff (verified by inspecting each candidate PR's changes directly, not
just its declared `Closes` list).
## Fixes
- **`poll_host()` skips `mysql_thread_end()` when the device row is
missing** — the early `return` on `num_rows != 1` released both DB
connections and freed six allocations, but never called
`mysql_thread_end()`, unlike the other two exit paths. Added the missing
call.
- **SNMPv3 passphrase scrubbing flag never set** — `zero_sensitive` was
a function-local initialized to 0 and never assigned, so every guarded
`memset()` was dead code. Made scrubbing of this module's own heap
copies (`Apsz`/`Xpsz`) unconditional, and moved the wipe to immediately
precede each free (including both `generate_Ku()` error paths) after
review found the first pass still scrubbed before assignment. Left the
caller's `snmp_password`/`snmp_priv_passphrase` alone rather than also
scrubbing those in place — those are `host_t`-owned and get reused for
later session rebuilds against the same host, so zeroing them would have
broken subsequent polls.
- **`php_readpipe()` FD_SET without an FD_SETSIZE check** — `FD_SET()`
doesn't range-check, so a read descriptor at or above `FD_SETSIZE` (1024
on glibc) wrote past the end of the on-stack `fd_set`. Added a bounds
check; on an out-of-range descriptor it marks the result undefined and
closes the slot (a review finding caught the first version recursing
without bound through `php_init()` → `php_readpipe()` under sustained
descriptor exhaustion, so it no longer restarts synchronously from
here).
- **Signal handler never re-arms, so the second SIGPIPE kills the
poller** — every fatal signal reset itself to `SIG_DFL` and never
re-armed. That's the right idiom for a real fault (SIGSEGV and friends
re-execute and die with a core), but SIGPIPE is a routine, recurring
condition — a second one previously killed the whole poller. A review
finding pointed out that re-arming only at the end still left a window
(signal dispositions are process-wide) where another thread's SIGPIPE
could hit `SIG_DFL`; fixed by never resetting SIGPIPE's disposition in
the first place.
- **spine fails to link on Fedora: undefined reference to
`usmDESPrivProtocol`** — some distributions ship a `net-snmp-config.h`
that advertises DES as the default privacy protocol while the shipped
`libnetsnmp` doesn't export the symbol. Added an `AC_LINK_IFELSE` probe
for the actual symbol and guarded the fallback to use
`usmAESPrivProtocol` when it's absent.
- **Poller results ≥1024 bytes silently truncated to 1022** —
`db_escape()` capped its internal trim buffer at a fixed `DBL_BUFSIZE`
regardless of the caller's `max_size`, so raising
`--with-results-buffer` never helped. The trim buffer now scales with
`max_size` (a review finding caught the trim-size formula going
negative/zero for `max_size` 1-3), and `escaped_result` in `poll_host()`
is sized to `RESULTS_BUFFER * 2 + 1` instead of the fixed `DBL_BUFSIZE`.
(A very large escaped result can still hit the separate, already-tracked
row-size limit in #598/#618 — this fix only addresses the escaping-stage
truncation.)
## Also included
- **Seed `spine.conf` from `spine.conf.dist` on first install** — `make
install` only ever installed `spine.conf.dist`; nothing produced a live
`spine.conf`. Added an `install-data-hook` that copies it over only when
`spine.conf` doesn't already exist, plus a matching `uninstall-hook`
that only removes it again when it's still byte-identical to the `.dist`
(so a real uninstall never discards a customized config, while `make
distcheck`'s install/uninstall symmetry check still passes).
## Dropped from this PR
- A custom `codeql.yml` workflow (originally added for #580) was removed
— the repo already has GitHub's default CodeQL setup enabled and running
(`gh api repos/Cacti/spine/code-scanning/default-setup` → `state:
configured`), and a custom "advanced configuration" workflow cannot
coexist with it; GitHub rejected the SARIF upload and failed CI. #580's
underlying gap was already resolved out of band, so this PR no longer
claims to close it.
## Testing
Full build via WSL (`./bootstrap && ./configure --enable-warnings &&
make`) succeeds with no new warnings in `poller.c`, `snmp.c`, `php.c`,
`error.c`, or `sql.c` beyond two pre-existing, unrelated
`-Wformat-truncation` warnings in `php_init()`. Confirmed the
`usmDESPrivProtocol` configure probe correctly detects the missing
symbol in this build environment and takes the AES fallback path without
a link error. `make distcheck` passes end to end.
Closes#594Closes#600Closes#605Closes#607Closes#575Closes#583
## Summary
Fix five security issues found during code review:
1. **SQL injection via unescaped SNMP result in INSERT**
(`poller.c:1844`): `poller_items[i].result` and `rrd_name` flow directly
into SQL VALUES clause. Added `db_escape()` calls for both values before
interpolation.
2. **Buffer overflow in `buffer_output_errors`** (`poller.c:2047`):
`snprintf` used `DBL_BUFSIZE` as the size limit instead of the remaining
buffer space `DBL_BUFSIZE - *buf_size`, allowing a write past the end of
`error_string`.
3. **SQL injection via `--hostlist` CLI argument** (`spine.c:388`): The
`host_id_list` string is interpolated into SQL queries without
validation. Added input validation to reject any characters other than
digits, commas, and spaces.
4. **Undocumented command injection trust boundary** (`nft_popen.c:126`,
`poller.c:2245`): `nft_popen()` passes commands to `/bin/sh -c` without
shell escaping. Added warning comments documenting that callers must
ensure commands originate from the Cacti database.
5. **Unbounded `sprintf` calls in `util.c`** (lines 99, 141, 145, 191,
286, 2036): Replaced all 6 `sprintf(qstring, ...)` calls with
`snprintf(qstring, sizeof(qstring), ...)` to prevent buffer overflows.
## Files changed
- `poller.c` - escape SQL values (fix 1), bound snprintf (fix 2),
document trust boundary (fix 4)
- `spine.c` - validate --hostlist input (fix 3)
- `nft_popen.c` - document trust boundary (fix 4)
- `util.c` - replace sprintf with snprintf (fix 5)
## Test plan
- [x] `make clean && make` compiles without errors or warnings
- [ ] Run spine against a test Cacti instance with SNMP results
containing single quotes
- [ ] Run spine with `--hostlist "1; DROP TABLE"` and verify it exits
with error
- [ ] Verify poller output results are correctly escaped in the database
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Fixes compiler warnings across spine source files: adds missing void in
function prototypes, marks internal functions static, removes unused
variables, and fixes const-correctness on function signatures.
**Note:** This PR also includes the following non-mechanical changes
that should be reviewed carefully:
- sql.c: db_column_exists now passes through the caller's type argument
instead of hardcoding LOCAL (bug fix, changes runtime behavior for
REMOTE callers)
- poller.c: Removed write-only snmp_poller_items counter and its
increment block (dead-code elimination)
- util.c: getglobalvariable now returns a strdup'd copy instead of a
pointer into opttable (fixes latent use-after-free on free())
- ping.c: Fixed strncopy(stack, hostname, strlen(stack)) to
strlen(hostname) (old code was a no-op after memset)
- util.c: Changed getsetting/getpsetting/getglobalvariable return type
from const char * to char * to reflect caller ownership semantics
### Addressed performance and DRY issues:
- Addresses #501 (Exponential Backoff)
- Addresses #503 (Centralized Logging)
- Addresses #504 (SNMP_FREE Macro)
- Addresses #505 (O(1) Hash Table)
### Test plan
- [ ] Verify spine compiles with both gcc and clang
- [ ] Run spine against a test Cacti instance and verify polling works
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Twelve bugs found by running cppcheck 2.20
(--enable=warning,portability) and shellcheck v0.10.0.1 across the full
source tree.
**poller.c**
- `%llu` used for `atoll()` return (signed `long long`) at line 970 —
changed to `%lld`
- `%lld` used for `hex2dec()` return (unsigned `long long`) at five call
sites (lines 1414, 1515, 1597, 1655, 1727) — changed to `%llu`
Mismatched sign in printf format specifiers is undefined behaviour under
C99 §7.19.6.1.
**snmp.c**
- `session.localname` is `strdup()`'d at line 140 (when `SNMP_LOCALNAME
== 1`) but the early-return path for an unknown SNMP version at line 190
did not free it. Added `free(session.localname)` before that return.
`free(NULL)` is a no-op so the call is safe when `SNMP_LOCALNAME != 1`.
- `calloc(num_oids, sizeof(*name))` at line 999 has no NULL check. If
allocation fails, the immediately following `namep->name_len =
MAX_OID_LEN` dereferences a null pointer. Added a NULL guard that logs
via `SPINE_LOG` and returns early before the PDU is created.
**sql.c**
- `attempts` is `int` but printed with `%u` — changed to `%d`.
- `error` is `int` but printed with `%u` — changed to `%d`.
- `mysql = NULL` in `db_disconnect()` assigns to a by-value parameter;
the write has no effect on the caller. Removed the dead assignment.
**util.c**
- `pthread_self()` is cast to `unsigned long int` then printed with
`%ld` (signed) — changed to `%lu`.
**copyright_year.sh / scripts/verify.sh**
- `$1` unquoted in `sed` invocation in `copyright_year.sh` — word-splits
on filenames with spaces.
- `*.c *.h` glob in `scripts/verify.sh` passed without `--` separator —
filenames starting with `-` would be misinterpreted as cppcheck options.
---------
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Co-authored-by: TheWitness <thewitness@cacti.net>
Fixes#416
Five minor safety fixes:
1. poller.c: `exec_poll()` return value leaked after `char_count()`
2. poller.c: `php_cmd()` return value leaked + `sprintf` to `snprintf`
3. sql.c: `mysql_init()` return value captured to handle reallocation
4. spine.c: `debug_devices` tokenizer bounded to array size
5. spine.c: `sem_trywait` checks `errno` instead of return code for
EINTR
All declarations at block top per project convention. Tested with gcc
and clang.
---------
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Fixes#411
MYSQL_OPT_RECONNECT was deprecated in MySQL 8.0.34 and flagged for
removal in MariaDB. Without the option, `mysql_ping()` detects a dead
connection but does not auto-reconnect, causing `db_reconnect()` to
always fail.
Changes:
- Add `int type` parameter to `db_reconnect()` so it knows LOCAL vs
REMOTE credentials
- When `mysql_ping()` does not reconnect (thread ID unchanged), call
`mysql_close()` + `db_connect(type, mysql)` explicitly
- Remove `MYSQL_OPT_RECONNECT` option call and `reconnect` variable from
`db_connect()`
- Update callers in `db_insert()` and `db_query()` to pass `type`
through
Only sql.c and sql.h modified. Tested with gcc and clang on macOS (MySQL
8.x client).
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Co-authored-by: TheWitness <thewitness@cacti.net>
Fixes#414
Four bugs found during code review of poller.c, sql.c, and spine.c:
1. poller.c: double-free in `poll_host()` when `host_thread ==
last_host_thread == 1`
2. sql.c: `db_get_connection()` off-by-one returns wrong pool connection
3. spine.c: `die()` uses `vsprintf` into 256-byte buffer, switched to
`vsnprintf`
4. poller.c: result set leak on early return in host-range filtering
All declarations at block top per project convention. Tested with gcc
and clang.
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
net-snmp 5.9.5 injects `-Werror=declaration-after-statement` via
`net-snmp-config --cflags`, breaking builds on FreeBSD and other
systems where net-snmp is compiled with strict C89 flags.
Moves all variable declarations to the top of their enclosing
block in `spine.c` (main) and `util.c` (read_config_options,
spine_log, strncopy, checkAsRoot, regex_replace).
Verified: clean compile with `-Werror=declaration-after-statement`
across sql.c, spine.c, and util.c.
Supersedes #407 (which stripped the flag instead of fixing the root
cause).
Resolves#406
---------
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
While compiling sql.c clang 16 generates the following warning:
```
sql.c:407:27: warning: comparison of address of 'db_pool_local[id].mysql' not equal to a null pointer is always true [-Wtautological-pointer-compare]
if (&db_pool_local[id].mysql != NULL) {
~~~~~~~~~~~~~~~~~~^~~~~ ~~~~
sql.c:428:28: warning: comparison of address of 'db_pool_remote[id].mysql' not equal to a null pointer is always true [-Wtautological-pointer-compare]
if (&db_pool_remote[id].mysql != NULL) {
~~~~~~~~~~~~~~~~~~~^~~~~ ~~~~
```
If the conditional is always true then the fix is to remove the
conditional in this instance
Co-authored-by: Erik Sejr <eriks@ssicanada.com>
-issue#202: MySQL SSL Key capability check does not work
-issue#203: Backtracing support checks do not work
-issue#204: MySQL Retry Count capability check always fails
Closes#202Closes#203Closes#204
On my gravestone, I want a quote something to the affect of "Here lies a man who hated the C, but loved the world around it". Great language, fast, but too many rules for this old critter.
There are really four fixes here.
- This first is that the db_escape() function was basically not even working breaking the whole '--mibs' thing.
- When limiting the host list, you should not send everything on the poller back to the main server, only what was polled. Minor, but a finer point.
- The signal handler needs to not mix it up by calling die(). Just fprintf() to standard error, backtrace if you segfaulted and go away. I've found in a few cases that the spine will be left running, and when straced, it's blocked on I/O from trying to do too much while trying to 'die'.
- Lastly, when pushing log data to standard output for debugging, don't trim output
- Spine does not properly use the mysql reconnect option
- Spine segfaults when performing certain re-index operation
- This change also corrects some compiler warnings.
* Correct segfault due to use of wrong variable
* Remove some whitespace
* More declarations to headers
* Add 'Device[]' to some log messages
* PID[] instead of Process[]
* Some block formatting
* Remove an automake warning
* Use stdbool.h
Thomas Vincent