NetBSD Problem Report #40414

From sjamaan@frohike.homeunix.org  Fri Jan 16 14:36:35 2009
Return-Path: <sjamaan@frohike.homeunix.org>
Received: from mail.netbsd.org (mail.netbsd.org [204.152.190.11])
	by narn.NetBSD.org (Postfix) with ESMTP id B441063B8BA
	for <gnats-bugs@gnats.NetBSD.org>; Fri, 16 Jan 2009 14:36:35 +0000 (UTC)
Message-Id: <20090116143953.C8CB4F7E02E@frohike.homeunix.org>
Date: Fri, 16 Jan 2009 15:39:53 +0100 (CET)
From: Peter.Bex@xs4all.nl
Reply-To: Peter.Bex@xs4all.nl
To: gnats-bugs@gnats.NetBSD.org
Subject: nvi abort()s in autoindent/autoindent differs from historical vi
X-Send-Pr-Version: 3.95

>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:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Fri Jan 16 14:40:00 +0000 2009
>Closed-Date:    Fri Jan 16 15:06:33 +0000 2009
>Last-Modified:  Tue Jan 20 03:10:03 +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:/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

>Release-Note:

>Audit-Trail:

State-Changed-From-To: open->closed
State-Changed-By: christos@NetBSD.org
State-Changed-When: Fri, 16 Jan 2009 10:06:33 -0500
State-Changed-Why:
fix committed, thanks!


From: Christos Zoulas <christos@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/40414 CVS commit: src/dist/nvi/vi
Date: Fri, 16 Jan 2009 15:05:55 +0000 (UTC)

 Module Name:	src
 Committed By:	christos
 Date:		Fri Jan 16 15:05:55 UTC 2009

 Modified Files:
 	src/dist/nvi/vi: v_txt.c

 Log Message:
 PR/40414: Peter Bex: nvi abort()s in autoindent/autoindent differs from
 historical vi


 To generate a diff of this commit:
 cvs rdiff -r1.3 -r1.4 src/dist/nvi/vi/v_txt.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: Soren Jacobsen <snj@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/40414 CVS commit: [netbsd-5] src/dist/nvi/vi
Date: Tue, 20 Jan 2009 03:05:01 +0000 (UTC)

 Module Name:	src
 Committed By:	snj
 Date:		Tue Jan 20 03:05:01 UTC 2009

 Modified Files:
 	src/dist/nvi/vi [netbsd-5]: v_txt.c

 Log Message:
 Pull up following revision(s) (requested by lukem in ticket #292):
 	dist/nvi/vi/v_txt.c: revision 1.4
 PR/40414: Peter Bex: nvi abort()s in autoindent/autoindent differs from
 historical vi


 To generate a diff of this commit:
 cvs rdiff -r1.1.1.2.6.2 -r1.1.1.2.6.3 src/dist/nvi/vi/v_txt.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

>Unformatted:

Home	PR Database Search