From c16115a9088df91c3e090d333348df62c030ed55 Mon Sep 17 00:00:00 2001 From: Marcel Verdult Date: Thu, 12 Nov 2026 13:16:32 +0100 Subject: [PATCH] Check value ranges after loading /prefs.json loadPrefs() reads /prefs.json and uses whatever is in it. Only the legacy binary loader (loadPrefsInt) clamps values, so a hand edited or externally written file can put freq, bw, sf, cr, tx_power_dbm, the delay factors or the bridge settings outside the limits the node otherwise enforces. These values go straight into the radio setup. Changes: - Move the clamping out of loadPrefsInt() into CommonCLI::sanitisePrefs() and call it from both loaders. The clamped fields and limits are the ones the legacy loader already used, with the airtime_factor change below. - airtime_factor is clamped to 0..99 instead of 0..9. "set dutycycle 1" stores 100/1 - 1 = 99, so a limit of 9 would reset any duty cycle below 10 percent on the next boot, now also on the /prefs.json path. "set af" still accepts 0..9 only; that is not touched here. - ConfigSerializer no longer stores a float or double that is not finite. constrain() cannot handle NaN (both comparisons are false), so a token like nan, inf or 1e40 now leaves the field at the default set before the file is read. The check uses the bits, not a comparison, because nRF52 builds use -Ofast. For floats it runs after the narrowing, since a double can overflow to infinity as a float. - loadPrefs() logs a parse error from loadSerial(). The file is still applied and now sanitised. The ConfigSerializer change also applies to the companion NodePrefs, which uses the same serializer. --- src/helpers/CommonCLI.cpp | 80 +++++++++++++++++++------------- src/helpers/CommonCLI.h | 1 + src/helpers/ConfigSerializer.cpp | 33 ++++++++++++- 3 files changed, 80 insertions(+), 34 deletions(-) diff --git a/src/helpers/CommonCLI.cpp b/src/helpers/CommonCLI.cpp index 64fed99b34d..6a801be7d64 100644 --- a/src/helpers/CommonCLI.cpp +++ b/src/helpers/CommonCLI.cpp @@ -27,6 +27,49 @@ static bool isValidName(const char *n) { return true; } +// The binary loader has sanitised these values since before /prefs.json existed. +// Both loaders call it now, so a file that was edited by hand or written by +// another tool cannot leave the node on values its own CLI would refuse. +// Non-finite floats never reach this point: ConfigSerializer keeps the default +// in place for those, and constrain() would pass a NaN through unchanged. +void CommonCLI::sanitisePrefs() { + // sanitise bad pref values + _prefs->rx_delay_base = constrain(_prefs->rx_delay_base, 0, 20.0f); + _prefs->tx_delay_factor = constrain(_prefs->tx_delay_factor, 0, 2.0f); + _prefs->direct_tx_delay_factor = constrain(_prefs->direct_tx_delay_factor, 0, 2.0f); + // 99 is what the CLI can produce, not the 9 of "set af": "set dutycycle 1" + // stores 100/1 - 1. The old limit of 9 predates that command by a year + // (01d84d5d 2025-03-05 against 0aa0ec1f 2026-03-07) and would throw away a + // duty cycle below 10 percent at the next boot. + _prefs->airtime_factor = constrain(_prefs->airtime_factor, 0, 99.0f); + _prefs->freq = constrain(_prefs->freq, 150.0f, 2500.0f); + _prefs->bw = constrain(_prefs->bw, 7.8f, 500.0f); + _prefs->sf = constrain(_prefs->sf, 5, 12); + _prefs->cr = constrain(_prefs->cr, 5, 8); + _prefs->tx_power_dbm = constrain(_prefs->tx_power_dbm, -9, 30); + _prefs->multi_acks = constrain(_prefs->multi_acks, 0, 1); + _prefs->adc_multiplier = constrain(_prefs->adc_multiplier, 0.0f, 10.0f); + _prefs->path_hash_mode = constrain(_prefs->path_hash_mode, 0, 2); // NOTE: mode 3 reserved for future + + // sanitise bad bridge pref values + _prefs->bridge_enabled = constrain(_prefs->bridge_enabled, 0, 1); + _prefs->bridge_delay = constrain(_prefs->bridge_delay, 0, 10000); + _prefs->bridge_pkt_src = constrain(_prefs->bridge_pkt_src, 0, 1); + _prefs->bridge_baud = constrain(_prefs->bridge_baud, 9600, BRIDGE_MAX_BAUD); + _prefs->bridge_channel = constrain(_prefs->bridge_channel, 0, 14); + + _prefs->powersaving_enabled = constrain(_prefs->powersaving_enabled, 0, 1); + + _prefs->gps_enabled = constrain(_prefs->gps_enabled, 0, 1); + _prefs->advert_loc_policy = constrain(_prefs->advert_loc_policy, 0, 2); + + // sanitise settings + _prefs->rx_boosted_gain = constrain(_prefs->rx_boosted_gain, 0, 1); // boolean + _prefs->radio_fem_rxgain = constrain(_prefs->radio_fem_rxgain, 0, 1); // boolean + _prefs->radio_fem_txgain = constrain(_prefs->radio_fem_txgain, 0, 1); // boolean + _prefs->cad_enabled = constrain(_prefs->cad_enabled, 0, 1); // boolean +} + void CommonCLI::loadPrefs(FILESYSTEM* fs) { if (fs->exists("/prefs.json")) { #if defined(RP2040_PLATFORM) @@ -35,8 +78,11 @@ void CommonCLI::loadPrefs(FILESYSTEM* fs) { File file = fs->open("/prefs.json"); #endif if (file) { - _prefs->loadSerial(file); // new Serial prefs + if (!_prefs->loadSerial(file)) { + MESH_DEBUG_PRINTLN("loadPrefs: /prefs.json did not parse cleanly"); + } file.close(); + sanitisePrefs(); } } else if (fs->exists("/com_prefs")) { loadPrefsInt(fs, "/com_prefs"); @@ -104,39 +150,9 @@ void CommonCLI::loadPrefsInt(FILESYSTEM* fs, const char* filename) { // Legacy file.read((uint8_t *)&_prefs->cad_enabled, sizeof(_prefs->cad_enabled)); // 294 // next: 295 - // sanitise bad pref values - _prefs->rx_delay_base = constrain(_prefs->rx_delay_base, 0, 20.0f); - _prefs->tx_delay_factor = constrain(_prefs->tx_delay_factor, 0, 2.0f); - _prefs->direct_tx_delay_factor = constrain(_prefs->direct_tx_delay_factor, 0, 2.0f); - _prefs->airtime_factor = constrain(_prefs->airtime_factor, 0, 9.0f); - _prefs->freq = constrain(_prefs->freq, 150.0f, 2500.0f); - _prefs->bw = constrain(_prefs->bw, 7.8f, 500.0f); - _prefs->sf = constrain(_prefs->sf, 5, 12); - _prefs->cr = constrain(_prefs->cr, 5, 8); - _prefs->tx_power_dbm = constrain(_prefs->tx_power_dbm, -9, 30); - _prefs->multi_acks = constrain(_prefs->multi_acks, 0, 1); - _prefs->adc_multiplier = constrain(_prefs->adc_multiplier, 0.0f, 10.0f); - _prefs->path_hash_mode = constrain(_prefs->path_hash_mode, 0, 2); // NOTE: mode 3 reserved for future - - // sanitise bad bridge pref values - _prefs->bridge_enabled = constrain(_prefs->bridge_enabled, 0, 1); - _prefs->bridge_delay = constrain(_prefs->bridge_delay, 0, 10000); - _prefs->bridge_pkt_src = constrain(_prefs->bridge_pkt_src, 0, 1); - _prefs->bridge_baud = constrain(_prefs->bridge_baud, 9600, BRIDGE_MAX_BAUD); - _prefs->bridge_channel = constrain(_prefs->bridge_channel, 0, 14); - - _prefs->powersaving_enabled = constrain(_prefs->powersaving_enabled, 0, 1); - - _prefs->gps_enabled = constrain(_prefs->gps_enabled, 0, 1); - _prefs->advert_loc_policy = constrain(_prefs->advert_loc_policy, 0, 2); - - // sanitise settings - _prefs->rx_boosted_gain = constrain(_prefs->rx_boosted_gain, 0, 1); // boolean - _prefs->radio_fem_rxgain = constrain(_prefs->radio_fem_rxgain, 0, 1); // boolean - _prefs->radio_fem_txgain = constrain(_prefs->radio_fem_txgain, 0, 1); // boolean - _prefs->cad_enabled = constrain(_prefs->cad_enabled, 0, 1); // boolean file.close(); + sanitisePrefs(); } } diff --git a/src/helpers/CommonCLI.h b/src/helpers/CommonCLI.h index cb85f6f993c..b8cdea2e747 100644 --- a/src/helpers/CommonCLI.h +++ b/src/helpers/CommonCLI.h @@ -338,6 +338,7 @@ class CommonCLI { mesh::RTCClock* getRTCClock() { return _rtc; } void savePrefs(); void loadPrefsInt(FILESYSTEM* _fs, const char* filename); + void sanitisePrefs(); void handleRegionCmd(char* command, char* reply); void handleGetCmd(uint32_t sender_timestamp, char* command, char* reply); diff --git a/src/helpers/ConfigSerializer.cpp b/src/helpers/ConfigSerializer.cpp index daf834b2b10..4e1913b4690 100644 --- a/src/helpers/ConfigSerializer.cpp +++ b/src/helpers/ConfigSerializer.cpp @@ -1,5 +1,27 @@ #include "ConfigSerializer.h" #include // atoi/atol/atof (Arduino.h pulls this in on-device, native builds do not) +#include // memcpy, for the bit check below +#include + +namespace { + +// True if the value is a finite number. NaN and infinity are recognised by +// their bits, not by a comparison: nRF52 builds compile with -Ofast, whose +// -ffast-math lets float comparisons assume that NaN and infinity never +// occur, so a comparison alone may let them through. +bool isFinite(float v) { + uint32_t bits; + memcpy(&bits, &v, sizeof(bits)); + return (bits & 0x7F800000u) != 0x7F800000u; +} + +bool isFinite(double v) { + uint64_t bits; + memcpy(&bits, &v, sizeof(bits)); + return (bits & 0x7FF0000000000000ull) != 0x7FF0000000000000ull; +} + +} // namespace bool ConfigSerializer::saveSerial(Stream& s) { Context context(&s, OP::WRITE); @@ -294,7 +316,11 @@ void ConfigSerializer::def(const char* key, double& value) { } } else { if (_context->keyMatch(_depth, key)) { - value = atof(_context->getToken()); + // A token that is not a finite number is no value: leave what is already + // in the field, which is the default the firmware set up before reading + // the file. + double parsed = atof(_context->getToken()); + if (isFinite(parsed)) value = parsed; } } } @@ -311,7 +337,10 @@ void ConfigSerializer::def(const char* key, float& value) { } } else { if (_context->keyMatch(_depth, key)) { - value = (float) atof(_context->getToken()); + // Checked after the narrowing to float, since a value that is finite as + // a double can overflow to infinity as a float. + float parsed = (float) atof(_context->getToken()); + if (isFinite(parsed)) value = parsed; } } }