Skip to content

fix: NULL checks, switch fall-through, thread safety, buffer bounds#520

Closed
somethingwithproof wants to merge 2 commits intoCacti:developfrom
somethingwithproof:fix/high-null-checks-and-thread-safety
Closed

fix: NULL checks, switch fall-through, thread safety, buffer bounds#520
somethingwithproof wants to merge 2 commits intoCacti:developfrom
somethingwithproof:fix/high-null-checks-and-thread-safety

Conversation

@somethingwithproof
Copy link
Copy Markdown
Contributor

Summary

Fixes all 10 HIGH-severity findings from static analysis / code review.

Findings Addressed

HIGH-1: Missing NULL check after calloc for poller_items (poller.c)
Added die() guard after calloc at the poller_items allocation.

HIGH-2: Missing NULL check after calloc for snmp_oids (poller.c)
Added die() guard after calloc at the snmp_oids allocation.

HIGH-3: Missing NULL check after malloc for error_string, buf_size, buf_errors (poller.c)
Added combined NULL check for all three malloc calls before dereferencing.

HIGH-4: Switch fall-through in get_date_format (util.c)
Added break; after each case body. Every case was falling through to the next, so only the default format was ever used.

HIGH-5: db_get_connection NULL return not checked (poller.c)
Added NULL checks with die() for both local and remote database connections.

HIGH-6: db_escape output buffer overflow risk (sql.c)
Input is now always capped to (trim_limit / 2) - 1 before passing to mysql_real_escape_string, regardless of whether the escaped size exceeds max_size.

HIGH-7: db_column_exists column name unescaped (sql.c)
Added character validation loop: only alphanumeric and underscore characters are accepted. Invalid column names log an error and return FALSE.

HIGH-8: Thread-unsafe static seq in ping_icmp (ping.c)
Replaced mutex-protected seq++ with __sync_fetch_and_add(&seq, 1) (GCC atomic built-in). Variable declared volatile for visibility.

HIGH-9: Unbounded sprintf in poller_push_data_to_main (util.c)
Replaced all sprintf calls with snprintf bounded by remaining space in the HUGE_BUFSIZE buffer.

HIGH-10: snprintf uses wrong buffer size constant (poller.c)
Changed snprintf(poll_result, BUFSIZE, ...) to snprintf(poll_result, RESULTS_BUFFER, ...) at both locations to match the actual malloc(RESULTS_BUFFER) allocation from snmp_get.

Verification

  • Build: make clean && make -> 0 errors, 0 warnings
  • No new dependencies or API changes

Test plan

  • Verify build on Linux (CI)
  • Run polling cycle with SNMP devices to confirm no regressions
  • Confirm date format logging uses correct format per setting

…RITICAL)

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
@somethingwithproof somethingwithproof force-pushed the fix/high-null-checks-and-thread-safety branch from 9aee4f4 to 4f2f753 Compare March 26, 2026 04:54
…HIGH)

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
@somethingwithproof
Copy link
Copy Markdown
Contributor Author

Consolidated into mega PR #522 for independent mergeability.

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.

1 participant