Re: [PATCH v2 4/4] use wrapper for unchecked setenv/putenv calls

On Wed, Dec 14, 2011 at 03:07:11PM +0100, Erik Faye-Lund wrote:

This avoids us from accidentally dropping state, possibly leading
to unexpected behaviour.

I do think this is fine in a "be extra cautious" kind of way.

This is especially important on Windows, where the maximum size of
the environment is 32 kB.

But does your patch actually detect that? As Andreas pointed out, these
limits don't typically come into play at setenv time. Instead, the
environment is allocated on the heap, and then the result is passed to
exec/spawn, which will fail.

So your patch is really detecting a failure to malloc, not an overflow
of the environment size, and Windows is just as (un)likely to run out of
heap as any other platform.

You can check how your platform behaves by applying this patch:

diff --git a/git.c b/git.c
index f10e434..57f6b12 100644
--- a/git.c
+++ b/git.c

@@ -223,6 +223,16 @@ static int handle_alias(int *argcp, const char ***argv)
 				alias_argv[i] = (*argv)[i];
 			alias_argv[argc] = NULL;
 
+			/* make gigantic environment */
+			{
+				int len = 256 * 1024;
+				char *buf = xmalloc(len);
+				memset(buf, 'z', len);
+				buf[len-1] = '\0';
+				if (setenv("FOO", buf, 1))
+					die("setenv failed");
+			}
+
 			ret = run_command_v_opt(alias_argv, RUN_USING_SHELL);
 			if (ret >= 0)   /* normal exit */
 				exit(ret);

and then running:

  git -c alias.foo='!echo ok' foo

which yields:

  fatal: cannot exec 'echo ok': Argument list too long

on Linux.

-Peff

PS I tried to come up with an invocation of git that would demonstrate
   this, but it turns out it's really hard. The contents of environment
   variables we set are either constants, come from the environment (so
   they can't be too big already!), or come from filesystem paths. So
   it's possible to overflow now, but you have to have a nearly-full
   environment in the first place, and then have a long path that tips
   it over the limit.