From 515fe8b94f6e6261d727459a9152913924cbf35b Mon Sep 17 00:00:00 2001 From: Ben Meadors Date: Thu, 9 Jul 2026 10:30:11 -0500 Subject: [PATCH] Clamp bw for malformed use_preset false with empty bandwidth --- src/mesh/MeshRadio.h | 10 +++ src/modules/AdminModule.cpp | 11 +++ test/test_admin_radio/test_main.cpp | 103 ++++++++++++++++++++++++++++ 3 files changed, 124 insertions(+) diff --git a/src/mesh/MeshRadio.h b/src/mesh/MeshRadio.h index 3387887c2..7cc8cc58e 100644 --- a/src/mesh/MeshRadio.h +++ b/src/mesh/MeshRadio.h @@ -190,6 +190,16 @@ static inline uint16_t bwKHzToCode(float bwKHz) return (uint16_t)(bwKHz + 0.5f); } +/// Clamp a bandwidth *code* (the on-wire config.lora.bandwidth value) to a usable value. +/// A code of 0 (proto default / unset) returns the default bandwidth's code; any other value +/// is returned unchanged (RadioLib validates the exact discrete bandwidth downstream). +static inline uint16_t clampBandwidthCode(uint16_t bwCode) +{ + if (bwCode == 0) + return bwKHzToCode(LORA_BW_DEFAULT_KHZ); + return bwCode; +} + static inline void modemPresetToParams(meshtastic_Config_LoRaConfig_ModemPreset preset, bool wideLora, float &bwKHz, uint8_t &sf, uint8_t &cr) { diff --git a/src/modules/AdminModule.cpp b/src/modules/AdminModule.cpp index 7c7e12907..a3ee10b12 100644 --- a/src/modules/AdminModule.cpp +++ b/src/modules/AdminModule.cpp @@ -916,6 +916,17 @@ void AdminModule::handleSetConfig(const meshtastic_Config &c, bool fromOthers) validatedLora.spread_factor = LORA_SF_DEFAULT; } + // A custom (non-preset) config that leaves bandwidth at its proto zero-value otherwise slips + // through validateConfigLora() and persists as 0, while the radio silently falls back to the + // default (config.lora.bandwidth then reads back 0 even though the radio runs at 250kHz). + // Coerce it here like coding_rate/spread_factor so the stored config matches the radio. In + // preset mode bandwidth 0 is expected (the preset supplies it), so leave it untouched. + const uint16_t clampedBandwidth = clampBandwidthCode(validatedLora.bandwidth); + if (!validatedLora.use_preset && validatedLora.bandwidth != clampedBandwidth) { + LOG_WARN("Invalid bandwidth %d, setting to %d", validatedLora.bandwidth, clampedBandwidth); + validatedLora.bandwidth = clampedBandwidth; + } + // If we're setting a new region, check the region is valid and then init the region or discard the change if (validatedLora.region != myRegion->code) { // Region has changed so check whether it is valid for e.g. licensing conditions and if the lora config is valid diff --git a/test/test_admin_radio/test_main.cpp b/test/test_admin_radio/test_main.cpp index e8c0e9108..8a839f934 100644 --- a/test/test_admin_radio/test_main.cpp +++ b/test/test_admin_radio/test_main.cpp @@ -1019,6 +1019,104 @@ static void test_handleSetConfig_fromOthers_invalidChannelNumFullyRejected() TEST_ASSERT_EQUAL_UINT32(0, config.lora.channel_num); } +// clampBandwidthCode: an unset (0) bandwidth code maps to the default; any other code is left as-is. +static void test_clampBandwidthCode_zeroMapsToDefaultOthersUnchanged() +{ + TEST_ASSERT_NOT_EQUAL_UINT16(0, clampBandwidthCode(0)); // the point of the fix: 0 must not stay 0 + TEST_ASSERT_EQUAL_UINT16(bwKHzToCode(LORA_BW_DEFAULT_KHZ), clampBandwidthCode(0)); + TEST_ASSERT_EQUAL_UINT16(250, clampBandwidthCode(250)); + TEST_ASSERT_EQUAL_UINT16(125, clampBandwidthCode(125)); + TEST_ASSERT_EQUAL_UINT16(31, clampBandwidthCode(31)); +} + +// A custom (non-preset) config that leaves bandwidth at its proto zero-value must not persist as 0. +// Pre-fix it slipped past validateConfigLora() and the radio silently ran at the default while +// get_config still reported bandwidth 0. It is now coerced to the default code on ingest. +static void test_handleSetConfig_fromLocal_customBandwidthZeroClampedToDefault() +{ + config.lora = meshtastic_Config_LoRaConfig_init_zero; + config.lora.region = meshtastic_Config_LoRaConfig_RegionCode_US; + config.lora.use_preset = true; + config.lora.modem_preset = meshtastic_Config_LoRaConfig_ModemPreset_LONG_FAST; + initRegion(); + + meshtastic_Config c = + makeLoraSetConfig(meshtastic_Config_LoRaConfig_RegionCode_US, false, meshtastic_Config_LoRaConfig_ModemPreset_LONG_FAST); + c.payload_variant.lora.spread_factor = 11; + c.payload_variant.lora.coding_rate = 5; + c.payload_variant.lora.bandwidth = 0; // the footgun: unset custom bandwidth + + testAdmin->handleSetConfig(c, false); // fromOthers = false (local client) + + TEST_ASSERT_FALSE(config.lora.use_preset); + TEST_ASSERT_NOT_EQUAL_UINT16(0, config.lora.bandwidth); // must not persist as 0 + TEST_ASSERT_EQUAL_UINT16(bwKHzToCode(LORA_BW_DEFAULT_KHZ), config.lora.bandwidth); +} + +// Remote admin (fromOthers) is subject to the same ingest clamp: a custom bandwidth 0 from another +// node is normalized to the default rather than persisted as 0 (it does not weaken the wholesale +// rejection of configs that actually fail validation - a 0 bandwidth already passed validation). +static void test_handleSetConfig_fromOthers_customBandwidthZeroClampedToDefault() +{ + config.lora = meshtastic_Config_LoRaConfig_init_zero; + config.lora.region = meshtastic_Config_LoRaConfig_RegionCode_US; + config.lora.use_preset = true; + config.lora.modem_preset = meshtastic_Config_LoRaConfig_ModemPreset_LONG_FAST; + initRegion(); + + meshtastic_Config c = + makeLoraSetConfig(meshtastic_Config_LoRaConfig_RegionCode_US, false, meshtastic_Config_LoRaConfig_ModemPreset_LONG_FAST); + c.payload_variant.lora.spread_factor = 11; + c.payload_variant.lora.coding_rate = 5; + c.payload_variant.lora.bandwidth = 0; + + testAdmin->handleSetConfig(c, true); // fromOthers = true + + TEST_ASSERT_FALSE(config.lora.use_preset); + TEST_ASSERT_EQUAL_UINT16(bwKHzToCode(LORA_BW_DEFAULT_KHZ), config.lora.bandwidth); +} + +// In preset mode bandwidth 0 is the norm (the preset supplies it); the ingest clamp must leave it +// untouched so preset configs still read back bandwidth 0. +static void test_handleSetConfig_fromLocal_presetBandwidthZeroLeftUntouched() +{ + config.lora = meshtastic_Config_LoRaConfig_init_zero; + config.lora.region = meshtastic_Config_LoRaConfig_RegionCode_US; + config.lora.use_preset = true; + config.lora.modem_preset = meshtastic_Config_LoRaConfig_ModemPreset_LONG_FAST; + initRegion(); + + meshtastic_Config c = + makeLoraSetConfig(meshtastic_Config_LoRaConfig_RegionCode_US, true, meshtastic_Config_LoRaConfig_ModemPreset_LONG_FAST); + c.payload_variant.lora.bandwidth = 0; + + testAdmin->handleSetConfig(c, false); + + TEST_ASSERT_TRUE(config.lora.use_preset); + TEST_ASSERT_EQUAL_UINT16(0, config.lora.bandwidth); +} + +// A custom (non-preset) config with an already-valid bandwidth must be preserved verbatim. +static void test_handleSetConfig_fromLocal_customBandwidthNonZeroPreserved() +{ + config.lora = meshtastic_Config_LoRaConfig_init_zero; + config.lora.region = meshtastic_Config_LoRaConfig_RegionCode_US; + config.lora.use_preset = true; + config.lora.modem_preset = meshtastic_Config_LoRaConfig_ModemPreset_LONG_FAST; + initRegion(); + + meshtastic_Config c = + makeLoraSetConfig(meshtastic_Config_LoRaConfig_RegionCode_US, false, meshtastic_Config_LoRaConfig_ModemPreset_LONG_FAST); + c.payload_variant.lora.spread_factor = 11; + c.payload_variant.lora.coding_rate = 5; + c.payload_variant.lora.bandwidth = 125; + + testAdmin->handleSetConfig(c, false); + + TEST_ASSERT_FALSE(config.lora.use_preset); + TEST_ASSERT_EQUAL_UINT16(125, config.lora.bandwidth); +} + // A security-config SET that omits the private key (partial/legacy client editing some other security field) // must NOT regenerate our keypair: our NodeNum is crc32(public_key), so a new keypair would silently change // our identity. The existing keypair has to be preserved. @@ -1246,6 +1344,11 @@ void setup() RUN_TEST(test_handleSetConfig_fromLocal_invalidPresetClamped); RUN_TEST(test_handleSetConfig_fromOthers_validPresetAccepted); RUN_TEST(test_handleSetConfig_fromOthers_invalidChannelNumFullyRejected); + RUN_TEST(test_clampBandwidthCode_zeroMapsToDefaultOthersUnchanged); + RUN_TEST(test_handleSetConfig_fromLocal_customBandwidthZeroClampedToDefault); + RUN_TEST(test_handleSetConfig_fromOthers_customBandwidthZeroClampedToDefault); + RUN_TEST(test_handleSetConfig_fromLocal_presetBandwidthZeroLeftUntouched); + RUN_TEST(test_handleSetConfig_fromLocal_customBandwidthNonZeroPreserved); RUN_TEST(test_handleSetConfig_security_preservesKeypairWhenPrivateOmitted); RUN_TEST(test_handleSetConfig_security_acceptsSuppliedKeypair); RUN_TEST(test_regionInfo_supportsPreset);