From fde2b4b9bce53445c84155fddeb65e3777f6938f Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 8 Aug 2023 14:14:36 -0400 Subject: [PATCH 01/12] send-email: drop FakeTerm hack Back in 280242d1cc (send-email: do not barf when Term::ReadLine does not like your terminal, 2006-07-02), we added a fallback for when Term::ReadLine's constructor failed: we'd have a FakeTerm object instead, which would then die if anybody actually tried to call readline() on it. Since we instantiated the $term variable at program startup, we needed this workaround to let the program run in modes when we did not prompt the user. But later, in f4dc9432fd (send-email: lazily load modules for a big speedup, 2021-05-28), we started loading Term::ReadLine lazily only when ask() is called. So at that point we know we're trying to prompt the user, and we can just die if ReadLine instantiation fails, rather than making this fake object to lazily delay showing the error. This should be OK even if there is no tty (e.g., we're in a cron job), because Term::ReadLine will return a stub object in that case whose "IN" and "OUT" functions return undef. And since 5906f54e47 (send-email: don't attempt to prompt if tty is closed, 2009-03-31), we check for that case and skip prompting. And we can be sure that FakeTerm was not kicking in for such a situation, because it has actually been broken since that commit! It does not define "IN" or "OUT" methods, so perl would barf with an error. If FakeTerm was in use, we were neither honoring what 5906f54e47 tried to do, nor producing the readable message that 280242d1cc intended. So we're better off just dropping FakeTerm entirely, and letting the error reported by constructing Term::ReadLine through. [jc: cherry-picked from v2.42.0-rc2~6^2~1] Signed-off-by: Jeff King Acked-by: Taylor Blau Signed-off-by: Junio C Hamano --- git-send-email.perl | 22 ++-------------------- 1 file changed, 2 insertions(+), 20 deletions(-) diff --git a/git-send-email.perl b/git-send-email.perl index 5861e99a6e..72d876f0a0 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -26,18 +26,6 @@ use Git::I18N; Getopt::Long::Configure qw/ pass_through /; -package FakeTerm; -sub new { - my ($class, $reason) = @_; - return bless \$reason, shift; -} -sub readline { - my $self = shift; - die "Cannot use readline on FakeTerm: $$self"; -} -package main; - - sub usage { print <] @@ -930,16 +918,10 @@ EOT3 } sub term { - my $term = eval { - require Term::ReadLine; - $ENV{"GIT_SEND_EMAIL_NOTTY"} + require Term::ReadLine; + return $ENV{"GIT_SEND_EMAIL_NOTTY"} ? Term::ReadLine->new('git-send-email', \*STDIN, \*STDOUT) : Term::ReadLine->new('git-send-email'); - }; - if ($@) { - $term = FakeTerm->new("$@: going non-interactive"); - } - return $term; } sub ask { From d11c51eec8a77e7f9af214b0d0c0e1d1ff951883 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 8 Aug 2023 14:15:31 -0400 Subject: [PATCH 02/12] send-email: avoid creating more than one Term::ReadLine object Every time git-send-email calls its ask() function to prompt the user, we call term(), which instantiates a new Term::ReadLine object. But in v1.46 of Term::ReadLine::Gnu (which provides the Term::ReadLine interface on some platforms), its constructor refuses to create a second instance[1]. So on systems with that version of the module, most git-send-email instances will fail (as we usually prompt for both "to" and "in-reply-to" unless the user provided them on the command line). We can fix this by keeping a single instance variable and returning it for each call to term(). In perl 5.10 and up, we could do that with a "state" variable. But since we only require 5.008, we'll do it the old-fashioned way, with a lexical "my" in its own scope. Note that the tests in t9001 detect this problem as-is, since the failure mode is for the program to die. But let's also beef up the "Prompting works" test to check that it correctly handles multiple inputs (if we had chosen to keep our FakeTerm hack in the previous commit, then the failure mode would be incorrectly ignoring prompts after the first). [1] For discussion of why multiple instances are forbidden, see: https://github.com/hirooih/perl-trg/issues/16 [jc: cherry-picked from v2.42.0-rc2~6^2] Signed-off-by: Jeff King Acked-by: Taylor Blau Signed-off-by: Junio C Hamano --- git-send-email.perl | 18 +++++++++++++----- t/t9001-send-email.sh | 5 +++-- 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/git-send-email.perl b/git-send-email.perl index 72d876f0a0..ad51508790 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -917,11 +917,19 @@ EOT3 do_edit(@files); } -sub term { - require Term::ReadLine; - return $ENV{"GIT_SEND_EMAIL_NOTTY"} - ? Term::ReadLine->new('git-send-email', \*STDIN, \*STDOUT) - : Term::ReadLine->new('git-send-email'); +{ + # Only instantiate one $term per program run, since some + # Term::ReadLine providers refuse to create a second instance. + my $term; + sub term { + require Term::ReadLine; + if (!defined $term) { + $term = $ENV{"GIT_SEND_EMAIL_NOTTY"} + ? Term::ReadLine->new('git-send-email', \*STDIN, \*STDOUT) + : Term::ReadLine->new('git-send-email'); + } + return $term; + } } sub ask { diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh index 1130ef21b3..0f08a9542b 100755 --- a/t/t9001-send-email.sh +++ b/t/t9001-send-email.sh @@ -337,13 +337,14 @@ test_expect_success $PREREQ 'Show all headers' ' test_expect_success $PREREQ 'Prompting works' ' clean_fake_sendmail && (echo "to@example.com" && - echo "" + echo "my-message-id@example.com" ) | GIT_SEND_EMAIL_NOTTY=1 git send-email \ --smtp-server="$(pwd)/fake.sendmail" \ $patches \ 2>errors && grep "^From: A U Thor \$" msgtxt1 && - grep "^To: to@example.com\$" msgtxt1 + grep "^To: to@example.com\$" msgtxt1 && + grep "^In-Reply-To: " msgtxt1 ' test_expect_success $PREREQ,AUTOIDENT 'implicit ident is allowed' ' From b6b9fafacb2e420f8e38ddf808e410382fdec932 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 9 May 2024 12:23:05 -0400 Subject: [PATCH 03/12] ci: drop mention of BREW_INSTALL_PACKAGES variable The last user of this variable went away in 4a6e4b9602 (CI: remove Travis CI support, 2021-11-23), so it's doing nothing except making it more confusing to find out which packages _are_ installed. [jc: cherry-picked from v2.45.0-1-g9d4453e8d6] Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- ci/install-dependencies.sh | 2 -- 1 file changed, 2 deletions(-) diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh index 4f407530d3..33039d516b 100755 --- a/ci/install-dependencies.sh +++ b/ci/install-dependencies.sh @@ -34,8 +34,6 @@ macos-*) export HOMEBREW_NO_AUTO_UPDATE=1 HOMEBREW_NO_INSTALL_CLEANUP=1 # Uncomment this if you want to run perf tests: # brew install gnu-time - test -z "$BREW_INSTALL_PACKAGES" || - brew install $BREW_INSTALL_PACKAGES brew link --force gettext mkdir -p $HOME/bin ( From 93ec0a7cbfceefaa0a618181998100b2884e84c6 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 9 May 2024 12:24:15 -0400 Subject: [PATCH 04/12] ci: avoid bare "gcc" for osx-gcc job On macOS, a bare "gcc" (without a version) will invoke a wrapper for clang, not actual gcc. Even when gcc is installed via homebrew, that only provides version-specific links in /usr/local/bin (like "gcc-13"), and never a version-agnostic "gcc" wrapper. As far as I can tell, this has been the case for a long time, and this osx-gcc job has largely been doing nothing. We can point it at "gcc-13", which will pick up the homebrew-installed version. The fix here is specific to the github workflow file, as the gitlab one does not have a matching job. It's a little unfortunate that we cannot just ask for the latest version of gcc which homebrew provides, but as far as I can tell there is no easy alias (you'd have to find the highest number gcc-* in /usr/local/bin yourself). [jc: cherry-picked from v2.45.0-2-g11c7001e3d] Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- .github/workflows/main.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 2dc0221f7f..583e7cd5f0 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -264,7 +264,7 @@ jobs: cc: clang pool: macos-13 - jobname: osx-gcc - cc: gcc + cc: gcc-13 cc_package: gcc-13 pool: macos-13 - jobname: linux-gcc-default From d17d18f85a2d41a307d48c54b9da942cca530c1e Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 9 May 2024 12:25:44 -0400 Subject: [PATCH 05/12] ci: stop installing "gcc-13" for osx-gcc Our osx-gcc job explicitly asks to install gcc-13. But since the GitHub runner image already comes with gcc-13 installed, this is mostly doing nothing (or in some cases it may install an incremental update over the runner image). But worse, it recently started causing errors like: ==> Fetching gcc@13 ==> Downloading https://ghcr.io/v2/homebrew/core/gcc/13/blobs/sha256:fb2403d97e2ce67eb441b54557cfb61980830f3ba26d4c5a1fe5ecd0c9730d1a ==> Pouring gcc@13--13.2.0.ventura.bottle.tar.gz Error: The `brew link` step did not complete successfully The formula built, but is not symlinked into /usr/local Could not symlink bin/c++-13 Target /usr/local/bin/c++-13 is a symlink belonging to gcc. You can unlink it: brew unlink gcc which cause the whole CI job to bail. I didn't track down the root cause, but I suspect it may be related to homebrew recently switching the "gcc" default to gcc-14. And it may even be fixed when a new runner image is released. But if we don't need to run brew at all, it's one less thing for us to worry about. [jc: cherry-picked from v2.45.0-3-g7df2405b38] Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- .github/workflows/main.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 583e7cd5f0..76e3f1e768 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -265,7 +265,6 @@ jobs: pool: macos-13 - jobname: osx-gcc cc: gcc-13 - cc_package: gcc-13 pool: macos-13 - jobname: linux-gcc-default cc: gcc From ee052533bb81aca6f94c0961222bed2a19789dae Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Mon, 20 May 2024 20:22:00 +0000 Subject: [PATCH 06/12] hook: plug a new memory leak In 8db1e8743c0 (clone: prevent hooks from running during a clone, 2024-03-28), I introduced an inadvertent memory leak that was unfortunately not caught before v2.45.1 was released. Here is a fix. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- hook.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/hook.c b/hook.c index 632b537b99..fc974cee1d 100644 --- a/hook.c +++ b/hook.c @@ -18,8 +18,10 @@ static int identical_to_template_hook(const char *name, const char *path) found_template_hook = access(template_path.buf, X_OK) >= 0; } #endif - if (!found_template_hook) + if (!found_template_hook) { + strbuf_release(&template_path); return 0; + } ret = do_files_match(template_path.buf, path); From 197a772c48652d94ddb340fc6bcd8ed4440ff233 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Mon, 20 May 2024 20:22:01 +0000 Subject: [PATCH 07/12] init: use the correct path of the templates directory again In df93e407f06 (init: refactor the template directory discovery into its own function, 2024-03-29), I refactored the way the templates directory is discovered. The refactoring was faithful, but missed a reference in the `Makefile` where the `DEFAULT_GIT_TEMPLATE_DIR` constant is defined. As a consequence, Git v2.45.1 and friends will always use the hard-coded path `/usr/share/git-core/templates`. Let's fix that by defining the `DEFAULT_GIT_TEMPLATE_DIR` when building `setup.o`, where that constant is actually used. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 093829ae28..4b1502ba2c 100644 --- a/Makefile +++ b/Makefile @@ -2751,7 +2751,7 @@ exec-cmd.sp exec-cmd.s exec-cmd.o: EXTRA_CPPFLAGS = \ '-DFALLBACK_RUNTIME_PREFIX="$(prefix_SQ)"' builtin/init-db.sp builtin/init-db.s builtin/init-db.o: GIT-PREFIX -builtin/init-db.sp builtin/init-db.s builtin/init-db.o: EXTRA_CPPFLAGS = \ +setup.sp setup.s setup.o: EXTRA_CPPFLAGS = \ -DDEFAULT_GIT_TEMPLATE_DIR='"$(template_dir_SQ)"' config.sp config.s config.o: GIT-PREFIX From 75631a3cd84887657c634a35d1095f4a0884e48a Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Mon, 20 May 2024 20:22:02 +0000 Subject: [PATCH 08/12] Revert "core.hooksPath: add some protection while cloning" This defense-in-depth was intended to protect the clone operation against future escalations where bugs in `git clone` would allow attackers to write arbitrary files in the `.git/` directory would allow for Remote Code Execution attacks via maliciously-placed hooks. However, it turns out that the `core.hooksPath` protection has unintentional side effects so severe that they do not justify the benefit of the protections. For example, it has been reported in https://lore.kernel.org/git/FAFA34CB-9732-4A0A-87FB-BDB272E6AEE8@alchemists.io/ that the following invocation, which is intended to make `git clone` safer, is itself broken by that protective measure: git clone --config core.hooksPath=/dev/null Since it turns out that the benefit does not justify the cost, let's revert 20f3588efc6 (core.hooksPath: add some protection while cloning, 2024-03-30). Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- config.c | 13 +------------ t/t1800-hook.sh | 15 --------------- 2 files changed, 1 insertion(+), 27 deletions(-) diff --git a/config.c b/config.c index 85b37f2ee0..8c1c4071f0 100644 --- a/config.c +++ b/config.c @@ -1525,19 +1525,8 @@ static int git_default_core_config(const char *var, const char *value, void *cb) if (!strcmp(var, "core.attributesfile")) return git_config_pathname(&git_attributes_file, var, value); - if (!strcmp(var, "core.hookspath")) { - if (current_config_scope() == CONFIG_SCOPE_LOCAL && - git_env_bool("GIT_CLONE_PROTECTION_ACTIVE", 0)) - die(_("active `core.hooksPath` found in the local " - "repository config:\n\t%s\nFor security " - "reasons, this is disallowed by default.\nIf " - "this is intentional and the hook should " - "actually be run, please\nrun the command " - "again with " - "`GIT_CLONE_PROTECTION_ACTIVE=false`"), - value); + if (!strcmp(var, "core.hookspath")) return git_config_pathname(&git_hooks_path, var, value); - } if (!strcmp(var, "core.bare")) { is_bare_repository_cfg = git_config_bool(var, value); diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh index 7ee12e6f48..2ef3579fa7 100755 --- a/t/t1800-hook.sh +++ b/t/t1800-hook.sh @@ -177,19 +177,4 @@ test_expect_success 'git hook run a hook with a bad shebang' ' test_cmp expect actual ' -test_expect_success 'clone protections' ' - test_config core.hooksPath "$(pwd)/my-hooks" && - mkdir -p my-hooks && - write_script my-hooks/test-hook <<-\EOF && - echo Hook ran $1 - EOF - - git hook run test-hook 2>err && - grep "Hook ran" err && - test_must_fail env GIT_CLONE_PROTECTION_ACTIVE=true \ - git hook run test-hook 2>err && - grep "active .core.hooksPath" err && - ! grep "Hook ran" err -' - test_done From c8f64781c8b3d44ecb57d14fbffcdbf063583812 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Mon, 20 May 2024 20:22:03 +0000 Subject: [PATCH 09/12] tests: verify that `clone -c core.hooksPath=/dev/null` works again As part of the protections added in Git v2.45.1 and friends, repository-local `core.hooksPath` settings are no longer allowed, as a defense-in-depth mechanism to prevent future Git vulnerabilities to raise to critical level if those vulnerabilities inadvertently allow the repository-local config to be written. What the added protection did not anticipate is that such a repository-local `core.hooksPath` can not only be used to point to maliciously-placed scripts in the current worktree, but also to _prevent_ hooks from being called altogether. We just reverted the `core.hooksPath` protections, based on the Git maintainer's recommendation in https://lore.kernel.org/git/xmqq4jaxvm8z.fsf@gitster.g/ to address this concern as well as related ones. Let's make sure that we won't regress while trying to protect the clone operation further. Reported-by: Brooke Kuhlmann Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- t/t1350-config-hooks-path.sh | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/t/t1350-config-hooks-path.sh b/t/t1350-config-hooks-path.sh index f6dc83e2aa..45a0492917 100755 --- a/t/t1350-config-hooks-path.sh +++ b/t/t1350-config-hooks-path.sh @@ -41,4 +41,11 @@ test_expect_success 'git rev-parse --git-path hooks' ' test .git/custom-hooks/abc = "$(cat actual)" ' +test_expect_success 'core.hooksPath=/dev/null' ' + git clone -c core.hooksPath=/dev/null . no-templates && + value="$(git -C no-templates config --local core.hooksPath)" && + # The Bash used by Git for Windows rewrites `/dev/null` to `nul` + { test /dev/null = "$value" || test nul = "$value"; } +' + test_done From 873a466ea3f233d4fb11f894a311de06939a2a3e Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Mon, 20 May 2024 20:22:04 +0000 Subject: [PATCH 10/12] clone: drop the protections where hooks aren't run As part of the security bug-fix releases v2.39.4, ..., v2.45.1, I introduced logic to safeguard `git clone` from running hooks that were installed _during_ the clone operation. The rationale was that Git's CVE-2024-32002, CVE-2021-21300, CVE-2019-1354, CVE-2019-1353, CVE-2019-1352, and CVE-2019-1349 should have been low-severity vulnerabilities but were elevated to critical/high severity by the attack vector that allows a weakness where files inside `.git/` can be inadvertently written during a `git clone` to escalate to a Remote Code Execution attack by virtue of installing a malicious `post-checkout` hook that Git will then run at the end of the operation without giving the user a chance to see what code is executed. Unfortunately, Git LFS uses a similar strategy to install its own `post-checkout` hook during a `git clone`; In fact, Git LFS is installing four separate hooks while running the `smudge` filter. While this pattern is probably in want of being improved by introducing better support in Git for Git LFS and other tools wishing to register hooks to be run at various stages of Git's commands, let's undo the clone protections to unbreak Git LFS-enabled clones. This reverts commit 8db1e8743c0 (clone: prevent hooks from running during a clone, 2024-03-28). Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- builtin/clone.c | 12 +----------- hook.c | 34 -------------------------------- t/t5601-clone.sh | 51 ------------------------------------------------ 3 files changed, 1 insertion(+), 96 deletions(-) diff --git a/builtin/clone.c b/builtin/clone.c index e7721f5c22..9ec500d427 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -937,8 +937,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix) int err = 0, complete_refs_before_fetch = 1; int submodule_progress; int filter_submodules = 0; - const char *template_dir; - char *template_dir_dup = NULL; struct transport_ls_refs_options transport_ls_refs_options = TRANSPORT_LS_REFS_OPTIONS_INIT; @@ -958,13 +956,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix) usage_msg_opt(_("You must specify a repository to clone."), builtin_clone_usage, builtin_clone_options); - xsetenv("GIT_CLONE_PROTECTION_ACTIVE", "true", 0 /* allow user override */); - template_dir = get_template_dir(option_template); - if (*template_dir && !is_absolute_path(template_dir)) - template_dir = template_dir_dup = - absolute_pathdup(template_dir); - xsetenv("GIT_CLONE_TEMPLATE_DIR", template_dir, 1); - if (option_depth || option_since || option_not.nr) deepen = 1; if (option_single_branch == -1) @@ -1112,7 +1103,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) } } - init_db(git_dir, real_git_dir, template_dir, GIT_HASH_UNKNOWN, NULL, + init_db(git_dir, real_git_dir, option_template, GIT_HASH_UNKNOWN, NULL, INIT_DB_QUIET); if (real_git_dir) { @@ -1430,7 +1421,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix) free(unborn_head); free(dir); free(path); - free(template_dir_dup); UNLEAK(repo); junk_mode = JUNK_LEAVE_ALL; diff --git a/hook.c b/hook.c index fc974cee1d..22b274b60b 100644 --- a/hook.c +++ b/hook.c @@ -3,32 +3,6 @@ #include "run-command.h" #include "config.h" -static int identical_to_template_hook(const char *name, const char *path) -{ - const char *env = getenv("GIT_CLONE_TEMPLATE_DIR"); - const char *template_dir = get_template_dir(env && *env ? env : NULL); - struct strbuf template_path = STRBUF_INIT; - int found_template_hook, ret; - - strbuf_addf(&template_path, "%s/hooks/%s", template_dir, name); - found_template_hook = access(template_path.buf, X_OK) >= 0; -#ifdef STRIP_EXTENSION - if (!found_template_hook) { - strbuf_addstr(&template_path, STRIP_EXTENSION); - found_template_hook = access(template_path.buf, X_OK) >= 0; - } -#endif - if (!found_template_hook) { - strbuf_release(&template_path); - return 0; - } - - ret = do_files_match(template_path.buf, path); - - strbuf_release(&template_path); - return ret; -} - const char *find_hook(const char *name) { static struct strbuf path = STRBUF_INIT; @@ -64,14 +38,6 @@ const char *find_hook(const char *name) } return NULL; } - if (!git_hooks_path && git_env_bool("GIT_CLONE_PROTECTION_ACTIVE", 0) && - !identical_to_template_hook(name, path.buf)) - die(_("active `%s` hook found during `git clone`:\n\t%s\n" - "For security reasons, this is disallowed by default.\n" - "If this is intentional and the hook should actually " - "be run, please\nrun the command again with " - "`GIT_CLONE_PROTECTION_ACTIVE=false`"), - name, path.buf); return path.buf; } diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh index 20deca0231..fd02984330 100755 --- a/t/t5601-clone.sh +++ b/t/t5601-clone.sh @@ -771,57 +771,6 @@ test_expect_success 'batch missing blob request does not inadvertently try to fe git clone --filter=blob:limit=0 "file://$(pwd)/server" client ' -test_expect_success 'clone with init.templatedir runs hooks' ' - git init tmpl/hooks && - write_script tmpl/hooks/post-checkout <<-EOF && - echo HOOK-RUN >&2 - echo I was here >hook.run - EOF - git -C tmpl/hooks add . && - test_tick && - git -C tmpl/hooks commit -m post-checkout && - - test_when_finished "git config --global --unset init.templateDir || :" && - test_when_finished "git config --unset init.templateDir || :" && - ( - sane_unset GIT_TEMPLATE_DIR && - NO_SET_GIT_TEMPLATE_DIR=t && - export NO_SET_GIT_TEMPLATE_DIR && - - git -c core.hooksPath="$(pwd)/tmpl/hooks" \ - clone tmpl/hooks hook-run-hookspath 2>err && - ! grep "active .* hook found" err && - test_path_is_file hook-run-hookspath/hook.run && - - git -c init.templateDir="$(pwd)/tmpl" \ - clone tmpl/hooks hook-run-config 2>err && - ! grep "active .* hook found" err && - test_path_is_file hook-run-config/hook.run && - - git clone --template=tmpl tmpl/hooks hook-run-option 2>err && - ! grep "active .* hook found" err && - test_path_is_file hook-run-option/hook.run && - - git config --global init.templateDir "$(pwd)/tmpl" && - git clone tmpl/hooks hook-run-global-config 2>err && - git config --global --unset init.templateDir && - ! grep "active .* hook found" err && - test_path_is_file hook-run-global-config/hook.run && - - # clone ignores local `init.templateDir`; need to create - # a new repository because we deleted `.git/` in the - # `setup` test case above - git init local-clone && - cd local-clone && - - git config init.templateDir "$(pwd)/../tmpl" && - git clone ../tmpl/hooks hook-run-local-config 2>err && - git config --unset init.templateDir && - ! grep "active .* hook found" err && - test_path_is_missing hook-run-local-config/hook.run - ) -' - . "$TEST_DIRECTORY"/lib-httpd.sh start_httpd From 35dfccb2b430ee67fddb7b1a1f8cd7a1a8fb7cbe Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Mon, 20 May 2024 20:22:05 +0000 Subject: [PATCH 11/12] Revert "Add a helper function to compare file contents" Now that during a `git clone`, the hooks' contents are no longer compared to the templates' files', the caller for which the `do_files_match()` function was introduced is gone, and therefore this function can be retired, too. This reverts commit 584de0b4c23 (Add a helper function to compare file contents, 2024-03-30). Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- cache.h | 14 --------- copy.c | 58 -------------------------------------- t/helper/test-path-utils.c | 10 ------- t/t0060-path-utils.sh | 41 --------------------------- 4 files changed, 123 deletions(-) diff --git a/cache.h b/cache.h index 16b34799bf..8c5fb1e1ba 100644 --- a/cache.h +++ b/cache.h @@ -1785,20 +1785,6 @@ int copy_fd(int ifd, int ofd); int copy_file(const char *dst, const char *src, int mode); int copy_file_with_time(const char *dst, const char *src, int mode); -/* - * Compare the file mode and contents of two given files. - * - * If both files are actually symbolic links, the function returns 1 if the link - * targets are identical or 0 if they are not. - * - * If any of the two files cannot be accessed or in case of read failures, this - * function returns 0. - * - * If the file modes and contents are identical, the function returns 1, - * otherwise it returns 0. - */ -int do_files_match(const char *path1, const char *path2); - void write_or_die(int fd, const void *buf, size_t count); void fsync_or_die(int fd, const char *); int fsync_component(enum fsync_component component, int fd); diff --git a/copy.c b/copy.c index 8492f6fc83..4de6a110f0 100644 --- a/copy.c +++ b/copy.c @@ -65,61 +65,3 @@ int copy_file_with_time(const char *dst, const char *src, int mode) return copy_times(dst, src); return status; } - -static int do_symlinks_match(const char *path1, const char *path2) -{ - struct strbuf buf1 = STRBUF_INIT, buf2 = STRBUF_INIT; - int ret = 0; - - if (!strbuf_readlink(&buf1, path1, 0) && - !strbuf_readlink(&buf2, path2, 0)) - ret = !strcmp(buf1.buf, buf2.buf); - - strbuf_release(&buf1); - strbuf_release(&buf2); - return ret; -} - -int do_files_match(const char *path1, const char *path2) -{ - struct stat st1, st2; - int fd1 = -1, fd2 = -1, ret = 1; - char buf1[8192], buf2[8192]; - - if ((fd1 = open_nofollow(path1, O_RDONLY)) < 0 || - fstat(fd1, &st1) || !S_ISREG(st1.st_mode)) { - if (fd1 < 0 && errno == ELOOP) - /* maybe this is a symbolic link? */ - return do_symlinks_match(path1, path2); - ret = 0; - } else if ((fd2 = open_nofollow(path2, O_RDONLY)) < 0 || - fstat(fd2, &st2) || !S_ISREG(st2.st_mode)) { - ret = 0; - } - - if (ret) - /* to match, neither must be executable, or both */ - ret = !(st1.st_mode & 0111) == !(st2.st_mode & 0111); - - if (ret) - ret = st1.st_size == st2.st_size; - - while (ret) { - ssize_t len1 = read_in_full(fd1, buf1, sizeof(buf1)); - ssize_t len2 = read_in_full(fd2, buf2, sizeof(buf2)); - - if (len1 < 0 || len2 < 0 || len1 != len2) - ret = 0; /* read error or different file size */ - else if (!len1) /* len2 is also 0; hit EOF on both */ - break; /* ret is still true */ - else - ret = !memcmp(buf1, buf2, len1); - } - - if (fd1 >= 0) - close(fd1); - if (fd2 >= 0) - close(fd2); - - return ret; -} diff --git a/t/helper/test-path-utils.c b/t/helper/test-path-utils.c index 0e0de21807..f69709d674 100644 --- a/t/helper/test-path-utils.c +++ b/t/helper/test-path-utils.c @@ -495,16 +495,6 @@ int cmd__path_utils(int argc, const char **argv) return !!res; } - if (argc == 4 && !strcmp(argv[1], "do_files_match")) { - int ret = do_files_match(argv[2], argv[3]); - - if (ret) - printf("equal\n"); - else - printf("different\n"); - return !ret; - } - fprintf(stderr, "%s: unknown function name: %s\n", argv[0], argv[1] ? argv[1] : "(there was none)"); return 1; diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh index 73d0e1a7f1..68e29c904a 100755 --- a/t/t0060-path-utils.sh +++ b/t/t0060-path-utils.sh @@ -560,45 +560,4 @@ test_expect_success !VALGRIND,RUNTIME_PREFIX,CAN_EXEC_IN_PWD '%(prefix)/ works' test_cmp expect actual ' -test_expect_success 'do_files_match()' ' - test_seq 0 10 >0-10.txt && - test_seq -1 10 >-1-10.txt && - test_seq 1 10 >1-10.txt && - test_seq 1 9 >1-9.txt && - test_seq 0 8 >0-8.txt && - - test-tool path-utils do_files_match 0-10.txt 0-10.txt >out && - - assert_fails() { - test_must_fail \ - test-tool path-utils do_files_match "$1" "$2" >out && - grep different out - } && - - assert_fails 0-8.txt 1-9.txt && - assert_fails -1-10.txt 0-10.txt && - assert_fails 1-10.txt 1-9.txt && - assert_fails 1-10.txt .git && - assert_fails does-not-exist 1-10.txt && - - if test_have_prereq FILEMODE - then - cp 0-10.txt 0-10.x && - chmod a+x 0-10.x && - assert_fails 0-10.txt 0-10.x - fi && - - if test_have_prereq SYMLINKS - then - ln -sf 0-10.txt symlink && - ln -s 0-10.txt another-symlink && - ln -s over-the-ocean yet-another-symlink && - ln -s "$PWD/0-10.txt" absolute-symlink && - assert_fails 0-10.txt symlink && - test-tool path-utils do_files_match symlink another-symlink && - assert_fails symlink yet-another-symlink && - assert_fails symlink absolute-symlink - fi -' - test_done From 1991703bdbb7839e3b43e0e0238f8efd76c8fe8d Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Wed, 22 May 2024 21:55:31 -0700 Subject: [PATCH 12/12] Revert "fsck: warn about symlink pointing inside a gitdir" This reverts commit a33fea08 (fsck: warn about symlink pointing inside a gitdir, 2024-04-10), which warns against symbolic links commonly created by git-annex. --- Documentation/fsck-msgids.txt | 12 -------- fsck.c | 56 ----------------------------------- fsck.h | 12 -------- t/t1450-fsck.sh | 37 ----------------------- 4 files changed, 117 deletions(-) diff --git a/Documentation/fsck-msgids.txt b/Documentation/fsck-msgids.txt index b06ec385af..12eae8a222 100644 --- a/Documentation/fsck-msgids.txt +++ b/Documentation/fsck-msgids.txt @@ -157,18 +157,6 @@ `nullSha1`:: (WARN) Tree contains entries pointing to a null sha1. -`symlinkPointsToGitDir`:: - (WARN) Symbolic link points inside a gitdir. - -`symlinkTargetBlob`:: - (ERROR) A non-blob found instead of a symbolic link's target. - -`symlinkTargetLength`:: - (WARN) Symbolic link target longer than maximum path length. - -`symlinkTargetMissing`:: - (ERROR) Unable to read symbolic link target's blob. - `treeNotSorted`:: (ERROR) A tree is not properly sorted. diff --git a/fsck.c b/fsck.c index b85868e122..47eaeedd70 100644 --- a/fsck.c +++ b/fsck.c @@ -636,8 +636,6 @@ static int fsck_tree(const struct object_id *tree_oid, retval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_MAILMAP_SYMLINK, ".mailmap is a symlink"); - oidset_insert(&options->symlink_targets_found, - entry_oid); } if ((backslash = strchr(name, '\\'))) { @@ -1230,56 +1228,6 @@ static int fsck_blob(const struct object_id *oid, const char *buf, } } - if (oidset_contains(&options->symlink_targets_found, oid)) { - const char *ptr = buf; - const struct object_id *reported = NULL; - - oidset_insert(&options->symlink_targets_done, oid); - - if (!buf || size > PATH_MAX) { - /* - * A missing buffer here is a sign that the caller found the - * blob too gigantic to load into memory. Let's just consider - * that an error. - */ - return report(options, oid, OBJ_BLOB, - FSCK_MSG_SYMLINK_TARGET_LENGTH, - "symlink target too long"); - } - - while (!reported && ptr) { - const char *p = ptr; - char c, *slash = strchrnul(ptr, '/'); - char *backslash = memchr(ptr, '\\', slash - ptr); - - c = *slash; - *slash = '\0'; - - while (!reported && backslash) { - *backslash = '\0'; - if (is_ntfs_dotgit(p)) - ret |= report(options, reported = oid, OBJ_BLOB, - FSCK_MSG_SYMLINK_POINTS_TO_GIT_DIR, - "symlink target points to git dir"); - *backslash = '\\'; - p = backslash + 1; - backslash = memchr(p, '\\', slash - p); - } - if (!reported && is_ntfs_dotgit(p)) - ret |= report(options, reported = oid, OBJ_BLOB, - FSCK_MSG_SYMLINK_POINTS_TO_GIT_DIR, - "symlink target points to git dir"); - - if (!reported && is_hfs_dotgit(ptr)) - ret |= report(options, reported = oid, OBJ_BLOB, - FSCK_MSG_SYMLINK_POINTS_TO_GIT_DIR, - "symlink target points to git dir"); - - *slash = c; - ptr = c ? slash + 1 : NULL; - } - } - return ret; } @@ -1371,10 +1319,6 @@ int fsck_finish(struct fsck_options *options) FSCK_MSG_GITATTRIBUTES_MISSING, FSCK_MSG_GITATTRIBUTES_BLOB, options, ".gitattributes"); - ret |= fsck_blobs(&options->symlink_targets_found, &options->symlink_targets_done, - FSCK_MSG_SYMLINK_TARGET_MISSING, FSCK_MSG_SYMLINK_TARGET_BLOB, - options, ""); - return ret; } diff --git a/fsck.h b/fsck.h index 130fa8d8f9..fcecf4101c 100644 --- a/fsck.h +++ b/fsck.h @@ -63,8 +63,6 @@ enum fsck_msg_type { FUNC(GITATTRIBUTES_LARGE, ERROR) \ FUNC(GITATTRIBUTES_LINE_LENGTH, ERROR) \ FUNC(GITATTRIBUTES_BLOB, ERROR) \ - FUNC(SYMLINK_TARGET_MISSING, ERROR) \ - FUNC(SYMLINK_TARGET_BLOB, ERROR) \ /* warnings */ \ FUNC(EMPTY_NAME, WARN) \ FUNC(FULL_PATHNAME, WARN) \ @@ -74,8 +72,6 @@ enum fsck_msg_type { FUNC(NULL_SHA1, WARN) \ FUNC(ZERO_PADDED_FILEMODE, WARN) \ FUNC(NUL_IN_COMMIT, WARN) \ - FUNC(SYMLINK_TARGET_LENGTH, WARN) \ - FUNC(SYMLINK_POINTS_TO_GIT_DIR, WARN) \ /* infos (reported as warnings, but ignored by default) */ \ FUNC(BAD_FILEMODE, INFO) \ FUNC(GITMODULES_PARSE, INFO) \ @@ -143,8 +139,6 @@ struct fsck_options { struct oidset gitmodules_done; struct oidset gitattributes_found; struct oidset gitattributes_done; - struct oidset symlink_targets_found; - struct oidset symlink_targets_done; kh_oid_map_t *object_names; }; @@ -154,8 +148,6 @@ struct fsck_options { .gitmodules_done = OIDSET_INIT, \ .gitattributes_found = OIDSET_INIT, \ .gitattributes_done = OIDSET_INIT, \ - .symlink_targets_found = OIDSET_INIT, \ - .symlink_targets_done = OIDSET_INIT, \ .error_func = fsck_error_function \ } #define FSCK_OPTIONS_STRICT { \ @@ -164,8 +156,6 @@ struct fsck_options { .gitmodules_done = OIDSET_INIT, \ .gitattributes_found = OIDSET_INIT, \ .gitattributes_done = OIDSET_INIT, \ - .symlink_targets_found = OIDSET_INIT, \ - .symlink_targets_done = OIDSET_INIT, \ .error_func = fsck_error_function, \ } #define FSCK_OPTIONS_MISSING_GITMODULES { \ @@ -174,8 +164,6 @@ struct fsck_options { .gitmodules_done = OIDSET_INIT, \ .gitattributes_found = OIDSET_INIT, \ .gitattributes_done = OIDSET_INIT, \ - .symlink_targets_found = OIDSET_INIT, \ - .symlink_targets_done = OIDSET_INIT, \ .error_func = fsck_error_cb_print_missing_gitmodules, \ } diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh index 5669872bc8..de0f6d5e7f 100755 --- a/t/t1450-fsck.sh +++ b/t/t1450-fsck.sh @@ -1023,41 +1023,4 @@ test_expect_success 'fsck error on gitattributes with excessive size' ' test_cmp expected actual ' -test_expect_success 'fsck warning on symlink target with excessive length' ' - symlink_target=$(printf "pattern %032769d" 1 | git hash-object -w --stdin) && - test_when_finished "remove_object $symlink_target" && - tree=$(printf "120000 blob %s\t%s\n" $symlink_target symlink | git mktree) && - test_when_finished "remove_object $tree" && - cat >expected <<-EOF && - warning in blob $symlink_target: symlinkTargetLength: symlink target too long - EOF - git fsck --no-dangling >actual 2>&1 && - test_cmp expected actual -' - -test_expect_success 'fsck warning on symlink target pointing inside git dir' ' - gitdir=$(printf ".git" | git hash-object -w --stdin) && - ntfs_gitdir=$(printf "GIT~1" | git hash-object -w --stdin) && - hfs_gitdir=$(printf ".${u200c}git" | git hash-object -w --stdin) && - inside_gitdir=$(printf "nested/.git/config" | git hash-object -w --stdin) && - benign_target=$(printf "legit/config" | git hash-object -w --stdin) && - tree=$(printf "120000 blob %s\t%s\n" \ - $benign_target benign_target \ - $gitdir gitdir \ - $hfs_gitdir hfs_gitdir \ - $inside_gitdir inside_gitdir \ - $ntfs_gitdir ntfs_gitdir | - git mktree) && - for o in $gitdir $ntfs_gitdir $hfs_gitdir $inside_gitdir $benign_target $tree - do - test_when_finished "remove_object $o" || return 1 - done && - printf "warning in blob %s: symlinkPointsToGitDir: symlink target points to git dir\n" \ - $gitdir $hfs_gitdir $inside_gitdir $ntfs_gitdir | - sort >expected && - git fsck --no-dangling >actual 2>&1 && - sort actual >actual.sorted && - test_cmp expected actual.sorted -' - test_done