Re: [PATCH 0/6] getenv() timing fixes

On Tue, Jan 15, 2019 at 11:32:56AM -0800, Junio C Hamano wrote:

Jeff King [off-list ref] writes:

quoted

On Sat, Jan 12, 2019 at 10:51:42AM -0800, Stefan Beller wrote:

quoted

quoted

I wonder, and not as "you should do this" feedback on this series, just

There is a getenv_safe() in environment.c, but I guess a xgetenv() that
takes the same parameters as getenv() is better for ease of use.

Yes, but it punts on the memory ownership by stuffing everything into an
argv_array. That saves a few lines if you're going to ask for five
variables, but for a single variable it's no better than:

  char *foo = getenv_safe("FOO");

You meant xstrdup_or_null(getenv("FOO")) here?  And did Stefan mean

	#define xgetenv(e) xstrdup_or_null(getenv(e))

?

Yes, I think that would be one possible implementation of a "safe"
getenv (and what I was thinking of specifically in that example).

The more involved one (that doesn't pass along memory ownership) is
something like:

  static struct hashmap env_cache;

  const char *getenv_safe(const char *name)
  {

	if (e = hashmap_get(&env_cache, name))
		return e->value;

        /* need some trickery to make sure xstrdup does not call getenv */
	e->value = xstrdup_or_null(getenv(name));
	e->name = xstrdup(name);
	hashmap_put(&env_cache, e);

	return e->value;
  }

with a matching setenv_safe() to drop the hashmap entry. Come to think
of it, this is really pretty equivalent to string-interning, which we
already have a hashmap for. I think one could argue that string
interning is basically just a controlled form of memory leaking, but
it's probably a reasonable compromise in this instance (i.e., we expect
to ask about a finite number of variables anyway; the important thing is
just that we don't leak memory for the same variable over and over).

-Peff