auxdisplay: charlcd: fix x/y command parsing

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.

---
 drivers/auxdisplay/charlcd.c | 3 +++
 1 file changed, 3 insertions(+)

index 81c22d20d9d9..60e0b772673f 100644
@@ -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

Hi Mans,

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

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

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

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

Thanks for checking! Are you able to test this?

Cheers,
Miguel

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

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

Måns Rullgård

Hi Måns,

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

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

Måns Rullgård

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

Hi Miguel,

On 05 Dec 2018 17:47, Miguel Ojeda wrote:> Hi Mans,

Thanks for CC'ing me!

On 06 Dec 2018 11:06, Miguel Ojeda wrote [to Mans Rullgard]:

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

Hi Robert,

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