From 0841be04fda378cc55cbeef75ca8b11955830150 Mon Sep 17 00:00:00 2001 From: Jonathan Hui Date: Sat, 9 May 2026 10:04:07 -0700 Subject: [PATCH] [posix] detect and fail on unused radio URL parameters (#13087) This commit enhances the radio URL parsing logic to detect and fail when unused parameters are provided in the URL. This prevents typos or unsupported parameters from being silently ignored. The following changes were made: - Updated ot::Url::Url to track parameter usage by appending a trailing '&' delimiter in Init() and replacing it with '\0' in GetValue() when a parameter is matched. This marks the parameter as used and removes any limit on the number of trackable parameters. - Added a Validate() method to ot::Url::Url to verify that all parameters in the query string were accessed. - Refactored ot::Posix::Radio to share a single RadioUrl instance with SpinelManager, ensuring all components track usage on the same URL object. - Integrated Validate() calls in otSysInit() and platformTrelInit() to perform validation after all platform components have been initialized. - Updated Radio::ProcessMaxPowerTable to use a local copy of the parameter string to avoid premature modification of the URL buffer. - Adjusted RadioUrl and unit tests to provide sufficient buffer space for the additional tracking delimiter. - Added new unit tests in tests/unit/test_url.cpp to verify the usage tracking and validation logic. --- src/lib/url/url.cpp | 92 +++++++++++++++++++++++---- src/lib/url/url.hpp | 10 +++ src/posix/platform/hdlc_interface.cpp | 47 +++++++------- src/posix/platform/radio.cpp | 43 ++++++++----- src/posix/platform/radio.hpp | 1 - src/posix/platform/radio_url.cpp | 6 +- src/posix/platform/spinel_manager.hpp | 7 ++ src/posix/platform/system.cpp | 11 ++++ src/posix/platform/trel.cpp | 9 +++ tests/unit/test_url.cpp | 75 ++++++++++++++++++---- 10 files changed, 233 insertions(+), 68 deletions(-) diff --git a/src/lib/url/url.cpp b/src/lib/url/url.cpp index 869c183b2..ee783c3c4 100644 --- a/src/lib/url/url.cpp +++ b/src/lib/url/url.cpp @@ -49,6 +49,7 @@ otError Url::Init(char *aUrl) { otError error = OT_ERROR_NONE; char *url = aUrl; + char *query; mEnd = aUrl + strlen(aUrl); mProtocol = aUrl; @@ -59,15 +60,31 @@ otError Url::Init(char *aUrl) url += sizeof("://") - 1; mPath = url; - url = strstr(url, "?"); + query = strchr(url, '?'); - if (url != nullptr) + if (query != nullptr) { - mQuery = ++url; + char *end; - for (char *cur = strtok(url, "&"); cur != nullptr; cur = strtok(nullptr, "&")) + *query = '\0'; + mQuery = query + 1; + + end = strchr(const_cast(mQuery), '#'); + + if (end == nullptr) { - cur[-1] = '\0'; + end = const_cast(mEnd); + } + + if (end > mQuery && end[-1] != '&') + { + memmove(end + 1, end, mEnd - end + 1); + *end = '&'; + mEnd = end + 1; + } + else + { + mEnd = end; } } else @@ -97,21 +114,40 @@ const char *Url::GetValue(const char *aName, const char *aLastValue) const while (start < mEnd) { - const char *last = nullptr; - - if (!strncmp(aName, start, len)) + if (!strncmp(aName, start, len) && (start[len] == '=' || start[len] == '&' || start[len] == '\0')) { + const char *delimiter = start + len; + + while (delimiter < mEnd && *delimiter != '&' && *delimiter != '\0') + { + delimiter++; + } + + if (delimiter < mEnd && *delimiter == '&') + { + *const_cast(delimiter) = '\0'; + } + if (start[len] == '=') { - ExitNow(rval = &start[len + 1]); + rval = &start[len + 1]; } - else if (start[len] == '\0') + else { - ExitNow(rval = &start[len]); + rval = &start[len]; } + break; + } + + while (start < mEnd && *start != '&' && *start != '\0') + { + start++; + } + + if (start < mEnd) + { + start++; } - last = start; - start = last + strlen(last) + 1; } exit: @@ -202,5 +238,35 @@ exit: return error; } +otError Url::Validate(const char **aUnusedParam) const +{ + otError error = OT_ERROR_NONE; + const char *cur = mQuery; + + while (cur < mEnd) + { + if (*cur == '&') + { + const char *p = cur; + + while (p > mQuery && p[-1] != '\0') + { + p--; + } + + if (aUnusedParam != nullptr) + { + *aUnusedParam = p; + } + + ExitNow(error = OT_ERROR_INVALID_ARGS); + } + cur++; + } + +exit: + return error; +} + } // namespace Url } // namespace ot diff --git a/src/lib/url/url.hpp b/src/lib/url/url.hpp index df1deab49..ec0cc703b 100644 --- a/src/lib/url/url.hpp +++ b/src/lib/url/url.hpp @@ -192,6 +192,16 @@ public: * @retval OT_ERROR_INVALID_ARGS The parameter value was not contain valid number (e.g., value out of range). */ otError ParseInt8(const char *aName, int8_t &aValue) const; + + /** + * Checks if all parameters are used. + * + * @param[out] aUnusedParam A pointer to a string to store the name of the first unused parameter. + * + * @retval OT_ERROR_NONE All parameters were used. + * @retval OT_ERROR_INVALID_ARGS Some parameters were not used. + */ + otError Validate(const char **aUnusedParam = nullptr) const; }; } // namespace Url diff --git a/src/posix/platform/hdlc_interface.cpp b/src/posix/platform/hdlc_interface.cpp index 775722a04..1807e6fd7 100644 --- a/src/posix/platform/hdlc_interface.cpp +++ b/src/posix/platform/hdlc_interface.cpp @@ -685,9 +685,29 @@ exit: #if OPENTHREAD_POSIX_CONFIG_RCP_PTY_ENABLE int HdlcInterface::ForkPty(const Url::Url &aRadioUrl) { - int fd = -1; - int pid = -1; - int rval = -1; + int fd = -1; + int pid = -1; + int rval = -1; + constexpr int kMaxArguments = 32; + char *argv[kMaxArguments + 1]; + size_t index = 0; + + argv[index++] = const_cast(aRadioUrl.GetPath()); + + for (const char *arg = nullptr; + index < OT_ARRAY_LENGTH(argv) && (arg = aRadioUrl.GetValue("forkpty-arg", arg)) != nullptr; + argv[index++] = const_cast(arg)) + { + } + + if (index < OT_ARRAY_LENGTH(argv)) + { + argv[index] = nullptr; + } + else + { + DieNowWithMessage("Too many arguments!", OT_EXIT_INVALID_ARGUMENTS); + } { struct termios tios; @@ -701,27 +721,6 @@ int HdlcInterface::ForkPty(const Url::Url &aRadioUrl) if (0 == pid) { - constexpr int kMaxArguments = 32; - char *argv[kMaxArguments + 1]; - size_t index = 0; - - argv[index++] = const_cast(aRadioUrl.GetPath()); - - for (const char *arg = nullptr; - index < OT_ARRAY_LENGTH(argv) && (arg = aRadioUrl.GetValue("forkpty-arg", arg)) != nullptr; - argv[index++] = const_cast(arg)) - { - } - - if (index < OT_ARRAY_LENGTH(argv)) - { - argv[index] = nullptr; - } - else - { - DieNowWithMessage("Too many arguments!", OT_EXIT_INVALID_ARGUMENTS); - } - VerifyOrDie((rval = execvp(argv[0], argv)) != -1, OT_EXIT_ERROR_ERRNO); } else diff --git a/src/posix/platform/radio.cpp b/src/posix/platform/radio.cpp index 4a525e7d1..05689f8d1 100644 --- a/src/posix/platform/radio.cpp +++ b/src/posix/platform/radio.cpp @@ -63,8 +63,7 @@ extern "C" void platformRadioInit(const char *aUrl) { sRadio.Init(aUrl); } const char Radio::kLogModuleName[] = "Radio"; Radio::Radio(void) - : mRadioUrl(nullptr) - , mRadioSpinel() + : mRadioSpinel() #if OPENTHREAD_POSIX_CONFIG_RCP_CAPS_DIAG_ENABLE , mRcpCapsDiag(mRadioSpinel) #endif @@ -80,8 +79,10 @@ OT_TOOL_WEAK void platformCoprocessorResetFailed(void *) {} void Radio::Init(const char *aUrl) { - bool resetRadio; - bool skipCompatibilityCheck; + bool resetRadio; + bool skipCompatibilityCheck; + RadioUrl &radioUrl = SpinelManager::GetSpinelManager().GetRadioUrl(); + #if OPENTHREAD_CONFIG_THREAD_VERSION >= OT_THREAD_VERSION_1_2 && OPENTHREAD_CONFIG_MAC_CSL_TRANSMITTER_ENABLE bool aEnableRcpTimeSync = true; #else @@ -89,8 +90,8 @@ void Radio::Init(const char *aUrl) #endif struct ot::Spinel::RadioSpinelCallbacks callbacks; - mRadioUrl.Init(aUrl); - VerifyOrDie(mRadioUrl.GetPath() != nullptr, OT_EXIT_INVALID_ARGUMENTS); + VerifyOrDie(radioUrl.GetPath() != nullptr, OT_EXIT_INVALID_ARGUMENTS); + OT_UNUSED_VARIABLE(aUrl); memset(&callbacks, 0, sizeof(callbacks)); callbacks.mEnergyScanDone = otPlatRadioEnergyScanDone; @@ -105,8 +106,8 @@ void Radio::Init(const char *aUrl) mTmpStorage.Init(); #endif - resetRadio = !mRadioUrl.HasParam("no-reset"); - skipCompatibilityCheck = mRadioUrl.HasParam("skip-rcp-compatibility-check"); + resetRadio = !radioUrl.HasParam("no-reset"); + skipCompatibilityCheck = radioUrl.HasParam("skip-rcp-compatibility-check"); #if OPENTHREAD_SPINEL_CONFIG_COPROCESSOR_RESET_FAILURE_CALLBACK_ENABLE GetSpinelDriver().SetCoprocessorResetFailureCallback(platformCoprocessorResetFailed, this); @@ -116,7 +117,7 @@ void Radio::Init(const char *aUrl) mRadioSpinel.Init(skipCompatibilityCheck, resetRadio, &GetSpinelDriver(), (skipCompatibilityCheck ? 0 : kRequiredRadioCaps), aEnableRcpTimeSync); - ProcessRadioUrl(mRadioUrl); + ProcessRadioUrl(radioUrl); } void Radio::Deinit(void) @@ -201,17 +202,21 @@ void Radio::ProcessMaxPowerTable(const RadioUrl &aRadioUrl) #if OPENTHREAD_POSIX_CONFIG_MAX_POWER_TABLE_ENABLE otError error; - constexpr int8_t kPowerDefault = 30; // Default power 1 watt (30 dBm). - const char *str = nullptr; - char *pSave = nullptr; - uint8_t channel = ot::Radio::kChannelMin; - int8_t power = kPowerDefault; - const char *maxPowerTable; + constexpr int8_t kPowerDefault = 30; // Default power 1 watt (30 dBm). + char *str = nullptr; + char *pSave = nullptr; + uint8_t channel = ot::Radio::kChannelMin; + int8_t power = kPowerDefault; + const char *maxPowerTable = nullptr; + char *maxPowerTableCopy = nullptr; VerifyOrExit((maxPowerTable = aRadioUrl.GetValue("max-power-table")) != nullptr); - for (str = strtok_r(const_cast(maxPowerTable), ",", &pSave); - str != nullptr && channel <= ot::Radio::kChannelMax; str = strtok_r(nullptr, ",", &pSave)) + maxPowerTableCopy = strdup(maxPowerTable); + VerifyOrDie(maxPowerTableCopy != nullptr, OT_EXIT_FAILURE); + + for (str = strtok_r(maxPowerTableCopy, ",", &pSave); str != nullptr && channel <= ot::Radio::kChannelMax; + str = strtok_r(nullptr, ",", &pSave)) { power = static_cast(strtol(str, nullptr, 0)); error = mRadioSpinel.SetChannelMaxTransmitPower(channel, power); @@ -240,6 +245,10 @@ void Radio::ProcessMaxPowerTable(const RadioUrl &aRadioUrl) VerifyOrDie(str == nullptr, OT_EXIT_INVALID_ARGUMENTS); exit: + if (maxPowerTableCopy != nullptr) + { + free(maxPowerTableCopy); + } return; #endif // OPENTHREAD_POSIX_CONFIG_MAX_POWER_TABLE_ENABLE } diff --git a/src/posix/platform/radio.hpp b/src/posix/platform/radio.hpp index a24d269a6..aa2492054 100644 --- a/src/posix/platform/radio.hpp +++ b/src/posix/platform/radio.hpp @@ -140,7 +140,6 @@ private: #endif OT_RADIO_CAPS_ACK_TIMEOUT | OT_RADIO_CAPS_TRANSMIT_RETRIES | OT_RADIO_CAPS_CSMA_BACKOFF; - RadioUrl mRadioUrl; #if OPENTHREAD_SPINEL_CONFIG_VENDOR_HOOK_ENABLE Spinel::VendorRadioSpinel mRadioSpinel; #else diff --git a/src/posix/platform/radio_url.cpp b/src/posix/platform/radio_url.cpp index 08f34ad96..3de487250 100644 --- a/src/posix/platform/radio_url.cpp +++ b/src/posix/platform/radio_url.cpp @@ -149,8 +149,10 @@ void RadioUrl::Init(const char *aUrl) { if (aUrl != nullptr) { - VerifyOrDie(strnlen(aUrl, sizeof(mUrl)) < sizeof(mUrl), OT_EXIT_INVALID_ARGUMENTS); - strncpy(mUrl, aUrl, sizeof(mUrl) - 1); + size_t len = strlen(aUrl); + + VerifyOrDie(len + 1 < sizeof(mUrl), OT_EXIT_INVALID_ARGUMENTS); + strcpy(mUrl, aUrl); SuccessOrDie(Url::Url::Init(mUrl)); } } diff --git a/src/posix/platform/spinel_manager.hpp b/src/posix/platform/spinel_manager.hpp index e2af9b473..f5702f206 100644 --- a/src/posix/platform/spinel_manager.hpp +++ b/src/posix/platform/spinel_manager.hpp @@ -91,6 +91,13 @@ public: return *mSpinelInterface; } + /** + * Returns the radio URL. + * + * @returns The radio URL. + */ + RadioUrl &GetRadioUrl(void) { return mUrl; } + private: #if OPENTHREAD_POSIX_VIRTUAL_TIME void VirtualTimeInit(void); diff --git a/src/posix/platform/system.cpp b/src/posix/platform/system.cpp index 8decdb14d..ec63dfda7 100644 --- a/src/posix/platform/system.cpp +++ b/src/posix/platform/system.cpp @@ -59,6 +59,7 @@ #include "posix/platform/mdns_socket.hpp" #include "posix/platform/radio_url.hpp" #include "posix/platform/spinel_driver_getter.hpp" +#include "posix/platform/spinel_manager.hpp" #include "posix/platform/udp.hpp" otInstance *gInstance = nullptr; @@ -275,6 +276,16 @@ otInstance *otSysInit(otPlatformConfig *aPlatformConfig) platformInit(aPlatformConfig); + { + const char *unusedParam = nullptr; + + if (ot::Posix::SpinelManager::GetSpinelManager().GetRadioUrl().Validate(&unusedParam) != OT_ERROR_NONE) + { + otLogCritPlat("Radio URL contains unused parameter: \"%s\"", unusedParam); + DieNow(OT_EXIT_INVALID_ARGUMENTS); + } + } + gDryRun = aPlatformConfig->mDryRun; if (sCoprocessorType == OT_COPROCESSOR_RCP) { diff --git a/src/posix/platform/trel.cpp b/src/posix/platform/trel.cpp index 57b0da736..f2d2d8e0d 100644 --- a/src/posix/platform/trel.cpp +++ b/src/posix/platform/trel.cpp @@ -658,6 +658,15 @@ void platformTrelInit(const char *aTrelUrl) ot::Posix::RadioUrl url(aTrelUrl); otSysTrelInit(url.GetPath()); + { + const char *unusedParam = nullptr; + + if (url.Validate(&unusedParam) != OT_ERROR_NONE) + { + otLogCritPlat("TREL radio URL contains unused parameter: \"%s\"", unusedParam); + DieNow(OT_EXIT_INVALID_ARGUMENTS); + } + } } } diff --git a/tests/unit/test_url.cpp b/tests/unit/test_url.cpp index 63c2e88cf..a0f57ade9 100644 --- a/tests/unit/test_url.cpp +++ b/tests/unit/test_url.cpp @@ -36,7 +36,7 @@ namespace Url { void TestSimple(void) { - char url[] = "spinel:///dev/ttyUSB0?baudrate=460800"; + char url[100] = "spinel:///dev/ttyUSB0?baudrate=460800"; Url args; VerifyOrQuit(!args.Init(url)); @@ -53,7 +53,7 @@ void TestSimple(void) void TestSimpleNoQueryString(void) { - char url[] = "spinel:///dev/ttyUSB0"; + char url[100] = "spinel:///dev/ttyUSB0"; Url args; VerifyOrQuit(!args.Init(url)); @@ -67,7 +67,7 @@ void TestSimpleNoQueryString(void) void TestEmptyValue(void) { - char url[] = "spinel:///dev/ttyUSB0?rtscts&baudrate=460800&verbose&verbose&verbose"; + char url[100] = "spinel:///dev/ttyUSB0?rtscts&baudrate=460800&verbose&verbose&verbose"; Url args; const char *arg = nullptr; @@ -85,7 +85,7 @@ void TestEmptyValue(void) void TestMultipleProtocols(void) { - char url[] = "spinel+spi:///dev/ttyUSB0?baudrate=460800"; + char url[100] = "spinel+spi:///dev/ttyUSB0?baudrate=460800"; Url args; VerifyOrQuit(!args.Init(url)); @@ -97,7 +97,7 @@ void TestMultipleProtocols(void) void TestMultipleProtocolsAndDuplicateParameters(void) { - char url[] = "spinel+exec:///path/to/ot-rcp?arg=1&arg=arg2&arg=3"; + char url[100] = "spinel+exec:///path/to/ot-rcp?arg=1&arg=arg2&arg=3"; Url args; const char *arg = nullptr; @@ -122,9 +122,9 @@ void TestMultipleProtocolsAndDuplicateParameters(void) void TestIntValue(void) { - char int8url[] = "spinel:///dev/ttyUSB0?no-reset&val1=1&val2=0x02&val3=-0X03&val4=-4&val5=+5&val6=128&val7=-129"; - char int16url[] = "spinel:///dev/ttyUSB0?val1=1&val2=0x02&val3=-0X03&val4=-400&val5=+500&val6=32768&val7=-32769"; - char int32url[] = + char int8url[200] = "spinel:///dev/ttyUSB0?no-reset&val1=1&val2=0x02&val3=-0X03&val4=-4&val5=+5&val6=128&val7=-129"; + char int16url[200] = "spinel:///dev/ttyUSB0?val1=1&val2=0x02&val3=-0X03&val4=-400&val5=+500&val6=32768&val7=-32769"; + char int32url[200] = "spinel:///dev/ttyUSB0?val1=1&val2=0x02&val3=-0X03&val4=-40000&val5=+50000&val6=2147483648&val7=-2147483649"; Url args; int8_t int8val; @@ -195,9 +195,9 @@ void TestIntValue(void) void TestUintValue(void) { - char uint8url[] = "spinel:///dev/ttyUSB0?no-reset&val1=1&val2=0x02&val3=0X03&val4=-4&val5=+5&val6=256&val7=-1"; - char uint16url[] = "spinel:///dev/ttyUSB0?val1=1&val2=0x02&val3=0X03&val4=-400&val5=+500&val6=65536&val7=-1"; - char uint32url[] = + char uint8url[200] = "spinel:///dev/ttyUSB0?no-reset&val1=1&val2=0x02&val3=0X03&val4=-4&val5=+5&val6=256&val7=-1"; + char uint16url[200] = "spinel:///dev/ttyUSB0?val1=1&val2=0x02&val3=0X03&val4=-400&val5=+500&val6=65536&val7=-1"; + char uint32url[200] = "spinel:///dev/ttyUSB0?val1=1&val2=0x02&val3=0X03&val4=-40000&val5=+70000&val6=4294967296&val7=-1"; Url args; uint8_t uint8val; @@ -264,6 +264,58 @@ void TestUintValue(void) printf("PASS %s\r\n", __func__); } +void TestValidate(void) +{ + char url[100] = "spinel:///dev/ttyUSB0?val1=1&val2=0x02&val3=0X03"; + Url args; + const char *unusedParam = nullptr; + + VerifyOrQuit(!args.Init(url)); + + // Test idempotency + VerifyOrQuit(args.Validate(&unusedParam) == OT_ERROR_INVALID_ARGS); + VerifyOrQuit(!strncmp(unusedParam, "val1=1", 6)); + VerifyOrQuit(args.Validate(&unusedParam) == OT_ERROR_INVALID_ARGS); + VerifyOrQuit(!strncmp(unusedParam, "val1=1", 6)); + + VerifyOrQuit(!strcmp(args.GetValue("val1"), "1")); + VerifyOrQuit(args.Validate(&unusedParam) == OT_ERROR_INVALID_ARGS); + VerifyOrQuit(!strncmp(unusedParam, "val2=0x02", 9)); + + VerifyOrQuit(!strcmp(args.GetValue("val2"), "0x02")); + VerifyOrQuit(args.Validate(&unusedParam) == OT_ERROR_INVALID_ARGS); + VerifyOrQuit(!strncmp(unusedParam, "val3=0X03", 9)); + + VerifyOrQuit(!strcmp(args.GetValue("val3"), "0X03")); + VerifyOrQuit(args.Validate(&unusedParam) == OT_ERROR_NONE); + + // Test with multiple same-named parameters + { + char url2[100] = "spinel:///dev/tty?arg=1&arg=2&val=3"; + Url args2; + + VerifyOrQuit(!args2.Init(url2)); + VerifyOrQuit(args2.Validate(&unusedParam) == OT_ERROR_INVALID_ARGS); + VerifyOrQuit(!strncmp(unusedParam, "arg=1", 5)); + + const char *val = args2.GetValue("arg"); + VerifyOrQuit(!strcmp(val, "1")); + VerifyOrQuit(args2.Validate(&unusedParam) == OT_ERROR_INVALID_ARGS); + VerifyOrQuit(!strncmp(unusedParam, "arg=2", 5)); + + val = args2.GetValue("arg", val); + VerifyOrQuit(!strcmp(val, "2")); + VerifyOrQuit(args2.Validate(&unusedParam) == OT_ERROR_INVALID_ARGS); + VerifyOrQuit(!strncmp(unusedParam, "val=3", 5)); + + val = args2.GetValue("val"); + VerifyOrQuit(!strcmp(val, "3")); + VerifyOrQuit(args2.Validate(&unusedParam) == OT_ERROR_NONE); + } + + printf("PASS %s\r\n", __func__); +} + } // namespace Url } // namespace ot @@ -276,6 +328,7 @@ int main(void) ot::Url::TestMultipleProtocolsAndDuplicateParameters(); ot::Url::TestIntValue(); ot::Url::TestUintValue(); + ot::Url::TestValidate(); return 0; }