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

From: Erik Faye-Lund <kusmabite@gmail.com>
Date: Dec 15, 2011, 18:04 UTC
Message-ID: <CABPQNSY+Q=Dt8oKodJMqRRb+ADdfDgQTxWKDfOpHvb5L9J=vvw@mail.gmail.com>
In-Reply-To: <20111214181658.GA1691@sigill.intra.peff.net>

On Wed, Dec 14, 2011 at 7:16 PM, Jeff King <peff@peff.net> wrote:

Show 19 quoted lines

> 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 are right; I just assumed that our setenv was implemented on top of SetEnvironmentVariable, which does impose max-limits on setting (if I read the documentation correctly): http://msdn.microsoft.com/en-us/library/windows/desktop/ms686206(v=vs.85).aspx

But we don't. We have mingw_setenv, which simply modifies environ.

Show 34 quoted lines

> 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.
>

Yeah, but I'm getting: $ git -c alias.foo='!echo ok' foo ok

Which is strange to me.

But if I do: $ git -c alias.foo='!echo $FOO' foo

it does echo a bunch of 'z's. And we get the expected amount:
$ git -c alias.foo='!echo $FOO' foo | wc -c
 262144

This strikes me as very strange, because we end up calling the ANSI version of CreateProcess with an environment-block beyond 32767 chars, which MSDN says should fail:

"The ANSI version of this function, CreateProcessA fails if the total size of the environment block for the process exceeds 32,767 characters."

http://msdn.microsoft.com/en-us/library/windows/desktop/ms682425(v=vs.85).aspx

Oh well, this is strange behavior in a good way; I'm not going to complain :P