[BUG/PATCH] setup: Copy an environment variable to avoid overwrites

It is possible for this pointer of the GIT_DIR environment variable to
survive unduplicated until further getenv calls are made.  The standards
allow for subsequent calls of getenv to overwrite the string located at
its returned pointer, and this can result in broken git operations on
certain platforms.

Signed-off-by: David Michael <redacted>
---

I have encountered an issue with consecutive calls to getenv
overwriting earlier values.  Most notably, it prevents a plain "git
clone" from working.

Long story short: This value of GIT_DIR gets passed around setup.c
until it reaches check_repository_format_gently.  This function calls
git_config_early, which eventually runs getenv("HOME").  When it
returns back to check_repository_format_gently, the gitdir variable
contains my home directory path.  The end result is that I wind up
with ~/objects/ etc. and a failed repository clone.  (Simply adding a
bare getenv("GIT_DIR") afterwards to reset the pointer also corrects
the problem.)

Since other platforms are apparently working, yet this getenv behavior
is supported by the standards, I am left wondering if this could be a
symptom of something else being broken on my platform (z/OS).  Can
anyone more familiar with this part of git identify any condition that
obviously should not be occurring?

Thanks.

 setup.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/setup.c b/setup.c
index f108c4b..64fb160 100644
--- a/setup.c
+++ b/setup.c

@@ -675,8 +675,12 @@ static const char

*setup_git_directory_gently_1(int *nongit_ok)
      * validation.
      */
     gitdirenv = getenv(GIT_DIR_ENVIRONMENT);
-    if (gitdirenv)
-        return setup_explicit_git_dir(gitdirenv, cwd, len, nongit_ok);
+    if (gitdirenv) {
+        gitdirenv = xstrdup(gitdirenv);
+        ret = setup_explicit_git_dir(gitdirenv, cwd, len, nongit_ok);
+        free(gitdirenv);
+        return ret;
+    }

     if (env_ceiling_dirs) {
         string_list_split(&ceiling_dirs, env_ceiling_dirs, PATH_SEP, -1);
--
1.7.11.7

David Michael [off-list ref] writes:

I have encountered an issue with consecutive calls to getenv
overwriting earlier values.  Most notably, it prevents a plain "git
clone" from working.

Long story short: This value of GIT_DIR gets passed around setup.c
until it reaches check_repository_format_gently.  This function calls
git_config_early, which eventually runs getenv("HOME").  When it
returns back to check_repository_format_gently, the gitdir variable
contains my home directory path.  The end result is that I wind up
with ~/objects/ etc. and a failed repository clone.  (Simply adding a
bare getenv("GIT_DIR") afterwards to reset the pointer also corrects
the problem.)

Since other platforms are apparently working, yet this getenv behavior
is supported by the standards, I am left wondering if this could be a
symptom of something else being broken on my platform (z/OS).

The execve(2) function

       int execve(const char *filename, char *const argv[],
                  char *const envp[]);

takes a NULL terminated array of NUL terminated strings of form
"VAR=VAL" in envp[], and this is kept in:

	extern char **environ;

of the new image that runs.

The most naive and straight-forward way to implement getenv(3) is to
iterate over this environ[] array to look for an element that begins
with "GIT_DIR=", and return the pointer pointing at the location one
byte past that equal sign.  So even if the standard allowed the
returned value to be volatile across calls to getenv(3), it will
take *more* work for implementations if they want to break our use
pattern.  They have to deliberately return a string that they will
overwrite in subsequent calls to getenv(3).

Also the natural way to implement putenv(3) and setenv(3) is to
replace the pointer in the environ[] array (not overwrite the
existing string in environ[] that holds the "VAR=VAL" string that
represents the current value, which might be shorter than the new
value of the enviornment variable), hence even calling these
functions is unlikely to invalidate the result you previously
received from getenv(3).

I am not at all surprised that nobody from other platforms has seen
this breakage.

In fact,

    http://pubs.opengroup.org/onlinepubs/9699919799/functions/getenv.html

says that only setenv(), unsetenv() and putenv() may invalidate
previous return values.  Note that getenv() is not listed as a
function that is allowed to break return values from a previous call
to getenv().

So in short, your platform's getenv(3) emulation may be broken.  We
have other calls to getenv() where we rely on the result of it being
stable, and you might discover these places also break.

Having said that, we do have codepaths to update a handful of
environment variables ourselves (GIT_DIR is among them), so I think
your patch is a good safety measure in general.

Hi,

On Fri, Jan 4, 2013 at 8:17 PM, Junio C Hamano [off-list ref] wrote:

In fact,

    http://pubs.opengroup.org/onlinepubs/9699919799/functions/getenv.html

says that only setenv(), unsetenv() and putenv() may invalidate
previous return values.  Note that getenv() is not listed as a
function that is allowed to break return values from a previous call
to getenv().

Before I sent the e-mail, I checked that very page to be sure I wasn't
entirely insane.  Specifically, the second paragraph begins with:

The string pointed to may be overwritten by a subsequent call to getenv(), [...]

I read that line as confirmation that this is indeed acceptably
standard behavior.  Even the getenv man page on my Fedora workstation
says:

The string pointed to by the return value of getenv() may be statically allocated, and can be modified by a subsequent call to getenv(), putenv(3), setenv(3), or unsetenv(3).

Am I misinterpreting these statements?

Thanks.

David

On Sat, Jan 5, 2013 at 7:35 AM, David Michael [off-list ref] wrote:

-    if (gitdirenv)
-        return setup_explicit_git_dir(gitdirenv, cwd, len, nongit_ok);
+    if (gitdirenv) {
+        gitdirenv = xstrdup(gitdirenv);
+        ret = setup_explicit_git_dir(gitdirenv, cwd, len, nongit_ok);
+        free(gitdirenv);
+        return ret;
+    }

Maybe we could all this into a wrapper? If getenv() here has a
problem, many other places may have the same problem too. This
simplifies the change. But one has to check that getenv() must not be
used in threaded code.

char *git_getenv(const char *env)
{
   static int bufno;
   static char *buf[4];
   bufno = (bufno + 1) % 4;
   free(buf[bufno]);
   buf[bufno] = xstrdup(getenv(env));
   return buf[bufno];
}
#define getenv(x) git_getenv(x)

-- 
Duy

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 Perhaps this will help the getenv bug hunting (I assume we do the
 hunting on Linux platform only). So far it catches this and is stuck
 at getenv in git_pager().

  diff --git a/exec_cmd.c b/exec_cmd.c
  index 125fa6f..d8be5ce 100644
  --- a/exec_cmd.c
  +++ b/exec_cmd.c
  @@ -97,7 +97,7 @@ static void add_path(struct strbuf *out, const char *path)
   
   void setup_path(void)
   {
  -       const char *old_path = getenv("PATH");
  +       char *old_path = xstrdup(getenv("PATH"));
          struct strbuf new_path = STRBUF_INIT;
   
          add_path(&new_path, git_exec_path());
  @@ -110,6 +110,7 @@ void setup_path(void)
   
          setenv("PATH", new_path.buf, 1);
   
  +       free(old_path);
          strbuf_release(&new_path);
   }

 contrib/getenv/Makefile |  2 ++
 contrib/getenv/getenv.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 69 insertions(+)
 create mode 100644 contrib/getenv/Makefile
 create mode 100644 contrib/getenv/getenv.c

diff --git a/contrib/getenv/Makefile b/contrib/getenv/Makefile
new file mode 100644
index 0000000..4881b85
--- /dev/null
+++ b/contrib/getenv/Makefile

@@ -0,0 +1,2 @@
+getenv.so: getenv.c
+	$(CC) -g -shared -fPIC -ldl -o $@ $<

diff --git a/contrib/getenv/getenv.c b/contrib/getenv/getenv.c
new file mode 100644
index 0000000..e351e10
--- /dev/null
+++ b/contrib/getenv/getenv.c

@@ -0,0 +1,67 @@
+#include <gnu/lib-names.h>
+#include <sys/mman.h>
+#include <dlfcn.h>
+#include <execinfo.h>
+#include <stdlib.h>
+#include <string.h>
+#include <stdio.h>
+
+/* Global symbols for easy access from gdb */
+static char *getenv_current;
+static char *getenv_prev;
+
+/*
+ * Intercept standard getenv() via LD_PRELOAD. The return value is
+ * made inaccessible by the next getenv() call. This helps catch
+ * places that ignore the statement "The string pointed to may be
+ * overwritten by a subsequent call to getenv()" [1].
+ *
+ * The backtrace is appended after the env string, which may be
+ * helpful to identify where this getenv() is called in a core dump.
+ *
+ * [1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/getenv.html
+ */
+char *getenv(const char *name)
+{
+	static char *(*libc_getenv)(const char*);
+	char *value;
+
+	if (!libc_getenv) {
+		void *libc = dlopen(LIBC_SO, RTLD_LAZY);
+		libc_getenv = dlsym(libc, "getenv");
+	}
+	if (getenv_current) {
+		mprotect(getenv_current, strlen(getenv_current) + 1, PROT_NONE);
+		getenv_prev = getenv_current;
+		getenv_current = NULL;
+	}
+
+	value = libc_getenv(name);
+	if (value) {
+		int len = strlen(value) + 1;
+		int backtrace_len = 0;
+		void *buffer[100];
+		char **symbols;
+		int i, n;
+
+		n = backtrace(buffer, 100);
+		symbols = backtrace_symbols(buffer, n);
+		if (symbols) {
+			for (i = 0;i < n; i++)
+				backtrace_len += strlen(symbols[i]) + 1; /* \n */
+			backtrace_len++; /* NULL */
+		}
+
+		getenv_current = mmap(NULL, len + backtrace_len, PROT_READ | PROT_WRITE,
+				   MAP_PRIVATE | MAP_ANONYMOUS, 0, 0);
+		memcpy(getenv_current, value, len);
+		value = getenv_current;
+
+		if (symbols) {
+			char *p = getenv_current + len;
+			for (i = 0; i < n; i++)
+				p += sprintf(p, "%s\n", symbols[i]);
+		}
+	}
+	return value;
+}

-- 
1.8.0.rc2.23.g1fb49df

On Sat, Jan 05, 2013 at 03:55:46PM +0700, Nguyễn Thái Ngọc Duy wrote:

 Perhaps this will help the getenv bug hunting (I assume we do the
 hunting on Linux platform only). So far it catches this and is stuck
 at getenv in git_pager().

It seems like a static analysis tool might be able to detect these
problems.  Is there a way to do so using sparse?

+		n = backtrace(buffer, 100);
+		symbols = backtrace_symbols(buffer, n);
+		if (symbols) {
+			for (i = 0;i < n; i++)

s/;i/; i/

-- 
Matt Kraai
https://ftbfs.org/kraai

On Sat, Jan 5, 2013 at 5:39 PM, Matt Kraai [off-list ref] wrote:

On Sat, Jan 05, 2013 at 03:55:46PM +0700, Nguyễn Thái Ngọc Duy wrote:

quoted

 Perhaps this will help the getenv bug hunting (I assume we do the
 hunting on Linux platform only). So far it catches this and is stuck
 at getenv in git_pager().

It seems like a static analysis tool might be able to detect these
problems.  Is there a way to do so using sparse?

That was my first thought. But this may involve flow analysis and I
don't think sparse is up to it. ccc-analyzer is still pretty basic.
And between static analysis and runtime check, I prefer the latter as
it's more reliable as long as you have a good coverage test.

quoted

+             n = backtrace(buffer, 100);
+             symbols = backtrace_symbols(buffer, n);
+             if (symbols) {
+                     for (i = 0;i < n; i++)

s/;i/; i/

Thanks. I will fix it later if people actually want this.
-- 
Duy

Nguyễn Thái Ngọc Duy wrote:

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 Perhaps this will help the getenv bug hunting

Even if no one decides to do the getenv hunting (I haven't decided yet
whether it's worth the trouble, though patches like the setup_path()
one that make string lifetimes clearer are valuable anyway), this
looks useful as a debugging tool when people on strange platforms
report problems.  And unlike the trick I sent a while ago, it doesn't
involve recompiling git.  So for what it's worth,

Acked-by: Jonathan Nieder <redacted>

Thanks.

On Sat, Jan 5, 2013 at 1:35 AM, David Michael [off-list ref] wrote:

It is possible for this pointer of the GIT_DIR environment variable to
survive unduplicated until further getenv calls are made.  The standards
allow for subsequent calls of getenv to overwrite the string located at
its returned pointer, and this can result in broken git operations on
certain platforms.

Signed-off-by: David Michael <redacted>
---

I have encountered an issue with consecutive calls to getenv
overwriting earlier values.  Most notably, it prevents a plain "git
clone" from working.

Long story short: This value of GIT_DIR gets passed around setup.c
until it reaches check_repository_format_gently.  This function calls
git_config_early, which eventually runs getenv("HOME").  When it
returns back to check_repository_format_gently, the gitdir variable
contains my home directory path.  The end result is that I wind up
with ~/objects/ etc. and a failed repository clone.  (Simply adding a
bare getenv("GIT_DIR") afterwards to reset the pointer also corrects
the problem.)

Since other platforms are apparently working, yet this getenv behavior
is supported by the standards, I am left wondering if this could be a
symptom of something else being broken on my platform (z/OS).  Can
anyone more familiar with this part of git identify any condition that
obviously should not be occurring?

Thanks.

I have some patches of a similar nature here:

https://github.com/kusma/git/commits/work/getenv-safety

These were written for an earlier version of the UTF-8 patches for Git
for Windows, where we were looking into allowing getenv to use a
static buffer to convert the environment variables from UTF-16 (which
is what Windows maintains) to UTF-8. We ended converting the
environment on start-up instead, so these weren't needed for us. But
perhaps they can be of use to someone else?

Hi,

On Sat, Jan 5, 2013 at 3:55 AM, Nguyễn Thái Ngọc Duy [off-list ref] wrote:

 Perhaps this will help the getenv bug hunting (I assume we do the
 hunting on Linux platform only). So far it catches this and is stuck
 at getenv in git_pager().

For the record: I have been testing a macro pointing getenv at an
alternate implementation for the purpose of my port.  This alternate
function will return a unique pointer for each variable, as opposed to
using the same static buffer.  IBM considers this function a
proprietary z/OS extension (whereas the other is labelled as
conforming to half a dozen standards), but it seems to be more in-line
with the behavior git expects.

So I agree this patch is useful for reaching strict conformance to the
published specs, but I think we can go back to assuming no known
platforms are broken and unusable because of this.

Thanks.

David