Re: Assignment with command: export only, or sets variable, too?

Re: Assignment with command: export only, or sets variable, too?

[Herbert Xu]

On Fri, Apr 09, 2010 at 09:26:02AM +0100, Geoff Clare wrote:
> Herbert Xu <herbert@gondor.hengli.com.au> wrote, on 09 Apr 2010:
> >
> > While we're on this topic, what is the correct output for
> > 
> > 	X=a Y=${X=b} Z=$X sh -c 'echo $Z'
> 
> My reading of the standard is that this should output "b"
> (assuming X is unset beforehand).  But, as I mentioned earlier in
> the thread, this is a strict reading and might not be what was
> intended.

Thank you for your clarifications.

These four patches should help bring dash in-line with the POSIX
requirements.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[PATCH 1/4] [VAR] Add localvars nesting

[Herbert Xu]

[VAR] Add localvars nesting

This patch adds localvars nesting infrastructure so we can reuse
the localvars mechanism for command evaluation.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 ChangeLog  |    4 ++++
 src/eval.c |    7 ++-----
 src/var.c  |   50 ++++++++++++++++++++++++++++++++++++++++++++------
 src/var.h  |    1 +
 4 files changed, 51 insertions(+), 11 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index ee78154..7285a23 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+2010-05-24  Herbert Xu <herbert@gondor.apana.org.au>
+
+	* Add localvars nesting.
+
 2010-05-03  Gerrit Pape <pape@smarden.org>
 
 	* Fix command -- crash.
diff --git a/src/eval.c b/src/eval.c
index 62d9d5d..8d2767c 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -928,20 +928,17 @@ STATIC int
 evalfun(struct funcnode *func, int argc, char **argv, int flags)
 {
 	volatile struct shparam saveparam;
-	struct localvar *volatile savelocalvars;
 	struct jmploc *volatile savehandler;
 	struct jmploc jmploc;
 	int e;
 
 	saveparam = shellparam;
-	savelocalvars = localvars;
 	if ((e = setjmp(jmploc.loc))) {
 		goto funcdone;
 	}
 	INTOFF;
 	savehandler = handler;
 	handler = &jmploc;
-	localvars = NULL;
 	shellparam.malloc = 0;
 	func->count++;
 	funcnest++;
@@ -950,13 +947,13 @@ evalfun(struct funcnode *func, int argc, char **argv, int flags)
 	shellparam.p = argv + 1;
 	shellparam.optind = 1;
 	shellparam.optoff = -1;
+	pushlocalvars();
 	evaltree(&func->n, flags & EV_TESTED);
+	poplocalvars();
 funcdone:
 	INTOFF;
 	funcnest--;
 	freefunc(func);
-	poplocalvars();
-	localvars = savelocalvars;
 	freeparam(&shellparam);
 	shellparam = saveparam;
 	handler = savehandler;
diff --git a/src/var.c b/src/var.c
index 2737fb1..de1a5f5 100644
--- a/src/var.c
+++ b/src/var.c
@@ -64,7 +64,12 @@
 #define VTABSIZE 39
 
 
-struct localvar *localvars;
+struct localvar_list {
+	struct localvar_list *next;
+	struct localvar *lv;
+};
+
+MKINIT struct localvar_list *localvar_stack;
 
 const char defpathvar[] =
 	"PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin";
@@ -139,6 +144,11 @@ INIT {
 			p = 0;
 	setpwd(p, 0);
 }
+
+RESET {
+	while (localvar_stack)
+		poplocalvars();
+}
 #endif
 
 
@@ -446,6 +456,9 @@ localcmd(int argc, char **argv)
 {
 	char *name;
 
+	if (!localvar_stack)
+		sh_error("not in a function");
+
 	argv = argptr;
 	while ((name = *argv++) != NULL) {
 		mklocal(name);
@@ -497,8 +510,8 @@ mklocal(char *name)
 		}
 	}
 	lvp->vp = vp;
-	lvp->next = localvars;
-	localvars = lvp;
+	lvp->next = localvar_stack->lv;
+	localvar_stack->lv = lvp;
 	INTON;
 }
 
@@ -511,11 +524,19 @@ mklocal(char *name)
 void
 poplocalvars(void)
 {
-	struct localvar *lvp;
+	struct localvar_list *ll;
+	struct localvar *lvp, *next;
 	struct var *vp;
 
-	while ((lvp = localvars) != NULL) {
-		localvars = lvp->next;
+	INTOFF;
+	ll = localvar_stack;
+	localvar_stack = ll->next;
+
+	next = ll->lv;
+	ckfree(ll);
+
+	while ((lvp = next) != NULL) {
+		next = lvp->next;
 		vp = lvp->vp;
 		TRACE(("poplocalvar %s", vp ? vp->text : "-"));
 		if (vp == NULL) {	/* $- saved */
@@ -534,6 +555,23 @@ poplocalvars(void)
 		}
 		ckfree(lvp);
 	}
+	INTON;
+}
+
+
+/*
+ * Create a new localvar environment.
+ */
+void pushlocalvars(void)
+{
+	struct localvar_list *ll;
+
+	INTOFF;
+	ll = ckmalloc(sizeof(*ll));
+	ll->lv = NULL;
+	ll->next = localvar_stack;
+	localvar_stack = ll;
+	INTON;
 }
 
 
diff --git a/src/var.h b/src/var.h
index e4e2cff..32b0dde 100644
--- a/src/var.h
+++ b/src/var.h
@@ -139,6 +139,7 @@ char **listvars(int, int, char ***);
 int showvars(const char *, int, int);
 int exportcmd(int, char **);
 int localcmd(int, char **);
+void pushlocalvars(void);
 void poplocalvars(void);
 int unsetcmd(int, char **);
 int unsetvar(const char *);

Re: [PATCH 1/4] [VAR] Add localvars nesting

[Jilles Tjoelker]

On Wed, May 26, 2010 at 09:00:31PM +1000, Herbert Xu wrote:
> [VAR] Add localvars nesting

> This patch adds localvars nesting infrastructure so we can reuse
> the localvars mechanism for command evaluation.

localvars could already nest, it's just that this change makes the
nesting explicit instead of on the C stack.

Further comments inline.

> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> ---

>  ChangeLog  |    4 ++++
>  src/eval.c |    7 ++-----
>  src/var.c  |   50 ++++++++++++++++++++++++++++++++++++++++++++------
>  src/var.h  |    1 +
>  4 files changed, 51 insertions(+), 11 deletions(-)

> diff --git a/ChangeLog b/ChangeLog
> index ee78154..7285a23 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,7 @@
> +2010-05-24  Herbert Xu <herbert@gondor.apana.org.au>
> +
> +	* Add localvars nesting.
> +
>  2010-05-03  Gerrit Pape <pape@smarden.org>
>  
>  	* Fix command -- crash.
> diff --git a/src/eval.c b/src/eval.c
> index 62d9d5d..8d2767c 100644
> --- a/src/eval.c
> +++ b/src/eval.c
> @@ -928,20 +928,17 @@ STATIC int
>  evalfun(struct funcnode *func, int argc, char **argv, int flags)
>  {
>  	volatile struct shparam saveparam;
> -	struct localvar *volatile savelocalvars;
>  	struct jmploc *volatile savehandler;
>  	struct jmploc jmploc;
>  	int e;
>  
>  	saveparam = shellparam;
> -	savelocalvars = localvars;
>  	if ((e = setjmp(jmploc.loc))) {
>  		goto funcdone;
>  	}
>  	INTOFF;
>  	savehandler = handler;
>  	handler = &jmploc;
> -	localvars = NULL;
>  	shellparam.malloc = 0;
>  	func->count++;
>  	funcnest++;
> @@ -950,13 +947,13 @@ evalfun(struct funcnode *func, int argc, char **argv, int flags)
>  	shellparam.p = argv + 1;
>  	shellparam.optind = 1;
>  	shellparam.optoff = -1;
> +	pushlocalvars();
>  	evaltree(&func->n, flags & EV_TESTED);
> +	poplocalvars();
>  funcdone:
>  	INTOFF;
>  	funcnest--;
>  	freefunc(func);
> -	poplocalvars();
> -	localvars = savelocalvars;
>  	freeparam(&shellparam);
>  	shellparam = saveparam;
>  	handler = savehandler;

This change does not do poplocalvars() if the function was exited with
an error. While this is normally not a problem because the shell exits
or executes the RESET actions, 'command eval', 'command .' and 'fc' will
"catch" any errors and continue normally.

Example (with the below "disallow local outside a function" change):
  dash -c 'f() { unset xyz; ${xyz?}; }; command eval f; local i'

> diff --git a/src/var.c b/src/var.c
> index 2737fb1..de1a5f5 100644
> [...]
> @@ -446,6 +456,9 @@ localcmd(int argc, char **argv)
>  {
>  	char *name;
>  
> +	if (!localvar_stack)
> +		sh_error("not in a function");
> +
>  	argv = argptr;
>  	while ((name = *argv++) != NULL) {
>  		mklocal(name);

This change (failing local outside functions), while a good idea, should
be mentioned in the commit message/changelog, as it might break certain
scripts.

(Note that in mksh local is an alias for typeset, which will work
outside functions.)

-- 
Jilles Tjoelker

Re: [PATCH 1/4] [VAR] Add localvars nesting

[Herbert Xu]

On Wed, May 26, 2010 at 11:55:33PM +0200, Jilles Tjoelker wrote:
>
> > @@ -950,13 +947,13 @@ evalfun(struct funcnode *func, int argc, char **argv, int flags)
> >  	shellparam.p = argv + 1;
> >  	shellparam.optind = 1;
> >  	shellparam.optoff = -1;
> > +	pushlocalvars();
> >  	evaltree(&func->n, flags & EV_TESTED);
> > +	poplocalvars();
> >  funcdone:
> >  	INTOFF;
> >  	funcnest--;
> >  	freefunc(func);
> > -	poplocalvars();
> > -	localvars = savelocalvars;
> >  	freeparam(&shellparam);
> >  	shellparam = saveparam;
> >  	handler = savehandler;
> 
> This change does not do poplocalvars() if the function was exited with
> an error. While this is normally not a problem because the shell exits
> or executes the RESET actions, 'command eval', 'command .' and 'fc' will
> "catch" any errors and continue normally.
> 
> Example (with the below "disallow local outside a function" change):
>   dash -c 'f() { unset xyz; ${xyz?}; }; command eval f; local i'

Thanks for catching this! The following patch should fix that
problem.  BTW, the same issue has existed for redirections for
quite some time.  I will fix this in a subsequent patch.

> This change (failing local outside functions), while a good idea, should
> be mentioned in the commit message/changelog, as it might break certain
> scripts.

I will modify the Changelog to highlight this change.

commit 127788364951212c356aadc39deb21e01b0161c8
Author: Herbert Xu <herbert@gondor.apana.org.au>
Date:   Thu May 27 11:32:55 2010 +0800

    [VAR] Fix poplocalvar on abnormal exit from function
    
    The new localvar code broke the abnormal exit from functions
    and built-ins by not restoring the original localvar state.
    
    This patch fixes this by storing the previous localvar state so
    that we always unwind correctly in case of an abnormal exit.
    
    Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/ChangeLog b/ChangeLog
index bf1f13c..c9b5e75 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+2010-05-27  Herbert Xu <herbert@gondor.apana.org.au>
+
+	* Fix poplocalvar on abnormal exit from function.
+
 2010-05-26  Herbert Xu <herbert@gondor.apana.org.au>
 
 	* Replace cmdenviron with localvars.
diff --git a/src/eval.c b/src/eval.c
index a6981a9..2cd931b 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -681,6 +681,7 @@ evalcommand(union node *cmd, int flags, struct backcmd *backcmd)
 evalcommand(union node *cmd, int flags)
 #endif
 {
+	struct localvar_list *localvar_stop;
 	struct stackmark smark;
 	union node *argp;
 	struct arglist arglist;
@@ -703,7 +704,7 @@ evalcommand(union node *cmd, int flags)
 	/* First expand the arguments. */
 	TRACE(("evalcommand(0x%lx, %d) called\n", (long)cmd, flags));
 	setstackmark(&smark);
-	pushlocalvars();
+	localvar_stop = pushlocalvars();
 	back_exitstatus = 0;
 
 	cmdentry.cmdtype = CMDBUILTIN;
@@ -837,7 +838,6 @@ bail:
 			if (forkshell(jp, cmd, FORK_FG) != 0) {
 				exitstatus = waitforjob(jp);
 				INTON;
-				poplocalvars(0);
 				break;
 			}
 			FORCEINTON;
@@ -878,6 +878,7 @@ raise:
 
 out:
 	popredir(execcmd);
+	unwindlocalvars(localvar_stop);
 	if (lastarg)
 		/* dsl: I think this is intended to be used to support
 		 * '_' in 'vi' command mode during line editing...
diff --git a/src/var.c b/src/var.c
index 40bd3fd..f456fbd 100644
--- a/src/var.c
+++ b/src/var.c
@@ -144,8 +144,7 @@ INIT {
 }
 
 RESET {
-	while (localvar_stack)
-		poplocalvars(0);
+	unwindlocalvars(0);
 }
 #endif
 
@@ -570,7 +569,7 @@ poplocalvars(int keep)
 /*
  * Create a new localvar environment.
  */
-void pushlocalvars(void)
+struct localvar_list *pushlocalvars(void)
 {
 	struct localvar_list *ll;
 
@@ -580,6 +579,15 @@ void pushlocalvars(void)
 	ll->next = localvar_stack;
 	localvar_stack = ll;
 	INTON;
+
+	return ll->next;
+}
+
+
+void unwindlocalvars(struct localvar_list *stop)
+{
+	while (localvar_stack != stop)
+		poplocalvars(0);
 }
 
 
diff --git a/src/var.h b/src/var.h
index ef6d583..7e7e505 100644
--- a/src/var.h
+++ b/src/var.h
@@ -69,6 +69,8 @@ struct localvar {
 	const char *text;		/* saved text */
 };
 
+struct localvar_list;
+
 
 extern struct localvar *localvars;
 extern struct var varinit[];
@@ -139,8 +141,9 @@ int showvars(const char *, int, int);
 int exportcmd(int, char **);
 int localcmd(int, char **);
 void mklocal(char *);
-void pushlocalvars(void);
+struct localvar_list *pushlocalvars(void);
 void poplocalvars(int);
+void unwindlocalvars(struct localvar_list *stop);
 int unsetcmd(int, char **);
 void unsetvar(const char *);
 int varcmp(const char *, const char *);

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[PATCH 1/2] [REDIR] Move null redirect checks into caller

[Herbert Xu]

[REDIR] Move null redirect checks into caller

The null redirect checks were added as an optimisation to avoid
unnecessary memory allocations.  However, we could avoid this
completely by simply making the caller avoid making a redirection
unless it is not null.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 ChangeLog   |    1 +
 src/eval.c  |    6 ++++--
 src/redir.c |   14 +-------------
 3 files changed, 6 insertions(+), 15 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 4fc35a6..1fd184b 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -2,6 +2,7 @@
 
 	* Fix poplocalvar on abnormal exit from function.
 	* Do not poplocalvars prematurely on regular utilities.
+	* Move null redirect checks into caller.
 
 2010-05-26  Herbert Xu <herbert@gondor.apana.org.au>
 
diff --git a/src/eval.c b/src/eval.c
index 337667f..59bded9 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -224,7 +224,8 @@ evaltree(union node *n, int flags)
 			evaltree(n->nredir.n, flags & EV_TESTED);
 			status = exitstatus;
 		}
-		popredir(0);
+		if (n->nredir.redirect)
+			popredir(0);
 		goto setstatus;
 	case NCMD:
 #ifdef notyet
@@ -879,7 +880,8 @@ raise:
 	}
 
 out:
-	popredir(execcmd);
+	if (cmd->ncmd.redirect)
+		popredir(execcmd);
 	unwindlocalvars(localvar_stop);
 	if (lastarg)
 		/* dsl: I think this is intended to be used to support
diff --git a/src/redir.c b/src/redir.c
index 54af96b..16decfc 100644
--- a/src/redir.c
+++ b/src/redir.c
@@ -72,12 +72,10 @@ MKINIT
 struct redirtab {
 	struct redirtab *next;
 	int renamed[10];
-	int nullredirs;
 };
 
 
 MKINIT struct redirtab *redirlist;
-MKINIT int nullredirs;
 
 STATIC int openredirect(union node *);
 #ifdef notyet
@@ -113,7 +111,6 @@ redirect(union node *redir, int flags)
 		memory[i] = 0;
 	memory[1] = flags & REDIR_BACKQ;
 #endif
-	nullredirs++;
 	if (!redir) {
 		return;
 	}
@@ -124,10 +121,8 @@ redirect(union node *redir, int flags)
 		q = ckmalloc(sizeof (struct redirtab));
 		q->next = redirlist;
 		redirlist = q;
-		q->nullredirs = nullredirs - 1;
 		for (i = 0 ; i < 10 ; i++)
 			q->renamed[i] = EMPTY;
-		nullredirs = 0;
 		sv = q;
 	}
 	n = redir;
@@ -343,8 +338,6 @@ popredir(int drop)
 	struct redirtab *rp;
 	int i;
 
-	if (--nullredirs >= 0)
-		return;
 	INTOFF;
 	rp = redirlist;
 	for (i = 0 ; i < 10 ; i++) {
@@ -364,7 +357,6 @@ popredir(int drop)
 		}
 	}
 	redirlist = rp->next;
-	nullredirs = rp->nullredirs;
 	ckfree(rp);
 	INTON;
 }
@@ -381,12 +373,8 @@ RESET {
 	/*
 	 * Discard all saved file descriptors.
 	 */
-	for (;;) {
-		nullredirs = 0;
-		if (!redirlist)
-			break;
+	while (redirlist)
 		popredir(0);
-	}
 }
 
 #endif

[PATCH 2/2] [REDIR] Fix popredir on abnormal exit from built-in

[Herbert Xu]

[REDIR] Fix popredir on abnormal exit from built-in

Just like the poplocalvar problem recently fixed, redirections
can also be leaked in case of an abnormal exit.  This patch fixes
it using the same method as poplocalvar, by storing the previous
redirection state and restoring to that point.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 ChangeLog   |    1 +
 src/eval.c  |    4 ++++
 src/redir.c |   45 ++++++++++++++++++++++++++++++++-------------
 src/redir.h |    3 +++
 4 files changed, 40 insertions(+), 13 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 1fd184b..650899a 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -3,6 +3,7 @@
 	* Fix poplocalvar on abnormal exit from function.
 	* Do not poplocalvars prematurely on regular utilities.
 	* Move null redirect checks into caller.
+	* Fix popredir on abnormal exit from built-in.
 
 2010-05-26  Herbert Xu <herbert@gondor.apana.org.au>
 
diff --git a/src/eval.c b/src/eval.c
index 59bded9..d5c1e6c 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -219,6 +219,7 @@ evaltree(union node *n, int flags)
 		goto setstatus;
 	case NREDIR:
 		expredir(n->nredir.redirect);
+		pushredir(n->nredir.redirect);
 		status = redirectsafe(n->nredir.redirect, REDIR_PUSH);
 		if (!status) {
 			evaltree(n->nredir.n, flags & EV_TESTED);
@@ -683,6 +684,7 @@ evalcommand(union node *cmd, int flags)
 #endif
 {
 	struct localvar_list *localvar_stop;
+	struct redirtab *redir_stop;
 	struct stackmark smark;
 	union node *argp;
 	struct arglist arglist;
@@ -740,6 +742,7 @@ evalcommand(union node *cmd, int flags)
 
 	preverrout.fd = 2;
 	expredir(cmd->ncmd.redirect);
+	redir_stop = pushredir(cmd->ncmd.redirect);
 	status = redirectsafe(cmd->ncmd.redirect, REDIR_PUSH|REDIR_SAVEFD2);
 
 	path = vpath.text;
@@ -882,6 +885,7 @@ raise:
 out:
 	if (cmd->ncmd.redirect)
 		popredir(execcmd);
+	unwindredir(redir_stop);
 	unwindlocalvars(localvar_stop);
 	if (lastarg)
 		/* dsl: I think this is intended to be used to support
diff --git a/src/redir.c b/src/redir.c
index 16decfc..b4e49c0 100644
--- a/src/redir.c
+++ b/src/redir.c
@@ -111,20 +111,12 @@ redirect(union node *redir, int flags)
 		memory[i] = 0;
 	memory[1] = flags & REDIR_BACKQ;
 #endif
-	if (!redir) {
+	if (!redir)
 		return;
-	}
 	sv = NULL;
 	INTOFF;
-	if (likely(flags & REDIR_PUSH)) {
-		struct redirtab *q;
-		q = ckmalloc(sizeof (struct redirtab));
-		q->next = redirlist;
-		redirlist = q;
-		for (i = 0 ; i < 10 ; i++)
-			q->renamed[i] = EMPTY;
-		sv = q;
-	}
+	if (likely(flags & REDIR_PUSH))
+		sv = redirlist;
 	n = redir;
 	do {
 		newfd = openredirect(n);
@@ -373,8 +365,7 @@ RESET {
 	/*
 	 * Discard all saved file descriptors.
 	 */
-	while (redirlist)
-		popredir(0);
+	unwindredir(0);
 }
 
 #endif
@@ -485,3 +476,31 @@ redirectsafe(union node *redir, int flags)
 	RESTOREINT(saveint);
 	return err;
 }
+
+
+void unwindredir(struct redirtab *stop)
+{
+	while (redirlist != stop)
+		popredir(0);
+}
+
+
+struct redirtab *pushredir(union node *redir)
+{
+	struct redirtab *sv;
+	struct redirtab *q;
+	int i;
+
+	q = redirlist;
+	if (!redir)
+		goto out;
+
+	sv = ckmalloc(sizeof (struct redirtab));
+	sv->next = q;
+	redirlist = sv;
+	for (i = 0; i < 10; i++)
+		sv->renamed[i] = EMPTY;
+
+out:
+	return q;
+}
diff --git a/src/redir.h b/src/redir.h
index d1d160e..8e56995 100644
--- a/src/redir.h
+++ b/src/redir.h
@@ -41,10 +41,13 @@
 #endif
 #define REDIR_SAVEFD2 03	/* set preverrout */
 
+struct redirtab;
 union node;
 void redirect(union node *, int);
 void popredir(int);
 void clearredir(void);
 int savefd(int, int);
 int redirectsafe(union node *, int);
+void unwindredir(struct redirtab *stop);
+struct redirtab *pushredir(union node *redir);
 

[PATCH 2/4] [VAR] Fix poplocalvar leak

[Herbert Xu]

[VAR] Fix poplocalvar leak

When a variable is marked as local, we set VSTRFIXED on its vp
recored.  However, poplocalvar never clears this flag for variables
that were unset to begin with.  Thus if you ever made an unset
variable local, it would get the VSTRFIXED bit and stick around
forever.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 ChangeLog |    4 ++++
 src/var.c |    8 +++-----
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 7285a23..f3c7701 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+2010-05-25  Herbert Xu <herbert@gondor.apana.org.au>
+
+	* Fix poplocalvar leak.
+
 2010-05-24  Herbert Xu <herbert@gondor.apana.org.au>
 
 	* Add localvars nesting.
diff --git a/src/var.c b/src/var.c
index de1a5f5..fc6d367 100644
--- a/src/var.c
+++ b/src/var.c
@@ -543,7 +543,8 @@ poplocalvars(void)
 			memcpy(optlist, lvp->text, sizeof(optlist));
 			ckfree(lvp->text);
 			optschanged();
-		} else if ((lvp->flags & (VUNSET|VSTRFIXED)) == VUNSET) {
+		} else if (lvp->flags == VUNSET) {
+			vp->flags &= ~(VSTRFIXED|VREADONLY);
 			unsetvar(vp->text);
 		} else {
 			if (vp->func)
@@ -627,8 +628,6 @@ unsetvar(const char *s)
 		retval = 1;
 		if (flags & VREADONLY)
 			goto out;
-		if (flags & VUNSET)
-			goto ok;
 		if ((flags & VSTRFIXED) == 0) {
 			INTOFF;
 			if ((flags & (VTEXTFIXED|VSTACK)) == 0)
@@ -636,11 +635,10 @@ unsetvar(const char *s)
 			*vpp = vp->next;
 			ckfree(vp);
 			INTON;
-		} else {
+		} else if (!(flags & VUNSET)) {
 			setvar(s, 0, 0);
 			vp->flags &= ~VEXPORT;
 		}
-ok:
 		retval = 0;
 	}
 

[PATCH 3/4] [VAR] Move unsetvar functionality into setvareq

[Herbert Xu]

[VAR] Move unsetvar functionality into setvareq

This patch moves the unsetvar code into setvareq so that we can
no have a pathological case of an unset variable hanging around
unless it has a bit pinning it like VEXPORT.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 ChangeLog |    1 +
 src/var.c |   54 +++++++++++++++++-------------------------------------
 src/var.h |    2 +-
 3 files changed, 19 insertions(+), 38 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index f3c7701..1f7120f 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,6 +1,7 @@
 2010-05-25  Herbert Xu <herbert@gondor.apana.org.au>
 
 	* Fix poplocalvar leak.
+	* Move unsetvar functionality into setvareq.
 
 2010-05-24  Herbert Xu <herbert@gondor.apana.org.au>
 
diff --git a/src/var.c b/src/var.c
index fc6d367..12f2f6c 100644
--- a/src/var.c
+++ b/src/var.c
@@ -266,10 +266,22 @@ setvareq(char *s, int flags)
 		if ((vp->flags & (VTEXTFIXED|VSTACK)) == 0)
 			ckfree(vp->text);
 
+		if (((flags & (VEXPORT|VREADONLY|VSTRFIXED|VUNSET)) |
+		     (vp->flags & VSTRFIXED)) == VUNSET) {
+			*vpp = vp->next;
+			ckfree(vp);
+out_free:
+			if ((flags & (VTEXTFIXED|VSTACK|VNOSAVE)) == VNOSAVE)
+				ckfree(s);
+			return;
+		}
+
 		flags |= vp->flags & ~(VTEXTFIXED|VSTACK|VNOSAVE|VUNSET);
 	} else {
 		if (flags & VNOSET)
 			return;
+		if ((flags & (VEXPORT|VREADONLY|VSTRFIXED|VUNSET)) == VUNSET)
+			goto out_free;
 		/* not found */
 		vp = ckmalloc(sizeof (*vp));
 		vp->next = *vpp;
@@ -588,7 +600,6 @@ unsetcmd(int argc, char **argv)
 	char **ap;
 	int i;
 	int flag = 0;
-	int ret = 0;
 
 	while ((i = nextopt("vf")) != '\0') {
 		flag = i;
@@ -596,15 +607,13 @@ unsetcmd(int argc, char **argv)
 
 	for (ap = argptr; *ap ; ap++) {
 		if (flag != 'f') {
-			i = unsetvar(*ap);
-			ret |= i;
-			if (!(i & 2))
-				continue;
+			unsetvar(*ap);
+			continue;
 		}
 		if (flag != 'v')
 			unsetfunc(*ap);
 	}
-	return ret & 1;
+	return 0;
 }
 
 
@@ -612,38 +621,9 @@ unsetcmd(int argc, char **argv)
  * Unset the specified variable.
  */
 
-int
-unsetvar(const char *s)
+void unsetvar(const char *s)
 {
-	struct var **vpp;
-	struct var *vp;
-	int retval;
-
-	vpp = findvar(hashvar(s), s);
-	vp = *vpp;
-	retval = 2;
-	if (vp) {
-		int flags = vp->flags;
-
-		retval = 1;
-		if (flags & VREADONLY)
-			goto out;
-		if ((flags & VSTRFIXED) == 0) {
-			INTOFF;
-			if ((flags & (VTEXTFIXED|VSTACK)) == 0)
-				ckfree(vp->text);
-			*vpp = vp->next;
-			ckfree(vp);
-			INTON;
-		} else if (!(flags & VUNSET)) {
-			setvar(s, 0, 0);
-			vp->flags &= ~VEXPORT;
-		}
-		retval = 0;
-	}

[PATCH 4/4] [VAR] Replace cmdenviron with localvars

[Herbert Xu]

[VAR] Replace cmdenviron with localvars

This patch replaces the cmdenviron mechanism for temporary command
variables with the localvars mechanism used by functions.

This reduces code size, and more importantly, makes the variable
assignment take effect immediately as required by POSIX.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 ChangeLog  |    4 ++++
 src/eval.c |   23 +++++++++--------------
 src/eval.h |    1 -
 src/var.c  |   45 ++++++++++++++++++++-------------------------
 src/var.h  |   15 +++++++++++++--
 5 files changed, 46 insertions(+), 42 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 1f7120f..bf1f13c 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+2010-05-26  Herbert Xu <herbert@gondor.apana.org.au>
+
+	* Replace cmdenviron with localvars.
+
 2010-05-25  Herbert Xu <herbert@gondor.apana.org.au>
 
 	* Fix poplocalvar leak.
diff --git a/src/eval.c b/src/eval.c
index 8d2767c..a6981a9 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -77,7 +77,6 @@ static int funcnest;		/* depth of function calls */
 
 
 char *commandname;
-struct strlist *cmdenviron;
 int exitstatus;			/* exit status of last command */
 int back_exitstatus;		/* exit status of backquoted command */
 
@@ -704,6 +703,7 @@ evalcommand(union node *cmd, int flags)
 	/* First expand the arguments. */
 	TRACE(("evalcommand(0x%lx, %d) called\n", (long)cmd, flags));
 	setstackmark(&smark);
+	pushlocalvars();
 	back_exitstatus = 0;
 
 	cmdentry.cmdtype = CMDBUILTIN;
@@ -748,6 +748,8 @@ evalcommand(union node *cmd, int flags)
 		spp = varlist.lastp;
 		expandarg(argp, &varlist, EXP_VARTILDE);
 
+		mklocal((*spp)->text);
+
 		/*
 		 * Modify the command lookup path, if a PATH= assignment
 		 * is present
@@ -835,6 +837,7 @@ bail:
 			if (forkshell(jp, cmd, FORK_FG) != 0) {
 				exitstatus = waitforjob(jp);
 				INTON;
+				poplocalvars(0);
 				break;
 			}
 			FORCEINTON;
@@ -844,17 +847,9 @@ bail:
 		/* NOTREACHED */
 
 	case CMDBUILTIN:
-		cmdenviron = varlist.list;
-		if (cmdenviron) {
-			struct strlist *list = cmdenviron;
-			int i = VNOSET;
-			if (spclbltin > 0 || argc == 0) {
-				i = 0;
-				if (execcmd && argc > 1)
-					i = VEXPORT;
-			}
-			listsetvar(list, i);
-		}
+		poplocalvars(spclbltin > 0 || argc == 0);
+		if (execcmd && argc > 1)
+			listsetvar(varlist.list, VEXPORT);
 		if (evalbltin(cmdentry.u.cmd, argc, argv, flags)) {
 			int status;
 			int i;
@@ -875,7 +870,7 @@ raise:
 		break;
 
 	case CMDFUNCTION:
-		listsetvar(varlist.list, 0);
+		poplocalvars(1);
 		if (evalfun(cmdentry.u.func, argc, argv, flags))
 			goto raise;
 		break;
@@ -949,7 +944,7 @@ evalfun(struct funcnode *func, int argc, char **argv, int flags)
 	shellparam.optoff = -1;
 	pushlocalvars();
 	evaltree(&func->n, flags & EV_TESTED);
-	poplocalvars();
+	poplocalvars(0);
 funcdone:
 	INTOFF;
 	funcnest--;
diff --git a/src/eval.h b/src/eval.h
index e190b28..ac394e8 100644
--- a/src/eval.h
+++ b/src/eval.h
@@ -37,7 +37,6 @@
 extern char *commandname;	/* currently executing command */
 extern int exitstatus;		/* exit status of last command */
 extern int back_exitstatus;	/* exit status of backquoted command */
-extern struct strlist *cmdenviron;  /* environment for builtin command */
 
 
 struct backcmd {		/* result of evalbackcmd */
diff --git a/src/var.c b/src/var.c
index 12f2f6c..40bd3fd 100644
--- a/src/var.c
+++ b/src/var.c
@@ -44,7 +44,6 @@
 #include "output.h"
 #include "expand.h"
 #include "nodes.h"	/* for other headers */
-#include "eval.h"	/* defines cmdenviron */
 #include "exec.h"
 #include "syntax.h"
 #include "options.h"
@@ -104,7 +103,6 @@ struct var varinit[] = {
 
 STATIC struct var *vartab[VTABSIZE];
 
-STATIC void mklocal(char *);
 STATIC struct var **hashvar(const char *);
 STATIC int vpcmp(const void *, const void *);
 STATIC struct var **findvar(struct var **, const char *);
@@ -147,7 +145,7 @@ INIT {
 
 RESET {
 	while (localvar_stack)
-		poplocalvars();
+		poplocalvars(0);
 }
 #endif
 
@@ -339,24 +337,6 @@ intmax_t lookupvarint(const char *name)
 
 
 /*
- * Search the environment of a builtin command.
- */
-
-char *
-bltinlookup(const char *name)
-{
-	struct strlist *sp;
-
-	for (sp = cmdenviron ; sp ; sp = sp->next) {
-		if (varequal(sp->text, name))
-			return strchrnul(sp->text, '=') + 1;
-	}
-	return lookupvar(name);
-}
-
-
-
-/*
  * Generate a list of variables satisfying the given conditions.
  */
 
@@ -486,8 +466,7 @@ localcmd(int argc, char **argv)
  * "-" as a special case.
  */
 
-STATIC void
-mklocal(char *name)
+void mklocal(char *name)
 {
 	struct localvar *lvp;
 	struct var **vpp;
@@ -534,7 +513,7 @@ mklocal(char *name)
  */
 
 void
-poplocalvars(void)
+poplocalvars(int keep)
 {
 	struct localvar_list *ll;
 	struct localvar *lvp, *next;
@@ -551,7 +530,23 @@ poplocalvars(void)
 		next = lvp->next;
 		vp = lvp->vp;
 		TRACE(("poplocalvar %s", vp ? vp->text : "-"));
-		if (vp == NULL) {	/* $- saved */
+		if (keep) {
+			int bits = VSTRFIXED;
+
+			if (lvp->flags != VUNSET) {
+				if (vp->text == lvp->text)
+					bits |= VTEXTFIXED;
+				else if (!(lvp->flags & (VTEXTFIXED|VSTACK)))
+					ckfree(lvp->text);
+			}
+
+			vp->flags &= ~bits;
+			vp->flags |= (lvp->flags & bits);
+
+			if ((vp->flags &
+			     (VEXPORT|VREADONLY|VSTRFIXED|VUNSET)) == VUNSET)
+				unsetvar(vp->text);
+		} else if (vp == NULL) {	/* $- saved */
 			memcpy(optlist, lvp->text, sizeof(optlist));
 			ckfree(lvp->text);
 			optschanged();
diff --git a/src/var.h b/src/var.h
index 2bb82b1..ef6d583 100644
--- a/src/var.h
+++ b/src/var.h
@@ -133,14 +133,14 @@ struct strlist;
 void listsetvar(struct strlist *, int);
 char *lookupvar(const char *);
 intmax_t lookupvarint(const char *);
-char *bltinlookup(const char *);
 char **listvars(int, int, char ***);
 #define environment() listvars(VEXPORT, VUNSET, 0)
 int showvars(const char *, int, int);
 int exportcmd(int, char **);
 int localcmd(int, char **);
+void mklocal(char *);
 void pushlocalvars(void);
-void poplocalvars(void);
+void poplocalvars(int);
 int unsetcmd(int, char **);
 void unsetvar(const char *);
 int varcmp(const char *, const char *);
@@ -148,3 +148,14 @@ int varcmp(const char *, const char *);
 static inline int varequal(const char *a, const char *b) {
 	return !varcmp(a, b);
 }
+
+/*
+ * Search the environment of a builtin command.
+ */
+
+static inline char *bltinlookup(const char *name)
+{
+	return lookupvar(name);
+}
+
+

Re: [PATCH 4/4] [VAR] Replace cmdenviron with localvars

[Jilles Tjoelker]

On Wed, May 26, 2010 at 09:00:34PM +1000, Herbert Xu wrote:
> [VAR] Replace cmdenviron with localvars

> This patch replaces the cmdenviron mechanism for temporary command
> variables with the localvars mechanism used by functions.

> This reduces code size, and more importantly, makes the variable
> assignment take effect immediately as required by POSIX.

This change breaks passing variable assignments to regular builtins.
For example,
  dash -c 'HOME=/ cd; pwd'
This should print /. (For some reason, this does not work in ksh93
either.)
Or
  dash -c 'cd /bin&&CDPATH=/ cd bin; pwd'
This should print /bin twice and not fail any command. (Again this fails
in ksh93.)

A more common example,
  IFS=: read -r x y z
is already broken in master before this change (due to git commit
55c46b7286f5d9f2d8291158203e2b61d2494420 which effectively replaced
bltinlookup("IFS") with ifsval()).

Similar issues may pop up with
  PATH=/foo command -V bar
  PATH=/foo type bar
  LC_NUMERIC=de_DE.UTF-8 printf '%f\n' 1,2

(Note: the latter is broken in bash, but in ksh93 and zsh it works, as
well as with an external printf(1) program.)

'local' may need additional magic to avoid making things local to the
temporary regular-builtin scope. I suppose only functions should induce
scope for 'local', such that things like command eval 'local i' continue
to create a variable local to the function.

-- 
Jilles Tjoelker

Re: [PATCH 4/4] [VAR] Replace cmdenviron with localvars

[Herbert Xu]

On Thu, May 27, 2010 at 12:37:23AM +0200, Jilles Tjoelker wrote:
> 
> This change breaks passing variable assignments to regular builtins.
> For example,
>   dash -c 'HOME=/ cd; pwd'
> This should print /. (For some reason, this does not work in ksh93
> either.)

Thanks, I have fixed it as follows.

commit 1d806ac1fbafb867f6252e184e1be05c0829ab71
Author: Herbert Xu <herbert@gondor.apana.org.au>
Date:   Thu May 27 11:50:19 2010 +0800

    [VAR] Do not poplocalvars prematurely on regular utilities
    
    The recent cmdenviron removal broke regular utilities by calling
    poplocalvars too early.  This patch fixes that by postponing the
    poplocalvars for regular utilities until they have completed.
    
    In order to ensure that local still works, it is now a special
    built-in.
    
    Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/ChangeLog b/ChangeLog
index eb12538..4fc35a6 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,6 +1,7 @@
 2010-05-27  Herbert Xu <herbert@gondor.apana.org.au>
 
 	* Fix poplocalvar on abnormal exit from function.
+	* Do not poplocalvars prematurely on regular utilities.
 
 2010-05-26  Herbert Xu <herbert@gondor.apana.org.au>
 
diff --git a/src/builtins.def.in b/src/builtins.def.in
index 266d0ec..4441fe4 100644
--- a/src/builtins.def.in
+++ b/src/builtins.def.in
@@ -71,7 +71,7 @@ falsecmd	-u false
 getoptscmd	-u getopts
 hashcmd		hash
 jobscmd		-u jobs
-localcmd	-a local
+localcmd	-as local
 printfcmd	printf
 pwdcmd		pwd
 readcmd		-u read
diff --git a/src/eval.c b/src/eval.c
index 2cd931b..337667f 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -847,9 +847,11 @@ bail:
 		/* NOTREACHED */
 
 	case CMDBUILTIN:
-		poplocalvars(spclbltin > 0 || argc == 0);
-		if (execcmd && argc > 1)
-			listsetvar(varlist.list, VEXPORT);
+		if (spclbltin > 0 || argc == 0) {
+			poplocalvars(1);
+			if (execcmd && argc > 1)
+				listsetvar(varlist.list, VEXPORT);
+		}
 		if (evalbltin(cmdentry.u.cmd, argc, argv, flags)) {
 			int status;
 			int i;

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 4/4] [VAR] Replace cmdenviron with localvars

[Jilles Tjoelker]

On Thu, May 27, 2010 at 01:51:41PM +1000, Herbert Xu wrote:
>     In order to ensure that local still works, it is now a special
>     built-in.

Although there are reasons to do this (a 'local' function containing
'command local' would not work, and export, readonly and ksh93's typeset
are special as well), consensus seems to be for local to be a regular
builtin. posh's local was special for a short time, but this was then
reverted.

Nevertheless, it is very nice to have IFS=: read ... working again.

-- 
Jilles Tjoelker