Skip to content

fix: memory-safety and correctness backports for 1.2.x - #574

Open
somethingwithproof wants to merge 10 commits into
Cacti:1.2.xfrom
somethingwithproof:fix/memory-safety-1.2.x
Open

fix: memory-safety and correctness backports for 1.2.x#574
somethingwithproof wants to merge 10 commits into
Cacti:1.2.xfrom
somethingwithproof:fix/memory-safety-1.2.x

Conversation

@somethingwithproof

Copy link
Copy Markdown
Member

Four defects that are fixed on develop and still present on the released 1.2.31. Grouped into one pull request because they are one review decision: small, independent backports to the maintenance branch, each its own commit.

issue#447 — out-of-bounds write in strncopy()

len clamped to obuf when the source filled the destination, then dst[obuf] = \0 wrote one byte past a buffer of exactly that size. Demonstrated against a canary either side of an 8-byte destination:

before:  dst="ABCDEFGH" len=8   byte past dst 0xAA -> 0x00   CLOBBERED
after:   dst="ABCDEFG"  len=7   byte past dst 0xAA           INTACT

The #pragma GCC diagnostic ignored that hid the compiler warning about this is removed.

issue#561 — heap overflow in php_readpipe()

read() was given the whole remaining buffer, so a script server result of exactly RESULTS_BUFFER bytes filled it and the terminator landed one past the end. Capacity now reserves the terminator, and the loop stops with the existing "longer than the acceptable range" error when nothing is left.

issue#562 — script server orphaned on shutdown

php_close() sent SIGTERM and moved on. A script server that ignores it, or is stuck in uninterruptible I/O, was never reaped. Shutdown now polls with WNOHANG over a bounded window, escalates to SIGKILL, polls again, and logs if the child still has not exited.

issue#573 — get_namebyhost() copies nothing

memset(stack, \0, strlen(hostname)+1);
strncopy(stack, hostname, strlen(stack));   /* stack was just zeroed */

strlen(stack) is 0, so nothing is copied, strtok() sees an empty string, and every branch that parses a transport prefix or port is unreachable. A device configured as tcp:host:port is treated as a literal hostname. Also moved to strtok_r, since this runs on every poller thread.

One supporting change. rdb_ssl_key, rdb_ssl_cert and rdb_ssl_ca were BIG_BUFSIZE while the config parser reads at most 255 characters into a BUFSIZE scratch buffer, so the corrected strncopy bound legitimately exceeded the source and the compiler said so. develop already carries these three at BUFSIZE; this matches.

Verification. Clean ubuntu:24.04 container: builds at 0 warnings, matching the 1.2.x baseline exactly.

Not backported: the ICMP reply classifier and the async-signal-safe fatal handler. Both are larger and entangled with refactors that are develop-only, and neither is as clearly exploitable as the two overflows above. Worth a separate look if wanted.

…the buffer

When strlen(src) was at least obuf the length clamped to obuf and the
terminator went to dst[obuf], one past a buffer of exactly that size. The
pragma that suppressed the compiler's warning about it is no longer needed.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
read() was handed the whole remaining buffer, so a script server result of
exactly RESULTS_BUFFER bytes filled it and the terminator went one past the
end. The loop also had no guard for the case where no capacity was left.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
…st()

The copy used strlen(stack) as its size, and stack had just been zeroed, so
nothing was copied and strtok() saw an empty string. Every branch that parses
a transport prefix or port was unreachable as a result. strtok() also keeps one
process-wide save pointer while this runs on each poller thread.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
php_close() sent SIGTERM and moved on, so a script server that ignores it or
is stuck in uninterruptible I/O was left as an orphan. Shutdown now polls for
the child over a bounded window, escalates to SIGKILL, and polls again.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
rdb_ssl_key, rdb_ssl_cert and rdb_ssl_ca were BIG_BUFSIZE while the config
parser reads at most 255 characters into a BUFSIZE scratch buffer, so the
copy bound exceeded the source and the compiler said so. develop already
carries these at BUFSIZE.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
TheWitness
TheWitness previously approved these changes Aug 31, 2026
php_processes, debug_devices and both connection pools were dereferenced on
the next statement. Backport of the develop fix for issue#564.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Four settings helpers returned from the NULL-row branch without freeing while
every other exit frees. Backport of the develop fix for issue#566.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
strcat() wrote the newline at LOGSIZE-1 and its terminator one past the end
once the message filled the buffer. Backport of the develop fix for
issue#565, with the changelog entries for this batch.

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

Copy link
Copy Markdown
Member Author

Rolled in the 1.2.x half of three more bugs that are present on both branches, so this is now seven fixes rather than four. Still 0 warnings, matching the 1.2.x baseline.

  • issue#564: the four unchecked calloc() results in spine.c, dereferenced on the next statement
  • issue#565: strcat() wrote the log newline one byte past flogmessage once the message filled LOGSIZE
  • issue#566: MYSQL_RES leaked on the no-row branch of four settings helpers

The develop half of the same three is #576.

@somethingwithproof
somethingwithproof requested review from TheWitness and a lite review from Copilot and removed request for cigamit and netniV September 4, 2026 06:45

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

There are remaining correctness/portability issues in php.c (inconsistent BUFSIZE vs RESULTS_BUFFER bounds check and an unguarded usleep() relative to existing SOLAR_THREAD handling) that should be resolved before merging.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR backports several small, independent fixes from develop to the 1.2.x maintenance branch, primarily addressing memory-safety issues (off-by-one/overflow) and shutdown correctness in the PHP script-server path.

Changes:

  • Harden string/buffer handling (strncopy(), php_readpipe(), and log newline appending) to prevent out-of-bounds writes.
  • Improve correctness and thread-safety in hostname parsing (get_namebyhost() uses proper copy + strtok_r).
  • Ensure resources/processes are cleaned up (free MySQL result sets on empty fetch paths; reap script-server children on shutdown).
File summaries
File Description
util.c Fix result-set leaks in settings helpers; make spine_log() newline append bounded; fix strncopy() off-by-one/OOB behavior and remove warning-suppression pragmas.
spine.h Reduce remote DB SSL path buffers to BUFSIZE to match config parsing limits and avoid misleading copy bounds.
spine.c Add fatal checks for several calloc() allocations before dereference.
ping.c Fix hostname copying so parsing is reachable; switch to strtok_r for reentrant tokenization.
php.c Reserve NUL terminator space in php_readpipe(); add bounded shutdown/reap logic for script-server processes.
CHANGELOG Document the backported fixes for 1.2.32.
Review details
  • Files reviewed: 6/6 changed files
  • Comments generated: 2
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread php.c
Comment thread php.c
@somethingwithproof

Copy link
Copy Markdown
Member Author

@copilot Fix the code for all comments in this review thread.

When a review comment includes a suggested change, apply the suggestion exactly.

Do not make changes beyond what is described in the linked review thread.

RESULTS_BUFFER defaults to 2048 and BUFSIZE is 1024, so the second guard
rejected any reply over 1024 bytes that the read loop had already accepted,
and it did not break, so the next read overwrote the buffer it had just
declared out of range. The loop bound above it is the real limit.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Every other usleep() in php.c sits inside #ifndef SOLAR_THREAD.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants