Re: [PATCH 4/5] run-command: prepare child environment before forking

[Brandon Williams <bmwill@google.com>]

On 04/10, Jonathan Nieder wrote:
> Brandon Williams wrote:
> 
> > In order to avoid allocation between 'fork()' and 'exec()' prepare the
> > environment to be used in the child process prior to forking.
> 
> If using something like posix_spawn(), this would be needed anyway, so
> I'll review it.
> 
> [...]
> > +++ b/run-command.c
> [...]
> > +static char **prep_childenv(const char *const *deltaenv)
> > +{
> > +	char **childenv;
> > +	int childenv_nr = 0, childenv_alloc = 0;
> > +	int i;
> > +
> > +	for (i = 0; environ[i]; i++)
> > +		childenv_nr++;
> > +	for (i = 0; deltaenv && deltaenv[i]; i++)
> > +		childenv_alloc++;
> > +	/* Add one for the NULL termination */
> > +	childenv_alloc += childenv_nr + 1;
> > +
> > +	childenv = xcalloc(childenv_alloc, sizeof(char *));
> > +	memcpy(childenv, environ, childenv_nr * sizeof(char *));
> > +
> > +	/* merge in deltaenv */
> > +	for (i = 0; deltaenv && deltaenv[i]; i++)
> > +		childenv_nr = do_putenv(childenv, childenv_nr, deltaenv[i]);
> > +
> > +	return childenv;
> > +}
> 
> This potentially copies around most of 'environ' several times as it
> adjusts for each deltaenv item. Can it be simplified? E.g.

The only time it copies anything is that first memcpy which makes a
duplicate of the environ, and then when an item is being removed in
do_putenv it will move the back of the array to fill in for the entry
being removed.

> 
> 	struct argv_array result = ARGV_ARRAY_INIT;
> 	struct string_list mods = STRING_LIST_INIT_DUP;
> 	struct strbuf key = STRBUF_INIT;
> 	const char **p;
> 
> 	for (p = cmd_env; *p; p++) {
> 		const char *equals = strchr(*p, '=');
> 		if (equals) {
> 			strbuf_reset(&key);
> 			strbuf_add(&key, *p, equals - *p);
> 			string_list_append(&mods, key.buf)->util = *p;
> 		} else {
> 			string_list_append(&mods, *p);
> 		}
> 	}
> 	string_list_sort(&mods);
> 
> 	for (p = environ; *p; p++) {
> 		struct string_list_item *item;
> 		const char *equals = strchr(*p, '=');
> 		if (!equals)
> 			continue;
> 		strbuf_reset(&key);
> 		strbuf_add(&key, *p, equals - *p);
> 		item = string_list_lookup(&mods, key.buf);
> 
> 		if (!item) /* no change */
> 			argv_array_push(&result, *p);
> 		else if (!item->util) /* unsetenv */
> 			; /* skip */
> 		else /* setenv */
> 			argv_array_push(&result, item->util);
> 	}
> 
> 	strbuf_release(&key);
> 	string_list_clear(&mods);
> 	return argv_array_detach(&result);

This is probably still incomplete as I don't see how this accounts for
entries in 'cmd_env' which are being added to the environment and not
just replacing existing ones.

-- 
Brandon Williams