[Qemu-devel] [PATCH] console: fix cell overflow

[Qemu-devel] [PATCH] console: fix cell overflow

[Gerd Hoffmann]

Linux terminal behavior (coming from vt100 I think) is somewhat strange
when it comes to line wraps:  When a character is printed to the last
char cell of a line the cursor does NOT jump to the next line but stays
where it is.  The line feed happens when the next character is printed.

So the valid range for the cursor position is not 0 .. width-1 but
0 .. width, where x == width represents the state where the line is
full but the cursor didn't jump to the next line yet.

The code for the 'clear from start of line' control sequence (ESC[1K)
fails to handle this corner case correctly and may call
console_clear_xy() with x == width.  That will incorrectly clear the
first char cell of the next line, or in case the cursor happens to be on
the last line overflow the cell buffer by one character (three bytes).

Add a check to the loop to fix that.

Didn't spot any other places with the same problem.  But it's easy to
miss that corner case, so also allocate one extra cell as precaution, so
in case we have simliar issues lurking elsewhere it at least wouldn't be
a buffer overflow.

Reported-by: Alexander Oleinik <alxndr@bu.edu>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/console.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ui/console.c b/ui/console.c
index eb7e7e0c517a..13d933510cdb 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -484,7 +484,7 @@ static void text_console_resize(QemuConsole *s)
     if (s->width < w1)
         w1 = s->width;
 
-    cells = g_new(TextCell, s->width * s->total_height);
+    cells = g_new(TextCell, s->width * s->total_height + 1);
     for(y = 0; y < s->total_height; y++) {
         c = &cells[y * s->width];
         if (w1 > 0) {
@@ -992,7 +992,7 @@ static void console_putchar(QemuConsole *s, int ch)
                     break;
                 case 1:
                     /* clear from beginning of line */
-                    for (x = 0; x <= s->x; x++) {
+                    for (x = 0; x <= s->x && x < s->width; x++) {
                         console_clear_xy(s, x, s->y);
                     }
                     break;
-- 
2.18.1

Re: [Qemu-devel] [PATCH] console: fix cell overflow

[Christophe de Dinechin]

> On 27 Jun 2019, at 07:35, Gerd Hoffmann <kraxel@redhat.com> wrote:
> 
> Linux terminal behavior (coming from vt100 I think) is somewhat strange
> when it comes to line wraps:  When a character is printed to the last
> char cell of a line the cursor does NOT jump to the next line but stays
> where it is.  The line feed happens when the next character is printed.
> 
> So the valid range for the cursor position is not 0 .. width-1 but
> 0 .. width, where x == width represents the state where the line is
> full but the cursor didn't jump to the next line yet.
> 
> The code for the 'clear from start of line' control sequence (ESC[1K)
> fails to handle this corner case correctly and may call
> console_clear_xy() with x == width.  That will incorrectly clear the
> first char cell of the next line, or in case the cursor happens to be on
> the last line overflow the cell buffer by one character (three bytes).
> 
> Add a check to the loop to fix that.
> 
> Didn't spot any other places with the same problem.  But it's easy to
> miss that corner case, so also allocate one extra cell as precaution, so
> in case we have simliar issues lurking elsewhere it at least wouldn't be
> a buffer overflow.
> 
> Reported-by: Alexander Oleinik <alxndr@bu.edu>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
> ui/console.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/ui/console.c b/ui/console.c
> index eb7e7e0c517a..13d933510cdb 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -484,7 +484,7 @@ static void text_console_resize(QemuConsole *s)
>     if (s->width < w1)
>         w1 = s->width;
> 
> -    cells = g_new(TextCell, s->width * s->total_height);
> +    cells = g_new(TextCell, s->width * s->total_height + 1);

I don’t like allocating just in case. At least put a comment explaining why ;-)
This extra byte only catches a single case (arguably an existing one).

What about adding a couple of extra tests where cell[… + x] is used?
(there is a third location I did not protect, because it already had
exactly that test)

diff --git a/ui/console.c b/ui/console.c
index eb7e7e0c51..00d27f6384 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -541,6 +541,9 @@ static void update_xy(QemuConsole *s, int x, int y)
         y2 += s->total_height;
     }
     if (y2 < s->height) {
+        if (x >= s->width) {
+            x = s->width - 1;
+        }
         c = &s->cells[y1 * s->width + x];
         vga_putcharxy(s, x, y2, c->ch,
                       &(c->t_attrib));
@@ -787,6 +790,9 @@ static void console_handle_escape(QemuConsole *s)
 static void console_clear_xy(QemuConsole *s, int x, int y)
 {
     int y1 = (s->y_base + y) % s->total_height;
+    if (x >= s->width) {
+        x = s->width - 1;
+    }
     TextCell *c = &s->cells[y1 * s->width + x];
     c->ch = ' ';
     c->t_attrib = s->t_attrib_default;


Reviewed-by: Christophe de Dinechin <dinechin@redhat.com>

>     for(y = 0; y < s->total_height; y++) {
>         c = &cells[y * s->width];
>         if (w1 > 0) {
> @@ -992,7 +992,7 @@ static void console_putchar(QemuConsole *s, int ch)
>                     break;
>                 case 1:
>                     /* clear from beginning of line */
> -                    for (x = 0; x <= s->x; x++) {
> +                    for (x = 0; x <= s->x && x < s->width; x++) {
>                         console_clear_xy(s, x, s->y);
>                     }
>                     break;
> -- 
> 2.18.1
> 
>