drop! sys/shell: correctly detect and handle long lines (dependency to …

…RIOT-OS#13195)

The numeric value for EOF is -1. This caused the shell to return
the same code when EOF was encountered and when the line lenght was
exceeded. Additionally, if the line length is exceeded, the correct
behaviour is to consume the remaining characters until the end of
the line, to prevent the following line from containing (potentially
dangerous) garbage.

Co-authored-by: Hendrik van Essen <hendrik.ve@fu-berlin.de>

sys/shell: rephrase/reformat some comments

tests/shell: add test case for shell's buffer size

Co-authored-by: Juan Carrano <j.carrano@fu-berlin.de>

tests/shell: avoid sending an extra empty line on native

Co-authored-by: Juan Carrano <j.carrano@fu-berlin.de>

tests/shell: check for startup message

Co-authored-by: Juan Carrano <j.carrano@fu-berlin.de>

tests/shell: check for shell prompt

Co-authored-by: Juan Carrano <j.carrano@fu-berlin.de>

tests/shell: fix test case for line cancelling

The test for the line cancellation (ctrl-c) functionality was unable to
detect error because of the way pexpect matches output.

While working on the long line shell bug, a regression was about to be
introduced because of this. This commit fixes the test by directly reading from
the child process and expects an exact response.

Co-authored-by: Juan Carrano <j.carrano@fu-berlin.de>

tests/shell: add test case for exceeding lines

Co-authored-by: Juan Carrano <j.carrano@fu-berlin.de>

tests/shell: remove redundant parentheses

sys/shell/shell.c

@@ -29,6 +29,8 @@
 #include <stdio.h>
 #include <stdint.h>
 #include <stdlib.h>
+#include <stdbool.h>
+#include <assert.h>
 #include "shell.h"
 #include "shell_commands.h"
 
@@ -51,6 +53,11 @@ static void flush_if_needed(void)
     fflush(stdout);
 #endif
 }
+/**
+ * Code indicating that the line buffer size was exceeded.
+ * This definition ensures there's no collision with EOF.
+ */
+#define ERROR_READLINE_EXCEEDED (EOF - 1)
 
 static shell_command_handler_t find_handler(const shell_command_t *command_list, char *command)
 {
@@ -227,42 +234,75 @@ static void handle_input_line(const shell_command_t *command_list, char *line)
     }
 }
 
+/**
+ * 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  ERROR_READLINE_EXCEEDED 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)? ERROR_READLINE_EXCEEDED : 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 do not 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,22 @@ void shell_run_once(const shell_command_t *shell_commands,
     while (1) {
         int res = readline(line_buf, len);
 
-        if (res == EOF) {
-            break;
-        }
+        switch (res) {
 
-        if (!res) {
-            handle_input_line(shell_commands, line_buf);
+            case EOF:
+                return;
+
+            case ERROR_READLINE_EXCEEDED:
+                puts("shell: maximum line length exceeded");
+                break;
+
+            case 0:
+                puts("shell: line is empty");
+                break;
+
+            default:
+                handle_input_line(shell_commands, line_buf);
+                break;
         }
 
         print_prompt();

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
 
 
@@ -40,30 +41,88 @@
 CONTROL_D = DLE+'\x04'
 
 CMDS = (
-    (CONTROL_C, ('>')),
-    ('start_test', ('[TEST_START]')),
-    ('end_test', ('[TEST_END]')),
-    ('\n', ('>')),
+    ('start_test', '[TEST_START]'),
+    ('', 'shell: line is empty\r\n'),
+    (CONTROL_C, '>'),
+    ('\n', '>'),
     ('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]'),
 )
 
+PROMPT = '> '
+
+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.expect('test_shell.\r\n')
+
+
+def check_and_get_bufsize(child):
+    child.sendline('bufsize')
+    child.expect('([0-9]+)\r\n')
+    bufsize = int(child.match[1])
+
+    return bufsize
+
+
+def check_line_exceeded(child, bufsize):
+
+    if BOARD == 'nrf52dk':
+        # looks like the nrf52dk runs in to undefined behaviour when sending more
+        # than 64 bytes over UART
+        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)