sys/shell: correctly detect and handle long lines

sys/shell/shell.c

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2009, Freie Universitaet Berlin (FUB).
+ * Copyright (C) 2009, 2020 Freie Universität Berlin
  * Copyright (C) 2013, INRIA.
  * Copyright (C) 2015 Kaspar Schleiser <kaspar@schleiser.de>
  *
@@ -21,6 +21,8 @@
  *
  * @author      Kaspar Schleiser <kaspar@schleiser.de>
  * @author      René Kijewski <rene.kijewski@fu-berlin.de>
+ * @author      Juan Carrano <j.carrano@fu-berlin.de>
+ * @author      Hendrik van Essen <hendrik.ve@fu-berlin.de>
  *
  * @}
  */
@@ -29,6 +31,10 @@
 #include <stdio.h>
 #include <stdint.h>
 #include <stdlib.h>
+#include <stdbool.h>
+#include <assert.h>
+#include <errno.h>
+
 #include "shell.h"
 #include "shell_commands.h"
 
@@ -169,7 +175,7 @@ static void handle_input_line(const shell_command_t *command_list, char *line)
             ++argc;
         }
 
-        /* zero out the current position (space or quotation mark) and advance */
+        /* zero out current position (space or quotation mark) and advance */
         if (*pos > 0) {
             *pos = 0;
             ++pos;
@@ -227,42 +233,76 @@ static void handle_input_line(const shell_command_t *command_list, char *line)
     }
 }
 
+/**
+ * @brief   Read a single line from standard input into a buffer.
+ *
+ * In addition to copying characters, this routine echoes the line back to
+ * stdout and also supports primitive line editing.
+ *
+ * If the input line is too long, the input will still be consumed until the end
+ * to prevent the next line from containing garbage.
+ *
+ * @param   buf     Buffer where the input will be placed.
+ * @param   size    Size of the buffer. The maximum line length will be one less
+ *                  than size, to accommodate for the null terminator.
+ *                  The minimum buffer size is 1.
+ *
+ * @return  length of the read line, excluding the terminator, if reading was
+ *          successful.
+ * @return  EOF, if the end of the input stream was reached.
+ * @return  ENOBUFS if the buffer size was exceeded.
+ */
 static int readline(char *buf, size_t size)
 {
-    char *line_buf_ptr = buf;
+    int curr_pos = 0;
+    bool length_exceeded = false;
+
+    assert((size_t) size > 0);
 
     while (1) {
-        if ((line_buf_ptr - buf) >= ((int) size) - 1) {
-            return -1;
-        }
+        /* At the start of the loop, cur_pos should point inside of
+         * buf. This ensures the terminator can always fit. */
+        assert((size_t) curr_pos < size);
 
         int c = getchar();
         if (c < 0) {
             return EOF;
         }
 
-        /* We allow Unix linebreaks (\n), DOS linebreaks (\r\n), and Mac linebreaks (\r). */
-        /* QEMU transmits only a single '\r' == 13 on hitting enter ("-serial stdio"). */
-        /* DOS newlines are handled like hitting enter twice, but empty lines are ignored. */
-        /* Ctrl-C cancels the current line. */
+        /* We allow Unix linebreaks (\n), DOS linebreaks (\r\n), and Mac
+         * linebreaks (\r). QEMU transmits only a single '\r' == 13 on hitting
+         * enter ("-serial stdio"). DOS newlines are handled like hitting enter
+         * twice, but empty lines are ignored. Ctrl-C cancels the current line.
+         */
         if (c == '\r' || c == '\n' || c == ETX) {
-            *line_buf_ptr = '\0';
+            if (c == ETX) {
+                curr_pos = 0;
+                length_exceeded = false;
+            }
+
+            buf[curr_pos] = '\0';
 #ifndef SHELL_NO_ECHO
             _putchar('\r');
             _putchar('\n');
 #endif
 
-            /* return 1 if line is empty, 0 otherwise */
-            return c == ETX || line_buf_ptr == buf;
+            return (length_exceeded) ? -ENOBUFS : curr_pos;
         }
-        /* QEMU uses 0x7f (DEL) as backspace, while 0x08 (BS) is for most terminals */
-        else if (c == 0x08 || c == 0x7f) {
-            if (line_buf_ptr == buf) {
-                /* The line is empty. */
+
+        /* check for backspace:
+         * 0x7f (DEL) when using QEMU
+         * 0x08 (BS) for most terminals */
+        if (c == 0x08 || c == 0x7f) {
+            if (curr_pos == 0) {
+                /* ignore empty line */
                 continue;
             }
 
-            *--line_buf_ptr = '\0';
+            /* after we dropped characters don't edit the line, yet keep the
+             * visual effects */
+            if (!length_exceeded) {
+                buf[--curr_pos] = '\0';
+            }
             /* white-tape the character */
 #ifndef SHELL_NO_ECHO
             _putchar('\b');
@@ -271,7 +311,13 @@ static int readline(char *buf, size_t size)
 #endif
         }
         else {
-            *line_buf_ptr++ = c;
+            /* Always consume characters, but do not not always store them */
+            if ((size_t) curr_pos < size - 1) {
+                buf[curr_pos++] = c;
+            }
+            else {
+                length_exceeded = true;
+            }
 #ifndef SHELL_NO_ECHO
             _putchar(c);
 #endif
@@ -298,12 +344,18 @@ void shell_run_once(const shell_command_t *shell_commands,
     while (1) {
         int res = readline(line_buf, len);
 
-        if (res == EOF) {
-            break;
-        }
+        switch (res) {
+
+            case EOF:
+                return;
 
-        if (!res) {
-            handle_input_line(shell_commands, line_buf);
+            case -ENOBUFS:
+                puts("shell: maximum line length exceeded");
+                break;
+
+            default:
+                handle_input_line(shell_commands, line_buf);
+                break;
         }
 
         print_prompt();

tests/driver_pca9685/Makefile.ci

@@ -1,6 +1,8 @@
 BOARD_INSUFFICIENT_MEMORY := \
     arduino-duemilanove \
+    arduino-leonardo \
     arduino-nano \
     arduino-uno \
     atmega328p \
+    atmega32u4 \
     #

tests/shell/main.c

@@ -56,7 +56,17 @@ static int print_echo(int argc, char **argv)
     return 0;
 }
 
+static int print_shell_bufsize(int argc, char **argv)
+{
+    (void) argc;
+    (void) argv;
+    printf("%d\n", SHELL_DEFAULT_BUFSIZE);
+
+    return 0;
+}
+
 static const shell_command_t shell_commands[] = {
+    { "bufsize", "Get the shell's buffer size", print_shell_bufsize },
     { "start_test", "starts a test", print_teststart },
     { "end_test", "ends a test", print_testend },
     { "echo", "prints the input command", print_echo },

tests/shell/tests/01-run.py

@@ -7,6 +7,7 @@
 # directory for more details.
 
 import sys
+import os
 from testrunner import run
 
 
@@ -39,31 +40,91 @@
 CONTROL_C = DLE+'\x03'
 CONTROL_D = DLE+'\x04'
 
+PROMPT = '> '
+
 CMDS = (
-    (CONTROL_C, ('>')),
-    ('start_test', ('[TEST_START]')),
-    ('end_test', ('[TEST_END]')),
-    ('\n', ('>')),
+    ('start_test', '[TEST_START]'),
+    (CONTROL_C, PROMPT),
+    ('\n', PROMPT),
     ('123456789012345678901234567890123456789012345678901234567890',
-        ('shell: command not found: '
-         '123456789012345678901234567890123456789012345678901234567890')),
-    ('unknown_command', ('shell: command not found: unknown_command')),
+     'shell: command not found: '
+     '123456789012345678901234567890123456789012345678901234567890'),
+    ('unknown_command', 'shell: command not found: unknown_command'),
     ('help', EXPECTED_HELP),
-    ('echo a string', ('\"echo\"\"a\"\"string\"')),
+    ('echo a string', '\"echo\"\"a\"\"string\"'),
     ('ps', EXPECTED_PS),
-    ('garbage1234'+CONTROL_C, ('>')),  # test cancelling a line
     ('help', EXPECTED_HELP),
-    ('reboot', ('test_shell.'))
+    ('reboot', 'test_shell.'),
+    ('end_test', '[TEST_END]'),
 )
 
+BOARD = os.environ['BOARD']
+
+
+def print_error(message):
+    FAIL = '\033[91m'
+    ENDC = '\033[0m'
+    print(FAIL + message + ENDC)
+
 
 def check_cmd(child, cmd, expected):
+    child.expect(PROMPT)
     child.sendline(cmd)
     for line in expected:
         child.expect_exact(line)
 
 
+def check_startup(child):
+    child.sendline(CONTROL_C)
+    child.expect_exact(PROMPT)
+
+
+def check_and_get_bufsize(child):
+    child.sendline('bufsize')
+    child.expect('([0-9]+)\r\n')
+    bufsize = int(child.match.group(1))
+
+    return bufsize
+
+
+def check_line_exceeded(child, bufsize):
+
+    if BOARD == 'nrf52dk':
+        # There is an issue with nrf52dk when the Virtual COM port is connected
+        # and sending more than 64 bytes over UART. If no terminal is connected
+        # to the Virtual COM and interfacing directly to the nrf52832 UART pins
+        # the issue is not present. See issue #10639 on GitHub.
+        print_error('test case "check_line_exceeded" broken for nrf52dk. SKIP')
+        return
+
+    longline = "_"*bufsize + "verylong"
+
+    child.sendline(longline)
+    child.expect('shell: maximum line length exceeded')
+
+
+def check_line_canceling(child):
+    child.expect(PROMPT)
+    child.sendline('garbage1234' + CONTROL_C)
+    garbage_expected = 'garbage1234\r\r\n'
+    garbage_received = child.read(len(garbage_expected))
+
+    assert garbage_expected == garbage_received
+
+
 def testfunc(child):
+    # avoid sending an extra empty line on native.
+    if BOARD == 'native':
+        child.crlf = '\n'
+
+    check_startup(child)
+
+    bufsize = check_and_get_bufsize(child)
+
+    check_line_exceeded(child, bufsize)
+
+    check_line_canceling(child)
+
     # loop other defined commands and expected output
     for cmd, expected in CMDS:
         check_cmd(child, cmd, expected)