{"thread":{"id":"50204","subject":"[PATCH 0/6] getenv() timing fixes","startedAt":"2019-01-11T22:14:18Z","lastAt":"2019-01-16T14:07:09Z","messageCount":26,"participants":["Jeff King","Junio C Hamano","Ævar Arnfjörð Bjarmason","Stefan Beller","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"366577","messageId":"20190111221414.GA31335@sigill.intra.peff.net","threadId":"50204","inReplyTo":null,"subject":"[PATCH 0/6] getenv() timing fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-11T22:14:14Z","receivedAt":"2019-01-11T22:14:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Similar to the recent:\n\n https://public-inbox.org/git/20190109221007.21624-1-kgybels@infogroep.be/\n\nthere are some other places where we do not follow the POSIX rule that\ngetenv()'s return value may be invalidated by other calls to getenv() or\nsetenv().\n\nFor the most part we haven't noticed because:\n\n - on many platforms, you can call getenv() as many times as you want.\n This changed recently in our mingw_getenv() helper, which is why\n people are noticing now.\n\n - calling setenv() in between _often_ works, but it depends on whether\n libc feels like it needs to reallocate memory. Which is itself\n platform specific, and even on a single platform may depend on\n things like how many environment variables you have set.\n\nThe first patch here is a problem somebody actually found in the wild.\nThat led me to start looking through the results of:\n\n git grep '= getenv('\n\nThere are a ton of hits. I poked at the first 20 or so. A lot of them\nare fine, as they do something like this:\n\n rla = getenv(\"GIT_REFLOG_ACTION\");\n strbuf_addstr(\"blah blah %s\", rla);\n\nThat's not _strictly_ correct, because strbuf_addstr() may actually look\nat the environment. But it works for our mingw_getenv() case, because\nthere we use a rotating series of buffers. So as long as it doesn't look at\n30 environment variables, we're fine. And many calls fall into that\nbucket (a more complicated one is get_ssh_command(), which runs a fair\nbit of code while holding the pointer, but ultimately probably has a\nsmall fixed number of opportunities to call getenv(). What is more\nworrisome is code that holds a pointer across an arbitrary number of\ncalls (like once per diff'd file, or once per submodule, etc).\n\nOf course it's possible for some platform libc to use a single buffer.\nBut in that case, I'd argue that the path of least resistance is\nwrapping getenv, like I described in:\n\n https://public-inbox.org/git/20181025062037.GC11460@sigill.intra.peff.net/\n\nSo anyway. Here are a handful of what seem like pretty low-hanging\nfruit. Beyond the first one, I'm not sure if they're triggerable, but\nthey're easy to fix. There are 100+ grep matches that I _didn't_ audit,\nso this is by no means a complete fix. I was mostly trying to get a\nsense of how painful these fixes would be.\n\n [1/6]: get_super_prefix(): copy getenv() result\n [2/6]: commit: copy saved getenv() result\n [3/6]: config: make a copy of $GIT_CONFIG string\n [4/6]: init: make a copy of $GIT_DIR string\n [5/6]: merge-recursive: copy $GITHEAD strings\n [6/6]: builtin_diff(): read $GIT_DIFF_OPTS closer to use\n\n builtin/commit.c | 3 ++-\n builtin/config.c | 2 +-\n builtin/init-db.c | 6 ++++--\n builtin/merge-recursive.c | 15 ++++++++++-----\n diff.c | 5 ++++-\n environment.c | 4 ++--\n 6 files changed, 23 insertions(+), 12 deletions(-)\n\n-Peff\n"},{"id":"366578","messageId":"20190111221500.GA10188@sigill.intra.peff.net","threadId":"50204","inReplyTo":"20190111221414.GA31335@sigill.intra.peff.net","subject":"[PATCH 1/6] get_super_prefix(): copy getenv() result","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-11T22:15:00Z","receivedAt":"2019-01-11T22:15:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The return value of getenv() is not guaranteed to remain valid across\nmultiple calls (nor across calls to setenv()). Since this function\ncaches the result for the length of the program, we must make a copy to\nensure that it is still valid when we need it.\n\nReported-by: Yngve N. Pettersen \nSigned-off-by: Jeff King \n---\n environment.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/environment.c b/environment.c\nindex 0e37741d83..89af47cb85 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -107,7 +107,7 @@ char *git_work_tree_cfg;\n \n static char *git_namespace;\n \n-static const char *super_prefix;\n+static char *super_prefix;\n \n /*\n * Repository-local GIT_* environment variables; see cache.h for details.\n@@ -240,7 +240,7 @@ const char *get_super_prefix(void)\n {\n \tstatic int initialized;\n \tif (!initialized) {\n-\t\tsuper_prefix = getenv(GIT_SUPER_PREFIX_ENVIRONMENT);\n+\t\tsuper_prefix = xstrdup_or_null(getenv(GIT_SUPER_PREFIX_ENVIRONMENT));\n \t\tinitialized = 1;\n \t}\n \treturn super_prefix;\n-- \n2.20.1.651.g2d41a78c67\n\n"},{"id":"366579","messageId":"20190111221539.GB10188@sigill.intra.peff.net","threadId":"50204","inReplyTo":"20190111221414.GA31335@sigill.intra.peff.net","subject":"[PATCH 2/6] commit: copy saved getenv() result","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-11T22:15:40Z","receivedAt":"2019-01-11T22:15:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We save the result of $GIT_INDEX_FILE so that we can restore it after\nsetting it to a new value and running add--interactive. However, the\npointer returned by getenv() is not guaranteed to be valid after calling\nsetenv(). This _usually_ works fine, but can fail if libc needs to\nreallocate the environment block during the setenv().\n\nLet's just duplicate the string, so we know that it remains valid.\n\nIn the long run it may be more robust to teach interactive_add() to take\na set of environment variables to pass along to run-command when it\nexecs add--interactive. And then we would not have to do this\nsave/restore dance at all. But this is an easy fix in the meantime.\n\nSigned-off-by: Jeff King \n---\n builtin/commit.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 004b816635..7d2e0b61e5 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -351,7 +351,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \t\tif (write_locked_index(&the_index, &index_lock, 0))\n \t\t\tdie(_(\"unable to create temporary index\"));\n \n-\t\told_index_env = getenv(INDEX_ENVIRONMENT);\n+\t\told_index_env = xstrdup_or_null(getenv(INDEX_ENVIRONMENT));\n \t\tsetenv(INDEX_ENVIRONMENT, get_lock_file_path(&index_lock), 1);\n \n \t\tif (interactive_add(argc, argv, prefix, patch_interactive) != 0)\n@@ -361,6 +361,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \t\t\tsetenv(INDEX_ENVIRONMENT, old_index_env, 1);\n \t\telse\n \t\t\tunsetenv(INDEX_ENVIRONMENT);\n+\t\tFREE_AND_NULL(old_index_env);\n \n \t\tdiscard_cache();\n \t\tread_cache_from(get_lock_file_path(&index_lock));\n-- \n2.20.1.651.g2d41a78c67\n\n"},{"id":"366580","messageId":"20190111221554.GC10188@sigill.intra.peff.net","threadId":"50204","inReplyTo":"20190111221414.GA31335@sigill.intra.peff.net","subject":"[PATCH 3/6] config: make a copy of $GIT_CONFIG string","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-11T22:15:54Z","receivedAt":"2019-01-11T22:15:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"cmd_config() points our source filename pointer at the return value of\ngetenv(), but that value may be invalidated by further calls to\nenvironment functions. Let's copy it to make sure it remains valid.\n\nWe don't need to bother freeing it, as it remains part of the\nwhole-process global state until we exit.\n\nSigned-off-by: Jeff King \n---\n builtin/config.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 84385ef165..2db4e763e7 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -598,7 +598,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \tint nongit = !startup_info->have_repository;\n \tchar *value;\n \n-\tgiven_config_source.file = getenv(CONFIG_ENVIRONMENT);\n+\tgiven_config_source.file = xstrdup_or_null(getenv(CONFIG_ENVIRONMENT));\n \n \targc = parse_options(argc, argv, prefix, builtin_config_options,\n \t\t\t builtin_config_usage,\n-- \n2.20.1.651.g2d41a78c67\n\n"},{"id":"366581","messageId":"20190111221631.GD10188@sigill.intra.peff.net","threadId":"50204","inReplyTo":"20190111221414.GA31335@sigill.intra.peff.net","subject":"[PATCH 4/6] init: make a copy of $GIT_DIR string","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-11T22:16:31Z","receivedAt":"2019-01-11T22:16:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We pass the result of getenv(\"GIT_DIR\") to init_db() and assume that the\nstring remains valid. But that's not guaranteed across calls to setenv()\nor even getenv(), although it often works in practice. Let's make a copy\nof the string so that we follow the rules.\n\nNote that we need to mark it with UNLEAK(), since the value persists\nuntil the end of program (but we have no opportunity to free it).\n\nThis patch also handles $GIT_WORK_TREE the same way. It actually doesn't\nhave as long a lifetime and is probably fine, but it's simpler to just\ntreat the two side-by-side variables the same.\n\nSigned-off-by: Jeff King \n---\n builtin/init-db.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/init-db.c b/builtin/init-db.c\nindex 41faffd28d..93eff7618c 100644\n--- a/builtin/init-db.c\n+++ b/builtin/init-db.c\n@@ -542,8 +542,8 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)\n \t * GIT_WORK_TREE makes sense only in conjunction with GIT_DIR\n \t * without --bare. Catch the error early.\n \t */\n-\tgit_dir = getenv(GIT_DIR_ENVIRONMENT);\n-\twork_tree = getenv(GIT_WORK_TREE_ENVIRONMENT);\n+\tgit_dir = xstrdup_or_null(getenv(GIT_DIR_ENVIRONMENT));\n+\twork_tree = xstrdup_or_null(getenv(GIT_WORK_TREE_ENVIRONMENT));\n \tif ((!git_dir || is_bare_repository_cfg == 1) && work_tree)\n \t\tdie(_(\"%s (or --work-tree=) not allowed without \"\n \t\t\t \"specifying %s (or --git-dir=)\"),\n@@ -582,6 +582,8 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)\n \t}\n \n \tUNLEAK(real_git_dir);\n+\tUNLEAK(git_dir);\n+\tUNLEAK(work_tree);\n \n \tflags |= INIT_DB_EXIST_OK;\n \treturn init_db(git_dir, real_git_dir, template_dir, flags);\n-- \n2.20.1.651.g2d41a78c67\n\n"},{"id":"366582","messageId":"20190111221655.GE10188@sigill.intra.peff.net","threadId":"50204","inReplyTo":"20190111221414.GA31335@sigill.intra.peff.net","subject":"[PATCH 5/6] merge-recursive: copy $GITHEAD strings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-11T22:16:55Z","receivedAt":"2019-01-11T22:16:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If $GITHEAD_1234abcd is set in the environment, we use its value as a\n\"better branch name\" in generating conflict markers. However, we pick\nthese better names early in the process, and the return value from\ngetenv() is not guaranteed to stay valid.\n\nLet's make a copy of the returned string. And to make memory management\neasier, let's just always return an allocated string from\nbetter_branch_name(), so we know that it must always be freed.\n\nSigned-off-by: Jeff King \n---\n builtin/merge-recursive.c | 15 ++++++++++-----\n 1 file changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c\nindex 9b2f707c29..7545136c2a 100644\n--- a/builtin/merge-recursive.c\n+++ b/builtin/merge-recursive.c\n@@ -7,16 +7,16 @@\n static const char builtin_merge_recursive_usage[] =\n \t\"git %s ... -- ...\";\n \n-static const char *better_branch_name(const char *branch)\n+static char *better_branch_name(const char *branch)\n {\n \tstatic char githead_env[8 + GIT_MAX_HEXSZ + 1];\n \tchar *name;\n \n \tif (strlen(branch) != the_hash_algo->hexsz)\n-\t\treturn branch;\n+\t\treturn xstrdup(branch);\n \txsnprintf(githead_env, sizeof(githead_env), \"GITHEAD_%s\", branch);\n \tname = getenv(githead_env);\n-\treturn name ? name : branch;\n+\treturn xstrdup(name ? name : branch);\n }\n \n int cmd_merge_recursive(int argc, const char **argv, const char *prefix)\n@@ -26,6 +26,7 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix)\n \tint i, failed;\n \tstruct object_id h1, h2;\n \tstruct merge_options o;\n+\tchar *better1, *better2;\n \tstruct commit *result;\n \n \tinit_merge_options(&o);\n@@ -70,13 +71,17 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix)\n \tif (get_oid(o.branch2, &h2))\n \t\tdie(_(\"could not resolve ref '%s'\"), o.branch2);\n \n-\to.branch1 = better_branch_name(o.branch1);\n-\to.branch2 = better_branch_name(o.branch2);\n+\to.branch1 = better1 = better_branch_name(o.branch1);\n+\to.branch2 = better2 = better_branch_name(o.branch2);\n \n \tif (o.verbosity >= 3)\n \t\tprintf(_(\"Merging %s with %s\\n\"), o.branch1, o.branch2);\n \n \tfailed = merge_recursive_generic(&o, &h1, &h2, bases_count, bases, &result);\n+\n+\tfree(better1);\n+\tfree(better2);\n+\n \tif (failed < 0)\n \t\treturn 128; /* die() error code */\n \treturn failed;\n-- \n2.20.1.651.g2d41a78c67\n\n"},{"id":"366583","messageId":"20190111221722.GF10188@sigill.intra.peff.net","threadId":"50204","inReplyTo":"20190111221414.GA31335@sigill.intra.peff.net","subject":"[PATCH 6/6] builtin_diff(): read $GIT_DIFF_OPTS closer to use","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-11T22:17:22Z","receivedAt":"2019-01-11T22:17:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The value returned by getenv() is not guaranteed to remain valid across\nother environment function calls. But in between our call and using the\nvalue, we run fill_textconv(), which may do quite a bit of work,\nincluding spawning sub-processes.\n\nWe can make this safer by calling getenv() right before we actually look\nat its value.\n\nSigned-off-by: Jeff King \n---\n diff.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex 15556c190d..6751ec29f0 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3476,7 +3476,7 @@ static void builtin_diff(const char *name_a,\n \t\to->found_changes = 1;\n \t} else {\n \t\t/* Crazy xdl interfaces.. */\n-\t\tconst char *diffopts = getenv(\"GIT_DIFF_OPTS\");\n+\t\tconst char *diffopts;\n \t\tconst char *v;\n \t\txpparam_t xpp;\n \t\txdemitconf_t xecfg;\n@@ -3519,12 +3519,15 @@ static void builtin_diff(const char *name_a,\n \t\t\txecfg.flags |= XDL_EMIT_FUNCCONTEXT;\n \t\tif (pe)\n \t\t\txdiff_set_find_func(&xecfg, pe->pattern, pe->cflags);\n+\n+\t\tdiffopts = getenv(\"GIT_DIFF_OPTS\");\n \t\tif (!diffopts)\n \t\t\t;\n \t\telse if (skip_prefix(diffopts, \"--unified=\", &v))\n \t\t\txecfg.ctxlen = strtoul(v, NULL, 10);\n \t\telse if (skip_prefix(diffopts, \"-u\", &v))\n \t\t\txecfg.ctxlen = strtoul(v, NULL, 10);\n+\n \t\tif (o->word_diff)\n \t\t\tinit_diff_words_data(&ecbdata, o, one, two);\n \t\tif (xdi_diff_outf(&mf1, &mf2, NULL, fn_out_consume,\n-- \n2.20.1.651.g2d41a78c67\n"},{"id":"366610","messageId":"xmqqwonawpbd.fsf@gitster-ct.c.googlers.com","threadId":"50204","inReplyTo":"20190111221500.GA10188@sigill.intra.peff.net","subject":"Re: [PATCH 1/6] get_super_prefix(): copy getenv() result","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-12T03:02:46Z","receivedAt":"2019-01-12T03:02:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The return value of getenv() is not guaranteed to remain valid across\n> multiple calls (nor across calls to setenv()). Since this function\n> caches the result for the length of the program, we must make a copy to\n> ensure that it is still valid when we need it.\n>\n> Reported-by: Yngve N. Pettersen \n> Signed-off-by: Jeff King \n> ---\n\nMakes sense. Thanks.\n"},{"id":"366611","messageId":"xmqqsgxywp3w.fsf@gitster-ct.c.googlers.com","threadId":"50204","inReplyTo":"20190111221539.GB10188@sigill.intra.peff.net","subject":"Re: [PATCH 2/6] commit: copy saved getenv() result","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-12T03:07:15Z","receivedAt":"2019-01-12T03:07:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> We save the result of $GIT_INDEX_FILE so that we can restore it after\n> setting it to a new value and running add--interactive. However, the\n> pointer returned by getenv() is not guaranteed to be valid after calling\n> setenv(). This _usually_ works fine, but can fail if libc needs to\n> reallocate the environment block during the setenv().\n>\n> Let's just duplicate the string, so we know that it remains valid.\n>\n> In the long run it may be more robust to teach interactive_add() to take\n> a set of environment variables to pass along to run-command when it\n> execs add--interactive. And then we would not have to do this\n> save/restore dance at all. But this is an easy fix in the meantime.\n>\n> Signed-off-by: Jeff King \n> ---\n> builtin/commit.c | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 004b816635..7d2e0b61e5 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -351,7 +351,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n> \t\tif (write_locked_index(&the_index, &index_lock, 0))\n> \t\t\tdie(_(\"unable to create temporary index\"));\n> \n> -\t\told_index_env = getenv(INDEX_ENVIRONMENT);\n> +\t\told_index_env = xstrdup_or_null(getenv(INDEX_ENVIRONMENT));\n> \t\tsetenv(INDEX_ENVIRONMENT, get_lock_file_path(&index_lock), 1);\n> \n> \t\tif (interactive_add(argc, argv, prefix, patch_interactive) != 0)\n> @@ -361,6 +361,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n> \t\t\tsetenv(INDEX_ENVIRONMENT, old_index_env, 1);\n> \t\telse\n> \t\t\tunsetenv(INDEX_ENVIRONMENT);\n> +\t\tFREE_AND_NULL(old_index_env);\n> \n> \t\tdiscard_cache();\n> \t\tread_cache_from(get_lock_file_path(&index_lock));\n\nEven though it is not wrong per-se to assign a NULL to the\nnow-no-longer-referenced variable, I do not quite get why it is\nfree-and-null, not a straight free. This may be a taste-thing,\nthough.\n\nEven if a future update needs to make it possible to access\nold_index_env somewhere in the block after discard_cache() gets\ncalled, we would need to push down the free (or free-and-null) to\nprolong its lifetime a bit anyway, so...\n\n\n"},{"id":"366612","messageId":"xmqqo98mwp2d.fsf@gitster-ct.c.googlers.com","threadId":"50204","inReplyTo":"20190111221631.GD10188@sigill.intra.peff.net","subject":"Re: [PATCH 4/6] init: make a copy of $GIT_DIR string","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-12T03:08:10Z","receivedAt":"2019-01-12T03:08:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> We pass the result of getenv(\"GIT_DIR\") to init_db() and assume that the\n> string remains valid. But that's not guaranteed across calls to setenv()\n> or even getenv(), although it often works in practice. Let's make a copy\n> of the string so that we follow the rules.\n>\n> Note that we need to mark it with UNLEAK(), since the value persists\n> until the end of program (but we have no opportunity to free it).\n\nMakes sense. Thanks.\n"},{"id":"366613","messageId":"xmqqk1jawoyl.fsf@gitster-ct.c.googlers.com","threadId":"50204","inReplyTo":"20190111221655.GE10188@sigill.intra.peff.net","subject":"Re: [PATCH 5/6] merge-recursive: copy $GITHEAD strings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-12T03:10:26Z","receivedAt":"2019-01-12T03:10:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> If $GITHEAD_1234abcd is set in the environment, we use its value as a\n> \"better branch name\" in generating conflict markers. However, we pick\n> these better names early in the process, and the return value from\n> getenv() is not guaranteed to stay valid.\n>\n> Let's make a copy of the returned string. And to make memory management\n> easier, let's just always return an allocated string from\n> better_branch_name(), so we know that it must always be freed.\n>\n> Signed-off-by: Jeff King \n> ---\n> builtin/merge-recursive.c | 15 ++++++++++-----\n> 1 file changed, 10 insertions(+), 5 deletions(-)\n\nThanks. Will queue.\n\n>\n> diff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c\n> index 9b2f707c29..7545136c2a 100644\n> --- a/builtin/merge-recursive.c\n> +++ b/builtin/merge-recursive.c\n> @@ -7,16 +7,16 @@\n> static const char builtin_merge_recursive_usage[] =\n> \t\"git %s ... -- ...\";\n> \n> -static const char *better_branch_name(const char *branch)\n> +static char *better_branch_name(const char *branch)\n> {\n> \tstatic char githead_env[8 + GIT_MAX_HEXSZ + 1];\n> \tchar *name;\n> \n> \tif (strlen(branch) != the_hash_algo->hexsz)\n> -\t\treturn branch;\n> +\t\treturn xstrdup(branch);\n> \txsnprintf(githead_env, sizeof(githead_env), \"GITHEAD_%s\", branch);\n> \tname = getenv(githead_env);\n> -\treturn name ? name : branch;\n> +\treturn xstrdup(name ? name : branch);\n> }\n> \n> int cmd_merge_recursive(int argc, const char **argv, const char *prefix)\n> @@ -26,6 +26,7 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix)\n> \tint i, failed;\n> \tstruct object_id h1, h2;\n> \tstruct merge_options o;\n> +\tchar *better1, *better2;\n> \tstruct commit *result;\n> \n> \tinit_merge_options(&o);\n> @@ -70,13 +71,17 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix)\n> \tif (get_oid(o.branch2, &h2))\n> \t\tdie(_(\"could not resolve ref '%s'\"), o.branch2);\n> \n> -\to.branch1 = better_branch_name(o.branch1);\n> -\to.branch2 = better_branch_name(o.branch2);\n> +\to.branch1 = better1 = better_branch_name(o.branch1);\n> +\to.branch2 = better2 = better_branch_name(o.branch2);\n> \n> \tif (o.verbosity >= 3)\n> \t\tprintf(_(\"Merging %s with %s\\n\"), o.branch1, o.branch2);\n> \n> \tfailed = merge_recursive_generic(&o, &h1, &h2, bases_count, bases, &result);\n> +\n> +\tfree(better1);\n> +\tfree(better2);\n> +\n> \tif (failed < 0)\n> \t\treturn 128; /* die() error code */\n> \treturn failed;\n"},{"id":"366620","messageId":"20190112102635.GA16633@sigill.intra.peff.net","threadId":"50204","inReplyTo":"xmqqsgxywp3w.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/6] commit: copy saved getenv() result","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-12T10:26:35Z","receivedAt":"2019-01-12T10:26:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 11, 2019 at 07:07:15PM -0800, Junio C Hamano wrote:\n\n> > diff --git a/builtin/commit.c b/builtin/commit.c\n> > index 004b816635..7d2e0b61e5 100644\n> > --- a/builtin/commit.c\n> > +++ b/builtin/commit.c\n> > @@ -351,7 +351,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n> > \t\tif (write_locked_index(&the_index, &index_lock, 0))\n> > \t\t\tdie(_(\"unable to create temporary index\"));\n> > \n> > -\t\told_index_env = getenv(INDEX_ENVIRONMENT);\n> > +\t\told_index_env = xstrdup_or_null(getenv(INDEX_ENVIRONMENT));\n> > \t\tsetenv(INDEX_ENVIRONMENT, get_lock_file_path(&index_lock), 1);\n> > \n> > \t\tif (interactive_add(argc, argv, prefix, patch_interactive) != 0)\n> > @@ -361,6 +361,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n> > \t\t\tsetenv(INDEX_ENVIRONMENT, old_index_env, 1);\n> > \t\telse\n> > \t\t\tunsetenv(INDEX_ENVIRONMENT);\n> > +\t\tFREE_AND_NULL(old_index_env);\n> > \n> > \t\tdiscard_cache();\n> > \t\tread_cache_from(get_lock_file_path(&index_lock));\n> \n> Even though it is not wrong per-se to assign a NULL to the\n> now-no-longer-referenced variable, I do not quite get why it is\n> free-and-null, not a straight free. This may be a taste-thing,\n> though.\n> \n> Even if a future update needs to make it possible to access\n> old_index_env somewhere in the block after discard_cache() gets\n> called, we would need to push down the free (or free-and-null) to\n> prolong its lifetime a bit anyway, so...\n\nMy thinking was that if we simply call free(), then the variable is left\nas a dangling pointer for the rest of the function, making it easy to\naccidentally use-after-free.\n\nBut certainly it would not be the first such instance in our code base.\nIn theory a static analyzer should easily be able to figure out such a\nproblem, too, so maybe it is not worth being defensive about.\n\n-Peff\n"},{"id":"366622","messageId":"87va2u3yeu.fsf@evledraar.gmail.com","threadId":"50204","inReplyTo":"20190111221414.GA31335@sigill.intra.peff.net","subject":"Re: [PATCH 0/6] getenv() timing fixes","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-01-12T11:31:21Z","receivedAt":"2019-01-12T11:39:32Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Jan 11 2019, Jeff King wrote:\n\n> Similar to the recent:\n>\n> https://public-inbox.org/git/20190109221007.21624-1-kgybels@infogroep.be/\n>\n> there are some other places where we do not follow the POSIX rule that\n> getenv()'s return value may be invalidated by other calls to getenv() or\n> setenv().\n>\n> For the most part we haven't noticed because:\n>\n> - on many platforms, you can call getenv() as many times as you want.\n> This changed recently in our mingw_getenv() helper, which is why\n> people are noticing now.\n>\n> - calling setenv() in between _often_ works, but it depends on whether\n> libc feels like it needs to reallocate memory. Which is itself\n> platform specific, and even on a single platform may depend on\n> things like how many environment variables you have set.\n>\n> The first patch here is a problem somebody actually found in the wild.\n> That led me to start looking through the results of:\n>\n> git grep '= getenv('\n>\n> There are a ton of hits. I poked at the first 20 or so. A lot of them\n> are fine, as they do something like this:\n>\n> rla = getenv(\"GIT_REFLOG_ACTION\");\n> strbuf_addstr(\"blah blah %s\", rla);\n>\n> That's not _strictly_ correct, because strbuf_addstr() may actually look\n> at the environment. But it works for our mingw_getenv() case, because\n> there we use a rotating series of buffers. So as long as it doesn't look at\n> 30 environment variables, we're fine. And many calls fall into that\n> bucket (a more complicated one is get_ssh_command(), which runs a fair\n> bit of code while holding the pointer, but ultimately probably has a\n> small fixed number of opportunities to call getenv(). What is more\n> worrisome is code that holds a pointer across an arbitrary number of\n> calls (like once per diff'd file, or once per submodule, etc).\n>\n> Of course it's possible for some platform libc to use a single buffer.\n> But in that case, I'd argue that the path of least resistance is\n> wrapping getenv, like I described in:\n>\n> https://public-inbox.org/git/20181025062037.GC11460@sigill.intra.peff.net/\n>\n> So anyway. Here are a handful of what seem like pretty low-hanging\n> fruit. Beyond the first one, I'm not sure if they're triggerable, but\n> they're easy to fix. There are 100+ grep matches that I _didn't_ audit,\n> so this is by no means a complete fix. I was mostly trying to get a\n> sense of how painful these fixes would be.\n\nI wonder, and not as \"you should do this\" feedback on this series, just\non future development, whether we shouldn't just make our own getenv()\nwrapper for the majority of the GIT_* variables. The semantics would be\nfetch value X, and if it's ever requested again return the value we\nfound the first time.\n\nFor some things we rely on getenv(X) -> setenv(X) -> getenv(X) returning\ndifferent values of X, e.g. in passing along config, but for\ne.g. GIT_TEST_* variables we just want to check them once, and have our\nown ad-hoc caches (via static variables) in a couple of places.\n\nMaybe such an API would just loop over \"environ\" on startup, looking for\nany GIT_* variables, i.e. called from the setup.c code.\n"},{"id":"366623","messageId":"CAGZ79kZrcC=SBrBR_4JDWu4Odgz-Uf7LrusiKNe6tgs02JeAMA@mail.gmail.com","threadId":"50204","inReplyTo":"87va2u3yeu.fsf@evledraar.gmail.com","subject":"Re: [PATCH 0/6] getenv() timing fixes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2019-01-12T18:51:42Z","receivedAt":"2019-01-12T18:51:57Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> I wonder, and not as \"you should do this\" feedback on this series, just\n\nThere is a getenv_safe() in environment.c, but I guess a xgetenv() that\ntakes the same parameters as getenv() is better for ease of use.\n"},{"id":"366711","messageId":"nycvar.QRO.7.76.6.1901151502130.41@tvgsbejvaqbjf.bet","threadId":"50204","inReplyTo":"20190112102635.GA16633@sigill.intra.peff.net","subject":"Re: [PATCH 2/6] commit: copy saved getenv() result","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-01-15T14:05:50Z","receivedAt":"2019-01-15T14:06:17Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Sat, 12 Jan 2019, Jeff King wrote:\n\n> On Fri, Jan 11, 2019 at 07:07:15PM -0800, Junio C Hamano wrote:\n> \n> > > diff --git a/builtin/commit.c b/builtin/commit.c\n> > > index 004b816635..7d2e0b61e5 100644\n> > > --- a/builtin/commit.c\n> > > +++ b/builtin/commit.c\n> > > @@ -351,7 +351,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n> > > \t\tif (write_locked_index(&the_index, &index_lock, 0))\n> > > \t\t\tdie(_(\"unable to create temporary index\"));\n> > > \n> > > -\t\told_index_env = getenv(INDEX_ENVIRONMENT);\n> > > +\t\told_index_env = xstrdup_or_null(getenv(INDEX_ENVIRONMENT));\n> > > \t\tsetenv(INDEX_ENVIRONMENT, get_lock_file_path(&index_lock), 1);\n> > > \n> > > \t\tif (interactive_add(argc, argv, prefix, patch_interactive) != 0)\n> > > @@ -361,6 +361,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n> > > \t\t\tsetenv(INDEX_ENVIRONMENT, old_index_env, 1);\n> > > \t\telse\n> > > \t\t\tunsetenv(INDEX_ENVIRONMENT);\n> > > +\t\tFREE_AND_NULL(old_index_env);\n> > > \n> > > \t\tdiscard_cache();\n> > > \t\tread_cache_from(get_lock_file_path(&index_lock));\n> > \n> > Even though it is not wrong per-se to assign a NULL to the\n> > now-no-longer-referenced variable, I do not quite get why it is\n> > free-and-null, not a straight free. This may be a taste-thing,\n> > though.\n> > \n> > Even if a future update needs to make it possible to access\n> > old_index_env somewhere in the block after discard_cache() gets\n> > called, we would need to push down the free (or free-and-null) to\n> > prolong its lifetime a bit anyway, so...\n> \n> My thinking was that if we simply call free(), then the variable is left\n> as a dangling pointer for the rest of the function, making it easy to\n> accidentally use-after-free.\n\nFWIW I thought that was your reasoning (and did not think of asking you\nabout it) and totally agree with it.\n\nIt is *too* easy not to realize that the `free()` call needs to be moved,\nbut a segmentation fault is a very strong indicator that it should be\nmoved.\n\n> But certainly it would not be the first such instance in our code base.\n\nJust because a lot of our code has grown historically does not mean that\nwe need to add code that shares the same shortcomings. FREE_AND_NULL() was\nnot available for a long time, after all, so it is understandable that we\ndid not use it back then. But it is available now, so we no longer have an\nexcuse to add less defensive code.\n\n> In theory a static analyzer should easily be able to figure out such a\n> problem, too, so maybe it is not worth being defensive about.\n\nHow often do you run a static analyzer?\n\nMy point being: if we can prevent future mistakes easily, and it does not\nadd too much code churn, why not just do it. No need to rely on fancy\nstuff that might not even be available on your preferred platform.\n\nThanks,\nDscho\n"},{"id":"366736","messageId":"20190115191235.GB4886@sigill.intra.peff.net","threadId":"50204","inReplyTo":"87va2u3yeu.fsf@evledraar.gmail.com","subject":"Re: [PATCH 0/6] getenv() timing fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-15T19:12:35Z","receivedAt":"2019-01-15T19:12:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jan 12, 2019 at 12:31:21PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> > So anyway. Here are a handful of what seem like pretty low-hanging\n> > fruit. Beyond the first one, I'm not sure if they're triggerable, but\n> > they're easy to fix. There are 100+ grep matches that I _didn't_ audit,\n> > so this is by no means a complete fix. I was mostly trying to get a\n> > sense of how painful these fixes would be.\n> \n> I wonder, and not as \"you should do this\" feedback on this series, just\n> on future development, whether we shouldn't just make our own getenv()\n> wrapper for the majority of the GIT_* variables. The semantics would be\n> fetch value X, and if it's ever requested again return the value we\n> found the first time.\n\nYeah, that thought certainly crossed my mind while looking into this.\nBut as you noted below, you do sometimes have to worry about\ninvalidating that cache. The most general solution is that you'd hook\nsetenv(), too. At which point you've basically just constructed a shadow\nenvironment that has less-crappy semantics than what POSIX guarantees. ;)\n\nAnother option is to just have a getenv_safe() that always returns an\nallocated string. That's less efficient in some cases, but probably not\nmeaningfully so (you probably shouldn't be calling getenv() in a tight\nloop anyway). It does mean dealing with memory ownership, though, which\nis awkward in some cases (e.g., see git_editor).\n\nMostly I'm worried about making a system that's opaque or easy for\npeople to get wrong (e.g., if our getenv() wrapper quietly caches things\nbut setenv() does not invalidate that cache, that's a recipe for\nconfusion).\n\n> Maybe such an API would just loop over \"environ\" on startup, looking for\n> any GIT_* variables, i.e. called from the setup.c code.\n\nI think whatever we do could just lazy-load. There's no particular\ninitialization we have to do at the beginning of the program.\n\n-Peff\n"},{"id":"366737","messageId":"20190115191359.GC4886@sigill.intra.peff.net","threadId":"50204","inReplyTo":"CAGZ79kZrcC=SBrBR_4JDWu4Odgz-Uf7LrusiKNe6tgs02JeAMA@mail.gmail.com","subject":"Re: [PATCH 0/6] getenv() timing fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-15T19:13:59Z","receivedAt":"2019-01-15T19:14:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jan 12, 2019 at 10:51:42AM -0800, Stefan Beller wrote:\n\n> > I wonder, and not as \"you should do this\" feedback on this series, just\n> \n> There is a getenv_safe() in environment.c, but I guess a xgetenv() that\n> takes the same parameters as getenv() is better for ease of use.\n\nYes, but it punts on the memory ownership by stuffing everything into an\nargv_array. That saves a few lines if you're going to ask for five\nvariables, but for a single variable it's no better than:\n\n char *foo = getenv_safe(\"FOO\");\n\n ...use foo...\n\n free(foo);\n\n-Peff\n"},{"id":"366738","messageId":"20190115191714.GD4886@sigill.intra.peff.net","threadId":"50204","inReplyTo":"nycvar.QRO.7.76.6.1901151502130.41@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 2/6] commit: copy saved getenv() result","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-15T19:17:14Z","receivedAt":"2019-01-15T19:17:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 15, 2019 at 03:05:50PM +0100, Johannes Schindelin wrote:\n\n> > But certainly it would not be the first such instance in our code base.\n> \n> Just because a lot of our code has grown historically does not mean that\n> we need to add code that shares the same shortcomings. FREE_AND_NULL() was\n> not available for a long time, after all, so it is understandable that we\n> did not use it back then. But it is available now, so we no longer have an\n> excuse to add less defensive code.\n\nFair enough. I am happy to start using FREE_AND_NULL() consistently if\nnobody things it's too opaque (or that it creates confusion that we\nsomehow expect to look at the variable again and need it to be NULL). I\nthink the compiler should generally be able to optimize out the NULL\nassignment in most cases anyway.\n\n> > In theory a static analyzer should easily be able to figure out such a\n> > problem, too, so maybe it is not worth being defensive about.\n> \n> How often do you run a static analyzer?\n\nStefan was routinely running coverity, though I haven't seen results in\na while. I think we should make sure that continues, as it did turn up\nsome useful results (and a lot of cruft, too, but on the whole I have\nfound it useful).\n\nI also count gcc as a static analyzer, since it can and does point out\nmany simple problems. I don't know that it can point out an obvious\nuser-after-free like this, though.\n\nThat said....\n\n> My point being: if we can prevent future mistakes easily, and it does not\n> add too much code churn, why not just do it. No need to rely on fancy\n> stuff that might not even be available on your preferred platform.\n\nYeah, I do agree (which is why I included it in the patch ;) ).\n\n-Peff\n"},{"id":"366741","messageId":"CAGZ79kYMiy=j8z2Y3XA03OD07jeUEXs3frNpjvyAFguVxeoBow@mail.gmail.com","threadId":"50204","inReplyTo":"20190115191714.GD4886@sigill.intra.peff.net","subject":"Re: [PATCH 2/6] commit: copy saved getenv() result","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2019-01-15T19:25:45Z","receivedAt":"2019-01-15T19:25:59Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> Stefan was routinely running coverity, though I haven't seen results in\n> a while. I think we should make sure that continues, as it did turn up\n> some useful results (and a lot of cruft, too, but on the whole I have\n> found it useful).\n\ncoverity had some outage (end of last year?-ish) and then changed the\nway it dealt with automated uploads IIRC.\nI have not looked into redoing the automation again since then.\n\nSince 7th of Jan, they seem to have issues with hosting (for everyone\nor just the open source projects?)\nhttps://community.synopsys.com/s/article/Coverity-Scan-Update\n\nFor reference, the script that used to work is at\nhttps://github.com/stefanbeller/git/commit/039be8078bb0379db271135e0c0d7315c34fe243\n(which is on the `coverity` branch of that repo)\n"},{"id":"366743","messageId":"20190115193209.GF4886@sigill.intra.peff.net","threadId":"50204","inReplyTo":"CAGZ79kYMiy=j8z2Y3XA03OD07jeUEXs3frNpjvyAFguVxeoBow@mail.gmail.com","subject":"Re: [PATCH 2/6] commit: copy saved getenv() result","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-15T19:32:09Z","receivedAt":"2019-01-15T19:32:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 15, 2019 at 11:25:45AM -0800, Stefan Beller wrote:\n\n> > Stefan was routinely running coverity, though I haven't seen results in\n> > a while. I think we should make sure that continues, as it did turn up\n> > some useful results (and a lot of cruft, too, but on the whole I have\n> > found it useful).\n> \n> coverity had some outage (end of last year?-ish) and then changed the\n> way it dealt with automated uploads IIRC.\n> I have not looked into redoing the automation again since then.\n> \n> Since 7th of Jan, they seem to have issues with hosting (for everyone\n> or just the open source projects?)\n> https://community.synopsys.com/s/article/Coverity-Scan-Update\n\nYuck. :(\n\n> For reference, the script that used to work is at\n> https://github.com/stefanbeller/git/commit/039be8078bb0379db271135e0c0d7315c34fe243\n> (which is on the `coverity` branch of that repo)\n\nThanks for the update. I may take a look at trying to make this work\nagain at some point (but I won't be sad if somebody else gets to it\nfirst).\n\n-Peff\n"},{"id":"366744","messageId":"xmqqy37lra1j.fsf@gitster-ct.c.googlers.com","threadId":"50204","inReplyTo":"20190115191359.GC4886@sigill.intra.peff.net","subject":"Re: [PATCH 0/6] getenv() timing fixes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-15T19:32:56Z","receivedAt":"2019-01-15T19:33:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Sat, Jan 12, 2019 at 10:51:42AM -0800, Stefan Beller wrote:\n>\n>> > I wonder, and not as \"you should do this\" feedback on this series, just\n>> \n>> There is a getenv_safe() in environment.c, but I guess a xgetenv() that\n>> takes the same parameters as getenv() is better for ease of use.\n>\n> Yes, but it punts on the memory ownership by stuffing everything into an\n> argv_array. That saves a few lines if you're going to ask for five\n> variables, but for a single variable it's no better than:\n>\n> char *foo = getenv_safe(\"FOO\");\n\nYou meant xstrdup_or_null(getenv(\"FOO\")) here? And did Stefan mean\n\n\t#define xgetenv(e) xstrdup_or_null(getenv(e))\n\n?\n\n> ...use foo...\n>\n> free(foo);\n"},{"id":"366746","messageId":"CAGZ79kbbhdFCPbEEZzwmti0zTDCG429Moa-T77DNmCx07svM2A@mail.gmail.com","threadId":"50204","inReplyTo":"xmqqy37lra1j.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 0/6] getenv() timing fixes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2019-01-15T19:38:44Z","receivedAt":"2019-01-15T19:38:58Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Jan 15, 2019 at 11:32 AM Junio C Hamano wrote:\n>\n> Jeff King writes:\n>\n> > On Sat, Jan 12, 2019 at 10:51:42AM -0800, Stefan Beller wrote:\n> >\n> >> > I wonder, and not as \"you should do this\" feedback on this series, just\n> >>\n> >> There is a getenv_safe() in environment.c, but I guess a xgetenv() that\n> >> takes the same parameters as getenv() is better for ease of use.\n> >\n> > Yes, but it punts on the memory ownership by stuffing everything into an\n> > argv_array. That saves a few lines if you're going to ask for five\n> > variables, but for a single variable it's no better than:\n> >\n> > char *foo = getenv_safe(\"FOO\");\n>\n> You meant xstrdup_or_null(getenv(\"FOO\")) here? And did Stefan mean\n>\n> #define xgetenv(e) xstrdup_or_null(getenv(e))\n>\n> ?\n\nAssume I did. (I thought of it as a function effectively\nadding the xstrdup_or_null)\n\nIf we go further into assuming the usage patterns of\nthese xgetenv calls, we might throw in an UNLEAK\nas well, but that might be over board.\n"},{"id":"366752","messageId":"20190115194142.GG4886@sigill.intra.peff.net","threadId":"50204","inReplyTo":"xmqqy37lra1j.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 0/6] getenv() timing fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-15T19:41:43Z","receivedAt":"2019-01-15T19:41:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 15, 2019 at 11:32:56AM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > On Sat, Jan 12, 2019 at 10:51:42AM -0800, Stefan Beller wrote:\n> >\n> >> > I wonder, and not as \"you should do this\" feedback on this series, just\n> >> \n> >> There is a getenv_safe() in environment.c, but I guess a xgetenv() that\n> >> takes the same parameters as getenv() is better for ease of use.\n> >\n> > Yes, but it punts on the memory ownership by stuffing everything into an\n> > argv_array. That saves a few lines if you're going to ask for five\n> > variables, but for a single variable it's no better than:\n> >\n> > char *foo = getenv_safe(\"FOO\");\n> \n> You meant xstrdup_or_null(getenv(\"FOO\")) here? And did Stefan mean\n> \n> \t#define xgetenv(e) xstrdup_or_null(getenv(e))\n> \n> ?\n\nYes, I think that would be one possible implementation of a \"safe\"\ngetenv (and what I was thinking of specifically in that example).\n\nThe more involved one (that doesn't pass along memory ownership) is\nsomething like:\n\n static struct hashmap env_cache;\n\n const char *getenv_safe(const char *name)\n {\n\n\tif (e = hashmap_get(&env_cache, name))\n\t\treturn e->value;\n\n /* need some trickery to make sure xstrdup does not call getenv */\n\te->value = xstrdup_or_null(getenv(name));\n\te->name = xstrdup(name);\n\thashmap_put(&env_cache, e);\n\n\treturn e->value;\n }\n\nwith a matching setenv_safe() to drop the hashmap entry. Come to think\nof it, this is really pretty equivalent to string-interning, which we\nalready have a hashmap for. I think one could argue that string\ninterning is basically just a controlled form of memory leaking, but\nit's probably a reasonable compromise in this instance (i.e., we expect\nto ask about a finite number of variables anyway; the important thing is\njust that we don't leak memory for the same variable over and over).\n\n-Peff\n"},{"id":"366754","messageId":"20190115194726.GA5818@sigill.intra.peff.net","threadId":"50204","inReplyTo":"20190115194142.GG4886@sigill.intra.peff.net","subject":"Re: [PATCH 0/6] getenv() timing fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-15T19:47:26Z","receivedAt":"2019-01-15T19:47:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 15, 2019 at 02:41:42PM -0500, Jeff King wrote:\n\n> The more involved one (that doesn't pass along memory ownership) is\n> something like:\n> \n> static struct hashmap env_cache;\n> \n> const char *getenv_safe(const char *name)\n> {\n> \n> \tif (e = hashmap_get(&env_cache, name))\n> \t\treturn e->value;\n> \n> /* need some trickery to make sure xstrdup does not call getenv */\n> \te->value = xstrdup_or_null(getenv(name));\n> \te->name = xstrdup(name);\n> \thashmap_put(&env_cache, e);\n> \n> \treturn e->value;\n> }\n> \n> with a matching setenv_safe() to drop the hashmap entry. Come to think\n> of it, this is really pretty equivalent to string-interning, which we\n> already have a hashmap for. I think one could argue that string\n> interning is basically just a controlled form of memory leaking, but\n> it's probably a reasonable compromise in this instance (i.e., we expect\n> to ask about a finite number of variables anyway; the important thing is\n> just that we don't leak memory for the same variable over and over).\n\nSo actually, that's pretty easy to do without writing much code at all.\nSomething like:\n\n #define xgetenv(name) strintern(getenv(name))\n\nIt means we're effectively storing the environment twice in the worst\ncase, but that's probably not a big deal. Unless we have a loop which\ndoes repeated setenv()/getenv() calls, the strintern hashmap can't grow\nwithout bound.\n\n-Peff\n"},{"id":"366765","messageId":"xmqqef9dr6hf.fsf@gitster-ct.c.googlers.com","threadId":"50204","inReplyTo":"20190115194726.GA5818@sigill.intra.peff.net","subject":"Re: [PATCH 0/6] getenv() timing fixes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-15T20:49:48Z","receivedAt":"2019-01-15T20:49:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So actually, that's pretty easy to do without writing much code at all.\n> Something like:\n>\n> #define xgetenv(name) strintern(getenv(name))\n>\n> It means we're effectively storing the environment twice in the worst\n> case, but that's probably not a big deal. Unless we have a loop which\n> does repeated setenv()/getenv() calls, the strintern hashmap can't grow\n> without bound.\n\nMakes sense.\n\n\n\n hashmap.h | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/hashmap.h b/hashmap.h\nindex d375d9cce7..cff77f9890 100644\n--- a/hashmap.h\n+++ b/hashmap.h\n@@ -432,6 +432,8 @@ static inline void hashmap_enable_item_counting(struct hashmap *map)\n extern const void *memintern(const void *data, size_t len);\n static inline const char *strintern(const char *string)\n {\n+\tif (!string)\n+\t\treturn string;\n \treturn memintern(string, strlen(string));\n }\n \n"},{"id":"366852","messageId":"nycvar.QRO.7.76.6.1901161506180.41@tvgsbejvaqbjf.bet","threadId":"50204","inReplyTo":"20190115193209.GF4886@sigill.intra.peff.net","subject":"Re: [PATCH 2/6] commit: copy saved getenv() result","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-01-16T14:06:43Z","receivedAt":"2019-01-16T14:07:09Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Tue, 15 Jan 2019, Jeff King wrote:\n\n> On Tue, Jan 15, 2019 at 11:25:45AM -0800, Stefan Beller wrote:\n> \n> > > Stefan was routinely running coverity, though I haven't seen results in\n> > > a while. I think we should make sure that continues, as it did turn up\n> > > some useful results (and a lot of cruft, too, but on the whole I have\n> > > found it useful).\n> > \n> > coverity had some outage (end of last year?-ish) and then changed the\n> > way it dealt with automated uploads IIRC.\n> > I have not looked into redoing the automation again since then.\n> > \n> > Since 7th of Jan, they seem to have issues with hosting (for everyone\n> > or just the open source projects?)\n> > https://community.synopsys.com/s/article/Coverity-Scan-Update\n> \n> Yuck. :(\n\nWhat a timely coincidence to corroborate my argument, eh?\n\nCiao,\nDscho\n"}]}