From cc7f43ea37987853f46b4b53bf71cbeabee6d859 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 17 Jan 2019 11:06:39 +0100 Subject: [PATCH 01/13] fixup! tests: include detailed trace logs with --write-junit-xml upon failure The `cut -c` approach is *per line*, not *per file* as I had thought. So it does not work... Signed-off-by: Johannes Schindelin --- t/helper/test-path-utils.c | 20 ++++++++++++++++++++ t/test-lib.sh | 14 +++++++------- 2 files changed, 27 insertions(+), 7 deletions(-) diff --git a/t/helper/test-path-utils.c b/t/helper/test-path-utils.c index f4c57750902656..9438e69c592a51 100644 --- a/t/helper/test-path-utils.c +++ b/t/helper/test-path-utils.c @@ -311,6 +311,26 @@ int cmd__path_utils(int argc, const char **argv) return !!res; } + if (argc == 4 && !strcmp(argv[1], "skip-n-bytes")) { + int fd = open(argv[2], O_RDONLY), offset = atoi(argv[3]); + char buffer[65536]; + + if (fd < 0) + die_errno("could not open '%s'", argv[2]); + if (lseek(fd, offset, SEEK_SET) < 0) + die_errno("could not skip %d bytes", offset); + for (;;) { + ssize_t count = read(fd, buffer, sizeof(buffer)); + if (count < 0) + die_errno("could not read '%s'", argv[2]); + if (!count) + break; + write(1, buffer, count); + } + close(fd); + return 0; + } + if (argc > 5 && !strcmp(argv[1], "slice-tests")) { int res = 0; long offset, stride, i; diff --git a/t/test-lib.sh b/t/test-lib.sh index b7912d6ef44c13..fa8d0a6fbb8cdf 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -518,7 +518,8 @@ test_failure_ () { junit_insert="$junit_insert $(xml_attr_encode \ "$(if test -n "$GIT_TEST_TEE_OUTPUT_FILE" then - cut -c "$GIT_TEST_TEE_OFFSET-" <"$GIT_TEST_TEE_OUTPUT_FILE" + test-tool path-utils skip-n-bytes \ + "$GIT_TEST_TEE_OUTPUT_FILE" $GIT_TEST_TEE_OFFSET else printf '%s\n' "$@" | sed 1d fi)")" @@ -823,6 +824,11 @@ test_finish_ () { echo >&3 "" maybe_teardown_valgrind maybe_teardown_verbose + if test -n "$GIT_TEST_TEE_OFFSET" + then + GIT_TEST_TEE_OFFSET=$(test-tool path-utils file-size \ + "$GIT_TEST_TEE_OUTPUT_FILE") + fi } test_skip () { @@ -900,11 +906,6 @@ write_junit_xml_testcase () { write_junit_xml "$(printf '%s\n' \ " " "$@" " ")" junit_have_testcase=t - if test -n "$GIT_TEST_TEE_OUTPUT_FILE" - then - GIT_TEST_TEE_OFFSET=$(test-tool path-utils file-size \ - "$GIT_TEST_TEE_OUTPUT_FILE") - fi } test_done () { @@ -1198,7 +1199,6 @@ then if test -n "$GIT_TEST_TEE_OUTPUT_FILE" then GIT_TEST_TEE_OFFSET=0 - GIT_TEST_TEE_ERR_OFFSET=0 fi fi From acd1aaef1d81f1b365c353568c0f05393b8ed3d2 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 10 Jan 2019 17:27:56 +0100 Subject: [PATCH 02/13] DEBUG: invalidate getenv() calls when the next one is called This is a crude, and incomplete, way to diagnose getenv() issues where the return value is not used transiently. Signed-off-by: Johannes Schindelin --- compat/mingw.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/compat/mingw.c b/compat/mingw.c index e0c02fd9ecc5d1..ef9136c983449b 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -2079,11 +2079,14 @@ char *mingw_getenv(const char *name) len_value = len_value * 3 + 1; /* We cannot use xcalloc() here because that uses getenv() itself */ - value = calloc(len_value, sizeof(char)); + //value = calloc(len_value, sizeof(char)); +value = calloc(len_value < 16 ? 16 : len_value, sizeof(char)); if (!value) die("Out of memory, (tried to allocate %u bytes)", len_value); xwcstoutf(value, w_value, len_value); +/* Be mean and invalidate the previous getenv() result */ +{ int offset = !strcmp("GIT_COMMITTER_EMAIL", name) || !strcmp("GIT_AUTHOR_EMAIL", name) ? 2 : (!strcmp("GIT_COMMITTER_DATE", name) || !strcmp("GIT_AUTHOR_DATE", name) ? 3 : 1); char *p = values[(value_counter + ARRAY_SIZE(values) - offset) % ARRAY_SIZE(values)]; if (p) xsnprintf(p, 16, "NONONONONONONO!"); } /* * We return `value` which is an allocated value and the caller is NOT * expecting to have to free it, so we keep a round-robin array, From 48a2b7dc2e7f8db07daf42cdf9191b0bf4a06988 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 17 Jan 2019 13:33:35 +0100 Subject: [PATCH 03/13] DEBUG2: help diagnosing issues Signed-off-by: Johannes Schindelin --- compat/mingw.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compat/mingw.c b/compat/mingw.c index ef9136c983449b..bd38be97dc0c7f 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -2086,7 +2086,7 @@ value = calloc(len_value < 16 ? 16 : len_value, sizeof(char)); xwcstoutf(value, w_value, len_value); /* Be mean and invalidate the previous getenv() result */ -{ int offset = !strcmp("GIT_COMMITTER_EMAIL", name) || !strcmp("GIT_AUTHOR_EMAIL", name) ? 2 : (!strcmp("GIT_COMMITTER_DATE", name) || !strcmp("GIT_AUTHOR_DATE", name) ? 3 : 1); char *p = values[(value_counter + ARRAY_SIZE(values) - offset) % ARRAY_SIZE(values)]; if (p) xsnprintf(p, 16, "NONONONONONONO!"); } +{ int offset = !strcmp("GIT_COMMITTER_EMAIL", name) || !strcmp("GIT_AUTHOR_EMAIL", name) ? 2 : (!strcmp("GIT_COMMITTER_DATE", name) || !strcmp("GIT_AUTHOR_DATE", name) ? 3 : 1); char *p = values[(value_counter + ARRAY_SIZE(values) - offset) % ARRAY_SIZE(values)]; if (p) { if (0) error("invalidating '%s' because of '%s'", p, name); xsnprintf(p, 16, "NONONONONONONO!"); } } /* * We return `value` which is an allocated value and the caller is NOT * expecting to have to free it, so we keep a round-robin array, From 5a6c20a1a29fdabf72b0937cb6ae765012e6dc7a Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 17 Jan 2019 13:34:09 +0100 Subject: [PATCH 04/13] WORK-AROUND: force `fmt_ident(...)` to succeed Signed-off-by: Johannes Schindelin --- ident.c | 12 ++++++++++++ log-tree.c | 10 ++++++---- sequencer.c | 5 +++-- 3 files changed, 21 insertions(+), 6 deletions(-) diff --git a/ident.c b/ident.c index 33bcf40644cdf2..e914bc59a7207a 100644 --- a/ident.c +++ b/ident.c @@ -436,6 +436,12 @@ const char *git_author_info(int flag) author_ident_explicitly_given |= IDENT_NAME_GIVEN; if (getenv("GIT_AUTHOR_EMAIL")) author_ident_explicitly_given |= IDENT_MAIL_GIVEN; + if (1) { + char *name = xstrdup_or_null(getenv("GIT_AUTHOR_NAME")), *email = xstrdup_or_null(getenv("GIT_AUTHOR_EMAIL")), *date = xstrdup_or_null(getenv("GIT_AUTHOR_DATE")); + const char *res = fmt_ident(name, email, date, flag); + free(name); free(email); free(date); + return res; + } return fmt_ident(getenv("GIT_AUTHOR_NAME"), getenv("GIT_AUTHOR_EMAIL"), getenv("GIT_AUTHOR_DATE"), @@ -448,6 +454,12 @@ const char *git_committer_info(int flag) committer_ident_explicitly_given |= IDENT_NAME_GIVEN; if (getenv("GIT_COMMITTER_EMAIL")) committer_ident_explicitly_given |= IDENT_MAIL_GIVEN; + if (1) { + char *name = xstrdup_or_null(getenv("GIT_COMMITTER_NAME")), *email = xstrdup_or_null(getenv("GIT_COMMITTER_EMAIL")), *date = xstrdup_or_null(getenv("GIT_COMMITTER_DATE")); + const char *res = fmt_ident(name, email, date, flag); + free(name); free(email); free(date); + return res; + } return fmt_ident(getenv("GIT_COMMITTER_NAME"), getenv("GIT_COMMITTER_EMAIL"), getenv("GIT_COMMITTER_DATE"), diff --git a/log-tree.c b/log-tree.c index 10680c139eeb53..d13673a872fac5 100644 --- a/log-tree.c +++ b/log-tree.c @@ -685,10 +685,12 @@ void show_log(struct rev_info *opt) /* * And then the pretty-printed message itself */ - if (ctx.need_8bit_cte >= 0 && opt->add_signoff) - ctx.need_8bit_cte = - has_non_ascii(fmt_name(getenv("GIT_COMMITTER_NAME"), - getenv("GIT_COMMITTER_EMAIL"))); + if (ctx.need_8bit_cte >= 0 && opt->add_signoff) { + char *name = xstrdup_or_null(getenv("GIT_COMMITTER_NAME")), *email = xstrdup_or_null(getenv("GIT_COMMITTER_EMAIL")); + + ctx.need_8bit_cte = has_non_ascii(fmt_name(name, email)); + free(name); free(email); + } ctx.date_mode = opt->date_mode; ctx.date_mode_explicit = opt->date_mode_explicit; ctx.abbrev = opt->diffopt.abbrev; diff --git a/sequencer.c b/sequencer.c index e3883a0ed8b0bf..8260d6f8f74dee 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4034,10 +4034,11 @@ void append_signoff(struct strbuf *msgbuf, size_t ignore_footer, unsigned flag) unsigned no_dup_sob = flag & APPEND_SIGNOFF_DEDUP; struct strbuf sob = STRBUF_INIT; int has_footer; + char *name = xstrdup_or_null(getenv("GIT_COMMITTER_NAME")), *email = xstrdup_or_null(getenv("GIT_COMMITTER_EMAIL")); strbuf_addstr(&sob, sign_off_header); - strbuf_addstr(&sob, fmt_name(getenv("GIT_COMMITTER_NAME"), - getenv("GIT_COMMITTER_EMAIL"))); + strbuf_addstr(&sob, fmt_name(name, email)); + FREE_AND_NULL(name); FREE_AND_NULL(email); strbuf_addch(&sob, '\n'); if (!ignore_footer) From e388cfa4516208c523c25c936a126dcc1fd0e52c Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 10 Jan 2019 17:40:24 +0100 Subject: [PATCH 05/13] DEBUG: only test on Windows Signed-off-by: Johannes Schindelin --- azure-pipelines.yml | 216 -------------------------------------------- 1 file changed, 216 deletions(-) diff --git a/azure-pipelines.yml b/azure-pipelines.yml index 6cd27b3483997f..6fb0caf57d8856 100644 --- a/azure-pipelines.yml +++ b/azure-pipelines.yml @@ -126,219 +126,3 @@ jobs: platform: Windows publishRunAttachments: false condition: succeededOrFailed() - -- job: linux_clang - displayName: linux-clang - condition: succeeded() - pool: Hosted Ubuntu 1604 - steps: - - bash: | - test "$GITFILESHAREPWD" = '$(gitfileshare.pwd)' || ci/mount-fileshare.sh //gitfileshare.file.core.windows.net/test-cache gitfileshare "$GITFILESHAREPWD" "$HOME/test-cache" || exit 1 - - sudo apt-get update && - sudo apt-get -y install git gcc make libssl-dev libcurl4-openssl-dev libexpat-dev tcl tk gettext git-email zlib1g-dev apache2-bin && - - export CC=clang || exit 1 - - ci/install-dependencies.sh || exit 1 - ci/run-build-and-tests.sh || { - ci/print-test-failures.sh - exit 1 - } - - test "$GITFILESHAREPWD" = '$(gitfileshare.pwd)' || sudo umount "$HOME/test-cache" || exit 1 - displayName: 'ci/run-build-and-tests.sh' - env: - GITFILESHAREPWD: $(gitfileshare.pwd) - - task: PublishTestResults@2 - displayName: 'Publish Test Results **/TEST-*.xml' - inputs: - mergeTestResults: true - testRunTitle: 'linux-clang' - platform: Linux - publishRunAttachments: false - condition: succeededOrFailed() - -- job: linux_gcc - displayName: linux-gcc - condition: succeeded() - pool: Hosted Ubuntu 1604 - steps: - - bash: | - test "$GITFILESHAREPWD" = '$(gitfileshare.pwd)' || ci/mount-fileshare.sh //gitfileshare.file.core.windows.net/test-cache gitfileshare "$GITFILESHAREPWD" "$HOME/test-cache" || exit 1 - - sudo add-apt-repository ppa:ubuntu-toolchain-r/test && - sudo apt-get update && - sudo apt-get -y install git gcc make libssl-dev libcurl4-openssl-dev libexpat-dev tcl tk gettext git-email zlib1g-dev apache2 language-pack-is git-svn gcc-8 || exit 1 - - ci/install-dependencies.sh || exit 1 - ci/run-build-and-tests.sh || { - ci/print-test-failures.sh - exit 1 - } - - test "$GITFILESHAREPWD" = '$(gitfileshare.pwd)' || sudo umount "$HOME/test-cache" || exit 1 - displayName: 'ci/run-build-and-tests.sh' - env: - GITFILESHAREPWD: $(gitfileshare.pwd) - - task: PublishTestResults@2 - displayName: 'Publish Test Results **/TEST-*.xml' - inputs: - mergeTestResults: true - testRunTitle: 'linux-gcc' - platform: Linux - publishRunAttachments: false - condition: succeededOrFailed() - -- job: osx_clang - displayName: osx-clang - condition: succeeded() - pool: Hosted macOS - steps: - - bash: | - test "$GITFILESHAREPWD" = '$(gitfileshare.pwd)' || ci/mount-fileshare.sh //gitfileshare.file.core.windows.net/test-cache gitfileshare "$GITFILESHAREPWD" "$HOME/test-cache" || exit 1 - - export CC=clang - - ci/install-dependencies.sh || exit 1 - ci/run-build-and-tests.sh || { - ci/print-test-failures.sh - exit 1 - } - - test "$GITFILESHAREPWD" = '$(gitfileshare.pwd)' || umount "$HOME/test-cache" || exit 1 - displayName: 'ci/run-build-and-tests.sh' - env: - GITFILESHAREPWD: $(gitfileshare.pwd) - - task: PublishTestResults@2 - displayName: 'Publish Test Results **/TEST-*.xml' - inputs: - mergeTestResults: true - testRunTitle: 'osx-clang' - platform: macOS - publishRunAttachments: false - condition: succeededOrFailed() - -- job: osx_gcc - displayName: osx-gcc - condition: succeeded() - pool: Hosted macOS - steps: - - bash: | - test "$GITFILESHAREPWD" = '$(gitfileshare.pwd)' || ci/mount-fileshare.sh //gitfileshare.file.core.windows.net/test-cache gitfileshare "$GITFILESHAREPWD" "$HOME/test-cache" || exit 1 - - ci/install-dependencies.sh || exit 1 - ci/run-build-and-tests.sh || { - ci/print-test-failures.sh - exit 1 - } - - test "$GITFILESHAREPWD" = '$(gitfileshare.pwd)' || umount "$HOME/test-cache" || exit 1 - displayName: 'ci/run-build-and-tests.sh' - env: - GITFILESHAREPWD: $(gitfileshare.pwd) - - task: PublishTestResults@2 - displayName: 'Publish Test Results **/TEST-*.xml' - inputs: - mergeTestResults: true - testRunTitle: 'osx-gcc' - platform: macOS - publishRunAttachments: false - condition: succeededOrFailed() - -- job: gettext_poison - displayName: GETTEXT_POISON - condition: succeeded() - pool: Hosted Ubuntu 1604 - steps: - - bash: | - test "$GITFILESHAREPWD" = '$(gitfileshare.pwd)' || ci/mount-fileshare.sh //gitfileshare.file.core.windows.net/test-cache gitfileshare "$GITFILESHAREPWD" "$HOME/test-cache" || exit 1 - - sudo apt-get update && - sudo apt-get -y install git gcc make libssl-dev libcurl4-openssl-dev libexpat-dev tcl tk gettext git-email zlib1g-dev && - - export jobname=GETTEXT_POISON || exit 1 - - ci/run-build-and-tests.sh || { - ci/print-test-failures.sh - exit 1 - } - - test "$GITFILESHAREPWD" = '$(gitfileshare.pwd)' || sudo umount "$HOME/test-cache" || exit 1 - displayName: 'ci/run-build-and-tests.sh' - env: - GITFILESHAREPWD: $(gitfileshare.pwd) - - task: PublishTestResults@2 - displayName: 'Publish Test Results **/TEST-*.xml' - inputs: - mergeTestResults: true - testRunTitle: 'gettext-poison' - platform: Linux - publishRunAttachments: false - condition: succeededOrFailed() - -- job: linux32 - displayName: Linux32 - condition: succeeded() - pool: Hosted Ubuntu 1604 - steps: - - bash: | - test "$GITFILESHAREPWD" = '$(gitfileshare.pwd)' || ci/mount-fileshare.sh //gitfileshare.file.core.windows.net/test-cache gitfileshare "$GITFILESHAREPWD" "$HOME/test-cache" || exit 1 - - sudo AGENT_OS="$AGENT_OS" BUILD_BUILDNUMBER="$BUILD_BUILDNUMBER" BUILD_REPOSITORY_URI="$BUILD_REPOSITORY_URI" BUILD_SOURCEBRANCH="$BUILD_SOURCEBRANCH" BUILD_SOURCEVERSION="$BUILD_SOURCEVERSION" SYSTEM_PHASENAME="$SYSTEM_PHASENAME" SYSTEM_TASKDEFINITIONSURI="$SYSTEM_TASKDEFINITIONSURI" SYSTEM_TEAMPROJECT="$SYSTEM_TEAMPROJECT" CC=$CC MAKEFLAGS=-j3 bash -lxc ci/run-linux32-docker.sh || exit 1 - - sudo chmod a+r t/out/TEST-*.xml - - test "$GITFILESHAREPWD" = '$(gitfileshare.pwd)' || sudo umount "$HOME/test-cache" || exit 1 - displayName: 'ci/run-linux32-docker.sh' - env: - GITFILESHAREPWD: $(gitfileshare.pwd) - - task: PublishTestResults@2 - displayName: 'Publish Test Results **/TEST-*.xml' - inputs: - mergeTestResults: true - testRunTitle: 'linux32' - platform: Linux - publishRunAttachments: false - condition: succeededOrFailed() - -- job: static_analysis - displayName: StaticAnalysis - condition: succeeded() - pool: Hosted Ubuntu 1604 - steps: - - bash: | - test "$GITFILESHAREPWD" = '$(gitfileshare.pwd)' || ci/mount-fileshare.sh //gitfileshare.file.core.windows.net/test-cache gitfileshare "$GITFILESHAREPWD" "$HOME/test-cache" || exit 1 - - sudo apt-get update && - sudo apt-get install -y coccinelle && - - export jobname=StaticAnalysis && - - ci/run-static-analysis.sh || exit 1 - - test "$GITFILESHAREPWD" = '$(gitfileshare.pwd)' || sudo umount "$HOME/test-cache" || exit 1 - displayName: 'ci/run-static-analysis.sh' - env: - GITFILESHAREPWD: $(gitfileshare.pwd) - -- job: documentation - displayName: Documentation - condition: succeeded() - pool: Hosted Ubuntu 1604 - steps: - - bash: | - test "$GITFILESHAREPWD" = '$(gitfileshare.pwd)' || ci/mount-fileshare.sh //gitfileshare.file.core.windows.net/test-cache gitfileshare "$GITFILESHAREPWD" "$HOME/test-cache" || exit 1 - - sudo apt-get update && - sudo apt-get install -y asciidoc xmlto asciidoctor && - - export ALREADY_HAVE_ASCIIDOCTOR=yes. && - export jobname=Documentation && - - ci/test-documentation.sh || exit 1 - - test "$GITFILESHAREPWD" = '$(gitfileshare.pwd)' || sudo umount "$HOME/test-cache" || exit 1 - displayName: 'ci/test-documentation.sh' - env: - GITFILESHAREPWD: $(gitfileshare.pwd) From ce83ddc80566f60cb5f1ca939d7a287e21ae0248 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Mon, 14 Jan 2019 17:12:54 +0100 Subject: [PATCH 06/13] TODO: left-over bit from env conversion Signed-off-by: Johannes Schindelin --- compat/mingw.c | 1 + 1 file changed, 1 insertion(+) diff --git a/compat/mingw.c b/compat/mingw.c index bd38be97dc0c7f..4600e315e1effd 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -2924,6 +2924,7 @@ static void setup_windows_environment(void) * executable (by not mistaking the dir separators * for escape characters). */ + /* TODO: now that we no longer have a full cached env, we need to set this, right? */ convert_slashes(tmp); } From 0bf3c96559d0c1ae3754e0caa99562f5adf08869 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 11 Jan 2019 17:15:00 -0500 Subject: [PATCH 07/13] get_super_prefix(): copy getenv() result The return value of getenv() is not guaranteed to remain valid across multiple calls (nor across calls to setenv()). Since this function caches the result for the length of the program, we must make a copy to ensure that it is still valid when we need it. Reported-by: Yngve N. Pettersen Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- environment.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/environment.c b/environment.c index 346559770773e9..516df41cb8ab25 100644 --- a/environment.c +++ b/environment.c @@ -107,7 +107,7 @@ char *git_work_tree_cfg; static char *git_namespace; -static const char *super_prefix; +static char *super_prefix; /* * Repository-local GIT_* environment variables; see cache.h for details. @@ -240,7 +240,7 @@ const char *get_super_prefix(void) { static int initialized; if (!initialized) { - super_prefix = getenv(GIT_SUPER_PREFIX_ENVIRONMENT); + super_prefix = xstrdup_or_null(getenv(GIT_SUPER_PREFIX_ENVIRONMENT)); initialized = 1; } return super_prefix; From ce2c4c25949ed4996a91ce2b646d8f0d248d6222 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 11 Jan 2019 17:15:40 -0500 Subject: [PATCH 08/13] commit: copy saved getenv() result We save the result of $GIT_INDEX_FILE so that we can restore it after setting it to a new value and running add--interactive. However, the pointer returned by getenv() is not guaranteed to be valid after calling setenv(). This _usually_ works fine, but can fail if libc needs to reallocate the environment block during the setenv(). Let's just duplicate the string, so we know that it remains valid. In the long run it may be more robust to teach interactive_add() to take a set of environment variables to pass along to run-command when it execs add--interactive. And then we would not have to do this save/restore dance at all. But this is an easy fix in the meantime. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/commit.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/builtin/commit.c b/builtin/commit.c index c74a0948afc2e4..4d4ba2e6d7885a 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -351,7 +351,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix if (write_locked_index(&the_index, &index_lock, 0)) die(_("unable to create temporary index")); - old_index_env = getenv(INDEX_ENVIRONMENT); + old_index_env = xstrdup_or_null(getenv(INDEX_ENVIRONMENT)); setenv(INDEX_ENVIRONMENT, get_lock_file_path(&index_lock), 1); if (interactive_add(argc, argv, prefix, patch_interactive) != 0) @@ -361,6 +361,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix setenv(INDEX_ENVIRONMENT, old_index_env, 1); else unsetenv(INDEX_ENVIRONMENT); + FREE_AND_NULL(old_index_env); discard_cache(); read_cache_from(get_lock_file_path(&index_lock)); From 43652dccc77c69e3f3a352fc4cdd45d15a66245c Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 11 Jan 2019 17:15:54 -0500 Subject: [PATCH 09/13] config: make a copy of $GIT_CONFIG string cmd_config() points our source filename pointer at the return value of getenv(), but that value may be invalidated by further calls to environment functions. Let's copy it to make sure it remains valid. We don't need to bother freeing it, as it remains part of the whole-process global state until we exit. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/config.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/config.c b/builtin/config.c index 84385ef165195e..2db4e763e7ab3c 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -598,7 +598,7 @@ int cmd_config(int argc, const char **argv, const char *prefix) int nongit = !startup_info->have_repository; char *value; - given_config_source.file = getenv(CONFIG_ENVIRONMENT); + given_config_source.file = xstrdup_or_null(getenv(CONFIG_ENVIRONMENT)); argc = parse_options(argc, argv, prefix, builtin_config_options, builtin_config_usage, From 5bb5a8b3900da0bf28dcabab54a9b198ef147ede Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 11 Jan 2019 17:16:31 -0500 Subject: [PATCH 10/13] init: make a copy of $GIT_DIR string We pass the result of getenv("GIT_DIR") to init_db() and assume that the string remains valid. But that's not guaranteed across calls to setenv() or even getenv(), although it often works in practice. Let's make a copy of the string so that we follow the rules. Note that we need to mark it with UNLEAK(), since the value persists until the end of program (but we have no opportunity to free it). This patch also handles $GIT_WORK_TREE the same way. It actually doesn't have as long a lifetime and is probably fine, but it's simpler to just treat the two side-by-side variables the same. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/init-db.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/builtin/init-db.c b/builtin/init-db.c index bcefe4a203aa1e..94df241ad5f24f 100644 --- a/builtin/init-db.c +++ b/builtin/init-db.c @@ -548,8 +548,8 @@ int cmd_init_db(int argc, const char **argv, const char *prefix) * GIT_WORK_TREE makes sense only in conjunction with GIT_DIR * without --bare. Catch the error early. */ - git_dir = getenv(GIT_DIR_ENVIRONMENT); - work_tree = getenv(GIT_WORK_TREE_ENVIRONMENT); + git_dir = xstrdup_or_null(getenv(GIT_DIR_ENVIRONMENT)); + work_tree = xstrdup_or_null(getenv(GIT_WORK_TREE_ENVIRONMENT)); if ((!git_dir || is_bare_repository_cfg == 1) && work_tree) die(_("%s (or --work-tree=) not allowed without " "specifying %s (or --git-dir=)"), @@ -588,6 +588,8 @@ int cmd_init_db(int argc, const char **argv, const char *prefix) } UNLEAK(real_git_dir); + UNLEAK(git_dir); + UNLEAK(work_tree); flags |= INIT_DB_EXIST_OK; return init_db(git_dir, real_git_dir, template_dir, flags); From 69cbeaa1d7c65c10939356fe7338762bda40bd3e Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 11 Jan 2019 17:16:55 -0500 Subject: [PATCH 11/13] merge-recursive: copy $GITHEAD strings If $GITHEAD_1234abcd is set in the environment, we use its value as a "better branch name" in generating conflict markers. However, we pick these better names early in the process, and the return value from getenv() is not guaranteed to stay valid. Let's make a copy of the returned string. And to make memory management easier, let's just always return an allocated string from better_branch_name(), so we know that it must always be freed. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/merge-recursive.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c index 9b2f707c291cf9..7545136c2a6da5 100644 --- a/builtin/merge-recursive.c +++ b/builtin/merge-recursive.c @@ -7,16 +7,16 @@ static const char builtin_merge_recursive_usage[] = "git %s ... -- ..."; -static const char *better_branch_name(const char *branch) +static char *better_branch_name(const char *branch) { static char githead_env[8 + GIT_MAX_HEXSZ + 1]; char *name; if (strlen(branch) != the_hash_algo->hexsz) - return branch; + return xstrdup(branch); xsnprintf(githead_env, sizeof(githead_env), "GITHEAD_%s", branch); name = getenv(githead_env); - return name ? name : branch; + return xstrdup(name ? name : branch); } int cmd_merge_recursive(int argc, const char **argv, const char *prefix) @@ -26,6 +26,7 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix) int i, failed; struct object_id h1, h2; struct merge_options o; + char *better1, *better2; struct commit *result; init_merge_options(&o); @@ -70,13 +71,17 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix) if (get_oid(o.branch2, &h2)) die(_("could not resolve ref '%s'"), o.branch2); - o.branch1 = better_branch_name(o.branch1); - o.branch2 = better_branch_name(o.branch2); + o.branch1 = better1 = better_branch_name(o.branch1); + o.branch2 = better2 = better_branch_name(o.branch2); if (o.verbosity >= 3) printf(_("Merging %s with %s\n"), o.branch1, o.branch2); failed = merge_recursive_generic(&o, &h1, &h2, bases_count, bases, &result); + + free(better1); + free(better2); + if (failed < 0) return 128; /* die() error code */ return failed; From cb6228962600a3182e7f3cc7fbcb7c8e92060959 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 11 Jan 2019 17:17:22 -0500 Subject: [PATCH 12/13] builtin_diff(): read $GIT_DIFF_OPTS closer to use The value returned by getenv() is not guaranteed to remain valid across other environment function calls. But in between our call and using the value, we run fill_textconv(), which may do quite a bit of work, including spawning sub-processes. We can make this safer by calling getenv() right before we actually look at its value. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- diff.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/diff.c b/diff.c index 50d76ddde85bbd..184f7a572cb711 100644 --- a/diff.c +++ b/diff.c @@ -3476,7 +3476,7 @@ static void builtin_diff(const char *name_a, o->found_changes = 1; } else { /* Crazy xdl interfaces.. */ - const char *diffopts = getenv("GIT_DIFF_OPTS"); + const char *diffopts; const char *v; xpparam_t xpp; xdemitconf_t xecfg; @@ -3519,12 +3519,15 @@ static void builtin_diff(const char *name_a, xecfg.flags |= XDL_EMIT_FUNCCONTEXT; if (pe) xdiff_set_find_func(&xecfg, pe->pattern, pe->cflags); + + diffopts = getenv("GIT_DIFF_OPTS"); if (!diffopts) ; else if (skip_prefix(diffopts, "--unified=", &v)) xecfg.ctxlen = strtoul(v, NULL, 10); else if (skip_prefix(diffopts, "-u", &v)) xecfg.ctxlen = strtoul(v, NULL, 10); + if (o->word_diff) init_diff_words_data(&ecbdata, o, one, two); if (xdi_diff_outf(&mf1, &mf2, NULL, fn_out_consume, From 162ee3fba21716dd97c9b7c1adccd5a8a0077fc0 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Mon, 14 Jan 2019 17:13:00 +0100 Subject: [PATCH 13/13] TO-SPLIT Signed-off-by: Johannes Schindelin --- builtin/grep.c | 7 +++++-- builtin/help.c | 3 ++- config.c | 12 ++++-------- connect.c | 15 +++++++-------- editor.c | 2 +- help.c | 3 ++- http.c | 4 ++-- notes-utils.c | 8 ++++++-- notes.c | 11 ++++++++--- pager.c | 3 ++- setup.c | 6 ++++-- 11 files changed, 43 insertions(+), 31 deletions(-) diff --git a/builtin/grep.c b/builtin/grep.c index 71df52a333cbc7..b394360fd382f3 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -962,8 +962,11 @@ int cmd_grep(int argc, const char **argv, const char *prefix) argc--; } - if (show_in_pager == default_pager) - show_in_pager = git_pager(1); + if (show_in_pager == default_pager) { + char *pager = xstrdup_or_null(git_pager(1)); + UNLEAK(pager); + show_in_pager = pager; + } if (show_in_pager) { opt.color = 0; opt.name_only = 1; diff --git a/builtin/help.c b/builtin/help.c index 7739a5c1551426..94edaafef81f94 100644 --- a/builtin/help.c +++ b/builtin/help.c @@ -350,13 +350,14 @@ static void show_man_page(const char *git_cmd) { struct man_viewer_list *viewer; const char *page = cmd_to_page(git_cmd); - const char *fallback = getenv("GIT_MAN_VIEWER"); + const char *fallback; setup_man_path(); for (viewer = man_viewer_list; viewer; viewer = viewer->next) { exec_viewer(viewer->name, page); /* will return when unable */ } + fallback = getenv("GIT_MAN_VIEWER"); if (fallback) exec_viewer(fallback, page); exec_viewer("man", page); diff --git a/config.c b/config.c index f433b15b55c65c..7642929efb5e1e 100644 --- a/config.c +++ b/config.c @@ -444,9 +444,8 @@ int git_config_parse_parameter(const char *text, int git_config_from_parameters(config_fn_t fn, void *data) { - const char *env = getenv(CONFIG_DATA_ENVIRONMENT); + char *env = xstrdup_or_null(getenv(CONFIG_DATA_ENVIRONMENT)); int ret = 0; - char *envw; const char **argv = NULL; int nr = 0, alloc = 0; int i; @@ -460,10 +459,7 @@ int git_config_from_parameters(config_fn_t fn, void *data) source.origin_type = CONFIG_ORIGIN_CMDLINE; cf = &source; - /* sq_dequote will write over it */ - envw = xstrdup(env); - - if (sq_dequote_to_argv(envw, &argv, &nr, &alloc) < 0) { + if (sq_dequote_to_argv(env, &argv, &nr, &alloc) < 0) { ret = error(_("bogus format in %s"), CONFIG_DATA_ENVIRONMENT); goto out; } @@ -477,7 +473,7 @@ int git_config_from_parameters(config_fn_t fn, void *data) out: free(argv); - free(envw); + free(env); cf = source.prev; return ret; } @@ -2290,7 +2286,7 @@ int git_config_get_max_percent_split_change(void) int git_config_get_fsmonitor(void) { if (git_config_get_pathname("core.fsmonitor", &core_fsmonitor)) - core_fsmonitor = getenv("GIT_TEST_FSMONITOR"); + core_fsmonitor = xstrdup_or_null(getenv("GIT_TEST_FSMONITOR")); if (core_fsmonitor && !*core_fsmonitor) core_fsmonitor = NULL; diff --git a/connect.c b/connect.c index d72772a36d01ff..da97dda6ce7f3c 100644 --- a/connect.c +++ b/connect.c @@ -832,7 +832,7 @@ static int git_proxy_command_options(const char *var, const char *value, static int git_use_proxy(const char *host) { - git_proxy_command = getenv("GIT_PROXY_COMMAND"); + git_proxy_command = xstrdup_or_null(getenv("GIT_PROXY_COMMAND")); git_config(git_proxy_command_options, (void*)host); return (git_proxy_command && *git_proxy_command); } @@ -954,12 +954,12 @@ static enum protocol parse_connect_url(const char *url_orig, char **ret_host, static const char *get_ssh_command(void) { - const char *ssh; + static char *ssh; - if ((ssh = getenv("GIT_SSH_COMMAND"))) + if ((ssh = xstrdup_or_null(getenv("GIT_SSH_COMMAND")))) return ssh; - if (!git_config_get_string_const("core.sshcommand", &ssh)) + if (!git_config_get_string("core.sshcommand", &ssh)) return ssh; return NULL; @@ -1060,10 +1060,9 @@ static struct child_process *git_connect_git(int fd[2], char *hostandport, * connect, unless the user has overridden us in * the environment. */ - char *target_host = getenv("GIT_OVERRIDE_VIRTUAL_HOST"); - if (target_host) - target_host = xstrdup(target_host); - else + char *target_host = + xstrdup_or_null(getenv("GIT_OVERRIDE_VIRTUAL_HOST")); + if (!target_host) target_host = xstrdup(hostandport); transport_check_allowed("git"); diff --git a/editor.c b/editor.c index c985eee1f9d1c7..f50ef916d1fd98 100644 --- a/editor.c +++ b/editor.c @@ -16,8 +16,8 @@ int is_terminal_dumb(void) const char *git_editor(void) { - const char *editor = getenv("GIT_EDITOR"); int terminal_is_dumb = is_terminal_dumb(); + const char *editor = getenv("GIT_EDITOR"); if (!editor && editor_program) editor = editor_program; diff --git a/help.c b/help.c index ff05fd22dff064..1a82d2bd2e96ce 100644 --- a/help.c +++ b/help.c @@ -259,7 +259,7 @@ void load_command_list(const char *prefix, struct cmdnames *main_cmds, struct cmdnames *other_cmds) { - const char *env_path = getenv("PATH"); + const char *env_path; const char *exec_path = git_exec_path(); if (exec_path) { @@ -268,6 +268,7 @@ void load_command_list(const char *prefix, uniq(main_cmds); } + env_path = getenv("PATH"); if (env_path) { char *paths, *path, *colon; path = paths = xstrdup(env_path); diff --git a/http.c b/http.c index b8258fc78fddbb..035fbbe725a930 100644 --- a/http.c +++ b/http.c @@ -1052,7 +1052,7 @@ static void set_from_env(const char **var, const char *envname) { const char *val = getenv(envname); if (val) - *var = val; + *var = xstrdup(val); } void http_init(struct remote *remote, const char *url, int proactive_auth) @@ -1119,7 +1119,7 @@ void http_init(struct remote *remote, const char *url, int proactive_auth) #ifdef USE_CURL_MULTI { - char *http_max_requests = getenv("GIT_HTTP_MAX_REQUESTS"); + const char *http_max_requests = getenv("GIT_HTTP_MAX_REQUESTS"); if (http_max_requests != NULL) max_requests = atoi(http_max_requests); } diff --git a/notes-utils.c b/notes-utils.c index 14ea03178e9c44..dabcd71d8450d9 100644 --- a/notes-utils.c +++ b/notes-utils.c @@ -123,8 +123,10 @@ static int notes_rewrite_config(const char *k, const char *v, void *cb) struct notes_rewrite_cfg *init_copy_notes_for_rewrite(const char *cmd) { struct notes_rewrite_cfg *c = xmalloc(sizeof(struct notes_rewrite_cfg)); - const char *rewrite_mode_env = getenv(GIT_NOTES_REWRITE_MODE_ENVIRONMENT); - const char *rewrite_refs_env = getenv(GIT_NOTES_REWRITE_REF_ENVIRONMENT); + char *rewrite_mode_env = + xstrdup_or_null(getenv(GIT_NOTES_REWRITE_MODE_ENVIRONMENT)); + char *rewrite_refs_env = + xstrdup_or_null(getenv(GIT_NOTES_REWRITE_REF_ENVIRONMENT)); c->cmd = cmd; c->enabled = 1; c->combine = combine_notes_concatenate; @@ -143,10 +145,12 @@ struct notes_rewrite_cfg *init_copy_notes_for_rewrite(const char *cmd) */ error(_("Bad %s value: '%s'"), GIT_NOTES_REWRITE_MODE_ENVIRONMENT, rewrite_mode_env); + free(rewrite_mode_env); } if (rewrite_refs_env) { c->refs_from_env = 1; string_list_add_refs_from_colon_sep(c->refs, rewrite_refs_env); + free(rewrite_refs_env); } git_config(notes_rewrite_config, c); if (!c->enabled || !c->refs->nr) { diff --git a/notes.c b/notes.c index 25cdce28b71a3f..37afa31bab8cff 100644 --- a/notes.c +++ b/notes.c @@ -973,8 +973,12 @@ static int notes_display_config(const char *k, const char *v, void *cb) const char *default_notes_ref(void) { const char *notes_ref = NULL; - if (!notes_ref) - notes_ref = getenv(GIT_NOTES_REF_ENVIRONMENT); + if (!notes_ref) { + static char *env; + free(env); + env = xstrdup_or_null(getenv(GIT_NOTES_REF_ENVIRONMENT)); + notes_ref = env; + } if (!notes_ref) notes_ref = notes_ref_name; /* value of core.notesRef config */ if (!notes_ref) @@ -1039,7 +1043,6 @@ struct notes_tree **load_notes_trees(struct string_list *refs, int flags) void init_display_notes(struct display_notes_opt *opt) { - char *display_ref_env; int load_config_refs = 0; display_notes_refs.strdup_strings = 1; @@ -1047,6 +1050,8 @@ void init_display_notes(struct display_notes_opt *opt) if (!opt || opt->use_default_notes > 0 || (opt->use_default_notes == -1 && !opt->extra_notes_refs.nr)) { + const char *display_ref_env; + string_list_append(&display_notes_refs, default_notes_ref()); display_ref_env = getenv(GIT_NOTES_DISPLAY_REF_ENVIRONMENT); if (display_ref_env) { diff --git a/pager.c b/pager.c index a768797fcfcc44..043b39a2c8084f 100644 --- a/pager.c +++ b/pager.c @@ -104,7 +104,7 @@ void prepare_pager_args(struct child_process *pager_process, const char *pager) void setup_pager(void) { - const char *pager = git_pager(isatty(1)); + char *pager = xstrdup_or_null(git_pager(isatty(1))); if (!pager) return; @@ -124,6 +124,7 @@ void setup_pager(void) /* spawn the pager */ prepare_pager_args(&pager_process, pager); + FREE_AND_NULL(pager); pager_process.in = -1; argv_array_push(&pager_process.env_array, "GIT_PAGER_IN_USE"); if (start_command(&pager_process)) diff --git a/setup.c b/setup.c index 7589360e52849f..ad7f62a4cb0144 100644 --- a/setup.c +++ b/setup.c @@ -654,7 +654,7 @@ static const char *setup_explicit_git_dir(const char *gitdirenv, struct repository_format *repo_fmt, int *nongit_ok) { - const char *work_tree_env = getenv(GIT_WORK_TREE_ENVIRONMENT); + const char *work_tree_env; const char *worktree; char *gitfile; int offset; @@ -683,6 +683,7 @@ static const char *setup_explicit_git_dir(const char *gitdirenv, } /* #3, #7, #11, #15, #19, #23, #27, #31 (see t1510) */ + work_tree_env = getenv(GIT_WORK_TREE_ENVIRONMENT); if (work_tree_env) set_git_work_tree(work_tree_env); else if (is_bare_repository_cfg > 0) { @@ -913,7 +914,7 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir, struct strbuf *gitdir, int die_on_error) { - const char *env_ceiling_dirs = getenv(CEILING_DIRECTORIES_ENVIRONMENT); + const char *env_ceiling_dirs; struct string_list ceiling_dirs = STRING_LIST_INIT_DUP; const char *gitdirenv; int ceil_offset = -1, min_offset = offset_1st_component(dir->buf); @@ -931,6 +932,7 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir, return GIT_DIR_EXPLICIT; } + env_ceiling_dirs = getenv(CEILING_DIRECTORIES_ENVIRONMENT); if (env_ceiling_dirs) { int empty_entry_found = 0;