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

[Christophe de Dinechin]

> On 27 Jun 2019, at 07:35, Gerd Hoffmann <address@hidden> 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 <address@hidden>
> Signed-off-by: Gerd Hoffmann <address@hidden>
> ---
> 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 <address@hidden>

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