[diags] fix parsing command line and enhancements (#2480)

This commit contains fixes and enchantments for diagnostics module
in OpenThread. In particular, the following changes are made to the
implementation of `otDiagProcessCmdLine()`:

- Fixes an issue with the parsing of command line input (where if
  more than 8 arguments were given, it could cause an out-of bound
  array access and a possible NCP crash). The new code will check for
  this and outputs an error if too many arguments are provided.
- Parsing logic is simplified and now allows for extra spaces between
  arguments.
- A local buffer is used to store the arguments to avoid modifying
  the passed-in input string.

This commit also simplifies and does a code clean-up of `Diag`
class (variable name changes, removing extra method paramters, and
minor style changes).

The `test_diag` unit test implementation is also updated:

- It includes new test commands.
- The test prints/logs the issued input commands and their
  corresponding output (as human-readable printable strings).

include/openthread/diag.h

@@ -62,23 +62,23 @@ void otDiagInit(otInstance *aInstance);
 /**
  * This function processes a factory diagnostics command line.
  *
- * @param[in]  aArgc  The argument counter of diagnostics command line.
- * @param[in]  aArgv  The argument vector of diagnostics command line.
+ * @param[in]  aArgCount   The argument counter of diagnostics command line.
+ * @param[in]  aArgVector  The argument vector of diagnostics command line.
  *
  * @returns A pointer to the output string.
  *
  */
-char *otDiagProcessCmd(int aArgc, char *aArgv[]);
+const char *otDiagProcessCmd(int aArgCount, char *aArgVector[]);
 
 /**
  * This function processes a factory diagnostics command line.
  *
- * @param[in]  aString  A NULL-terminated string.
+ * @param[in]  aString  A NULL-terminated input string.
  *
  * @returns A pointer to the output string.
  *
  */
-char *otDiagProcessCmdLine(char *aString);
+const char *otDiagProcessCmdLine(const char *aString);
 
 /**
  * This function indicates whether or not the factory diagnostics mode is enabled.

src/diag/diag_process.cpp

@@ -44,7 +44,7 @@
 namespace ot {
 namespace Diagnostics {
 
-const struct Command Diag::sCommands[] =
+const struct Diag::Command Diag::sCommands[] =
 {
     { "start", &ProcessStart },
     { "stop", &ProcessStop },
@@ -53,10 +53,11 @@ const struct Command Diag::sCommands[] =
     { "send", &ProcessSend },
     { "repeat", &ProcessRepeat },
     { "stats", &ProcessStats },
+    { NULL, NULL },
 };
 
-char Diag::sDiagOutput[MAX_DIAG_OUTPUT];
-struct DiagStats Diag::sStats;
+char Diag::sOutput[Diag::kMaxOutputSize];
+struct Diag::DiagStats Diag::sStats;
 
 int8_t Diag::sTxPower;
 uint8_t Diag::sChannel;
@@ -65,12 +66,11 @@ uint32_t Diag::sTxPeriod;
 uint32_t Diag::sTxPackets;
 otRadioFrame * Diag::sTxPacket;
 bool Diag::sRepeatActive;
-
-otInstance *Diag::sContext;
+otInstance *Diag::sInstance;
 
 void Diag::Init(otInstance *aInstance)
 {
-    sContext = aInstance;
+    sInstance = aInstance;
     sChannel = 20;
     sTxPower = 0;
     sTxPeriod = 0;
@@ -78,110 +78,98 @@ void Diag::Init(otInstance *aInstance)
     sTxPackets = 0;
     sRepeatActive = false;
     memset(&sStats, 0, sizeof(struct DiagStats));
-    sTxPacket = otPlatRadioGetTransmitBuffer(sContext);
+    sTxPacket = otPlatRadioGetTransmitBuffer(sInstance);
     otPlatDiagChannelSet(sChannel);
     otPlatDiagTxPowerSet(sTxPower);
 }
 
-char *Diag::ProcessCmd(int argc, char *argv[])
+const char *Diag::ProcessCmd(int aArgCount, char *aArgVector[])
 {
-    unsigned int i;
-
-    if (argc == 0)
+    if (aArgCount == 0)
     {
-        snprintf(sDiagOutput, sizeof(sDiagOutput), "diagnostics mode is %s\r\n", otPlatDiagModeGet() ? "enabled" : "disabled");
+        snprintf(sOutput, sizeof(sOutput), "diagnostics mode is %s\r\n", otPlatDiagModeGet() ? "enabled" : "disabled");
+        ExitNow();
     }
-    else
-    {
-        for (i = 0; i < sizeof(sCommands) / sizeof(sCommands[0]); i++)
-        {
-            if (strcmp(argv[0], sCommands[i].mName) == 0)
-            {
-                sCommands[i].mCommand(argc - 1, argc > 1 ? &argv[1] : NULL, sDiagOutput, sizeof(sDiagOutput));
-                break;
-            }
-        }
 
-        // more platform specific features will be processed under platform layer
-        if (i == sizeof(sCommands) / sizeof(sCommands[0]))
+    for (const Command *command = &sCommands[0]; command->mName != NULL; command++)
+    {
+        if (strcmp(aArgVector[0], command->mName) == 0)
         {
-            otPlatDiagProcess(sContext, argc, argv, sDiagOutput, sizeof(sDiagOutput));
+            command->mHandler(aArgCount - 1, (aArgCount > 1) ? &aArgVector[1] : NULL);
+            ExitNow();
         }
     }
 
-    return sDiagOutput;
+    // more platform specific features will be processed under platform layer
+    otPlatDiagProcess(sInstance, aArgCount, aArgVector, sOutput, sizeof(sOutput));
+
+exit:
+    return sOutput;
 }
 
-bool Diag::isEnabled()
+bool Diag::IsEnabled(void)
 {
     return otPlatDiagModeGet();
 }
 
-void Diag::AppendErrorResult(otError aError, char *aOutput, size_t aOutputMaxLen)
+void Diag::AppendErrorResult(otError aError)
 {
     if (aError != OT_ERROR_NONE)
     {
-        snprintf(aOutput, aOutputMaxLen, "failed\r\nstatus %#x\r\n", aError);
+        snprintf(sOutput, sizeof(sOutput), "failed\r\nstatus %#x\r\n", aError);
     }
 }
 
-void Diag::ProcessStart(int argc, char *argv[], char *aOutput, size_t aOutputMaxLen)
+void Diag::ProcessStart(int aArgCount, char *aArgVector[])
 {
     otError error = OT_ERROR_NONE;
 
-    // enable radio
-    otPlatRadioEnable(sContext);
+    OT_UNUSED_VARIABLE(aArgCount);
+    OT_UNUSED_VARIABLE(aArgVector);
 
-    // enable promiscuous mode
-    otPlatRadioSetPromiscuous(sContext, true);
-
-    // stop timer
-    otPlatAlarmMilliStop(sContext);
-
-    // start to listen on the default channel
-    SuccessOrExit(error = otPlatRadioReceive(sContext, sChannel));
-
-    // enable diagnostics mode
+    otPlatRadioEnable(sInstance);
+    otPlatRadioSetPromiscuous(sInstance, true);
+    otPlatAlarmMilliStop(sInstance);
+    SuccessOrExit(error = otPlatRadioReceive(sInstance, sChannel));
     otPlatDiagModeSet(true);
-
     memset(&sStats, 0, sizeof(struct DiagStats));
-
-    snprintf(aOutput, aOutputMaxLen, "start diagnostics mode\r\nstatus 0x%02x\r\n", error);
+    snprintf(sOutput, sizeof(sOutput), "start diagnostics mode\r\nstatus 0x%02x\r\n", error);
 
 exit:
-    OT_UNUSED_VARIABLE(argc);
-    OT_UNUSED_VARIABLE(argv);
-    AppendErrorResult(error, aOutput, aOutputMaxLen);
+    AppendErrorResult(error);
 }
 
-void Diag::ProcessStop(int argc, char *argv[], char *aOutput, size_t aOutputMaxLen)
+void Diag::ProcessStop(int aArgCount, char *aArgVector[])
 {
     otError error = OT_ERROR_NONE;
 
+    OT_UNUSED_VARIABLE(aArgCount);
+    OT_UNUSED_VARIABLE(aArgVector);
+
     VerifyOrExit(otPlatDiagModeGet(), error = OT_ERROR_INVALID_STATE);
 
-    otPlatAlarmMilliStop(sContext);
+    otPlatAlarmMilliStop(sInstance);
     otPlatDiagModeSet(false);
-    otPlatRadioSetPromiscuous(sContext, false);
+    otPlatRadioSetPromiscuous(sInstance, false);
 
-    snprintf(aOutput, aOutputMaxLen, "received packets: %d\r\nsent packets: %d\r\nfirst received packet: rssi=%d, lqi=%d\r\n\nstop diagnostics mode\r\nstatus 0x%02x\r\n",
-            static_cast<int>(sStats.received_packets), static_cast<int>(sStats.sent_packets), static_cast<int>(sStats.first_rssi),
-            static_cast<int>(sStats.first_lqi), error);
+    snprintf(sOutput, sizeof(sOutput),
+             "received packets: %d\r\nsent packets: %d\r\nfirst received packet: rssi=%d, lqi=%d\r\n"
+             "\nstop diagnostics mode\r\nstatus 0x%02x\r\n",
+             static_cast<int>(sStats.mReceivedPackets), static_cast<int>(sStats.mSentPackets),
+             static_cast<int>(sStats.mFirstRssi), static_cast<int>(sStats.mFirstLqi), error);
 
 exit:
-    OT_UNUSED_VARIABLE(argc);
-    OT_UNUSED_VARIABLE(argv);
-    AppendErrorResult(error, aOutput, aOutputMaxLen);
+    AppendErrorResult(error);
 }
 
-otError Diag::ParseLong(char *argv, long &value)
+otError Diag::ParseLong(char *aArgVector, long &aValue)
 {
     char *endptr;
-    value = strtol(argv, &endptr, 0);
+    aValue = strtol(aArgVector, &endptr, 0);
     return (*endptr == '\0') ? OT_ERROR_NONE : OT_ERROR_PARSE;
 }
 
-void Diag::TxPacket()
+void Diag::TxPacket(void)
 {
     sTxPacket->mLength = sTxLen;
     sTxPacket->mChannel = sChannel;
@@ -190,142 +178,150 @@ void Diag::TxPacket()
     {
         sTxPacket->mPsdu[i] = i;
     }
-    otPlatRadioTransmit(sContext, sTxPacket);
+
+    otPlatRadioTransmit(sInstance, sTxPacket);
 }
 
-void Diag::ProcessChannel(int argc, char *argv[], char *aOutput, size_t aOutputMaxLen)
+void Diag::ProcessChannel(int aArgCount, char *aArgVector[])
 {
     otError error = OT_ERROR_NONE;
 
+
     VerifyOrExit(otPlatDiagModeGet(), error = OT_ERROR_INVALID_STATE);
 
-    if (argc == 0)
+    if (aArgCount == 0)
     {
-        snprintf(aOutput, aOutputMaxLen, "channel: %d\r\n", sChannel);
+        snprintf(sOutput, sizeof(sOutput), "channel: %d\r\n", sChannel);
     }
     else
     {
         long value;
 
-        SuccessOrExit(error = ParseLong(argv[0], value));
+        SuccessOrExit(error = ParseLong(aArgVector[0], value));
         VerifyOrExit(value >= OT_RADIO_CHANNEL_MIN && value <= OT_RADIO_CHANNEL_MAX, error = OT_ERROR_INVALID_ARGS);
-        sChannel = static_cast<uint8_t>(value);
 
-        // listen on the set channel immediately
-        otPlatRadioReceive(sContext, sChannel);
+        sChannel = static_cast<uint8_t>(value);
+        otPlatRadioReceive(sInstance, sChannel);
         otPlatDiagChannelSet(sChannel);
-        snprintf(aOutput, aOutputMaxLen, "set channel to %d\r\nstatus 0x%02x\r\n", sChannel, error);
+
+        snprintf(sOutput, sizeof(sOutput), "set channel to %d\r\nstatus 0x%02x\r\n", sChannel, error);
     }
 
 exit:
-    AppendErrorResult(error, aOutput, aOutputMaxLen);
+    AppendErrorResult(error);
 }
 
-void Diag::ProcessPower(int argc, char *argv[], char *aOutput, size_t aOutputMaxLen)
+void Diag::ProcessPower(int aArgCount, char *aArgVector[])
 {
     otError error = OT_ERROR_NONE;
 
     VerifyOrExit(otPlatDiagModeGet(), error = OT_ERROR_INVALID_STATE);
 
-    if (argc == 0)
+    if (aArgCount == 0)
     {
-        snprintf(aOutput, aOutputMaxLen, "tx power: %d dBm\r\n", sTxPower);
+        snprintf(sOutput, sizeof(sOutput), "tx power: %d dBm\r\n", sTxPower);
     }
     else
     {
         long value;
 
-        SuccessOrExit(error = ParseLong(argv[0], value));
+        SuccessOrExit(error = ParseLong(aArgVector[0], value));
+
         sTxPower = static_cast<int8_t>(value);
         otPlatDiagTxPowerSet(sTxPower);
-        snprintf(aOutput, aOutputMaxLen, "set tx power to %d dBm\r\nstatus 0x%02x\r\n", sTxPower, error);
+
+        snprintf(sOutput, sizeof(sOutput), "set tx power to %d dBm\r\nstatus 0x%02x\r\n", sTxPower, error);
     }
 
 exit:
-    AppendErrorResult(error, aOutput, aOutputMaxLen);
+    AppendErrorResult(error);
 }
 
-void Diag::ProcessSend(int argc, char *argv[], char *aOutput, size_t aOutputMaxLen)
+void Diag::ProcessSend(int aArgCount, char *aArgVector[])
 {
     otError error = OT_ERROR_NONE;
     long value;
 
     VerifyOrExit(otPlatDiagModeGet(), error = OT_ERROR_INVALID_STATE);
-    VerifyOrExit(argc == 2, error = OT_ERROR_INVALID_ARGS);
+    VerifyOrExit(aArgCount == 2, error = OT_ERROR_INVALID_ARGS);
 
-    SuccessOrExit(error = ParseLong(argv[0], value));
+    SuccessOrExit(error = ParseLong(aArgVector[0], value));
     sTxPackets = static_cast<uint32_t>(value);
 
-    SuccessOrExit(error = ParseLong(argv[1], value));
+    SuccessOrExit(error = ParseLong(aArgVector[1], value));
     VerifyOrExit(value <= OT_RADIO_FRAME_MAX_SIZE, error = OT_ERROR_INVALID_ARGS);
     sTxLen = static_cast<uint8_t>(value);
 
-    snprintf(aOutput, aOutputMaxLen, "sending %#x packet(s), length %#x\r\nstatus 0x%02x\r\n", static_cast<int>(sTxPackets), static_cast<int>(sTxLen), error);
+    snprintf(sOutput, sizeof(sOutput), "sending %#x packet(s), length %#x\r\nstatus 0x%02x\r\n",
+             static_cast<int>(sTxPackets), static_cast<int>(sTxLen), error);
     TxPacket();
 
 exit:
-    AppendErrorResult(error, aOutput, aOutputMaxLen);
+    AppendErrorResult(error);
 }
 
-void Diag::ProcessRepeat(int argc, char *argv[], char *aOutput, size_t aOutputMaxLen)
+void Diag::ProcessRepeat(int aArgCount, char *aArgVector[])
 {
     otError error = OT_ERROR_NONE;
 
     VerifyOrExit(otPlatDiagModeGet(), error = OT_ERROR_INVALID_STATE);
-    VerifyOrExit(argc > 0, error = OT_ERROR_INVALID_ARGS);
+    VerifyOrExit(aArgCount > 0, error = OT_ERROR_INVALID_ARGS);
 
-    if (strcmp(argv[0], "stop") == 0)
+    if (strcmp(aArgVector[0], "stop") == 0)
     {
-        otPlatAlarmMilliStop(sContext);
+        otPlatAlarmMilliStop(sInstance);
         sRepeatActive = false;
-        snprintf(aOutput, aOutputMaxLen, "repeated packet transmission is stopped\r\nstatus 0x%02x\r\n", error);
+        snprintf(sOutput, sizeof(sOutput), "repeated packet transmission is stopped\r\nstatus 0x%02x\r\n", error);
     }
     else
     {
         long value;
 
-        VerifyOrExit(argc == 2, error = OT_ERROR_INVALID_ARGS);
+        VerifyOrExit(aArgCount == 2, error = OT_ERROR_INVALID_ARGS);
 
-        SuccessOrExit(error = ParseLong(argv[0], value));
+        SuccessOrExit(error = ParseLong(aArgVector[0], value));
         sTxPeriod = static_cast<uint32_t>(value);
 
-        SuccessOrExit(error = ParseLong(argv[1], value));
+        SuccessOrExit(error = ParseLong(aArgVector[1], value));
         VerifyOrExit(value <= OT_RADIO_FRAME_MAX_SIZE, error = OT_ERROR_INVALID_ARGS);
         sTxLen = static_cast<uint8_t>(value);
 
         sRepeatActive = true;
         uint32_t now = otPlatAlarmMilliGetNow();
-        otPlatAlarmMilliStartAt(sContext, now, sTxPeriod);
-        snprintf(aOutput, aOutputMaxLen, "sending packets of length %#x at the delay of %#x ms\r\nstatus 0x%02x\r\n", static_cast<int>(sTxLen), static_cast<int>(sTxPeriod), error);
+        otPlatAlarmMilliStartAt(sInstance, now, sTxPeriod);
+        snprintf(sOutput, sizeof(sOutput), "sending packets of length %#x at the delay of %#x ms\r\nstatus 0x%02x\r\n",
+                 static_cast<int>(sTxLen), static_cast<int>(sTxPeriod), error);
     }
 
 exit:
-    AppendErrorResult(error, aOutput, aOutputMaxLen);
+    AppendErrorResult(error);
 }
 
-void Diag::ProcessStats(int argc, char *argv[], char *aOutput, size_t aOutputMaxLen)
+void Diag::ProcessStats(int aArgCount, char *aArgVector[])
 {
     otError error = OT_ERROR_NONE;
 
+    OT_UNUSED_VARIABLE(aArgCount);
+    OT_UNUSED_VARIABLE(aArgVector);
+
     VerifyOrExit(otPlatDiagModeGet(), error = OT_ERROR_INVALID_STATE);
 
-    snprintf(aOutput, aOutputMaxLen, "received packets: %d\r\nsent packets: %d\r\nfirst received packet: rssi=%d, lqi=%d\r\n",
-            static_cast<int>(sStats.received_packets), static_cast<int>(sStats.sent_packets),
-            static_cast<int>(sStats.first_rssi), static_cast<int>(sStats.first_lqi));
+    snprintf(sOutput, sizeof(sOutput),
+             "received packets: %d\r\nsent packets: %d\r\nfirst received packet: rssi=%d, lqi=%d\r\n",
+            static_cast<int>(sStats.mReceivedPackets), static_cast<int>(sStats.mSentPackets),
+            static_cast<int>(sStats.mFirstRssi), static_cast<int>(sStats.mFirstLqi));
 
 exit:
-    OT_UNUSED_VARIABLE(argc);
-    OT_UNUSED_VARIABLE(argv);
-    AppendErrorResult(error, aOutput, aOutputMaxLen);
+    AppendErrorResult(error);
 }
 
 void Diag::DiagTransmitDone(otInstance *aInstance, otError aError)
 {
-    VerifyOrExit(aInstance == sContext);
+    VerifyOrExit(aInstance == sInstance);
 
     if (aError == OT_ERROR_NONE)
     {
-        sStats.sent_packets++;
+        sStats.mSentPackets++;
 
         if (sTxPackets > 1)
         {
@@ -344,18 +340,18 @@ exit:
 
 void Diag::DiagReceiveDone(otInstance *aInstance, otRadioFrame *aFrame, otError aError)
 {
-    VerifyOrExit(aInstance == sContext);
+    VerifyOrExit(aInstance == sInstance);
 
     if (aError == OT_ERROR_NONE)
     {
         // for sensitivity test, only record the rssi and lqi for the first packet
-        if (sStats.received_packets == 0)
+        if (sStats.mReceivedPackets == 0)
         {
-            sStats.first_rssi = aFrame->mRssi;
-            sStats.first_lqi = aFrame->mLqi;
+            sStats.mFirstRssi = aFrame->mRssi;
+            sStats.mFirstLqi = aFrame->mLqi;
         }
 
-        sStats.received_packets++;
+        sStats.mReceivedPackets++;
     }
     otPlatDiagRadioReceived(aInstance, aFrame, aError);
 
@@ -365,7 +361,7 @@ exit:
 
 void Diag::AlarmFired(otInstance *aInstance)
 {
-    VerifyOrExit(aInstance == sContext);
+    VerifyOrExit(aInstance == sInstance);
 
     if(sRepeatActive)
     {

src/diag/diag_process.hpp

@@ -47,46 +47,50 @@ namespace ot {
 
 namespace Diagnostics {
 
-#define MAX_DIAG_OUTPUT 256
-
-struct Command
-{
-    const char *mName;                         ///< A pointer to the command string.
-    void (*mCommand)(int argc, char *argv[], char *aOutput, size_t aOutputMaxLen);  ///< A function pointer to process the command.
-};
-
-struct DiagStats
-{
-    uint32_t received_packets;
-    uint32_t sent_packets;
-    int8_t first_rssi;
-    uint8_t first_lqi;
-};
-
 class Diag
 {
 public:
     static void Init(otInstance *aInstance);
-    static char *ProcessCmd(int argc, char *argv[]);
-    static bool isEnabled(void);
+    static const char *ProcessCmd(int aArgCount, char *aArgVector[]);
+    static bool IsEnabled(void);
 
     static void DiagTransmitDone(otInstance *aInstance, otError aError);
     static void DiagReceiveDone(otInstance *aInstance, otRadioFrame *aFrame, otError aError);
     static void AlarmFired(otInstance *aInstance);
 
 private:
-    static void AppendErrorResult(otError error, char *aOutput, size_t aOutputMaxLen);
-    static void ProcessStart(int argc, char *argv[], char *aOutput, size_t aOutputMaxLen);
-    static void ProcessStop(int argc, char *argv[], char *aOutput, size_t aOutputMaxLen);
-    static void ProcessSend(int argc, char *argv[], char *aOutput, size_t aOutputMaxLen);
-    static void ProcessRepeat(int argc, char *argv[], char *aOutput, size_t aOutputMaxLen);
-    static void ProcessStats(int argc, char *argv[], char *aOutput, size_t aOutputMaxLen);
-    static void ProcessChannel(int argc, char *argv[], char *aOutput, size_t aOutputMaxLen);
-    static void ProcessPower(int argc, char *argv[], char *aOutput, size_t aOutputMaxLen);
+
+    enum
+    {
+        kMaxOutputSize = 256,
+    };
+
+    struct DiagStats
+    {
+        uint32_t mReceivedPackets;
+        uint32_t mSentPackets;
+        int8_t   mFirstRssi;
+        uint8_t  mFirstLqi;
+    };
+
+    struct Command
+    {
+        const char *mName;
+        void (*mHandler)(int aArgCount, char *aArgVector[]);
+    };
+
+    static void AppendErrorResult(otError aError);
+    static void ProcessStart(int aArgCount, char *aArgVector[]);
+    static void ProcessStop(int aArgCount, char *aArgVector[]);
+    static void ProcessSend(int aArgCount, char *aArgVector[]);
+    static void ProcessRepeat(int aArgCount, char *aArgVector[]);
+    static void ProcessStats(int aArgCount, char *aArgVector[]);
+    static void ProcessChannel(int aArgCount, char *aArgVector[]);
+    static void ProcessPower(int aArgCount, char *aArgVector[]);
     static void TxPacket(void);
     static otError ParseLong(char *aString, long &aLong);
 
-    static char sDiagOutput[];
+    static char sOutput[];
     static const struct Command sCommands[];
     static struct DiagStats sStats;
     static int8_t sTxPower;
@@ -95,7 +99,7 @@ private:
     static uint32_t sTxPeriod;
     static uint32_t sTxPackets;
     static otRadioFrame *sTxPacket;
-    static otInstance *sContext;
+    static otInstance *sInstance;
     static bool sRepeatActive;
 };
 

src/diag/openthread-diag.cpp

@@ -40,6 +40,7 @@
 #include <openthread/diag.h>
 
 #include "diag_process.hpp"
+#include "common/code_utils.hpp"
 
 using namespace ot::Diagnostics;
 
@@ -48,37 +49,96 @@ void otDiagInit(otInstance *aInstance)
     Diag::Init(aInstance);
 }
 
-char *otDiagProcessCmd(int aArgc, char *aArgv[])
+const char *otDiagProcessCmd(int aArgCount, char *aArgVector[])
 {
-    return Diag::ProcessCmd(aArgc, aArgv);
+    return Diag::ProcessCmd(aArgCount, aArgVector);
 }
 
-char *otDiagProcessCmdLine(char *aString)
+static bool IsSpace(char aChar)
 {
-    char *argv[8];
-    int argc = 0;
-    int length = static_cast<int>(strlen(aString));
-    char *cmd;
+    return (aChar == ' ') || (aChar == '\t');
+}
 
-    for (; *aString == ' '; aString++, length--);
+static bool IsNullOrNewline(char aChar)
+{
+    return (aChar == 0) || (aChar == '\n') || (aChar == '\r');
+}
 
-    for (cmd = aString + 1; (cmd < aString + length) && (cmd != NULL); ++cmd)
+const char *otDiagProcessCmdLine(const char *aInput)
+{
+    enum
     {
-        if (*cmd == ' ' || *cmd == '\r' || *cmd == '\n')
+        kMaxArgs = 32,
+        kMaxCommandBuffer = 256,
+    };
+
+    otError error = OT_ERROR_NONE;
+    char buffer[kMaxCommandBuffer];
+    char *argVector[kMaxArgs];
+    int argCount = 0;
+    char *bufPtr = &buffer[0];
+    uint16_t bufLen = sizeof(buffer);
+    const char *output = "\r\n";
+
+    while (!IsNullOrNewline(*aInput))
+    {
+        while (IsSpace(*aInput))
         {
-            *cmd = '\0';
+            aInput++;
         }
 
-        if (*(cmd - 1) == '\0' && *cmd != ' ')
+        argVector[argCount] = bufPtr;
+
+        while (!IsSpace(*aInput) && !IsNullOrNewline(*aInput))
         {
-            argv[argc++] = cmd;
+            *bufPtr++ = *aInput++;
+            VerifyOrExit(--bufLen > 0, error = OT_ERROR_NO_BUFS);
+        }
+
+        if (argVector[argCount] != bufPtr)
+        {
+            *bufPtr++ = 0;
+            VerifyOrExit(--bufLen > 0, error = OT_ERROR_NO_BUFS);
+
+            argCount++;
+            VerifyOrExit(argCount < kMaxArgs, error = OT_ERROR_INVALID_ARGS);
         }
     }
 
-    return Diag::ProcessCmd(argc, argv);
+exit:
+
+    switch (error)
+    {
+    case OT_ERROR_NONE:
+
+        if (argCount >= 1)
+        {
+            output = Diag::ProcessCmd(argCount - 1, (argCount == 1) ? NULL : &argVector[1]);
+        }
+        else
+        {
+            output = Diag::ProcessCmd(0, NULL);
+        }
+
+        break;
+
+    case OT_ERROR_NO_BUFS:
+        output = "failed: command string too long\r\n";
+        break;
+
+    case OT_ERROR_INVALID_ARGS:
+        output = "failed: command string contains too many arguments\r\n";
+        break;
+
+    default:
+        output = "failed to parse command string\n\r";
+        break;
+    }
+
+    return output;
 }
 
-bool otDiagIsEnabled()
+bool otDiagIsEnabled(void)
 {
-    return Diag::isEnabled();
+    return Diag::IsEnabled();
 }

src/ncp/ncp_base_mtd.cpp

@@ -2145,15 +2145,14 @@ exit:
 otError NcpBase::SetPropertyHandler_NEST_STREAM_MFG(uint8_t aHeader)
 {
     const char *string = NULL;
-    char *output = NULL;
+    const char *output = NULL;
     otError error = OT_ERROR_NONE;
 
     error = mDecoder.ReadUtf8(string);
 
     VerifyOrExit(error == OT_ERROR_NONE, error = WriteLastStatusFrame(aHeader, ThreadErrorToSpinelStatus(error)));
 
-    // All diagnostics related features are processed within diagnostics module
-    output = otDiagProcessCmdLine(const_cast<char *>(string));
+    output = otDiagProcessCmdLine(string);
 
     // Prepare the response
     SuccessOrExit(error = mEncoder.BeginFrame(aHeader, SPINEL_CMD_PROP_VALUE_IS, SPINEL_PROP_NEST_STREAM_MFG));

tests/unit/test_diag.cpp

@@ -79,18 +79,83 @@ extern "C" void otPlatRadioTxStarted(otInstance *, otRadioFrame *aFrame)
     (void) aFrame;
 }
 
-
 /**
- *  diagnostics module tests
+ * Converts a given string replacing '\n', '\r', '\t', etc. with literal strings "\n", "\r", "\t", etc. to make it
+ * printable on console.
+ *
  */
-void TestDiag()
+const char *MakePrintable(const char *aString)
 {
-    static const struct
+    static char printableString[256];
+
+    char *ptr = &printableString[0];
+
+    *ptr++ = '\"';
+
+    while ((*aString != 0) && (ptr < &printableString[sizeof(printableString) - 4]))
     {
-        const char *command;
-        const char *output;
-    } tests[] =
+        // We ensure that at least 4 characters are remaining in output,
+        // two for encoding the current char (in case it needs a backslash)
+        // and two for the ending `"` and null char.
+
+        char c = *aString++;
+        bool addBackslash = true;
+
+        switch (c)
+        {
+        case '\n':
+            c = 'n';
+            break;
+
+        case '\r':
+            c = 'r';
+            break;
+
+        case '\t':
+            c = 't';
+            break;
+
+        case '\"':
+            c = '\"';
+            break;
+
+        case '\'':
+            c = '\'';
+            break;
+
+        case '\\':
+            break;
+
+        default:
+            addBackslash = false;
+            break;
+        }
+
+        if (addBackslash)
+        {
+            *ptr++ = '\\';
+        }
+
+        *ptr++ = c;
+    }
+
+    *ptr++ = '\"';
+    *ptr = 0;
+
+    return printableString;
+}
+
+void TestDiag(void)
+{
+    struct TestCommand
     {
+        const char *mCommand;
+        const char *mExpectedOutput;
+    };
+
+    static const TestCommand tests[] =
+    {
+
         {
             "diag\n",
             "diagnostics mode is disabled\r\n",
@@ -104,7 +169,11 @@ void TestDiag()
             "start diagnostics mode\r\nstatus 0x00\r\n",
         },
         {
-            "diag\n",
+            "diag",
+            "diagnostics mode is enabled\r\n",
+        },
+        {
+            "",
             "diagnostics mode is enabled\r\n",
         },
         {
@@ -135,6 +204,10 @@ void TestDiag()
             "diag send 20 100\n",
             "sending 0x14 packet(s), length 0x64\r\nstatus 0x00\r\n",
         },
+        {
+            "  diag \t send    \t 20\t100",           // Check parsing of extra space chars between args
+            "sending 0x14 packet(s), length 0x64\r\nstatus 0x00\r\n",
+        },
         {
             "diag repeat 100 100\n",
             "sending packets of length 0x64 at the delay of 0x64 ms\r\nstatus 0x00\r\n"
@@ -144,9 +217,17 @@ void TestDiag()
             "received packets: 0\r\nsent packets: 0\r\nfirst received packet: rssi=0, lqi=0\r\n\nstop diagnostics mode\r\nstatus 0x00\r\n",
         },
         {
-            "diag\n",
+            "diag",
             "diagnostics mode is disabled\r\n",
         },
+        {
+            "diag 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32",
+            "failed: command string contains too many arguments\r\n",
+        },
+        {
+            NULL,
+            NULL,
+        }
     };
 
     // initialize platform layer
@@ -157,19 +238,17 @@ void TestDiag()
     // initialize diagnostics module
     otDiagInit(NULL);
 
-    // test diagnostics commands
-    VerifyOrQuit(!otDiagIsEnabled(), "diagnostics mode shoud be disabled as default\n");
+    VerifyOrQuit(!otDiagIsEnabled(), "diagnostics mode should be disabled as default\n");
 
-    for (unsigned int i = 0; i < sizeof(tests) / sizeof(tests[0]);  i++)
+    for (const TestCommand *test = &tests[0]; test->mCommand != NULL; test++)
     {
-        char string[50];
-        char *output = NULL;
+        const char *output = NULL;
 
-        memcpy(string, tests[i].command, strlen(tests[i].command) + 1);
+        printf("\nCommand: %s", MakePrintable(test->mCommand));
+        output = otDiagProcessCmdLine(test->mCommand);
+        printf("\nOutput:  %s\n", MakePrintable(output));
 
-        output = otDiagProcessCmdLine(string);
-        VerifyOrQuit(memcmp(output, tests[i].output, strlen(tests[i].output)) == 0,
-                     "Test Diagnostics module failed\r\n");
+        VerifyOrQuit(strcmp(output, test->mExpectedOutput) == 0, "diagnostics output does not match expected result\n");
     }
 }
 
@@ -177,7 +256,7 @@ void TestDiag()
 int main(void)
 {
     TestDiag();
-    printf("All tests passed\n");
+    printf("\nAll tests passed\n");
     return 0;
 }
 #endif