Skip to content

Commit e87b8b3

Browse files
somethingwithproofsomethingwithproof
authored
fix: CF fallback selection overwritten by cf_reference (#6884)
* fix: remove CF fallback overwrite that made fallback chain dead code The consolidation function fallback selection (lines 2046-2056) chose an available CF when the preferred CF had no DEF, but line 2060 unconditionally overwrote the result with graph_item['cf_reference']. Remove the overwrite so the fallback chain (AVERAGE > MAX > MIN > LAST) actually takes effect for GPRINT items with missing DEFs. Includes 16 regression tests proving the fallback priority and demonstrating the buggy vs fixed behavior. Fixes #6883 Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix: replace undefined cacti_redirect with header+exit in oauth2.php Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * revert: remove oauth2.php fix (moved to standalone PR #6886) Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * test: replace redundant 100-iteration loop with single determinism check Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> --------- Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
1 parent 06d5237 commit e87b8b3

File tree

2 files changed

+211
-3
lines changed

2 files changed

+211
-3
lines changed

‎lib/rrd.php‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2056,9 +2056,6 @@ function rrdtool_function_graph(int $local_graph_id, mixed $rra_id, array $graph
20562056
}
20572057
}
20582058

2059-
// now remember the correct CF reference
2060-
$cf_id = $graph_item['cf_reference'];
2061-
20622059
// +++++++++++++++++++++++ GRAPH ITEMS: CDEF START +++++++++++++++++++++++
20632060

20642061
/**

‎tests/Unit/RrdCfFallbackTest.php‎

Lines changed: 211 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,211 @@
1+
<?php
2+
declare(strict_types=1);
3+
/*
4+
+-------------------------------------------------------------------------+
5+
| Copyright (C) 2004-2026 The Cacti Group |
6+
+-------------------------------------------------------------------------+
7+
| Cacti: The Complete RRDtool-based Graphing Solution |
8+
+-------------------------------------------------------------------------+
9+
*/
10+
11+
/*
12+
* Regression test for CF fallback overwrite bug in rrdtool_function_graph().
13+
*
14+
* The bug was at lib/rrd.php:2060 where $cf_id was unconditionally
15+
* overwritten with $graph_item['cf_reference'] after the fallback
16+
* chain at lines 2041-2056 had selected an available CF.
17+
*
18+
* @group regression
19+
*/
20+
21+
require_once dirname(__DIR__) . '/Helpers/CactiStubs.php';
22+
23+
/**
24+
* Simulate the CF fallback selection logic (FIXED version).
25+
*
26+
* When the preferred CF has no DEF, falls back to AVERAGE > MAX > MIN > LAST.
27+
*
28+
* @param array $cf_ds_cache Available CF/DS combinations (keyed as "rrd_id:cf_id")
29+
* @param int $data_template_rrd_id The data template RRD ID
30+
* @param int $preferred_cf The preferred consolidation function ID
31+
* @param int $cf_reference The graph item's cf_reference (was used to overwrite)
32+
*
33+
* @return int The selected CF ID
34+
*/
35+
function cf_fallback_fixed(array $cf_ds_cache, int $data_template_rrd_id, int $preferred_cf, int $cf_reference): int {
36+
$cf_ds_key = "$data_template_rrd_id:$preferred_cf";
37+
38+
if (isset($cf_ds_cache[$cf_ds_key])) {
39+
return $preferred_cf;
40+
}
41+
42+
// Fallback: AVERAGE(1) > MAX(3) > MIN(2) > LAST(4)
43+
if (isset($cf_ds_cache["$data_template_rrd_id:1"])) return 1;
44+
if (isset($cf_ds_cache["$data_template_rrd_id:3"])) return 3;
45+
if (isset($cf_ds_cache["$data_template_rrd_id:2"])) return 2;
46+
if (isset($cf_ds_cache["$data_template_rrd_id:4"])) return 4;
47+
48+
return 1; // default AVERAGE
49+
}
50+
51+
/**
52+
* Simulate the BUGGY version that overwrites fallback with cf_reference.
53+
*
54+
* @return int Always returns cf_reference (bug)
55+
*/
56+
function cf_fallback_buggy(array $cf_ds_cache, int $data_template_rrd_id, int $preferred_cf, int $cf_reference): int {
57+
$cf_ds_key = "$data_template_rrd_id:$preferred_cf";
58+
59+
if (isset($cf_ds_cache[$cf_ds_key])) {
60+
$cf_id = $preferred_cf;
61+
} else {
62+
if (isset($cf_ds_cache["$data_template_rrd_id:1"])) { $cf_id = 1; }
63+
elseif (isset($cf_ds_cache["$data_template_rrd_id:3"])) { $cf_id = 3; }
64+
elseif (isset($cf_ds_cache["$data_template_rrd_id:2"])) { $cf_id = 2; }
65+
elseif (isset($cf_ds_cache["$data_template_rrd_id:4"])) { $cf_id = 4; }
66+
else { $cf_id = 1; }
67+
}
68+
69+
// BUG: overwrite with cf_reference (was line 2060)
70+
$cf_id = $cf_reference;
71+
72+
return $cf_id;
73+
}
74+
75+
// ===========================================================================
76+
// Fixed behavior tests
77+
// ===========================================================================
78+
79+
describe('CF fallback selection (fixed)', function () {
80+
test('preferred CF available: uses it directly', function () {
81+
$cache = ['10:3' => true, '10:1' => true];
82+
expect(cf_fallback_fixed($cache, 10, 3, 3))->toBe(3);
83+
});
84+
85+
test('preferred CF missing: falls back to AVERAGE', function () {
86+
$cache = ['10:1' => true]; // only AVERAGE available
87+
expect(cf_fallback_fixed($cache, 10, 3, 3))->toBe(1);
88+
});
89+
90+
test('preferred CF missing, only MAX available: falls back to MAX', function () {
91+
$cache = ['10:3' => true];
92+
expect(cf_fallback_fixed($cache, 10, 1, 1))->toBe(3);
93+
});
94+
95+
test('preferred CF missing, only MIN available: falls back to MIN', function () {
96+
$cache = ['10:2' => true];
97+
expect(cf_fallback_fixed($cache, 10, 3, 3))->toBe(2);
98+
});
99+
100+
test('preferred CF missing, only LAST available: falls back to LAST', function () {
101+
$cache = ['10:4' => true];
102+
expect(cf_fallback_fixed($cache, 10, 1, 1))->toBe(4);
103+
});
104+
105+
test('no CFs available: defaults to AVERAGE (1)', function () {
106+
expect(cf_fallback_fixed([], 10, 3, 3))->toBe(1);
107+
});
108+
109+
test('fallback priority: AVERAGE > MAX > MIN > LAST', function () {
110+
// All available: AVERAGE wins
111+
$all = ['10:1' => true, '10:2' => true, '10:3' => true, '10:4' => true];
112+
expect(cf_fallback_fixed($all, 10, 99, 99))->toBe(1);
113+
114+
// No AVERAGE: MAX wins
115+
$no_avg = ['10:2' => true, '10:3' => true, '10:4' => true];
116+
expect(cf_fallback_fixed($no_avg, 10, 99, 99))->toBe(3);
117+
118+
// No AVERAGE or MAX: MIN wins
119+
$no_avg_max = ['10:2' => true, '10:4' => true];
120+
expect(cf_fallback_fixed($no_avg_max, 10, 99, 99))->toBe(2);
121+
122+
// Only LAST: LAST wins
123+
$only_last = ['10:4' => true];
124+
expect(cf_fallback_fixed($only_last, 10, 99, 99))->toBe(4);
125+
});
126+
});
127+
128+
// ===========================================================================
129+
// Bug demonstration
130+
// ===========================================================================
131+
132+
describe('Bug demonstration: buggy vs fixed', function () {
133+
test('buggy version ignores fallback, always returns cf_reference', function () {
134+
$cache = ['10:1' => true]; // AVERAGE available
135+
// Preferred CF 3 (MAX) is not available, fallback should give 1 (AVERAGE)
136+
$buggy = cf_fallback_buggy($cache, 10, 3, 3);
137+
expect($buggy)->toBe(3); // BUG: returns cf_reference instead of fallback
138+
});
139+
140+
test('fixed version correctly falls back', function () {
141+
$cache = ['10:1' => true];
142+
$fixed = cf_fallback_fixed($cache, 10, 3, 3);
143+
expect($fixed)->toBe(1); // Correctly returns AVERAGE
144+
});
145+
146+
test('buggy and fixed produce different results when fallback needed', function () {
147+
$cache = ['10:1' => true];
148+
$buggy = cf_fallback_buggy($cache, 10, 3, 3);
149+
$fixed = cf_fallback_fixed($cache, 10, 3, 3);
150+
expect($buggy)->not->toBe($fixed);
151+
});
152+
153+
test('both agree when preferred CF is available', function () {
154+
$cache = ['10:3' => true];
155+
$buggy = cf_fallback_buggy($cache, 10, 3, 3);
156+
$fixed = cf_fallback_fixed($cache, 10, 3, 3);
157+
expect($buggy)->toBe($fixed)->toBe(3);
158+
});
159+
});
160+
161+
// ===========================================================================
162+
// Mutation killers
163+
// ===========================================================================
164+
165+
describe('CF fallback mutation killers', function () {
166+
test('fallback returns different CF than preferred when preferred missing', function () {
167+
$cache = ['10:1' => true];
168+
$r = cf_fallback_fixed($cache, 10, 3, 3);
169+
expect($r)->toBe(1);
170+
expect($r)->not->toBe(3);
171+
});
172+
173+
test('AVERAGE(1) and MAX(3) are distinct fallback results', function () {
174+
$avg_only = ['10:1' => true];
175+
$max_only = ['10:3' => true];
176+
expect(cf_fallback_fixed($avg_only, 10, 99, 99))->toBe(1);
177+
expect(cf_fallback_fixed($max_only, 10, 99, 99))->toBe(3);
178+
});
179+
180+
test('deterministic for same inputs', function () {
181+
$cache = ['10:1' => true, '10:3' => true];
182+
$first = cf_fallback_fixed($cache, 10, 99, 99);
183+
$second = cf_fallback_fixed($cache, 10, 99, 99);
184+
expect($second)->toBe($first);
185+
});
186+
187+
test('different rrd_ids are independent', function () {
188+
$cache = ['10:1' => true, '20:3' => true];
189+
expect(cf_fallback_fixed($cache, 10, 99, 99))->toBe(1);
190+
expect(cf_fallback_fixed($cache, 20, 99, 99))->toBe(3);
191+
});
192+
});
193+
194+
// ===========================================================================
195+
// Source verification: bug is fixed
196+
// ===========================================================================
197+
198+
describe('Source verification: CF overwrite removed', function () {
199+
test('no unconditional cf_reference overwrite after fallback chain', function () {
200+
$c = file_get_contents(__DIR__ . '/../../lib/rrd.php');
201+
$p = strpos($c, 'first we need to check if there is a DEF');
202+
$s = substr($c, $p, 1000);
203+
204+
// The fallback chain should exist
205+
expect($s)->toContain('CF: AVERAGE');
206+
expect($s)->toContain('CF: MAX');
207+
208+
// But the overwrite line should NOT exist
209+
expect($s)->not->toContain('$cf_id = $graph_item[\'cf_reference\']');
210+
});
211+
});

0 commit comments

Comments
 (0)