{"thread":{"id":"61544","subject":"[PATCH v2 00/12] Fix various overly aggressive protections in 2.45.1 and friends","startedAt":"2024-05-24T19:47:18Z","lastAt":"2024-05-24T19:47:46Z","messageCount":13,"participants":["Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":12},"messages":[{"id":"495588","messageId":"20240524194715.695916-1-gitster@pobox.com","threadId":"61544","inReplyTo":null,"subject":"[PATCH v2 00/12] Fix various overly aggressive protections in 2.45.1 and friends","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-24T19:47:03Z","receivedAt":"2024-05-24T19:47:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"As people have seen, the latest \"security fix\" release turned out to\nbe a mixed bag of good vulnerability fixes with a bit over-eager\n\"layered defence\" that broke real uses cases like git-lfs.  Let's\nquickly get them in working order back first, with the vision that\nwe will then rebuild layered defence more carefully in the open on\ntop as necessary.\n\nWhat we have here are the first \"revert\" part.\n\nThese patches are designed to apply to 2.39.4; the series may have\nto grow as we discover more things to revert, but for now here are\nthe patches to\n\n - revert the over-eager \"refusal to work\" went into 2.39.4\n\n - adjust 2.39.4 codebase to cleanly build and test (at CI and\n   locally) by backported fixes\n\nIt would have been better if we did not have to have the latter\nclass, but such is life.\n\nRelative to the previous iteration, there are two differences, which\nare:\n\n * Old [12/12] that reverted the repository ownership check for\n   local case is gone.  A well known escape hatch is available that\n   is easy to use when the repositories are trusted (most notably,\n   in a hosting set-up, the repositories are trusted not to attack\n   the 'nobody' user that is running 'git').\n\n * New [12/12] reverts a dubious checks for targets of symbolic\n   links done in \"git fsck\" (and transfer).\n\nToday's integration cycle is pretty much committed to have these in\n'next' for the weekend, merge them down to 'master' by the end of\nmonth, hoping that we can do 2.45.2 and friends sometime early next\nmonth.\n\n\nJeff King (5):\n  send-email: drop FakeTerm hack\n  send-email: avoid creating more than one Term::ReadLine object\n  ci: drop mention of BREW_INSTALL_PACKAGES variable\n  ci: avoid bare \"gcc\" for osx-gcc job\n  ci: stop installing \"gcc-13\" for osx-gcc\n\nJohannes Schindelin (6):\n  hook: plug a new memory leak\n  init: use the correct path of the templates directory again\n  Revert \"core.hooksPath: add some protection while cloning\"\n  tests: verify that `clone -c core.hooksPath=/dev/null` works again\n  clone: drop the protections where hooks aren't run\n  Revert \"Add a helper function to compare file contents\"\n\nJunio C Hamano (1):\n  Revert \"fsck: warn about symlink pointing inside a gitdir\"\n\n .github/workflows/main.yml    |  3 +-\n Documentation/fsck-msgids.txt | 12 --------\n Makefile                      |  2 +-\n builtin/clone.c               | 12 +-------\n cache.h                       | 14 ---------\n ci/install-dependencies.sh    |  2 --\n config.c                      | 13 +-------\n copy.c                        | 58 -----------------------------------\n fsck.c                        | 56 ---------------------------------\n fsck.h                        | 12 --------\n git-send-email.perl           | 32 +++++++------------\n hook.c                        | 32 -------------------\n t/helper/test-path-utils.c    | 10 ------\n t/t0060-path-utils.sh         | 41 -------------------------\n t/t1350-config-hooks-path.sh  |  7 +++++\n t/t1450-fsck.sh               | 37 ----------------------\n t/t1800-hook.sh               | 15 ---------\n t/t5601-clone.sh              | 51 ------------------------------\n t/t9001-send-email.sh         |  5 +--\n 19 files changed, 25 insertions(+), 389 deletions(-)\n\n-- \n2.45.1-246-gb9cfe4845c\n\n"},{"id":"495589","messageId":"20240524194715.695916-2-gitster@pobox.com","threadId":"61544","inReplyTo":"20240524194715.695916-1-gitster@pobox.com","subject":"[PATCH v2 01/12] send-email: drop FakeTerm hack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-24T19:47:04Z","receivedAt":"2024-05-24T19:47:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Jeff King <peff@peff.net>\n\nBack in 280242d1cc (send-email: do not barf when Term::ReadLine does not\nlike your terminal, 2006-07-02), we added a fallback for when\nTerm::ReadLine's constructor failed: we'd have a FakeTerm object\ninstead, which would then die if anybody actually tried to call\nreadline() on it. Since we instantiated the $term variable at program\nstartup, we needed this workaround to let the program run in modes when\nwe did not prompt the user.\n\nBut later, in f4dc9432fd (send-email: lazily load modules for a big\nspeedup, 2021-05-28), we started loading Term::ReadLine lazily only when\nask() is called. So at that point we know we're trying to prompt the\nuser, and we can just die if ReadLine instantiation fails, rather than\nmaking this fake object to lazily delay showing the error.\n\nThis should be OK even if there is no tty (e.g., we're in a cron job),\nbecause Term::ReadLine will return a stub object in that case whose \"IN\"\nand \"OUT\" functions return undef. And since 5906f54e47 (send-email:\ndon't attempt to prompt if tty is closed, 2009-03-31), we check for that\ncase and skip prompting.\n\nAnd we can be sure that FakeTerm was not kicking in for such a\nsituation, because it has actually been broken since that commit! It\ndoes not define \"IN\" or \"OUT\" methods, so perl would barf with an error.\nIf FakeTerm was in use, we were neither honoring what 5906f54e47 tried\nto do, nor producing the readable message that 280242d1cc intended.\n\nSo we're better off just dropping FakeTerm entirely, and letting the\nerror reported by constructing Term::ReadLine through.\n\n[jc: cherry-picked from v2.42.0-rc2~6^2~1]\n\nSigned-off-by: Jeff King <peff@peff.net>\nAcked-by: Taylor Blau <me@ttaylorr.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n git-send-email.perl | 22 ++--------------------\n 1 file changed, 2 insertions(+), 20 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 5861e99a6e..72d876f0a0 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -26,18 +26,6 @@\n \n Getopt::Long::Configure qw/ pass_through /;\n \n-package FakeTerm;\n-sub new {\n-\tmy ($class, $reason) = @_;\n-\treturn bless \\$reason, shift;\n-}\n-sub readline {\n-\tmy $self = shift;\n-\tdie \"Cannot use readline on FakeTerm: $$self\";\n-}\n-package main;\n-\n-\n sub usage {\n \tprint <<EOT;\n git send-email' [<options>] <file|directory>\n@@ -930,16 +918,10 @@ sub get_patch_subject {\n }\n \n sub term {\n-\tmy $term = eval {\n-\t\trequire Term::ReadLine;\n-\t\t$ENV{\"GIT_SEND_EMAIL_NOTTY\"}\n+\trequire Term::ReadLine;\n+\treturn $ENV{\"GIT_SEND_EMAIL_NOTTY\"}\n \t\t\t? Term::ReadLine->new('git-send-email', \\*STDIN, \\*STDOUT)\n \t\t\t: Term::ReadLine->new('git-send-email');\n-\t};\n-\tif ($@) {\n-\t\t$term = FakeTerm->new(\"$@: going non-interactive\");\n-\t}\n-\treturn $term;\n }\n \n sub ask {\n-- \n2.45.1-246-gb9cfe4845c\n\n"},{"id":"495590","messageId":"20240524194715.695916-3-gitster@pobox.com","threadId":"61544","inReplyTo":"20240524194715.695916-1-gitster@pobox.com","subject":"[PATCH v2 02/12] send-email: avoid creating more than one Term::ReadLine object","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-24T19:47:05Z","receivedAt":"2024-05-24T19:47:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Jeff King <peff@peff.net>\n\nEvery time git-send-email calls its ask() function to prompt the user,\nwe call term(), which instantiates a new Term::ReadLine object. But in\nv1.46 of Term::ReadLine::Gnu (which provides the Term::ReadLine\ninterface on some platforms), its constructor refuses to create a second\ninstance[1]. So on systems with that version of the module, most\ngit-send-email instances will fail (as we usually prompt for both \"to\"\nand \"in-reply-to\" unless the user provided them on the command line).\n\nWe can fix this by keeping a single instance variable and returning it\nfor each call to term(). In perl 5.10 and up, we could do that with a\n\"state\" variable. But since we only require 5.008, we'll do it the\nold-fashioned way, with a lexical \"my\" in its own scope.\n\nNote that the tests in t9001 detect this problem as-is, since the\nfailure mode is for the program to die. But let's also beef up the\n\"Prompting works\" test to check that it correctly handles multiple\ninputs (if we had chosen to keep our FakeTerm hack in the previous\ncommit, then the failure mode would be incorrectly ignoring prompts\nafter the first).\n\n[1] For discussion of why multiple instances are forbidden, see:\n    https://github.com/hirooih/perl-trg/issues/16\n\n[jc: cherry-picked from v2.42.0-rc2~6^2]\n\nSigned-off-by: Jeff King <peff@peff.net>\nAcked-by: Taylor Blau <me@ttaylorr.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n git-send-email.perl   | 18 +++++++++++++-----\n t/t9001-send-email.sh |  5 +++--\n 2 files changed, 16 insertions(+), 7 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 72d876f0a0..ad51508790 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -917,11 +917,19 @@ sub get_patch_subject {\n \tdo_edit(@files);\n }\n \n-sub term {\n-\trequire Term::ReadLine;\n-\treturn $ENV{\"GIT_SEND_EMAIL_NOTTY\"}\n-\t\t\t? Term::ReadLine->new('git-send-email', \\*STDIN, \\*STDOUT)\n-\t\t\t: Term::ReadLine->new('git-send-email');\n+{\n+\t# Only instantiate one $term per program run, since some\n+\t# Term::ReadLine providers refuse to create a second instance.\n+\tmy $term;\n+\tsub term {\n+\t\trequire Term::ReadLine;\n+\t\tif (!defined $term) {\n+\t\t\t$term = $ENV{\"GIT_SEND_EMAIL_NOTTY\"}\n+\t\t\t\t? Term::ReadLine->new('git-send-email', \\*STDIN, \\*STDOUT)\n+\t\t\t\t: Term::ReadLine->new('git-send-email');\n+\t\t}\n+\t\treturn $term;\n+\t}\n }\n \n sub ask {\ndiff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\nindex 1130ef21b3..0f08a9542b 100755\n--- a/t/t9001-send-email.sh\n+++ b/t/t9001-send-email.sh\n@@ -337,13 +337,14 @@ test_expect_success $PREREQ 'Show all headers' '\n test_expect_success $PREREQ 'Prompting works' '\n \tclean_fake_sendmail &&\n \t(echo \"to@example.com\" &&\n-\t echo \"\"\n+\t echo \"my-message-id@example.com\"\n \t) | GIT_SEND_EMAIL_NOTTY=1 git send-email \\\n \t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n \t\t$patches \\\n \t\t2>errors &&\n \t\tgrep \"^From: A U Thor <author@example.com>\\$\" msgtxt1 &&\n-\t\tgrep \"^To: to@example.com\\$\" msgtxt1\n+\t\tgrep \"^To: to@example.com\\$\" msgtxt1 &&\n+\t\tgrep \"^In-Reply-To: <my-message-id@example.com>\" msgtxt1\n '\n \n test_expect_success $PREREQ,AUTOIDENT 'implicit ident is allowed' '\n-- \n2.45.1-246-gb9cfe4845c\n\n"},{"id":"495591","messageId":"20240524194715.695916-4-gitster@pobox.com","threadId":"61544","inReplyTo":"20240524194715.695916-1-gitster@pobox.com","subject":"[PATCH v2 03/12] ci: drop mention of BREW_INSTALL_PACKAGES variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-24T19:47:06Z","receivedAt":"2024-05-24T19:47:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Jeff King <peff@peff.net>\n\nThe last user of this variable went away in 4a6e4b9602 (CI: remove\nTravis CI support, 2021-11-23), so it's doing nothing except making it\nmore confusing to find out which packages _are_ installed.\n\n[jc: cherry-picked from v2.45.0-1-g9d4453e8d6]\n\nSigned-off-by: Jeff King <peff@peff.net>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n ci/install-dependencies.sh | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh\nindex 4f407530d3..33039d516b 100755\n--- a/ci/install-dependencies.sh\n+++ b/ci/install-dependencies.sh\n@@ -34,8 +34,6 @@ macos-*)\n \texport HOMEBREW_NO_AUTO_UPDATE=1 HOMEBREW_NO_INSTALL_CLEANUP=1\n \t# Uncomment this if you want to run perf tests:\n \t# brew install gnu-time\n-\ttest -z \"$BREW_INSTALL_PACKAGES\" ||\n-\tbrew install $BREW_INSTALL_PACKAGES\n \tbrew link --force gettext\n \tmkdir -p $HOME/bin\n \t(\n-- \n2.45.1-246-gb9cfe4845c\n\n"},{"id":"495592","messageId":"20240524194715.695916-5-gitster@pobox.com","threadId":"61544","inReplyTo":"20240524194715.695916-1-gitster@pobox.com","subject":"[PATCH v2 04/12] ci: avoid bare \"gcc\" for osx-gcc job","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-24T19:47:07Z","receivedAt":"2024-05-24T19:47:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Jeff King <peff@peff.net>\n\nOn macOS, a bare \"gcc\" (without a version) will invoke a wrapper for\nclang, not actual gcc. Even when gcc is installed via homebrew, that\nonly provides version-specific links in /usr/local/bin (like \"gcc-13\"),\nand never a version-agnostic \"gcc\" wrapper.\n\nAs far as I can tell, this has been the case for a long time, and this\nosx-gcc job has largely been doing nothing. We can point it at \"gcc-13\",\nwhich will pick up the homebrew-installed version.\n\nThe fix here is specific to the github workflow file, as the gitlab one\ndoes not have a matching job.\n\nIt's a little unfortunate that we cannot just ask for the latest version\nof gcc which homebrew provides, but as far as I can tell there is no\neasy alias (you'd have to find the highest number gcc-* in\n/usr/local/bin yourself).\n\n[jc: cherry-picked from v2.45.0-2-g11c7001e3d]\n\nSigned-off-by: Jeff King <peff@peff.net>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n .github/workflows/main.yml | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/.github/workflows/main.yml b/.github/workflows/main.yml\nindex 2dc0221f7f..583e7cd5f0 100644\n--- a/.github/workflows/main.yml\n+++ b/.github/workflows/main.yml\n@@ -264,7 +264,7 @@ jobs:\n             cc: clang\n             pool: macos-13\n           - jobname: osx-gcc\n-            cc: gcc\n+            cc: gcc-13\n             cc_package: gcc-13\n             pool: macos-13\n           - jobname: linux-gcc-default\n-- \n2.45.1-246-gb9cfe4845c\n\n"},{"id":"495593","messageId":"20240524194715.695916-6-gitster@pobox.com","threadId":"61544","inReplyTo":"20240524194715.695916-1-gitster@pobox.com","subject":"[PATCH v2 05/12] ci: stop installing \"gcc-13\" for osx-gcc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-24T19:47:08Z","receivedAt":"2024-05-24T19:47:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Jeff King <peff@peff.net>\n\nOur osx-gcc job explicitly asks to install gcc-13. But since the GitHub\nrunner image already comes with gcc-13 installed, this is mostly doing\nnothing (or in some cases it may install an incremental update over the\nrunner image). But worse, it recently started causing errors like:\n\n    ==> Fetching gcc@13\n    ==> Downloading https://ghcr.io/v2/homebrew/core/gcc/13/blobs/sha256:fb2403d97e2ce67eb441b54557cfb61980830f3ba26d4c5a1fe5ecd0c9730d1a\n    ==> Pouring gcc@13--13.2.0.ventura.bottle.tar.gz\n    Error: The `brew link` step did not complete successfully\n    The formula built, but is not symlinked into /usr/local\n    Could not symlink bin/c++-13\n    Target /usr/local/bin/c++-13\n    is a symlink belonging to gcc. You can unlink it:\n      brew unlink gcc\n\nwhich cause the whole CI job to bail.\n\nI didn't track down the root cause, but I suspect it may be related to\nhomebrew recently switching the \"gcc\" default to gcc-14. And it may even\nbe fixed when a new runner image is released. But if we don't need to\nrun brew at all, it's one less thing for us to worry about.\n\n[jc: cherry-picked from v2.45.0-3-g7df2405b38]\n\nSigned-off-by: Jeff King <peff@peff.net>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n .github/workflows/main.yml | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/.github/workflows/main.yml b/.github/workflows/main.yml\nindex 583e7cd5f0..76e3f1e768 100644\n--- a/.github/workflows/main.yml\n+++ b/.github/workflows/main.yml\n@@ -265,7 +265,6 @@ jobs:\n             pool: macos-13\n           - jobname: osx-gcc\n             cc: gcc-13\n-            cc_package: gcc-13\n             pool: macos-13\n           - jobname: linux-gcc-default\n             cc: gcc\n-- \n2.45.1-246-gb9cfe4845c\n\n"},{"id":"495594","messageId":"20240524194715.695916-7-gitster@pobox.com","threadId":"61544","inReplyTo":"20240524194715.695916-1-gitster@pobox.com","subject":"[PATCH v2 06/12] hook: plug a new memory leak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-24T19:47:09Z","receivedAt":"2024-05-24T19:47:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nIn 8db1e8743c0 (clone: prevent hooks from running during a clone,\n2024-03-28), I introduced an inadvertent memory leak that was\nunfortunately not caught before v2.45.1 was released. Here is a fix.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n hook.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/hook.c b/hook.c\nindex 632b537b99..fc974cee1d 100644\n--- a/hook.c\n+++ b/hook.c\n@@ -18,8 +18,10 @@ static int identical_to_template_hook(const char *name, const char *path)\n \t\tfound_template_hook = access(template_path.buf, X_OK) >= 0;\n \t}\n #endif\n-\tif (!found_template_hook)\n+\tif (!found_template_hook) {\n+\t\tstrbuf_release(&template_path);\n \t\treturn 0;\n+\t}\n \n \tret = do_files_match(template_path.buf, path);\n \n-- \n2.45.1-246-gb9cfe4845c\n\n"},{"id":"495595","messageId":"20240524194715.695916-8-gitster@pobox.com","threadId":"61544","inReplyTo":"20240524194715.695916-1-gitster@pobox.com","subject":"[PATCH v2 07/12] init: use the correct path of the templates directory again","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-24T19:47:10Z","receivedAt":"2024-05-24T19:47:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nIn df93e407f06 (init: refactor the template directory discovery into its\nown function, 2024-03-29), I refactored the way the templates directory\nis discovered.\n\nThe refactoring was faithful, but missed a reference in the `Makefile`\nwhere the `DEFAULT_GIT_TEMPLATE_DIR` constant is defined. As a\nconsequence, Git v2.45.1 and friends will always use the hard-coded path\n`/usr/share/git-core/templates`.\n\nLet's fix that by defining the `DEFAULT_GIT_TEMPLATE_DIR` when building\n`setup.o`, where that constant is actually used.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n Makefile | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/Makefile b/Makefile\nindex 093829ae28..4b1502ba2c 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2751,7 +2751,7 @@ exec-cmd.sp exec-cmd.s exec-cmd.o: EXTRA_CPPFLAGS = \\\n \t'-DFALLBACK_RUNTIME_PREFIX=\"$(prefix_SQ)\"'\n \n builtin/init-db.sp builtin/init-db.s builtin/init-db.o: GIT-PREFIX\n-builtin/init-db.sp builtin/init-db.s builtin/init-db.o: EXTRA_CPPFLAGS = \\\n+setup.sp setup.s setup.o: EXTRA_CPPFLAGS = \\\n \t-DDEFAULT_GIT_TEMPLATE_DIR='\"$(template_dir_SQ)\"'\n \n config.sp config.s config.o: GIT-PREFIX\n-- \n2.45.1-246-gb9cfe4845c\n\n"},{"id":"495596","messageId":"20240524194715.695916-9-gitster@pobox.com","threadId":"61544","inReplyTo":"20240524194715.695916-1-gitster@pobox.com","subject":"[PATCH v2 08/12] Revert \"core.hooksPath: add some protection while cloning\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-24T19:47:11Z","receivedAt":"2024-05-24T19:47:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nThis defense-in-depth was intended to protect the clone operation\nagainst future escalations where bugs in `git clone` would allow\nattackers to write arbitrary files in the `.git/` directory would allow\nfor Remote Code Execution attacks via maliciously-placed hooks.\n\nHowever, it turns out that the `core.hooksPath` protection has\nunintentional side effects so severe that they do not justify the\nbenefit of the protections. For example, it has been reported in\nhttps://lore.kernel.org/git/FAFA34CB-9732-4A0A-87FB-BDB272E6AEE8@alchemists.io/\nthat the following invocation, which is intended to make `git clone`\nsafer, is itself broken by that protective measure:\n\n\tgit clone --config core.hooksPath=/dev/null <url>\n\nSince it turns out that the benefit does not justify the cost, let's revert\n20f3588efc6 (core.hooksPath: add some protection while cloning,\n2024-03-30).\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n config.c        | 13 +------------\n t/t1800-hook.sh | 15 ---------------\n 2 files changed, 1 insertion(+), 27 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 85b37f2ee0..8c1c4071f0 100644\n--- a/config.c\n+++ b/config.c\n@@ -1525,19 +1525,8 @@ static int git_default_core_config(const char *var, const char *value, void *cb)\n \tif (!strcmp(var, \"core.attributesfile\"))\n \t\treturn git_config_pathname(&git_attributes_file, var, value);\n \n-\tif (!strcmp(var, \"core.hookspath\")) {\n-\t\tif (current_config_scope() == CONFIG_SCOPE_LOCAL &&\n-\t\t    git_env_bool(\"GIT_CLONE_PROTECTION_ACTIVE\", 0))\n-\t\t\tdie(_(\"active `core.hooksPath` found in the local \"\n-\t\t\t      \"repository config:\\n\\t%s\\nFor security \"\n-\t\t\t      \"reasons, this is disallowed by default.\\nIf \"\n-\t\t\t      \"this is intentional and the hook should \"\n-\t\t\t      \"actually be run, please\\nrun the command \"\n-\t\t\t      \"again with \"\n-\t\t\t      \"`GIT_CLONE_PROTECTION_ACTIVE=false`\"),\n-\t\t\t    value);\n+\tif (!strcmp(var, \"core.hookspath\"))\n \t\treturn git_config_pathname(&git_hooks_path, var, value);\n-\t}\n \n \tif (!strcmp(var, \"core.bare\")) {\n \t\tis_bare_repository_cfg = git_config_bool(var, value);\ndiff --git a/t/t1800-hook.sh b/t/t1800-hook.sh\nindex 7ee12e6f48..2ef3579fa7 100755\n--- a/t/t1800-hook.sh\n+++ b/t/t1800-hook.sh\n@@ -177,19 +177,4 @@ test_expect_success 'git hook run a hook with a bad shebang' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'clone protections' '\n-\ttest_config core.hooksPath \"$(pwd)/my-hooks\" &&\n-\tmkdir -p my-hooks &&\n-\twrite_script my-hooks/test-hook <<-\\EOF &&\n-\techo Hook ran $1\n-\tEOF\n-\n-\tgit hook run test-hook 2>err &&\n-\tgrep \"Hook ran\" err &&\n-\ttest_must_fail env GIT_CLONE_PROTECTION_ACTIVE=true \\\n-\t\tgit hook run test-hook 2>err &&\n-\tgrep \"active .core.hooksPath\" err &&\n-\t! grep \"Hook ran\" err\n-'\n-\n test_done\n-- \n2.45.1-246-gb9cfe4845c\n\n"},{"id":"495597","messageId":"20240524194715.695916-10-gitster@pobox.com","threadId":"61544","inReplyTo":"20240524194715.695916-1-gitster@pobox.com","subject":"[PATCH v2 09/12] tests: verify that `clone -c core.hooksPath=/dev/null` works again","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-24T19:47:12Z","receivedAt":"2024-05-24T19:47:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nAs part of the protections added in Git v2.45.1 and friends,\nrepository-local `core.hooksPath` settings are no longer allowed, as a\ndefense-in-depth mechanism to prevent future Git vulnerabilities to\nraise to critical level if those vulnerabilities inadvertently allow the\nrepository-local config to be written.\n\nWhat the added protection did not anticipate is that such a\nrepository-local `core.hooksPath` can not only be used to point to\nmaliciously-placed scripts in the current worktree, but also to\n_prevent_ hooks from being called altogether.\n\nWe just reverted the `core.hooksPath` protections, based on the Git\nmaintainer's recommendation in\nhttps://lore.kernel.org/git/xmqq4jaxvm8z.fsf@gitster.g/ to address this\nconcern as well as related ones. Let's make sure that we won't regress\nwhile trying to protect the clone operation further.\n\nReported-by: Brooke Kuhlmann <brooke@alchemists.io>\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n t/t1350-config-hooks-path.sh | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/t/t1350-config-hooks-path.sh b/t/t1350-config-hooks-path.sh\nindex f6dc83e2aa..45a0492917 100755\n--- a/t/t1350-config-hooks-path.sh\n+++ b/t/t1350-config-hooks-path.sh\n@@ -41,4 +41,11 @@ test_expect_success 'git rev-parse --git-path hooks' '\n \ttest .git/custom-hooks/abc = \"$(cat actual)\"\n '\n \n+test_expect_success 'core.hooksPath=/dev/null' '\n+\tgit clone -c core.hooksPath=/dev/null . no-templates &&\n+\tvalue=\"$(git -C no-templates config --local core.hooksPath)\" &&\n+\t# The Bash used by Git for Windows rewrites `/dev/null` to `nul`\n+\t{ test /dev/null = \"$value\" || test nul = \"$value\"; }\n+'\n+\n test_done\n-- \n2.45.1-246-gb9cfe4845c\n\n"},{"id":"495598","messageId":"20240524194715.695916-11-gitster@pobox.com","threadId":"61544","inReplyTo":"20240524194715.695916-1-gitster@pobox.com","subject":"[PATCH v2 10/12] clone: drop the protections where hooks aren't run","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-24T19:47:13Z","receivedAt":"2024-05-24T19:47:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nAs part of the security bug-fix releases v2.39.4, ..., v2.45.1, I\nintroduced logic to safeguard `git clone` from running hooks that were\ninstalled _during_ the clone operation.\n\nThe rationale was that Git's CVE-2024-32002, CVE-2021-21300,\nCVE-2019-1354, CVE-2019-1353, CVE-2019-1352, and CVE-2019-1349 should\nhave been low-severity vulnerabilities but were elevated to\ncritical/high severity by the attack vector that allows a weakness where\nfiles inside `.git/` can be inadvertently written during a `git clone`\nto escalate to a Remote Code Execution attack by virtue of installing a\nmalicious `post-checkout` hook that Git will then run at the end of the\noperation without giving the user a chance to see what code is executed.\n\nUnfortunately, Git LFS uses a similar strategy to install its own\n`post-checkout` hook during a `git clone`; In fact, Git LFS is\ninstalling four separate hooks while running the `smudge` filter.\n\nWhile this pattern is probably in want of being improved by introducing\nbetter support in Git for Git LFS and other tools wishing to register\nhooks to be run at various stages of Git's commands, let's undo the\nclone protections to unbreak Git LFS-enabled clones.\n\nThis reverts commit 8db1e8743c0 (clone: prevent hooks from running\nduring a clone, 2024-03-28).\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/clone.c  | 12 +-----------\n hook.c           | 34 --------------------------------\n t/t5601-clone.sh | 51 ------------------------------------------------\n 3 files changed, 1 insertion(+), 96 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex e7721f5c22..9ec500d427 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -937,8 +937,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tint err = 0, complete_refs_before_fetch = 1;\n \tint submodule_progress;\n \tint filter_submodules = 0;\n-\tconst char *template_dir;\n-\tchar *template_dir_dup = NULL;\n \n \tstruct transport_ls_refs_options transport_ls_refs_options =\n \t\tTRANSPORT_LS_REFS_OPTIONS_INIT;\n@@ -958,13 +956,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\tusage_msg_opt(_(\"You must specify a repository to clone.\"),\n \t\t\tbuiltin_clone_usage, builtin_clone_options);\n \n-\txsetenv(\"GIT_CLONE_PROTECTION_ACTIVE\", \"true\", 0 /* allow user override */);\n-\ttemplate_dir = get_template_dir(option_template);\n-\tif (*template_dir && !is_absolute_path(template_dir))\n-\t\ttemplate_dir = template_dir_dup =\n-\t\t\tabsolute_pathdup(template_dir);\n-\txsetenv(\"GIT_CLONE_TEMPLATE_DIR\", template_dir, 1);\n-\n \tif (option_depth || option_since || option_not.nr)\n \t\tdeepen = 1;\n \tif (option_single_branch == -1)\n@@ -1112,7 +1103,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n-\tinit_db(git_dir, real_git_dir, template_dir, GIT_HASH_UNKNOWN, NULL,\n+\tinit_db(git_dir, real_git_dir, option_template, GIT_HASH_UNKNOWN, NULL,\n \t\tINIT_DB_QUIET);\n \n \tif (real_git_dir) {\n@@ -1430,7 +1421,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tfree(unborn_head);\n \tfree(dir);\n \tfree(path);\n-\tfree(template_dir_dup);\n \tUNLEAK(repo);\n \tjunk_mode = JUNK_LEAVE_ALL;\n \ndiff --git a/hook.c b/hook.c\nindex fc974cee1d..22b274b60b 100644\n--- a/hook.c\n+++ b/hook.c\n@@ -3,32 +3,6 @@\n #include \"run-command.h\"\n #include \"config.h\"\n \n-static int identical_to_template_hook(const char *name, const char *path)\n-{\n-\tconst char *env = getenv(\"GIT_CLONE_TEMPLATE_DIR\");\n-\tconst char *template_dir = get_template_dir(env && *env ? env : NULL);\n-\tstruct strbuf template_path = STRBUF_INIT;\n-\tint found_template_hook, ret;\n-\n-\tstrbuf_addf(&template_path, \"%s/hooks/%s\", template_dir, name);\n-\tfound_template_hook = access(template_path.buf, X_OK) >= 0;\n-#ifdef STRIP_EXTENSION\n-\tif (!found_template_hook) {\n-\t\tstrbuf_addstr(&template_path, STRIP_EXTENSION);\n-\t\tfound_template_hook = access(template_path.buf, X_OK) >= 0;\n-\t}\n-#endif\n-\tif (!found_template_hook) {\n-\t\tstrbuf_release(&template_path);\n-\t\treturn 0;\n-\t}\n-\n-\tret = do_files_match(template_path.buf, path);\n-\n-\tstrbuf_release(&template_path);\n-\treturn ret;\n-}\n-\n const char *find_hook(const char *name)\n {\n \tstatic struct strbuf path = STRBUF_INIT;\n@@ -64,14 +38,6 @@ const char *find_hook(const char *name)\n \t\t}\n \t\treturn NULL;\n \t}\n-\tif (!git_hooks_path && git_env_bool(\"GIT_CLONE_PROTECTION_ACTIVE\", 0) &&\n-\t    !identical_to_template_hook(name, path.buf))\n-\t\tdie(_(\"active `%s` hook found during `git clone`:\\n\\t%s\\n\"\n-\t\t      \"For security reasons, this is disallowed by default.\\n\"\n-\t\t      \"If this is intentional and the hook should actually \"\n-\t\t      \"be run, please\\nrun the command again with \"\n-\t\t      \"`GIT_CLONE_PROTECTION_ACTIVE=false`\"),\n-\t\t    name, path.buf);\n \treturn path.buf;\n }\n \ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 20deca0231..fd02984330 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -771,57 +771,6 @@ test_expect_success 'batch missing blob request does not inadvertently try to fe\n \tgit clone --filter=blob:limit=0 \"file://$(pwd)/server\" client\n '\n \n-test_expect_success 'clone with init.templatedir runs hooks' '\n-\tgit init tmpl/hooks &&\n-\twrite_script tmpl/hooks/post-checkout <<-EOF &&\n-\techo HOOK-RUN >&2\n-\techo I was here >hook.run\n-\tEOF\n-\tgit -C tmpl/hooks add . &&\n-\ttest_tick &&\n-\tgit -C tmpl/hooks commit -m post-checkout &&\n-\n-\ttest_when_finished \"git config --global --unset init.templateDir || :\" &&\n-\ttest_when_finished \"git config --unset init.templateDir || :\" &&\n-\t(\n-\t\tsane_unset GIT_TEMPLATE_DIR &&\n-\t\tNO_SET_GIT_TEMPLATE_DIR=t &&\n-\t\texport NO_SET_GIT_TEMPLATE_DIR &&\n-\n-\t\tgit -c core.hooksPath=\"$(pwd)/tmpl/hooks\" \\\n-\t\t\tclone tmpl/hooks hook-run-hookspath 2>err &&\n-\t\t! grep \"active .* hook found\" err &&\n-\t\ttest_path_is_file hook-run-hookspath/hook.run &&\n-\n-\t\tgit -c init.templateDir=\"$(pwd)/tmpl\" \\\n-\t\t\tclone tmpl/hooks hook-run-config 2>err &&\n-\t\t! grep \"active .* hook found\" err &&\n-\t\ttest_path_is_file hook-run-config/hook.run &&\n-\n-\t\tgit clone --template=tmpl tmpl/hooks hook-run-option 2>err &&\n-\t\t! grep \"active .* hook found\" err &&\n-\t\ttest_path_is_file hook-run-option/hook.run &&\n-\n-\t\tgit config --global init.templateDir \"$(pwd)/tmpl\" &&\n-\t\tgit clone tmpl/hooks hook-run-global-config 2>err &&\n-\t\tgit config --global --unset init.templateDir &&\n-\t\t! grep \"active .* hook found\" err &&\n-\t\ttest_path_is_file hook-run-global-config/hook.run &&\n-\n-\t\t# clone ignores local `init.templateDir`; need to create\n-\t\t# a new repository because we deleted `.git/` in the\n-\t\t# `setup` test case above\n-\t\tgit init local-clone &&\n-\t\tcd local-clone &&\n-\n-\t\tgit config init.templateDir \"$(pwd)/../tmpl\" &&\n-\t\tgit clone ../tmpl/hooks hook-run-local-config 2>err &&\n-\t\tgit config --unset init.templateDir &&\n-\t\t! grep \"active .* hook found\" err &&\n-\t\ttest_path_is_missing hook-run-local-config/hook.run\n-\t)\n-'\n-\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n-- \n2.45.1-246-gb9cfe4845c\n\n"},{"id":"495599","messageId":"20240524194715.695916-12-gitster@pobox.com","threadId":"61544","inReplyTo":"20240524194715.695916-1-gitster@pobox.com","subject":"[PATCH v2 11/12] Revert \"Add a helper function to compare file contents\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-24T19:47:14Z","receivedAt":"2024-05-24T19:47:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nNow that during a `git clone`, the hooks' contents are no longer\ncompared to the templates' files', the caller for which the\n`do_files_match()` function was introduced is gone, and therefore this\nfunction can be retired, too.\n\nThis reverts commit 584de0b4c23 (Add a helper function to compare file\ncontents, 2024-03-30).\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n cache.h                    | 14 ---------\n copy.c                     | 58 --------------------------------------\n t/helper/test-path-utils.c | 10 -------\n t/t0060-path-utils.sh      | 41 ---------------------------\n 4 files changed, 123 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 16b34799bf..8c5fb1e1ba 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1785,20 +1785,6 @@ int copy_fd(int ifd, int ofd);\n int copy_file(const char *dst, const char *src, int mode);\n int copy_file_with_time(const char *dst, const char *src, int mode);\n \n-/*\n- * Compare the file mode and contents of two given files.\n- *\n- * If both files are actually symbolic links, the function returns 1 if the link\n- * targets are identical or 0 if they are not.\n- *\n- * If any of the two files cannot be accessed or in case of read failures, this\n- * function returns 0.\n- *\n- * If the file modes and contents are identical, the function returns 1,\n- * otherwise it returns 0.\n- */\n-int do_files_match(const char *path1, const char *path2);\n-\n void write_or_die(int fd, const void *buf, size_t count);\n void fsync_or_die(int fd, const char *);\n int fsync_component(enum fsync_component component, int fd);\ndiff --git a/copy.c b/copy.c\nindex 8492f6fc83..4de6a110f0 100644\n--- a/copy.c\n+++ b/copy.c\n@@ -65,61 +65,3 @@ int copy_file_with_time(const char *dst, const char *src, int mode)\n \t\treturn copy_times(dst, src);\n \treturn status;\n }\n-\n-static int do_symlinks_match(const char *path1, const char *path2)\n-{\n-\tstruct strbuf buf1 = STRBUF_INIT, buf2 = STRBUF_INIT;\n-\tint ret = 0;\n-\n-\tif (!strbuf_readlink(&buf1, path1, 0) &&\n-\t    !strbuf_readlink(&buf2, path2, 0))\n-\t\tret = !strcmp(buf1.buf, buf2.buf);\n-\n-\tstrbuf_release(&buf1);\n-\tstrbuf_release(&buf2);\n-\treturn ret;\n-}\n-\n-int do_files_match(const char *path1, const char *path2)\n-{\n-\tstruct stat st1, st2;\n-\tint fd1 = -1, fd2 = -1, ret = 1;\n-\tchar buf1[8192], buf2[8192];\n-\n-\tif ((fd1 = open_nofollow(path1, O_RDONLY)) < 0 ||\n-\t    fstat(fd1, &st1) || !S_ISREG(st1.st_mode)) {\n-\t\tif (fd1 < 0 && errno == ELOOP)\n-\t\t\t/* maybe this is a symbolic link? */\n-\t\t\treturn do_symlinks_match(path1, path2);\n-\t\tret = 0;\n-\t} else if ((fd2 = open_nofollow(path2, O_RDONLY)) < 0 ||\n-\t\t   fstat(fd2, &st2) || !S_ISREG(st2.st_mode)) {\n-\t\tret = 0;\n-\t}\n-\n-\tif (ret)\n-\t\t/* to match, neither must be executable, or both */\n-\t\tret = !(st1.st_mode & 0111) == !(st2.st_mode & 0111);\n-\n-\tif (ret)\n-\t\tret = st1.st_size == st2.st_size;\n-\n-\twhile (ret) {\n-\t\tssize_t len1 = read_in_full(fd1, buf1, sizeof(buf1));\n-\t\tssize_t len2 = read_in_full(fd2, buf2, sizeof(buf2));\n-\n-\t\tif (len1 < 0 || len2 < 0 || len1 != len2)\n-\t\t\tret = 0; /* read error or different file size */\n-\t\telse if (!len1) /* len2 is also 0; hit EOF on both */\n-\t\t\tbreak; /* ret is still true */\n-\t\telse\n-\t\t\tret = !memcmp(buf1, buf2, len1);\n-\t}\n-\n-\tif (fd1 >= 0)\n-\t\tclose(fd1);\n-\tif (fd2 >= 0)\n-\t\tclose(fd2);\n-\n-\treturn ret;\n-}\ndiff --git a/t/helper/test-path-utils.c b/t/helper/test-path-utils.c\nindex 0e0de21807..f69709d674 100644\n--- a/t/helper/test-path-utils.c\n+++ b/t/helper/test-path-utils.c\n@@ -495,16 +495,6 @@ int cmd__path_utils(int argc, const char **argv)\n \t\treturn !!res;\n \t}\n \n-\tif (argc == 4 && !strcmp(argv[1], \"do_files_match\")) {\n-\t\tint ret = do_files_match(argv[2], argv[3]);\n-\n-\t\tif (ret)\n-\t\t\tprintf(\"equal\\n\");\n-\t\telse\n-\t\t\tprintf(\"different\\n\");\n-\t\treturn !ret;\n-\t}\n-\n \tfprintf(stderr, \"%s: unknown function name: %s\\n\", argv[0],\n \t\targv[1] ? argv[1] : \"(there was none)\");\n \treturn 1;\ndiff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh\nindex 73d0e1a7f1..68e29c904a 100755\n--- a/t/t0060-path-utils.sh\n+++ b/t/t0060-path-utils.sh\n@@ -560,45 +560,4 @@ test_expect_success !VALGRIND,RUNTIME_PREFIX,CAN_EXEC_IN_PWD '%(prefix)/ works'\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'do_files_match()' '\n-\ttest_seq 0 10 >0-10.txt &&\n-\ttest_seq -1 10 >-1-10.txt &&\n-\ttest_seq 1 10 >1-10.txt &&\n-\ttest_seq 1 9 >1-9.txt &&\n-\ttest_seq 0 8 >0-8.txt &&\n-\n-\ttest-tool path-utils do_files_match 0-10.txt 0-10.txt >out &&\n-\n-\tassert_fails() {\n-\t\ttest_must_fail \\\n-\t\ttest-tool path-utils do_files_match \"$1\" \"$2\" >out &&\n-\t\tgrep different out\n-\t} &&\n-\n-\tassert_fails 0-8.txt 1-9.txt &&\n-\tassert_fails -1-10.txt 0-10.txt &&\n-\tassert_fails 1-10.txt 1-9.txt &&\n-\tassert_fails 1-10.txt .git &&\n-\tassert_fails does-not-exist 1-10.txt &&\n-\n-\tif test_have_prereq FILEMODE\n-\tthen\n-\t\tcp 0-10.txt 0-10.x &&\n-\t\tchmod a+x 0-10.x &&\n-\t\tassert_fails 0-10.txt 0-10.x\n-\tfi &&\n-\n-\tif test_have_prereq SYMLINKS\n-\tthen\n-\t\tln -sf 0-10.txt symlink &&\n-\t\tln -s 0-10.txt another-symlink &&\n-\t\tln -s over-the-ocean yet-another-symlink &&\n-\t\tln -s \"$PWD/0-10.txt\" absolute-symlink &&\n-\t\tassert_fails 0-10.txt symlink &&\n-\t\ttest-tool path-utils do_files_match symlink another-symlink &&\n-\t\tassert_fails symlink yet-another-symlink &&\n-\t\tassert_fails symlink absolute-symlink\n-\tfi\n-'\n-\n test_done\n-- \n2.45.1-246-gb9cfe4845c\n\n"},{"id":"495600","messageId":"20240524194715.695916-13-gitster@pobox.com","threadId":"61544","inReplyTo":"20240524194715.695916-1-gitster@pobox.com","subject":"[PATCH v2 12/12] Revert \"fsck: warn about symlink pointing inside a gitdir\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-24T19:47:15Z","receivedAt":"2024-05-24T19:47:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This reverts commit a33fea08 (fsck: warn about symlink pointing\ninside a gitdir, 2024-04-10), which warns against symbolic links\ncommonly created by git-annex.\n---\n Documentation/fsck-msgids.txt | 12 --------\n fsck.c                        | 56 -----------------------------------\n fsck.h                        | 12 --------\n t/t1450-fsck.sh               | 37 -----------------------\n 4 files changed, 117 deletions(-)\n\ndiff --git a/Documentation/fsck-msgids.txt b/Documentation/fsck-msgids.txt\nindex b06ec385af..12eae8a222 100644\n--- a/Documentation/fsck-msgids.txt\n+++ b/Documentation/fsck-msgids.txt\n@@ -157,18 +157,6 @@\n `nullSha1`::\n \t(WARN) Tree contains entries pointing to a null sha1.\n \n-`symlinkPointsToGitDir`::\n-\t(WARN) Symbolic link points inside a gitdir.\n-\n-`symlinkTargetBlob`::\n-\t(ERROR) A non-blob found instead of a symbolic link's target.\n-\n-`symlinkTargetLength`::\n-\t(WARN) Symbolic link target longer than maximum path length.\n-\n-`symlinkTargetMissing`::\n-\t(ERROR) Unable to read symbolic link target's blob.\n-\n `treeNotSorted`::\n \t(ERROR) A tree is not properly sorted.\n \ndiff --git a/fsck.c b/fsck.c\nindex b85868e122..47eaeedd70 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -636,8 +636,6 @@ static int fsck_tree(const struct object_id *tree_oid,\n \t\t\t\tretval += report(options, tree_oid, OBJ_TREE,\n \t\t\t\t\t\t FSCK_MSG_MAILMAP_SYMLINK,\n \t\t\t\t\t\t \".mailmap is a symlink\");\n-\t\t\toidset_insert(&options->symlink_targets_found,\n-\t\t\t\t      entry_oid);\n \t\t}\n \n \t\tif ((backslash = strchr(name, '\\\\'))) {\n@@ -1230,56 +1228,6 @@ static int fsck_blob(const struct object_id *oid, const char *buf,\n \t\t}\n \t}\n \n-\tif (oidset_contains(&options->symlink_targets_found, oid)) {\n-\t\tconst char *ptr = buf;\n-\t\tconst struct object_id *reported = NULL;\n-\n-\t\toidset_insert(&options->symlink_targets_done, oid);\n-\n-\t\tif (!buf || size > PATH_MAX) {\n-\t\t\t/*\n-\t\t\t * A missing buffer here is a sign that the caller found the\n-\t\t\t * blob too gigantic to load into memory. Let's just consider\n-\t\t\t * that an error.\n-\t\t\t */\n-\t\t\treturn report(options, oid, OBJ_BLOB,\n-\t\t\t\t\tFSCK_MSG_SYMLINK_TARGET_LENGTH,\n-\t\t\t\t\t\"symlink target too long\");\n-\t\t}\n-\n-\t\twhile (!reported && ptr) {\n-\t\t\tconst char *p = ptr;\n-\t\t\tchar c, *slash = strchrnul(ptr, '/');\n-\t\t\tchar *backslash = memchr(ptr, '\\\\', slash - ptr);\n-\n-\t\t\tc = *slash;\n-\t\t\t*slash = '\\0';\n-\n-\t\t\twhile (!reported && backslash) {\n-\t\t\t\t*backslash = '\\0';\n-\t\t\t\tif (is_ntfs_dotgit(p))\n-\t\t\t\t\tret |= report(options, reported = oid, OBJ_BLOB,\n-\t\t\t\t\t\t      FSCK_MSG_SYMLINK_POINTS_TO_GIT_DIR,\n-\t\t\t\t\t\t      \"symlink target points to git dir\");\n-\t\t\t\t*backslash = '\\\\';\n-\t\t\t\tp = backslash + 1;\n-\t\t\t\tbackslash = memchr(p, '\\\\', slash - p);\n-\t\t\t}\n-\t\t\tif (!reported && is_ntfs_dotgit(p))\n-\t\t\t\tret |= report(options, reported = oid, OBJ_BLOB,\n-\t\t\t\t\t      FSCK_MSG_SYMLINK_POINTS_TO_GIT_DIR,\n-\t\t\t\t\t      \"symlink target points to git dir\");\n-\n-\t\t\tif (!reported && is_hfs_dotgit(ptr))\n-\t\t\t\tret |= report(options, reported = oid, OBJ_BLOB,\n-\t\t\t\t\t      FSCK_MSG_SYMLINK_POINTS_TO_GIT_DIR,\n-\t\t\t\t\t      \"symlink target points to git dir\");\n-\n-\t\t\t*slash = c;\n-\t\t\tptr = c ? slash + 1 : NULL;\n-\t\t}\n-\t}\n-\n \treturn ret;\n }\n \n@@ -1371,10 +1319,6 @@ int fsck_finish(struct fsck_options *options)\n \t\t\t  FSCK_MSG_GITATTRIBUTES_MISSING, FSCK_MSG_GITATTRIBUTES_BLOB,\n \t\t\t  options, \".gitattributes\");\n \n-\tret |= fsck_blobs(&options->symlink_targets_found, &options->symlink_targets_done,\n-\t\t\t  FSCK_MSG_SYMLINK_TARGET_MISSING, FSCK_MSG_SYMLINK_TARGET_BLOB,\n-\t\t\t  options, \"<symlink-target>\");\n-\n \treturn ret;\n }\n \ndiff --git a/fsck.h b/fsck.h\nindex 130fa8d8f9..fcecf4101c 100644\n--- a/fsck.h\n+++ b/fsck.h\n@@ -63,8 +63,6 @@ enum fsck_msg_type {\n \tFUNC(GITATTRIBUTES_LARGE, ERROR) \\\n \tFUNC(GITATTRIBUTES_LINE_LENGTH, ERROR) \\\n \tFUNC(GITATTRIBUTES_BLOB, ERROR) \\\n-\tFUNC(SYMLINK_TARGET_MISSING, ERROR) \\\n-\tFUNC(SYMLINK_TARGET_BLOB, ERROR) \\\n \t/* warnings */ \\\n \tFUNC(EMPTY_NAME, WARN) \\\n \tFUNC(FULL_PATHNAME, WARN) \\\n@@ -74,8 +72,6 @@ enum fsck_msg_type {\n \tFUNC(NULL_SHA1, WARN) \\\n \tFUNC(ZERO_PADDED_FILEMODE, WARN) \\\n \tFUNC(NUL_IN_COMMIT, WARN) \\\n-\tFUNC(SYMLINK_TARGET_LENGTH, WARN) \\\n-\tFUNC(SYMLINK_POINTS_TO_GIT_DIR, WARN) \\\n \t/* infos (reported as warnings, but ignored by default) */ \\\n \tFUNC(BAD_FILEMODE, INFO) \\\n \tFUNC(GITMODULES_PARSE, INFO) \\\n@@ -143,8 +139,6 @@ struct fsck_options {\n \tstruct oidset gitmodules_done;\n \tstruct oidset gitattributes_found;\n \tstruct oidset gitattributes_done;\n-\tstruct oidset symlink_targets_found;\n-\tstruct oidset symlink_targets_done;\n \tkh_oid_map_t *object_names;\n };\n \n@@ -154,8 +148,6 @@ struct fsck_options {\n \t.gitmodules_done = OIDSET_INIT, \\\n \t.gitattributes_found = OIDSET_INIT, \\\n \t.gitattributes_done = OIDSET_INIT, \\\n-\t.symlink_targets_found = OIDSET_INIT, \\\n-\t.symlink_targets_done = OIDSET_INIT, \\\n \t.error_func = fsck_error_function \\\n }\n #define FSCK_OPTIONS_STRICT { \\\n@@ -164,8 +156,6 @@ struct fsck_options {\n \t.gitmodules_done = OIDSET_INIT, \\\n \t.gitattributes_found = OIDSET_INIT, \\\n \t.gitattributes_done = OIDSET_INIT, \\\n-\t.symlink_targets_found = OIDSET_INIT, \\\n-\t.symlink_targets_done = OIDSET_INIT, \\\n \t.error_func = fsck_error_function, \\\n }\n #define FSCK_OPTIONS_MISSING_GITMODULES { \\\n@@ -174,8 +164,6 @@ struct fsck_options {\n \t.gitmodules_done = OIDSET_INIT, \\\n \t.gitattributes_found = OIDSET_INIT, \\\n \t.gitattributes_done = OIDSET_INIT, \\\n-\t.symlink_targets_found = OIDSET_INIT, \\\n-\t.symlink_targets_done = OIDSET_INIT, \\\n \t.error_func = fsck_error_cb_print_missing_gitmodules, \\\n }\n \ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex 5669872bc8..de0f6d5e7f 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -1023,41 +1023,4 @@ test_expect_success 'fsck error on gitattributes with excessive size' '\n \ttest_cmp expected actual\n '\n \n-test_expect_success 'fsck warning on symlink target with excessive length' '\n-\tsymlink_target=$(printf \"pattern %032769d\" 1 | git hash-object -w --stdin) &&\n-\ttest_when_finished \"remove_object $symlink_target\" &&\n-\ttree=$(printf \"120000 blob %s\\t%s\\n\" $symlink_target symlink | git mktree) &&\n-\ttest_when_finished \"remove_object $tree\" &&\n-\tcat >expected <<-EOF &&\n-\twarning in blob $symlink_target: symlinkTargetLength: symlink target too long\n-\tEOF\n-\tgit fsck --no-dangling >actual 2>&1 &&\n-\ttest_cmp expected actual\n-'\n-\n-test_expect_success 'fsck warning on symlink target pointing inside git dir' '\n-\tgitdir=$(printf \".git\" | git hash-object -w --stdin) &&\n-\tntfs_gitdir=$(printf \"GIT~1\" | git hash-object -w --stdin) &&\n-\thfs_gitdir=$(printf \".${u200c}git\" | git hash-object -w --stdin) &&\n-\tinside_gitdir=$(printf \"nested/.git/config\" | git hash-object -w --stdin) &&\n-\tbenign_target=$(printf \"legit/config\" | git hash-object -w --stdin) &&\n-\ttree=$(printf \"120000 blob %s\\t%s\\n\" \\\n-\t\t$benign_target benign_target \\\n-\t\t$gitdir gitdir \\\n-\t\t$hfs_gitdir hfs_gitdir \\\n-\t\t$inside_gitdir inside_gitdir \\\n-\t\t$ntfs_gitdir ntfs_gitdir |\n-\t\tgit mktree) &&\n-\tfor o in $gitdir $ntfs_gitdir $hfs_gitdir $inside_gitdir $benign_target $tree\n-\tdo\n-\t\ttest_when_finished \"remove_object $o\" || return 1\n-\tdone &&\n-\tprintf \"warning in blob %s: symlinkPointsToGitDir: symlink target points to git dir\\n\" \\\n-\t\t$gitdir $hfs_gitdir $inside_gitdir $ntfs_gitdir |\n-\tsort >expected &&\n-\tgit fsck --no-dangling >actual 2>&1 &&\n-\tsort actual >actual.sorted &&\n-\ttest_cmp expected actual.sorted\n-'\n-\n test_done\n-- \n2.45.1-246-gb9cfe4845c\n\n"}]}