Repository navigation
bug: Restore Cacti 1.2.23 compatibility for the 4.11 upgrade (array primary keys and process helpers) - #439
Merged
Merged
Conversation
cacti_process_still_running() and cacti_process_kill() were added to Cacti core (lib/poller.php) after the 1.2.31 release and only ship in 1.2.32+. The syslog plugin declares compat 1.2.23 but called both helpers unguarded in includes/functions.php (syslog_workers_running) and syslog_process.php (syslog_kill_workers). On every current Cacti release this produced a fatal 'Call to undefined function cacti_process_still_running()' from the poller, which disabled the plugin. Add function_exists()-guarded, posix_kill()-based fallbacks that are used only when core does not provide the helpers; on 1.2.32+ the native core implementations continue to be used. posix_kill() is always available because Cacti core defines a win32 shim for it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Critical PID validation gaps can misidentify unrelated processes as workers and allow oversized PIDs to trigger SIGTERM against every process the poller user can signal.
2 open issues
What changed in this PR
This PR adds compatibility fallbacks for Cacti process helpers to prevent Syslog poller failures on older supported Cacti releases.
Changes:
- Adds guarded,
posix_kill()-based fallbacks for process liveness checks and signaling. - Preserves existing core helpers when already defined.
- Records the compatibility fix in the changelog.
| File | Description |
|---|---|
includes/functions.php |
Adds fallback implementations for both process helpers. |
CHANGELOG.md |
Documents the older-release compatibility fix. |
Review effort: Balanced
Cacti core's db_table_create() mishandled an array primary key before the 1.2.32 release: a typo passed implode() the glue string concatenated with the array instead of the column list, so creating a table with an array primary fataled with "implode(): Argument #1 must be of type array, string given". The plugin declares compat 1.2.23 and passes array primaries to db_update_table() when creating its dashboard, status, saved-search and share tables during the 4.2 -> 4.11 upgrade, so the upgrade crashed on every current Cacti release. Route those calls through a new syslog_db_update_table() wrapper. When a table does not yet exist on a Cacti older than 1.2.32 it rewrites the array primary to the back-tick joined string form the create path accepts, dodging the core typo. The array form is preserved when the table already exists (db_update_table() reconciles the primary with array_diff(), which requires an array) and on Cacti 1.2.32+ where the create path is fixed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
TheWitness
force-pushed
the
fix/process-helper-compat
branch
from
November 11, 2026 15:17
e13b65a to
e38a0bc
Compare
TheWitness
changed the title
2 of 6 tasks
…rsized PIDs Address review feedback on the compatibility shims for Cacti releases older than 1.2.32: - cacti_process_still_running() now confirms command identity through procfs (cmdline) and only falls back to a bare signal-0 liveness probe when that identity cannot be determined, so a dead worker's PID that has been reused by an unrelated process is no longer reported as a live worker. - Both fallbacks validate the PID against the platform pid_t range before calling posix_kill(). An oversized processes.pid value (e.g. 4294967295 on a 64-bit column) would otherwise narrow to pid_t -1 and make SIGTERM target every process the poller user can signal. The validation and identity logic mirror Cacti core's cacti_process_pid_is_valid() and cacti_process_identity_matches(), reproduced here because those helpers are also absent before 1.2.32. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…-compat # Conflicts: # CHANGELOG.md
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
The patch-coverage gate requires 100 percent coverage of changed lines in includes/functions.php, but the process-helper polyfills had uncovered branches: the non-scalar PID rejection, the procfs command-identity match, and cacti_process_kill(). The identity check read /proc directly and compared against the test runner cmdline, so its hash_equals() and parser branches could never be exercised deterministically in the isolated Pest process. Extract the cmdline parser into syslog_compat_script_from_cmdline() and give syslog_compat_pid_identity_matches() an optional injectable reader (defaulting to the real /proc read) so tests drive every branch with synthetic cmdlines. Expand ProcessHelperCompatTest to cover the non-scalar PID path, the parser, the identity match/mismatch/unresolvable cases, and cacti_process_kill(). Production behavior is unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The earlier fix for the undefined syslog_db_update_table() in the CLI schema regression test pulled the real wrapper in with a third eval(). SonarCloud rule S1523 flags dynamic code execution, so the new eval() raised a high severity security alert on the PR and failed the Quality Gate. includes/schema.php is allowlisted from the patch-coverage gate and the array-primary wrapper has dedicated coverage in SchemaPrimaryKeyCompatTest, so the regression test does not need to execute the real wrapper. Define a forwarding stub that delegates to the already-stubbed db_update_table(), matching the other stubs in this file, which keeps syslog_setup_table_new() runnable without adding another eval(). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
xmacan
approved these changes
Nov 11, 2026
2 of 6 tasks
This was referenced Nov 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Upgrading Syslog from 4.2 to 4.11 on a current Cacti release (reported on 1.2.30 / PHP 8.3) crashes partway through and then disables the plugin. Two independent spots in 4.11 assume a Cacti core that is newer than the plugin's declared
compat = 1.2.23. This PR restores the 1.2.23 floor.1. Table creation fataled with
implode(): Argument #1 must be array, string givenCacti core's
db_table_create()mishandled an array primary key before 1.2.32: a typo passedimplode()the glue string concatenated with the array (implode('+ "," +' . $data['primary'])) instead of the column list. Creating a table with an array primary therefore fataled. The plugin passes array primaries todb_update_table()when creating its dashboard, status, saved-search and share tables during the upgrade, so the upgrade crashed on every current release (the bug is present in 1.2.30 and 1.2.31; Cacti fixed it for 1.2.32).Fix: route those calls through a new
syslog_db_update_table()wrapper. When a table does not yet exist on a Cacti older than 1.2.32, it rewrites the array primary to the back-tick-joined string form the create path accepts, dodging the typo. The array form is preserved when the table already exists (db_update_table()reconciles the primary witharray_diff(), which requires an array) and on Cacti 1.2.32+ where the create path is fixed. A version/existence latch, not a blanket string conversion, is required because the existing-table path would otherwisearray_diff()a string and fatal.2. Poller fataled with
Call to undefined function cacti_process_still_running()cacti_process_still_running()andcacti_process_kill()live in Cacti corelib/poller.php, but were added after the 1.2.31 release (they ship in 1.2.32+). The plugin called both unguarded inincludes/functions.php(syslog_workers_running) andsyslog_process.php(syslog_kill_workers), so the poller fataled and Cacti disabled thesyslogplugin.Fix: add
function_exists()-guarded,posix_kill()-based fallbacks used only when core does not provide the helpers; on 1.2.32+ the native implementations are used.posix_kill()is always available because Cacti core defines a win32 shim for it.Related Issue
Reported on the Cacti forums: https://forums.cacti.net/viewtopic.php?t=64912 (upgrade Syslog 4.2 → 4.11 on Cacti 1.2.30 / PHP 8.3). Both fatals in that thread — the
implode()table-creation crash and thecacti_process_still_running()crash — are addressed here.Motivation and Context
Without this change, Syslog 4.11 cannot be upgraded onto or run its poller on any current Cacti release (1.2.23 – 1.2.31), even though the plugin advertises compatibility back to 1.2.23.
How Has This Been Tested?
php -lclean onincludes/functions.php,includes/schema.php, and the new test (PHP 8.3.36).plugins/syslog:php include/vendor/bin/pest --configuration plugins/syslog/phpunit.xml→ 635 passed (2658 assertions).
tests/Unit/SchemaPrimaryKeyCompatTest.phpcovers the version/existence latch: string primary when creating on < 1.2.32, array primary when the table exists, array primary on 1.2.32+, and back-tick-joined composite keys on older Cacti.Unit/SyslogWorkerWaitTestexercises thecacti_process_still_running()path.Testing environment: AlmaLinux 9, PHP 8.3.36, Cacti
1.2.xvendor tree.Types of changes
Checklist:
Fixes #440
Fixes #441