Hi Ingo,
thanks for your detailed analysis, makes sense to me OK florian@
Apologies to Hiltjo for slacking on this. I looked at it multiple
times but couldn't make any progress on it.
Florian
On Thu, Jul 06, 2017 at 04:08:09PM +0200, Ingo Schwarze wrote:
> 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);
> }
> }
>
--
I'm not entirely sure you are real.
