chip-shell: properly null-terminate maxed line (#16013)

* chip-shell: properly null-terminate maxed line * Refactor chip-shell ReadLine() * Apply Default ReadLine changes to MainLoopEFR32 * Shell ReadLine early continue if nothing read * Change shell default ReadLine() to return size_t Previously returned ssize_t, but no reason for this.
project-chip · Apr 20, 2022 · c6b4b4e · c6b4b4e
1 parent bed563a
commit c6b4b4e
Show file tree

Hide file tree

Showing 2 changed files with 81 additions and 94 deletions.
diff --git a/src/lib/shell/MainLoopDefault.cpp b/src/lib/shell/MainLoopDefault.cpp
@@ -31,71 +31,64 @@ using chip::Shell::streamer_get;
 
 namespace {
 
-ssize_t ReadLine(char * buffer, size_t max)
+// max > 1
+size_t ReadLine(char * buffer, size_t max)
 {
-    ssize_t read = 0;
-    bool done    = false;
-    char * inptr = buffer;
+    size_t line_sz = 0;
 
-    // Read in characters until we get a new line or we hit our max size.
-    while (((inptr - buffer) < static_cast<int>(max)) && !done)
+    // Read in characters until we get a line ending or EOT.
+    for (bool done = false; !done;)
     {
-        if (read == 0)
+        // Stop reading if we've run out of space in the buffer (still need to null-terminate).
+        if (line_sz >= max - 1u)
         {
-            read = streamer_read(streamer_get(), inptr, 1);
+            buffer[max - 1] = '\0';
+            break;
+        }
+
+        if (streamer_read(streamer_get(), buffer + line_sz, 1) != 1)
+        {
+            continue;
         }
 
-        // Process any characters we just read in.
-        while (read > 0)
+        // Process character we just read.
+        switch (buffer[line_sz])
         {
-            switch (*inptr)
+        case '\r':
+        case '\n':
+            streamer_printf(streamer_get(), "\r\n");
+            buffer[line_sz] = '\0';
+            line_sz++;
+            done = true;
+            break;
+        case 0x04:
+            // Do not accept EOT character (i.e. don't increment line_sz).
+            // Stop the read loop if the input is still empty.
+            if (line_sz == 0u)
             {
-            case '\r':
-            case '\n':
-                streamer_printf(streamer_get(), "\r\n");
-                *inptr = 0; // null terminate
-                done   = true;
-                break;
-            case 0x04:
-                // Delete EOT character.
-                inptr -= 1;
-                // Stop the read loop if the input is still empty.
-                if (inptr == buffer - 1)
-                {
-                    done = true;
-                }
-                break;
-            case 0x7F:
-                // delete backspace character + 1 more
-                inptr -= 2;
-                if (inptr >= buffer - 1)
-                {
-                    streamer_printf(streamer_get(), "\b \b");
-                }
-                else
-                {
-                    inptr = buffer - 1;
-                }
-                break;
-            default:
-                if (isprint(static_cast<int>(*inptr)) || *inptr == '\t')
-                {
-                    streamer_printf(streamer_get(), "%c", *inptr);
-                }
-                else
-                {
-                    inptr--;
-                }
-                break;
+                done = true;
             }
-
-            inptr++;
-            read--;
+            break;
+        case 0x7F:
+            // Do not accept backspace character (i.e. don't increment line_sz) and remove 1 additional character if it exists.
+            if (line_sz >= 1u)
+            {
+                streamer_printf(streamer_get(), "\b \b");
+                line_sz--;
+            }
+            break;
+        default:
+            if (isprint(static_cast<int>(buffer[line_sz])) || buffer[line_sz] == '\t')
+            {
+                streamer_printf(streamer_get(), "%c", buffer[line_sz]);
+                line_sz++;
+            }
+            break;
         }
     }
 
     // Return the length of the buffer including the terminating null byte.
-    return inptr - buffer;
+    return line_sz;
 }
 
 bool IsSeparator(char ch)
@@ -198,7 +191,7 @@ void Engine::RunMainLoop()
     while (true)
     {
         char * line = static_cast<char *>(Platform::MemoryAlloc(CHIP_SHELL_MAX_LINE_SIZE));
-        if (ReadLine(line, CHIP_SHELL_MAX_LINE_SIZE) == 0)
+        if (ReadLine(line, CHIP_SHELL_MAX_LINE_SIZE) == 0u)
         {
             // Stop loop in case of empty read (Ctrl-D).
             break;

diff --git a/src/lib/shell/MainLoopEFR32.cpp b/src/lib/shell/MainLoopEFR32.cpp
@@ -34,58 +34,52 @@ namespace {
 
 constexpr const char kShellPrompt[] = "matterCli > ";
 
+// max > 1
 void ReadLine(char * buffer, size_t max)
 {
-    ssize_t read = 0;
-    bool done    = false;
-    char * inptr = buffer;
+    size_t line_sz = 0;
 
-    // Read in characters until we get a new line or we hit our max size.
-    while (((inptr - buffer) < static_cast<int>(max)) && !done)
+    // Read in characters until we get a line ending or EOT.
+    for (bool done = false; !done;)
     {
+        // Stop reading if we've run out of space in the buffer (still need to null-terminate).
+        if (line_sz >= max - 1u)
+        {
+            buffer[max - 1] = '\0';
+            break;
+        }
+
         chip::WaitForShellActivity();
-        if (read == 0)
+        if (streamer_read(streamer_get(), buffer + line_sz, 1) != 1)
         {
-            read = streamer_read(streamer_get(), inptr, 1);
+            continue;
         }
 
-        // Process any characters we just read in.
-        while (read > 0)
+        // Process character we just read.
+        switch (buffer[line_sz])
         {
-            switch (*inptr)
+        case '\r':
+        case '\n':
+            streamer_printf(streamer_get(), "\r\n");
+            buffer[line_sz] = '\0';
+            line_sz++;
+            done = true;
+            break;
+        case 0x7F:
+            // Do not accept backspace character (i.e. don't increment line_sz) and remove 1 additional character if it exists.
+            if (line_sz >= 1u)
             {
-            case '\r':
-            case '\n':
-                streamer_printf(streamer_get(), "\r\n");
-                *inptr = 0; // null terminate
-                done   = true;
-                break;
-            case 0x7F:
-                // delete backspace character + 1 more
-                inptr -= 2;
-                if (inptr >= buffer - 1)
-                {
-                    streamer_printf(streamer_get(), "\b \b");
-                }
-                else
-                {
-                    inptr = buffer - 1;
-                }
-                break;
-            default:
-                if (isprint(static_cast<int>(*inptr)) || *inptr == '\t')
-                {
-                    streamer_printf(streamer_get(), "%c", *inptr);
-                }
-                else
-                {
-                    inptr--;
-                }
-                break;
+                streamer_printf(streamer_get(), "\b \b");
+                line_sz--;
             }
-
-            inptr++;
-            read--;
+            break;
+        default:
+            if (isprint(static_cast<int>(buffer[line_sz])) || buffer[line_sz] == '\t')
+            {
+                streamer_printf(streamer_get(), "%c", buffer[line_sz]);
+                line_sz++;
+            }
+            break;
         }
     }
 }