Re: off by one in vi(1)

[Todd C. Miller]

On Sun, 12 May 2013 20:57:50 +0300, Arto Jonsson wrote:

 Thanks for the review. Here's an updated diff. If anyone's curious this
 bug is about one month short of being 20 years old.

Committed, thanks.

 - todd

Re: off by one in vi(1)

[Arto Jonsson]

On Sat, May 11, 2013 at 05:33:20PM -0600, Todd C. Miller wrote:
 Good catch, I know folks who have hit this bug but I was never able
 to reproduce it.  Moving the isblank() check should be safe since
 trailing blanks are trimmed earlier on so we won't exit the loop
 prematurely.  I see you didn't change the TXT_ALTWERASE case, though.
 It looks like that also needs a fix but the break there will break
 out of the switch statement, not a loop.  However, since trailing
 blanks have already been trimmed I think that check is effectively
 a no-op and could simply be removed.

Thanks for the review. Here's an updated diff. If anyone's curious this
bug is about one month short of being 20 years old.

Index: vi/v_txt.c
===
RCS file: /cvs/src/usr.bin/vi/vi/v_txt.c,v
retrieving revision 1.22
diff -u -p -r1.22 v_txt.c
--- vi/v_txt.c  27 Oct 2009 23:59:48 -  1.22
+++ vi/v_txt.c  12 May 2013 17:24:25 -
@@ -1120,12 +1120,12 @@ leftmargin: tp-lb[tp-cno - 1] = ' ';
 */
if (LF_ISSET(TXT_TTYWERASE))
while (tp-cno  max) {
+   if (isblank(tp-lb[tp-cno - 1]))
+   break;
--tp-cno;
++tp-owrite;
if (FL_ISSET(is_flags, IS_RUNNING))
tp-lb[tp-cno] = ' ';
-   if (isblank(tp-lb[tp-cno - 1]))
-   break;
}
else {
if (LF_ISSET(TXT_ALTWERASE)) {
@@ -1133,19 +1133,17 @@ leftmargin: tp-lb[tp-cno - 1] = ' ';
++tp-owrite;
if (FL_ISSET(is_flags, IS_RUNNING))
tp-lb[tp-cno] = ' ';
-   if (isblank(tp-lb[tp-cno - 1]))
-   break;
}
if (tp-cno  max)
tmp = inword(tp-lb[tp-cno - 1]);
while (tp-cno  max) {
+   if (tmp != inword(tp-lb[tp-cno - 1])
+   || isblank(tp-lb[tp-cno - 1]))
+   break;
--tp-cno;
++tp-owrite;
if (FL_ISSET(is_flags, IS_RUNNING))
tp-lb[tp-cno] = ' ';
-   if (tmp != inword(tp-lb[tp-cno - 1])
-   || isblank(tp-lb[tp-cno - 1]))
-   break;
}
}
 

Re: off by one in vi(1)

[Todd C. Miller]

On Tue, 07 May 2013 19:10:44 +0300, Arto Jonsson wrote:

 While writing an email vi(1) crashed with segmentation fault.
 
 When ^W (WERASE) is hit in insert mode it's possible that the line
 buffer is accessed out of bounds. If 'max' == 0 and 'tp-cno' == 1 the
 'tp-cno' value is first reduced by one and then 'tp-lb' is accessed at
 'tp-cno' - 1. 

Good catch, I know folks who have hit this bug but I was never able
to reproduce it.  Moving the isblank() check should be safe since
trailing blanks are trimmed earlier on so we won't exit the loop
prematurely.  I see you didn't change the TXT_ALTWERASE case, though.
It looks like that also needs a fix but the break there will break
out of the switch statement, not a loop.  However, since trailing
blanks have already been trimmed I think that check is effectively
a no-op and could simply be removed.

 - todd