eval: Variable assignments on functions are no longer persistent

Commit Message

Herbert Xu April 4, 2018, 9:54 a.m. UTC

Dirk Fieldhouse <[e-mail]> wrote:
>
> In POSIX.1-2017 ("simultaneously IEEE Std 1003.1™-2017 and The Open
> Group Technical Standard Base Specifications, Issue 7")
> <http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09>,
> we read under '2.9.1 Simple Commands'
> 
> "Variable assignments shall be performed as follows:
> ...
> -    If the command name is a standard utility implemented as a function
> (see XBD Utility), the effect of variable assignments shall be as if the
> utility was not implemented as a function.
> ...
> -    If the command name is a function that is not a standard utility
> implemented as a function, variable assignments shall affect the current
> execution environment during the execution of the function. It is
> unspecified:
> 
>     *   Whether or not the variable assignments persist after the
> completion of the function
> 
>     *   Whether or not the variables gain the export attribute during
> the execution of the function
> 
>     *   Whether or not export attributes gained as a result of the
> variable assignments persist after the completion of the function (if
> variable assignments persist after the completion of the function)"

POSIX used to require the current dash behaviour.  However, you're
right that this is no longer the case.

This patch will remove the persistence of the variable assignment.

I have considered the exporting the variables during the function
execution but have decided against it because:

1) It makes the code bigger.
2) dash has never done this in the past.
3) You cannot use this portably anyway.

Reported-by: Dirk Fieldhouse <[e-mail]>
Signed-off-by: Herbert Xu <[e-mail]>

Comments

Dirk Fieldhouse April 5, 2018, 10:27 p.m. UTC | #1

On Wed, 04 Apr 2018 02:54:22 -0700, Herbert Xu
<[e-mail]> wrote:
> 
> Dirk Fieldhouse <[e-mail]> wrote:
>> [Variable assignment with function invocation persists contrary to manual]
> 
> POSIX used to require the current dash behaviour.  However, you're
> right that this is no longer the case.

Well, that's just luck: I hadn't studied the evolution of the wording.

> This patch will remove the persistence of the variable assignment.

Thanks for the patch: will it somehow propagate to Busybox, or should I
report the issue separately there?

> I have considered the exporting the variables during the function
> execution but have decided against it because:
> 
> 1) It makes the code bigger.
> 2) dash has never done this in the past.
> 3) You cannot use this portably anyway.
>...

Understandable, but I'm still not sure how this meets the spec regarding
'standard utilities' implemented as shell functions (is that also new?).

Presumably the spec has in mind that some OS might have (say, as before)
basename as a function and the OS provider might bundle a companion
POSIX shell, which is then required to treat that basename function as
if it were an external command.

However if a separate shell such as DASH is later installed on this
system, that shell would not know which functions are the special
'standard utilities': for instance, when executing a function
'basename', how would it distinguish between a basename provided by the
OS and one defined in a user-provided shell script? Surely not by
looking at the file ownership - yuk. Does the spec in fact mean that if
I as a user define a basename function meeting the POSIX utilities spec
in my script, a POSIX-conformant shell must execute  it as if it were an
external command?

So this provision of the spec together with the simplicity for the shell
script programmer of being able to switch transparently between shell
functions and external scripts or binary executables seems to argue for
'exporting'.

On the other hand, obvs it's your program, and many thanks.

regards
/df

Herbert Xu April 8, 2018, 3:21 a.m. UTC | #2

Dirk Fieldhouse <[e-mail]> wrote:
>
> Understandable, but I'm still not sure how this meets the spec regarding
> 'standard utilities' implemented as shell functions (is that also new?).

Some shells implement standard utilities as shell functions.  dash
does not do this so it's not relevant.

Cheers,

Patch

diff --git a/src/eval.c b/src/eval.c
index 7498f9d..623166d 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -883,7 +883,6 @@  raise:
 		goto readstatus;
 
 	case CMDFUNCTION:
-		poplocalvars(1);
 		if (evalfun(cmdentry.u.func, argc, argv, flags))
 			goto raise;
 readstatus:
@@ -967,9 +966,7 @@  evalfun(struct funcnode *func, int argc, char **argv, int flags)
 	shellparam.p = argv + 1;
 	shellparam.optind = 1;
 	shellparam.optoff = -1;
-	pushlocalvars();
 	evaltree(func->n.ndefun.body, flags & EV_TESTED);
-	poplocalvars(0);
 funcdone:
 	INTOFF;
 	loopnest = saveloopnest;