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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Feb 18, 2019, 18:57 UTC
Message-ID: <nycvar.QRO.7.76.6.1902181950130.45@tvgsbejvaqbjf.bet>
In-Reply-To: <96793350-0991-6e8f-6ab2-15777e2071be@jeffhostetler.com>

Hi Jeff,

On Fri, 15 Feb 2019, Jeff Hostetler wrote:

Show 23 quoted lines

> 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