From 9ea0dfd8c0615a634c410520b3b1096a3908dfef Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Tue, 27 Oct 2020 18:26:06 +0000 Subject: [PATCH] Fix memory corruption in scrollback compression. Introduced by commit 5891142aee5c, in which I invented strbuf_shrink_to() and went round replacing lots of assignments of the form 'sb->len = smaller_value' with calls to it. The bug is also in 0.74, because 34a0460f0561 is a cherry-pick of the commit that introduced it. The difference between that assignment and strbuf_shrink_to is partly that the latter checks by assertion that the new length really is _smaller_ - it doesn't let you accidentally grow a strbuf's length field beyond the limit of its buffer (or indeed at all). But also, strbuf_shrink_to re-establishes the strbuf invariant that the text logically in the buffer is always followed by a zero byte, so that it's a valid C string. Unfortunately, in one of the places I made this change, I was storing binary data in the strbuf (so the terminating NUL is unimportant), and immediately after decreasing the strbuf's length, I was doing a memcmp one of whose arguments was the data I'd just chopped off the end of the strbuf. So it _mattered_ that no random NUL had been splurged over it. Specifically, this happened in the run-length encoder used to compress scrollback data, and had the effect that two components of the compressed scrollback could be spuriously considered equal, if one of them started with a legitimate zero byte and the other had a zero byte written over it by this bug. Thanks to Michael Weller for a nice test case that demonstrated a compressed scrollback line being decompressed again as the wrong thing: "NORMAL TEXT, \033[42mGREEN BACKGROUND\033[0m, NORMAL TEXT AGAIN" If the above line is printed to the terminal (after being decoded as if it was a C string literal), then only the words "GREEN BACKGROUND" get a green background. But after that line is scrolled off the top of the window, if you find it in the scrollback, then the rest of the line to the right has also become green-backgrounded due to this bug. --- terminal.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/terminal.c b/terminal.c index 0329df85..d025a448 100644 --- a/terminal.c +++ b/terminal.c @@ -461,9 +461,10 @@ static void makerle(strbuf *b, termline *ldata, oldstate = state; makeliteral(b, c, &state); tmplen = b->len - tmppos; + bool match = tmplen == thislen && + !memcmp(b->u + runpos+1, b->u + tmppos, tmplen); strbuf_shrink_to(b, tmppos); - if (tmplen != thislen || - memcmp(b->u + runpos+1, b->u + tmppos, tmplen)) { + if (!match) { state = oldstate; break; /* run over */ }