From 85512e3c822e7d62d26fc11116f18b3c492e0e1e Mon Sep 17 00:00:00 2001 From: Andrey Golovanov Date: Mon, 24 Aug 2026 02:14:21 +0100 Subject: [PATCH 1/2] Cover max_path_cost_factor, salvaged from an abandoned branch While verifying that main supersedes cursor/critical-bug-fixes-6f68 (an unmerged March 2026 branch fixing the same defect class as v0.8.0) it turned out that max_path_cost_factor appears in no test at all, Python or C++. The 0.8.0 fix at src/flow_policy.cpp:149-158 - which skips the relative bound while best_path_cost_ is still the INT64_MAX sentinel, and drops it when the product would not fit - was shipped untested. The abandoned branch had a test for this, but not one that can be ported: it builds a graph with cost INT64_MAX-5, which main now rejects at construction (the 2**62 total-cost ceiling). These cover the same concern with graphs main accepts: the sentinel path before any best cost exists, a factor large enough to overflow the product, and the bound still functionally excluding a too-expensive alternative. Scope is stated in the docstring: these are coverage, not detectors of the original UB. Run against 0.7.2 they pass, because an out-of-range float->int conversion is undefined rather than reliably wrong. UBSan is what discriminates fixed from unfixed; this gives it something to instrument on a path it previously never reached. Co-Authored-By: Claude Opus 5 --- tests/py/test_review_regressions.py | 77 +++++++++++++++++++++++++++++ 1 file changed, 77 insertions(+) diff --git a/tests/py/test_review_regressions.py b/tests/py/test_review_regressions.py index 778763d..f7e3536 100644 --- a/tests/py/test_review_regressions.py +++ b/tests/py/test_review_regressions.py @@ -506,3 +506,80 @@ def test_flow_index_constructor_matches_the_binding(self): ngc.FlowIndex(src=1, dst=2, flowClass=3, flowId=4) # positional-only with pytest.raises(AttributeError): idx.src = 9 # read-only, as the stub's properties declare + + +class TestFlowPolicyMaxPathCostFactor: + """`max_path_cost_factor` had no test coverage at all. + + The gate multiplies the best path cost by the factor and casts back to Cost. + Before 0.8.0 it did that unconditionally, including while `best_path_cost_` was + still the INT64_MAX "no best path yet" sentinel -- the product is then far outside + the int64 range and the conversion is undefined behavior. The fix skips the + relative bound until a best cost exists and drops it when the product would not + fit. Neither branch was exercised by any test. + + Scope: these are *coverage* tests, not detectors of the original UB. They were run + against 0.7.2 (pre-fix) and pass there too, because an out-of-range float->int + conversion is undefined rather than reliably wrong -- in practice it saturates to + something that still admits the path. What discriminates fixed from unfixed here is + UBSan (`make sanitize-test`), and these tests are what gives it something to + instrument on this path. Their standalone value is guarding the *functional* + behaviour of the bound during future refactors. + """ + + @staticmethod + def _policy(algs, gh, *, max_flow_count=1, **cfg_kwargs): + sel = ngc.EdgeSelection( + multi_edge=True, + require_capacity=True, + tie_break=ngc.EdgeTieBreak.DETERMINISTIC, + ) + cfg = ngc.FlowPolicyConfig( + path_alg=ngc.PathAlg.SPF, + flow_placement=ngc.FlowPlacement.PROPORTIONAL, + selection=sel, + max_flow_count=max_flow_count, + **cfg_kwargs, + ) + return ngc.FlowPolicy(algs, gh, cfg) + + def test_factor_applies_before_any_best_cost_exists(self, algs): + """First placement: the sentinel path must not be multiplied and cast.""" + g = _graph(3, [(0, 1, 1.0, 5), (1, 2, 1.0, 5)]) + gh = algs.build_graph(g) + policy = self._policy(algs, gh, max_path_cost_factor=2.0) + placed, left = policy.place_demand(ngc.FlowGraph(g), 0, 2, 0, 1.0) + assert placed == pytest.approx(1.0) + assert left == pytest.approx(0.0) + + def test_huge_factor_does_not_wrap_the_bound(self, algs): + """best_cost * factor overflows int64; the bound must be dropped, not wrapped. + + A wrapped (negative) bound would reject the shortest path itself, so a + successful placement is what distinguishes the guard from the bug. + """ + big = 1 << 60 # legal: total stays below the 2**62 construction ceiling + g = _graph(3, [(0, 1, 1.0, big), (1, 2, 1.0, big)]) + gh = algs.build_graph(g) + policy = self._policy(algs, gh, max_path_cost_factor=1e9) + placed, _ = policy.place_demand(ngc.FlowGraph(g), 0, 2, 0, 1.0) + assert placed == pytest.approx(1.0) + + def test_factor_still_excludes_a_too_expensive_alternative(self, algs): + """The bound must remain functional, not merely safe.""" + g = _graph( + 4, + [ + (0, 1, 1.0, 1), + (1, 3, 1.0, 1), # cost 2 route + (0, 2, 5.0, 50), + (2, 3, 5.0, 50), # cost 100 route, far beyond 2x + ], + ) + gh = algs.build_graph(g) + policy = self._policy( + algs, gh, max_flow_count=4, max_path_cost_factor=2.0, min_flow_count=1 + ) + placed, _ = policy.place_demand(ngc.FlowGraph(g), 0, 3, 0, 6.0) + # Only the cost-2 route is within 2x; the cost-100 route must stay unused. + assert placed == pytest.approx(1.0) From a3f1e5b47c0a0595974af6867a6bc7227decf1d6 Mon Sep 17 00:00:00 2001 From: Andrey Golovanov Date: Mon, 24 Aug 2026 02:24:19 +0100 Subject: [PATCH 2/2] Make UBSan findings fatal and detect the max_path_cost_factor UB in C++ Codex review on #7 was right on both counts, and the first led somewhere worse than reported. 1. `make sanitize-test` never ran the Python tests. The target builds a sanitized extension and then invokes ctest, so the tests added in the previous commit were never instrumented. The claim in their docstring that UBSan discriminated fixed from unfixed was simply false. 2. Those tests could not reach the sentinel branch anyway. best_path_cost_ is updated from the sentinel at src/flow_policy.cpp:135, before the gate runs, so a reachable destination always leaves it set. Only an unreachable destination keeps INT64_MAX live into the multiply-and-cast. Following (1) turned up the larger problem: UBSan findings were never fatal. Nothing set -fno-sanitize-recover or UBSAN_OPTIONS, so UBSan printed "runtime error: ... is outside the range of representable values" and let the process continue to exit 0. ctest reported success. Undefined behaviour was being detected and then discarded, which is indistinguishable from not checking. SAN_FLAGS now carries -fno-sanitize-recover=undefined. The full sanitized suite passes with it, so nothing else in the codebase was relying on that leniency. Adds FlowPolicyCostBounds.MaxPathCostFactor_{UnreachableDst_KeepsSentinelOutOfTheCast, HugeProduct_DropsBoundInsteadOfWrapping} in C++, where ctest actually runs them. Both need min_flow_count >= 1: seeding is what calls get_path_bundle, and without it the gate is never reached at all. Verified by temporarily reverting the 0.8.0 guard to its pre-fix form: fixed code -> 156/156 pass under fatal UBSan pre-fix code -> both new tests fail, at src/flow_policy.cpp with the exact out-of-range diagnostic The Python tests keep their functional value and their docstring now states plainly that they are coverage only, and points at the C++ tests for detection. Co-Authored-By: Claude Opus 5 --- CMakeLists.txt | 8 +++- tests/cpp/flow_policy_tests.cpp | 71 +++++++++++++++++++++++++++++ tests/py/test_review_regressions.py | 19 +++++--- 3 files changed, 90 insertions(+), 8 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index a05aa3c..1f08d4c 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -230,7 +230,13 @@ if(NETGRAPH_CORE_SANITIZE) if(CMAKE_CXX_COMPILER_ID MATCHES "GNU|Clang") # Must be a CMake list: a single quoted string reaches the compiler as one # argument and clang rejects "-fsanitize=address,undefined -fno-omit-frame-pointer". - set(SAN_FLAGS -fsanitize=address,undefined -fno-omit-frame-pointer) + # + # -fno-sanitize-recover=undefined makes UBSan findings FATAL. Without it UBSan + # prints a "runtime error:" diagnostic and lets the program continue, so the test + # still exits 0 and ctest reports success -- undefined behaviour would be detected + # and then silently ignored, which is indistinguishable from not checking at all. + set(SAN_FLAGS -fsanitize=address,undefined -fno-omit-frame-pointer + -fno-sanitize-recover=undefined) set(SAN_TARGETS netgraph_core _netgraph_core) if(TARGET netgraph_core_tests) # The test executable links the sanitized static library, so it must be diff --git a/tests/cpp/flow_policy_tests.cpp b/tests/cpp/flow_policy_tests.cpp index 4b53ea2..ac2b499 100644 --- a/tests/cpp/flow_policy_tests.cpp +++ b/tests/cpp/flow_policy_tests.cpp @@ -434,3 +434,74 @@ TEST(FlowPolicyStatic, ValidationRejectsForeignAndDisconnectedBundles) { EXPECT_THROW(policy.set_static_paths(0, 2, std::move(b)), std::invalid_argument); } } + +// The max_path_cost_factor gate multiplies best_path_cost_ by the factor and casts +// the product back to Cost. best_path_cost_ is only updated when dst is reachable +// (flow_policy.cpp:135), so an UNREACHABLE destination is the one way to reach the +// gate while it still holds the INT64_MAX "no best path yet" sentinel. Before 0.8.0 +// the multiply-and-cast ran unconditionally there, and INT64_MAX * factor is far +// outside the int64 range -- undefined behavior. +// +// No assertion can distinguish fixed from unfixed here: both return "no path" for an +// unreachable destination. The check is the UB itself, so this test exists to give +// UBSan something to instrument. It only has teeth under `make sanitize-test`, which +// runs ctest -- which is why this lives in C++ and not in the Python suite. +TEST(FlowPolicyCostBounds, MaxPathCostFactor_UnreachableDst_KeepsSentinelOutOfTheCast) { + // 0 -> 1, plus an isolated node 2 that cannot be reached from 0. + std::vector src{0}, dst{1}; + std::vector cap{1.0}; + std::vector cost{7}; + auto g = StrictMultiDiGraph::from_arrays(3, src, dst, cap, cost); + FlowGraph fg(g); + + EdgeSelection sel; sel.multi_edge = true; sel.require_capacity = true; + sel.tie_break = EdgeTieBreak::Deterministic; + auto be = make_cpu_backend(); auto algs = std::make_shared(be); + auto gh = algs->build_graph(g); + ExecutionContext ctx(algs, gh); + + FlowPolicyConfig cfg; + cfg.flow_placement = FlowPlacement::Proportional; + cfg.selection = sel; + cfg.require_capacity = true; + cfg.max_flow_count = 1; + cfg.min_flow_count = 1; // seeding is what calls get_path_bundle + cfg.max_path_cost_factor = 2.0; + FlowPolicy policy(ctx, cfg); + + // Reaches the gate with best_path_cost_ == INT64_MAX. Nothing can be placed. + auto res = policy.place_demand(fg, /*src=*/0, /*dst=*/2, /*flowClass=*/0, /*volume=*/1.0); + EXPECT_NEAR(res.first, 0.0, 1e-9); + EXPECT_NEAR(res.second, 1.0, 1e-9); +} + +// A very large (but legal) best cost times a large factor overflows the int64 range +// too. The bound must be dropped rather than wrapped: a wrapped negative bound would +// reject the shortest path itself, so a successful placement is what separates the +// guard from the bug. Unlike the sentinel case above, this one IS assertable. +TEST(FlowPolicyCostBounds, MaxPathCostFactor_HugeProduct_DropsBoundInsteadOfWrapping) { + const std::int64_t big = std::int64_t{1} << 60; // total stays below the 2^62 ceiling + std::vector src{0, 1}, dst{1, 2}; + std::vector cap{1.0, 1.0}; + std::vector cost{big, big}; + auto g = StrictMultiDiGraph::from_arrays(3, src, dst, cap, cost); + FlowGraph fg(g); + + EdgeSelection sel; sel.multi_edge = true; sel.require_capacity = true; + sel.tie_break = EdgeTieBreak::Deterministic; + auto be = make_cpu_backend(); auto algs = std::make_shared(be); + auto gh = algs->build_graph(g); + ExecutionContext ctx(algs, gh); + + FlowPolicyConfig cfg; + cfg.flow_placement = FlowPlacement::Proportional; + cfg.selection = sel; + cfg.require_capacity = true; + cfg.max_flow_count = 1; + cfg.min_flow_count = 1; + cfg.max_path_cost_factor = 1e9; + FlowPolicy policy(ctx, cfg); + + auto res = policy.place_demand(fg, /*src=*/0, /*dst=*/2, /*flowClass=*/0, /*volume=*/1.0); + EXPECT_NEAR(res.first, 1.0, 1e-9); +} diff --git a/tests/py/test_review_regressions.py b/tests/py/test_review_regressions.py index f7e3536..611439c 100644 --- a/tests/py/test_review_regressions.py +++ b/tests/py/test_review_regressions.py @@ -518,13 +518,18 @@ class TestFlowPolicyMaxPathCostFactor: relative bound until a best cost exists and drops it when the product would not fit. Neither branch was exercised by any test. - Scope: these are *coverage* tests, not detectors of the original UB. They were run - against 0.7.2 (pre-fix) and pass there too, because an out-of-range float->int - conversion is undefined rather than reliably wrong -- in practice it saturates to - something that still admits the path. What discriminates fixed from unfixed here is - UBSan (`make sanitize-test`), and these tests are what gives it something to - instrument on this path. Their standalone value is guarding the *functional* - behaviour of the bound during future refactors. + Scope: these are *coverage* tests, not detectors of the original UB. They pass + against 0.7.2 (pre-fix) too, because an out-of-range float->int conversion is + undefined rather than reliably wrong -- in practice it saturates to something that + still admits the path. They also cannot reach the sentinel branch at all: a + reachable destination updates `best_path_cost_` (flow_policy.cpp:135) before the + gate runs. Their value is guarding the *functional* behaviour of the bound. + + Detection of the UB itself lives in C++, in + `FlowPolicyCostBounds.MaxPathCostFactor_*` -- `make sanitize-test` builds with + sanitizers but runs `ctest`, so it never executes this Python suite. Those tests + use an unreachable destination to keep the sentinel live, and fail against the + pre-fix conversion. """ @staticmethod