fix(correctness): loop-state leaks, chunk-aware poller CRC, header-suppression and tree false-guards (1.2.x)#7151
Merged
TheWitness merged 2 commits intoMay 16, 2026
Conversation
…ppression and tree false-guards (1.2.x) The realtime poller foreach in cmd_realtime.php at line 174 left $output addressed by a prior iteration when the current $item['action'] hit a switch branch that does not assign $output (the default branch logs but does not set it). The next poller_output_realtime insert for the new local_data_id then carried the previous iteration's value. The body now unsets $output at the top of every iteration so each action runs against a clean local. The reindex foreach in cmd.php at line 816 had the same defect: the assert comparison further down compared the new index against the previous iteration's $output, producing spurious assert pass/fail verdicts on default-branch actions. The reset now happens immediately after $assert_fail = false and before the action switch. remote_agent.php's get_snmp_data and get_snmp_data_walk assigned $output only inside the if (!empty(\$host_id)) branch and then printed it or fed it to cacti_sizeof() unconditionally. An empty host_id left $output undefined; a missing host row inside the branch reached cacti_snmp_session with NULL fields. Both functions now initialise $output before the host lookup (empty string and empty array respectively) and bail with the standard 'U' marker when the host row is missing. api_data_source_disable_multi() in lib/api_data_source.php reassigned $poller_ids on every 1000-id chunk, so the trailing CRC update only saw the pollers attached to the last chunk. Pollers that owned only items in earlier chunks kept stale poller_output_boost rows and their CRCs never advanced; remote pollers continued to push data for already disabled local_data_ids until a manual cache rebuild. An $all_poller_ids accumulator built with the array union (+) preserves keys across chunks so the CRC update covers every poller the batch touched. appendHeaderSuppression() in lib/functions.php compared strpos($url, 'header=false') against < 0. strpos returns false on no-match and a non-negative int on a hit; false < 0 is also false, so the guard never fired and repeated calls accumulated &header=false&header=false&header=false on the URL. Replaced with the canonical === false idiom. tree.php's get_host_sort_type and get_branch_sort_type compared db_fetch_cell_prepared() against integer constants and a switch. db_fetch_cell_prepared() returns false on a missing row; PHP's loose == and the switch statement both match false to 0, so the AJAX endpoint emitted hsgt / inherit for branches that no longer exist. Both call sites now bail with the empty-string response the front-end expects when the lookup returns false. Installer::getModules() in lib/installer.php cached its result in \$this->extensions but guarded with isset(\$this->extensions) || empty(...) which is always true on the first call (empty is true) and always true on every subsequent call (isset is now true), so utility_php_extensions() ran on every request. Inverted the isset check so the cache is rebuilt only when not yet computed or empty. month_shift() in lib/time.php detected month-granular shifts with strpos(...) > 0. A shift_size of 'month' or 'months' has 'month' at offset 0; strpos returns int(0) which is not > 0, so the helper said "no, this is not a month shift" for the most common inputs and reports that relied on month boundary checks silently slid into day-shift mode. Replaced with !== false. Regression tests in tests/Unit/ pin each fixed shape against future drift; the chunk-accumulator test also exercises the bug behaviourally against a 3-chunk fixture. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses several low-frequency correctness defects across Cacti 1.2.x poller/agent code paths and a few small logic guards in core helpers, with accompanying Pest unit/regression tests and updated security baselines.
Changes:
- Prevent per-iteration
$outputleakage in realtime polling and reindex loops; harden remote agent SNMP helpers against missing/invalid host rows. - Fix one-character logic bugs (
strposoffset-zero handling, false-vs-int comparisons, installer module-cache guard). - Make
api_data_source_disable_multi()chunk-aware when accumulating poller IDs for CRC updates; add targeted Pest regression tests and refresh security baselines.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tree.php | Guard AJAX sort-type endpoints against db_fetch_cell_prepared() returning false for missing branches. |
| remote_agent.php | Initialize $output in SNMP helpers and bail out with 'U' if the host row is missing. |
| lib/time.php | Fix month_shift() to handle strpos() returning 0 (offset-zero) via !== false. |
| lib/installer.php | Correct Installer::getModules() cache guard so extensions are not recomputed every call. |
| lib/functions.php | Fix appendHeaderSuppression() idempotency by using strpos(...) === false. |
| lib/api_data_source.php | Accumulate poller IDs across chunks so CRC updates cover all touched pollers. |
| cmd.php | Reset per-iteration $output in reindex loop to prevent leaking state between iterations. |
| cmd_realtime.php | Reset per-iteration $output in realtime poller loop to prevent leaking state between items. |
| tests/Unit/TreeSortFalseGuardTest.php | Source-pattern regression coverage for the tree sort-type false-guard. |
| tests/Unit/RemoteAgentOutputInitTest.php | Source-pattern regression coverage for remote agent $output initialization and missing-host bailout. |
| tests/Unit/MonthShiftPositionZeroTest.php | Regression coverage for month_shift() offset-zero strpos() fix. |
| tests/Unit/InstallerExtensionsCacheTest.php | Regression coverage for the installer extensions cache guard. |
| tests/Unit/CmdReindexLoopStateTest.php | Source-pattern regression coverage for $output reset within cmd.php reindex loop. |
| tests/Unit/CmdRealtimeLoopStateTest.php | Source-pattern regression coverage for $output reset within cmd_realtime.php loop. |
| tests/Unit/AppendHeaderSuppressionTest.php | Regression coverage for appendHeaderSuppression() idempotency fix. |
| tests/Unit/ApiDataSourceDisableMultiChunkTest.php | Regression coverage for chunk accumulator/CRC update behavior in api_data_source_disable_multi(). |
| tests/security/baselines/sink_inventory.baseline.tsv | Baseline refresh to account for line number drift. |
| tests/security/baselines/architectural_hotspots.baseline.tsv | Baseline refresh to account for line number drift. |
…er cache test cmd.php: replace unset($output) with $output = '' in the reindex foreach reset so the post-switch trim($output) is safe under PHP 8 when the default branch fires (unknown action only logs, no $output assignment). cmd_realtime.php is intentionally left with unset() because its post-switch is guarded by isset(). InstallerExtensionsCacheTest: drop the tautological strpos comparison (which matched both the buggy and fixed forms) in favour of an explicit not->toContain check for the buggy "if (isset(...) || empty(...))" shape, which cannot collide with the fixed "if (!isset(...". Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Member
|
Get these comments addressed @somethingwithproof . |
TheWitness
approved these changes
May 16, 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.
Summary
This branch fixes eight independent correctness defects in 1.2.x that have low-frequency but observable impact on operators. Two are loop-state leaks where
$outputcarried across iterations when the per-action switch incmd_realtime.php:174(foreach($poller_items as $item)) andcmd.php:816(foreach ($reindex as $index_item)) hit a branch that did not assign it, so realtime poller_output rows and reindex assert comparisons used the previous iteration's value. Two more are missing initialisations inremote_agent.php:333(get_snmp_data) andremote_agent.php:351(get_snmp_data_walk) where$outputwas only set inside the!empty($host_id)branch but printed unconditionally, plus no bailout for a missing host row. The chunk path inlib/api_data_source.php:379(api_data_source_disable_multi) reassigned$poller_idsevery 1000 ids so the trailing CRC update only touched the last chunk's pollers; pollers seen only in earlier chunks kept stale boost rows. The remaining four are one-character logic bugs:lib/functions.php:4863(appendHeaderSuppression) usedstrpos(...) < 0,tree.php:319andtree.php:388(host/branch sort type) compareddb_fetch_cell_prepared()'s false return against integer constants via==andswitch,lib/installer.php:1088(getModules) had an always-true cache guard, andlib/time.php:159(month_shift) usedstrpos(...) > 0and so missed'month'and'months'at offset zero.Each fix carries a Pest source-pattern regression test in
tests/Unit/; the chunk accumulator test also exercises the bug behaviourally against a three-chunk fixture. Security baselines were regenerated to absorb the line-number drift inremote_agent.php; bothverify_sink_inventory.shandverify_architectural_hotspots.shexit 0 and no new sinks were introduced.Test plan
php -lclean on all eight touched source filestests/vendor/bin/pest tests/Unit/{CmdRealtimeLoopState,CmdReindexLoopState,RemoteAgentOutputInit,ApiDataSourceDisableMultiChunk,AppendHeaderSuppression,TreeSortFalseGuard,InstallerExtensionsCache,MonthShiftPositionZero}Test.php-- 21 passed, 70 assertionsbash tests/security/verify_sink_inventory.shexits 0bash tests/security/verify_architectural_hotspots.shexits 0