[PATCH] Fix ENV handling in sh/ksh emulation

[PATCH] Fix ENV handling in sh/ksh emulation

[Teubel György]

At sh/ksh emulation the value of the ENV parameter is stored
before sourcing ~/.profile.  Hence setting the ENV parameter
in ~/.profile has no effect.

Additionally the ENV is sourced regardless of the interactive state of
the shell.  POSIX standard says that the ENV should be sourced
when and only when an interactive shell is invoked.
---
 Doc/Zsh/params.yo |  3 ++-
 Src/init.c        | 25 ++++++++++++++-----------
 2 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/Doc/Zsh/params.yo b/Doc/Zsh/params.yo
index 55930ed..03625ce 100644
--- a/Doc/Zsh/params.yo
+++ b/Doc/Zsh/params.yo
@@ -1059,7 +1059,8 @@ If the tt(ENV) environment variable is set when zsh is invoked as tt(sh)
 or tt(ksh), tt($ENV) is sourced after the profile scripts.  The value of
 tt(ENV) is subjected to parameter expansion, command substitution, and
 arithmetic expansion before being interpreted as a pathname.  Note that
-tt(ENV) is em(not) used unless zsh is emulating bf(sh) or bf(ksh).
+tt(ENV) is em(not) used unless the shell is interactive and zsh is
+emulating bf(sh) or bf(ksh).
 )
 vindex(FCEDIT)
 item(tt(FCEDIT))(
diff --git a/Src/init.c b/Src/init.c
index 20a07eb..3dea179 100644
--- a/Src/init.c
+++ b/Src/init.c
@@ -712,7 +712,7 @@ init_term(void)
     if (tgetent(termbuf, term) != TGETENT_SUCCESS)
 #endif
     {
-	if (isset(INTERACTIVE))
+	if (interact)
 	    zerr("can't find terminal definition for %s", term);
 	errflag &= ~ERRFLAG_ERROR;
 	termflags |= TERM_BAD;
@@ -1205,19 +1205,22 @@ run_init_scripts(void)
 	if (islogin)
 	    source("/etc/profile");
 	if (unset(PRIVILEGED)) {
-	    char *s = getsparam("ENV");
 	    if (islogin)
 		sourcehome(".profile");
-	    noerrs = 2;
-	    if (s) {
-		s = dupstring(s);
-		if (!parsestr(&s)) {
-		    singsub(&s);
-		    noerrs = 0;
-		    source(s);
+
+	    if (interact) {
+		noerrs = 2;
+		char *s = getsparam("ENV");
+		if (s) {
+		    s = dupstring(s);
+		    if (!parsestr(&s)) {
+			singsub(&s);
+			noerrs = 0;
+			source(s);
+		    }
 		}
+		noerrs = 0;
 	    }
-	    noerrs = 0;
 	} else
 	    source("/etc/suid_profile");
     } else {
@@ -1227,7 +1230,7 @@ run_init_scripts(void)
 
 	if (isset(RCS) && unset(PRIVILEGED))
 	{
-	    if (isset(INTERACTIVE)) {
+	    if (interact) {
 		/*
 		 * Always attempt to load the newuser module to perform
 		 * checks for new zsh users.  Don't care if we can't load it.
-- 
2.9.3

Re: [PATCH] Fix ENV handling in sh/ksh emulation

[Bart Schaefer]

Aside that this patch also included two other hunks changing
isset(INTERACTIVE) to the equivalent "interact" macro instead.

This makes them all the same in init.c, except for one use of
unset(INTERACTIVE) instead of !interact.  However, other files
don't seem to be consistent about making use of this macro,
nor even about whether unset() vs. !isset() is chosen.

I'm not suggesting we go to any special effort to clean this up,
but I'd rather not sneak this kind of thing into unrelated diffs
(at least, not without mentioning it) because, along with things
like whitespace clean-up, it makes it difficult to tell from a
delta what changes are semantically meaningful.

Re: [PATCH] Fix ENV handling in sh/ksh emulation

[Teubel György]

On Tue, Sep 06, 2016 at 10:57:36PM -0700, Bart Schaefer wrote:
> Aside that this patch also included two other hunks changing
> isset(INTERACTIVE) to the equivalent "interact" macro instead.

Sorry for forgetting to mention it :(                                                                                                                 
I thought that it would be somewhat related to the patch.

> 
> This makes them all the same in init.c, except for one use of
> unset(INTERACTIVE) instead of !interact.  However, other files
> don't seem to be consistent about making use of this macro,
> nor even about whether unset() vs. !isset() is chosen.
> 
> I'm not suggesting we go to any special effort to clean this up,
> but I'd rather not sneak this kind of thing into unrelated diffs
> (at least, not without mentioning it) because, along with things
> like whitespace clean-up, it makes it difficult to tell from a
> delta what changes are semantically meaningful.