Re: [PATCH 1/1] mingw: safe-guard a bit more against getenv() problems

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Jeff,

On Fri, 15 Feb 2019, Jeff Hostetler wrote:

> On 2/15/2019 10:17 AM, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> > 
> > diff --git a/compat/mingw.c b/compat/mingw.c
> > index 4276297595..8141f77189 100644
> > --- a/compat/mingw.c
> > +++ b/compat/mingw.c
> > @@ -1632,7 +1632,7 @@ int mingw_kill(pid_t pid, int sig)
> >   */
> >   char *mingw_getenv(const char *name)
> >   {
> > -#define GETENV_MAX_RETAIN 30
> > +#define GETENV_MAX_RETAIN 64
> >    static char *values[GETENV_MAX_RETAIN];
> >    static int value_counter;
> >    int len_key, len_value;
> > 
> 
> Why not use a mem_pool for this?  We have that code isolated
> and re-usable now.  Have mingw_getenv() copy the string into
> the pool always return the pointer from within the pool.  The
> pool automatically handles allocating new blocks as necessary.
> And (if we care) we can bulk free the pool before existing.

The problem with the mem_pool would be that I do *not* want to accumulate
tons and tons of stale copies (remember that `difftool` bug? Apparently it
calls `getenv()` in a loop that scales with the number of modified files;
I would hate to have copies for all of those).

And all you could do to remedy this would be to blow away all of them it
the mem_pool grew too much, *including* the latest `getenv()` (which we
painfully keep valid, e.g. in the case of `git_committer_info()` which
calls `getenv()` *three* times *inside* the parameter list of the
`fmt_ident()` function call.

Of course, we could use a hashmap combined with a priority queue that
would cap at a given capacity, and once that is reached, culls the oldest
entries until the capacity is maintained again.

Taking a step back, thought, we are talking only about the
Windows-specific code, which is code we happen to have *some* control
over. On other platforms, we have a lot less control.

Meaning: the true fix would be some sort of static analysis that ensures
that getenv()'s returned values are duplicated unless it is super easy to
reason that the use is safe.

Ciao,
Dscho