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

[Ian Abbott <abbotti@mev.co.uk>]

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