{"thread":{"id":"32529","subject":"[BUG/PATCH] setup: Copy an environment variable to avoid overwrites","startedAt":"2013-01-05T00:35:14Z","lastAt":"2013-01-07T15:45:46Z","messageCount":14,"participants":["David Michael","Junio C Hamano","Duy Nguyen","Nguyễn Thái Ngọc Duy","Matt Kraai","Jonathan Nieder","Erik Faye-Lund"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"205994","messageId":"CAEvUa7niTJVfp8_kuWs50kvhfZ59F-yAuAmeOXEduHXOq-tRFA@mail.gmail.com","threadId":"32529","inReplyTo":null,"subject":"[BUG/PATCH] setup: Copy an environment variable to avoid overwrites","fromName":"David Michael","fromEmail":"fedora.dm0@gmail.com","sentAt":"2013-01-05T00:35:14Z","receivedAt":"2013-01-05T00:35:14Z","isPatch":true,"sender":{"key":"fedora.dm0@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1379865?v=4"},"body":"It is possible for this pointer of the GIT_DIR environment variable to\nsurvive unduplicated until further getenv calls are made. The standards\nallow for subsequent calls of getenv to overwrite the string located at\nits returned pointer, and this can result in broken git operations on\ncertain platforms.\n\nSigned-off-by: David Michael \n---\n\nI have encountered an issue with consecutive calls to getenv\noverwriting earlier values. Most notably, it prevents a plain \"git\nclone\" from working.\n\nLong story short: This value of GIT_DIR gets passed around setup.c\nuntil it reaches check_repository_format_gently. This function calls\ngit_config_early, which eventually runs getenv(\"HOME\"). When it\nreturns back to check_repository_format_gently, the gitdir variable\ncontains my home directory path. The end result is that I wind up\nwith ~/objects/ etc. and a failed repository clone. (Simply adding a\nbare getenv(\"GIT_DIR\") afterwards to reset the pointer also corrects\nthe problem.)\n\nSince other platforms are apparently working, yet this getenv behavior\nis supported by the standards, I am left wondering if this could be a\nsymptom of something else being broken on my platform (z/OS). Can\nanyone more familiar with this part of git identify any condition that\nobviously should not be occurring?\n\nThanks.\n\n setup.c | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex f108c4b..64fb160 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -675,8 +675,12 @@ static const char\n*setup_git_directory_gently_1(int *nongit_ok)\n * validation.\n */\n gitdirenv = getenv(GIT_DIR_ENVIRONMENT);\n- if (gitdirenv)\n- return setup_explicit_git_dir(gitdirenv, cwd, len, nongit_ok);\n+ if (gitdirenv) {\n+ gitdirenv = xstrdup(gitdirenv);\n+ ret = setup_explicit_git_dir(gitdirenv, cwd, len, nongit_ok);\n+ free(gitdirenv);\n+ return ret;\n+ }\n\n if (env_ceiling_dirs) {\n string_list_split(&ceiling_dirs, env_ceiling_dirs, PATH_SEP, -1);\n--\n1.7.11.7\n"},{"id":"205995","messageId":"7vsj6gqvhc.fsf@alter.siamese.dyndns.org","threadId":"32529","inReplyTo":"CAEvUa7niTJVfp8_kuWs50kvhfZ59F-yAuAmeOXEduHXOq-tRFA@mail.gmail.com","subject":"Re: [BUG/PATCH] setup: Copy an environment variable to avoid overwrites","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-05T01:17:19Z","receivedAt":"2013-01-05T01:17:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Michael writes:\n\n> I have encountered an issue with consecutive calls to getenv\n> overwriting earlier values. Most notably, it prevents a plain \"git\n> clone\" from working.\n>\n> Long story short: This value of GIT_DIR gets passed around setup.c\n> until it reaches check_repository_format_gently. This function calls\n> git_config_early, which eventually runs getenv(\"HOME\"). When it\n> returns back to check_repository_format_gently, the gitdir variable\n> contains my home directory path. The end result is that I wind up\n> with ~/objects/ etc. and a failed repository clone. (Simply adding a\n> bare getenv(\"GIT_DIR\") afterwards to reset the pointer also corrects\n> the problem.)\n>\n> Since other platforms are apparently working, yet this getenv behavior\n> is supported by the standards, I am left wondering if this could be a\n> symptom of something else being broken on my platform (z/OS).\n\nThe execve(2) function\n\n int execve(const char *filename, char *const argv[],\n char *const envp[]);\n\ntakes a NULL terminated array of NUL terminated strings of form\n\"VAR=VAL\" in envp[], and this is kept in:\n\n\textern char **environ;\n\nof the new image that runs.\n\nThe most naive and straight-forward way to implement getenv(3) is to\niterate over this environ[] array to look for an element that begins\nwith \"GIT_DIR=\", and return the pointer pointing at the location one\nbyte past that equal sign. So even if the standard allowed the\nreturned value to be volatile across calls to getenv(3), it will\ntake *more* work for implementations if they want to break our use\npattern. They have to deliberately return a string that they will\noverwrite in subsequent calls to getenv(3).\n\nAlso the natural way to implement putenv(3) and setenv(3) is to\nreplace the pointer in the environ[] array (not overwrite the\nexisting string in environ[] that holds the \"VAR=VAL\" string that\nrepresents the current value, which might be shorter than the new\nvalue of the enviornment variable), hence even calling these\nfunctions is unlikely to invalidate the result you previously\nreceived from getenv(3).\n\nI am not at all surprised that nobody from other platforms has seen\nthis breakage.\n\nIn fact,\n\n http://pubs.opengroup.org/onlinepubs/9699919799/functions/getenv.html\n\nsays that only setenv(), unsetenv() and putenv() may invalidate\nprevious return values. Note that getenv() is not listed as a\nfunction that is allowed to break return values from a previous call\nto getenv().\n\nSo in short, your platform's getenv(3) emulation may be broken. We\nhave other calls to getenv() where we rely on the result of it being\nstable, and you might discover these places also break.\n\nHaving said that, we do have codepaths to update a handful of\nenvironment variables ourselves (GIT_DIR is among them), so I think\nyour patch is a good safety measure in general.\n\n> setup.c | 8 ++++++--\n> 1 file changed, 6 insertions(+), 2 deletions(-)\n>\n> diff --git a/setup.c b/setup.c\n> index f108c4b..64fb160 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -675,8 +675,12 @@ static const char\n> *setup_git_directory_gently_1(int *nongit_ok)\n> * validation.\n> */\n> gitdirenv = getenv(GIT_DIR_ENVIRONMENT);\n> - if (gitdirenv)\n> - return setup_explicit_git_dir(gitdirenv, cwd, len, nongit_ok);\n> + if (gitdirenv) {\n> + gitdirenv = xstrdup(gitdirenv);\n> + ret = setup_explicit_git_dir(gitdirenv, cwd, len, nongit_ok);\n> + free(gitdirenv);\n> + return ret;\n> + }\n>\n> if (env_ceiling_dirs) {\n> string_list_split(&ceiling_dirs, env_ceiling_dirs, PATH_SEP, -1);\n> --\n> 1.7.11.7\n"},{"id":"205996","messageId":"CAEvUa7mbs7rfGdfzbGb9p889K7GUbXFg9TUcFWFYKt8+8AKCiQ@mail.gmail.com","threadId":"32529","inReplyTo":"7vsj6gqvhc.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG/PATCH] setup: Copy an environment variable to avoid overwrites","fromName":"David Michael","fromEmail":"fedora.dm0@gmail.com","sentAt":"2013-01-05T02:15:55Z","receivedAt":"2013-01-05T02:15:55Z","isPatch":true,"sender":{"key":"fedora.dm0@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1379865?v=4"},"body":"Hi,\n\nOn Fri, Jan 4, 2013 at 8:17 PM, Junio C Hamano wrote:\n> In fact,\n>\n> http://pubs.opengroup.org/onlinepubs/9699919799/functions/getenv.html\n>\n> says that only setenv(), unsetenv() and putenv() may invalidate\n> previous return values. Note that getenv() is not listed as a\n> function that is allowed to break return values from a previous call\n> to getenv().\n\nBefore I sent the e-mail, I checked that very page to be sure I wasn't\nentirely insane. Specifically, the second paragraph begins with:\n\n> The string pointed to may be overwritten by a subsequent call to getenv(), [...]\n\nI read that line as confirmation that this is indeed acceptably\nstandard behavior. Even the getenv man page on my Fedora workstation\nsays:\n\n> 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).\n\nAm I misinterpreting these statements?\n\nThanks.\n\nDavid\n"},{"id":"205997","messageId":"CACsJy8BeuV8esGTWsQiT_G9pZE28s5KJxH6+dzdhioLgmSiNVg@mail.gmail.com","threadId":"32529","inReplyTo":"CAEvUa7niTJVfp8_kuWs50kvhfZ59F-yAuAmeOXEduHXOq-tRFA@mail.gmail.com","subject":"Re: [BUG/PATCH] setup: Copy an environment variable to avoid overwrites","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-01-05T02:45:40Z","receivedAt":"2013-01-05T02:45:40Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Jan 5, 2013 at 7:35 AM, David Michael wrote:\n> - if (gitdirenv)\n> - return setup_explicit_git_dir(gitdirenv, cwd, len, nongit_ok);\n> + if (gitdirenv) {\n> + gitdirenv = xstrdup(gitdirenv);\n> + ret = setup_explicit_git_dir(gitdirenv, cwd, len, nongit_ok);\n> + free(gitdirenv);\n> + return ret;\n> + }\n\nMaybe we could all this into a wrapper? If getenv() here has a\nproblem, many other places may have the same problem too. This\nsimplifies the change. But one has to check that getenv() must not be\nused in threaded code.\n\nchar *git_getenv(const char *env)\n{\n static int bufno;\n static char *buf[4];\n bufno = (bufno + 1) % 4;\n free(buf[bufno]);\n buf[bufno] = xstrdup(getenv(env));\n return buf[bufno];\n}\n#define getenv(x) git_getenv(x)\n\n-- \nDuy\n"},{"id":"205998","messageId":"7vobh4qmgl.fsf@alter.siamese.dyndns.org","threadId":"32529","inReplyTo":"7vsj6gqvhc.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG/PATCH] setup: Copy an environment variable to avoid overwrites","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-05T04:32:10Z","receivedAt":"2013-01-05T04:32:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> ... So even if the standard allowed the\n> returned value to be volatile across calls to getenv(3),...\n> ...\n> In fact,\n>\n> http://pubs.opengroup.org/onlinepubs/9699919799/functions/getenv.html\n>\n> says that only ...\n\nApparently I wasn't even reading what I was quoting carefully\nenough. The above does include getenv() as one of the functions\nthat are allowed to invalidate earlier return values.\n\nSorry about that. I'll go back to bed (I am a bit under the weather\nand OOO today). The conclusion in my original message is still\nvalid.\n\n> Having said that, we do have codepaths to update a handful of\n> environment variables ourselves (GIT_DIR is among them), so I think\n> your patch is a good safety measure in general.\n"},{"id":"205999","messageId":"7vk3rsqm6u.fsf@alter.siamese.dyndns.org","threadId":"32529","inReplyTo":"CACsJy8BeuV8esGTWsQiT_G9pZE28s5KJxH6+dzdhioLgmSiNVg@mail.gmail.com","subject":"Re: [BUG/PATCH] setup: Copy an environment variable to avoid overwrites","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-05T04:38:01Z","receivedAt":"2013-01-05T04:38:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> Maybe we could all this into a wrapper? If getenv() here has a\n> problem, many other places may have the same problem too. This\n> simplifies the change. But one has to check that getenv() must not be\n> used in threaded code.\n\nThat needs to be done regardless, if we care; POSIX explicitly says\ngetenv() need not be thread-safe.\n\nI personally do not think a wrapper with limited slots is a healthy\ndirection to go. Most places we use getenv() do not let the return\nvalue live across their scope, and those that do should explicitly\ncopy the value away. It's between validating that there is _no_ *env()\ncalls in the codepath between a getenv() call and the use of its\nreturn value, and validating that there is at most 4 such calls there.\nThe former is much easier to verify and maintain, I think.\n"},{"id":"206000","messageId":"CACsJy8CZe=qyzmG_1vdLYp07OvkDAU4wYc8MN3et7WBVmMhJOQ@mail.gmail.com","threadId":"32529","inReplyTo":"7vk3rsqm6u.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG/PATCH] setup: Copy an environment variable to avoid overwrites","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-01-05T06:24:19Z","receivedAt":"2013-01-05T06:24:19Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Jan 5, 2013 at 11:38 AM, Junio C Hamano wrote:\n> I personally do not think a wrapper with limited slots is a healthy\n> direction to go. Most places we use getenv() do not let the return\n> value live across their scope, and those that do should explicitly\n> copy the value away. It's between validating that there is _no_ *env()\n> calls in the codepath between a getenv() call and the use of its\n> return value, and validating that there is at most 4 such calls there.\n> The former is much easier to verify and maintain, I think.\n\nI did not look carefully and was scared of 143 getenv calls. But with\nabout 4 calls, yes it's best to do without the wrapper.\n-- \nDuy\n"},{"id":"206002","messageId":"7va9soqg6p.fsf@alter.siamese.dyndns.org","threadId":"32529","inReplyTo":"CACsJy8CZe=qyzmG_1vdLYp07OvkDAU4wYc8MN3et7WBVmMhJOQ@mail.gmail.com","subject":"Re: [BUG/PATCH] setup: Copy an environment variable to avoid overwrites","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-05T06:47:42Z","receivedAt":"2013-01-05T06:47:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> On Sat, Jan 5, 2013 at 11:38 AM, Junio C Hamano wrote:\n>> I personally do not think a wrapper with limited slots is a healthy\n>> direction to go. Most places we use getenv() do not let the return\n>> value live across their scope, and those that do should explicitly\n>> copy the value away. It's between validating that there is _no_ *env()\n>> calls in the codepath between a getenv() call and the use of its\n>> return value, and validating that there is at most 4 such calls there.\n>> The former is much easier to verify and maintain, I think.\n>\n> I did not look carefully and was scared of 143 getenv calls. But with\n> about 4 calls, yes it's best to do without the wrapper.\n\nJust to make sure you did not misunderstand, the 4 (four) in my\nmessage is not about \"4 calls among 143 are unsafe\".\n\nIt was referring to the number of rotating slots your patch defined,\nwhich means\n\n\tval = getenv(\"FOO\");\n ... random other code ...\n use(val)\n\nis safe only if random other code makes less than 4 getenv() calls.\n\nI didn't verify all of the call sites. It needs to be done with or\nwithout your wrapper patch. Without your wrapper, the validation\nneeds to make sure \"random other code\" above does not make any getenv()\ncall. With your wrapper, the validation needs to make sure \"random\nother code\" above does not make more than 4 such calls. My point\nwas that the effort needed for both are about the same, so your\nwrapper does not buy us much.\n"},{"id":"206016","messageId":"1357376146-7155-1-git-send-email-pclouds@gmail.com","threadId":"32529","inReplyTo":"CAEvUa7niTJVfp8_kuWs50kvhfZ59F-yAuAmeOXEduHXOq-tRFA@mail.gmail.com","subject":"[PATCH] Add getenv.so for catching invalid getenv() use via LD_PRELOAD","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-01-05T08:55:46Z","receivedAt":"2013-01-05T08:55:46Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Signed-off-by: Nguyễn Thái Ngọc Duy \n---\n Perhaps this will help the getenv bug hunting (I assume we do the\n hunting on Linux platform only). So far it catches this and is stuck\n at getenv in git_pager().\n\n diff --git a/exec_cmd.c b/exec_cmd.c\n index 125fa6f..d8be5ce 100644\n --- a/exec_cmd.c\n +++ b/exec_cmd.c\n @@ -97,7 +97,7 @@ static void add_path(struct strbuf *out, const char *path)\n \n void setup_path(void)\n {\n - const char *old_path = getenv(\"PATH\");\n + char *old_path = xstrdup(getenv(\"PATH\"));\n struct strbuf new_path = STRBUF_INIT;\n \n add_path(&new_path, git_exec_path());\n @@ -110,6 +110,7 @@ void setup_path(void)\n \n setenv(\"PATH\", new_path.buf, 1);\n \n + free(old_path);\n strbuf_release(&new_path);\n }\n\n contrib/getenv/Makefile | 2 ++\n contrib/getenv/getenv.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 69 insertions(+)\n create mode 100644 contrib/getenv/Makefile\n create mode 100644 contrib/getenv/getenv.c\n\ndiff --git a/contrib/getenv/Makefile b/contrib/getenv/Makefile\nnew file mode 100644\nindex 0000000..4881b85\n--- /dev/null\n+++ b/contrib/getenv/Makefile\n@@ -0,0 +1,2 @@\n+getenv.so: getenv.c\n+\t$(CC) -g -shared -fPIC -ldl -o $@ $<\ndiff --git a/contrib/getenv/getenv.c b/contrib/getenv/getenv.c\nnew file mode 100644\nindex 0000000..e351e10\n--- /dev/null\n+++ b/contrib/getenv/getenv.c\n@@ -0,0 +1,67 @@\n+#include \n+#include \n+#include \n+#include \n+#include \n+#include \n+#include \n+\n+/* Global symbols for easy access from gdb */\n+static char *getenv_current;\n+static char *getenv_prev;\n+\n+/*\n+ * Intercept standard getenv() via LD_PRELOAD. The return value is\n+ * made inaccessible by the next getenv() call. This helps catch\n+ * places that ignore the statement \"The string pointed to may be\n+ * overwritten by a subsequent call to getenv()\" [1].\n+ *\n+ * The backtrace is appended after the env string, which may be\n+ * helpful to identify where this getenv() is called in a core dump.\n+ *\n+ * [1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/getenv.html\n+ */\n+char *getenv(const char *name)\n+{\n+\tstatic char *(*libc_getenv)(const char*);\n+\tchar *value;\n+\n+\tif (!libc_getenv) {\n+\t\tvoid *libc = dlopen(LIBC_SO, RTLD_LAZY);\n+\t\tlibc_getenv = dlsym(libc, \"getenv\");\n+\t}\n+\tif (getenv_current) {\n+\t\tmprotect(getenv_current, strlen(getenv_current) + 1, PROT_NONE);\n+\t\tgetenv_prev = getenv_current;\n+\t\tgetenv_current = NULL;\n+\t}\n+\n+\tvalue = libc_getenv(name);\n+\tif (value) {\n+\t\tint len = strlen(value) + 1;\n+\t\tint backtrace_len = 0;\n+\t\tvoid *buffer[100];\n+\t\tchar **symbols;\n+\t\tint i, n;\n+\n+\t\tn = backtrace(buffer, 100);\n+\t\tsymbols = backtrace_symbols(buffer, n);\n+\t\tif (symbols) {\n+\t\t\tfor (i = 0;i < n; i++)\n+\t\t\t\tbacktrace_len += strlen(symbols[i]) + 1; /* \\n */\n+\t\t\tbacktrace_len++; /* NULL */\n+\t\t}\n+\n+\t\tgetenv_current = mmap(NULL, len + backtrace_len, PROT_READ | PROT_WRITE,\n+\t\t\t\t MAP_PRIVATE | MAP_ANONYMOUS, 0, 0);\n+\t\tmemcpy(getenv_current, value, len);\n+\t\tvalue = getenv_current;\n+\n+\t\tif (symbols) {\n+\t\t\tchar *p = getenv_current + len;\n+\t\t\tfor (i = 0; i < n; i++)\n+\t\t\t\tp += sprintf(p, \"%s\\n\", symbols[i]);\n+\t\t}\n+\t}\n+\treturn value;\n+}\n-- \n1.8.0.rc2.23.g1fb49df\n"},{"id":"206017","messageId":"20130105103900.GA4200@ftbfs.org","threadId":"32529","inReplyTo":"1357376146-7155-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] Add getenv.so for catching invalid getenv() use via LD_PRELOAD","fromName":"Matt Kraai","fromEmail":"kraai@ftbfs.org","sentAt":"2013-01-05T10:39:00Z","receivedAt":"2013-01-05T10:39:00Z","isPatch":true,"sender":{"key":"kraai@ftbfs.org","avatar":null},"body":"On Sat, Jan 05, 2013 at 03:55:46PM +0700, Nguyễn Thái Ngọc Duy wrote:\n> Perhaps this will help the getenv bug hunting (I assume we do the\n> hunting on Linux platform only). So far it catches this and is stuck\n> at getenv in git_pager().\n\nIt seems like a static analysis tool might be able to detect these\nproblems. Is there a way to do so using sparse?\n\n> +\t\tn = backtrace(buffer, 100);\n> +\t\tsymbols = backtrace_symbols(buffer, n);\n> +\t\tif (symbols) {\n> +\t\t\tfor (i = 0;i < n; i++)\n\ns/;i/; i/\n\n-- \nMatt Kraai\nhttps://ftbfs.org/kraai\n"},{"id":"206021","messageId":"CACsJy8A-Q8uDC0fJzspU8khCtWnMgmz66L2G9hLmsRYFYLaumw@mail.gmail.com","threadId":"32529","inReplyTo":"20130105103900.GA4200@ftbfs.org","subject":"Re: [PATCH] Add getenv.so for catching invalid getenv() use via LD_PRELOAD","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-01-05T11:37:54Z","receivedAt":"2013-01-05T11:37:54Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Jan 5, 2013 at 5:39 PM, Matt Kraai wrote:\n> On Sat, Jan 05, 2013 at 03:55:46PM +0700, Nguyễn Thái Ngọc Duy wrote:\n>> Perhaps this will help the getenv bug hunting (I assume we do the\n>> hunting on Linux platform only). So far it catches this and is stuck\n>> at getenv in git_pager().\n>\n> It seems like a static analysis tool might be able to detect these\n> problems. Is there a way to do so using sparse?\n\nThat was my first thought. But this may involve flow analysis and I\ndon't think sparse is up to it. ccc-analyzer is still pretty basic.\nAnd between static analysis and runtime check, I prefer the latter as\nit's more reliable as long as you have a good coverage test.\n\n>\n>> + n = backtrace(buffer, 100);\n>> + symbols = backtrace_symbols(buffer, n);\n>> + if (symbols) {\n>> + for (i = 0;i < n; i++)\n>\n> s/;i/; i/\n\nThanks. I will fix it later if people actually want this.\n-- \nDuy\n"},{"id":"206067","messageId":"20130105225338.GB3247@elie.Belkin","threadId":"32529","inReplyTo":"1357376146-7155-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] Add getenv.so for catching invalid getenv() use via LD_PRELOAD","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-01-05T22:53:38Z","receivedAt":"2013-01-05T22:53:38Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Nguyễn Thái Ngọc Duy wrote:\n\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> Perhaps this will help the getenv bug hunting\n\nEven if no one decides to do the getenv hunting (I haven't decided yet\nwhether it's worth the trouble, though patches like the setup_path()\none that make string lifetimes clearer are valuable anyway), this\nlooks useful as a debugging tool when people on strange platforms\nreport problems. And unlike the trick I sent a while ago, it doesn't\ninvolve recompiling git. So for what it's worth,\n\nAcked-by: Jonathan Nieder \n\nThanks.\n"},{"id":"206229","messageId":"CABPQNSbpyO5k8TauFi4+Yan_SiXgULn5F2SMJ_VkL-XX_vDB8w@mail.gmail.com","threadId":"32529","inReplyTo":"CAEvUa7niTJVfp8_kuWs50kvhfZ59F-yAuAmeOXEduHXOq-tRFA@mail.gmail.com","subject":"Re: [BUG/PATCH] setup: Copy an environment variable to avoid overwrites","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2013-01-07T15:28:28Z","receivedAt":"2013-01-07T15:28:28Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Sat, Jan 5, 2013 at 1:35 AM, David Michael wrote:\n> It is possible for this pointer of the GIT_DIR environment variable to\n> survive unduplicated until further getenv calls are made. The standards\n> allow for subsequent calls of getenv to overwrite the string located at\n> its returned pointer, and this can result in broken git operations on\n> certain platforms.\n>\n> Signed-off-by: David Michael \n> ---\n>\n> I have encountered an issue with consecutive calls to getenv\n> overwriting earlier values. Most notably, it prevents a plain \"git\n> clone\" from working.\n>\n> Long story short: This value of GIT_DIR gets passed around setup.c\n> until it reaches check_repository_format_gently. This function calls\n> git_config_early, which eventually runs getenv(\"HOME\"). When it\n> returns back to check_repository_format_gently, the gitdir variable\n> contains my home directory path. The end result is that I wind up\n> with ~/objects/ etc. and a failed repository clone. (Simply adding a\n> bare getenv(\"GIT_DIR\") afterwards to reset the pointer also corrects\n> the problem.)\n>\n> Since other platforms are apparently working, yet this getenv behavior\n> is supported by the standards, I am left wondering if this could be a\n> symptom of something else being broken on my platform (z/OS). Can\n> anyone more familiar with this part of git identify any condition that\n> obviously should not be occurring?\n>\n> Thanks.\n\nI have some patches of a similar nature here:\n\nhttps://github.com/kusma/git/commits/work/getenv-safety\n\nThese were written for an earlier version of the UTF-8 patches for Git\nfor Windows, where we were looking into allowing getenv to use a\nstatic buffer to convert the environment variables from UTF-16 (which\nis what Windows maintains) to UTF-8. We ended converting the\nenvironment on start-up instead, so these weren't needed for us. But\nperhaps they can be of use to someone else?\n"},{"id":"206230","messageId":"CAEvUa7nLKv3R5DcqCe6zJsxgENS2YkNRSzeYk3WT_TjCJsFdnQ@mail.gmail.com","threadId":"32529","inReplyTo":"1357376146-7155-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] Add getenv.so for catching invalid getenv() use via LD_PRELOAD","fromName":"David Michael","fromEmail":"fedora.dm0@gmail.com","sentAt":"2013-01-07T15:45:46Z","receivedAt":"2013-01-07T15:45:46Z","isPatch":true,"sender":{"key":"fedora.dm0@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1379865?v=4"},"body":"Hi,\n\nOn Sat, Jan 5, 2013 at 3:55 AM, Nguyễn Thái Ngọc Duy wrote:\n> Perhaps this will help the getenv bug hunting (I assume we do the\n> hunting on Linux platform only). So far it catches this and is stuck\n> at getenv in git_pager().\n\nFor the record: I have been testing a macro pointing getenv at an\nalternate implementation for the purpose of my port. This alternate\nfunction will return a unique pointer for each variable, as opposed to\nusing the same static buffer. IBM considers this function a\nproprietary z/OS extension (whereas the other is labelled as\nconforming to half a dozen standards), but it seems to be more in-line\nwith the behavior git expects.\n\nSo I agree this patch is useful for reaching strict conformance to the\npublished specs, but I think we can go back to assuming no known\nplatforms are broken and unusable because of this.\n\nThanks.\n\nDavid\n"}]}