Re: [PATCH] Poison gettext with the Ook language

[Jeff King <peff@peff.net>]

On Thu, Oct 25, 2018 at 12:52:55PM +0900, Junio C Hamano wrote:

> Duy Nguyen <pclouds@gmail.com> writes:
> 
> > The person who writes
> >
> >  printf(_("%s"), getenv("foo"));
> >
> > may not go through the same thought process as with complexFunction().
> > If _() calls getenv(), because you the order of parameter evaluation
> > is unspecified, you cannot be sure if getenv("foo") will be called
> > before or after the one inside _(). One of them may screw up the
> > other.
> 
> Yup, sometimes we've been sloppy but we should strive to mimick
> efforts like f4ef5173 ("determine_author_info(): copy getenv
> output", 2014-08-27).

I've wondered about this before. Even calling:

  foo = xstrdup(getenv("bar"));

is not necessarily correct, because xstrdup() relies on xmalloc(), which
may check GIT_ALLOC_LIMIT (we do cache that result, but it can happen on
the first malloc).

I also wouldn't be surprised if there are cases in our threaded code
that use getenv() without taking a lock.

I've definitely run into setenv()/getenv() races on Linux (inside Git,
even, though it was while working on custom code). But I wonder how
common it is for getenv() to be invalidated by another getenv() call.
Certainly POSIX allows it, but every implementation I've seen (which is
admittedly few) is passing back pointers to a chunk of environment
memory.

I.e., could we mostly ignore this problem as not applying to most modern
systems? And if there is such a system, give it a fallback like:

  /*
   * For systems that use a single buffer for getenv(), this hacks
   * around it by giving it _four_ buffers. That's just punting on
   * the problem, but it at least gives enough breathing room for
   * the caller to do something sane like use non-trivial functions
   * to copy the string. It still does nothing for threading, but
   * hopefully such systems don't support pthreads in the first place. ;)
   */
  const char *xgetenv(const char *key)
  {
	static struct strbuf bufs[] = {
		STRBUF_INIT, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT
	};
	static unsigned int cur;
	struct strbuf *buf;
	const char *value;
	size_t len;

	value = getenv(key);
	if (!value)
		return NULL;

	buf = bufs[cur++];
	cur %= ARRAY_SIZE(bufs);

	/*
	 * We have to do this length check ourselves, because allocating
	 * the strbuf may invalidate "value"!
	 */
	len = strlen(value);
	if (buf->alloc <= len) {
		strbuf_grow(buf, len);
		value = getenv(key);
		if (!value)
			return NULL; /* whoops, it went away! */
		len = strlen(value); /* paranoia that it didn't change */
	}

	strbuf_reset(buf);
	strbuf_add(buf, value, len);

	return buf->buf;
  }

I dunno. Maybe I am being overly optimistic. But I strongly suspect we
have such bugs already in our code base, and nobody has run into them
(OTOH, they are quite finicky due to things like the caching I
mentioned).

-Peff