Re: Environment handling broken in /bin/sh with changes to t,set,put}env()

On Sat, 14 Jul 2007, Andrey Chernov wrote:

> On Fri, Jul 13, 2007 at 07:08:37PM -0500, Sean C. Farley wrote:
>> different.  Are you saying that the addresses should not change for
>> environ, environ[0-1] or all?
>
> I try to say that *env() functions should avoid to change memory they
> not "own" when it is possible and feel free to change the memory they
> "own".
>
> What standard says is that initial copying all environ to the internal
> structure and changing it afterwards (exluding putenv case) is the
> right thing (because *env() functions "owns" that memory).
>
> To avoid even things like
>
> nenv[0] = "PATH=/bin";
> setenv("PATH", "/bin", 1);
>
> fails, better way is to strdup() (i.e. copying) every string from the
> original environ and you already do that initially in build_env() but
> not do the safe way in previous merge_environ().

OK.  I thought you were saying that setenv(name, value, 0) and
unsetenv(name) must not, as opposed to should not, alter environ if the
name was found and not found respectively.  It would require a careful
change, but it would be more efficient in some cases.  I would prefer to
keep the code simple.  At least as much as I would call it simple.  :)

On the bright side, keeping the current behavior of rebuilding after a
non-getenv() call is safe since the program cannot assume environ is the
same after such a call.  For example, unsetenv(somename) will return a
zero regardless if the name existed or not, therefore the program cannot
assume that environ is still at the same address.

> BTW, I find that code in build_env() very suspicious:
>
>       if (environ[0] == NULL)
>                goto SaveEnviron;
> ...
> SaveEnviron:
>        origEnviron = environ;
>        environ = NULL;
>        if (envVarsTotal > 0) {
> ...
>       } else
>                rtrnVal = 0;
>
>        return (rtrnVal);
>
> It ends up with environ = NULL; because envVarsTotal initialized to 0
> i.e.  makes from "environ[0] == NULL" case "environ == NULL" case
> which is different thing.

I changed it to return (0) if either environ or environ[0] equal NULL.
The SaveEnviron label is removed as well as the (envVarsTotal > 0)
check.

Sean
-- 
scf_at_FreeBSD.org