Skip to content

fix: comprehensive security hardening and code quality improvements#522

Closed
somethingwithproof wants to merge 4 commits intoCacti:developfrom
somethingwithproof:fix/comprehensive-hardening
Closed

fix: comprehensive security hardening and code quality improvements#522
somethingwithproof wants to merge 4 commits intoCacti:developfrom
somethingwithproof:fix/comprehensive-hardening

Conversation

@somethingwithproof
Copy link
Copy Markdown
Contributor

Summary

Mega PR consolidating all security review findings and code quality improvements. Depends on #519 being merged first.

Replaces stacked PRs: #520, #521, #518, #511

Security fixes (17 findings addressed)

  • NULL checks after all malloc/calloc to prevent NULL dereference
  • SNMPv3 credential zeroing with volatile pointers (prevents core dump exposure)
  • DB password zeroing at exit + disable core dumps on Linux
  • select(fd+1) with FD_SETSIZE bounds check (prevents stack corruption)
  • add_slashes buffer overflow fix
  • Pre-opened ICMP socket eliminates seteuid(0) race condition across threads
  • Column count validation before row[index] access
  • Empty command validation before exec_poll
  • atoi->strtol across all 55 DB/config parsing sites
  • Memory leak fix in db_connect socket path
  • switch fall-through fix in get_date_format

Code quality

  • GCC function attributes on 30+ declarations (found 3 memory leaks)
  • sql_buffer_t for safe dynamic SQL construction
  • db_escape hardening, db_fetch_cell_dup utility
  • Exponential backoff with jitter for retries
  • Centralized logging helpers
  • Unit tests for sql_buffer, allocation macros, db_escape, jitter_sleep

Independence

PR Status Can merge independently
#519 CRITICAL security Yes (merge first)
#517 Semaphore wrapper Yes
#512 CI pipeline Yes
This PR Everything else After #519

Test results

68/68 pass (16 unit, 21 smoke, 15 output_regex, 16 db_column_detect)

…RITICAL)

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Consolidates fixes from multiple security review findings:

Security (depends on Cacti#519 being merged first):
- NULL checks after all malloc/calloc (poller.c, util.c)
- SNMPv3 credential zeroing with volatile pointers (snmp.c)
- DB password zeroing at exit, disable core dumps (spine.c)
- select(fd+1) instead of select(FD_SETSIZE) with bounds check
- add_slashes buffer overflow fix (malloc length*2+1)
- strcat->snprintf for auth/priv capability strings
- Log newline bounds check, config truncation warning
- Memory leak fix in db_connect socket path (sql.c)
- Pre-opened ICMP socket eliminates seteuid race (ping.c)
- Column count validation before row[index] access
- Empty command validation before exec_poll
- atoi->strtol across all DB/config parsing (55 sites)

Code quality:
- GCC function attributes (format, noreturn, warn_unused, nonnull,
  pure, cold) on 30+ declarations across 8 headers
- 3 memory leaks found and fixed by warn_unused_result
- switch fall-through fix in get_date_format
- sql_buffer_t for safe dynamic SQL construction
- db_escape hardening, db_fetch_cell_dup utility
- Exponential backoff with jitter for retries
- Centralized logging (log_invalid_response, SPINE_LOG_DEV)
- Unit tests for sql_buffer, allocation macros, db_escape,
  debug_device, jitter_sleep, log_invalid_response

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
@TheWitness
Copy link
Copy Markdown
Member

First you have merge conflicts again. Second you are issuing a warning for every host. It's not even a warning. So, we should not be issuing a warning for missing column for every host, it's just an artifact and does not even need to be logged. Spine should proceed as if nothing happened. At a level like 3, you can issue one warning as startup and not for every host. But at low verbosity, no warning necessary. Lastly, you should do a valgrind. Leaks...

image

Copy link
Copy Markdown
Member

@TheWitness TheWitness left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comments about merge conflicts (still), valgrind and excessive logging. I'll have more comments as I go through this.

Copy link
Copy Markdown
Member

@TheWitness TheWitness left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like you to separate the workflow changes from the code changes. I'm not completely onboard with going kitchen sink on all these workflows.

@TheWitness
Copy link
Copy Markdown
Member

This is kind of turning into a train wreck. This change backs out the semaphore changes for mac. Going way too fast, and shit is flying off the shelf.

- ping.c: use dup()'d icmp_fd for select/FD_SET and FD_SETSIZE guard
- spine.c: validate --hostlist input and store in set.host_id_list
- util.c: remove legacy sprintf/snprintf batch builders in favor of sql_buffer batching and flush_sql_batch(), keep CLI override path in getsetting()

Co-Authored-By: Oz <oz-agent@warp.dev>
Copilot AI review requested due to automatic review settings March 27, 2026 06:21
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR consolidates a broad set of security hardening and code-quality improvements across Spine’s core poller/runtime (SNMP, DB, ping, polling pipeline) and its CI/testing automation.

Changes:

  • Introduces/extends compile-time hardening via GCC/Clang attribute macros and safer allocation/SQL-building helpers.
  • Hardens runtime behavior (ICMP socket handling, retry backoff with jitter, safer DB escaping/identifier validation, credential zeroing).
  • Expands CI workflows for static analysis, security posture, coverage, performance regression, and nightly/weekly deep checks; adds multiple unit/integration tests.

Reviewed changes

Copilot reviewed 43 out of 44 changed files in this pull request and generated no comments.

Show a summary per file
File Description
util.h Adds attribute annotations, fail-fast allocation macros, and new helper prototypes (jitter sleep, debug device tracking, invalid-response logging).
sql.h Adds attributes to DB APIs; introduces sql_buffer_t and db_fetch_cell_dup().
sql.c Adds jittered backoff on retries; hardens db_escape, NULL-safe db_free_result, identifier validation in db_column_exists, implements sql_buffer_* and db_fetch_cell_dup.
spine.h Adds SPINE_ATTR_* macros; adds SPINE_LOG_DEV; exports icmp_socket.
spine.c Applies allocation macros and strtol; disables core dumps on Linux; pre-opens ICMP raw socket before threading; updates hostlist validation.
snmp.h Adds attributes to SNMP APIs; adds SPINE_SNMP_FREE helper macro.
snmp.c Switches key allocations to fail-fast macros; adds sensitive-data zeroing; fixes calloc NULL-handling.
poller.h Adds attributes to poller APIs.
poller.c Uses allocation macros and centralized logging; adds output-regex post-processing and schema detection; fixes DB query result freeing; adds fd bounds checks and correct select() nfds usage in exec_poll.
ping.c Removes per-thread privilege toggling by using pre-opened ICMP socket; uses atomics for sequence; adds FD_SETSIZE checks and correct select() nfds usage.
php.h / php.c Adds attributes; uses STRDUP_OR_DIE; applies jittered backoff on retry.
keywords.h Marks parsers/printers as pure via attributes.
tests/unit/* Adds new unit tests for allocation macros, db_escape behavior (mocked), jitter sleep, debug-device hashing, log_invalid_response, and sql_buffer.
tests/integration/smoke_test.sh Adjusts host_errors handling to be informational in some cases.
.gitignore Expands ignored build artifacts and local/IDE patterns.
.github/workflows/* Adds/expands CI, static analysis, security, integration, fuzzing, coverage, nightly/weekly, perf regression, and release verification workflows.
.github/scripts/* Adds SARIF converters, workflow policy enforcement, unsafe API guardrail, and leak trend gates.
.github/*baseline* Adds warning/perf/leak baselines for gates.
.github/instructions/instructions.md Updates contributor guidance for safety/CI conventions.
.codespell-ignore-words.txt Adds ignore word list.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…ally

- spine.c: remove stray brace after --hostlist arg parsing
- util.c: remove leftover conflict lines; batch-flush logic intact

Tested: local make; docker compose smoke_test.sh completed OK

Co-Authored-By: Oz <oz-agent@warp.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants