The runtime/probe changes from #642 and GNU C17/build changes from #643
were squash-merged into #641's source branch, which made the
result-normalization review include later stages. This follow-up
preserves those approved changes while restoring #641's focused diff
against develop. No force-push or approved-code deletion is required.
Merge after #641 and before #644. #644 will target this branch. This
combines the already reviewed steps 5 and 6; it introduces no new
worker-stack or module-extraction changes.
The UDP follow-up also corrects a validated finding: EHOSTUNREACH on a
connected socket is ambiguous and can represent an unreachable host or
failed neighbour lookup. It now reports down; ECONNREFUSED/ECONNRESET
and actual peer datagrams retain their existing alive behavior. The
production-linked test rejects the original EHOSTUNREACH behavior.
Validation: Ubuntu 24.04/aarch64, clean GCC build and make check: 17
passed, one existing setuid prerequisite skip, zero failures. The full
tracked source tree is byte-identical to the independently validated
#641 functional-fix snapshot. The UDP and poll-host regressions fail
against the original code and pass after the fixes. The isolated step-4
tree independently passes 15 tests with one existing skip. No live
endpoints, host installation, privilege changes, or test suppression.
Repository squash merges still require restacking/retargeting the next
child after each merge; clean ordinary merge simulations are
insufficient by themselves.
---------
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Classify numeric, undefined, multipart and Cacti octet results through
one parser, normalize undefined agent values before persistence and
flush every SNMP batch consistently. Validate whole non-negative
uptimes, retain unknown reindex baselines and free script results
through their owning pointer. The mocked poll_host suite checks
session-profile boundaries, spike handling, output_regex input and
completion writes.
Originally step 4/14 of the C17/native-correctness series; its preceding
steps have merged. The current base is develop.
Earlier validation, before this review follow-up, in isolated copied
snapshots on Linux arm64:
- Clean GCC build and make check: 15 passed, 1 existing prerequisite
skips, 0 failures.
- ASan/UBSan: 14 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.
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.
## Current merge dependencies
Current base: `develop`. Immediate dependency: `develop` (earlier steps
already merged). Next open PR:
[#644](https://github.com/Cacti/spine/pull/644).
Merge order: #641 → #644 → #654 → #646 → #648 → #649 → #650 → #651 →
#655 → #658 → #659.
This repository uses squash-only merges and deletes merged branches.
Record the old parent SHA, squash one PR at a time, then restack and
retarget dependents before merging the next PR. Re-run merge validation
and checks on the resulting heads; an ordinary clean merge does not
replace squash/restack validation.
---------
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
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
Consolidates the connector/build work from #613 and the broader
test-infrastructure work extracted from #597 into one review:
- discover MariaDB Connector/C first while retaining MySQL and
Percona-compatible support
- test MariaDB and MySQL Connector/C with strict GCC and Clang builds on
Ubuntu 24.04
- add a real macOS 15 build and unit-test lane with portable Autotools
platform detection
- run unit tests under ASan/UBSan and ThreadSanitizer
- enforce the measured 7% global coverage baseline and 70% changed-line
coverage
- add libFuzzer coverage for output parsing alongside the existing
hostname parser
- exercise the SNMPv3 integration suite against MariaDB and MySQL 8.4
- add cppcheck, shellcheck, DCO verification, CODEOWNERS, distcheck, and
Linux-capability checks while retaining the repository-managed default
CodeQL setup
- expose a stable `CI / required` aggregate check for branch protection
- pin runners and third-party actions and keep workflow permissions
read-only by default
## Correctness fixes found by the new gates
- reject truncated capability strings and poller-output SQL tuples
instead of publishing partial data
- make `bootstrap` fail closed and select `libtoolize`/`glibtoolize`
portably
- use Autoconf's canonical host for Darwin detection instead of an empty
native-build alias
- remove stale initialization/name-shadow findings and make read-only
interfaces const-correct
- make the ICMP parser test portable across Linux and BSD header names
## Verification
- strict GCC and Clang builds: 4/4 unit targets passed
- ASan/UBSan: 4/4 passed
- ThreadSanitizer: 4/4 passed
- macOS Clang/Homebrew build: 4/4 passed
- MariaDB integration: passed
- MySQL 8.4 integration: passed
- both libFuzzer targets: passed sustained local runs
- coverage: 7.2% global and 84% on changed lines
- cppcheck, shellcheck, actionlint, DCO verification, and `git diff
--check`: passed locally
## Post-merge repository settings
After these workflows exist on `develop`, an administrator should
require `CI / required`, Fuzz, Integration, Rocky Linux, and the
repository-managed CodeQL/code-scanning result in the active ruleset;
enable code-owner approval; and require the branch to be current (or use
a merge queue). Enabling those settings before this PR merges would
create required checks that the base branch cannot yet emit.
Consolidates and supersedes #613. Closes#612.
---------
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: TheWitness <thewitness@cacti.net>
Signed-off-by: TheWitness <copilot@users.noreply.github.com>
Co-authored-by: TheWitness <thewitness@cacti.net>
Co-authored-by: TheWitness <copilot@users.noreply.github.com>
Extracted from #597 into a focused core correctness and memory-safety
change.
This checks startup allocations, bounds full log lines and query
construction, frees settings result sets on NULL-row paths, caches the
configured timestamp format, replaces advancing snprintf cursors with a
bounded append contract, fixes remote batch boundary loss, and converts
delimited hexadecimal OctetStrings without overflow. poller_output and
remote status pushes use the cross-vendor VALUES() form until #590
supplies an independently detected main-server capability.
Behavior worth calling out:
- colon/dash-delimited hexadecimal values that were previously stored as
zero now convert correctly
- values wider than 64 bits become U and count as collection errors
instead of producing undefined numeric conversion
- remote batches flush before the boundary-triggering row and refuse
truncated statements
Verification on macOS arm64:
- warning-enabled C99 configure/build
- forced make -B check on the final tree: 6 pass, 1 expected skip
- the skipped allocation-failure integration uses Linux LD_PRELOAD and
runs in Linux CI
- full VPATH make distcheck passed earlier in this branch
- mandatory independent pre-push review: PASS
Closes#564Closes#565Closes#566Closes#567Closes#588
Refs #590
Refs #597
---------
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: TheWitness <copilot@users.noreply.github.com>
Co-authored-by: TheWitness <copilot@users.noreply.github.com>
Co-authored-by: TheWitness <thewitness@cacti.net>
## Summary
Consolidates the overlapping ICMP work from #555 and #619 into one
review of the complete socket and resolver state machine.
- add ICMPv6 echo polling and dispatch IPv6 devices to it
- prefer unprivileged datagram ICMP sockets when the kernel permits them
- retain bounded raw-socket fallback for IPv4 and IPv6
- give packet and socket resources one cleanup owner
- leave caller-owned resolver state untouched on lookup failure
- keep every retry inside one cached capability and privilege-transition
decision
- prevent LOCK_SETEUID retry deadlocks
- validate raw and datagram reply layouts, source, sequence,
identifiers, and payload
- retain fuzz and unprivileged-socket integration coverage
The consolidation also fixed an integration defect found locally: the
controlled successful-reply test constructed only a raw IPv4 packet even
when the production path selected SOCK_DGRAM. It now models the selected
socket format, and IPv6 raw retries use the same privilege-safety
invariant as IPv4.
## Verification
- warning-enabled local build completed with MariaDB Connector/C,
Net-SNMP, OpenSSL 3, and cmocka
- make check: 5/5 test binaries passed
- focused ping_icmp suite: 9/9 cases passed
- actionlint and git diff --check passed
- all commits are signed
- GitHub CI is authoritative for Linux capability, sanitizer, fuzz, and
unprivileged-network coverage
Closes#127.
Closes#250.
Closes#593.
Closes#611.
Supersedes #555.
Refs #597 and #618.
---------
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: TheWitness <copilot@users.noreply.github.com>
Co-authored-by: TheWitness <thewitness@cacti.net>
Co-authored-by: TheWitness <copilot@users.noreply.github.com>
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#551Closes#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>
Fresh develop-based port of applicable memory-safety, process-cleanup,
SNMP, poller, and PHP transport fixes from #544. Obsolete release and CI
history was excluded.
---------
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Three small fixes for issues reported by users, the oldest open since
2018. Built and verified in a clean ubuntu:24.04 container at 39
warnings, matching develop.
**#552** — `die()` did not terminate its output, so consecutive fatal
messages ran together in `cacti_stderr.log`:
```
FATAL: Unable to read configuration file! (Spine init)FATAL: Unable to read configuration file! (Spine init)
```
Fix is the one-line change olafhering supplied in the report.
**#263** — twenty-five log sites emitted `DEBUG: Device[n] ...` while
the other hundred-plus use `Device[n] DEBUG: ...`, so a single poll
produced both orders. Four of them also printed `DEBUG` twice in one
line. All normalised to the majority form. Log text is otherwise
unchanged.
**#60** — Spine already reads the Cacti version into `set.cacti_version`
and knows its own from `VERSION`, but never compared them. It now warns
once at startup when the two are from different release lines. Only
major.minor is compared, so point-release drift stays quiet.
Closes#552Closes#263Closes#60
---------
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
## 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>
Follow-up PR for warning cleanup outside #452 scope.\n\nChanges:\n- make
read_spine_config take a const file path in util.h and util.c\n- fix
pthread_self log format specifier to match cast type
(%lu)\n\nValidation:\n- change is minimal and isolated\n- local
configure/build could not complete in this environment due missing MySQL
headers
## Linked issue
Fixes#464
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Co-authored-by: TheWitness <thewitness@cacti.net>
The SSL Options point to files, and a reasonable path should not exceed
1024 bytes. Additionally, we should comment out the cppcheck until we
have had more time to review the warnings. These are mostly, but not all
style and non-fatal, but good to have addressed for optimal program
flow.
Fixes#447
When \`strlen(src) >= obuf\`, the old code set \`len = obuf\` then wrote
\`dst[obuf] = '\0'\` one byte past the end of the buffer. A
\`-Wstringop-overflow\` pragma was suppressing the compiler warning.
Fix: cap copy length at \`obuf - 1\` so the NUL terminator always fits
within the buffer. Remove the pragma so the compiler can flag similar
patterns in future. Add an \`obuf == 0\` guard for the degenerate case.
Rename \`len\` to \`src_len\` for clarity -- the value is the source
length being copied, not a generic counter.
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Co-authored-by: netniV <netniv@hotmail.com>
Fixes#445
Two code-safety fixes targeting undefined behavior in the polling
threads.
**php.c**: `vfork()` is deprecated in POSIX.1-2024 and unsafe in spine's
pthreaded model because the child shares the parent's address space
until `execv()`. Replace with `posix_spawn()` +
`posix_spawn_file_actions_t` for stdin/stdout redirection. The
EAGAIN/ENOMEM retry loop is preserved as a `do/while`. `extern char
**environ` declared explicitly since it is not guaranteed to be visible
under `-std=c99`.
**util.c**: `regex_replace()` returned a pointer to a stack-local `char
msgbuf[100]`. The buffer lifetime ends when the function returns;
callers dereference freed stack. Changed to `static __thread char
msgbuf[SMALL_BUFSIZE]` -- each polling thread gets its own persistent
buffer, no heap allocation, no races. Bounds check added for matches
longer than `SMALL_BUFSIZE`. All five call sites in poller.c consume the
result before the next call on the same thread.
Both files compile clean under `gcc -std=c99` with `-Wall -Wextra`.
---------
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Co-authored-by: TheWitness <thewitness@cacti.net>
Co-authored-by: netniV <netniv@hotmail.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>
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>
## Summary
`die()` uses `vsprintf` to format error messages into a 1024-byte stack
buffer with no length limit. If the formatted string is long enough
(think: a really verbose database error with a hostname and query
fragment), it writes past the end of the buffer.
Same story with the `strcat` that appends the errno string — no check on
remaining space.
Swapped both for their bounded cousins:
- `vsprintf` → `vsnprintf` with `BUFSIZE`
- `strcat` → `strncat` with remaining capacity
Two-line fix. Truncation beats a stack smash.
## Test plan
- [x] Builds clean with `-Wall -Wextra` (verified via Docker ASan build)
- [x] cppcheck shows zero new findings vs develop baseline
- [x] `spine --version` exits 0
- [ ] ASan build: trigger `die()` with a >1024 char format string —
should exit cleanly, no crash
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
I actually think this one should be an unsigned long int instead of just
a long int.
---------
Co-authored-by: Erik Sejr <eriks@ssicanada.com>
Co-authored-by: TheWitness <thewitness@cacti.net>
While compiling Clang 16 generates the following warnings.
in spine.c:
```
spine.c:934:101: warning: format specifies type 'long' but the argument has type 'pthread_t' (aka 'struct pthread *') [-Wformat]
SPINE_LOG_DEBUG(("DEBUG: Device[%i] Valid Thread to be Created (%ld)", poller_details->host_id, threads[device_counter]));
```
in util.c:
```
util.c:1265:100: warning: format specifies type 'long' but the argument has type 'pthread_t' (aka 'struct pthread *') [-Wformat]
snprintf(logprefix, SMALL_BUFSIZE, "SPINE: Poller[%i] PID[%i] PT[%ld] ", set.poller_id, getpid(), pthread_self());
```
This one i'm not as sure about, maybe this should be cast to a long
unsigned int instead? I have not had problems running it this way but
i'm not 100% sure this the right fix.
Co-authored-by: Erik Sejr <eriks@ssicanada.com>
I actually think this one should be an unsigned long int instead of just
a long int.
---------
Co-authored-by: Erik Sejr <eriks@ssicanada.com>
Co-authored-by: TheWitness <thewitness@cacti.net>
While compiling Clang 16 generates the following warnings.
in spine.c:
```
spine.c:934:101: warning: format specifies type 'long' but the argument has type 'pthread_t' (aka 'struct pthread *') [-Wformat]
SPINE_LOG_DEBUG(("DEBUG: Device[%i] Valid Thread to be Created (%ld)", poller_details->host_id, threads[device_counter]));
```
in util.c:
```
util.c:1265:100: warning: format specifies type 'long' but the argument has type 'pthread_t' (aka 'struct pthread *') [-Wformat]
snprintf(logprefix, SMALL_BUFSIZE, "SPINE: Poller[%i] PID[%i] PT[%ld] ", set.poller_id, getpid(), pthread_self());
```
This one i'm not as sure about, maybe this should be cast to a long
unsigned int instead? I have not had problems running it this way but
i'm not 100% sure this the right fix.
Co-authored-by: Erik Sejr <eriks@ssicanada.com>
Thomas Vincent