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

On Thu, 5 Jul 2007, Andrey Chernov wrote:

> On Wed, Jul 04, 2007 at 09:53:15PM -0500, Sean C. Farley wrote:
>> The latest patch at the same URL fixes that issue.  It basically
>> deactivates all existing variables and inserts the new environ
>> variables into the envVars array.
>
> Calling __clean_env(false) is good but the rest looks like a bit
> overkill.

It has the advantage that if environ was replace with an empty array or
a list of variables that already were defined (limited copy of
environment), then it would not need to make any allocations (i.e.,
usr.sbin/zic/zdump.c).  Also, cleaning up almost everything and starting
from the beginning would actually be trickier to get it right if using
__build_env().  It would require adding more intelligence (complexity)
to __build_env() to know how to add a new environ to an existing envVars
array.  Right now, it builds it from scratch.

> Previously the goal of veryfy_env() is just deactivate, the goal of
> build_env() is just build. It was build_env() who insetrts new environ
> variables into envVars array in old variant, isn't?

Yes, it was.  Now, it is to merge in a new environ array.  I renamed it
__merge_environ() to better reflect its new role.

> Now verify_env() takes the role of build_env() too, moreover, may
> cause setenv() to be called recursively which isn't good.

Do you see a problem that I am missing, or are you suggesting a change
to prevent potential problems from future changes?

The test at issue is this:
if (__merge_environ() == -1 || (envVars == NULL && __build_env() == -1))

1. environ was changed
2. program calls setenv()[1]
3. setenv()[1] calls __merge_environ()[1]
4. __merge_environ()[1] calls setenv()[2]
5. setenv()[2] calls __merge_environ()[2]
6. __merge_environ()[2] returns 0 since environ == watchEnviron
7. setenv()[2] adds new name/value pair and returns
8. Jump to step 4 until environ is inserted.
9. __merge_environ()[1] returns
10. setenv()[1] returns
11. program is happy  :)

The alternative, which I had actually considered, is to split setenv()
into __setenv() which is almost the entire current setenv() and a new
setenv() that is just a wrapper around __setenv() with the beginning
checks.  This seems a bit of a waste, but I may be mistaken.

Sean
-- 
scf_at_FreeBSD.org