> > > +	environ = p;
> > > +	environ[cnt + 1] = NULL;
> > 
> > These two would be safer the other way around (just in case someone
> > is reading environ without the lock held.
> 
> Doesn't just switching the order solve one problem and create
> another?  Actually, wouldn't it need to be more like this?
> 
> p[cnt + 1] = NULL;
> environ = p;

That is what I meant :-)

> > 
> > AT line 95 the code has:
> > 
> > 	if (strlen(c) >= l_value) {     /* old larger; copy over */
> > 		while ((*c++ = *value++) != '\0');
> > 		rwlock_unlock(&__environ_lock);
> > 		return (0);
> > 	} 
> > 
> > This isn't safe anymore....
> 
> Isn't it?  I guess I'm missing something then as I don't see it.
> Probably not wise to be setenv()ing a variable previously putenv()ed
> but it doesn't appear any more unsafe than it was...

Ah, but before the memory you we overwriting had always been allocated
by the setenv code - it didn't belong to a user.

> > I'm also not 100% about how the code behaves in the presence of
> > environment string that don't contain an '=' (eg after putenv("fred");).
> > 
> > It needs to do something sensible.
> 
> It should probably return -1 like the current putenv() function.
> I don't see anything in the standard that prevents this.  I'll
> check what other implementations do.

That doesn't help, you cannot rely on the memory area not being overwritten
by the application after putenv has been called.

Something 'sensible' is all that is required...
FWIW in setenv, I'd do p = malloc(strlen(name) + strlen(value) + 2)
without worrying about allocatin an extra byte if name ends or value
starts with an '='.

To free the memory allocated by setenv, I'd suggest keeping another
list of the memory blocks allocated by setenv.  Whenever something is
freed, check the address matches.  The same index will work...
That will bound the memory use even if parts of the application
explicitly overwrite entries in the environ vector.

	David

-- 
David Laight: david@l8s.co.uk