[PATCH] auxdisplay: charlcd: fix x/y command parsing

[PATCH] auxdisplay: charlcd: fix x/y command parsing

[Mans Rullgard]

Commit b34050fadb86 ("auxdisplay: charlcd: Fix and clean up handling of
x/y commands") fixed some problems by rewriting the parsing code,
but also broke things further by removing the check for a complete
command before attempting to parse it.  As a result, parsing is
terminated at the first x or y character.

This reinstates the check for a final semicolon.  Whereas the original
code use strchr(), this is wasteful seeing as the semicolon is always
at the end of the buffer.  Thus check this character directly instead.

Signed-off-by: Mans Rullgard <mans@mansr.com>
---
 drivers/auxdisplay/charlcd.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c
index 81c22d20d9d9..60e0b772673f 100644
--- a/drivers/auxdisplay/charlcd.c
+++ b/drivers/auxdisplay/charlcd.c
@@ -538,6 +538,9 @@ static inline int handle_lcd_special_code(struct charlcd *lcd)
 	}
 	case 'x':	/* gotoxy : LxXXX[yYYY]; */
 	case 'y':	/* gotoxy : LyYYY[xXXX]; */
+		if (priv->esc_seq.buf[priv->esc_seq.len - 1] != ';')
+			break;
+
 		/* If the command is valid, move to the new address */
 		if (parse_xy(esc, &priv->addr.x, &priv->addr.y))
 			charlcd_gotoxy(lcd);
-- 
2.19.2

Re: [PATCH] auxdisplay: charlcd: fix x/y command parsing

[Miguel Ojeda]

Hi Mans,

[CC'ing a few people involved previously on this]

On Wed, Dec 5, 2018 at 2:53 PM Mans Rullgard <mans@mansr.com> wrote:
>
> Commit b34050fadb86 ("auxdisplay: charlcd: Fix and clean up handling of
> x/y commands") fixed some problems by rewriting the parsing code,
> but also broke things further by removing the check for a complete
> command before attempting to parse it.  As a result, parsing is
> terminated at the first x or y character.

Why is it necessary to check for ";" to be exactly at the end? Or, in
other words, what requires it?

If the syntax supported by parse_xy() is wrong for some reason, we
should fix that (instead of adding a check before calling it).

Thanks for taking a look at this!

Cheers,
Miguel

Re: [PATCH] auxdisplay: charlcd: fix x/y command parsing

[Måns Rullgård]

Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> writes:

> Hi Mans,
>
> [CC'ing a few people involved previously on this]
>
> On Wed, Dec 5, 2018 at 2:53 PM Mans Rullgard <mans@mansr.com> wrote:
>>
>> Commit b34050fadb86 ("auxdisplay: charlcd: Fix and clean up handling of
>> x/y commands") fixed some problems by rewriting the parsing code,
>> but also broke things further by removing the check for a complete
>> command before attempting to parse it.  As a result, parsing is
>> terminated at the first x or y character.
>
> Why is it necessary to check for ";" to be exactly at the end? Or, in
> other words, what requires it?
>
> If the syntax supported by parse_xy() is wrong for some reason, we
> should fix that (instead of adding a check before calling it).

Suppose the command "\e[Lx0y0;" is written to the device.  The
charlcd_write_char() function adds one character at a time to the escape
sequence buffer.  Once the 'L' has been seen, it calls
handle_lcd_special_code() after each additional character is added.  On
encountering the 'x' this function will attempt to parse the command
using parse_xy(), which fails since it is incomplete.  It is nonetheless
reported as processed, and the escape sequence is flushed.  The
remainder of the command (i.e. "0y0;") is then displayed as text.

Since parse_xy() can never return success (true) unless there is a
semicolon, it is pointless to call it until we have seen one.  With the
characters being added to the buffer one by one, it is enough to check
for semicolon at the end.

BTW, the parsing of this command has been broken since 3.2-rc1 due to
commit 129957069e6a.

-- 
Måns Rullgård

Re: [PATCH] auxdisplay: charlcd: fix x/y command parsing

[Miguel Ojeda]

On Wed, Dec 5, 2018 at 6:58 PM Måns Rullgård <mans@mansr.com> wrote:
>
> Suppose the command "\e[Lx0y0;" is written to the device.  The
> charlcd_write_char() function adds one character at a time to the escape
> sequence buffer.

Ah, yes, that is much more clear. Indeed, parse_xy() expects the
entire command; the strchr() should still be there.

By the way, if we are going to move to a direct check, I would also do
so in the generator command too if possible, to be consistent (in
another patch, possibly).

> BTW, the parsing of this command has been broken since 3.2-rc1 due to
> commit 129957069e6a.

Thanks for checking! Are you able to test this?

Cheers,
Miguel

Re: [PATCH] auxdisplay: charlcd: fix x/y command parsing

[Måns Rullgård]

Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> writes:

> On Wed, Dec 5, 2018 at 6:58 PM Måns Rullgård <mans@mansr.com> wrote:
>>
>> Suppose the command "\e[Lx0y0;" is written to the device.  The
>> charlcd_write_char() function adds one character at a time to the escape
>> sequence buffer.
>
> Ah, yes, that is much more clear. Indeed, parse_xy() expects the
> entire command; the strchr() should still be there.
>
> By the way, if we are going to move to a direct check, I would also do
> so in the generator command too if possible, to be consistent (in
> another patch, possibly).

I'd consider that separately.  You're the maintainer, so it's up to you.

>> BTW, the parsing of this command has been broken since 3.2-rc1 due to
>> commit 129957069e6a.
>
> Thanks for checking! Are you able to test this?

Yes, that's how I noticed it was broken.

-- 
Måns Rullgård

Re: [PATCH] auxdisplay: charlcd: fix x/y command parsing

[Miguel Ojeda]

Hi Måns,

On Thu, Dec 6, 2018 at 1:06 PM Måns Rullgård <mans@mansr.com> wrote:
>
> >> BTW, the parsing of this command has been broken since 3.2-rc1 due to
> >> commit 129957069e6a.
> >
> > Thanks for checking! Are you able to test this?
>
> Yes, that's how I noticed it was broken.

Fantastic, thank you.

Can you please add the explanation about commit 129957069e6a
("staging: panel: Fixed checkpatch warning about simple_strtoul()") in
the commit message so that we don't lose it? I will pick it into
linux-next.

Cheers,
Miguel

Re: [PATCH] auxdisplay: charlcd: fix x/y command parsing

[Måns Rullgård]

Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> writes:

> Hi Måns,
>
> On Thu, Dec 6, 2018 at 1:06 PM Måns Rullgård <mans@mansr.com> wrote:
>>
>> >> BTW, the parsing of this command has been broken since 3.2-rc1 due to
>> >> commit 129957069e6a.
>> >
>> > Thanks for checking! Are you able to test this?
>>
>> Yes, that's how I noticed it was broken.
>
> Fantastic, thank you.
>
> Can you please add the explanation about commit 129957069e6a
> ("staging: panel: Fixed checkpatch warning about simple_strtoul()") in
> the commit message so that we don't lose it? I will pick it into
> linux-next.

The bad code from that commit was already completely replaced with the
new parse_xy() function.

-- 
Måns Rullgård

Re: [PATCH] auxdisplay: charlcd: fix x/y command parsing

[Miguel Ojeda]

On Fri, Dec 14, 2018 at 1:15 PM Måns Rullgård <mans@mansr.com> wrote:
>
> The bad code from that commit was already completely replaced with the
> new parse_xy() function.

Yes, but this fixes something that was broken for a longer time, so it
is good to be able to tell the range of commits that were broken.

Cheers,
Miguel

Re: [PATCH] auxdisplay: charlcd: fix x/y command parsing

[Robert Abel]

Hi Miguel,

On 05 Dec 2018 17:47, Miguel Ojeda wrote:> Hi Mans,
>
> [CC'ing a few people involved previously on this]

Thanks for CC'ing me!

On 06 Dec 2018 11:06, Miguel Ojeda wrote [to Mans Rullgard]:
> Are you able to test this?

It's unfortunate that my original comment got ignored back when the breaking patch was submitted.
Nevertheless, if somebody posts a patch, I'm happy to test.

Regards,

Robert

Re: [PATCH] auxdisplay: charlcd: fix x/y command parsing

[Miguel Ojeda]

Hi Robert,

On Fri, Dec 7, 2018 at 12:13 AM Robert Abel <rabel@robertabel.eu> wrote:
>
> Hi Miguel,
>
> On 05 Dec 2018 17:47, Miguel Ojeda wrote:> Hi Mans,
> >
> > [CC'ing a few people involved previously on this]
>
> Thanks for CC'ing me!
>
> On 06 Dec 2018 11:06, Miguel Ojeda wrote [to Mans Rullgard]:
> > Are you able to test this?
>
> It's unfortunate that my original comment got ignored back when the breaking patch was submitted.
> Nevertheless, if somebody posts a patch, I'm happy to test.

Which one? Do you mean the original 129957069e6a in 3.2-rc1? Or the
more recent ones? At the time of 3.2-rc1 I couldn't be involved in
kernel dev :-( If you mean in the recent ones, sorry if I failed to
see some email or missed something -- it was not intended at all.
Please feel free to nag me/everyone a bit more :-)

If you are also able to test, that is wonderful as well!

Cheers,
Miguel