Fixes#442
Spine has no CI. This adds a basic safety net.
- **Build matrix** (gcc + clang): catches cross-compiler regressions
- **cppcheck**: configured with reasonable flags; suppresses known false
positives
- **flawfinder**: `error-level=5` blocks only critical hits; full report
uploads as artifact
- **CodeQL**: GitHub's semantic scanner, runs weekly and on every
push/PR
All actions pinned to full SHA. Least-privilege permissions per job.
What this does not do: run tests (there are none), use -Werror (existing
warning count would block it), or block on every flawfinder hit.
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#443
Companion to #401. Adds four .PHONY make targets:
make docker # build the runtime image
make docker-dev # build the dev image (ASan, cppcheck, scan-build,
valgrind)
make verify # build dev image + run the full analysis suite
make cppcheck # just cppcheck
None interfere with the normal make / make install workflow. If Docker
is not installed they fail with a clear error. Depends on #401 being
merged first.
---------
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Builds on #413 with three additional improvements to SNMPv3 error
handling.
**snmp_sess_error() retrieval before USM switch**
Without calling `snmp_sess_error()` the `sess_liberr` variable is always
`SNMPERR_SUCCESS`, so the USM switch introduced in #413 always falls
through to `default`. This call populates `sess_liberr` from the actual
session state before the switch runs.
**snmp_sess_close() mutex**
`snmp_sess_open()` acquires `LOCK_SNMP` but `snmp_host_cleanup()` calls
`snmp_sess_close()` unprotected. These operations share the net-snmp USM
engine cache. Wrap `snmp_sess_close()` with `LOCK_SNMP` to match.
**Portability guards for USM error constants**
The newer `SNMPERR_NOT_IN_TIME_WINDOW` and the older
`SNMPERR_USM_NOTINTIMEWINDOW` symbol sets map to different numeric
values. Outer `#if defined(A) || defined(B)` guards ensure both sets
compile on Net-SNMP 5.7+ and that neither case label appears when
neither symbol is defined (older versions).
Same USM decoding applied to `snmp_getnext()` which had the same
catch-all gap as `snmp_get_base()`.
Fixes: Cacti/cacti#6668
Ref: #413
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Co-authored-by: TheWitness <thewitness@cacti.net>
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>
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#415
Four resource leaks in snmp.c and poller.c:
1. snmp.c: `session.peername` not `strdup()`'d, dangling reference after
`snmp_sess_open()`
2. snmp.c: `securityAuthProto`/`securityPrivProto` OIDs leaked on 4
error paths and happy path
3. snmp.c: PDU leaked in `snmp_get_base()` and `snmp_getnext()` when
`snmp_parse_oid()` fails
4. poller.c: `sprintf` overflow in `exec_poll()`, switched to `snprintf`
All declarations at block top per project convention. Tested with gcc
and clang.
---------
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Co-authored-by: TheWitness <thewitness@cacti.net>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Retrieve the actual session errno via `snmp_sess_error()` after
`STAT_ERROR` and decode the six USM report types individually.
`notInTimeWindow` is treated as recoverable since net-snmp resolves it
via internal retry on the next poll.
Also wraps `snmp_sess_close()` in `LOCK_SNMP` to protect the shared USM
engine cache.
Fixes#412
Related: Cacti/cacti#6668
---------
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Co-authored-by: TheWitness <thewitness@cacti.net>
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>
## Summary
`buf_length` is initialized to 0 and never updated before four
`memset(query3, 0, buf_length)` calls in the reindex assert logic. So
these memsets clear exactly zero bytes — they're expensive no-ops.
`query3` is allocated with `malloc(LRG_BUFSIZE)` right before this code,
so the fix is straightforward: use `LRG_BUFSIZE` instead of `buf_length`
in those four calls.
The fifth memset at line 1777 is fine — `buf_length` gets its real value
assigned at line 1769, well before that call. Left it alone.
Not a security issue (the buffer gets overwritten by `snprintf` before
each use anyway), but it's the kind of bug that bites you the day
someone refactors the surrounding code and assumes the buffer is
actually zeroed.
## Test plan
- [x] Builds clean with `-Wall -Wextra` + ASan (verified via Docker dev
image)
- [x] cppcheck identical to develop baseline — zero new findings
- [x] `spine --version` exits 0
- [ ] Run against a host with data query reindex configured, confirm
reindex asserts work correctly
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Co-authored-by: TheWitness <thewitness@cacti.net>
## 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>
This pull request introduces a new blueprint generator for creating
comprehensive `copilot-instructions.md` files, aimed at guiding GitHub
Copilot to generate code that strictly follows project-specific
standards, architecture, and technology versions. The blueprint is
technology-agnostic and ensures that generated instructions are based
solely on actual patterns found in the codebase, avoiding any
assumptions.
Key additions and improvements:
**Blueprint Generator and Configuration:**
- Added `.github/instructions/instructions.md`, which provides a
detailed, configurable template for generating `copilot-instructions.md`
files. The template supports variables for project type, architecture
style, code quality focus, documentation level, testing requirements,
and versioning approach.
**Instruction Structure and Guidance:**
- Outlined a core instruction structure that enforces strict adherence
to detected technology versions, project architecture, and codebase
patterns, with explicit prioritization of context files and code quality
standards.
- Included technology-specific sections for .NET, Java,
JavaScript/TypeScript, React, Angular, and Python, each with guidelines
to match exact versions and code patterns found in the codebase.
**Codebase Analysis and Implementation:**
- Provided detailed steps for analyzing the codebase to extract
technology versions, architectural style, code patterns, and quality
standards, ensuring the generated instructions are fully aligned with
the existing code. (F7dd6
If sysDescr or sysUpTime return "NoSuchObject" then accept the return
and process normally, otherwise clear it and change status to an error.
The behavior changed due to the patch for #272, and I have not been able
to poll a few devices with spine newer than 1.2.21. I have tested this
change on 1.2.27 (my current version in production)
Co-authored-by: TheWitness <thewitness@cacti.net>
Thomas Vincent