Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 5 commits into from
Closed

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

wants to merge 5 commits into from

Conversation

jcarrano
Copy link
Contributor

@jcarrano jcarrano commented Dec 18, 2018 •
edited
Loading

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

@jcarrano 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
kaspar030 previously requested changes Dec 18, 2018
View reviewed changes
Copy link
Contributor

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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
Copy link
Contributor Author

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).

@aabadie aabadie added this to the Release 2019.01 milestone Dec 19, 2018
jcarrano
jcarrano commented Jan 16, 2019
View reviewed changes
Copy link
Contributor Author

@jcarrano jcarrano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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 jcarrano mentioned this pull request Jan 17, 2019
Closed
@jcarrano jcarrano mentioned this pull request Feb 11, 2019
Open
9 tasks
@jcarrano jcarrano mentioned this pull request Feb 28, 2019
Closed
@jcarrano
Copy link
Contributor Author

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

@jcarrano
Copy link
Contributor Author

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

@jcarrano jcarrano force-pushed the shell-fix-longcmd branch from b035c31 to 61ae1ec Compare March 1, 2019 10:32
@jcarrano jcarrano force-pushed the shell-fix-longcmd branch from 61ae1ec to bd8c993 Compare May 28, 2019 13:53
@jcarrano
Copy link
Contributor Author

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

@jcarrano jcarrano force-pushed the shell-fix-longcmd branch 2 times, most recently from 80e30e2 to 9f52bef Compare June 26, 2019 09:32
smlng
smlng suggested changes Jun 28, 2019
View reviewed changes
tests/shell/Makefile Outdated Show resolved Hide resolved
tests/shell/main.c Show resolved Hide resolved
sys/shell/shell.c Show resolved Hide resolved
sys/shell/shell.c

if (!res) {
handle_input_line(shell_commands, line_buf);
switch (res) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

sys/shell/shell.c Show resolved Hide resolved
tests/shell/tests/01-run.py Outdated Show resolved Hide resolved
@jcarrano jcarrano force-pushed the shell-fix-longcmd branch 2 times, most recently from ebe823e to 997341f Compare August 23, 2019 12:51
@aabadie aabadie mentioned this pull request Aug 23, 2019
Closed
@jcarrano jcarrano mentioned this pull request Aug 26, 2019
Closed
@jcarrano 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
Copy link
Contributor Author

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.

@jcarrano
tests/shell: fix false negative in the Ctrl-C test.
8e4f737 
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
Copy link
Contributor Author

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

@jcarrano
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.
@jcarrano
fixup! work around uart bug, but better.
8a477b4
@jcarrano
sys/shell: correctly detect and handle long lines.
fb0fd2c 
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.
@jcarrano
fixup! fix regression in ctrl-c handling.
9e50b9b
@jcarrano jcarrano mentioned this pull request Aug 27, 2019
Closed
2 tasks
smlng
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
Copy link
Contributor Author

@smlng much of this code will disappear with #12099

@jcarrano 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
Copy link
Contributor Author

@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.

@jcarrano jcarrano mentioned this pull request Aug 28, 2019
Open
8 tasks
@smlng smlng dismissed their stale review August 28, 2019 13:42

won't block

@kb2ma kb2ma modified the milestones: Release 2019.10, Release 2020.01 Oct 9, 2019
@HendrikVE HendrikVE mentioned this pull request Jan 24, 2020
Merged
@HendrikVE
Copy link
Contributor

Can be closed due to #13195

@smlng
Copy link
Member

smlng commented Mar 6, 2020

closing as needs rebase and is superseded by #13195

@smlng smlng closed this Mar 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cladmi cladmi cladmi left review comments

@smlng smlng smlng left review comments

@kaspar030 kaspar030 kaspar030 left review comments

@jia200x jia200x Awaiting requested review from jia200x

@MrKevinWeiss MrKevinWeiss Awaiting requested review from MrKevinWeiss

Assignees
No one assigned
Labels
Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
Release 2020.04
Development

Successfully merging this pull request may close these issues.
10 participants
@jcarrano @HendrikVE @smlng @cladmi @kaspar030 @miri64 @aabadie @kb2ma @MrKevinWeiss @fjmolinas