Skip to content

fix: allocation checks, a log buffer overflow, result leaks, and a per-line rebuild - #578

Closed
somethingwithproof wants to merge 6 commits into
Cacti:developfrom
somethingwithproof:fix/spine-alloc-and-log-hardening
Closed

fix: allocation checks, a log buffer overflow, result leaks, and a per-line rebuild#578
somethingwithproof wants to merge 6 commits into
Cacti:developfrom
somethingwithproof:fix/spine-alloc-and-log-hardening

Conversation

@somethingwithproof

Copy link
Copy Markdown
Member

Four small fixes in spine.c and util.c, all found while auditing classes that #542 and #557 fixed elsewhere in the tree but did not finish.

Based on current develop, after #569 and #570.

#564php_processes, debug_devices, db_pool_local and db_pool_remote were dereferenced on the line after calloc() without testing the result. #542 gave the two in poller.c a die(); these were missed. Same treatment, same message shape. snmp.c:1028 and util.c:124 already checked theirs.

#565spine_log() bounds both strncat() calls so the message may fill flogmessage exactly, then appended the newline unconditionally, putting the terminator one byte past a LOGSIZE stack buffer. Same class as issue#561. The append is now bounded.

#566 — four settings helpers returned without db_free_result() when the query reported rows but the fetch yielded NULL. Every other exit in each function frees it.

#567get_date_format() did a malloc/free pair, two range checks and two switches on every log line to rebuild a string that only depends on two settings read once at config load. It is now built in read_config_options(), while spine is still single threaded, and get_date_format() returns that storage. spine_log() no longer frees it. No behaviour change; the format for every code is unchanged, which the existing tests in test_safety_fixes.c already assert.

Verification. Clean ubuntu:24.04 container: builds at 4 warnings, matching develop, and make check passes all four suites. The new test is wired into the check_PROGRAMS list #570 added.

tests/unit/test_log_newline_bound.c is new and pins #565 down. It fails against the old strcat(): built with ASan and the pre-fix predicate, test_full_buffer_is_left_alone reports a stack-buffer-overflow; with the fix all five pass.

Two things deliberately not here. All fourteen mysql_fetch_row() call sites are already guarded, by the loop condition or an explicit test, so there was nothing to fix beyond the leak in #566. And the eight strcat() calls at util.c:885-914 append compile-time literals into BUFSIZE buffers that were just zeroed, well under 100 bytes total; they are safe by construction and left alone.

TheWitness
TheWitness previously approved these changes Sep 1, 2026
@TheWitness

Copy link
Copy Markdown
Member

Merge conflicts.

php_processes, debug_devices and the two connection pools were dereferenced
on the next line without testing the allocation.

Closes Cacti#564

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
The two strncat() calls above it may fill flogmessage exactly, so the
unconditional strcat() put the terminator one byte past a LOGSIZE buffer.

Closes Cacti#565

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Closes Cacti#566

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
The format depends only on two settings read in read_config_options(), so
rebuilding it per log line cost a malloc/free pair and two switches.

Closes Cacti#567

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Adds a suite that includes util.c the way test_util_strings.c does, so the
date format caching, the settings-helper result release and the bounded
newline are exercised as shipped rather than as copies, plus an LD_PRELOAD
allocation-failure test for the two startup guards no unit test can reach.

test_linked freed the result of get_date_format() and expected that call to
clamp; both moved to set_date_format() with this change, so its assertions
move with them.

Changed-line coverage on this branch goes from 38% to 79%; the remainder
needs a live database and is noted in the pull request.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
@somethingwithproof

Copy link
Copy Markdown
Member Author

Conflicts are cleared. GitHub reports this MERGEABLE against current develop, and #576 is closed as a duplicate of this one, so the overlap on spine.c and util.c is gone.

@somethingwithproof

Copy link
Copy Markdown
Member Author

Consolidated into #597, which carries this branch's commits unchanged.

Every pair of these ten conflicted on tests/unit/test_linked.c because each appended to the same registration array, so whichever merged first would have forced a rebase on the other nine. #597 is one review and one approval for the same 20 commits, still one logical change each.

Nothing here is dropped. Reopen this if you would rather review it separately.

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.

2 participants