[PATCH 1/2] Fix broken reflowing after insert/delete characters

[Balazs Kezes]

Steps to reproduce:
1. Create a vertical split.
2. Assuming we are running bash in the left pane, enter this:
echo -e 'xy\e[D\e[@'
# or
echo -e 'xyz\e[2D\e[P'
3. Make the left pane smaller:
tmux resize-pane -L 5
4. Observe the extra newline. This is ugly.

This happens because whenever insert/delete characters in a line, we
extend that line to full width with blanks at the end. This patch will
use the real length instead.

This is especially annoying if you use readline's insert-comment a lot
which uses this facility to insert the comment at the beginning of the
line.
---
grid-view.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/grid-view.c b/grid-view.c
index e75b604..45737e3 100644
--- a/grid-view.c
+++ b/grid-view.c
@@ -184,7 +184,7 @@ grid_view_insert_cells(struct grid *gd, u_int px, u_int py, u_int nx)
px = grid_view_x(gd, px);
py = grid_view_y(gd, py);

- sx = grid_view_x(gd, gd->sx);
+ sx = grid_view_x(gd, gd->linedata[py].cellsize);

if (px == sx - 1)
grid_clear(gd, px, py, 1, 1);
@@ -201,7 +201,7 @@ grid_view_delete_cells(struct grid *gd, u_int px, u_int py, u_int nx)
px = grid_view_x(gd, px);
py = grid_view_y(gd, py);

- sx = grid_view_x(gd, gd->sx);
+ sx = grid_view_x(gd, gd->linedata[py].cellsize);

grid_move_cells(gd, px, px + nx, py, sx - px - nx);
grid_clear(gd, sx - nx, py, px + nx - (sx - nx), 1);

[Nicholas Marriott]

Both this and the other one make sense to me - applied, thanks!

Post by Balazs Kezes
1. Create a vertical split.
# or
echo -e 'xyz\e[2D\e[P'
tmux resize-pane -L 5
4. Observe the extra newline. This is ugly.
This happens because whenever insert/delete characters in a line, we
extend that line to full width with blanks at the end. This patch will
use the real length instead.
This is especially annoying if you use readline's insert-comment a lot
which uses this facility to insert the comment at the beginning of the
line.
---
grid-view.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/grid-view.c b/grid-view.c
index e75b604..45737e3 100644
--- a/grid-view.c
+++ b/grid-view.c
@@ -184,7 +184,7 @@ grid_view_insert_cells(struct grid *gd, u_int px, u_int py, u_int nx)
px = grid_view_x(gd, px);
py = grid_view_y(gd, py);
- sx = grid_view_x(gd, gd->sx);
+ sx = grid_view_x(gd, gd->linedata[py].cellsize);
if (px == sx - 1)
grid_clear(gd, px, py, 1, 1);
@@ -201,7 +201,7 @@ grid_view_delete_cells(struct grid *gd, u_int px, u_int py, u_int nx)
px = grid_view_x(gd, px);
py = grid_view_y(gd, py);
- sx = grid_view_x(gd, gd->sx);
+ sx = grid_view_x(gd, gd->linedata[py].cellsize);
grid_move_cells(gd, px, px + nx, py, sx - px - nx);
grid_clear(gd, sx - nx, py, px + nx - (sx - nx), 1);
--
2.1.3
------------------------------------------------------------------------------
_______________________________________________
tmux-users mailing list
https://lists.sourceforge.net/lists/listinfo/tmux-users

------------------------------------------------------------------------------

[Nicholas Marriott]

Hi

I reverted the grid-view.c change because it breaks insertion. Try:

$ tmux new 'tput ich 10'

I guess probably the line does need to be extended a bit...

Post by Nicholas Marriott
Both this and the other one make sense to me - applied, thanks!

Post by Balazs Kezes
1. Create a vertical split.
# or
echo -e 'xyz\e[2D\e[P'
tmux resize-pane -L 5
4. Observe the extra newline. This is ugly.
This happens because whenever insert/delete characters in a line, we
extend that line to full width with blanks at the end. This patch will
use the real length instead.
This is especially annoying if you use readline's insert-comment a lot
which uses this facility to insert the comment at the beginning of the
line.
---
grid-view.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/grid-view.c b/grid-view.c
index e75b604..45737e3 100644
--- a/grid-view.c
+++ b/grid-view.c
@@ -184,7 +184,7 @@ grid_view_insert_cells(struct grid *gd, u_int px, u_int py, u_int nx)
px = grid_view_x(gd, px);
py = grid_view_y(gd, py);
- sx = grid_view_x(gd, gd->sx);
+ sx = grid_view_x(gd, gd->linedata[py].cellsize);
if (px == sx - 1)
grid_clear(gd, px, py, 1, 1);
@@ -201,7 +201,7 @@ grid_view_delete_cells(struct grid *gd, u_int px, u_int py, u_int nx)
px = grid_view_x(gd, px);
py = grid_view_y(gd, py);
- sx = grid_view_x(gd, gd->sx);
+ sx = grid_view_x(gd, gd->linedata[py].cellsize);
grid_move_cells(gd, px, px + nx, py, sx - px - nx);
grid_clear(gd, sx - nx, py, px + nx - (sx - nx), 1);
--
2.1.3
------------------------------------------------------------------------------
_______________________________________________
tmux-users mailing list
https://lists.sourceforge.net/lists/listinfo/tmux-users

[Balazs Kezes]

Post by Nicholas Marriott
$ tmux new 'tput ich 10'

Oh, sorry about that. What a silly mistake, the 4th parameter
underflows in grid_move_cells:

...
#1 0x000000000042a3f9 in log_fatal (msg=0x46e461 "%s: %s") at log.c:105
#2 0x0000000000458dba in xreallocarray (oldptr=0x0, nmemb=4294967286, size=14) at xmalloc.c:92
#3 0x000000000042073b in grid_expand_line (gd=0x1af9790, py=1, sx=4294967286) at grid.c:226
#4 0x0000000000420d89 in grid_move_cells (gd=0x1af9790, dx=10, px=0, py=1, nx=4294967286) at grid.c:381
#5 0x0000000000420182 in grid_view_insert_cells (gd=0x1af9790, px=0, py=1, nx=10) at grid-view.c:192
#6 0x0000000000432d14 in screen_write_insertcharacter (ctx=0x1af9528, nx=10) at screen-write.c:543
...

How about this as the bugfix:

diff --git a/grid-view.c b/grid-view.c
index 45737e3..39017c1 100644
--- a/grid-view.c
+++ b/grid-view.c
@@ -185,6 +185,8 @@ grid_view_insert_cells(struct grid *gd, u_int px, u_int py, u_int nx)
py = grid_view_y(gd, py);

sx = grid_view_x(gd, gd->linedata[py].cellsize);
+ if (sx < px + nx)
+ sx = px + nx;

if (px == sx - 1)
grid_clear(gd, px, py, 1, 1);
@@ -202,6 +204,8 @@ grid_view_delete_cells(struct grid *gd, u_int px, u_int py, u_int nx)
py = grid_view_y(gd, py);

sx = grid_view_x(gd, gd->linedata[py].cellsize);
+ if (sx < px + nx)
+ sx = px + nx;

grid_move_cells(gd, px, px + nx, py, sx - px - nx);
grid_clear(gd, sx - nx, py, px + nx - (sx - nx), 1);

[Nicholas Marriott]

Looks good, applied thanks

Post by Balazs Kezes

Post by Nicholas Marriott
$ tmux new 'tput ich 10'

Oh, sorry about that. What a silly mistake, the 4th parameter
...
#1 0x000000000042a3f9 in log_fatal (msg=0x46e461 "%s: %s") at log.c:105
#2 0x0000000000458dba in xreallocarray (oldptr=0x0, nmemb=4294967286, size=14) at xmalloc.c:92
#3 0x000000000042073b in grid_expand_line (gd=0x1af9790, py=1, sx=4294967286) at grid.c:226
#4 0x0000000000420d89 in grid_move_cells (gd=0x1af9790, dx=10, px=0, py=1, nx=4294967286) at grid.c:381
#5 0x0000000000420182 in grid_view_insert_cells (gd=0x1af9790, px=0, py=1, nx=10) at grid-view.c:192
#6 0x0000000000432d14 in screen_write_insertcharacter (ctx=0x1af9528, nx=10) at screen-write.c:543
...
diff --git a/grid-view.c b/grid-view.c
index 45737e3..39017c1 100644
--- a/grid-view.c
+++ b/grid-view.c
@@ -185,6 +185,8 @@ grid_view_insert_cells(struct grid *gd, u_int px, u_int py, u_int nx)
py = grid_view_y(gd, py);
sx = grid_view_x(gd, gd->linedata[py].cellsize);
+ if (sx < px + nx)
+ sx = px + nx;
if (px == sx - 1)
grid_clear(gd, px, py, 1, 1);
@@ -202,6 +204,8 @@ grid_view_delete_cells(struct grid *gd, u_int px, u_int py, u_int nx)
py = grid_view_y(gd, py);
sx = grid_view_x(gd, gd->linedata[py].cellsize);
+ if (sx < px + nx)
+ sx = px + nx;
grid_move_cells(gd, px, px + nx, py, sx - px - nx);
grid_clear(gd, sx - nx, py, px + nx - (sx - nx), 1);
--
Balazs

[Balazs Kezes]

Hey Nick!

Sorry, but this is still broken. :(

You can reproduce it in bash, if you type "a b" then go to the beginning
press "c": you can see that the "b" has disappeared and all you can see
is "ca".

Here's a patch to fix this.

diff --git a/grid-view.c b/grid-view.c
index a34c5a0..defb86d 100644
--- a/grid-view.c
+++ b/grid-view.c
@@ -184,7 +184,10 @@ grid_view_insert_cells(struct grid *gd, u_int px, u_int py, u_int nx)
px = grid_view_x(gd, px);
py = grid_view_y(gd, py);

- sx = grid_view_x(gd, gd->linedata[py].cellsize);
+ sx = gd->linedata[py].cellsize + nx;
+ if (gd->sx < sx)
+ sx = gd->sx;
+ sx = grid_view_x(gd, sx);
if (sx < px + nx)
sx = px + nx;

@@ -203,7 +206,10 @@ grid_view_delete_cells(struct grid *gd, u_int px, u_int py, u_int nx)
px = grid_view_x(gd, px);
py = grid_view_y(gd, py);

- sx = grid_view_x(gd, gd->linedata[py].cellsize);
+ sx = gd->linedata[py].cellsize + nx;
+ if (gd->sx < sx)
+ sx = gd->sx;
+ sx = grid_view_x(gd, sx);
if (sx < px + nx)
sx = px + nx;


But I'm just complicating stuff and I'm not even sure this will cover
all cases based on my previous track record so it's fine if you just
revert my patches for these two functions. I'd need more time to rethink
my approach to fix the original bug in a nice way but I don't think I'll
be able to do that in the very near future.

Thanks!

[Balazs Kezes]

Disregard my previous patch, after a little bit of thinking I came up
with this:

diff --git a/grid-view.c b/grid-view.c
index a34c5a0..0b890d8 100644
--- a/grid-view.c
+++ b/grid-view.c
@@ -184,9 +184,10 @@ grid_view_insert_cells(struct grid *gd, u_int px, u_int py, u_int nx)
px = grid_view_x(gd, px);
py = grid_view_y(gd, py);

- sx = grid_view_x(gd, gd->linedata[py].cellsize);
- if (sx < px + nx)
- sx = px + nx;
+ if (gd->linedata[py].cellsize + nx < gd->sx)
+ sx = grid_view_x(gd, gd->linedata[py].cellsize + nx);
+ else
+ sx = grid_view_x(gd, gd->sx);

if (px == sx - 1)
grid_clear(gd, px, py, 1, 1);

For insertion we need to size it to (cellsize+nx) because we have (nx)
new characters and now the expression (sx-px-nx) cannot go below zero
because (sx-nx) is just (cellsize) and (px) is always strictly smaller
than (cellsize). For the other branch, (gd->sx-px-nx >= 0) is guaranteed
by the calling site.

And I believe the deletion part is already handled well.

Let me know if there's a better way of testing this than trial and
error.

[Nicholas Marriott]

I think this is alright, the calculations seem sensible anyway.

Post by Balazs Kezes
Disregard my previous patch, after a little bit of thinking I came up
diff --git a/grid-view.c b/grid-view.c
index a34c5a0..0b890d8 100644
--- a/grid-view.c
+++ b/grid-view.c
@@ -184,9 +184,10 @@ grid_view_insert_cells(struct grid *gd, u_int px, u_int py, u_int nx)
px = grid_view_x(gd, px);
py = grid_view_y(gd, py);
- sx = grid_view_x(gd, gd->linedata[py].cellsize);
- if (sx < px + nx)
- sx = px + nx;
+ if (gd->linedata[py].cellsize + nx < gd->sx)
+ sx = grid_view_x(gd, gd->linedata[py].cellsize + nx);
+ else
+ sx = grid_view_x(gd, gd->sx);
if (px == sx - 1)
grid_clear(gd, px, py, 1, 1);
For insertion we need to size it to (cellsize+nx) because we have (nx)
new characters and now the expression (sx-px-nx) cannot go below zero
because (sx-nx) is just (cellsize) and (px) is always strictly smaller
than (cellsize). For the other branch, (gd->sx-px-nx >= 0) is guaranteed
by the calling site.
And I believe the deletion part is already handled well.
Let me know if there's a better way of testing this than trial and
error.
--
Balazs

[Nicholas Marriott]

This was causing random crashes, so it's clearly still wrong and I've
reverted back to before any of these changes. I don't have time to look
into it further but maybe you want to have a look (with valgrind
maybe?).

Cheers

Post by Nicholas Marriott
I think this is alright, the calculations seem sensible anyway.

Post by Balazs Kezes
Disregard my previous patch, after a little bit of thinking I came up
diff --git a/grid-view.c b/grid-view.c
index a34c5a0..0b890d8 100644
--- a/grid-view.c
+++ b/grid-view.c
@@ -184,9 +184,10 @@ grid_view_insert_cells(struct grid *gd, u_int px, u_int py, u_int nx)
px = grid_view_x(gd, px);
py = grid_view_y(gd, py);
- sx = grid_view_x(gd, gd->linedata[py].cellsize);
- if (sx < px + nx)
- sx = px + nx;
+ if (gd->linedata[py].cellsize + nx < gd->sx)
+ sx = grid_view_x(gd, gd->linedata[py].cellsize + nx);
+ else
+ sx = grid_view_x(gd, gd->sx);
if (px == sx - 1)
grid_clear(gd, px, py, 1, 1);
For insertion we need to size it to (cellsize+nx) because we have (nx)
new characters and now the expression (sx-px-nx) cannot go below zero
because (sx-nx) is just (cellsize) and (px) is always strictly smaller
than (cellsize). For the other branch, (gd->sx-px-nx >= 0) is guaranteed
by the calling site.
And I believe the deletion part is already handled well.
Let me know if there's a better way of testing this than trial and
error.
--
Balazs

[Balazs Kezes]

Post by Nicholas Marriott
This was causing random crashes, so it's clearly still wrong and I've
reverted back to before any of these changes. I don't have time to
look into it further but maybe you want to have a look (with valgrind
maybe?).

I had no luck reproducing. Neither did valgrind show any suspicious
behavior. I presume you don't have a stack trace, right?

Anyways, I'm fine having this feature reverted as it is harmless and
wasn't that important for me as my other patch from this series. :)

Thanks!

[Nicholas Marriott]

Hi

Unfortunately I don't have the backtrace anymore, I was busy and the
easiest thing at the time was to revert it, it was definitely under
grid_view_insert_cells and I couldn't see anything obvious.

I only saw it very occasionally and never managed to reproduce - I know
it was crashing regularly to someone else in zenicb in emacs on OpenBSD
along with occasional screen corruption - I suspect it is some edge case
that emacs sends under rare circumstances.

tools/fuzz.c (possibly modified to favour insert/delete cells?) might
turn something up but if you don't mind about the feature, don't worry
about it - we can just leave it reverted. We'd need to be very sure it
was right before it went in again anyway.

Post by Balazs Kezes

Post by Nicholas Marriott
This was causing random crashes, so it's clearly still wrong and I've
reverted back to before any of these changes. I don't have time to
look into it further but maybe you want to have a look (with valgrind
maybe?).

I had no luck reproducing. Neither did valgrind show any suspicious
behavior. I presume you don't have a stack trace, right?
Anyways, I'm fine having this feature reverted as it is harmless and
wasn't that important for me as my other patch from this series. :)
Thanks!
--
Balazs

[Balazs Kezes]

Post by Nicholas Marriott
Unfortunately I don't have the backtrace anymore, I was busy and the
easiest thing at the time was to revert it, it was definitely under
grid_view_insert_cells and I couldn't see anything obvious.

I've kept running my main instance with this patch and managed to hit
the problem. After some trial and error I've managed to reproduce and
track down the problem. I made a wrong assumption, namely this one:

(px) is always strictly smaller than (cellsize)

This is not true. Here's a simple repro where this patch segfaults tmux:

echo -e '\e[C\e[@' # cursor right, insert 1 character

So basically my patch needs an extra line like much like the delete has:

if (sx < px+nx) sx = px+nx;

Now the (sx - px - nx) expression will never underflow.

Having said that, I don't mind that this is not in tmux, just wanted to
point out the bug for historical reasons. :)