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

From: Jeff King <peff@peff.net>
Date: Feb 21, 2019, 13:58 UTC
Message-ID: <20190221135805.GB20899@sigill.intra.peff.net>
In-Reply-To: <nycvar.QRO.7.76.6.1902181950130.45@tvgsbejvaqbjf.bet>

On Mon, Feb 18, 2019 at 07:57:14PM +0100, Johannes Schindelin wrote:

Show 16 quoted lines

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

One suggestion I made elsewhere[1] is to use strintern() to de-dup the responses, and then keep them around forever. That bounds the memory usage by the number of unique environment variables requested (which is probably finite and limited), rather than the number of getenv() calls (which as we have seen can be done in an arbitrary-sized loop).

I say "probably" because it's possible that there is code which mutates the environment, reads it back, etc, in a loop. But that would be pretty out-of-character for Git, I think.

-Peff

[1] https://public-inbox.org/git/20190115194726.GA5818@sigill.intra.peff.net/