sys/shell: correctly detect and handle long lines #10635

jcarrano · 2018-12-18T18:10:11Z

Contribution description

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.

Testing procedure

The first commit adds a test to tests/shell. It adds a command to retrieve buffer size and then forces an extremely long line. This is failing right now because of a bug in the shell.

Murdock should be checking this, but you can do it locally by checkout both commits and running

BOARD=some_board make -C tests/shell flash test

In each one. The first one fails and the second succeeds.

I can switch the order of the commits if keeping a working history is important.

Issues/PRs references

Testing was made specially difficult because of #10634 .
Part of #12105

kaspar030

First round. The fix is necessary. The implementation is a little hacky.

sys/shell/shell.c

jcarrano · 2018-12-19T11:28:09Z

I force pushed because I more or less redid the second PR.

Because readline is now returning the line length, it made more sense to use an index variable and not a pointer to access the buffer. I think code looks cleaner now (assertions look more expressive).

jcarrano

Added some comments on the reviews that dissapeared because of GH magic.

sys/shell/shell.c

jcarrano · 2019-02-28T17:17:48Z

I rebased (due to #10630 being merged) and squashed the fixup.

jcarrano · 2019-02-28T17:19:28Z

I would be a good idea to merge this considering people are wasting time in fixing the same bugs all over again (#11054 )

jcarrano · 2019-05-28T13:54:28Z

I rebased on top of (the rebased) rawterm. Note that rawterm is not needed for this fix, but it makes testing easier.

Stale.

tests/shell/Makefile

tests/shell/main.c

sys/shell/shell.c

smlng · 2019-06-28T20:46:24Z

sys/shell/shell.c

-
-        if (!res) {
-            handle_input_line(shell_commands, line_buf);
+        switch (res) {


as there is no other use of res could be replace with switch (readline(line_buf, len)) {

sys/shell/shell.c

tests/shell/tests/01-run.py

jcarrano · 2019-08-27T12:15:01Z

Ouch! I discovered I broke control-c handling when I rebased this and also that testing via expect would miss that bug. I'm working on fixing it.

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. To make the test more robust, the newline handling was changed in native, where an extra empty line was being sent with each character.

jcarrano · 2019-08-27T13:04:15Z

Sorry for so many force-pushing- pexpect and the shell are conspiring against me.

Add a command to retrieve buffer size and then force an extremely long line. This is failing right now because of a bug in the shell. The terminal was changed to socat, as pyterm messes up the I/O and makes testing impossible.

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.

sys/shell/shell.c

jcarrano · 2019-08-28T12:30:05Z

@smlng much of this code will disappear with #12099

jcarrano · 2019-08-28T12:49:45Z

@smlng I'm doing what I can trying to repair a code that is of the lowest standard. At the end I decided it needs more or complete rewrite (thus my "refactor" PRs) but first it is necessary to fix the bugs in the old code.

There is no point trying to make this perfect or even good, because I'm building on bad foundations.

won't block

HendrikVE · 2020-01-24T13:12:57Z

Can be closed due to #13195

smlng · 2020-03-06T07:47:44Z

closing as needs rebase and is superseded by #13195

jcarrano added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Dec 18, 2018

kaspar030 previously requested changes Dec 18, 2018

View reviewed changes

sys/shell/shell.c Outdated Show resolved Hide resolved

sys/shell/shell.c Outdated Show resolved Hide resolved

sys/shell/shell.c Outdated Show resolved Hide resolved

sys/shell/shell.c Outdated Show resolved Hide resolved

sys/shell/shell.c Show resolved Hide resolved

jcarrano force-pushed the shell-fix-longcmd branch from 3fd3fc1 to e3e756a Compare December 19, 2018 11:25

aabadie added this to the Release 2019.01 milestone Dec 19, 2018

jcarrano force-pushed the shell-fix-longcmd branch from e3e756a to 90a4116 Compare December 19, 2018 12:15

jcarrano commented Jan 16, 2019

View reviewed changes

sys/shell/shell.c Outdated Show resolved Hide resolved

sys/shell/shell.c Outdated Show resolved Hide resolved

sys/shell/shell.c Show resolved Hide resolved

jcarrano mentioned this pull request Jan 17, 2019

sys/shell: Exit the shell on ctrl-D #10788

Closed

jcarrano mentioned this pull request Feb 11, 2019

[TRACKING] The RIOT shell as a testing interface #10994

Open

9 tasks

aabadie modified the milestones: Release 2019.01, Release 2019.04 Feb 12, 2019

jcarrano mentioned this pull request Feb 28, 2019

sys/shell: handle correctly long strings that overflow buffer #11054

Closed

jcarrano force-pushed the shell-fix-longcmd branch from a7e7b15 to b035c31 Compare February 28, 2019 17:16

jcarrano force-pushed the shell-fix-longcmd branch from b035c31 to 61ae1ec Compare March 1, 2019 10:32

miri64 modified the milestones: Release 2019.04, Release 2019.07 May 7, 2019

jcarrano force-pushed the shell-fix-longcmd branch from 61ae1ec to bd8c993 Compare May 28, 2019 13:53

jcarrano force-pushed the shell-fix-longcmd branch 2 times, most recently from 80e30e2 to 9f52bef Compare June 26, 2019 09:32

smlng requested changes Jun 28, 2019

View reviewed changes

MrKevinWeiss modified the milestones: Release 2019.07, Release 2019.10 Jul 16, 2019

jcarrano force-pushed the shell-fix-longcmd branch 2 times, most recently from ebe823e to 997341f Compare August 23, 2019 12:51

jcarrano force-pushed the shell-fix-longcmd branch from 997341f to 1a2a239 Compare August 23, 2019 15:27

aabadie mentioned this pull request Aug 23, 2019

tests/pkg_semtech-loramac insufficient memory for arduino-mega2560 #12071

Closed

jcarrano mentioned this pull request Aug 26, 2019

sys/shell: Make echoing and prompt runtime-configurable. #12087

Closed

jcarrano added the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Aug 27, 2019

jcarrano force-pushed the shell-fix-longcmd branch from 1a2a239 to 9e50b9b Compare August 27, 2019 13:03

jcarrano added 4 commits August 27, 2019 15:07

tests/shell: test longer-than-supported lines.

a89c188

Add a command to retrieve buffer size and then force an extremely long line. This is failing right now because of a bug in the shell. The terminal was changed to socat, as pyterm messes up the I/O and makes testing impossible.

fixup! work around uart bug, but better.

8a477b4

fixup! fix regression in ctrl-c handling.

9e50b9b

jcarrano mentioned this pull request Aug 27, 2019

sys/shell: refactor readline function. #12099

Closed

2 tasks

smlng previously requested changes Aug 28, 2019

View reviewed changes

sys/shell/shell.c Show resolved Hide resolved

sys/shell/shell.c Show resolved Hide resolved

sys/shell/shell.c Show resolved Hide resolved

sys/shell/shell.c Show resolved Hide resolved

jcarrano added Area: sys Area: System and removed State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels Aug 28, 2019

jcarrano requested review from jia200x and MrKevinWeiss August 28, 2019 12:49

jcarrano mentioned this pull request Aug 28, 2019

[TRACKING] sys/shell refactoring. #12105

Open

8 tasks

kb2ma modified the milestones: Release 2019.10, Release 2020.01 Oct 9, 2019

fjmolinas modified the milestones: Release 2020.01, Release 2020.04 Jan 15, 2020

HendrikVE mentioned this pull request Jan 24, 2020

sys/shell: correctly detect and handle long lines #13195

Merged

smlng closed this Mar 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sys/shell: correctly detect and handle long lines #10635

sys/shell: correctly detect and handle long lines #10635

jcarrano commented Dec 18, 2018 •

edited

kaspar030 left a comment

jcarrano commented Dec 19, 2018

jcarrano left a comment

jcarrano commented Feb 28, 2019

jcarrano commented Feb 28, 2019

jcarrano commented May 28, 2019

smlng Jun 28, 2019

jcarrano commented Aug 27, 2019

jcarrano commented Aug 27, 2019

jcarrano commented Aug 28, 2019

jcarrano commented Aug 28, 2019

HendrikVE commented Jan 24, 2020

smlng commented Mar 6, 2020

sys/shell: correctly detect and handle long lines #10635

sys/shell: correctly detect and handle long lines #10635

Conversation

jcarrano commented Dec 18, 2018 • edited

Contribution description

Testing procedure

Issues/PRs references

kaspar030 left a comment

Choose a reason for hiding this comment

jcarrano commented Dec 19, 2018

jcarrano left a comment

Choose a reason for hiding this comment

jcarrano commented Feb 28, 2019

jcarrano commented Feb 28, 2019

jcarrano commented May 28, 2019

smlng Jun 28, 2019

Choose a reason for hiding this comment

jcarrano commented Aug 27, 2019

jcarrano commented Aug 27, 2019

jcarrano commented Aug 28, 2019

jcarrano commented Aug 28, 2019

HendrikVE commented Jan 24, 2020

smlng commented Mar 6, 2020

jcarrano commented Dec 18, 2018 •

edited