[PATCH 0/2] readline_simple: return -1 if getc fails

[PATCH 0/2] readline_simple: return -1 if getc fails

[Gaël PORTAY]

Dear maintainers,

I came accross this issue when I was trying to add support for a new MIPS
board.

The getc() function from console_simple.c returns -EINVAL when no console is
registered.

It causes readline_simple to print a non-printable character (-EINVAL in this
case). Furthermore, it causes an infinite loop: readline never returns.

	barebox:/ ���(...)���

With this patch, the error is no more explicit but the readline function stops
to print a dummy character and returns to shell.

	barebox:/ <INTERRUPT>
	barebox:/ <INTERRUPT>
	barebox:/ <INTERRUPT>
	(...)
	barebox:/ <INTERRUPT>
	barebox:/ <INTERRUPT>
	barebox:/ <INTERRUPT>

The second patch is a simple cleanup of the in-file documentation.

Regards,
Gaël PORTAY (2):
  readline_simple: return -1 if getc fails
  readline_simple: remove obsolete documentation

 lib/readline_simple.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

-- 
2.13.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 1/2] readline_simple: return -1 if getc fails

[Gaël PORTAY]

The getc function may return an errno code if an error happens.

This patch prevents readline from printing a non printable character and
from looping to infinity and beyong.

Signed-off-by: Gaël PORTAY <gael.portay@savoirfairelinux.com>
---
 lib/readline_simple.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/readline_simple.c b/lib/readline_simple.c
index c4d3d240e..1283c9602 100644
--- a/lib/readline_simple.c
+++ b/lib/readline_simple.c
@@ -58,6 +58,8 @@ int readline (const char *prompt, char *line, int len)
 
 	for (;;) {
 		c = getchar();
+		if (c < 0)
+			return (-1);
 
 		/*
 		 * Special character handling
-- 
2.13.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/2] readline_simple: return -1 if getc fails

[Lucas Stach]

Am Montag, den 07.08.2017, 18:10 -0400 schrieb Gaël PORTAY:
> The getc function may return an errno code if an error happens.
> 
> This patch prevents readline from printing a non printable character and
> from looping to infinity and beyong.
> 
> Signed-off-by: Gaël PORTAY <gael.portay@savoirfairelinux.com>
> ---
>  lib/readline_simple.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/lib/readline_simple.c b/lib/readline_simple.c
> index c4d3d240e..1283c9602 100644
> --- a/lib/readline_simple.c
> +++ b/lib/readline_simple.c
> @@ -58,6 +58,8 @@ int readline (const char *prompt, char *line, int len)
>  
>  	for (;;) {
>  		c = getchar();
> +		if (c < 0)
> +			return (-1);

I don't like made up error codes. Is there any reason why we couldn't
just pass through the negative error code from getchar?

Regards,
Lucas


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/2] readline_simple: return -1 if getc fails

[Ian Abbott]

On 08/08/17 08:51, Lucas Stach wrote:
> Am Montag, den 07.08.2017, 18:10 -0400 schrieb Gaël PORTAY:
>> The getc function may return an errno code if an error happens.
>>
>> This patch prevents readline from printing a non printable character and
>> from looping to infinity and beyong.
>>
>> Signed-off-by: Gaël PORTAY <gael.portay@savoirfairelinux.com>
>> ---
>>   lib/readline_simple.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/lib/readline_simple.c b/lib/readline_simple.c
>> index c4d3d240e..1283c9602 100644
>> --- a/lib/readline_simple.c
>> +++ b/lib/readline_simple.c
>> @@ -58,6 +58,8 @@ int readline (const char *prompt, char *line, int len)
>>   
>>   	for (;;) {
>>   		c = getchar();
>> +		if (c < 0)
>> +			return (-1);
> 
> I don't like made up error codes. Is there any reason why we couldn't
> just pass through the negative error code from getchar?

And also change the type of the 'c' variable, as it is currently a plain 
'char'?

> Regards,
> Lucas

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk> )=-
-=(                          Web: http://www.mev.co.uk/  )=-

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/2] readline_simple: return -1 if getc fails

[Gaël PORTAY]

Hi Ian,

On Tue, Aug 08, 2017 at 11:07:54AM +0100, Ian Abbott wrote:
> On 08/08/17 08:51, Lucas Stach wrote:
> > Am Montag, den 07.08.2017, 18:10 -0400 schrieb Gaël PORTAY:
> > > The getc function may return an errno code if an error happens.
> > > 
> > > This patch prevents readline from printing a non printable character and
> > > from looping to infinity and beyong.
> > > 
> > > Signed-off-by: Gaël PORTAY <gael.portay@savoirfairelinux.com>
> > > ---
> > >   lib/readline_simple.c | 2 ++
> > >   1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/lib/readline_simple.c b/lib/readline_simple.c
> > > index c4d3d240e..1283c9602 100644
> > > --- a/lib/readline_simple.c
> > > +++ b/lib/readline_simple.c
> > > @@ -58,6 +58,8 @@ int readline (const char *prompt, char *line, int len)
> > >   	for (;;) {
> > >   		c = getchar();
> > > +		if (c < 0)
> > > +			return (-1);
> > 
> > I don't like made up error codes. Is there any reason why we couldn't
> > just pass through the negative error code from getchar?
> 
> And also change the type of the 'c' variable, as it is currently a plain
> 'char'?
> 

Indeed :)

Regards,
Gaël

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/2] readline_simple: return -1 if getc fails

[Gaël PORTAY]

Hi Lucas,

On Tue, Aug 08, 2017 at 09:51:54AM +0200, Lucas Stach wrote:
> Am Montag, den 07.08.2017, 18:10 -0400 schrieb Gaël PORTAY:
> > The getc function may return an errno code if an error happens.
> > 
> > This patch prevents readline from printing a non printable character and
> > from looping to infinity and beyong.
> > 
> > Signed-off-by: Gaël PORTAY <gael.portay@savoirfairelinux.com>
> > ---
> >  lib/readline_simple.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/lib/readline_simple.c b/lib/readline_simple.c
> > index c4d3d240e..1283c9602 100644
> > --- a/lib/readline_simple.c
> > +++ b/lib/readline_simple.c
> > @@ -58,6 +58,8 @@ int readline (const char *prompt, char *line, int len)
> >  
> >  	for (;;) {
> >  		c = getchar();
> > +		if (c < 0)
> > +			return (-1);
> 
> I don't like made up error codes. Is there any reason why we couldn't
> just pass through the negative error code from getchar?
> 

The thing here is that getchar() may return an error, and that error is not
tested. This causes readline to print the character 0xea (-EINVAL) which is not
printable.

Maybe another solutions would be to print the errno string and continue; or to
return the number of characters already read.

Regards,
Gaël

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/2] readline_simple: return -1 if getc fails

[Lucas Stach]

Am Dienstag, den 08.08.2017, 11:20 -0400 schrieb Gaël PORTAY:
> Hi Lucas,
> 
> On Tue, Aug 08, 2017 at 09:51:54AM +0200, Lucas Stach wrote:
> > Am Montag, den 07.08.2017, 18:10 -0400 schrieb Gaël PORTAY:
> > > The getc function may return an errno code if an error happens.
> > > 
> > > This patch prevents readline from printing a non printable character and
> > > from looping to infinity and beyong.
> > > 
> > > Signed-off-by: Gaël PORTAY <gael.portay@savoirfairelinux.com>
> > > ---
> > >  lib/readline_simple.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/lib/readline_simple.c b/lib/readline_simple.c
> > > index c4d3d240e..1283c9602 100644
> > > --- a/lib/readline_simple.c
> > > +++ b/lib/readline_simple.c
> > > @@ -58,6 +58,8 @@ int readline (const char *prompt, char *line, int len)
> > >  
> > >  	for (;;) {
> > >  		c = getchar();
> > > +		if (c < 0)
> > > +			return (-1);
> > 
> > I don't like made up error codes. Is there any reason why we couldn't
> > just pass through the negative error code from getchar?
> > 
> 
> The thing here is that getchar() may return an error, and that error is not
> tested. This causes readline to print the character 0xea (-EINVAL) which is not
> printable.

So why wouldn't the following fix the issue?

signed char c;

if (c < 0)
	return c;

Regards,
Lucas



_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/2] readline_simple: return -1 if getc fails

[Ian Abbott]

On 08/08/17 16:36, Lucas Stach wrote:
> Am Dienstag, den 08.08.2017, 11:20 -0400 schrieb Gaël PORTAY:
>> Hi Lucas,
>>
>> On Tue, Aug 08, 2017 at 09:51:54AM +0200, Lucas Stach wrote:
>>> Am Montag, den 07.08.2017, 18:10 -0400 schrieb Gaël PORTAY:
>>>> The getc function may return an errno code if an error happens.
>>>>
>>>> This patch prevents readline from printing a non printable character and
>>>> from looping to infinity and beyong.
>>>>
>>>> Signed-off-by: Gaël PORTAY <gael.portay@savoirfairelinux.com>
>>>> ---
>>>>   lib/readline_simple.c | 2 ++
>>>>   1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/lib/readline_simple.c b/lib/readline_simple.c
>>>> index c4d3d240e..1283c9602 100644
>>>> --- a/lib/readline_simple.c
>>>> +++ b/lib/readline_simple.c
>>>> @@ -58,6 +58,8 @@ int readline (const char *prompt, char *line, int len)
>>>>   
>>>>   	for (;;) {
>>>>   		c = getchar();
>>>> +		if (c < 0)
>>>> +			return (-1);
>>>
>>> I don't like made up error codes. Is there any reason why we couldn't
>>> just pass through the negative error code from getchar?
>>>
>>
>> The thing here is that getchar() may return an error, and that error is not
>> tested. This causes readline to print the character 0xea (-EINVAL) which is not
>> printable.
> 
> So why wouldn't the following fix the issue?
> 
> signed char c;

`int` would be better to allow non-ASCII characters.

> 
> if (c < 0)
> 	return c;

There are places where the return value is checked for `-1` for example 
in get_user_input() ("common/hush.c"), and in run_shell() 
("common/parser.c").

I think Gaël's patch is reasonable, although perhaps it should also set 
`line[0] = '\0';` before returning.

Off topic: there is another oddity in the the "simple" version of 
readline(). It ignores the `len` parameter and uses `CONFIG_CBSIZE` instead.

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk> )=-
-=(                          Web: http://www.mev.co.uk/  )=-

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/2] readline_simple: return -1 if getc fails

[Gaël PORTAY]

On Tue, Aug 08, 2017 at 05:05:35PM +0100, Ian Abbott wrote:
> On 08/08/17 16:36, Lucas Stach wrote:
> > Am Dienstag, den 08.08.2017, 11:20 -0400 schrieb Gaël PORTAY:
> > > Hi Lucas,
> > > 
> > > On Tue, Aug 08, 2017 at 09:51:54AM +0200, Lucas Stach wrote:
> > > > (...)
> > > > I don't like made up error codes. Is there any reason why we couldn't
> > > > just pass through the negative error code from getchar?
> > > > 
> > > 
> > > The thing here is that getchar() may return an error, and that error is not
> > > tested. This causes readline to print the character 0xea (-EINVAL) which is not
> > > printable.
> > 
> > So why wouldn't the following fix the issue?
> > 
> > signed char c;
> 
> `int` would be better to allow non-ASCII characters.
> 
> > 
> > if (c < 0)
> > 	return c;
> 
> There are places where the return value is checked for `-1` for example in
> get_user_input() ("common/hush.c"), and in run_shell() ("common/parser.c").
> 
> I think Gaël's patch is reasonable, although perhaps it should also set
> `line[0] = '\0';` before returning.
> 

Indeed, or `line[n] = '\0';` to preserve characters already entered.

BTW, it already performed by get_user_input in hush.c (n is reset to 0).

console_buffer[n] = '\n';
console_buffer[n + 1]= '\0';

> Off topic: there is another oddity in the the "simple" version of
> readline(). It ignores the `len` parameter and uses `CONFIG_CBSIZE` instead.
> 

I made a patch for this; but I have not tested yet all cases.

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/2] readline_simple: return -1 if getc fails

[Gaël PORTAY]

On Tue, Aug 08, 2017 at 05:36:14PM +0200, Lucas Stach wrote:
> Am Dienstag, den 08.08.2017, 11:20 -0400 schrieb Gaël PORTAY:
> > Hi Lucas,
> > 
> > On Tue, Aug 08, 2017 at 09:51:54AM +0200, Lucas Stach wrote:
> > > > diff --git a/lib/readline_simple.c b/lib/readline_simple.c
> > > > index c4d3d240e..1283c9602 100644
> > > > --- a/lib/readline_simple.c
> > > > +++ b/lib/readline_simple.c
> > > > @@ -58,6 +58,8 @@ int readline (const char *prompt, char *line, int len)
> > > >  
> > > >  	for (;;) {
> > > >  		c = getchar();
> > > > +		if (c < 0)
> > > > +			return (-1);
> > > 
> > > I don't like made up error codes. Is there any reason why we couldn't
> > > just pass through the negative error code from getchar?
> > > 
> > 
> > The thing here is that getchar() may return an error, and that error is not
> > tested. This causes readline to print the character 0xea (-EINVAL) which is not
> > printable.
> 
> So why wouldn't the following fix the issue?
> 
> signed char c;
> 
> if (c < 0)
> 	return c;
> 

Okay. I do prefer your solution.

I returned -1 mainly because the function comment says it returns -1 when it
breaks; and because parser.c and hush.c test readline function against -1 and
not against a negative value.

Also readline returns -1 if character is 0x03. Maybe it should return -EINTR;
-1 is EPERM: Operation not permitted.

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/2] readline_simple: remove obsolete documentation

[Gaël PORTAY]

The readline function does not return timed-out anymore since the
commit:

	be5d13bcb readline_simple: drop unusable u-boot legacy stuff

Signed-off-by: Gaël PORTAY <gael.portay@savoirfairelinux.com>
---
 lib/readline_simple.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/lib/readline_simple.c b/lib/readline_simple.c
index 1283c9602..80e075bc5 100644
--- a/lib/readline_simple.c
+++ b/lib/readline_simple.c
@@ -35,11 +35,8 @@ static char * delete_char (char *buffer, char *p, int *colp, int *np, int plen)
 
 /*
  * Prompt for input and read a line.
- * If  CONFIG_BOOT_RETRY_TIME is defined and retry_time >= 0,
- * time out when time goes past endtime (timebase time in ticks).
  * Return:	number of read characters
  *		-1 if break
- *		-2 if timed out
  */
 int readline (const char *prompt, char *line, int len)
 {
-- 
2.13.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox