bin/53919: please revert changes that make /bin/sh evaluate $ENV non-interactively

["Greg A. Woods" <woods%planix.ca@localhost>]

>Number:         53919
>Category:       bin
>Synopsis:       please revert changes that make /bin/sh evaluate $ENV non-interactively
>Confidential:   no
>Severity:       serious
>Priority:       high
>Responsible:    bin-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Mon Jan 28 20:45:00 +0000 2019
>Originator:     Greg A. Woods
>Release:        NetBSD 8.99.27
>Organization:
Planix, Inc.; Kelowna, BC; Canada
>Environment:
System: NetBSD future 8.99.27 NetBSD 8.99.27 (XEN3_DOMU) #1: Mon Jan 14 20:32:17 PST 2019 woods@future:/build/woods/future/current-amd64-amd64-obj/building/work/woods/m-NetBSD-current/sys/arch/amd64/compile/XEN3_DOMU amd64
Architecture: x86_64
Machine: amd64
>Description:

	Please revert the changes to /bin/sh which cause it to evaluate
	$ENV when not running in "interactive" mode.

	I.e. revert, or disable, the changes made for PR 42829

	The expansion of $ENV by non-interactive /bin/sh process makes
	the system effectively and entirely unusable by some of us users
	of /bin/ksh who have been happily using a certain feature of
	/bin/ksh and similar shells for over 30 years now.

	Way back when I was discussing this issue for PR 42828 I failed
	to fully appreciate the implications of this change.  I am now
	vehemently opposed to these changes, at least in their current
	form.

>How-To-Repeat:

	I finally got around to upgrading a server built with Jan 14
	sources and was surprised to find a plethora of error messages
	that appeared to be coming from /bin/sh, sometimes appearing in
	the most unwelcome and unexpected places.

	Observe:

		$ /usr/bin/true
		/usr/bin/true: 0: Syntax error: Bad substitution

	Not having paid enough attention to recent changes in NetBSD
	made such an error message extremely confusing and concerning.

	Debugging this was also a bit of an adventure with the new GDB
	as it seems to have sometimes become more difficult to use
	without a fully populated /usr/libdata/debug, even for
	static-linked binaries with a full .debug file present.  But
	that's an issue for another PR.  Suffice it to say that without
	use of the debugger I would likely have taken far far longer to
	figure out what was going on.


>Fix:

	I don't know what the best fix is.  As I said in PR 42828 I
	think the best solution would be _full_ compatability with the
	original source of this feature, namely the original Korn Shell.
	However as I discussed this would require implementation of
	arrays for /bin/sh, and that's probably not a short term fix.

	My sort-term hack, just to make my system usable again, is to
	change the name of the variable to SHENV.  However I doubt I
	will ever set $SHENV for my own use, because:

	The current implementation, i.e. no matter what the variable is
	named, is extremely unsatisfactory as is.

	The reason the current implementation is unacceptable is because
	it forces the shell to open the file referenced by $ENV and to
	read the entire file's contents (in most cases, as per the
	recommended hack for interactive-only use) and, if I'm not
	mistaken, parse all of what it reads as well.  What a horrible
	useless waste, and for _EVERY_ shell invocation!  That is not
	acceptable to me, and it should not be acceptable to anyone who
	has even the slightest concern about userland performance.

	However I still can't think of a better fix than to implement
	arrays in /bin/sh.

	I also add a small error message to try to help explain the
	mystery error message.  That needs to be done to /bin/ksh too.
	(The real ksh93 seems to suppress all errors expanding $ENV,
	which might also be acceptable, though it certainly adds yet
	more mystery to $ENV expansion failures.)

Index: main.c
===================================================================
RCS file: /cvs/master/m-NetBSD/main/src/bin/sh/main.c,v
retrieving revision 1.80
diff -u -r1.80 main.c
--- main.c	19 Jan 2019 14:20:22 -0000	1.80
+++ main.c	28 Jan 2019 19:46:13 -0000
@@ -213,9 +213,15 @@
 		struct stackmark env_smark;
 
 		setstackmark(&env_smark);
-		if ((shinit = lookupvar("ENV")) != NULL && *shinit != '\0') {
-			state = 3;
-			read_profile(expandenv(shinit));
+		if ((shinit = lookupvar("SHENV")) != NULL && *shinit != '\0') {
+			const char *shenvf = expandenv(shinit);
+
+			if (strcmp(shinit, shenvf) == 0) {
+				sh_warnx("failed to expand SHENV");
+			} else {
+				state = 3;
+				read_profile(shenvf);
+			}
 		}
 		popstackmark(&env_smark);
 	}
Index: sh.1
===================================================================
RCS file: /cvs/master/m-NetBSD/main/src/bin/sh/sh.1,v
retrieving revision 1.217
diff -u -r1.217 sh.1
--- sh.1	21 Jan 2019 14:09:24 -0000	1.217
+++ sh.1	28 Jan 2019 19:54:38 -0000
@@ -137,7 +137,7 @@
 .Pq \&$HOME ,
 if they exist.
 If the environment variable
-.Ev ENV
+.Ev SHENV
 is set on entry to a shell,
 or is set in the
 .Pa .profile
@@ -147,40 +147,40 @@
 option is not set,
 the shell then performs parameter and arithmetic
 expansion on the value of
-.Ev ENV ,
+.Ev SHENV ,
 (these are described later)
 and then reads commands from the file name that results.
 If
-.Ev ENV
+.Ev SHENV
 contains a command substitution, or one of the
 other expansions fails, or if there are no expansions
 to expand, the value of
-.Ev ENV
+.Ev SHENV
 is used as the file name.
 .Pp
 Therefore, a user should place commands that are to be executed only at
 login time in the
 .Pa .profile
 file, and commands that are executed for every shell inside the
-.Ev ENV
+.Ev SHENV
 file.
 To set the
-.Ev ENV
+.Ev SHENV
 variable to some file, place the following line in your
 .Pa .profile
 of your home directory
 .Pp
-.Dl ENV=$HOME/.shinit; export ENV
+.Dl SHENV=$HOME/.shinit; export SHENV
 .Pp
 substituting for
 .Dq .shinit
 any filename you wish.
 Since the
-.Ev ENV
+.Ev SHENV
 file can be read for every invocation of the shell, including shell scripts
-and non-interactive shells, the following paradigm is useful for
+and non-interactive shells, the following paradigm is useful, though ugly and not ideal, for
 restricting commands in the
-.Ev ENV
+.Ev SHENV
 file to interactive invocations.
 Place commands within the
 .Dq Ic case
@@ -410,7 +410,7 @@
 .Pa /etc/profile ,
 .Pa .profile ,
 and the file specified by the
-.Ev ENV
+.Ev SHENV
 environment variable.
 .It Fl s Em stdin
 Read commands from standard input (set automatically if
@@ -518,7 +518,7 @@
 option on the command line.
 Currently this option controls whether (!posix) or not (posix)
 the file given by the
-.Ev ENV
+.Ev SHENV
 variable is read at startup by a non-interactive shell.
 It also controls whether file descriptors greater than 2
 opened using the
@@ -4055,7 +4057,7 @@
 If unset
 .Dq $HOME/.editrc
 is used.
-.It Ev ENV
+.It Ev SHENV
 Names the file sourced at startup by the shell.
 Unused by this shell after initialization,
 but is usually passed through the environment to