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 778763d..611439c 100644 --- a/tests/py/test_review_regressions.py +++ b/tests/py/test_review_regressions.py @@ -506,3 +506,85 @@ 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 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 + 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)