From 8d8ff21e7c4b0735fe72cc537021516932651e8c Mon Sep 17 00:00:00 2001 From: Ben Meadors Date: Tue, 28 Apr 2026 19:03:50 -0500 Subject: [PATCH] Add clamping logic for milliseconds conversion and unit tests (#10326) * Add clamping logic for milliseconds conversion and unit tests * Simplify comments in secondsToMsClamped function Removed detailed comments about seconds to milliseconds conversion. --- src/mesh/Default.cpp | 26 +++++++++----- test/test_default/test_main.cpp | 61 +++++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 8 deletions(-) diff --git a/src/mesh/Default.cpp b/src/mesh/Default.cpp index 3ecd766f1..7a2d9e410 100644 --- a/src/mesh/Default.cpp +++ b/src/mesh/Default.cpp @@ -2,18 +2,21 @@ #include "meshUtils.h" +// Convert seconds to ms, clamping at INT32_MAX (~24.86 days) +static inline uint32_t secondsToMsClamped(uint32_t secs) +{ + constexpr uint32_t MAX_MS = static_cast(INT32_MAX); + return (secs > MAX_MS / 1000U) ? MAX_MS : secs * 1000U; +} + uint32_t Default::getConfiguredOrDefaultMs(uint32_t configuredInterval, uint32_t defaultInterval) { - if (configuredInterval > 0) - return configuredInterval * 1000; - return defaultInterval * 1000; + return secondsToMsClamped(configuredInterval > 0 ? configuredInterval : defaultInterval); } uint32_t Default::getConfiguredOrDefaultMs(uint32_t configuredInterval) { - if (configuredInterval > 0) - return configuredInterval * 1000; - return default_broadcast_interval_secs * 1000; + return secondsToMsClamped(configuredInterval > 0 ? configuredInterval : default_broadcast_interval_secs); } uint32_t Default::getConfiguredOrDefault(uint32_t configured, uint32_t defaultValue) @@ -47,7 +50,14 @@ uint32_t Default::getConfiguredOrDefaultMsScaled(uint32_t configured, uint32_t d meshtastic_Config_DeviceConfig_Role_TAK_TRACKER)) return getConfiguredOrDefaultMs(configured, defaultValue); - return getConfiguredOrDefaultMs(configured, defaultValue) * congestionScalingCoefficient(numOnlineNodes); + // Saturate at INT32_MAX to match secondsToMsClamped: float→uint32_t when + // out of range is UB, and the result is consumed as an int32_t downstream. + constexpr uint32_t MAX_MS = static_cast(INT32_MAX); + uint32_t base = getConfiguredOrDefaultMs(configured, defaultValue); + float coef = congestionScalingCoefficient(numOnlineNodes); + if (static_cast(base) * static_cast(coef) >= static_cast(MAX_MS)) + return MAX_MS; + return base * coef; } uint32_t Default::getConfiguredOrMinimumValue(uint32_t configured, uint32_t minValue) @@ -66,4 +76,4 @@ uint8_t Default::getConfiguredOrDefaultHopLimit(uint8_t configured) #else return (configured >= HOP_MAX) ? HOP_MAX : config.lora.hop_limit; #endif -} \ No newline at end of file +} diff --git a/test/test_default/test_main.cpp b/test/test_default/test_main.cpp index 9da367897..4202d7b8d 100644 --- a/test/test_default/test_main.cpp +++ b/test/test_default/test_main.cpp @@ -127,6 +127,60 @@ void test_client_uses_public_channel_minimums() TEST_ASSERT_EQUAL_UINT32(60 * 60, position); } +// --- Saturation/clamp tests for getConfiguredOrDefaultMs[Scaled] --- +// These guard the INT32_MAX clamp added to avoid uint32 wrap of secs*1000 and +// to keep results safe to cast to int32_t for OSThread runOnce returns. + +void test_ms_below_threshold() +{ + // Ordinary value passes through unchanged. + TEST_ASSERT_EQUAL_UINT32(60000U, Default::getConfiguredOrDefaultMs(60, 0)); +} + +void test_ms_at_threshold() +{ + // INT32_MAX / 1000 = 2,147,483 — largest secs that does not clamp. + TEST_ASSERT_EQUAL_UINT32(2147483000U, Default::getConfiguredOrDefaultMs(2147483U, 0)); +} + +void test_ms_just_above_threshold() +{ + // One second over the boundary must saturate, not wrap. + TEST_ASSERT_EQUAL_UINT32(static_cast(INT32_MAX), Default::getConfiguredOrDefaultMs(2147484U, 0)); +} + +void test_ms_uint32_max() +{ + // default_sds_secs == UINT32_MAX on non-routers must not wrap. + TEST_ASSERT_EQUAL_UINT32(static_cast(INT32_MAX), Default::getConfiguredOrDefaultMs(UINT32_MAX, 0)); +} + +void test_ms_default_clamps() +{ + // Clamp also applies when the default-arg path is taken (configured == 0). + TEST_ASSERT_EQUAL_UINT32(static_cast(INT32_MAX), Default::getConfiguredOrDefaultMs(0, UINT32_MAX)); +} + +void test_ms_result_is_int32_safe() +{ + // Regression guard for runOnce returns: cast to int32_t must not go negative. + int32_t result = static_cast(Default::getConfiguredOrDefaultMs(UINT32_MAX, 0)); + TEST_ASSERT_GREATER_OR_EQUAL_INT32(0, result); +} + +void test_scaled_overflow_saturates() +{ + // long_fast (SF11/BW250) with a 24h base and heavy congestion overflows + // the uint32 result without the double-precision guard. Must saturate. + config.device.role = meshtastic_Config_DeviceConfig_Role_CLIENT; + config.lora.use_preset = false; + config.lora.spread_factor = 11; + config.lora.bandwidth = 250; + + uint32_t res = Default::getConfiguredOrDefaultMsScaled(0, ONE_DAY, 1000); + TEST_ASSERT_EQUAL_UINT32(static_cast(INT32_MAX), res); +} + void setup() { // Small delay to match other test mains @@ -140,6 +194,13 @@ void setup() RUN_TEST(test_router_uses_router_minimums); RUN_TEST(test_router_late_uses_router_minimums); RUN_TEST(test_client_uses_public_channel_minimums); + RUN_TEST(test_ms_below_threshold); + RUN_TEST(test_ms_at_threshold); + RUN_TEST(test_ms_just_above_threshold); + RUN_TEST(test_ms_uint32_max); + RUN_TEST(test_ms_default_clamps); + RUN_TEST(test_ms_result_is_int32_safe); + RUN_TEST(test_scaled_overflow_saturates); exit(UNITY_END()); }