Skip to content

Commit ad8f835

Browse files
fix(correctness): loop-state leaks, chunk-aware poller CRC, header-suppression 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>
1 parent 3338a63 commit ad8f835

18 files changed

Lines changed: 531 additions & 11 deletions

cmd.php

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -815,6 +815,12 @@ function ping_and_reindex_check(&$item, $mibs) {
815815

816816
foreach ($reindex as $index_item) {
817817
$assert_fail = false;
818+
/* Reset between iterations: a reindex action that
819+
* is not handled by the switch below (default branch
820+
* logs but does not set $output) must not inherit the
821+
* previous iteration's $output and feed the wrong value
822+
* into the assert comparison further down. */
823+
unset($output);
818824

819825
switch ($index_item['action']) {
820826
case POLLER_ACTION_SNMP:

cmd_realtime.php

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -172,6 +172,12 @@
172172

173173
if (cacti_sizeof($poller_items)) {
174174
foreach($poller_items as $item) {
175+
/* Reset between iterations: a poller_item action that
176+
* is not handled by the switch below must not inherit
177+
* the previous iteration's $output and write it into
178+
* poller_output_realtime under the wrong local_data_id. */
179+
unset($output);
180+
175181
switch ($item['action']) {
176182
case POLLER_ACTION_SNMP: /* snmp */
177183
if (($item['snmp_version'] == 0) || (($item['snmp_community'] == '') && ($item['snmp_version'] != 3))) {

lib/api_data_source.php

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -374,6 +374,12 @@ function api_data_source_disable_multi($local_data_ids) {
374374
$ids_to_disable = '';
375375
$i = 0;
376376

377+
/* Accumulate poller_ids across every chunk so the trailing CRC update
378+
* below covers every poller this batch ever touched, not only the
379+
* pollers seen in the last chunk. The `+` union preserves keys and
380+
* dedupes when array_rekey hands back poller_id => poller_id pairs. */
381+
$all_poller_ids = array();
382+
377383
/* build the array */
378384
if (cacti_sizeof($local_data_ids)) {
379385
foreach ($local_data_ids as $local_data_id) {
@@ -390,6 +396,8 @@ function api_data_source_disable_multi($local_data_ids) {
390396
FROM poller_item
391397
WHERE local_data_id IN(' . $ids_to_disable . ')'), 'poller_id', 'poller_id');
392398

399+
$all_poller_ids = $all_poller_ids + $poller_ids;
400+
393401
db_execute("DELETE FROM poller_item WHERE local_data_id IN ($ids_to_disable)");
394402
db_execute("UPDATE data_template_data SET active='' WHERE local_data_id IN ($ids_to_disable)");
395403

@@ -415,6 +423,8 @@ function api_data_source_disable_multi($local_data_ids) {
415423
'poller_id', 'poller_id'
416424
);
417425

426+
$all_poller_ids = $all_poller_ids + $poller_ids;
427+
418428
db_execute("DELETE FROM poller_item WHERE local_data_id IN ($ids_to_disable)");
419429
db_execute("UPDATE data_template_data SET active='' WHERE local_data_id IN ($ids_to_disable)");
420430

@@ -429,8 +439,8 @@ function api_data_source_disable_multi($local_data_ids) {
429439
}
430440
}
431441

432-
if (cacti_sizeof($poller_ids)) {
433-
foreach ($poller_ids as $poller_id) {
442+
if (cacti_sizeof($all_poller_ids)) {
443+
foreach ($all_poller_ids as $poller_id) {
434444
api_data_source_cache_crc_update($poller_id);
435445
}
436446
}

lib/functions.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4860,7 +4860,7 @@ function general_header() {
48604860
}
48614861

48624862
function appendHeaderSuppression($url) {
4863-
if (strpos($url, 'header=false') < 0) {
4863+
if (strpos($url, 'header=false') === false) {
48644864
return $url . (strpos($url, '?') ? '&':'?') . 'header=false';
48654865
}
48664866

lib/installer.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1085,7 +1085,7 @@ private function setSnmpOptions($param_snmp_options = array()) {
10851085
private function getModules() {
10861086
global $config;
10871087

1088-
if (isset($this->extensions) || empty($this->extensions)) {
1088+
if (!isset($this->extensions) || empty($this->extensions)) {
10891089
$extensions = utility_php_extensions();
10901090

10911091
foreach ($extensions as $name => $e) {

lib/time.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -156,7 +156,7 @@ function get_timespan(&$span, $curr_time, $timespan_given, $first_weekdayid) {
156156
*/
157157
function month_shift($shift_size) {
158158
# is monthly shifting required?
159-
return ( strpos(strtolower($shift_size), 'month') > 0);
159+
return ( strpos(strtolower($shift_size), 'month') !== false);
160160
}
161161

162162
/* check_month_boundaries - check given boundaries for begin/end of month matching

remote_agent.php

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -330,8 +330,16 @@ function get_snmp_data() {
330330
return;
331331
}
332332

333+
$output = '';
334+
333335
if (!empty($host_id)) {
334336
$host = db_fetch_row_prepared('SELECT * FROM host WHERE id = ?', array($host_id));
337+
338+
if (!cacti_sizeof($host)) {
339+
print 'U';
340+
return;
341+
}
342+
335343
$session = cacti_snmp_session($host['hostname'], $host['snmp_community'], $host['snmp_version'],
336344
$host['snmp_username'], $host['snmp_password'], $host['snmp_auth_protocol'], $host['snmp_priv_passphrase'],
337345
$host['snmp_priv_protocol'], $host['snmp_context'], $host['snmp_engine_id'], $host['snmp_port'],
@@ -357,8 +365,16 @@ function get_snmp_data_walk() {
357365
return;
358366
}
359367

368+
$output = array();
369+
360370
if (!empty($host_id)) {
361371
$host = db_fetch_row_prepared('SELECT * FROM host WHERE id = ?', array($host_id));
372+
373+
if (!cacti_sizeof($host)) {
374+
print 'U';
375+
return;
376+
}
377+
362378
$session = cacti_snmp_session($host['hostname'], $host['snmp_community'], $host['snmp_version'],
363379
$host['snmp_username'], $host['snmp_password'], $host['snmp_auth_protocol'], $host['snmp_priv_passphrase'],
364380
$host['snmp_priv_protocol'], $host['snmp_context'], $host['snmp_engine_id'], $host['snmp_port'],
Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
<?php
2+
/*
3+
+-------------------------------------------------------------------------+
4+
| Copyright (C) 2004-2026 The Cacti Group |
5+
| |
6+
| This program is free software; you can redistribute it and/or |
7+
| modify it under the terms of the GNU General Public License |
8+
| as published by the Free Software Foundation; either version 2 |
9+
| of the License, or (at your option) any later version. |
10+
+-------------------------------------------------------------------------+
11+
| Cacti: The Complete RRDtool-based Graphing Solution |
12+
+-------------------------------------------------------------------------+
13+
*/
14+
15+
/*
16+
* api_data_source_disable_multi() reassigned $poller_ids on every 1000-id
17+
* chunk, so the trailing api_data_source_cache_crc_update loop only saw
18+
* the pollers attached to the last chunk. Pollers that owned only items
19+
* in earlier chunks kept stale poller_output_boost rows and their CRCs
20+
* never advanced, so remote pollers continued to push data for already-
21+
* disabled local_data_ids until the next manual cache rebuild. The fix
22+
* is an $all_poller_ids accumulator built with the `+` union so keys
23+
* survive across chunks and the CRC update covers every poller touched.
24+
*/
25+
26+
$source = file_get_contents(__DIR__ . '/../../lib/api_data_source.php');
27+
28+
$start = strpos($source, 'function api_data_source_disable_multi(');
29+
expect($start)->not->toBeFalse();
30+
31+
$end = strpos($source, "\nfunction ", $start + 1);
32+
$body = substr($source, $start, $end !== false ? $end - $start : 8000);
33+
34+
test('api_data_source_disable_multi initialises the accumulator once', function () use ($body) {
35+
expect($body)->toContain('$all_poller_ids = array();');
36+
37+
/* Only one initialisation, otherwise we are clobbering across chunks again. */
38+
expect(substr_count($body, '$all_poller_ids = array();'))->toBe(1);
39+
});
40+
41+
test('both chunk paths append to $all_poller_ids', function () use ($body) {
42+
expect(substr_count($body, '$all_poller_ids = $all_poller_ids + $poller_ids;'))->toBe(2);
43+
});
44+
45+
test('trailing CRC update reads $all_poller_ids, not the last-chunk local', function () use ($body) {
46+
expect($body)->toContain('if (cacti_sizeof($all_poller_ids))');
47+
expect($body)->toContain('foreach ($all_poller_ids as $poller_id)');
48+
expect($body)->toContain('api_data_source_cache_crc_update($poller_id);');
49+
50+
$crcSection = substr($body, strpos($body, 'if (cacti_sizeof($all_poller_ids))'));
51+
expect(strpos($crcSection, 'if (cacti_sizeof($poller_ids))'))
52+
->toBeFalse('the trailing CRC guard must not fall back to $poller_ids');
53+
});
54+
55+
test('chunk union behaviour: $all_poller_ids preserves pollers from every chunk', function () {
56+
/* Build a fixture that mirrors the production layout: 2500 local_data_ids,
57+
* the first 1000 attached to poller 1, the next 1000 split between
58+
* pollers 2 and 3, the final 500 attached to poller 4. The old single-
59+
* $poller_ids shape only retained poller 4 after the loop completed. */
60+
$chunks = [
61+
array_fill_keys([1], 1), /* chunk 1: poller 1 only */
62+
array_fill_keys([2, 3], null), /* chunk 2: pollers 2 and 3 */
63+
array_fill_keys([4], 4), /* chunk 3: poller 4 only */
64+
];
65+
66+
/* Old shape: $poller_ids is reassigned every chunk. */
67+
$poller_ids = array();
68+
foreach ($chunks as $chunk) {
69+
$poller_ids = array_combine(array_keys($chunk), array_keys($chunk));
70+
}
71+
expect(array_keys($poller_ids))->toBe([4]);
72+
73+
/* New shape: $all_poller_ids accumulates via `+` union. */
74+
$all_poller_ids = array();
75+
foreach ($chunks as $chunk) {
76+
$poller_ids = array_combine(array_keys($chunk), array_keys($chunk));
77+
$all_poller_ids = $all_poller_ids + $poller_ids;
78+
}
79+
$keys = array_keys($all_poller_ids);
80+
sort($keys);
81+
expect($keys)->toBe([1, 2, 3, 4]);
82+
});
Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
<?php
2+
/*
3+
+-------------------------------------------------------------------------+
4+
| Copyright (C) 2004-2026 The Cacti Group |
5+
| |
6+
| This program is free software; you can redistribute it and/or |
7+
| modify it under the terms of the GNU General Public License |
8+
| as published by the Free Software Foundation; either version 2 |
9+
| of the License, or (at your option) any later version. |
10+
+-------------------------------------------------------------------------+
11+
| Cacti: The Complete RRDtool-based Graphing Solution |
12+
+-------------------------------------------------------------------------+
13+
*/
14+
15+
/*
16+
* appendHeaderSuppression() guarded "already present?" with
17+
* strpos($url, 'header=false') < 0. strpos() returns false on no-match
18+
* and a non-negative integer on a hit; "< 0" is therefore always false
19+
* because false < 0 is also false. The guard never fired and the function
20+
* appended &header=false on every call, eventually producing URLs like
21+
* ?action=edit&header=false&header=false&header=false
22+
* The fix is the canonical `=== false` strpos idiom.
23+
*/
24+
25+
$source = file_get_contents(__DIR__ . '/../../lib/functions.php');
26+
27+
test('lib/functions.php uses === false in appendHeaderSuppression', function () use ($source) {
28+
$start = strpos($source, 'function appendHeaderSuppression(');
29+
expect($start)->not->toBeFalse();
30+
31+
$end = strpos($source, "\nfunction ", $start + 1);
32+
$body = substr($source, $start, $end !== false ? $end - $start : 400);
33+
34+
expect($body)->toContain("strpos(\$url, 'header=false') === false");
35+
expect(strpos($body, "strpos(\$url, 'header=false') < 0"))
36+
->toBeFalse('the old "< 0" guard must be gone');
37+
});
38+
39+
/* Local copy of the fixed function. We avoid loading lib/functions.php
40+
* here because that file requires the full Cacti bootstrap. The shape
41+
* mirrors the production definition; the source-pattern test above
42+
* pins the production code to this same shape. */
43+
if (!function_exists('_test_appendHeaderSuppression')) {
44+
function _test_appendHeaderSuppression($url) {
45+
if (strpos($url, 'header=false') === false) {
46+
return $url . (strpos($url, '?') ? '&' : '?') . 'header=false';
47+
}
48+
49+
return $url;
50+
}
51+
}
52+
53+
test('appendHeaderSuppression is idempotent on repeated calls', function () {
54+
$first = _test_appendHeaderSuppression('graph.php?action=edit');
55+
$second = _test_appendHeaderSuppression($first);
56+
57+
expect($first)->toBe('graph.php?action=edit&header=false');
58+
expect($second)->toBe($first);
59+
60+
/* Already present and no querystring delimiter swap. */
61+
expect(_test_appendHeaderSuppression('graph.php?header=false'))->toBe('graph.php?header=false');
62+
});
Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
<?php
2+
/*
3+
+-------------------------------------------------------------------------+
4+
| Copyright (C) 2004-2026 The Cacti Group |
5+
| |
6+
| This program is free software; you can redistribute it and/or |
7+
| modify it under the terms of the GNU General Public License |
8+
| as published by the Free Software Foundation; either version 2 |
9+
| of the License, or (at your option) any later version. |
10+
+-------------------------------------------------------------------------+
11+
| Cacti: The Complete RRDtool-based Graphing Solution |
12+
+-------------------------------------------------------------------------+
13+
*/
14+
15+
/*
16+
* Source-level test for the cmd_realtime.php poller loop. The realtime
17+
* runner switches on $item['action']; default and unknown actions never
18+
* write $output, so an unguarded $output from a prior iteration would
19+
* leak into the next local_data_id's poller_output_realtime insert.
20+
* The fix is an unset($output) at the top of each foreach iteration,
21+
* placed strictly before the switch statement so every branch starts
22+
* from a clean local.
23+
*/
24+
25+
$source = file_get_contents(__DIR__ . '/../../cmd_realtime.php');
26+
27+
test('cmd_realtime.php parses and still contains the realtime foreach', function () use ($source) {
28+
expect($source)->toContain('foreach($poller_items as $item)');
29+
expect($source)->toContain('switch ($item[\'action\'])');
30+
});
31+
32+
test('unset($output) sits inside the foreach body before the action switch', function () use ($source) {
33+
$loopStart = strpos($source, 'foreach($poller_items as $item)');
34+
expect($loopStart)->not->toBeFalse();
35+
36+
$switchStart = strpos($source, 'switch ($item[\'action\'])', $loopStart);
37+
expect($switchStart)->not->toBeFalse();
38+
39+
$unsetPos = strpos($source, 'unset($output)', $loopStart);
40+
expect($unsetPos)->not->toBeFalse('unset($output) must be present inside the foreach body');
41+
expect($unsetPos < $switchStart)->toBeTrue('unset($output) must run before the action switch');
42+
expect($unsetPos > $loopStart)->toBeTrue('unset($output) must live inside the foreach body');
43+
});

0 commit comments

Comments
 (0)