mirror of
https://github.com/Cacti/spine.git
synced 2026-10-23 03:27:10 +09:00
develop
22
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Thomas Vincent
|
9582b71704 |
fix(runtime): [develop] retain reviewed probe and compiler stages after result normalization (#660)
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> |
||
|
Thomas Vincent
|
9be5f8ad2e |
fix(results): [develop] normalize output and unify SNMP batch persistence (#641)
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> |
||
|
Thomas Vincent
|
898d8c692c |
fix(privileges): drop root before input and retain owned ICMP sockets (#639)
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> |
||
|
Thomas Vincent
|
3065c6fc23 |
refactor(core): add bounded native interfaces and polling contracts (#638)
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> |
||
|
Thomas Vincent
|
dd73131f58 |
fix(php): validate reader process slots before accessing state (#637)
`php_readpipe()` can access `php_processes[php_process]` without the slot validation in `php_cmd()`. Validate the process table and both configured and fixed array bounds before the reader inspects descriptors or initiates recovery. Invalid inputs return an owned `"U"` result through the existing allocation/error contract. The regression exercises the public reader and startup reader with a missing process table, negative slot, configured limit, array limit, `INT_MAX`, zero/negative configured counts and a count larger than the array. It asserts `"U"`, frees each result and verifies no spawning or protocol writes. Existing real-child round-trip and shutdown tests remain in the suite. Validation against develop `ca662c1b` in isolated Linux containers: - Upstream pull-request CI: all 19 checks passed, including builds, integration, coverage, sanitizers, fuzzing and release-archive checks. - Clean Autotools build and `make check`: 11 passed, no skips or failures. - ASan/UBSan: 10 passed; the existing allocator-interposition suite is incompatible with sanitizer interposition and skips as designed. No sanitizer findings. - Measured gcov coverage exercises both outcomes of all four added guard conditions, including 32 invalid-input returns. - Removing the guard fails the new regression; restoring it passes. This is a focused, DCO-signed change to `php.c` and its existing unit suite. Valid script protocol behavior and child cleanup are preserved. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> |
||
|
Thomas Vincent
|
d1cc471967 |
fix(ping): match ICMPv6 peers by address octets (#636)
IPv6 replies are matched by comparing `struct in6_addr` as a whole. Compare its 16 `s6_addr` bytes explicitly so peer identity does not depend on the structure representation. The regression tests exercise both datagram and raw ICMPv6 receive paths: each rejects 16 replies with a different wrong address octet before accepting the matching peer. They check the existing response text, receive count and descriptor cleanup. Validation against develop `ca662c1b` in isolated Linux containers: - Upstream pull-request CI: all 19 checks passed, including builds, integration, coverage, sanitizers, fuzzing and release-archive checks. - Clean Autotools build and `make check`: 11 passed, no skips or failures. - ASan/UBSan: 10 passed; the existing allocator-interposition suite is incompatible with sanitizer interposition and skips as designed. No sanitizer findings. - Measured gcov coverage of the changed comparison: 34 executions, both outcomes covered (32 rejected / 2 matching). - A mutation comparing only 15 address bytes fails both new tests; restoring the full comparison passes. This is a focused, DCO-signed change to `ping.c` and its existing unit suite. Polling retries, output and privileges are preserved. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> |
||
|
TheWitness
|
ca662c1be7 |
fix: cap Solaris reap budget, bound EINTR retries, kill before parking on waitpid error (#635)
## What Follow-up to #633 (already merged), from the code review findings on the equivalent 1.2.x change (#634): - **SOLAR_THREAD sleep budget**: `spine_reap_child_bounded()` has no `usleep()`-paced spin phase on `SOLAR_THREAD` builds - it sleeps a flat 1s per attempt. #633 raised the shared attempt budget (`NFT_PCLOSE_SPIN_ATTEMPTS`) to 100 for `nft_pclose()`'s brief allowance, which made it sleep up to **~100 seconds** on that platform instead of the intended ~20ms, reintroducing the exact poller stall #633 was fixing. `NFT_PCLOSE_SPIN_ATTEMPTS` is now platform-specific (100 elsewhere, 2 under `SOLAR_THREAD`). - **Unbounded EINTR retries**: bounded the `EINTR` retry loop in `spine_reap_child_bounded()` and in `nft_sweep_abandoned()` (which runs with `ListMutex` held), so a stream of caught signals can no longer spin either indefinitely. - **Kill before park on waitpid() error**: `nft_pclose()` now SIGKILLs the child before parking it when `waitpid()` itself errors (previously only done when the child was confirmed still running), so a still-running child isn't left outside the abandoned-pid sweep's reach. Preserves `waitpid()`'s errno across the call. - **Docs**: `nft_popen.h`'s `nft_pclose()` comment still described the old blocking-wait behavior and didn't mention `ETIMEDOUT` - #633 updated `nft_popen.c`'s comment but missed the header. Both now document the bounded-check/parked-child contract. ## Why These were flagged by review of #634 (the same fix ported to 1.2.x, where the affected code showed up as new rather than a diff against pre-existing lines). Fixing here too since develop has the identical code. ## What was tested - `./bootstrap && ./configure --enable-warnings && make` - clean build, no new warnings. - `make check` - all 11 suites pass. - `cppcheck --enable=all --std=c11` on `nft_popen.c` - no new errors. Closes #632. --------- Signed-off-by: TheWitness <thewitness@cacti.net> |
||
|
TheWitness
|
08e2ae0b42 |
fix: reap script/PHP children without blocking the poller, drop USING_TPOPEN (#633)
## What - `nft_pclose()` (`nft_popen.c`) and `php_terminate_and_reap()` (`php.c`) no longer escalate through a spin/sleep/SIGTERM/SIGKILL sequence. Each now does one bounded, non-blocking check (~20ms allowance for normal fork/exec/exit scheduling latency), kills anything still running, and hands it to the existing abandoned-pid sweep instead of waiting on it in the calling (polling) thread. - Removed the `USING_TPOPEN`/`--enable-popen` native-`popen()` fallback in `exec_poll()` (`poller.c`, `configure.ac`). It could not track or kill a timed-out child (no pid available from a real `popen()`), so a timeout there could only ever abandon the descriptor without terminating the process. - Extended `tests/regression/test_child_process_safety.sh` with structural guards against reintroducing the blocking spin/sleep escalation in either function. - Updated `tests/unit/test_linked.c`'s `nft_pclose()` coverage for the new, faster contract (the old test asserted a graceful-SIGTERM-then-wait exit code that the new design intentionally no longer provides). ## Why Investigating a reported poller slowdown surfaced that `nft_pclose()` (added by #614) could block a poller thread for up to ~5 seconds per script whose process hadn't exited by the time its pipe was closed, and `php_terminate_and_reap()` had the same shape (up to ~2s), reachable mid-poll-cycle via `php_fail_read()`, not just at shutdown. Separately, the `USING_TPOPEN` fallback's inability to kill a timed-out child was flagged as out of scope in #574. Tracked in #632; design principles written up in #630. ## What was tested - `./bootstrap && ./configure --enable-warnings && make -j` — clean build, no new warnings. - `make check` — all 11 suites pass (unit, regression, integration), including the extended `test_child_process_safety.sh` guard and the updated `test_linked` case. - `cppcheck --enable=all --std=c11` on the three changed `.c` files — no new errors (pre-existing style-only notes unrelated to this change). Signed-off-by: TheWitness <thewitness@cacti.net> |
||
|
|
86a062dd09 |
ci: enforce cross-platform quality gates and connector coverage (#618)
## 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> |
||
|
|
dbbccc5263 |
fix: extract core safety fixes from #597 (#621)
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 #564 Closes #565 Closes #566 Closes #567 Closes #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> |
||
|
|
72b8904985 |
fix: restore safe SNMPv3 session construction (#620)
Extracted from #597 into a focused SNMPv3 correctness and safety change. This restores noAuthNoPriv and authNoPriv session construction, derives authentication keys on the authNoPriv path, validates configured algorithms when they are used, wipes temporary passphrase copies, and makes the effective security-level decision agree with Cacti. It also prevents a failed mid-loop session rebuild from passing NULL into Net-SNMP multi-get. Compatibility and failure policy: - [None] is the explicit no-protocol sentinel; stale passphrases paired with it downgrade with a warning to the Cacti effective level. - A selected protocol missing its passphrase downgrades with an explicit warning, preserving existing polling behavior. - A blank protocol field paired with a passphrase is ambiguous and now fails closed with an error. - Complete privacy credentials without usable authentication fail closed because USM cannot honor privacy without authentication. - Unknown authentication algorithms fail closed instead of silently substituting a default; unused privacy algorithms do not block authNoPriv. Verification: - warning-enabled C99 build on macOS arm64 - make check: all 5 suites passed - deterministic SNMPv3 session suite: 22/22 pass, including literal OID mapping, effective-level combinations, refusal paths, key derivation, caller-buffer ownership, and NULL multi-get handling - mandatory independent pre-push review: PASS Closes #582 Refs #597 --------- 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> |
||
|
|
478d330985 |
fix(ping): make IPv4 and IPv6 ICMP polling safe (#619)
## 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> |
||
|
Thomas VincentandTheWitness
|
90da94b94f |
fix(process): unify subprocess ownership and child lifecycle safety (#614)
## Summary - make subprocess-registry detach atomic so exactly one closer owns each entry - make cancellation cleanup release only the detached node it owns - replace `vfork`/`execve` child setup with `posix_spawn` and explicit file actions - mark every pipe and stdio-collision duplicate close-on-exec - restore the child's default SIGPIPE disposition while preserving the parent's handling - bound TERM/KILL reaping and park unreaped children in a fixed-capacity sweep list - harden PHP script-server slot ownership, startup, restart, signal, descriptor, and cleanup paths - retire and restart failed or oversized reads so stale response tails cannot poison later data sources - revalidate every command retry and cap recovery handshakes at two seconds This folds the child-process lifecycle portion of #597 into the narrower ownership PR. It does not absorb unrelated poll-host, SQL, SNMP, ICMP, or build-infrastructure work. ## Review-driven corrections - close-on-exec is explicitly restored after `dup()` in both read and write stdio-collision branches - the source invariant check now uses portable POSIX character classes - direct tests cover duplicate flags, both collision branches, failure after pipe creation, abandoned-child sweeping, and capacity overflow - collision cleanup preserves current-child redirects even when an older registry entry occupies fd 0 or 1 - script children receive SIGTERM before SIGKILL and unreaped PHP children transfer to the bounded sweep registry - the Docker integration suite now starts a real PHP script server, verifies its startup protocol and database result, and checks that shutdown leaves no zombie PHP process ## Validation - Ubuntu 24.04 container with GCC and GNU ld: `make check`, 6/6 targets passed with no skips, including `test_php_runtime` - real PHP script-server Docker integration: 6/6 checks passed; exact value `42` reached `poller_output` and no zombie remained - macOS Homebrew dependency build passed - macOS `make check`: 5/5 available binaries passed; GNU `--wrap` runtime suite is Linux-only - PHP fixture syntax checked through mise PHP 8.3 - actionlint, shellcheck, and `git diff --check` passed Closes #610. --------- Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> Co-authored-by: TheWitness <thewitness@cacti.net> |
||
|
Thomas VincentandTheWitness
|
f989722458 |
build: establish src/core layout for support modules (#617)
## What - move `keywords.c`/`.h` and `locks.c`/`.h` into `src/core/` - enable Automake `subdir-objects` for collision-safe nested object placement - update the Autotools binary, linked-test, standalone unit-test, and fuzz paths - make both cppcheck entry points share one complete nested-source discovery path - ignore nested Automake and Docker build artifacts - retain existing header include names and the Autotools build The four production source/header files are 100% renames with no content changes. The remaining changes are build and verification plumbing required by the new paths. ## Why Spine production sources currently live in the repository root. This starts the domain layout in a small, reviewable batch instead of repeating #523, where the source move was coupled to an 11k-line CMake/platform rewrite. `keywords` and `locks` were selected because active PRs #597 and #614 do not modify their implementation files; only shared build manifests overlap #597. `subdir-objects` is enabled in this first production move so future modules with the same basename cannot write the same top-level object under parallel builds. Closes #616. ## Verification - `./bootstrap` with no subdirectory-source warnings - `./configure --with-mysql=$(brew --prefix mariadb-connector-c) --with-snmp=$(brew --prefix net-snmp) CPPFLAGS=-I$(brew --prefix openssl@3)/include LDFLAGS=-L$(brew --prefix openssl@3)/lib` - warning-enabled `make -j$(sysctl -n hw.ncpu)`; project sources compile, with Homebrew Net-SNMP header warnings only - `make check` (cmocka unavailable to Autoconf locally; 0 local tests) - `make distcheck`; archive verified to contain all four `src/core` files - cppcheck source discovery verified under macOS Bash 3 and includes all four relocated files - standalone `tests/unit/Makefile` compiles through header resolution with `-I../../src/core` (the local macOS link retains unrelated existing client-library/toolchain issues) - GitHub checks pass, including fuzz, sanitizers, coverage, `distcheck`, gcc/clang, SNMPv3, and Rocky Linux The local macOS fuzz build reaches both relocated files, then fails identically on untouched `util.c` (`INT_MAX`) and Net-SNMP API declarations on both this branch and `develop`. Linux fuzz CI is the authoritative fuzz-build result. --------- Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> Co-authored-by: TheWitness <thewitness@cacti.net> |
||
|
Thomas Vincent
|
8208fcb593 |
test: link the shipped objects instead of copies of the logic (#577)
The three unit binaries in `tests/unit/` do not test the code that ships. `test_safety_fixes.c` says so in its own header: > the functions under test are inlined (or their exact predicate replicated) so this translation unit compiles standalone without linking util.o/ping.o/php.o and dragging in MySQL/SNMP That keeps them cheap, but a fix can land in `util.c` while the test carries on passing against its private copy. The 39 assertions currently guarantee less than they appear to. This adds a fourth binary that links the real translation units. `tests/fuzz/stubs.c` already solves the hard part for the fuzz targets by supplying the globals `spine.c` would define, so the same file is reused here. Everything except `spine.c` is linked as built. **Demonstration.** Reverting `strncopy()` to the issue#447 off-by-one and running the suite: ``` PASS: tests/unit/test_util_strings PASS: tests/unit/test_build_fixes PASS: tests/unit/test_safety_fixes FAIL: tests/unit/test_linked # TOTAL: 4 # PASS: 3 # FAIL: 1 ``` The three existing suites do not notice. Only the linked one does. Seven cases to start, covering `strncopy()` bounds and termination and `regex_replace()` match/no-match/bad-pattern. The point of the PR is the harness rather than the case count: with this in place, future tests can assert against real behaviour, and the existing binaries can migrate over one at a time. Verified in a clean `ubuntu:24.04` container: builds at 4 warnings matching develop, `make check` passes 4/4, `make distcheck` passes. --------- Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> |
||
|
Thomas Vincent
|
d63e136b89 |
test: add the unit, fuzz and integration suites (#563)
Split out of #542. Tests and CI only, no source file is touched, so this cannot conflict with anything currently open. Verified in a clean `ubuntu:24.04` container against develop as it stands. **Unit** — 39 cmocka assertions across three binaries, run with `make -C tests/unit`: ``` tests: 16 test(s) run. PASSED tests: 20 test(s) run. PASSED tests: 3 test(s) run. PASSED ``` `test_safety_fixes.c` covers the allocation, bounds and reap paths; `test_build_fixes.c` covers `regex_replace` and the config defaults; `test_util_strings.c` covers `strpos`. **Fuzz** — a `get_namebyhost` target with a 26-input corpus, built with `-fsanitize=fuzzer,address,undefined`: ``` #2000 DONE cov: 64 ft: 85 corp: 11/48b ``` **Integration** — the SNMPv3 fixture polls a live `snmpd` container and asserts on the rows spine wrote to `poller_output` rather than on its exit code, plus scripted checks for CLI argument handling, the script server reap, and `output_regex` column detection. **Not included.** The `fuzz_icmp_reply` target stays on #542, because it calls `spine_icmp_classify_reply()`, which that PR adds. Everything else stands on its own here. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> |
||
|
Thomas Vincent
|
0c481bc526 |
port: memory-safety fixes from Spine #544 to develop (#557)
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> |
||
|
Thomas Vincent
|
70459ac82c |
fix(build): resolve compilation failures on develop (#514)
## Summary Fixes #515. Spine on `develop` fails to compile after recent feature branch merges (everything after `4837ddd`). This PR fixes 6 build/runtime bugs, replaces deprecated `vfork`, and adds 68 automated tests. ## Fixes ### Build-breaking (prevented compilation) 1. **Missing uthash.h** - vendored v2.3.0 (header-only, BSD license). The O(1) hash table feature added `#include "uthash.h"` but never committed the file. 2. **Extra closing braces in poller.c** - duplicate `}` in POLLER_ACTION_SCRIPT and POLLER_ACTION_PHP_SCRIPT_SERVER cases broke the switch statement. ### Runtime bugs 3. **SNMP_FREE macro collision** - spine's `SNMP_FREE` (calls `snmp_host_cleanup`) collided with net-snmp's `SNMP_FREE` (calls `free`). Renamed to `SNMP_SESSION_FREE`. The `errstr` from `snmp_sess_error()` now correctly uses net-snmp's `free()` instead of `snmp_host_cleanup()`. 4. **output_regex column detection** - the output_regex feature (#210) assumed the column exists, but baseline Cacti does not have it. Added `db_column_exists()` check at startup to conditionally include it in queries. ### Deprecation warnings 5. **vfork in nft_popen.c** - replaced with `posix_spawn` using file actions, matching the approach already used in `php.c`. Eliminates macOS deprecation warning. ### Test fixture 6. **snmp_auth_protocol column width** - `char(6)` too narrow for `SHA-256` (7 chars). Fixed to `char(7)` in both host and poller_item tables. Added `rocommunity public` to snmpd.conf for SNMPv2c test coverage. ## Test Results (68/68 pass) | Suite | Count | Coverage | |-------|-------|----------| | Unit (cmocka) | 16 | uthash macros, config init, regex_replace, strncopy | | Smoke | 21 | SNMPv3 + SNMPv2c polling, DB writes, multi-device, runtime validation | | Output regex | 15 | Column absent, present, regex filtering, removal fallback | | DB column detect | 16 | 3 scenarios: absent, added via ALTER TABLE, dropped | ## Files changed (15) | File | Change | |------|--------| | `uthash.h` | New: vendored header | | `Makefile.am` | Add uthash.h to EXTRA_DIST | | `common.h` | Add uthash.h include | | `poller.c` | Fix braces, conditional output_regex in queries | | `spine.c` | Add has_output_regex detection at startup | | `spine.h` | Add has_output_regex to config struct | | `snmp.h` | Add SNMP_SESSION_FREE macro | | `sql.c` | Fix indentation in db_query(), restore missing continue | | `nft_popen.c` | Replace vfork+execve with posix_spawn | | `tests/snmpv3/db/init.sql` | Fix snmp_auth_protocol char(7) | | `tests/snmpv3/snmpd/snmpd.conf` | Add rocommunity public | | `tests/integration/smoke_test.sh` | New: e2e smoke test | | `tests/integration/test_output_regex.sh` | New: output_regex scenarios | | `tests/integration/test_db_column_detect.sh` | New: db_column_exists scenarios | | `tests/unit/test_build_fixes.c` + `Makefile` | New: cmocka unit tests | ## Follow-up PRs - #517: Replace deprecated POSIX semaphores with pthread wrapper (9 remaining warnings) - SNMP error handling, spike kill, script server path testing Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> |
||
|
Thomas Vincent
|
0dd91c4602 |
chore(warnings): mechanical low-risk cleanup pass #499 (#513)
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> |
||
|
Thomas VincentandnetniV
|
e37cecf21f |
fix(poller): add NULL check after malloc in host_errors insert path (#431)
Fixes #426 `malloc()` at `poller.c:1941` had no NULL check. On failure the subsequent `snprintf(NULL, ...)` is undefined behavior and segfaults, killing the spine process and leaving all polled hosts without output for that cycle. Add `die()` on NULL to match every other `malloc` call in the codebase. Also adds a Docker-based SNMPv3 regression fixture under `tests/snmpv3/` covering the NULL-check path, ICMP retry behavior, and `snmp_count` validation. --------- Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> Co-authored-by: netniV <netniv@hotmail.com> |
||
|
Thomas VincentandnetniV
|
31848b522d |
fix(poller): correct inverted spike_kill condition for PHP script server (#429)
Fixes #424 `POLLER_ACTION_PHP_SCRIPT_SERVER` at `poller.c:1678` used `IS_UNDEFINED` where it should use `!IS_UNDEFINED`. The guard was true only when the result was already `"U"`, making `SET_UNDEFINED` a no-op. PHP script server data sources were never spike-killed after agent restarts, producing spurious spikes in graphs. Change `IS_UNDEFINED` to `!IS_UNDEFINED` to match the adjacent `POLLER_ACTION_SCRIPT` block. Also adds a Docker-based SNMPv3 regression fixture under `tests/snmpv3/` that covers the spike_kill fix and serves as a shared harness for related poller correctness tests. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> Co-authored-by: netniV <netniv@hotmail.com> |
||
|
Thomas VincentandTheWitness
|
c501bc8499 |
fix(snmp): move count++ inside valid-type branch in snmp_count() (#427)
Fixes #422 `count++` at `snmp.c:899` fired before the `SNMP_ENDOFMIBVIEW` type check. When an agent returns ENDOFMIBVIEW with the OID still within the requested subtree (RFC 3416 permits this), the out-of-subtree guard does not fire and count reaches 1 for an empty table. The poller reindex assertion then incorrectly triggers a reindex every poll cycle. Move `count++` inside the `if (vars->type != SNMP_ENDOFMIBVIEW && ...)` branch so only real entries increment the counter. Verified by `tests/snmpv3/scripts/run_test.sh` Phase 4: inserts a `poller_reindex` row with `action=10` for a non-existent OID subtree and confirms spine logs `output: 0`. --------- Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> Co-authored-by: TheWitness <thewitness@cacti.net> |