From 66d271482bef884ac7ee82eb699e2e6f4e7ca5bf Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 17 Mar 2015 19:41:02 +0100 Subject: [PATCH 1/3] UTF-8 environment: be a little bit more defensive It is unlikely that we have an empty environment, ever, but *if* we do, when `environ_size - 1` is passed to `bsearchenv()` it is misinterpreted as a real large integer. To make the code truly defensive, refuse to do anything at all if the size is negative (which should not happen, of course). Signed-off-by: Johannes Schindelin --- compat/mingw.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/compat/mingw.c b/compat/mingw.c index b8675ae314..678631957a 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -1239,7 +1239,7 @@ static int bsearchenv(char **env, const char *name, size_t size) */ static int do_putenv(char **env, const char *name, int size, int free_old) { - int i = bsearchenv(env, name, size - 1); + int i = size <= 0 ? -1 : bsearchenv(env, name, size - 1); /* optionally free removed / replaced entry */ if (i >= 0 && free_old) @@ -1264,7 +1264,13 @@ static int do_putenv(char **env, const char *name, int size, int free_old) char *mingw_getenv(const char *name) { char *value; - int pos = bsearchenv(environ, name, environ_size - 1); + int pos; + + if (environ_size <= 0) + return NULL; + + pos = bsearchenv(environ, name, environ_size - 1); + if (pos < 0) return NULL; value = strchr(environ[pos], '='); From 6432af8787be04ab51e938e6461f47fc821817a1 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 18 Mar 2015 10:57:52 +0100 Subject: [PATCH 2/3] mingw: be more defensive when making the environment block Outside of our Windows-specific code, the end of the environment can be marked also by a pointer to a NUL character, not only by a NULL pointer as our code assumed so far. That led to a buffer overrun in `make_environment_block()` when running `git-remote-https` in `mintty` (because `curl_global_init()` added the `CHARSET` environment variable *outside* of `mingw_putenv()`, ending the environment in a pointer to an empty string). Side note for future debugging on Windows: when running programs in `mintty`, the standard input/output/error is not connected to a Win32 Console, but instead is pipe()d. That means that even stderr may not be written completely before a crash, but has to be fflush()ed explicitly. For example, when debugging crashes, the developer should insert an `fflush(stderr);` at the end of the `error()` function defined in usage.c. Signed-off-by: Johannes Schindelin --- compat/mingw.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/compat/mingw.c b/compat/mingw.c index 678631957a..f3a6c89dba 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -916,7 +916,7 @@ static wchar_t *make_environment_block(char **deltaenv) char **tmpenv; int i = 0, size = environ_size, wenvsz = 0, wenvpos = 0; - while (deltaenv && deltaenv[i]) + while (deltaenv && deltaenv[i] && *deltaenv[i]) i++; /* copy the environment, leaving space for changes */ @@ -924,11 +924,11 @@ static wchar_t *make_environment_block(char **deltaenv) memcpy(tmpenv, environ, size * sizeof(char*)); /* merge supplied environment changes into the temporary environment */ - for (i = 0; deltaenv && deltaenv[i]; i++) + for (i = 0; deltaenv && deltaenv[i] && *deltaenv[i]; i++) size = do_putenv(tmpenv, deltaenv[i], size, 0); /* create environment block from temporary environment */ - for (i = 0; tmpenv[i]; i++) { + for (i = 0; tmpenv[i] && *tmpenv[i]; i++) { size = 2 * strlen(tmpenv[i]) + 2; /* +2 for final \0 */ ALLOC_GROW(wenvblk, (wenvpos + size) * sizeof(wchar_t), wenvsz); wenvpos += xutftowcs(&wenvblk[wenvpos], tmpenv[i], size) + 1; From 12e5c37398674aa3f6d8804399f7219ad4fa325e Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 17 Mar 2015 19:41:31 +0100 Subject: [PATCH 3/3] mingw: be *very* wary about outside environment changes The environment is modified in most surprising circumstances, and not all of them are under Git's control. For example, calling curl_global_init() on Windows will ensure that the CHARSET variable is set, adding one if necessary. While the previous commit worked around crashes triggered by such outside changes of the environment by relaxing the requirement that the environment be terminated by a NULL pointer, the other assumption made by `mingw_getenv()` and `mingw_putenv()` is that the environment is sorted, for efficient lookup via binary search. Let's make real sure that our environment is intact before querying or modifying it, and reinitialize our idea of the environment if necessary. With this commit, before working on the environment we look briefly for indicators that the environment was modified outside of our control, and to ensure that it is terminated with a NULL pointer and sorted again in that case. Note: the indicators are maybe not sufficient. For example, when a variable is removed, it will not be noticed. It might also be a problem if outside changes to the environment result in a modified `environ` pointer: it is unclear whether such a modification could result in a problem when `mingw_putenv()` needs to `realloc()` the environment buffer. For the moment, however, the current fix works well enough, so let's only face the potential problems when (and if!) they occur. Signed-off-by: Johannes Schindelin --- compat/mingw.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 47 insertions(+), 2 deletions(-) diff --git a/compat/mingw.c b/compat/mingw.c index f3a6c89dba..d272476c13 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -905,6 +905,10 @@ static int do_putenv(char **env, const char *name, int size, int free_old); static int environ_size = 0; /* allocated size of environ array, in bytes */ static int environ_alloc = 0; +/* used as a indicator when the environment has been changed outside mingw.c */ +static char **saved_environ; + +static void maybe_reinitialize_environ(void); /* * Create environment block suitable for CreateProcess. Merges current @@ -914,7 +918,10 @@ static wchar_t *make_environment_block(char **deltaenv) { wchar_t *wenvblk = NULL; char **tmpenv; - int i = 0, size = environ_size, wenvsz = 0, wenvpos = 0; + int i = 0, size, wenvsz = 0, wenvpos = 0; + + maybe_reinitialize_environ(); + size = environ_size; while (deltaenv && deltaenv[i] && *deltaenv[i]) i++; @@ -1216,6 +1223,41 @@ static int compareenv(const void *v1, const void *v2) } } +/* + * Functions implemented outside Git are able to modify the environment, + * too. For example, cURL's curl_global_init() function sets the CHARSET + * environment variable (at least in certain circumstances). + * + * Therefore we need to be *really* careful *not* to assume that we have + * sole control over the environment and reinitalize it when necessary. + */ +static void maybe_reinitialize_environ(void) +{ + int i; + + if (!saved_environ) { + warning("MinGW environment not initialized yet"); + return; + } + + if (environ_size <= 0) + return; + + if (saved_environ != environ) + /* We have *no* idea how much space was allocated outside */ + environ_alloc = 0; + else if (!environ[environ_size - 1]) + return; /* still consistent */ + + for (i = 0; environ[i] && *environ[i]; i++) + ; /* continue counting */ + environ[i] = NULL; + environ_size = i + 1; + + /* sort environment for O(log n) getenv / putenv */ + qsort(environ, i, sizeof(char*), compareenv); +} + static int bsearchenv(char **env, const char *name, size_t size) { unsigned low = 0, high = size; @@ -1269,6 +1311,7 @@ char *mingw_getenv(const char *name) if (environ_size <= 0) return NULL; + maybe_reinitialize_environ(); pos = bsearchenv(environ, name, environ_size - 1); if (pos < 0) @@ -1279,7 +1322,9 @@ char *mingw_getenv(const char *name) int mingw_putenv(const char *namevalue) { + maybe_reinitialize_environ(); ALLOC_GROW(environ, (environ_size + 1) * sizeof(char*), environ_alloc); + saved_environ = environ; environ_size = do_putenv(environ, namevalue, environ_size, 1); return 0; } @@ -2104,7 +2149,7 @@ void mingw_startup() */ environ_size = i + 1; environ_alloc = alloc_nr(environ_size * sizeof(char*)); - environ = malloc_startup(environ_alloc); + saved_environ = environ = malloc_startup(environ_alloc); /* allocate buffer (wchar_t encodes to max 3 UTF-8 bytes) */ maxlen = 3 * maxlen + 1;