Add clamping logic for milliseconds conversion and unit tests

src/mesh/Default.cpp

@@ -2,18 +2,25 @@
 
 #include "meshUtils.h"
 
+// Convert seconds to ms, clamping at INT32_MAX (~24.86 days). Callers store the
+// result in uint32_t but the ultimate consumers (OSThread runOnce returns,
+// `MAX_INTERVAL`, Apple-client interop per the FIXME at Default.h:14) treat it
+// as int32_t. Saturating at INT32_MAX keeps every conversion well-defined and
+// non-negative; no realistic interval (broadcast, throttle, sleep) needs more.
+static inline uint32_t secondsToMsClamped(uint32_t secs)
+{
+    constexpr uint32_t MAX_MS = static_cast<uint32_t>(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 +54,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<uint32_t>(INT32_MAX);
+    uint32_t base = getConfiguredOrDefaultMs(configured, defaultValue);
+    float coef = congestionScalingCoefficient(numOnlineNodes);
+    if (static_cast<double>(base) * static_cast<double>(coef) >= static_cast<double>(MAX_MS))
+        return MAX_MS;
+    return base * coef;
 }
 
 uint32_t Default::getConfiguredOrMinimumValue(uint32_t configured, uint32_t minValue)

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<uint32_t>(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<uint32_t>(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<uint32_t>(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<int32_t>(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<uint32_t>(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());
 }