Repository navigation
Fix a batch of unaddressed bugs (#594, #600, #605, #607, #575, #583, #580) - #627
Merged
Merged
Conversation
…scrub, FD_SETSIZE, SIGPIPE, DES link, escape sizing, CodeQL poll_host(): call mysql_thread_end() on the device-row-missing exit path, matching the other two exits (#594). snmp_host_init(): the zero_sensitive scrub flag was always 0, so the guarded memset() calls that were supposed to clear SNMPv3 passphrase copies never ran. Make scrubbing of this module's own heap copies (Apsz/Xpsz) unconditional; drop the guarded memsets against the caller's snmp_password/snmp_priv_passphrase, since those are host_t-owned and reused for later session rebuilds on the same host (#600). php_readpipe(): FD_SET() does not range-check, so a read descriptor at or above FD_SETSIZE wrote past the end of the on-stack fd_set. Added a bounds check that treats an out-of-range descriptor as a script-server failure and restarts it (#605). spine_signal_handler(): every fatal signal reset itself to SIG_DFL and never re-armed. That is the correct 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 killed the poller outright. Re-arm only SIGPIPE (#607). configure.ac/snmp.c: some distributions (e.g. Fedora) ship a net-snmp-config.h that advertises usmDESPrivProtocol as the default privacy protocol without libnetsnmp actually exporting the symbol, so spine failed to link. Added an AC_LINK_IFELSE probe for the real symbol and guarded the fallback to use usmAESPrivProtocol when it is absent (#575). sql.c db_escape() capped its internal trim buffer at a fixed DBL_BUFSIZE regardless of the caller's max_size, so any result of 1024 bytes or more was silently cut to 1022 characters no matter how large --with-results-buffer was set. The trim buffer now scales with max_size, and poller.c's escaped_result destination is sized to match RESULTS_BUFFER instead of the fixed DBL_BUFSIZE (#583). Restored a CodeQL workflow for develop. It was dropped as an incidental side effect of an unrelated CI change in #452 and nothing replaced it, so develop has had no static analysis coverage since (#580).
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Critical and moderate issues remain in workflow permissions, signal handling, restart behavior, secret scrubbing, and SQL buffer sizing.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 3
Open (5)
What changed in this PR
This pull request fixes seven independent bugs across polling, SNMP, portability, SQL escaping, signal handling, and CodeQL CI.
Changes:
- Fixes cleanup, descriptor bounds, signal handling, and result-buffer sizing.
- Improves SNMPv3 scrubbing and DES/AES portability.
- Restores the CodeQL workflow.
| File | Changes |
|---|---|
sql.c |
Scales escaping buffers with the requested output size. |
snmp.c |
Updates passphrase handling and privacy-protocol fallback. |
poller.c |
Adds MySQL thread cleanup and larger escaped-result storage. |
php.c |
Guards against oversized descriptors. |
error.c |
Re-arms SIGPIPE handling. |
configure.ac |
Probes DES symbol availability. |
.github/workflows/codeql.yml |
Restores CodeQL scanning. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codifies the existing -issue#NNN: convention so future PRs (including AI-assisted ones) add a CHANGELOG entry referencing the issue they close.
codeql.yml: the job-level permissions block overrode the workflow defaults and dropped contents: read, which checkout needs. Added it back at the job level. error.c: re-arming SIGPIPE only at the end of the handler still left a window, between the top-of-handler reset and the re-arm, where another thread's broken-pipe write would hit SIG_DFL and terminate the process (signal dispositions are process-wide). Skip the reset entirely for SIGPIPE instead, so the handler stays installed throughout. sql.c: db_escape()'s max_size guard allowed 1-3, where (max_size/2)-1 became 0 (unterminated output) or -1 (converted to a huge size_t that overflowed the allocation). Reworked the trim size calculation so it always reserves the terminator for any max_size >= 1. php.c: the new FD_SETSIZE guard called php_close()+php_init() to restart the server, but php_init() validates the replacement by calling back into this function, which can hit the same guard again under sustained descriptor exhaustion and recurse without bound. Close and mark the slot busy instead of restarting synchronously from within the guard; the next independent call attempts recovery. snmp.c: the zero_sensitive fix scrubbed Apsz/Xpsz before they were assigned, which is always a no-op since both start NULL every call. Moved the memset to immediately precede each of their frees, including the two generate_Ku() error-return paths, so the live passphrase copy is actually cleared before every exit.
config_DATA only ever installed spine.conf.dist itself; nothing produced a live spine.conf, so a fresh install had to be copied by hand before spine would read any configuration. Add an install-data-hook that copies spine.conf.dist to spine.conf under $(configdir) only when spine.conf does not already exist, so a first install seeds a usable config and a re-install never clobbers local edits. Verified via WSL: a fresh 'make install DESTDIR=...' creates etc/spine.conf identical to spine.conf.dist; a second install with a hand-edited spine.conf leaves it untouched.
TheWitness
requested review from
bmfmancini,
browniebraun,
netniV,
somethingwithproof and
xmacan
September 22, 2026 13:55
…etry - .github/workflows/codeql.yml: the repo already has GitHub's default CodeQL setup enabled and running successfully (confirmed via `gh api repos/Cacti/spine/code-scanning/default-setup` -> state: "configured", covering actions/c-cpp/ruby). A custom "advanced configuration" workflow cannot coexist with default setup - GitHub rejected the uploaded SARIF with "CodeQL analyses from advanced configurations cannot be processed when the default setup is enabled", failing the analyze job outright. Removed the redundant workflow; issue #580's underlying gap (no scanning on develop) was already resolved out of band by enabling default setup, so this PR no longer claims to close it. - Makefile.am: `make distcheck`'s distuninstallcheck step install/ uninstalls into a sandbox and fails if any file is left behind. install-data-hook seeds spine.conf, but nothing removed it again on uninstall, so distcheck failed with "files left after uninstall: ./etc/spine.conf". Added a matching uninstall-hook that only removes spine.conf when it is still byte-identical to spine.conf.dist (i.e. never customized) - a real uninstall never discards a live config, while distcheck's untouched sandbox copy still gets cleaned up. Verified via WSL: `make distcheck` now passes end to end.
xmacan
approved these changes
Sep 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
Closeslist).Fixes
poll_host()skipsmysql_thread_end()when the device row is missing — the earlyreturnonnum_rows != 1released both DB connections and freed six allocations, but never calledmysql_thread_end(), unlike the other two exit paths. Added the missing call.zero_sensitivewas a function-local initialized to 0 and never assigned, so every guardedmemset()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 bothgenerate_Ku()error paths) after review found the first pass still scrubbed before assignment. Left the caller'ssnmp_password/snmp_priv_passphrasealone rather than also scrubbing those in place — those arehost_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 aboveFD_SETSIZE(1024 on glibc) wrote past the end of the on-stackfd_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 throughphp_init()→php_readpipe()under sustained descriptor exhaustion, so it no longer restarts synchronously from here).SIG_DFLand 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 hitSIG_DFL; fixed by never resetting SIGPIPE's disposition in the first place.usmDESPrivProtocol— some distributions ship anet-snmp-config.hthat advertises DES as the default privacy protocol while the shippedlibnetsnmpdoesn't export the symbol. Added anAC_LINK_IFELSEprobe for the actual symbol and guarded the fallback to useusmAESPrivProtocolwhen it's absent.db_escape()capped its internal trim buffer at a fixedDBL_BUFSIZEregardless of the caller'smax_size, so raising--with-results-buffernever helped. The trim buffer now scales withmax_size(a review finding caught the trim-size formula going negative/zero formax_size1-3), andescaped_resultinpoll_host()is sized toRESULTS_BUFFER * 2 + 1instead of the fixedDBL_BUFSIZE. (A very large escaped result can still hit the separate, already-tracked row-size limit in poller_output rows are truncated when the escaped result exceeds 2304 bytes #598/ci: enforce cross-platform quality gates and connector coverage #618 — this fix only addresses the escaping-stage truncation.)Also included
spine.conffromspine.conf.diston first install —make installonly ever installedspine.conf.dist; nothing produced a livespine.conf. Added aninstall-data-hookthat copies it over only whenspine.confdoesn't already exist, plus a matchinguninstall-hookthat only removes it again when it's still byte-identical to the.dist(so a real uninstall never discards a customized config, whilemake distcheck's install/uninstall symmetry check still passes).Dropped from this PR
codeql.ymlworkflow (originally added for CodeQL stopped running on develop after #452 #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. CodeQL stopped running on develop after #452 #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 inpoller.c,snmp.c,php.c,error.c, orsql.cbeyond two pre-existing, unrelated-Wformat-truncationwarnings inphp_init(). Confirmed theusmDESPrivProtocolconfigure probe correctly detects the missing symbol in this build environment and takes the AES fallback path without a link error.make distcheckpasses end to end.Closes #594
Closes #600
Closes #605
Closes #607
Closes #575
Closes #583