git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: "Duy Nguyen" <pclouds@gmail.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Johannes Schindelin" <Johannes.Schindelin@gmx.de>,
	"Git Mailing List" <git@vger.kernel.org>,
	"SZEDER Gábor" <szeder.dev@gmail.com>,
	"Vasco Almeida" <vascomalmeida@sapo.pt>,
	"Jiang Xin" <worldhello.net@gmail.com>
Subject: Re: [PATCH] Poison gettext with the Ook language
Date: Thu, 25 Oct 2018 02:20:38 -0400	[thread overview]
Message-ID: <20181025062037.GC11460@sigill.intra.peff.net> (raw)
In-Reply-To: <xmqqh8halm20.fsf@gitster-ct.c.googlers.com>

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

  reply	other threads:[~2018-10-25  6:20 UTC|newest]

Thread overview: 49+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-22 15:36 [PATCH] Poison gettext with the Ook language Nguyễn Thái Ngọc Duy
2018-10-22 20:22 ` SZEDER Gábor
2018-10-22 20:22   ` [PATCH 1/8] test-lib.sh: preserve GIT_GETTEXT_POISON from the environment SZEDER Gábor
2018-10-22 20:22   ` [PATCH 2/8] gettext: don't poison if GIT_GETTEXT_POISON is set but empty SZEDER Gábor
2018-10-22 20:38     ` Ævar Arnfjörð Bjarmason
2018-10-22 20:22   ` [PATCH 3/8] lib-rebase: loosen GETTEXT_POISON check in fake editor SZEDER Gábor
2018-10-22 20:22   ` [PATCH 4/8] gettext: #ifdef away GETTEXT POISON-related code from _() and Q_() SZEDER Gábor
2018-10-22 20:22   ` [PATCH 5/8] gettext: put "# GETTEXT POISON #" string literal into a macro SZEDER Gábor
2018-10-22 20:22   ` [PATCH 6/8] gettext: use an enum for the mode of GETTEXT POISONing SZEDER Gábor
2018-10-22 20:22   ` [PATCH 7/8] gettext: introduce GIT_GETTEXT_POISON=scrambled SZEDER Gábor
2018-10-23 14:44     ` Duy Nguyen
2018-10-22 20:22   ` [PATCH 8/8] travis-ci: run GETTEXT POISON build job in scrambled mode, too SZEDER Gábor
2018-10-23 14:37   ` [PATCH] Poison gettext with the Ook language Duy Nguyen
2018-10-27 10:11   ` Jakub Narebski
2018-10-22 20:52 ` Johannes Schindelin
2018-10-22 21:09 ` Ævar Arnfjörð Bjarmason
2018-10-22 21:46   ` Ævar Arnfjörð Bjarmason
2018-10-22 23:04   ` Junio C Hamano
2018-10-23 21:01     ` [PATCH] i18n: make GETTEXT_POISON a runtime option Ævar Arnfjörð Bjarmason
2018-10-24  5:45       ` Junio C Hamano
2018-10-24  7:44         ` Jeff King
2018-10-25  1:00           ` Junio C Hamano
2018-10-25  1:09             ` Jeff King
2018-10-25  1:24               ` Ramsay Jones
2018-10-25 21:23                 ` Jeff King
2018-10-26 19:20                   ` Ævar Arnfjörð Bjarmason
2018-10-27  6:59                     ` Jeff King
2018-10-27 10:42                       ` Ævar Arnfjörð Bjarmason
2018-10-24 11:47         ` [PATCH v2] " Ævar Arnfjörð Bjarmason
2018-11-01 19:31           ` [PATCH v3] " Ævar Arnfjörð Bjarmason
2018-11-02  3:11             ` Junio C Hamano
2018-11-02 16:37             ` SZEDER Gábor
2018-11-08 20:26               ` Ævar Arnfjörð Bjarmason
2018-11-08 20:51                 ` SZEDER Gábor
2018-11-08  3:24             ` Junio C Hamano
2018-11-08 19:12               ` Eric Sunshine
2018-11-08 21:15             ` [PATCH v4 0/2] " Ævar Arnfjörð Bjarmason
2018-11-08 21:15             ` [PATCH v4 1/2] " Ævar Arnfjörð Bjarmason
2018-11-08 21:15             ` [PATCH v4 2/2] Makefile: ease dynamic-gettext-poison transition Ævar Arnfjörð Bjarmason
2018-10-23  9:30   ` [PATCH] Poison gettext with the Ook language Johannes Schindelin
2018-10-23 10:17     ` Ævar Arnfjörð Bjarmason
2018-10-23 11:07       ` Johannes Schindelin
2018-10-23 15:00       ` Duy Nguyen
2018-10-23 16:45         ` Ævar Arnfjörð Bjarmason
2018-10-24 14:41           ` Duy Nguyen
2018-10-24 17:54             ` Ævar Arnfjörð Bjarmason
2018-10-25  3:52             ` Junio C Hamano
2018-10-25  6:20               ` Jeff King [this message]
2018-10-27  6:55                 ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20181025062037.GC11460@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=pclouds@gmail.com \
    --cc=szeder.dev@gmail.com \
    --cc=vascomalmeida@sapo.pt \
    --cc=worldhello.net@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).