bin/40414: nvi abort()s in autoindent/autoindent differs from historical vi

[Peter.Bex%xs4all.nl@localhost]

>Number:         40414
>Category:       bin
>Synopsis:       nvi abort()s in autoindent/autoindent differs from historical 
>vi
>Confidential:   no
>Severity:       non-critical
>Priority:       low
>Responsible:    bin-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Fri Jan 16 14:40:00 +0000 2009
>Originator:     Peter Bex
>Release:        NetBSD 4.0.0_PATCH
>Organization:
>Environment:
        
        
System: NetBSD frohike.homeunix.org 4.0.0_PATCH NetBSD 4.0.0_PATCH (GENERIC) 
#0: Fri Dec 21 17:12:08 CET 2007 
sjamaan%frohike.homeunix.org@localhost:/usr/obj/sys/arch/amd64/compile/GENERIC 
amd64
Architecture: x86_64
Machine: amd64
>Description:
        nvi has a broken edge case in autoindent mode; it abort()s when
        the user (accidentally) presses ^D directly after using the ^^D
        sequence to reset back to the first column, and then advancing
        to any column beyond the first.

        The abort() seems to be a "safety measure" to punt when things
        are not right; that code is never supposed to be reached in
        regular operation; see vi/v_txt.c:980

        The case that triggers this abort is when the 'carat' variable
        has the value C_NOCHANGE, as set by the code under the
        C_CARATSET case of the same switch statement; see line 961.
        After receiving this value, carat will only be changed in the
        cases K_NL (line 795), K_CARAT (line 930) and K_ZERO (line 934),
        as can be easily verified. Otherwise, when ^D is entered,
        K_CNTRLD is triggered again, causing the case to be entered
        once again, but now with C_NOCHANGE, which is not expected.

        But, this only happens when the cursor is *not* on the first
        column, since the 'if' check for tp->cno on line 948 shortcuts
        the operation.

>How-To-Repeat:
        Open vi, ":set autoindent", then do the following:

        1) Press ^I to indent the current line
        2) Press ^M; the next line should now be aligned below the first
        3) Press ^^D (caret sign followed by <control-D>) to reset back
            to the start of the line (with reset on the following line)
        4) Type at least one character (but not '^', '0', '^D' or
            newline) so you're not on the first column
        5) Press ^D

        vi now aborts, dumping core.
        
>Fix:
        Fixing the problem is not hard; I found two ways to solve it.
        The first was to add a check for C_NOCHANGE to line 946, this
        would make it ignore the ^D. The second would be to add a
        FALLTHROUGH case for C_NOCHANGE just above the C_NOTSET case
        in the switch that starts at line 949, which would make the ^D
        insert a literal ^D character. I decided to have a look at the
        current version of the historic vi (available from
        ex-vi.sourceforge.net, this is the historic vi that nvi tries to
        stay compatible to)

        In that version of vi, 0^D, ^D and ^^D only work from the
        "current indent level".  ^D and ^T are the *only* two
        keystrokes that change the (internal) indent level. After a
        newline, the indent level is recalculated, unless ^^D was used.
        This leads to the unintuitive behaviour that you can press 0^D
        and then enter whitespace characters (tab or space) until
        you reach the original indent point again, you can then
        _again_ press any of these three indentation keystrokes,
        and the indentation level will be changed again. This is
        different from how nvi behaves.

        The attached patch makes nvi behave the same way.
        At the very least it fixes the abort() issue!  This patch was
        also sent upstream and accepted there.

        The patch removes the "ai" level reset that 0^D would normally
        cause. Instead of doing the AI changes there, it pushes more of
        the AI settings into the txt_dent function by removing the
        bookkeeping of whether to reset the AI count there. I have a
        feeling this may break stuff (since that code must be there for
        a reason), but after doing my best in testing it, I can't seem
        to make it fail. It looks like that code does not have effect
        because the indent level is always recalculated after newline.
        The changes to the indent level checks (from "greater than" to
        "not equal") change ^D so it does not work when you type it in
        a column _before_ the current indent level. So you can use ^^D,
        and when you press tab you cannot press ^D, unless you ended up
        exactly on the "current indent level" column (from where you
        pressed ^^D).


Index: vi/v_txt.c
===================================================================
RCS file: /cvsroot/src/dist/nvi/vi/v_txt.c,v
retrieving revision 1.1.1.2
diff -u -r1.1.1.2 v_txt.c
--- vi/v_txt.c  18 May 2008 14:31:47 -0000      1.1.1.2
+++ vi/v_txt.c  24 Dec 2008 23:10:49 -0000
@@ -950,7 +950,7 @@
 
                switch (carat) {
                case C_CARATSET:        /* ^^D */
-                       if (tp->ai == 0 || tp->cno > tp->ai + tp->offset + 1)
+                       if (tp->ai == 0 || tp->cno != tp->ai + tp->offset + 1)
                                goto ins_ch;
 
                        /* Save the ai string for later. */
@@ -963,17 +963,18 @@
                        carat = C_NOCHANGE;
                        goto leftmargin;
                case C_ZEROSET:         /* 0^D */
-                       if (tp->ai == 0 || tp->cno > tp->ai + tp->offset + 1)
+                       if (tp->ai == 0 || tp->cno != tp->ai + tp->offset + 1)
                                goto ins_ch;
 
                        carat = C_NOTSET;
 leftmargin:            tp->lb[tp->cno - 1] = ' ';
                        tp->owrite += tp->cno - tp->offset;
-                       tp->ai = 0;
                        tp->cno = tp->offset;
                        break;
+               case C_NOCHANGE:        /* ^D after reset with ^^D */
+                       /* FALLTHROUGH */
                case C_NOTSET:          /* ^D */
-                       if (tp->ai == 0 || tp->cno > tp->ai + tp->offset)
+                       if (tp->ai == 0 || tp->cno != tp->ai + tp->offset)
                                goto ins_ch;
 
                        (void)txt_dent(sp, tp, 0);
@@ -1891,7 +1892,6 @@
        CHAR_T ch;
        u_long sw, ts;
        size_t cno, current, spaces, target, tabs, off;
-       int ai_reset;
 
        ts = O_VAL(sp, O_TABSTOP);
        sw = O_VAL(sp, O_SHIFTWIDTH);
@@ -1923,16 +1923,6 @@
                target -= --target % sw;
 
        /*
-        * The AI characters will be turned into overwrite characters if the
-        * cursor immediately follows them.  We test both the cursor position
-        * and the indent flag because there's no single test.  (^T can only
-        * be detected by the cursor position, and while we know that the test
-        * is always true for ^D, the cursor can be in more than one place, as
-        * "0^D" and "^D" are different.)
-        */
-       ai_reset = !isindent || tp->cno == tp->ai + tp->offset;
-
-       /*
         * Back up over any previous <blank> characters, changing them into
         * overwrite characters (including any ai characters).  Then figure
         * out the current screen column.
@@ -1965,9 +1955,7 @@
                spaces = target - cno;
        }
 
-       /* If we overwrote ai characters, reset the ai count. */
-       if (ai_reset)
-               tp->ai = tabs + spaces;
+       tp->ai = tabs + spaces;
 
        /*
         * Call txt_insch() to insert each character, so that we get the

>Unformatted: