Hi,
Hiltjo Posthuma wrote on Sun, Jun 18, 2017 at 03:04:31PM +0200:
> mg crashes with certain (unicode) characters and moving the cursor to the
> end of the line.
Even though i failed to reproduce the crash, even with MALLOC_OPTIONS=S,
i see how it may happen. Your analysis looks at the right functions,
but part of it is inaccurate.
> The characters are printed to the screen as \nnn in vtpute()
No, they are not, which is another bug that needs fixing.
> and vtcol is updated,
No, not sufficiently, which is exactly the problem.
> however vteeol() will write beyond the buffer.
More precisely, before the beginning of the buffer.
> A test file contains the data:
>
> ——————————————————
[ Regarding vteeol() ]
> It is safer to use vtpute() here because it checks if vtcol >= 0.
No, that is not needed. Apart from incrementing vtcol, the only
place where vtcol is changed is vtmove(). The only place where
vtmove() is called with a non-zero second argument is in updext().
In updext(), when calculating lbound, both numbers subtracted are
clearly non-negative, so we have lbound <= curcol. So after the
loop calling vtpute() repeatedly, vtcol >= 0 ought to be guaranteed.
To summarize, vtcol >= 0 is an invariant except for a very short
section of code inside updext(), and the check in vtpute() is only
needed for the call from that section. In vteeol(), no such check
is needed.
The problem is that vtpute() is not handling bytes correctly that
are neither printable nor control characters. It does not print
anything for them and it increments vtcol only by one. Consequently,
whenn there are many bytes with the high bit set, vtcol may remain
negative after finishing the loop in updext(), and vteeol() gets
fatally called with negative vtcol.
The patch below fixes the root cause by adding the same logic for
handling special characters to vtpute() that you can already see
in vtputc(). I tried to keep the patch minimal, and to also mimick
the existing style.
OK to commit that?
> Below is a patch:
Your patch would merely hide the bug, and the editor still wouldn't
operate correctly.
Yours,
Ingo
Index: display.c
===================================================================
RCS file: /cvs/src/usr.bin/mg/display.c,v
retrieving revision 1.47
diff -u -p -r1.47 display.c
--- display.c 3 Apr 2015 22:10:29 -0000 1.47
+++ display.c 6 Jul 2017 13:55:43 -0000
@@ -366,10 +366,16 @@ vtpute(int c)
} else if (ISCTRL(c) != FALSE) {
vtpute('^');
vtpute(CCHR(c));
- } else {
+ } else if (isprint(c)) {
if (vtcol >= 0)
vp->v_text[vtcol] = c;
++vtcol;
+ } else {
+ char bf[5], *cp;
+
+ snprintf(bf, sizeof(bf), "\\%o", c);
+ for (cp = bf; *cp != '\0'; cp++)
+ vtpute(*cp);
}
}
