Re: Bug Report: Env Vars and shell functions

[Peter Stephenson <pws@ifh.de>]

schaefer@candle.brasslantern.com wrote:
> On Jul 8,  1:49pm, Peter Bray wrote:
> } Subject: Bug Report: Env Vars and shell functions
> }
> } 	Can others reproduce this bug in zsh-3.0-pre2, where a command
> } line environment variable is ignored in other functions called by the
> } original functions.
> 
> The local environment seems to get lost on the second and succeeding
> passes around the 'for' loop.
> 
> zagzig[52] bar() { echo $1 $BAR }
> zagzig[53] foo() { bar }
> zagzig[54] BAR=foo foo
> foo
> zagzig[55] foo() { for x in 1 2 3 ; do bar $x ; done }
> zagzig[56] foo
> 1
> 2
> 3
> zagzig[57] BAR=foo foo
> 1 foo
> 2
> 3

The culprit is the save_params()/restore_params() mechanism called
from execcmd() to handle temporary settings during builtins, which
uses static variables.  Only the original author of that can say why
it wasn't scoped within execcmd() and therefore if I'm missing
something with the following patch.  I honestly can't see any problem:
I've made sure everything gets restored, including the only abnormal
return.  However, that can hardly have been the problem being worked
around, since in fact the same abnormal return would before have
potentially left dreck hanging on removelist/restorelist.  I can't see
any new scope for leakage.

*** Src/exec.c.savpar	Mon Jul  8 09:29:41 1996
--- Src/exec.c	Mon Jul  8 14:37:10 1996
***************
*** 1561,1571 ****
  
  	    lastval = (func[type - CURSH]) (cmd);
  	} else if (is_builtin || is_shfunc) {
  	    /* builtin or shell function */
  
  	    if (!forked && !assign) {
  		PERMALLOC {
! 		    save_params(cmd);
  		} LASTALLOC;
  	    }
  	    
--- 1561,1572 ----
  
  	    lastval = (func[type - CURSH]) (cmd);
  	} else if (is_builtin || is_shfunc) {
+ 	    LinkList restorelist = 0, removelist = 0;
  	    /* builtin or shell function */
  
  	    if (!forked && !assign) {
  		PERMALLOC {
! 		    save_params(cmd, &restorelist, &removelist);
  		} LASTALLOC;
  	    }
  	    
***************
*** 1575,1580 ****
--- 1576,1582 ----
  		 */
  		addvars(cmd->vars, is_shfunc);
  		if (errflag) {
+ 		    restore_params(restorelist, removelist);
  		    lastval = 1;
  		    return;
  		}
***************
*** 1632,1639 ****
  		exit(lastval);
  	    }
  
! 	    if (!forked && !assign)
! 		restore_params();
  
  	} else {
  	    if (cmd->flags & CFLAG_EXEC) {
--- 1634,1640 ----
  		exit(lastval);
  	    }
  
! 	    restore_params(restorelist, removelist);
  
  	} else {
  	    if (cmd->flags & CFLAG_EXEC) {
***************
*** 1672,1693 ****
      fixfds(save);
  }
  
- /* Link list used to save parameters during *
-  * execution of shfunc or builtin.          */
- static LinkList restorelist;
- static LinkList removelist;
- 
- /* Setup the link lists use to save parameters *
-  * for shfuncs or builtins.                    */
- 
- /**/
- void
- init_save_params(void)
- {
-     restorelist = newlinklist();
-     removelist  = newlinklist();
- }
- 
  /* Arrange to have variables restored.                *
   * As yet special parameters won't work, nor will     *
   * parameter names that need substituting.            *
--- 1673,1678 ----
***************
*** 1695,1716 ****
  
  /**/
  void
! save_params(Cmd cmd)
  {
      Param pm;
      LinkNode node;
      char *s;
  
      for (node = firstnode(cmd->vars); node; incnode(node)) {
  	s = ((Varasg) getdata(node))->name;
  	if ((pm = (Param) paramtab->getnode(paramtab, s))) {
  	    if (!(pm->flags & PM_SPECIAL)) {
  		paramtab->removenode(paramtab, s);
! 		addlinknode(removelist, s);
! 		addlinknode(restorelist, pm);
  	    }
  	} else {
! 	    addlinknode(removelist, s);
  	}
      }
  }
--- 1680,1704 ----
  
  /**/
  void
! save_params(Cmd cmd, LinkList *restore_p, LinkList *remove_p)
  {
      Param pm;
      LinkNode node;
      char *s;
  
+     *restore_p = newlinklist();
+     *remove_p = newlinklist();
+ 
      for (node = firstnode(cmd->vars); node; incnode(node)) {
  	s = ((Varasg) getdata(node))->name;
  	if ((pm = (Param) paramtab->getnode(paramtab, s))) {
  	    if (!(pm->flags & PM_SPECIAL)) {
  		paramtab->removenode(paramtab, s);
! 		addlinknode(*remove_p, s);
! 		addlinknode(*restore_p, pm);
  	    }
  	} else {
! 	    addlinknode(*remove_p, s);
  	}
      }
  }
***************
*** 1719,1736 ****
  
  /**/
  void
! restore_params(void)
  {
      Param pm;
      char *s;
  
!     /* remove temporary parameters */
!     while ((s = (char *) getlinknode(removelist)))
! 	unsetparam(s);
! 
!     /* restore saved parameters */
!     while ((pm = (Param) getlinknode(restorelist)))
! 	paramtab->addnode(paramtab, pm->nam, pm);
  }
  
  /* restore fds after redirecting a builtin */
--- 1707,1730 ----
  
  /**/
  void
! restore_params(LinkList restorelist, LinkList removelist)
  {
      Param pm;
      char *s;
  
!     if (removelist) {
! 	/* remove temporary parameters */
! 	while ((s = (char *) getlinknode(removelist)))
! 	    unsetparam(s);
! 	freelinklist(removelist, 0);
!     }
! 
!     if (restorelist) {
! 	/* restore saved parameters */
! 	while ((pm = (Param) getlinknode(restorelist)))
! 	    paramtab->addnode(paramtab, pm->nam, pm);
! 	freelinklist(restorelist, 0);
!     }
  }
  
  /* restore fds after redirecting a builtin */
*** Src/init.c.savpar	Mon Jul  8 09:30:24 1996
--- Src/init.c	Mon Jul  8 14:30:47 1996
***************
*** 597,603 ****
  
      initkeybindings();	    /* initialize key bindings */
      compctlsetup();
-     init_save_params(); /* init saving params during shfunc or builtin */
  
  #ifdef HAVE_GETRLIMIT
      for (i = 0; i != RLIM_NLIMITS; i++)
--- 597,602 ----

-- 
Peter Stephenson <pws@ifh.de>       Tel: +49 33762 77366
WWW:  http://www.ifh.de/~pws/       Fax: +49 33762 77330
Deutches Electronen-Synchrotron --- Institut fuer Hochenergiephysik Zeuthen
DESY-IfH, 15735 Zeuthen, Germany.