{"thread":{"id":"10966","subject":"[PATCH 1/3] sha1_file.c: Fix size_t related printf format warnings","startedAt":"2007-11-21T20:27:18Z","lastAt":"2008-01-03T07:53:30Z","messageCount":18,"participants":["Steffen Prohaska","Johannes Schindelin","Junio C Hamano","Johannes Sixt","Dmitry Kakurin"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"60566","messageId":"11956768414090-git-send-email-prohaska@zib.de","threadId":"10966","inReplyTo":null,"subject":"[PATCH 0/3] msysgit fallout","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2007-11-21T20:27:18Z","receivedAt":"2007-11-21T20:27:18Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"Here are three patches that come from msysgit. They do not bring\nany functional changes, but only clean up code, or fix warnings.\n\n Steffen\n\n builtin-init-db.c | 4 +---\n git.c | 6 +++---\n path.c | 2 +-\n setup.c | 2 +-\n sha1_file.c | 10 ++++++----\n 5 files changed, 12 insertions(+), 12 deletions(-)\n\n [PATCH 1/3] sha1_file.c: Fix size_t related printf format warnings\n [PATCH 2/3] builtin-init-db: use get_git_dir() instead of getenv()\n [PATCH 3/3] Replace setenv(GIT_DIR_ENVIRONMENT, ...) with set_git_dir()\n"},{"id":"60564","messageId":"11956768412804-git-send-email-prohaska@zib.de","threadId":"10966","inReplyTo":"11956768414090-git-send-email-prohaska@zib.de","subject":"[PATCH 1/3] sha1_file.c: Fix size_t related printf format warnings","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2007-11-21T20:27:19Z","receivedAt":"2007-11-21T20:27:19Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"The old way of fixing warnings did not succeed on MinGW. MinGW\ndoes not support C99 printf format strings for size_t [1]. But\ngcc on MinGW issues warnings if C99 printf format is not used.\nHence, the old stragegy to avoid warnings fails.\n\n[1] http://www.mingw.org/MinGWiki/index.php/C99\n\nThis commits passes arguments of type size_t through a tiny\nhelper functions that casts to the type expected by the format\nstring.\n\nSigned-off-by: Steffen Prohaska \n---\n sha1_file.c | 10 ++++++----\n 1 files changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex f007874..4f68e53 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -25,8 +25,10 @@\n \n #ifdef NO_C99_FORMAT\n #define SZ_FMT \"lu\"\n+static unsigned long sz_fmt(size_t s) { return (unsigned long)s; }\n #else\n #define SZ_FMT \"zu\"\n+static size_t sz_fmt(size_t s) { return s; }\n #endif\n \n const unsigned char null_sha1[20];\n@@ -423,9 +425,9 @@ void pack_report(void)\n \t\t\"pack_report: getpagesize() = %10\" SZ_FMT \"\\n\"\n \t\t\"pack_report: core.packedGitWindowSize = %10\" SZ_FMT \"\\n\"\n \t\t\"pack_report: core.packedGitLimit = %10\" SZ_FMT \"\\n\",\n-\t\t(size_t) getpagesize(),\n-\t\tpacked_git_window_size,\n-\t\tpacked_git_limit);\n+\t\tsz_fmt(getpagesize()),\n+\t\tsz_fmt(packed_git_window_size),\n+\t\tsz_fmt(packed_git_limit));\n \tfprintf(stderr,\n \t\t\"pack_report: pack_used_ctr = %10u\\n\"\n \t\t\"pack_report: pack_mmap_calls = %10u\\n\"\n@@ -435,7 +437,7 @@ void pack_report(void)\n \t\tpack_used_ctr,\n \t\tpack_mmap_calls,\n \t\tpack_open_windows, peak_pack_open_windows,\n-\t\tpack_mapped, peak_pack_mapped);\n+\t\tsz_fmt(pack_mapped), sz_fmt(peak_pack_mapped));\n }\n \n static int check_packed_git_idx(const char *path, struct packed_git *p)\n-- \n1.5.3.5.750.g8692\n"},{"id":"60565","messageId":"11956768413887-git-send-email-prohaska@zib.de","threadId":"10966","inReplyTo":"11956768412804-git-send-email-prohaska@zib.de","subject":"[PATCH 2/3] builtin-init-db: use get_git_dir() instead of getenv()","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2007-11-21T20:27:20Z","receivedAt":"2007-11-21T20:27:20Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"We have a function get_git_dir(). So let's use it,\ninstead of querying the environment.\n\nSigned-off-by: Steffen Prohaska \n---\n builtin-init-db.c | 4 +---\n 1 files changed, 1 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin-init-db.c b/builtin-init-db.c\nindex 763fa55..80d2f27 100644\n--- a/builtin-init-db.c\n+++ b/builtin-init-db.c\n@@ -378,9 +378,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)\n \t/*\n \t * Set up the default .git directory contents\n \t */\n-\tgit_dir = getenv(GIT_DIR_ENVIRONMENT);\n-\tif (!git_dir)\n-\t\tgit_dir = DEFAULT_GIT_DIR_ENVIRONMENT;\n+\tgit_dir = get_git_dir();\n \tsafe_create_dir(git_dir, 0);\n \n \t/* Check to see if the repository version is right.\n-- \n1.5.3.5.750.g8692\n"},{"id":"60567","messageId":"11956768412755-git-send-email-prohaska@zib.de","threadId":"10966","inReplyTo":"11956768413887-git-send-email-prohaska@zib.de","subject":"[PATCH 3/3] Replace setenv(GIT_DIR_ENVIRONMENT, ...) with set_git_dir()","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2007-11-21T20:27:21Z","receivedAt":"2007-11-21T20:27:21Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"From: Dmitry Kakurin \n\nWe have a function set_git_dir(). So let's use it, instead\nof setting the evironment directly.\n\nThis also fixes a problem on Windows: environment.c caches\nresults of many getenv calls. A setenv(X) call to the Windows\nC runtime (CRT) invalidates all previous values returned by\ngetenv(X). So cached values become dangling pointers.\n\nBefore this change, git clone was failing with 'invalid object\nname HEAD' if ran from Windows' cmd.exe.\n\n[sp: rebased; split off get_git_dir() in builtin-init-db.c\n to separate commit; adjusted commit message. ]\n\nSigned-off-by: Dmitry Kakurin \nSigned-off-by: Steffen Prohaska \n---\n git.c | 6 +++---\n path.c | 2 +-\n setup.c | 2 +-\n 3 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 7604319..8bc25b7 100644\n--- a/git.c\n+++ b/git.c\n@@ -45,14 +45,14 @@ static int handle_options(const char*** argv, int* argc, int* envchanged)\n \t\t\t\tfprintf(stderr, \"No directory given for --git-dir.\\n\" );\n \t\t\t\tusage(git_usage_string);\n \t\t\t}\n-\t\t\tsetenv(GIT_DIR_ENVIRONMENT, (*argv)[1], 1);\n+\t\t\tset_git_dir( (*argv)[1] );\n \t\t\tif (envchanged)\n \t\t\t\t*envchanged = 1;\n \t\t\t(*argv)++;\n \t\t\t(*argc)--;\n \t\t\thandled++;\n \t\t} else if (!prefixcmp(cmd, \"--git-dir=\")) {\n-\t\t\tsetenv(GIT_DIR_ENVIRONMENT, cmd + 10, 1);\n+\t\t\tset_git_dir(cmd + 10);\n \t\t\tif (envchanged)\n \t\t\t\t*envchanged = 1;\n \t\t} else if (!strcmp(cmd, \"--work-tree\")) {\n@@ -72,7 +72,7 @@ static int handle_options(const char*** argv, int* argc, int* envchanged)\n \t\t} else if (!strcmp(cmd, \"--bare\")) {\n \t\t\tstatic char git_dir[PATH_MAX+1];\n \t\t\tis_bare_repository_cfg = 1;\n-\t\t\tsetenv(GIT_DIR_ENVIRONMENT, getcwd(git_dir, sizeof(git_dir)), 0);\n+\t\t\tset_git_dir(getcwd(git_dir, sizeof(git_dir)));\n \t\t\tif (envchanged)\n \t\t\t\t*envchanged = 1;\n \t\t} else {\ndiff --git a/path.c b/path.c\nindex 4260952..f26a4a1 100644\n--- a/path.c\n+++ b/path.c\n@@ -248,7 +248,7 @@ char *enter_repo(char *path, int strict)\n \n \tif (access(\"objects\", X_OK) == 0 && access(\"refs\", X_OK) == 0 &&\n \t validate_headref(\"HEAD\") == 0) {\n-\t\tsetenv(GIT_DIR_ENVIRONMENT, \".\", 1);\n+\t\tset_git_dir(\".\");\n \t\tcheck_repository_format();\n \t\treturn path;\n \t}\ndiff --git a/setup.c b/setup.c\nindex 43cd3f9..8dbd46c 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -285,7 +285,7 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \t\t\tinside_git_dir = 1;\n \t\t\tif (!work_tree_env)\n \t\t\t\tinside_work_tree = 0;\n-\t\t\tsetenv(GIT_DIR_ENVIRONMENT, \".\", 1);\n+\t\t\tset_git_dir(\".\");\n \t\t\treturn NULL;\n \t\t}\n \t\tchdir(\"..\");\n-- \n1.5.3.5.750.g8692\n"},{"id":"60592","messageId":"Pine.LNX.4.64.0711220121560.27959@racer.site","threadId":"10966","inReplyTo":"11956768412755-git-send-email-prohaska@zib.de","subject":"Re: [PATCH 3/3] Replace setenv(GIT_DIR_ENVIRONMENT, ...) with set_git_dir()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-11-22T01:22:38Z","receivedAt":"2007-11-22T01:22:38Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 21 Nov 2007, Steffen Prohaska wrote:\n\n> We have a function set_git_dir(). So let's use it, instead of setting \n> the evironment directly.\n\nDoes this not have a fundamental issue? When you call other git programs \nwith run_command(), you _need_ GIT_DIR to be set, no?\n\nCiao,\nDscho\n"},{"id":"60599","messageId":"7v63zv9fel.fsf@gitster.siamese.dyndns.org","threadId":"10966","inReplyTo":"Pine.LNX.4.64.0711220121560.27959@racer.site","subject":"Re: [PATCH 3/3] Replace setenv(GIT_DIR_ENVIRONMENT, ...) with set_git_dir()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-22T02:34:42Z","receivedAt":"2007-11-22T02:34:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Hi,\n>\n> On Wed, 21 Nov 2007, Steffen Prohaska wrote:\n>\n>> We have a function set_git_dir(). So let's use it, instead of setting \n>> the evironment directly.\n>\n> Does this not have a fundamental issue? When you call other git programs \n> with run_command(), you _need_ GIT_DIR to be set, no?\n\nIt is much worse. set_git_dir() does not just setenv() but does\nsetup_git_env() as well.\n"},{"id":"60605","messageId":"C50619A0-4A67-4968-8431-D7A685F723B7@zib.de","threadId":"10966","inReplyTo":"7v63zv9fel.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 3/3] Replace setenv(GIT_DIR_ENVIRONMENT, ...) with set_git_dir()","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2007-11-22T06:13:50Z","receivedAt":"2007-11-22T06:13:50Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"\nOn Nov 22, 2007, at 3:34 AM, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n>\n>> Hi,\n>>\n>> On Wed, 21 Nov 2007, Steffen Prohaska wrote:\n>>\n>>> We have a function set_git_dir(). So let's use it, instead of \n>>> setting\n>>> the evironment directly.\n>>\n>> Does this not have a fundamental issue? When you call other git \n>> programs\n>> with run_command(), you _need_ GIT_DIR to be set, no?\n>\n> It is much worse. set_git_dir() does not just setenv() but does\n> setup_git_env() as well.\n\nWhat do your comments mean?\n\nMy understanding is that set_git_dir() sets the environment and\nthen calls setup_git_env() to cache all pointers. This call\nupdates dangling pointer if they have been cached earlier.\n\nBut maybe there is a hidden secret that I don't see.\n\n\tSteffen\n"},{"id":"60607","messageId":"47452FD9.9000501@viscovery.net","threadId":"10966","inReplyTo":"7v63zv9fel.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 3/3] Replace setenv(GIT_DIR_ENVIRONMENT, ...) with set_git_dir()","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2007-11-22T07:29:29Z","receivedAt":"2007-11-22T07:29:29Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Junio C Hamano schrieb:\n> Johannes Schindelin writes:\n> \n>> Hi,\n>>\n>> On Wed, 21 Nov 2007, Steffen Prohaska wrote:\n>>\n>>> We have a function set_git_dir(). So let's use it, instead of setting \n>>> the evironment directly.\n>> Does this not have a fundamental issue? When you call other git programs \n>> with run_command(), you _need_ GIT_DIR to be set, no?\n> \n> It is much worse. set_git_dir() does not just setenv() but does\n> setup_git_env() as well.\n\nI don't see what's wrong with that. Could you please explain?\n\n-- Hannes\n"},{"id":"60609","messageId":"7vsl2y90pm.fsf@gitster.siamese.dyndns.org","threadId":"10966","inReplyTo":"C50619A0-4A67-4968-8431-D7A685F723B7@zib.de","subject":"Re: [PATCH 3/3] Replace setenv(GIT_DIR_ENVIRONMENT, ...) with set_git_dir()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-22T07:52:05Z","receivedAt":"2007-11-22T07:52:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Steffen Prohaska writes:\n\n> On Nov 22, 2007, at 3:34 AM, Junio C Hamano wrote:\n>\n>> Johannes Schindelin writes:\n>>\n>>> Does this not have a fundamental issue? When you call other git\n>>> programs\n>>> with run_command(), you _need_ GIT_DIR to be set, no?\n>>\n>> It is much worse. set_git_dir() does not just setenv() but does\n>> setup_git_env() as well.\n>\n> What do your comments mean?\n>\n> My understanding is that set_git_dir() sets the environment and\n> then calls setup_git_env() to cache all pointers. This call\n> updates dangling pointer if they have been cached earlier.\n\nWell, I was agreeing with you. \"Worse\" was about what the\ncurrent code does _not_ do.\n\nIf there are earlier calls that obtain locations relative to the\nearlier definition of GIT_DIR, the locations they obtained are\nnot just stored in memory that is \"dangling\" (which was what\nyour proposed log message described) but they are also\ninconsistent with the updated definition of GIT_DIR.\n\nI suspect Johannes mistook set_git_dir() was only local\n(i.e. per calling process) matter without noticing that it has\nits own setenv() when he made that comment, hence my response to\npoint out that the current code only calls setenv(), but\nset_git_dir() does setup_git_env() too, which should hide the\ninconsistency problem.\n\nHOWEVER.\n\nI suspect that if there are even earlier callers than these\nearly parts in the codepaths (handle_options, enter_repo, and\nsetup_git_directory_gently), maybe these earlier callers are\ndoing something wrong. Logically, if you are somewhere very\nearly in the codepath that you can still change the value of\nGIT_DIR, you shouldn't have assumed the unknown value of GIT_DIR\nand cached locations relative to that directory, no? What are\nthe problematic callers? What values do they access and why?\n"},{"id":"60617","messageId":"52415F60-C080-4260-86CD-32A379482341@zib.de","threadId":"10966","inReplyTo":"7vsl2y90pm.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 3/3] Replace setenv(GIT_DIR_ENVIRONMENT, ...) with set_git_dir()","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2007-11-22T08:31:49Z","receivedAt":"2007-11-22T08:31:49Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"\nOn Nov 22, 2007, at 8:52 AM, Junio C Hamano wrote:\n\n> Steffen Prohaska writes:\n>\n>> On Nov 22, 2007, at 3:34 AM, Junio C Hamano wrote:\n>>\n>>> Johannes Schindelin writes:\n>>>\n>>>> Does this not have a fundamental issue? When you call other git\n>>>> programs\n>>>> with run_command(), you _need_ GIT_DIR to be set, no?\n>>>\n>>> It is much worse. set_git_dir() does not just setenv() but does\n>>> setup_git_env() as well.\n>>\n>> What do your comments mean?\n>>\n>> My understanding is that set_git_dir() sets the environment and\n>> then calls setup_git_env() to cache all pointers. This call\n>> updates dangling pointer if they have been cached earlier.\n>\n> Well, I was agreeing with you. \"Worse\" was about what the\n> current code does _not_ do.\n>\n> If there are earlier calls that obtain locations relative to the\n> earlier definition of GIT_DIR, the locations they obtained are\n> not just stored in memory that is \"dangling\" (which was what\n> your proposed log message described) but they are also\n> inconsistent with the updated definition of GIT_DIR.\n>\n> I suspect Johannes mistook set_git_dir() was only local\n> (i.e. per calling process) matter without noticing that it has\n> its own setenv() when he made that comment, hence my response to\n> point out that the current code only calls setenv(), but\n> set_git_dir() does setup_git_env() too, which should hide the\n> inconsistency problem.\n>\n> HOWEVER.\n>\n> I suspect that if there are even earlier callers than these\n> early parts in the codepaths (handle_options, enter_repo, and\n> setup_git_directory_gently), maybe these earlier callers are\n> doing something wrong. Logically, if you are somewhere very\n> early in the codepath that you can still change the value of\n> GIT_DIR, you shouldn't have assumed the unknown value of GIT_DIR\n> and cached locations relative to that directory, no? What are\n> the problematic callers? What values do they access and why?\n\n\nI thought about these questions, too. But only very briefly.\nI did not analyze the code path that lead to calls of getenv().\n\nI'm not sure if it's really necessary. Calling set_git_dir()\nlooks more sensible too me than the old code. I believe using\nset_git_dir() is the safer choice, and should not do any harm.\nSo I stopped analyzing too much, and instead proposed to use\nset_git_dir().\n\nInteresting, though, is to find out if we have other potentially\ndangerous calls to getenv() that are not removed by this patch.\n\n\tSteffen\n"},{"id":"60631","messageId":"474552D6.5060305@viscovery.net","threadId":"10966","inReplyTo":"52415F60-C080-4260-86CD-32A379482341@zib.de","subject":"Re: [PATCH 3/3] Replace setenv(GIT_DIR_ENVIRONMENT, ...) with set_git_dir()","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2007-11-22T09:58:46Z","receivedAt":"2007-11-22T09:58:46Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Steffen Prohaska schrieb:\n> On Nov 22, 2007, at 8:52 AM, Junio C Hamano wrote:\n>> I suspect that if there are even earlier callers than these\n>> early parts in the codepaths (handle_options, enter_repo, and\n>> setup_git_directory_gently), maybe these earlier callers are\n>> doing something wrong. Logically, if you are somewhere very\n>> early in the codepath that you can still change the value of\n>> GIT_DIR, you shouldn't have assumed the unknown value of GIT_DIR\n>> and cached locations relative to that directory, no? What are\n>> the problematic callers? What values do they access and why?\n> \n> \n> I thought about these questions, too. But only very briefly.\n> I did not analyze the code path that lead to calls of getenv().\n> \n> I'm not sure if it's really necessary. Calling set_git_dir()\n> looks more sensible too me than the old code. I believe using\n> set_git_dir() is the safer choice, and should not do any harm.\n> So I stopped analyzing too much, and instead proposed to use\n> set_git_dir().\n\nJunio's point is this: If we stumble over a dangling pointer that getenv() \nproduced, then this has obviously happened before setenv(GIT_DIR), and \ncaching that pointer is probably the wrong thing to do anyway (because it \nrefers to the wrong GIT_DIR) and needs to be fixed.\n\nSo the task is to find those traps. Dmitry obviously stumbled over one case, \nbut I haven't ever encountered any problems with the current code. But then \nthis might be sheer luck. And I'm not a heavy user of export GIT_DIR=foo, \neither. Do *you* know a problematic case?\n\n> Interesting, though, is to find out if we have other potentially\n> dangerous calls to getenv() that are not removed by this patch.\n\nSide note for other readers: This is a Windows specific problem for the \nmoment because its getenv() does not behave well.\n\n-- Hannes\n"},{"id":"60696","messageId":"3B6B19E6-255F-4D8F-B6A3-255A9E8E0AB0@zib.de","threadId":"10966","inReplyTo":"474552D6.5060305@viscovery.net","subject":"Re: [PATCH 3/3] Replace setenv(GIT_DIR_ENVIRONMENT, ...) with set_git_dir()","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2007-11-22T17:56:23Z","receivedAt":"2007-11-22T17:56:23Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"\nOn Nov 22, 2007, at 10:58 AM, Johannes Sixt wrote:\n\n> Steffen Prohaska schrieb:\n>> On Nov 22, 2007, at 8:52 AM, Junio C Hamano wrote:\n>>> I suspect that if there are even earlier callers than these\n>>> early parts in the codepaths (handle_options, enter_repo, and\n>>> setup_git_directory_gently), maybe these earlier callers are\n>>> doing something wrong. Logically, if you are somewhere very\n>>> early in the codepath that you can still change the value of\n>>> GIT_DIR, you shouldn't have assumed the unknown value of GIT_DIR\n>>> and cached locations relative to that directory, no? What are\n>>> the problematic callers? What values do they access and why?\n>> I thought about these questions, too. But only very briefly.\n>> I did not analyze the code path that lead to calls of getenv().\n>> I'm not sure if it's really necessary. Calling set_git_dir()\n>> looks more sensible too me than the old code. I believe using\n>> set_git_dir() is the safer choice, and should not do any harm.\n>> So I stopped analyzing too much, and instead proposed to use\n>> set_git_dir().\n>\n> Junio's point is this: If we stumble over a dangling pointer that \n> getenv() produced, then this has obviously happened before setenv \n> (GIT_DIR), and caching that pointer is probably the wrong thing to \n> do anyway (because it refers to the wrong GIT_DIR) and needs to be \n> fixed.\n\nI see your point. It is probably more important to investigate\nthis than I recognized at a first glance.\n\n\n> So the task is to find those traps. Dmitry obviously stumbled over \n> one case, but I haven't ever encountered any problems with the \n> current code. But then this might be sheer luck. And I'm not a \n> heavy user of export GIT_DIR=foo, either. Do *you* know a \n> problematic case?\n\nNo. I only stumbled over the code, when I reviewed differences\nbetween msysgit and mingw. I rarely use GIT_DIR=foo. Actually,\nI can't remember the last time I did.\n\n\n>> Interesting, though, is to find out if we have other potentially\n>> dangerous calls to getenv() that are not removed by this patch.\n>\n> Side note for other readers: This is a Windows specific problem for \n> the moment because its getenv() does not behave well.\n\nYes, and apparently even nobody knows how to trigger the problem\non Windows. At this point, we only know that caching getenv()\ncalls is unsafe, while on Unix it is safe (at least for BSD\nit's documented to be safe).\n\n\tSteffen\n"},{"id":"60715","messageId":"Pine.LNX.4.64.0711222149390.27959@racer.site","threadId":"10966","inReplyTo":"3B6B19E6-255F-4D8F-B6A3-255A9E8E0AB0@zib.de","subject":"Re: [PATCH 3/3] Replace setenv(GIT_DIR_ENVIRONMENT, ...) with set_git_dir()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-11-22T22:09:10Z","receivedAt":"2007-11-22T22:09:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 22 Nov 2007, Steffen Prohaska wrote:\n\n> Yes, and apparently even nobody knows how to trigger the problem on \n> Windows.\n\nA quick and easy way would be to instrument getenv(), unsetenv() and \nsetenv(), which would trigger an error. Something like this (but you \nwill have to put in a few \"extern called_getenv; called_getenv = 0;\", \nsince already a simple git-init fails because of setup_path()):\n\n-- snipsnap --\n[PATCH] Instrument getenv(), setenv(), unsetenv() and putenv()\n\n... for finding places where a pointer obtained by getenv() could\nbe invalidated later.\n\n---\n\n environment.c | 1 +\n git-compat-util.h | 31 +++++++++++++++++++++++++++++++\n 2 files changed, 32 insertions(+), 0 deletions(-)\n\ndiff --git a/environment.c b/environment.c\nindex ce75e98..027340e 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -9,6 +9,7 @@\n */\n #include \"cache.h\"\n \n+int called_setenv, called_getenv;\n char git_default_email[MAX_GITNAME];\n char git_default_name[MAX_GITNAME];\n int trust_executable_bit = 1;\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 79eb10e..a41469b 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -427,4 +427,35 @@ static inline int strtol_i(char const *s, int base, int *result)\n \treturn 0;\n }\n \n+extern int called_setenv, called_getenv;\n+static inline char *test_getenv(const char *name)\n+{\n+\tif (!called_setenv)\n+\t\twarning (\"called test_getenv %s\", name);\n+\tcalled_getenv = 1;\n+\treturn getenv(name);\n+}\n+static inline int test_setenv(const char *name, const char *value, int overwrite)\n+{\n+\tif (!called_setenv && called_getenv)\n+\t\tdie (\"getenv was called before setenv(%s, %s, %d)\",\n+\t\t\t\tname, value, overwrite);\n+\treturn setenv(name, value, overwrite);\n+}\n+static inline int test_unsetenv(const char *name)\n+{\n+\tif (!called_setenv && called_getenv)\n+\t\tdie (\"getenv was called before unsetenv(%s)\", name);\n+\treturn unsetenv(name);\n+}\n+static inline int test_putenv(char *string)\n+{\n+\tif (!called_setenv && called_getenv)\n+\t\tdie (\"getenv was called before putenv(%s)\", string);\n+\treturn putenv(string);\n+}\n+#define getenv test_getenv\n+#define setenv test_setenv\n+#define unsetenv test_unsetenv\n+\n #endif\n"},{"id":"64301","messageId":"0FFA1D0A-DB70-446C-9C43-A6FBAEFE7CA4@zib.de","threadId":"10966","inReplyTo":"3B6B19E6-255F-4D8F-B6A3-255A9E8E0AB0@zib.de","subject":"Re: [PATCH 3/3] Replace setenv(GIT_DIR_ENVIRONMENT, ...) with set_git_dir()","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2008-01-01T18:52:30Z","receivedAt":"2008-01-01T18:52:30Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"Eventually I found some time to investigate this issue ...\n\n\nOn Nov 22, 2007, at 6:56 PM, Steffen Prohaska wrote:\n\n>\n> On Nov 22, 2007, at 10:58 AM, Johannes Sixt wrote:\n>\n>> Steffen Prohaska schrieb:\n>>> On Nov 22, 2007, at 8:52 AM, Junio C Hamano wrote:\n>>>> I suspect that if there are even earlier callers than these\n>>>> early parts in the codepaths (handle_options, enter_repo, and\n>>>> setup_git_directory_gently), maybe these earlier callers are\n>>>> doing something wrong. Logically, if you are somewhere very\n>>>> early in the codepath that you can still change the value of\n>>>> GIT_DIR, you shouldn't have assumed the unknown value of GIT_DIR\n>>>> and cached locations relative to that directory, no? What are\n>>>> the problematic callers? What values do they access and why?\n>>> I thought about these questions, too. But only very briefly.\n>>> I did not analyze the code path that lead to calls of getenv().\n>>> I'm not sure if it's really necessary. Calling set_git_dir()\n>>> looks more sensible too me than the old code. I believe using\n>>> set_git_dir() is the safer choice, and should not do any harm.\n>>> So I stopped analyzing too much, and instead proposed to use\n>>> set_git_dir().\n>>\n>> Junio's point is this: If we stumble over a dangling pointer that \n>> getenv() produced, then this has obviously happened before setenv \n>> (GIT_DIR), and caching that pointer is probably the wrong thing to \n>> do anyway (because it refers to the wrong GIT_DIR) and needs to be \n>> fixed.\n>\n> I see your point. It is probably more important to investigate\n> this than I recognized at a first glance.\n\nI instrumented the code to verify if setenv(GIT_DIR) is called after\nsetup_git_env(). This is not the case for all tests.\n\nI also searched for problematic code paths.\n\nsetup_git_directory_gently() looks correct. It explicitly calls\ngetenv(GIT_DIR_ENVIRONMENT); but uses the value returned in a\nsafe manner. It does not cache the result and the only code path\nthat calls set_git_dir() does not access the return value of the\ngetenv() call after the call to set_git_dir().\n\nsetup_work_tree() looks correct, too. Here, get_git_dir() is\ncalled, which implicitly results in caching the pointer returned\nfrom getenv(GIT_DIR_ENVIRONMENT). But the result of get_git_dir() is\nneither cached nor used after a subsequent call to set_git_dir().\n\nSo, I don't find any obvious problems.\n\n\n>>> Interesting, though, is to find out if we have other potentially\n>>> dangerous calls to getenv() that are not removed by this patch.\n>>\n>> Side note for other readers: This is a Windows specific problem \n>> for the moment because its getenv() does not behave well.\n>\n> Yes, and apparently even nobody knows how to trigger the problem\n> on Windows. At this point, we only know that caching getenv()\n> calls is unsafe, while on Unix it is safe (at least for BSD\n> it's documented to be safe).\n\nIn conclusion, using setenv() as in the original code instead of\nset_git_dir() should be safe and this patch is not needed.\n\nI tend to revert the changes in msysgit and see if we hit any\nproblems. But I'll wait until 1.5.4 is released.\n\n\tSteffen\n"},{"id":"64367","messageId":"a1bbc6950801022007w164d050bhf8a8a8c0ad6736b@mail.gmail.com","threadId":"10966","inReplyTo":"0FFA1D0A-DB70-446C-9C43-A6FBAEFE7CA4@zib.de","subject":"Re: [PATCH 3/3] Replace setenv(GIT_DIR_ENVIRONMENT, ...) with set_git_dir()","fromName":"Dmitry Kakurin","fromEmail":"dmitry.kakurin@gmail.com","sentAt":"2008-01-03T04:07:35Z","receivedAt":"2008-01-03T04:07:35Z","isPatch":true,"sender":{"key":"dmitry.kakurin@gmail.com","avatar":null},"body":"On Jan 1, 2008 10:52 AM, Steffen Prohaska wrote:\n> In conclusion, using setenv() as in the original code instead of\n> set_git_dir() should be safe and this patch is not needed.\n>\n> I tend to revert the changes in msysgit and see if we hit any\n> problems. But I'll wait until 1.5.4 is released.\n>\n> Steffen\n\nPlease don't revert this change. I've made it in response to git clone\nfailing, commit 855f254b2b5b083a63fc8d7709a42e2cbdc5a136.\n\n-- \n- Dmitry\n"},{"id":"64373","messageId":"42F546CE-2564-4DFB-B260-FD50BD51F2BB@zib.de","threadId":"10966","inReplyTo":"a1bbc6950801022007w164d050bhf8a8a8c0ad6736b@mail.gmail.com","subject":"Re: [PATCH 3/3] Replace setenv(GIT_DIR_ENVIRONMENT, ...) with set_git_dir()","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2008-01-03T06:02:23Z","receivedAt":"2008-01-03T06:02:23Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"\nOn Jan 3, 2008, at 5:07 AM, Dmitry Kakurin wrote:\n\n> On Jan 1, 2008 10:52 AM, Steffen Prohaska wrote:\n>> In conclusion, using setenv() as in the original code instead of\n>> set_git_dir() should be safe and this patch is not needed.\n>>\n>> I tend to revert the changes in msysgit and see if we hit any\n>> problems. But I'll wait until 1.5.4 is released.\n>>\n>> Steffen\n>\n> Please don't revert this change. I've made it in response to git clone\n> failing, commit 855f254b2b5b083a63fc8d7709a42e2cbdc5a136.\n\nI know. But I cannot reproduce the error.\n\nDo you have a test case that demonstrates the problem?\n\nI either want to see the patch upstream in official git or revert\nit in msysgit. But I cannot answer the questions that were\nraised after I sent the patch (see earlier in this thread). And\nI can't see the problem that your patch solves, even after\nspending some time on reading and instrumenting code.\n\n\tSteffen\n"},{"id":"64374","messageId":"a1bbc6950801022226k32cfd9b9t62939b3cbedef247@mail.gmail.com","threadId":"10966","inReplyTo":"42F546CE-2564-4DFB-B260-FD50BD51F2BB@zib.de","subject":"Re: [PATCH 3/3] Replace setenv(GIT_DIR_ENVIRONMENT, ...) with set_git_dir()","fromName":"Dmitry Kakurin","fromEmail":"dmitry.kakurin@gmail.com","sentAt":"2008-01-03T06:26:55Z","receivedAt":"2008-01-03T06:26:55Z","isPatch":true,"sender":{"key":"dmitry.kakurin@gmail.com","avatar":null},"body":"On Jan 2, 2008 10:02 PM, Steffen Prohaska wrote:\n>\n>\n> On Jan 3, 2008, at 5:07 AM, Dmitry Kakurin wrote:\n>\n> > On Jan 1, 2008 10:52 AM, Steffen Prohaska wrote:\n> >> In conclusion, using setenv() as in the original code instead of\n> >> set_git_dir() should be safe and this patch is not needed.\n> >>\n> >> I tend to revert the changes in msysgit and see if we hit any\n> >> problems. But I'll wait until 1.5.4 is released.\n> >>\n> >> Steffen\n> >\n> > Please don't revert this change. I've made it in response to git clone\n> > failing, commit 855f254b2b5b083a63fc8d7709a42e2cbdc5a136.\n>\n> I know. But I cannot reproduce the error.\n>\n> Do you have a test case that demonstrates the problem?\n>\n> I either want to see the patch upstream in official git or revert\n> it in msysgit. But I cannot answer the questions that were\n> raised after I sent the patch (see earlier in this thread). And\n> I can't see the problem that your patch solves, even after\n> spending some time on reading and instrumenting code.\n\nI remember that the problem was as simple as git clone or git clone\n--bare failing.\nAlso I'm not sure if it matters but I'm running Vista.\nThere is also a chance that code has changed since then and this\nproblem went away.\n\n-- \n- Dmitry\n"},{"id":"64375","messageId":"8BA05F64-A3E6-48B8-9A8C-1BA361DDF65E@zib.de","threadId":"10966","inReplyTo":"a1bbc6950801022226k32cfd9b9t62939b3cbedef247@mail.gmail.com","subject":"Re: [PATCH 3/3] Replace setenv(GIT_DIR_ENVIRONMENT, ...) with set_git_dir()","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2008-01-03T07:53:30Z","receivedAt":"2008-01-03T07:53:30Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"\nOn Jan 3, 2008, at 7:26 AM, Dmitry Kakurin wrote:\n\n> On Jan 2, 2008 10:02 PM, Steffen Prohaska wrote:\n>>\n>>\n>> On Jan 3, 2008, at 5:07 AM, Dmitry Kakurin wrote:\n>>\n>>> On Jan 1, 2008 10:52 AM, Steffen Prohaska wrote:\n>>>> In conclusion, using setenv() as in the original code instead of\n>>>> set_git_dir() should be safe and this patch is not needed.\n>>>>\n>>>> I tend to revert the changes in msysgit and see if we hit any\n>>>> problems. But I'll wait until 1.5.4 is released.\n>>>>\n>>>> Steffen\n>>>\n>>> Please don't revert this change. I've made it in response to git \n>>> clone\n>>> failing, commit 855f254b2b5b083a63fc8d7709a42e2cbdc5a136.\n>>\n>> I know. But I cannot reproduce the error.\n>>\n>> Do you have a test case that demonstrates the problem?\n>>\n>> I either want to see the patch upstream in official git or revert\n>> it in msysgit. But I cannot answer the questions that were\n>> raised after I sent the patch (see earlier in this thread). And\n>> I can't see the problem that your patch solves, even after\n>> spending some time on reading and instrumenting code.\n>\n> I remember that the problem was as simple as git clone or git clone\n> --bare failing.\n\nThis is what I understood from the commit message.\n\nI need a script that I can run to see the error. All tests that\ncome with git pass (on my machine).\n\n\n> Also I'm not sure if it matters but I'm running Vista.\n\nIt only matters if you see an error on Vista that I don't see on\nXP. If this was the case I'd debug on Vista.\n\n\n> There is also a chance that code has changed since then and this\n> problem went away.\n\nMaybe.\n\n\tSteffen\n"}]}