From cc305213d78e282d9e8a43106491d033e990ccdc Mon Sep 17 00:00:00 2001 From: Björn Linse Date: Tue, 2 Oct 2018 13:46:53 +0200 Subject: TUI: always use safe cursor movement after resize The old code could lead to a memory error in the following situation: 0. The previous cursor position was row 50 since before, on a grid larger than 50 rows. 1. grid_resize changes the grid height to 40, and invalidly assumes the resize moved the physical cursor to row 0 2. Some event used a operation that could move the cursor (such as clear), and then reset the cursor to the "true" position row 50 (pointless after #8221, but I forgot to remove it) 3. raw_line/cheap_to_print is invoked, and tries to inspect the grid at row 50 (memory error) 4. grid_cursor_goto would have been called at this point, and set a valid cursor position 0-39. --- src/nvim/tui/tui.c | 19 +++++++++++-------- src/nvim/ugrid.c | 1 - 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/src/nvim/tui/tui.c b/src/nvim/tui/tui.c index 3ed0fe0cd6..bc85b43401 100644 --- a/src/nvim/tui/tui.c +++ b/src/nvim/tui/tui.c @@ -577,7 +577,7 @@ static void final_column_wrap(UI *ui) { TUIData *data = ui->data; UGrid *grid = &data->grid; - if (grid->col == ui->width) { + if (grid->row != -1 && grid->col == ui->width) { grid->col = 0; if (grid->row < MIN(ui->height, grid->height - 1)) { grid->row++; @@ -647,6 +647,9 @@ static void cursor_goto(UI *ui, int row, int col) ugrid_goto(grid, row, col); return; } + if (grid->row == -1) { + goto safe_move; + } if (0 == col ? col != grid->col : row != grid->row ? false : 1 == col ? 2 < grid->col && cheap_to_print(ui, grid->row, 0, col) : @@ -725,6 +728,8 @@ static void cursor_goto(UI *ui, int row, int col) return; } } + +safe_move: unibi_goto(ui, row, col); ugrid_goto(grid, row, col); } @@ -782,9 +787,6 @@ static void clear_region(UI *ui, int top, int bot, int left, int right, data->did_resize = false; } } - - // restore cursor - cursor_goto(ui, data->row, data->col); } static void set_scroll_region(UI *ui, int top, int bot, int left, int right) @@ -808,7 +810,7 @@ static void set_scroll_region(UI *ui, int top, int bot, int left, int right) unibi_out(ui, unibi_set_right_margin_parm); } } - unibi_goto(ui, grid->row, grid->col); + grid->row = -1; } static void reset_scroll_region(UI *ui, bool fullwidth) @@ -836,7 +838,7 @@ static void reset_scroll_region(UI *ui, bool fullwidth) } unibi_out_ext(ui, data->unibi_ext.disable_lr_margin); } - unibi_goto(ui, grid->row, grid->col); + grid->row = -1; } static void tui_grid_resize(UI *ui, Integer g, Integer width, Integer height) @@ -864,6 +866,7 @@ static void tui_grid_resize(UI *ui, Integer g, Integer width, Integer height) } } else { // Already handled the SIGWINCH signal; avoid double-resize. got_winch = false; + grid->row = -1; } } @@ -880,9 +883,10 @@ static void tui_grid_clear(UI *ui, Integer g) static void tui_grid_cursor_goto(UI *ui, Integer grid, Integer row, Integer col) { TUIData *data = ui->data; + + // cursor position is validated in tui_flush data->row = (int)row; data->col = (int)col; - cursor_goto(ui, (int)row, (int)col); } CursorShape tui_cursor_decode_shape(const char *shape_str) @@ -1070,7 +1074,6 @@ static void tui_grid_scroll(UI *ui, Integer g, Integer startrow, Integer endrow, if (!data->scroll_region_is_full_screen) { reset_scroll_region(ui, fullwidth); } - cursor_goto(ui, data->row, data->col); if (!(data->bce || no_bg(ui, data->clear_attrs))) { // Scrolling will leave wrong background in the cleared area on non-BCE diff --git a/src/nvim/ugrid.c b/src/nvim/ugrid.c index e2b92d7112..b741a61d8c 100644 --- a/src/nvim/ugrid.c +++ b/src/nvim/ugrid.c @@ -32,7 +32,6 @@ void ugrid_resize(UGrid *grid, int width, int height) grid->cells[i] = xcalloc((size_t)width, sizeof(UCell)); } - grid->row = grid->col = 0; grid->width = width; grid->height = height; } -- cgit From 1bf83ea8e1314dff43f78bb41835537afda9cbaf Mon Sep 17 00:00:00 2001 From: Björn Linse Date: Tue, 2 Oct 2018 13:48:52 +0200 Subject: TUI: delete "first-row" workaround after resize This was caused by cursor position being invalid right after tui_grid_resize, which is now fixed --- src/nvim/tui/tui.c | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/src/nvim/tui/tui.c b/src/nvim/tui/tui.c index bc85b43401..dd22f00de0 100644 --- a/src/nvim/tui/tui.c +++ b/src/nvim/tui/tui.c @@ -88,7 +88,6 @@ typedef struct { bool cont_received; UGrid grid; kvec_t(Rect) invalid_regions; - bool did_resize; int row, col; int out_fd; bool scroll_region_is_full_screen; @@ -778,14 +777,6 @@ static void clear_region(UI *ui, int top, int bot, int left, int right, cursor_goto(ui, row, col); print_cell(ui, cell); }); - - if (data->did_resize && top == 0) { - // TODO(bfredl): the first line of the screen doesn't gets properly - // cleared after resize by the loop above, so redraw the final state - // after the next flush. - invalidate(ui, 0, bot, left, right); - data->did_resize = false; - } } } @@ -846,7 +837,6 @@ static void tui_grid_resize(UI *ui, Integer g, Integer width, Integer height) TUIData *data = ui->data; UGrid *grid = &data->grid; ugrid_resize(grid, (int)width, (int)height); - data->did_resize = true; // resize might not always be followed by a clear before flush // so clip the invalid region -- cgit From 075dc42fb24b98c3241f39c6edb1da21e8cede09 Mon Sep 17 00:00:00 2001 From: Björn Linse Date: Sat, 6 Oct 2018 10:56:00 +0200 Subject: test: replace wait() with pre-assertion in assert_term_colors --- test/functional/terminal/tui_spec.lua | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/test/functional/terminal/tui_spec.lua b/test/functional/terminal/tui_spec.lua index 09f80ca849..834720edc6 100644 --- a/test/functional/terminal/tui_spec.lua +++ b/test/functional/terminal/tui_spec.lua @@ -17,7 +17,6 @@ local nvim_prog = helpers.nvim_prog local nvim_set = helpers.nvim_set local ok = helpers.ok local read_file = helpers.read_file -local wait = helpers.wait if helpers.pending_win32(pending) then return end @@ -473,14 +472,24 @@ describe("tui 't_Co' (terminal colors)", function() nvim_prog, nvim_set)) - feed_data(":echo &t_Co\n") - wait() local tline if maxcolors == 8 or maxcolors == 16 then tline = "~ " else tline = "{4:~ }" end + + screen:expect(string.format([[ + {1: } | + %s| + %s| + %s| + %s| + | + {3:-- TERMINAL --} | + ]], tline, tline, tline, tline)) + + feed_data(":echo &t_Co\n") screen:expect(string.format([[ {1: } | %s| -- cgit