[PATCH 00/12] Fix various overly aggressive protections in 2.45.1 and friends

git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [PATCH 00/12] Fix various overly aggressive protections in 2.45.1 and friends
@ 2024-05-21 19:56 Junio C Hamano
  2024-05-21 19:56 ` [PATCH 01/12] send-email: drop FakeTerm hack Junio C Hamano
                   ` (15 more replies)
  0 siblings, 16 replies; 36+ messages in thread
From: Junio C Hamano @ 2024-05-21 19:56 UTC (permalink / raw)
  To: git

As people have seen, the latest "security fix" release turned out to
be a mixed bag of good vulnerability fixes with a bit over-eager
"layered defence" that broke real uses cases like git-lfs.  Let's
quickly get them in working order back first, with the vision that
we will then rebuild layered defence more carefully in the open on
top as necessary.

What we have here are the first "revert" part.

These patches are designed to apply to 2.39.4; the series may have
to grow as we discover more things to revert, but for now here are
the patches to

 - revert the over-eager "refusal to work" went into 2.39.4

 - adjust 2.39.4 codebase to cleanly build and test (at CI and
   locally) by backported fixes

It would have been better if we did not have to have the latter
class, but such is life.

I'll figure out a way to convey conflict resolutions as this topic
gets merged up to newer maintenance tracks on the list so that
people can assist with ensuring correctness of the result by
reviewing, and follow up. ("git show --remerge-diff" might turn out
to be such a way, but I do not know yet).


Jeff King (5):
  send-email: drop FakeTerm hack
  send-email: avoid creating more than one Term::ReadLine object
  ci: drop mention of BREW_INSTALL_PACKAGES variable
  ci: avoid bare "gcc" for osx-gcc job
  ci: stop installing "gcc-13" for osx-gcc

Johannes Schindelin (6):
  hook: plug a new memory leak
  init: use the correct path of the templates directory again
  Revert "core.hooksPath: add some protection while cloning"
  tests: verify that `clone -c core.hooksPath=/dev/null` works again
  clone: drop the protections where hooks aren't run
  Revert "Add a helper function to compare file contents"

Junio C Hamano (1):
  Revert "fetch/clone: detect dubious ownership of local repositories"

 .github/workflows/main.yml    |  3 +-
 Makefile                      |  2 +-
 builtin/clone.c               | 12 +-------
 cache.h                       | 14 ---------
 ci/install-dependencies.sh    |  2 --
 config.c                      | 13 +-------
 copy.c                        | 58 -----------------------------------
 git-send-email.perl           | 32 +++++++------------
 hook.c                        | 32 -------------------
 path.c                        |  2 --
 t/helper/test-path-utils.c    | 10 ------
 t/t0060-path-utils.sh         | 41 -------------------------
 t/t0411-clone-from-partial.sh |  6 ++--
 t/t1350-config-hooks-path.sh  |  7 +++++
 t/t1800-hook.sh               | 15 ---------
 t/t5601-clone.sh              | 51 ------------------------------
 t/t9001-send-email.sh         |  5 +--
 17 files changed, 28 insertions(+), 277 deletions(-)

-- 
2.45.1-216-g4365c6fcf9


^ permalink raw reply	[flat|nested] 36+ messages in thread

* [PATCH 01/12] send-email: drop FakeTerm hack
  2024-05-21 19:56 [PATCH 00/12] Fix various overly aggressive protections in 2.45.1 and friends Junio C Hamano
@ 2024-05-21 19:56 ` Junio C Hamano
  2024-05-22  8:19   ` Dragan Simic
  2024-05-21 19:56 ` [PATCH 02/12] send-email: avoid creating more than one Term::ReadLine object Junio C Hamano
                   ` (14 subsequent siblings)
  15 siblings, 1 reply; 36+ messages in thread
From: Junio C Hamano @ 2024-05-21 19:56 UTC (permalink / raw)
  To: git; +Cc: Jeff King, Taylor Blau

From: Jeff King <peff@peff.net>

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 <peff@peff.net>
Acked-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 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 @@

 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 <<EOT;
 git send-email' [<options>] <file|directory>
@@ -930,16 +918,10 @@ sub get_patch_subject {
 }

 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 {
-- 
2.45.1-216-g4365c6fcf9

^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH 02/12] send-email: avoid creating more than one Term::ReadLine object
  2024-05-21 19:56 [PATCH 00/12] Fix various overly aggressive protections in 2.45.1 and friends Junio C Hamano
  2024-05-21 19:56 ` [PATCH 01/12] send-email: drop FakeTerm hack Junio C Hamano
@ 2024-05-21 19:56 ` Junio C Hamano
  2024-05-22  8:15   ` Dragan Simic
  2024-05-21 19:56 ` [PATCH 03/12] ci: drop mention of BREW_INSTALL_PACKAGES variable Junio C Hamano
                   ` (13 subsequent siblings)
  15 siblings, 1 reply; 36+ messages in thread
From: Junio C Hamano @ 2024-05-21 19:56 UTC (permalink / raw)
  To: git; +Cc: Jeff King, Taylor Blau

From: Jeff King <peff@peff.net>

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 <peff@peff.net>
Acked-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 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 @@ sub get_patch_subject {
 	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 <author@example.com>\$" msgtxt1 &&
-		grep "^To: to@example.com\$" msgtxt1
+		grep "^To: to@example.com\$" msgtxt1 &&
+		grep "^In-Reply-To: <my-message-id@example.com>" msgtxt1
 '
 
 test_expect_success $PREREQ,AUTOIDENT 'implicit ident is allowed' '
-- 
2.45.1-216-g4365c6fcf9


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH 03/12] ci: drop mention of BREW_INSTALL_PACKAGES variable
  2024-05-21 19:56 [PATCH 00/12] Fix various overly aggressive protections in 2.45.1 and friends Junio C Hamano
  2024-05-21 19:56 ` [PATCH 01/12] send-email: drop FakeTerm hack Junio C Hamano
  2024-05-21 19:56 ` [PATCH 02/12] send-email: avoid creating more than one Term::ReadLine object Junio C Hamano
@ 2024-05-21 19:56 ` Junio C Hamano
  2024-05-21 19:56 ` [PATCH 04/12] ci: avoid bare "gcc" for osx-gcc job Junio C Hamano
                   ` (12 subsequent siblings)
  15 siblings, 0 replies; 36+ messages in thread
From: Junio C Hamano @ 2024-05-21 19:56 UTC (permalink / raw)
  To: git; +Cc: Jeff King

From: Jeff King <peff@peff.net>

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 <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 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
 	(
-- 
2.45.1-216-g4365c6fcf9


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH 04/12] ci: avoid bare "gcc" for osx-gcc job
  2024-05-21 19:56 [PATCH 00/12] Fix various overly aggressive protections in 2.45.1 and friends Junio C Hamano
                   ` (2 preceding siblings ...)
  2024-05-21 19:56 ` [PATCH 03/12] ci: drop mention of BREW_INSTALL_PACKAGES variable Junio C Hamano
@ 2024-05-21 19:56 ` Junio C Hamano
  2024-05-21 19:56 ` [PATCH 05/12] ci: stop installing "gcc-13" for osx-gcc Junio C Hamano
                   ` (11 subsequent siblings)
  15 siblings, 0 replies; 36+ messages in thread
From: Junio C Hamano @ 2024-05-21 19:56 UTC (permalink / raw)
  To: git; +Cc: Jeff King

From: Jeff King <peff@peff.net>

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 <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 .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
-- 
2.45.1-216-g4365c6fcf9

^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH 05/12] ci: stop installing "gcc-13" for osx-gcc
  2024-05-21 19:56 [PATCH 00/12] Fix various overly aggressive protections in 2.45.1 and friends Junio C Hamano
                   ` (3 preceding siblings ...)
  2024-05-21 19:56 ` [PATCH 04/12] ci: avoid bare "gcc" for osx-gcc job Junio C Hamano
@ 2024-05-21 19:56 ` Junio C Hamano
  2024-05-21 19:56 ` [PATCH 06/12] hook: plug a new memory leak Junio C Hamano
                   ` (10 subsequent siblings)
  15 siblings, 0 replies; 36+ messages in thread
From: Junio C Hamano @ 2024-05-21 19:56 UTC (permalink / raw)
  To: git; +Cc: Jeff King

From: Jeff King <peff@peff.net>

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 <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 .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
-- 
2.45.1-216-g4365c6fcf9


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH 06/12] hook: plug a new memory leak
  2024-05-21 19:56 [PATCH 00/12] Fix various overly aggressive protections in 2.45.1 and friends Junio C Hamano
                   ` (4 preceding siblings ...)
  2024-05-21 19:56 ` [PATCH 05/12] ci: stop installing "gcc-13" for osx-gcc Junio C Hamano
@ 2024-05-21 19:56 ` Junio C Hamano
  2024-05-21 19:56 ` [PATCH 07/12] init: use the correct path of the templates directory again Junio C Hamano
                   ` (9 subsequent siblings)
  15 siblings, 0 replies; 36+ messages in thread
From: Junio C Hamano @ 2024-05-21 19:56 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 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);
 
-- 
2.45.1-216-g4365c6fcf9


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH 07/12] init: use the correct path of the templates directory again
  2024-05-21 19:56 [PATCH 00/12] Fix various overly aggressive protections in 2.45.1 and friends Junio C Hamano
                   ` (5 preceding siblings ...)
  2024-05-21 19:56 ` [PATCH 06/12] hook: plug a new memory leak Junio C Hamano
@ 2024-05-21 19:56 ` Junio C Hamano
  2024-05-21 19:56 ` [PATCH 08/12] Revert "core.hooksPath: add some protection while cloning" Junio C Hamano
                   ` (8 subsequent siblings)
  15 siblings, 0 replies; 36+ messages in thread
From: Junio C Hamano @ 2024-05-21 19:56 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 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
-- 
2.45.1-216-g4365c6fcf9


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH 08/12] Revert "core.hooksPath: add some protection while cloning"
  2024-05-21 19:56 [PATCH 00/12] Fix various overly aggressive protections in 2.45.1 and friends Junio C Hamano
                   ` (6 preceding siblings ...)
  2024-05-21 19:56 ` [PATCH 07/12] init: use the correct path of the templates directory again Junio C Hamano
@ 2024-05-21 19:56 ` Junio C Hamano
  2024-05-21 19:56 ` [PATCH 09/12] tests: verify that `clone -c core.hooksPath=/dev/null` works again Junio C Hamano
                   ` (7 subsequent siblings)
  15 siblings, 0 replies; 36+ messages in thread
From: Junio C Hamano @ 2024-05-21 19:56 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <url>

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 <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 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
-- 
2.45.1-216-g4365c6fcf9


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH 09/12] tests: verify that `clone -c core.hooksPath=/dev/null` works again
  2024-05-21 19:56 [PATCH 00/12] Fix various overly aggressive protections in 2.45.1 and friends Junio C Hamano
                   ` (7 preceding siblings ...)
  2024-05-21 19:56 ` [PATCH 08/12] Revert "core.hooksPath: add some protection while cloning" Junio C Hamano
@ 2024-05-21 19:56 ` Junio C Hamano
  2024-05-21 22:57   ` Brooke Kuhlmann
  2024-05-21 19:56 ` [PATCH 10/12] clone: drop the protections where hooks aren't run Junio C Hamano
                   ` (6 subsequent siblings)
  15 siblings, 1 reply; 36+ messages in thread
From: Junio C Hamano @ 2024-05-21 19:56 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Brooke Kuhlmann

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <brooke@alchemists.io>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 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
-- 
2.45.1-216-g4365c6fcf9


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH 10/12] clone: drop the protections where hooks aren't run
  2024-05-21 19:56 [PATCH 00/12] Fix various overly aggressive protections in 2.45.1 and friends Junio C Hamano
                   ` (8 preceding siblings ...)
  2024-05-21 19:56 ` [PATCH 09/12] tests: verify that `clone -c core.hooksPath=/dev/null` works again Junio C Hamano
@ 2024-05-21 19:56 ` Junio C Hamano
  2024-05-21 19:56 ` [PATCH 11/12] Revert "Add a helper function to compare file contents" Junio C Hamano
                   ` (5 subsequent siblings)
  15 siblings, 0 replies; 36+ messages in thread
From: Junio C Hamano @ 2024-05-21 19:56 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 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
 
-- 
2.45.1-216-g4365c6fcf9


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH 11/12] Revert "Add a helper function to compare file contents"
  2024-05-21 19:56 [PATCH 00/12] Fix various overly aggressive protections in 2.45.1 and friends Junio C Hamano
                   ` (9 preceding siblings ...)
  2024-05-21 19:56 ` [PATCH 10/12] clone: drop the protections where hooks aren't run Junio C Hamano
@ 2024-05-21 19:56 ` Junio C Hamano
  2024-05-21 19:56 ` [PATCH 12/12] Revert "fetch/clone: detect dubious ownership of local repositories" Junio C Hamano
                   ` (4 subsequent siblings)
  15 siblings, 0 replies; 36+ messages in thread
From: Junio C Hamano @ 2024-05-21 19:56 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 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
-- 
2.45.1-216-g4365c6fcf9


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH 12/12] Revert "fetch/clone: detect dubious ownership of local repositories"
  2024-05-21 19:56 [PATCH 00/12] Fix various overly aggressive protections in 2.45.1 and friends Junio C Hamano
                   ` (10 preceding siblings ...)
  2024-05-21 19:56 ` [PATCH 11/12] Revert "Add a helper function to compare file contents" Junio C Hamano
@ 2024-05-21 19:56 ` Junio C Hamano
  2024-05-21 20:43   ` Junio C Hamano
  2024-05-21 20:45 ` [rPATCH 13/12] Merge branch 'jc/fix-aggressive-protection-2.39' Junio C Hamano
                   ` (3 subsequent siblings)
  15 siblings, 1 reply; 36+ messages in thread
From: Junio C Hamano @ 2024-05-21 19:56 UTC (permalink / raw)
  To: git

This partially reverts f4aa8c8b (fetch/clone: detect dubious
ownership of local repositories, 2024-04-10) that broke typical
read-only use cases (e.g. by git-daemon serving fetches and clones)
where "nobody" who has no write permission serves repositories owned
by others.  The function to die upon seeing dubious ownership is
still kept, as there are other users of it, but calls to it from the
generic repository discovery code path, which triggered in cases far
wider than originally intended (i.e. to stop local clones), have
been removed.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 path.c                        | 2 --
 t/t0411-clone-from-partial.sh | 6 +++---
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/path.c b/path.c
index d61f70e87d..492e17ad12 100644
--- a/path.c
+++ b/path.c
@@ -840,7 +840,6 @@ const char *enter_repo(const char *path, int strict)
 		if (!suffix[i])
 			return NULL;
 		gitfile = read_gitfile(used_path.buf);
-		die_upon_dubious_ownership(gitfile, NULL, used_path.buf);
 		if (gitfile) {
 			strbuf_reset(&used_path);
 			strbuf_addstr(&used_path, gitfile);
@@ -851,7 +850,6 @@ const char *enter_repo(const char *path, int strict)
 	}
 	else {
 		const char *gitfile = read_gitfile(path);
-		die_upon_dubious_ownership(gitfile, NULL, path);
 		if (gitfile)
 			path = gitfile;
 		if (chdir(path))
diff --git a/t/t0411-clone-from-partial.sh b/t/t0411-clone-from-partial.sh
index b3d6ddc4bc..a481ed97b7 100755
--- a/t/t0411-clone-from-partial.sh
+++ b/t/t0411-clone-from-partial.sh
@@ -23,7 +23,7 @@ test_expect_success 'create evil repo' '
 	>evil/.git/shallow
 '
 
-test_expect_success 'local clone must not fetch from promisor remote and execute script' '
+test_expect_failure 'local clone must not fetch from promisor remote and execute script' '
 	rm -f script-executed &&
 	test_must_fail git clone \
 		--upload-pack="GIT_TEST_ASSUME_DIFFERENT_OWNER=true git-upload-pack" \
@@ -33,7 +33,7 @@ test_expect_success 'local clone must not fetch from promisor remote and execute
 	test_path_is_missing script-executed
 '
 
-test_expect_success 'clone from file://... must not fetch from promisor remote and execute script' '
+test_expect_failure 'clone from file://... must not fetch from promisor remote and execute script' '
 	rm -f script-executed &&
 	test_must_fail git clone \
 		--upload-pack="GIT_TEST_ASSUME_DIFFERENT_OWNER=true git-upload-pack" \
@@ -43,7 +43,7 @@ test_expect_success 'clone from file://... must not fetch from promisor remote a
 	test_path_is_missing script-executed
 '
 
-test_expect_success 'fetch from file://... must not fetch from promisor remote and execute script' '
+test_expect_failure 'fetch from file://... must not fetch from promisor remote and execute script' '
 	rm -f script-executed &&
 	test_must_fail git fetch \
 		--upload-pack="GIT_TEST_ASSUME_DIFFERENT_OWNER=true git-upload-pack" \
-- 
2.45.1-216-g4365c6fcf9


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* Re: [PATCH 12/12] Revert "fetch/clone: detect dubious ownership of local repositories"
  2024-05-21 19:56 ` [PATCH 12/12] Revert "fetch/clone: detect dubious ownership of local repositories" Junio C Hamano
@ 2024-05-21 20:43   ` Junio C Hamano
  2024-05-22  7:27     ` Johannes Schindelin
  0 siblings, 1 reply; 36+ messages in thread
From: Junio C Hamano @ 2024-05-21 20:43 UTC (permalink / raw)
  To: git

Junio C Hamano <gitster@pobox.com> writes:

> This partially reverts f4aa8c8b (fetch/clone: detect dubious
> ownership of local repositories, 2024-04-10) that broke typical
> read-only use cases (e.g. by git-daemon serving fetches and clones)
> where "nobody" who has no write permission serves repositories owned
> by others.  The function to die upon seeing dubious ownership is
> still kept, as there are other users of it, but calls to it from the
> generic repository discovery code path, which triggered in cases far
> wider than originally intended (i.e. to stop local clones), have
> been removed.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---

I am inclined to propose dropping this step, actually.

cf. https://lore.kernel.org/git/xmqq7cfmaofl.fsf@gitster.g/

^ permalink raw reply	[flat|nested] 36+ messages in thread

* [rPATCH 13/12] Merge branch 'jc/fix-aggressive-protection-2.39'
  2024-05-21 19:56 [PATCH 00/12] Fix various overly aggressive protections in 2.45.1 and friends Junio C Hamano
                   ` (11 preceding siblings ...)
  2024-05-21 19:56 ` [PATCH 12/12] Revert "fetch/clone: detect dubious ownership of local repositories" Junio C Hamano
@ 2024-05-21 20:45 ` Junio C Hamano
  2024-05-23 10:36   ` Reviewing merge commits, was " Johannes Schindelin
  2024-05-21 20:45 ` [rPATCH 14/12] Merge branch 'jc/fix-aggressive-protection-2.40' Junio C Hamano
                   ` (2 subsequent siblings)
  15 siblings, 1 reply; 36+ messages in thread
From: Junio C Hamano @ 2024-05-21 20:45 UTC (permalink / raw)
  To: git

This is "git show --remerge-diff" of the result of adjusting the
jc/fix-aggressive-protection-2.39 topic (which you just saw) into
maint-2.40 track.
The result is called jc/fix-aggressive-protection-2.40.

It is not for direct consumption by "git am".

If this format looks reasonable to folks as a way to review the result
of merging up fixes, I'll follow up with "patches" for more recent
maintenance tracks.

diff --git a/builtin/clone.c b/builtin/clone.c
remerge CONFLICT (content): Merge conflict in builtin/clone.c
index 17d34efebd..399b2d3f42 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1447,15 +1447,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	free(unborn_head);
 	free(dir);
 	free(path);
-<<<<<<< b9b439e0e3 (Git 2.40.2)
 	free(repo_to_free);
-	free(template_dir_dup);
-||||||| 47b6d90e91
-	free(template_dir_dup);
 	UNLEAK(repo);
-=======
-	UNLEAK(repo);
->>>>>>> 9074ec92e7 (Revert "fetch/clone: detect dubious ownership of local repositories")
 	junk_mode = JUNK_LEAVE_ALL;
 
 	transport_ls_refs_options_release(&transport_ls_refs_options);
diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh
remerge CONFLICT (content): Merge conflict in t/t1800-hook.sh
index edf0fa1334..3506f627b6 100755
--- a/t/t1800-hook.sh
+++ b/t/t1800-hook.sh
@@ -177,7 +177,6 @@ test_expect_success 'git hook run a hook with a bad shebang' '
 	test_cmp expect actual
 '
 
-<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< b9b439e0e3 (Git 2.40.2)
 test_expect_success 'stdin to hooks' '
 	write_script .git/hooks/test-hook <<-\EOF &&
 	echo BEGIN stdin
@@ -196,37 +195,4 @@ test_expect_success 'stdin to hooks' '
 	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
-'
-
-|||||||||||||||||||||||||||||||| 47b6d90e91
-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
-'
-
-================================
->>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 9074ec92e7 (Revert "fetch/clone: detect dubious ownership of local repositories")
 test_done

^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [rPATCH 14/12] Merge branch 'jc/fix-aggressive-protection-2.40'
  2024-05-21 19:56 [PATCH 00/12] Fix various overly aggressive protections in 2.45.1 and friends Junio C Hamano
                   ` (12 preceding siblings ...)
  2024-05-21 20:45 ` [rPATCH 13/12] Merge branch 'jc/fix-aggressive-protection-2.39' Junio C Hamano
@ 2024-05-21 20:45 ` Junio C Hamano
  2024-05-21 21:33   ` Junio C Hamano
  2024-05-21 21:14 ` [PATCH 00/12] Fix various overly aggressive protections in 2.45.1 and friends Johannes Schindelin
  2024-05-22 10:01 ` Joey Hess
  15 siblings, 1 reply; 36+ messages in thread
From: Junio C Hamano @ 2024-05-21 20:45 UTC (permalink / raw)
  To: git

With patches 01/12 - 12/12 applied on top of maint-2.39 (the result of
which is called jc/fix-aggressive-protection-2.39), and then the result
merged to maint-2.40 with conflict resolutions shown in 13/12, I've
prepared jc/fix-aggressive-protection-2.40.

This is "git show --remerge-diff" of the result of adjusting the
jc/fix-aggressive-protection-2.40 topic to maint-2.41 track.  
The result is called jc/fix-aggressive-protection-2.41.

It is not for direct consumption by "git am".

The only tricky part is an evil merge to copy.h that was made
necessarily due to recent header file shuffling.

diff --git a/builtin/clone.c b/builtin/clone.c
remerge CONFLICT (content): Merge conflict in builtin/clone.c
index 85ecda8e6f..b7db074b7e 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -958,15 +958,7 @@ 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;
-<<<<<<< 0f15832059 (Git 2.41.1)
 	int hash_algo;
-	const char *template_dir;
-	char *template_dir_dup = NULL;
-||||||| b9b439e0e3
-	const char *template_dir;
-	char *template_dir_dup = NULL;
-=======
->>>>>>> 1b2b92753e (Merge branch 'jc/fix-aggressive-protection-2.39' into HEAD)
 
 	struct transport_ls_refs_options transport_ls_refs_options =
 		TRANSPORT_LS_REFS_OPTIONS_INIT;
diff --git a/cache.h b/cache.h
remerge CONFLICT (content): Merge conflict in cache.h
index 534e983b90..bdedb87e83 100644
--- a/cache.h
+++ b/cache.h
@@ -555,1301 +555,7 @@ extern int verify_ce_order;
 #define DATA_CHANGED    0x0020
 #define TYPE_CHANGED    0x0040
 
-<<<<<<< 0f15832059 (Git 2.41.1)
 int cmp_cache_name_compare(const void *a_, const void *b_);
-||||||| b9b439e0e3
-/*
- * Return an abbreviated sha1 unique within this repository's object database.
- * The result will be at least `len` characters long, and will be NUL
- * terminated.
- *
- * The non-`_r` version returns a static buffer which remains valid until 4
- * more calls to find_unique_abbrev are made.
- *
- * The `_r` variant writes to a buffer supplied by the caller, which must be at
- * least `GIT_MAX_HEXSZ + 1` bytes. The return value is the number of bytes
- * written (excluding the NUL terminator).
- *
- * Note that while this version avoids the static buffer, it is not fully
- * reentrant, as it calls into other non-reentrant git code.
- */
-const char *repo_find_unique_abbrev(struct repository *r, const struct object_id *oid, int len);
-#define find_unique_abbrev(oid, len) repo_find_unique_abbrev(the_repository, oid, len)
-int repo_find_unique_abbrev_r(struct repository *r, char *hex, const struct object_id *oid, int len);
-#define find_unique_abbrev_r(hex, oid, len) repo_find_unique_abbrev_r(the_repository, hex, oid, len)
-
-/* set default permissions by passing mode arguments to open(2) */
-int git_mkstemps_mode(char *pattern, int suffix_len, int mode);
-int git_mkstemp_mode(char *pattern, int mode);
-
-/*
- * NOTE NOTE NOTE!!
- *
- * PERM_UMASK, OLD_PERM_GROUP and OLD_PERM_EVERYBODY enumerations must
- * not be changed. Old repositories have core.sharedrepository written in
- * numeric format, and therefore these values are preserved for compatibility
- * reasons.
- */
-enum sharedrepo {
-	PERM_UMASK          = 0,
-	OLD_PERM_GROUP      = 1,
-	OLD_PERM_EVERYBODY  = 2,
-	PERM_GROUP          = 0660,
-	PERM_EVERYBODY      = 0664
-};
-int git_config_perm(const char *var, const char *value);
-int adjust_shared_perm(const char *path);
-
-/*
- * Create the directory containing the named path, using care to be
- * somewhat safe against races. Return one of the scld_error values to
- * indicate success/failure. On error, set errno to describe the
- * problem.
- *
- * SCLD_VANISHED indicates that one of the ancestor directories of the
- * path existed at one point during the function call and then
- * suddenly vanished, probably because another process pruned the
- * directory while we were working.  To be robust against this kind of
- * race, callers might want to try invoking the function again when it
- * returns SCLD_VANISHED.
- *
- * safe_create_leading_directories() temporarily changes path while it
- * is working but restores it before returning.
- * safe_create_leading_directories_const() doesn't modify path, even
- * temporarily. Both these variants adjust the permissions of the
- * created directories to honor core.sharedRepository, so they are best
- * suited for files inside the git dir. For working tree files, use
- * safe_create_leading_directories_no_share() instead, as it ignores
- * the core.sharedRepository setting.
- */
-enum scld_error {
-	SCLD_OK = 0,
-	SCLD_FAILED = -1,
-	SCLD_PERMS = -2,
-	SCLD_EXISTS = -3,
-	SCLD_VANISHED = -4
-};
-enum scld_error safe_create_leading_directories(char *path);
-enum scld_error safe_create_leading_directories_const(const char *path);
-enum scld_error safe_create_leading_directories_no_share(char *path);
-
-int mkdir_in_gitdir(const char *path);
-char *interpolate_path(const char *path, int real_home);
-/* NEEDSWORK: remove this synonym once in-flight topics have migrated */
-#define expand_user_path interpolate_path
-const char *enter_repo(const char *path, int strict);
-static inline int is_absolute_path(const char *path)
-{
-	return is_dir_sep(path[0]) || has_dos_drive_prefix(path);
-}
-int is_directory(const char *);
-char *strbuf_realpath(struct strbuf *resolved, const char *path,
-		      int die_on_error);
-char *strbuf_realpath_forgiving(struct strbuf *resolved, const char *path,
-				int die_on_error);
-char *real_pathdup(const char *path, int die_on_error);
-const char *absolute_path(const char *path);
-char *absolute_pathdup(const char *path);
-const char *remove_leading_path(const char *in, const char *prefix);
-const char *relative_path(const char *in, const char *prefix, struct strbuf *sb);
-int normalize_path_copy_len(char *dst, const char *src, int *prefix_len);
-int normalize_path_copy(char *dst, const char *src);
-int longest_ancestor_length(const char *path, struct string_list *prefixes);
-char *strip_path_suffix(const char *path, const char *suffix);
-int daemon_avoid_alias(const char *path);
-
-/*
- * These functions match their is_hfs_dotgit() counterparts; see utf8.h for
- * details.
- */
-int is_ntfs_dotgit(const char *name);
-int is_ntfs_dotgitmodules(const char *name);
-int is_ntfs_dotgitignore(const char *name);
-int is_ntfs_dotgitattributes(const char *name);
-int is_ntfs_dotmailmap(const char *name);
-
-/*
- * Returns true iff "str" could be confused as a command-line option when
- * passed to a sub-program like "ssh". Note that this has nothing to do with
- * shell-quoting, which should be handled separately; we're assuming here that
- * the string makes it verbatim to the sub-program.
- */
-int looks_like_command_line_option(const char *str);
-
-/**
- * Return a newly allocated string with the evaluation of
- * "$XDG_CONFIG_HOME/$subdir/$filename" if $XDG_CONFIG_HOME is non-empty, otherwise
- * "$HOME/.config/$subdir/$filename". Return NULL upon error.
- */
-char *xdg_config_home_for(const char *subdir, const char *filename);
-
-/**
- * Return a newly allocated string with the evaluation of
- * "$XDG_CONFIG_HOME/git/$filename" if $XDG_CONFIG_HOME is non-empty, otherwise
- * "$HOME/.config/git/$filename". Return NULL upon error.
- */
-char *xdg_config_home(const char *filename);
-
-/**
- * Return a newly allocated string with the evaluation of
- * "$XDG_CACHE_HOME/git/$filename" if $XDG_CACHE_HOME is non-empty, otherwise
- * "$HOME/.cache/git/$filename". Return NULL upon error.
- */
-char *xdg_cache_home(const char *filename);
-
-int git_open_cloexec(const char *name, int flags);
-#define git_open(name) git_open_cloexec(name, O_RDONLY)
-
-/**
- * unpack_loose_header() initializes the data stream needed to unpack
- * a loose object header.
- *
- * Returns:
- *
- * - ULHR_OK on success
- * - ULHR_BAD on error
- * - ULHR_TOO_LONG if the header was too long
- *
- * It will only parse up to MAX_HEADER_LEN bytes unless an optional
- * "hdrbuf" argument is non-NULL. This is intended for use with
- * OBJECT_INFO_ALLOW_UNKNOWN_TYPE to extract the bad type for (error)
- * reporting. The full header will be extracted to "hdrbuf" for use
- * with parse_loose_header(), ULHR_TOO_LONG will still be returned
- * from this function to indicate that the header was too long.
- */
-enum unpack_loose_header_result {
-	ULHR_OK,
-	ULHR_BAD,
-	ULHR_TOO_LONG,
-};
-enum unpack_loose_header_result unpack_loose_header(git_zstream *stream,
-						    unsigned char *map,
-						    unsigned long mapsize,
-						    void *buffer,
-						    unsigned long bufsiz,
-						    struct strbuf *hdrbuf);
-
-/**
- * parse_loose_header() parses the starting "<type> <len>\0" of an
- * object. If it doesn't follow that format -1 is returned. To check
- * the validity of the <type> populate the "typep" in the "struct
- * object_info". It will be OBJ_BAD if the object type is unknown. The
- * parsed <len> can be retrieved via "oi->sizep", and from there
- * passed to unpack_loose_rest().
- */
-struct object_info;
-int parse_loose_header(const char *hdr, struct object_info *oi);
-
-/**
- * With in-core object data in "buf", rehash it to make sure the
- * object name actually matches "oid" to detect object corruption.
- *
- * A negative value indicates an error, usually that the OID is not
- * what we expected, but it might also indicate another error.
- */
-int check_object_signature(struct repository *r, const struct object_id *oid,
-			   void *map, unsigned long size,
-			   enum object_type type);
-
-/**
- * A streaming version of check_object_signature().
- * Try reading the object named with "oid" using
- * the streaming interface and rehash it to do the same.
- */
-int stream_object_signature(struct repository *r, const struct object_id *oid);
-
-int finalize_object_file(const char *tmpfile, const char *filename);
-
-/* Helper to check and "touch" a file */
-int check_and_freshen_file(const char *fn, int freshen);
-
-extern const signed char hexval_table[256];
-static inline unsigned int hexval(unsigned char c)
-{
-	return hexval_table[c];
-}
-
-/*
- * Convert two consecutive hexadecimal digits into a char.  Return a
- * negative value on error.  Don't run over the end of short strings.
- */
-static inline int hex2chr(const char *s)
-{
-	unsigned int val = hexval(s[0]);
-	return (val & ~0xf) ? val : (val << 4) | hexval(s[1]);
-}
-
-/* Convert to/from hex/sha1 representation */
-#define MINIMUM_ABBREV minimum_abbrev
-#define DEFAULT_ABBREV default_abbrev
-
-/* used when the code does not know or care what the default abbrev is */
-#define FALLBACK_DEFAULT_ABBREV 7
-
-struct object_context {
-	unsigned short mode;
-	/*
-	 * symlink_path is only used by get_tree_entry_follow_symlinks,
-	 * and only for symlinks that point outside the repository.
-	 */
-	struct strbuf symlink_path;
-	/*
-	 * If GET_OID_RECORD_PATH is set, this will record path (if any)
-	 * found when resolving the name. The caller is responsible for
-	 * releasing the memory.
-	 */
-	char *path;
-};
-
-#define GET_OID_QUIETLY           01
-#define GET_OID_COMMIT            02
-#define GET_OID_COMMITTISH        04
-#define GET_OID_TREE             010
-#define GET_OID_TREEISH          020
-#define GET_OID_BLOB             040
-#define GET_OID_FOLLOW_SYMLINKS 0100
-#define GET_OID_RECORD_PATH     0200
-#define GET_OID_ONLY_TO_DIE    04000
-#define GET_OID_REQUIRE_PATH  010000
-
-#define GET_OID_DISAMBIGUATORS \
-	(GET_OID_COMMIT | GET_OID_COMMITTISH | \
-	GET_OID_TREE | GET_OID_TREEISH | \
-	GET_OID_BLOB)
-
-enum get_oid_result {
-	FOUND = 0,
-	MISSING_OBJECT = -1, /* The requested object is missing */
-	SHORT_NAME_AMBIGUOUS = -2,
-	/* The following only apply when symlinks are followed */
-	DANGLING_SYMLINK = -4, /*
-				* The initial symlink is there, but
-				* (transitively) points to a missing
-				* in-tree file
-				*/
-	SYMLINK_LOOP = -5,
-	NOT_DIR = -6, /*
-		       * Somewhere along the symlink chain, a path is
-		       * requested which contains a file as a
-		       * non-final element.
-		       */
-};
-
-int repo_get_oid(struct repository *r, const char *str, struct object_id *oid);
-__attribute__((format (printf, 2, 3)))
-int get_oidf(struct object_id *oid, const char *fmt, ...);
-int repo_get_oid_commit(struct repository *r, const char *str, struct object_id *oid);
-int repo_get_oid_committish(struct repository *r, const char *str, struct object_id *oid);
-int repo_get_oid_tree(struct repository *r, const char *str, struct object_id *oid);
-int repo_get_oid_treeish(struct repository *r, const char *str, struct object_id *oid);
-int repo_get_oid_blob(struct repository *r, const char *str, struct object_id *oid);
-int repo_get_oid_mb(struct repository *r, const char *str, struct object_id *oid);
-void maybe_die_on_misspelt_object_name(struct repository *repo,
-				       const char *name,
-				       const char *prefix);
-enum get_oid_result get_oid_with_context(struct repository *repo, const char *str,
-					 unsigned flags, struct object_id *oid,
-					 struct object_context *oc);
-
-#define get_oid(str, oid)		repo_get_oid(the_repository, str, oid)
-#define get_oid_commit(str, oid)	repo_get_oid_commit(the_repository, str, oid)
-#define get_oid_committish(str, oid)	repo_get_oid_committish(the_repository, str, oid)
-#define get_oid_tree(str, oid)		repo_get_oid_tree(the_repository, str, oid)
-#define get_oid_treeish(str, oid)	repo_get_oid_treeish(the_repository, str, oid)
-#define get_oid_blob(str, oid)		repo_get_oid_blob(the_repository, str, oid)
-#define get_oid_mb(str, oid) 		repo_get_oid_mb(the_repository, str, oid)
-
-typedef int each_abbrev_fn(const struct object_id *oid, void *);
-int repo_for_each_abbrev(struct repository *r, const char *prefix, each_abbrev_fn, void *);
-#define for_each_abbrev(prefix, fn, data) repo_for_each_abbrev(the_repository, prefix, fn, data)
-
-int set_disambiguate_hint_config(const char *var, const char *value);
-
-/*
- * Try to read a SHA1 in hexadecimal format from the 40 characters
- * starting at hex.  Write the 20-byte result to sha1 in binary form.
- * Return 0 on success.  Reading stops if a NUL is encountered in the
- * input, so it is safe to pass this function an arbitrary
- * null-terminated string.
- */
-int get_sha1_hex(const char *hex, unsigned char *sha1);
-int get_oid_hex(const char *hex, struct object_id *sha1);
-
-/* Like get_oid_hex, but for an arbitrary hash algorithm. */
-int get_oid_hex_algop(const char *hex, struct object_id *oid, const struct git_hash_algo *algop);
-
-/*
- * Read `len` pairs of hexadecimal digits from `hex` and write the
- * values to `binary` as `len` bytes. Return 0 on success, or -1 if
- * the input does not consist of hex digits).
- */
-int hex_to_bytes(unsigned char *binary, const char *hex, size_t len);
-
-/*
- * Convert a binary hash in "unsigned char []" or an object name in
- * "struct object_id *" to its hex equivalent. The `_r` variant is reentrant,
- * and writes the NUL-terminated output to the buffer `out`, which must be at
- * least `GIT_MAX_HEXSZ + 1` bytes, and returns a pointer to out for
- * convenience.
- *
- * The non-`_r` variant returns a static buffer, but uses a ring of 4
- * buffers, making it safe to make multiple calls for a single statement, like:
- *
- *   printf("%s -> %s", hash_to_hex(one), hash_to_hex(two));
- *   printf("%s -> %s", oid_to_hex(one), oid_to_hex(two));
- */
-char *hash_to_hex_algop_r(char *buffer, const unsigned char *hash, const struct git_hash_algo *);
-char *oid_to_hex_r(char *out, const struct object_id *oid);
-char *hash_to_hex_algop(const unsigned char *hash, const struct git_hash_algo *);	/* static buffer result! */
-char *hash_to_hex(const unsigned char *hash);						/* same static buffer */
-char *oid_to_hex(const struct object_id *oid);						/* same static buffer */
-
-/*
- * Parse a 40-character hexadecimal object ID starting from hex, updating the
- * pointer specified by end when parsing stops.  The resulting object ID is
- * stored in oid.  Returns 0 on success.  Parsing will stop on the first NUL or
- * other invalid character.  end is only updated on success; otherwise, it is
- * unmodified.
- */
-int parse_oid_hex(const char *hex, struct object_id *oid, const char **end);
-
-/* Like parse_oid_hex, but for an arbitrary hash algorithm. */
-int parse_oid_hex_algop(const char *hex, struct object_id *oid, const char **end,
-			const struct git_hash_algo *algo);
-
-
-/*
- * These functions work like get_oid_hex and parse_oid_hex, but they will parse
- * a hex value for any algorithm. The algorithm is detected based on the length
- * and the algorithm in use is returned. If this is not a hex object ID in any
- * algorithm, returns GIT_HASH_UNKNOWN.
- */
-int get_oid_hex_any(const char *hex, struct object_id *oid);
-int parse_oid_hex_any(const char *hex, struct object_id *oid, const char **end);
-
-/*
- * This reads short-hand syntax that not only evaluates to a commit
- * object name, but also can act as if the end user spelled the name
- * of the branch from the command line.
- *
- * - "@{-N}" finds the name of the Nth previous branch we were on, and
- *   places the name of the branch in the given buf and returns the
- *   number of characters parsed if successful.
- *
- * - "<branch>@{upstream}" finds the name of the other ref that
- *   <branch> is configured to merge with (missing <branch> defaults
- *   to the current branch), and places the name of the branch in the
- *   given buf and returns the number of characters parsed if
- *   successful.
- *
- * If the input is not of the accepted format, it returns a negative
- * number to signal an error.
- *
- * If the input was ok but there are not N branch switches in the
- * reflog, it returns 0.
- */
-#define INTERPRET_BRANCH_LOCAL (1<<0)
-#define INTERPRET_BRANCH_REMOTE (1<<1)
-#define INTERPRET_BRANCH_HEAD (1<<2)
-struct interpret_branch_name_options {
-	/*
-	 * If "allowed" is non-zero, it is a treated as a bitfield of allowable
-	 * expansions: local branches ("refs/heads/"), remote branches
-	 * ("refs/remotes/"), or "HEAD". If no "allowed" bits are set, any expansion is
-	 * allowed, even ones to refs outside of those namespaces.
-	 */
-	unsigned allowed;
-
-	/*
-	 * If ^{upstream} or ^{push} (or equivalent) is requested, and the
-	 * branch in question does not have such a reference, return -1 instead
-	 * of die()-ing.
-	 */
-	unsigned nonfatal_dangling_mark : 1;
-};
-int repo_interpret_branch_name(struct repository *r,
-			       const char *str, int len,
-			       struct strbuf *buf,
-			       const struct interpret_branch_name_options *options);
-#define interpret_branch_name(str, len, buf, options) \
-	repo_interpret_branch_name(the_repository, str, len, buf, options)
-
-int validate_headref(const char *ref);
-
-int base_name_compare(const char *name1, size_t len1, int mode1,
-		      const char *name2, size_t len2, int mode2);
-int df_name_compare(const char *name1, size_t len1, int mode1,
-		    const char *name2, size_t len2, int mode2);
-int name_compare(const char *name1, size_t len1, const char *name2, size_t len2);
-int cache_name_stage_compare(const char *name1, int len1, int stage1, const char *name2, int len2, int stage2);
-
-void *read_object_with_reference(struct repository *r,
-				 const struct object_id *oid,
-				 enum object_type required_type,
-				 unsigned long *size,
-				 struct object_id *oid_ret);
-
-struct object *repo_peel_to_type(struct repository *r,
-				 const char *name, int namelen,
-				 struct object *o, enum object_type);
-#define peel_to_type(name, namelen, obj, type) \
-	repo_peel_to_type(the_repository, name, namelen, obj, type)
-
-#define IDENT_STRICT	       1
-#define IDENT_NO_DATE	       2
-#define IDENT_NO_NAME	       4
-
-enum want_ident {
-	WANT_BLANK_IDENT,
-	WANT_AUTHOR_IDENT,
-	WANT_COMMITTER_IDENT
-};
-
-const char *git_author_info(int);
-const char *git_committer_info(int);
-const char *fmt_ident(const char *name, const char *email,
-		      enum want_ident whose_ident,
-		      const char *date_str, int);
-const char *fmt_name(enum want_ident);
-const char *ident_default_name(void);
-const char *ident_default_email(void);
-const char *git_editor(void);
-const char *git_sequence_editor(void);
-const char *git_pager(int stdout_is_tty);
-int is_terminal_dumb(void);
-int git_ident_config(const char *, const char *, void *);
-/*
- * Prepare an ident to fall back on if the user didn't configure it.
- */
-void prepare_fallback_ident(const char *name, const char *email);
-void reset_ident_date(void);
-
-struct ident_split {
-	const char *name_begin;
-	const char *name_end;
-	const char *mail_begin;
-	const char *mail_end;
-	const char *date_begin;
-	const char *date_end;
-	const char *tz_begin;
-	const char *tz_end;
-};
-/*
- * Signals an success with 0, but time part of the result may be NULL
- * if the input lacks timestamp and zone
- */
-int split_ident_line(struct ident_split *, const char *, int);
-
-/*
- * Given a commit or tag object buffer and the commit or tag headers, replaces
- * the idents in the headers with their canonical versions using the mailmap mechanism.
- */
-void apply_mailmap_to_header(struct strbuf *, const char **, struct string_list *);
-
-/*
- * Compare split idents for equality or strict ordering. Note that we
- * compare only the ident part of the line, ignoring any timestamp.
- *
- * Because there are two fields, we must choose one as the primary key; we
- * currently arbitrarily pick the email.
- */
-int ident_cmp(const struct ident_split *, const struct ident_split *);
-
-struct cache_def {
-	struct strbuf path;
-	int flags;
-	int track_flags;
-	int prefix_len_stat_func;
-};
-#define CACHE_DEF_INIT { \
-	.path = STRBUF_INIT, \
-}
-static inline void cache_def_clear(struct cache_def *cache)
-{
-	strbuf_release(&cache->path);
-}
-
-int has_symlink_leading_path(const char *name, int len);
-int threaded_has_symlink_leading_path(struct cache_def *, const char *, int);
-int check_leading_path(const char *name, int len, int warn_on_lstat_err);
-int has_dirs_only_path(const char *name, int len, int prefix_len);
-void invalidate_lstat_cache(void);
-void schedule_dir_for_removal(const char *name, int len);
-void remove_scheduled_dirs(void);
-
-struct pack_window {
-	struct pack_window *next;
-	unsigned char *base;
-	off_t offset;
-	size_t len;
-	unsigned int last_used;
-	unsigned int inuse_cnt;
-};
-
-struct pack_entry {
-	off_t offset;
-	struct packed_git *p;
-};
-
-/*
- * Create a temporary file rooted in the object database directory, or
- * die on failure. The filename is taken from "pattern", which should have the
- * usual "XXXXXX" trailer, and the resulting filename is written into the
- * "template" buffer. Returns the open descriptor.
- */
-int odb_mkstemp(struct strbuf *temp_filename, const char *pattern);
-
-/*
- * Create a pack .keep file named "name" (which should generally be the output
- * of odb_pack_name). Returns a file descriptor opened for writing, or -1 on
- * error.
- */
-int odb_pack_keep(const char *name);
-
-/*
- * Set this to 0 to prevent oid_object_info_extended() from fetching missing
- * blobs. This has a difference only if extensions.partialClone is set.
- *
- * Its default value is 1.
- */
-extern int fetch_if_missing;
-
-/* Dumb servers support */
-int update_server_info(int);
-
-const char *get_log_output_encoding(void);
-const char *get_commit_output_encoding(void);
-
-int committer_ident_sufficiently_given(void);
-int author_ident_sufficiently_given(void);
-
-extern const char *git_commit_encoding;
-extern const char *git_log_output_encoding;
-extern const char *git_mailmap_file;
-extern const char *git_mailmap_blob;
-
-/* IO helper functions */
-void maybe_flush_or_die(FILE *, const char *);
-__attribute__((format (printf, 2, 3)))
-void fprintf_or_die(FILE *, const char *fmt, ...);
-void fwrite_or_die(FILE *f, const void *buf, size_t count);
-void fflush_or_die(FILE *f);
-
-#define COPY_READ_ERROR (-2)
-#define COPY_WRITE_ERROR (-3)
-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);
-void fsync_component_or_die(enum fsync_component component, int fd, const char *msg);
-
-static inline int batch_fsync_enabled(enum fsync_component component)
-{
-	return (fsync_components & component) && (fsync_method == FSYNC_METHOD_BATCH);
-}
-
-ssize_t read_in_full(int fd, void *buf, size_t count);
-ssize_t write_in_full(int fd, const void *buf, size_t count);
-ssize_t pread_in_full(int fd, void *buf, size_t count, off_t offset);
-
-static inline ssize_t write_str_in_full(int fd, const char *str)
-{
-	return write_in_full(fd, str, strlen(str));
-}
-
-/**
- * Open (and truncate) the file at path, write the contents of buf to it,
- * and close it. Dies if any errors are encountered.
- */
-void write_file_buf(const char *path, const char *buf, size_t len);
-
-/**
- * Like write_file_buf(), but format the contents into a buffer first.
- * Additionally, write_file() will append a newline if one is not already
- * present, making it convenient to write text files:
- *
- *   write_file(path, "counter: %d", ctr);
- */
-__attribute__((format (printf, 2, 3)))
-void write_file(const char *path, const char *fmt, ...);
-
-/* pager.c */
-void setup_pager(void);
-int pager_in_use(void);
-extern int pager_use_color;
-int term_columns(void);
-void term_clear_line(void);
-int decimal_width(uintmax_t);
-int check_pager_config(const char *cmd);
-void prepare_pager_args(struct child_process *, const char *pager);
-
-extern const char *editor_program;
-extern const char *askpass_program;
-extern const char *excludes_file;
-
-/* base85 */
-int decode_85(char *dst, const char *line, int linelen);
-void encode_85(char *buf, const unsigned char *data, int bytes);
-
-/* pkt-line.c */
-void packet_trace_identity(const char *prog);
-=======
-/*
- * Return an abbreviated sha1 unique within this repository's object database.
- * The result will be at least `len` characters long, and will be NUL
- * terminated.
- *
- * The non-`_r` version returns a static buffer which remains valid until 4
- * more calls to find_unique_abbrev are made.
- *
- * The `_r` variant writes to a buffer supplied by the caller, which must be at
- * least `GIT_MAX_HEXSZ + 1` bytes. The return value is the number of bytes
- * written (excluding the NUL terminator).
- *
- * Note that while this version avoids the static buffer, it is not fully
- * reentrant, as it calls into other non-reentrant git code.
- */
-const char *repo_find_unique_abbrev(struct repository *r, const struct object_id *oid, int len);
-#define find_unique_abbrev(oid, len) repo_find_unique_abbrev(the_repository, oid, len)
-int repo_find_unique_abbrev_r(struct repository *r, char *hex, const struct object_id *oid, int len);
-#define find_unique_abbrev_r(hex, oid, len) repo_find_unique_abbrev_r(the_repository, hex, oid, len)
-
-/* set default permissions by passing mode arguments to open(2) */
-int git_mkstemps_mode(char *pattern, int suffix_len, int mode);
-int git_mkstemp_mode(char *pattern, int mode);
-
-/*
- * NOTE NOTE NOTE!!
- *
- * PERM_UMASK, OLD_PERM_GROUP and OLD_PERM_EVERYBODY enumerations must
- * not be changed. Old repositories have core.sharedrepository written in
- * numeric format, and therefore these values are preserved for compatibility
- * reasons.
- */
-enum sharedrepo {
-	PERM_UMASK          = 0,
-	OLD_PERM_GROUP      = 1,
-	OLD_PERM_EVERYBODY  = 2,
-	PERM_GROUP          = 0660,
-	PERM_EVERYBODY      = 0664
-};
-int git_config_perm(const char *var, const char *value);
-int adjust_shared_perm(const char *path);
-
-/*
- * Create the directory containing the named path, using care to be
- * somewhat safe against races. Return one of the scld_error values to
- * indicate success/failure. On error, set errno to describe the
- * problem.
- *
- * SCLD_VANISHED indicates that one of the ancestor directories of the
- * path existed at one point during the function call and then
- * suddenly vanished, probably because another process pruned the
- * directory while we were working.  To be robust against this kind of
- * race, callers might want to try invoking the function again when it
- * returns SCLD_VANISHED.
- *
- * safe_create_leading_directories() temporarily changes path while it
- * is working but restores it before returning.
- * safe_create_leading_directories_const() doesn't modify path, even
- * temporarily. Both these variants adjust the permissions of the
- * created directories to honor core.sharedRepository, so they are best
- * suited for files inside the git dir. For working tree files, use
- * safe_create_leading_directories_no_share() instead, as it ignores
- * the core.sharedRepository setting.
- */
-enum scld_error {
-	SCLD_OK = 0,
-	SCLD_FAILED = -1,
-	SCLD_PERMS = -2,
-	SCLD_EXISTS = -3,
-	SCLD_VANISHED = -4
-};
-enum scld_error safe_create_leading_directories(char *path);
-enum scld_error safe_create_leading_directories_const(const char *path);
-enum scld_error safe_create_leading_directories_no_share(char *path);
-
-int mkdir_in_gitdir(const char *path);
-char *interpolate_path(const char *path, int real_home);
-/* NEEDSWORK: remove this synonym once in-flight topics have migrated */
-#define expand_user_path interpolate_path
-const char *enter_repo(const char *path, int strict);
-static inline int is_absolute_path(const char *path)
-{
-	return is_dir_sep(path[0]) || has_dos_drive_prefix(path);
-}
-int is_directory(const char *);
-char *strbuf_realpath(struct strbuf *resolved, const char *path,
-		      int die_on_error);
-char *strbuf_realpath_forgiving(struct strbuf *resolved, const char *path,
-				int die_on_error);
-char *real_pathdup(const char *path, int die_on_error);
-const char *absolute_path(const char *path);
-char *absolute_pathdup(const char *path);
-const char *remove_leading_path(const char *in, const char *prefix);
-const char *relative_path(const char *in, const char *prefix, struct strbuf *sb);
-int normalize_path_copy_len(char *dst, const char *src, int *prefix_len);
-int normalize_path_copy(char *dst, const char *src);
-int longest_ancestor_length(const char *path, struct string_list *prefixes);
-char *strip_path_suffix(const char *path, const char *suffix);
-int daemon_avoid_alias(const char *path);
-
-/*
- * These functions match their is_hfs_dotgit() counterparts; see utf8.h for
- * details.
- */
-int is_ntfs_dotgit(const char *name);
-int is_ntfs_dotgitmodules(const char *name);
-int is_ntfs_dotgitignore(const char *name);
-int is_ntfs_dotgitattributes(const char *name);
-int is_ntfs_dotmailmap(const char *name);
-
-/*
- * Returns true iff "str" could be confused as a command-line option when
- * passed to a sub-program like "ssh". Note that this has nothing to do with
- * shell-quoting, which should be handled separately; we're assuming here that
- * the string makes it verbatim to the sub-program.
- */
-int looks_like_command_line_option(const char *str);
-
-/**
- * Return a newly allocated string with the evaluation of
- * "$XDG_CONFIG_HOME/$subdir/$filename" if $XDG_CONFIG_HOME is non-empty, otherwise
- * "$HOME/.config/$subdir/$filename". Return NULL upon error.
- */
-char *xdg_config_home_for(const char *subdir, const char *filename);
-
-/**
- * Return a newly allocated string with the evaluation of
- * "$XDG_CONFIG_HOME/git/$filename" if $XDG_CONFIG_HOME is non-empty, otherwise
- * "$HOME/.config/git/$filename". Return NULL upon error.
- */
-char *xdg_config_home(const char *filename);
-
-/**
- * Return a newly allocated string with the evaluation of
- * "$XDG_CACHE_HOME/git/$filename" if $XDG_CACHE_HOME is non-empty, otherwise
- * "$HOME/.cache/git/$filename". Return NULL upon error.
- */
-char *xdg_cache_home(const char *filename);
-
-int git_open_cloexec(const char *name, int flags);
-#define git_open(name) git_open_cloexec(name, O_RDONLY)
-
-/**
- * unpack_loose_header() initializes the data stream needed to unpack
- * a loose object header.
- *
- * Returns:
- *
- * - ULHR_OK on success
- * - ULHR_BAD on error
- * - ULHR_TOO_LONG if the header was too long
- *
- * It will only parse up to MAX_HEADER_LEN bytes unless an optional
- * "hdrbuf" argument is non-NULL. This is intended for use with
- * OBJECT_INFO_ALLOW_UNKNOWN_TYPE to extract the bad type for (error)
- * reporting. The full header will be extracted to "hdrbuf" for use
- * with parse_loose_header(), ULHR_TOO_LONG will still be returned
- * from this function to indicate that the header was too long.
- */
-enum unpack_loose_header_result {
-	ULHR_OK,
-	ULHR_BAD,
-	ULHR_TOO_LONG,
-};
-enum unpack_loose_header_result unpack_loose_header(git_zstream *stream,
-						    unsigned char *map,
-						    unsigned long mapsize,
-						    void *buffer,
-						    unsigned long bufsiz,
-						    struct strbuf *hdrbuf);
-
-/**
- * parse_loose_header() parses the starting "<type> <len>\0" of an
- * object. If it doesn't follow that format -1 is returned. To check
- * the validity of the <type> populate the "typep" in the "struct
- * object_info". It will be OBJ_BAD if the object type is unknown. The
- * parsed <len> can be retrieved via "oi->sizep", and from there
- * passed to unpack_loose_rest().
- */
-struct object_info;
-int parse_loose_header(const char *hdr, struct object_info *oi);
-
-/**
- * With in-core object data in "buf", rehash it to make sure the
- * object name actually matches "oid" to detect object corruption.
- *
- * A negative value indicates an error, usually that the OID is not
- * what we expected, but it might also indicate another error.
- */
-int check_object_signature(struct repository *r, const struct object_id *oid,
-			   void *map, unsigned long size,
-			   enum object_type type);
-
-/**
- * A streaming version of check_object_signature().
- * Try reading the object named with "oid" using
- * the streaming interface and rehash it to do the same.
- */
-int stream_object_signature(struct repository *r, const struct object_id *oid);
-
-int finalize_object_file(const char *tmpfile, const char *filename);
-
-/* Helper to check and "touch" a file */
-int check_and_freshen_file(const char *fn, int freshen);
-
-extern const signed char hexval_table[256];
-static inline unsigned int hexval(unsigned char c)
-{
-	return hexval_table[c];
-}
-
-/*
- * Convert two consecutive hexadecimal digits into a char.  Return a
- * negative value on error.  Don't run over the end of short strings.
- */
-static inline int hex2chr(const char *s)
-{
-	unsigned int val = hexval(s[0]);
-	return (val & ~0xf) ? val : (val << 4) | hexval(s[1]);
-}
-
-/* Convert to/from hex/sha1 representation */
-#define MINIMUM_ABBREV minimum_abbrev
-#define DEFAULT_ABBREV default_abbrev
-
-/* used when the code does not know or care what the default abbrev is */
-#define FALLBACK_DEFAULT_ABBREV 7
-
-struct object_context {
-	unsigned short mode;
-	/*
-	 * symlink_path is only used by get_tree_entry_follow_symlinks,
-	 * and only for symlinks that point outside the repository.
-	 */
-	struct strbuf symlink_path;
-	/*
-	 * If GET_OID_RECORD_PATH is set, this will record path (if any)
-	 * found when resolving the name. The caller is responsible for
-	 * releasing the memory.
-	 */
-	char *path;
-};
-
-#define GET_OID_QUIETLY           01
-#define GET_OID_COMMIT            02
-#define GET_OID_COMMITTISH        04
-#define GET_OID_TREE             010
-#define GET_OID_TREEISH          020
-#define GET_OID_BLOB             040
-#define GET_OID_FOLLOW_SYMLINKS 0100
-#define GET_OID_RECORD_PATH     0200
-#define GET_OID_ONLY_TO_DIE    04000
-#define GET_OID_REQUIRE_PATH  010000
-
-#define GET_OID_DISAMBIGUATORS \
-	(GET_OID_COMMIT | GET_OID_COMMITTISH | \
-	GET_OID_TREE | GET_OID_TREEISH | \
-	GET_OID_BLOB)
-
-enum get_oid_result {
-	FOUND = 0,
-	MISSING_OBJECT = -1, /* The requested object is missing */
-	SHORT_NAME_AMBIGUOUS = -2,
-	/* The following only apply when symlinks are followed */
-	DANGLING_SYMLINK = -4, /*
-				* The initial symlink is there, but
-				* (transitively) points to a missing
-				* in-tree file
-				*/
-	SYMLINK_LOOP = -5,
-	NOT_DIR = -6, /*
-		       * Somewhere along the symlink chain, a path is
-		       * requested which contains a file as a
-		       * non-final element.
-		       */
-};
-
-int repo_get_oid(struct repository *r, const char *str, struct object_id *oid);
-__attribute__((format (printf, 2, 3)))
-int get_oidf(struct object_id *oid, const char *fmt, ...);
-int repo_get_oid_commit(struct repository *r, const char *str, struct object_id *oid);
-int repo_get_oid_committish(struct repository *r, const char *str, struct object_id *oid);
-int repo_get_oid_tree(struct repository *r, const char *str, struct object_id *oid);
-int repo_get_oid_treeish(struct repository *r, const char *str, struct object_id *oid);
-int repo_get_oid_blob(struct repository *r, const char *str, struct object_id *oid);
-int repo_get_oid_mb(struct repository *r, const char *str, struct object_id *oid);
-void maybe_die_on_misspelt_object_name(struct repository *repo,
-				       const char *name,
-				       const char *prefix);
-enum get_oid_result get_oid_with_context(struct repository *repo, const char *str,
-					 unsigned flags, struct object_id *oid,
-					 struct object_context *oc);
-
-#define get_oid(str, oid)		repo_get_oid(the_repository, str, oid)
-#define get_oid_commit(str, oid)	repo_get_oid_commit(the_repository, str, oid)
-#define get_oid_committish(str, oid)	repo_get_oid_committish(the_repository, str, oid)
-#define get_oid_tree(str, oid)		repo_get_oid_tree(the_repository, str, oid)
-#define get_oid_treeish(str, oid)	repo_get_oid_treeish(the_repository, str, oid)
-#define get_oid_blob(str, oid)		repo_get_oid_blob(the_repository, str, oid)
-#define get_oid_mb(str, oid) 		repo_get_oid_mb(the_repository, str, oid)
-
-typedef int each_abbrev_fn(const struct object_id *oid, void *);
-int repo_for_each_abbrev(struct repository *r, const char *prefix, each_abbrev_fn, void *);
-#define for_each_abbrev(prefix, fn, data) repo_for_each_abbrev(the_repository, prefix, fn, data)
-
-int set_disambiguate_hint_config(const char *var, const char *value);
-
-/*
- * Try to read a SHA1 in hexadecimal format from the 40 characters
- * starting at hex.  Write the 20-byte result to sha1 in binary form.
- * Return 0 on success.  Reading stops if a NUL is encountered in the
- * input, so it is safe to pass this function an arbitrary
- * null-terminated string.
- */
-int get_sha1_hex(const char *hex, unsigned char *sha1);
-int get_oid_hex(const char *hex, struct object_id *sha1);
-
-/* Like get_oid_hex, but for an arbitrary hash algorithm. */
-int get_oid_hex_algop(const char *hex, struct object_id *oid, const struct git_hash_algo *algop);
-
-/*
- * Read `len` pairs of hexadecimal digits from `hex` and write the
- * values to `binary` as `len` bytes. Return 0 on success, or -1 if
- * the input does not consist of hex digits).
- */
-int hex_to_bytes(unsigned char *binary, const char *hex, size_t len);
-
-/*
- * Convert a binary hash in "unsigned char []" or an object name in
- * "struct object_id *" to its hex equivalent. The `_r` variant is reentrant,
- * and writes the NUL-terminated output to the buffer `out`, which must be at
- * least `GIT_MAX_HEXSZ + 1` bytes, and returns a pointer to out for
- * convenience.
- *
- * The non-`_r` variant returns a static buffer, but uses a ring of 4
- * buffers, making it safe to make multiple calls for a single statement, like:
- *
- *   printf("%s -> %s", hash_to_hex(one), hash_to_hex(two));
- *   printf("%s -> %s", oid_to_hex(one), oid_to_hex(two));
- */
-char *hash_to_hex_algop_r(char *buffer, const unsigned char *hash, const struct git_hash_algo *);
-char *oid_to_hex_r(char *out, const struct object_id *oid);
-char *hash_to_hex_algop(const unsigned char *hash, const struct git_hash_algo *);	/* static buffer result! */
-char *hash_to_hex(const unsigned char *hash);						/* same static buffer */
-char *oid_to_hex(const struct object_id *oid);						/* same static buffer */
-
-/*
- * Parse a 40-character hexadecimal object ID starting from hex, updating the
- * pointer specified by end when parsing stops.  The resulting object ID is
- * stored in oid.  Returns 0 on success.  Parsing will stop on the first NUL or
- * other invalid character.  end is only updated on success; otherwise, it is
- * unmodified.
- */
-int parse_oid_hex(const char *hex, struct object_id *oid, const char **end);
-
-/* Like parse_oid_hex, but for an arbitrary hash algorithm. */
-int parse_oid_hex_algop(const char *hex, struct object_id *oid, const char **end,
-			const struct git_hash_algo *algo);
-
-
-/*
- * These functions work like get_oid_hex and parse_oid_hex, but they will parse
- * a hex value for any algorithm. The algorithm is detected based on the length
- * and the algorithm in use is returned. If this is not a hex object ID in any
- * algorithm, returns GIT_HASH_UNKNOWN.
- */
-int get_oid_hex_any(const char *hex, struct object_id *oid);
-int parse_oid_hex_any(const char *hex, struct object_id *oid, const char **end);
-
-/*
- * This reads short-hand syntax that not only evaluates to a commit
- * object name, but also can act as if the end user spelled the name
- * of the branch from the command line.
- *
- * - "@{-N}" finds the name of the Nth previous branch we were on, and
- *   places the name of the branch in the given buf and returns the
- *   number of characters parsed if successful.
- *
- * - "<branch>@{upstream}" finds the name of the other ref that
- *   <branch> is configured to merge with (missing <branch> defaults
- *   to the current branch), and places the name of the branch in the
- *   given buf and returns the number of characters parsed if
- *   successful.
- *
- * If the input is not of the accepted format, it returns a negative
- * number to signal an error.
- *
- * If the input was ok but there are not N branch switches in the
- * reflog, it returns 0.
- */
-#define INTERPRET_BRANCH_LOCAL (1<<0)
-#define INTERPRET_BRANCH_REMOTE (1<<1)
-#define INTERPRET_BRANCH_HEAD (1<<2)
-struct interpret_branch_name_options {
-	/*
-	 * If "allowed" is non-zero, it is a treated as a bitfield of allowable
-	 * expansions: local branches ("refs/heads/"), remote branches
-	 * ("refs/remotes/"), or "HEAD". If no "allowed" bits are set, any expansion is
-	 * allowed, even ones to refs outside of those namespaces.
-	 */
-	unsigned allowed;
-
-	/*
-	 * If ^{upstream} or ^{push} (or equivalent) is requested, and the
-	 * branch in question does not have such a reference, return -1 instead
-	 * of die()-ing.
-	 */
-	unsigned nonfatal_dangling_mark : 1;
-};
-int repo_interpret_branch_name(struct repository *r,
-			       const char *str, int len,
-			       struct strbuf *buf,
-			       const struct interpret_branch_name_options *options);
-#define interpret_branch_name(str, len, buf, options) \
-	repo_interpret_branch_name(the_repository, str, len, buf, options)
-
-int validate_headref(const char *ref);
-
-int base_name_compare(const char *name1, size_t len1, int mode1,
-		      const char *name2, size_t len2, int mode2);
-int df_name_compare(const char *name1, size_t len1, int mode1,
-		    const char *name2, size_t len2, int mode2);
-int name_compare(const char *name1, size_t len1, const char *name2, size_t len2);
-int cache_name_stage_compare(const char *name1, int len1, int stage1, const char *name2, int len2, int stage2);
-
-void *read_object_with_reference(struct repository *r,
-				 const struct object_id *oid,
-				 enum object_type required_type,
-				 unsigned long *size,
-				 struct object_id *oid_ret);
-
-struct object *repo_peel_to_type(struct repository *r,
-				 const char *name, int namelen,
-				 struct object *o, enum object_type);
-#define peel_to_type(name, namelen, obj, type) \
-	repo_peel_to_type(the_repository, name, namelen, obj, type)
-
-#define IDENT_STRICT	       1
-#define IDENT_NO_DATE	       2
-#define IDENT_NO_NAME	       4
-
-enum want_ident {
-	WANT_BLANK_IDENT,
-	WANT_AUTHOR_IDENT,
-	WANT_COMMITTER_IDENT
-};
-
-const char *git_author_info(int);
-const char *git_committer_info(int);
-const char *fmt_ident(const char *name, const char *email,
-		      enum want_ident whose_ident,
-		      const char *date_str, int);
-const char *fmt_name(enum want_ident);
-const char *ident_default_name(void);
-const char *ident_default_email(void);
-const char *git_editor(void);
-const char *git_sequence_editor(void);
-const char *git_pager(int stdout_is_tty);
-int is_terminal_dumb(void);
-int git_ident_config(const char *, const char *, void *);
-/*
- * Prepare an ident to fall back on if the user didn't configure it.
- */
-void prepare_fallback_ident(const char *name, const char *email);
-void reset_ident_date(void);
-
-struct ident_split {
-	const char *name_begin;
-	const char *name_end;
-	const char *mail_begin;
-	const char *mail_end;
-	const char *date_begin;
-	const char *date_end;
-	const char *tz_begin;
-	const char *tz_end;
-};
-/*
- * Signals an success with 0, but time part of the result may be NULL
- * if the input lacks timestamp and zone
- */
-int split_ident_line(struct ident_split *, const char *, int);
-
-/*
- * Given a commit or tag object buffer and the commit or tag headers, replaces
- * the idents in the headers with their canonical versions using the mailmap mechanism.
- */
-void apply_mailmap_to_header(struct strbuf *, const char **, struct string_list *);
-
-/*
- * Compare split idents for equality or strict ordering. Note that we
- * compare only the ident part of the line, ignoring any timestamp.
- *
- * Because there are two fields, we must choose one as the primary key; we
- * currently arbitrarily pick the email.
- */
-int ident_cmp(const struct ident_split *, const struct ident_split *);
-
-struct cache_def {
-	struct strbuf path;
-	int flags;
-	int track_flags;
-	int prefix_len_stat_func;
-};
-#define CACHE_DEF_INIT { \
-	.path = STRBUF_INIT, \
-}
-static inline void cache_def_clear(struct cache_def *cache)
-{
-	strbuf_release(&cache->path);
-}
-
-int has_symlink_leading_path(const char *name, int len);
-int threaded_has_symlink_leading_path(struct cache_def *, const char *, int);
-int check_leading_path(const char *name, int len, int warn_on_lstat_err);
-int has_dirs_only_path(const char *name, int len, int prefix_len);
-void invalidate_lstat_cache(void);
-void schedule_dir_for_removal(const char *name, int len);
-void remove_scheduled_dirs(void);
-
-struct pack_window {
-	struct pack_window *next;
-	unsigned char *base;
-	off_t offset;
-	size_t len;
-	unsigned int last_used;
-	unsigned int inuse_cnt;
-};
-
-struct pack_entry {
-	off_t offset;
-	struct packed_git *p;
-};
-
-/*
- * Create a temporary file rooted in the object database directory, or
- * die on failure. The filename is taken from "pattern", which should have the
- * usual "XXXXXX" trailer, and the resulting filename is written into the
- * "template" buffer. Returns the open descriptor.
- */
-int odb_mkstemp(struct strbuf *temp_filename, const char *pattern);
-
-/*
- * Create a pack .keep file named "name" (which should generally be the output
- * of odb_pack_name). Returns a file descriptor opened for writing, or -1 on
- * error.
- */
-int odb_pack_keep(const char *name);
-
-/*
- * Set this to 0 to prevent oid_object_info_extended() from fetching missing
- * blobs. This has a difference only if extensions.partialClone is set.
- *
- * Its default value is 1.
- */
-extern int fetch_if_missing;
-
-/* Dumb servers support */
-int update_server_info(int);
-
-const char *get_log_output_encoding(void);
-const char *get_commit_output_encoding(void);
-
-int committer_ident_sufficiently_given(void);
-int author_ident_sufficiently_given(void);
-
-extern const char *git_commit_encoding;
-extern const char *git_log_output_encoding;
-extern const char *git_mailmap_file;
-extern const char *git_mailmap_blob;
-
-/* IO helper functions */
-void maybe_flush_or_die(FILE *, const char *);
-__attribute__((format (printf, 2, 3)))
-void fprintf_or_die(FILE *, const char *fmt, ...);
-void fwrite_or_die(FILE *f, const void *buf, size_t count);
-void fflush_or_die(FILE *f);
-
-#define COPY_READ_ERROR (-2)
-#define COPY_WRITE_ERROR (-3)
-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);
-
-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);
-void fsync_component_or_die(enum fsync_component component, int fd, const char *msg);
-
-static inline int batch_fsync_enabled(enum fsync_component component)
-{
-	return (fsync_components & component) && (fsync_method == FSYNC_METHOD_BATCH);
-}
-
-ssize_t read_in_full(int fd, void *buf, size_t count);
-ssize_t write_in_full(int fd, const void *buf, size_t count);
-ssize_t pread_in_full(int fd, void *buf, size_t count, off_t offset);
-
-static inline ssize_t write_str_in_full(int fd, const char *str)
-{
-	return write_in_full(fd, str, strlen(str));
-}
-
-/**
- * Open (and truncate) the file at path, write the contents of buf to it,
- * and close it. Dies if any errors are encountered.
- */
-void write_file_buf(const char *path, const char *buf, size_t len);
-
-/**
- * Like write_file_buf(), but format the contents into a buffer first.
- * Additionally, write_file() will append a newline if one is not already
- * present, making it convenient to write text files:
- *
- *   write_file(path, "counter: %d", ctr);
- */
-__attribute__((format (printf, 2, 3)))
-void write_file(const char *path, const char *fmt, ...);
-
-/* pager.c */
-void setup_pager(void);
-int pager_in_use(void);
-extern int pager_use_color;
-int term_columns(void);
-void term_clear_line(void);
-int decimal_width(uintmax_t);
-int check_pager_config(const char *cmd);
-void prepare_pager_args(struct child_process *, const char *pager);
-
-extern const char *editor_program;
-extern const char *askpass_program;
-extern const char *excludes_file;
-
-/* base85 */
-int decode_85(char *dst, const char *line, int linelen);
-void encode_85(char *buf, const unsigned char *data, int bytes);
-
-/* pkt-line.c */
-void packet_trace_identity(const char *prog);
->>>>>>> 1b2b92753e (Merge branch 'jc/fix-aggressive-protection-2.39' into HEAD)
 
 /* add */
 /*
diff --git a/copy.h b/copy.h
index 057259a3a7..2af77cba86 100644
--- a/copy.h
+++ b/copy.h
@@ -7,18 +7,4 @@ 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);
-
 #endif /* COPY_H */

^ permalink raw reply related	[flat|nested] 36+ messages in thread

* Re: [PATCH 00/12] Fix various overly aggressive protections in 2.45.1 and friends
  2024-05-21 19:56 [PATCH 00/12] Fix various overly aggressive protections in 2.45.1 and friends Junio C Hamano
                   ` (13 preceding siblings ...)
  2024-05-21 20:45 ` [rPATCH 14/12] Merge branch 'jc/fix-aggressive-protection-2.40' Junio C Hamano
@ 2024-05-21 21:14 ` Johannes Schindelin
  2024-05-21 21:46   ` Junio C Hamano
  2024-05-22 10:01 ` Joey Hess
  15 siblings, 1 reply; 36+ messages in thread
From: Johannes Schindelin @ 2024-05-21 21:14 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

Hi Junio,

On Tue, 21 May 2024, Junio C Hamano wrote:

> I'll figure out a way to convey conflict resolutions as this topic
> gets merged up to newer maintenance tracks on the list so that
> people can assist with ensuring correctness of the result by
> reviewing, and follow up. ("git show --remerge-diff" might turn out
> to be such a way, but I do not know yet).

I pushed 12/12 to https://github.com/dscho/git's
`various-fixes-for-v2.45.1-and-friends` branch, and updated the
`tentative/maint-*` branches accordingly:

 + b9a96c4e5dc...c6da96aa5f0 maint-2.39 -> tentative/maint-2.39 (forced update)
 + 4bf5d57da62...fff57b200d1 maint-2.40 -> tentative/maint-2.40 (forced update)
 + 5215e4e3687...616450032a0 maint-2.41 -> tentative/maint-2.41 (forced update)
 + 33efa2ad1a6...b1ea89bc2d6 maint-2.42 -> tentative/maint-2.42 (forced update)
 + 0aeca2f80b1...093c42a6c6b maint-2.43 -> tentative/maint-2.43 (forced update)
 + 9953011fcdd...3c7a7b923b3 maint-2.44 -> tentative/maint-2.44 (forced update)

Ciao,
Johannes

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [rPATCH 14/12] Merge branch 'jc/fix-aggressive-protection-2.40'
  2024-05-21 20:45 ` [rPATCH 14/12] Merge branch 'jc/fix-aggressive-protection-2.40' Junio C Hamano
@ 2024-05-21 21:33   ` Junio C Hamano
  0 siblings, 0 replies; 36+ messages in thread
From: Junio C Hamano @ 2024-05-21 21:33 UTC (permalink / raw)
  To: git

Junio C Hamano <gitster@pobox.com> writes:

> The only tricky part is an evil merge to copy.h that was made
> necessarily due to recent header file shuffling.

Up to this step it seems there is no cascading impact if we dropped
the [12/12] from the series.  I forgot that hook.c needs to lose
"#include <copy.h>" during this merge, which also was made
necessarily due to recent header file shuffling.

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 00/12] Fix various overly aggressive protections in 2.45.1 and friends
  2024-05-21 21:14 ` [PATCH 00/12] Fix various overly aggressive protections in 2.45.1 and friends Johannes Schindelin
@ 2024-05-21 21:46   ` Junio C Hamano
  2024-05-21 22:13     ` Junio C Hamano
  0 siblings, 1 reply; 36+ messages in thread
From: Junio C Hamano @ 2024-05-21 21:46 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: git

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Tue, 21 May 2024, Junio C Hamano wrote:
>
>> I'll figure out a way to convey conflict resolutions as this topic
>> gets merged up to newer maintenance tracks on the list so that
>> people can assist with ensuring correctness of the result by
>> reviewing, and follow up. ("git show --remerge-diff" might turn out
>> to be such a way, but I do not know yet).
>
> I pushed 12/12 to https://github.com/dscho/git's
> `various-fixes-for-v2.45.1-and-friends` branch, and updated the
> `tentative/maint-*` branches accordingly:

Thanks, but I suspect it is not productive use of your time to merge
these up until we know we are happy with what we are merging up.

Even though I did the 12/12 that reverts the "iffy ownership check
during repository discovery", it is far from clear without
discussion if that is a reasonable thing to do or use of
safe.directory by narrow non-target audience (like k.org that may
use gitolite and/or bare git for hosting) that lets nobody access
trusted user repositories.  Every time we find new issues or
different solutions, somebody has to merge it up, eventually to
maint-2.45, but I am afraid it may be a bit too early to commit.

>  + b9a96c4e5dc...c6da96aa5f0 maint-2.39 -> tentative/maint-2.39 (forced update)
>  + 4bf5d57da62...fff57b200d1 maint-2.40 -> tentative/maint-2.40 (forced update)
>  + 5215e4e3687...616450032a0 maint-2.41 -> tentative/maint-2.41 (forced update)
>  + 33efa2ad1a6...b1ea89bc2d6 maint-2.42 -> tentative/maint-2.42 (forced update)
>  + 0aeca2f80b1...093c42a6c6b maint-2.43 -> tentative/maint-2.43 (forced update)
>  + 9953011fcdd...3c7a7b923b3 maint-2.44 -> tentative/maint-2.44 (forced update)
>
> Ciao,
> Johannes

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 00/12] Fix various overly aggressive protections in 2.45.1 and friends
  2024-05-21 21:46   ` Junio C Hamano
@ 2024-05-21 22:13     ` Junio C Hamano
  0 siblings, 0 replies; 36+ messages in thread
From: Junio C Hamano @ 2024-05-21 22:13 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: git

Junio C Hamano <gitster@pobox.com> writes:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>> On Tue, 21 May 2024, Junio C Hamano wrote:
>>
>>> I'll figure out a way to convey conflict resolutions as this topic
>>> gets merged up to newer maintenance tracks on the list so that
>>> people can assist with ensuring correctness of the result by
>>> reviewing, and follow up. ("git show --remerge-diff" might turn out
>>> to be such a way, but I do not know yet).
>>
>> I pushed 12/12 to https://github.com/dscho/git's
>> `various-fixes-for-v2.45.1-and-friends` branch, and updated the
>> `tentative/maint-*` branches accordingly:
>
> Thanks, but I suspect it is not productive use of your time to merge
> these up until we know we are happy with what we are merging up.
>
> Even though I did the 12/12 that reverts the "iffy ownership check
> during repository discovery", it is far from clear without
> discussion if that is a reasonable thing to do or use of
> safe.directory by narrow non-target audience (like k.org that may
> use gitolite and/or bare git for hosting) that lets nobody access
> trusted user repositories.  Every time we find new issues or
> different solutions, somebody has to merge it up, eventually to
> maint-2.45, but I am afraid it may be a bit too early to commit.

Another thing is that, now this is not an embargoed set of secret
releases, I'd want to have them go through 'next' down to 'master'
in the usual way, with the usual "develop in the open" fashion,
before we convince ourselves that the whole cascade is ready.  We
may find necessary updates while these fixes are in 'next' due to
$WORK folks feeding 'next' based updates to $CORP users and helping
us find issues, in which case we would need to add more patches on
top of the topic based on maint-2.39 (and merge it up all the way).
After that is all done, we would finally become ready to write
release notes for the tracks, which will be merged up the same way
as the "oops, we found we need another patch while the series was in
'next'" case.  Preparing tentative/maint-* set of branches your way,
with release notes and GIT-VERSION-GEN updates together ready to be
tagged, would not help prepare something that I can feed other
developers in 'seen' and then 'next'.

Thanks.

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 09/12] tests: verify that `clone -c core.hooksPath=/dev/null` works again
  2024-05-21 19:56 ` [PATCH 09/12] tests: verify that `clone -c core.hooksPath=/dev/null` works again Junio C Hamano
@ 2024-05-21 22:57   ` Brooke Kuhlmann
  0 siblings, 0 replies; 36+ messages in thread
From: Brooke Kuhlmann @ 2024-05-21 22:57 UTC (permalink / raw)
  To: Junio C Hamano, Johannes Schindelin; +Cc: git

Thanks, Junio and Johannes. Looking forward to making use of this again!

> On May 21, 2024, at 1:56 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> 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 <brooke@alchemists.io>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> 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
> -- 
> 2.45.1-216-g4365c6fcf9
> 


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 12/12] Revert "fetch/clone: detect dubious ownership of local repositories"
  2024-05-21 20:43   ` Junio C Hamano
@ 2024-05-22  7:27     ` Johannes Schindelin
  2024-05-22 17:20       ` Junio C Hamano
  0 siblings, 1 reply; 36+ messages in thread
From: Johannes Schindelin @ 2024-05-22  7:27 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

Hi Junio,

On Tue, 21 May 2024, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
>
> > This partially reverts f4aa8c8b (fetch/clone: detect dubious
> > ownership of local repositories, 2024-04-10) that broke typical
> > read-only use cases (e.g. by git-daemon serving fetches and clones)
> > where "nobody" who has no write permission serves repositories owned
> > by others.  The function to die upon seeing dubious ownership is
> > still kept, as there are other users of it, but calls to it from the
> > generic repository discovery code path, which triggered in cases far
> > wider than originally intended (i.e. to stop local clones), have
> > been removed.
> >
> > Signed-off-by: Junio C Hamano <gitster@pobox.com>
> > ---
>
> I am inclined to propose dropping this step, actually.
>
> cf. https://lore.kernel.org/git/xmqq7cfmaofl.fsf@gitster.g/

To https://github.com/dscho/git
 + c6da96aa5f0...f71d7009814 maint-2.39 -> tentative/maint-2.39 (forced update)
 + fff57b200d1...21dc6c4d521 maint-2.40 -> tentative/maint-2.40 (forced update)
 + 616450032a0...0d21b3451cd maint-2.41 -> tentative/maint-2.41 (forced update)
 + b1ea89bc2d6...e9bd0c8f8c4 maint-2.42 -> tentative/maint-2.42 (forced update)
 + 093c42a6c6b...9926037ce8c maint-2.43 -> tentative/maint-2.43 (forced update)
 + 3c7a7b923b3...aec5a9bf52c maint-2.44 -> tentative/maint-2.44 (forced update)
 + aeddcb02756...d3c56966d13 maint-2.45 -> tentative/maint-2.45 (forced update)

This command-line comes up with no differences (meaning: you resolved the
merge conflicts, even the ones without conflict markers, in the same way
as I did, which is good):

  for i in $(seq 39 45)
  do
    case $i in
    39) gitster=jc/fix-2.45.1-and-friends-for-2.39;;
    45) gitster=jc/fix-2.45.1-and-friends-for-maint;;
    *)  gitster=fixes/2.45.1/2.$i;;
    esac &&
    git diff gitster/$gitster..dscho/tentative/maint-2.$i -- \
      ':(exclude)Documentation/RelNotes/' \
      ':(exclude)GIT-VERSION-GEN' \
      ':(exclude)RelNotes'
  done

Ciao,
Johannes

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 02/12] send-email: avoid creating more than one Term::ReadLine object
  2024-05-21 19:56 ` [PATCH 02/12] send-email: avoid creating more than one Term::ReadLine object Junio C Hamano
@ 2024-05-22  8:15   ` Dragan Simic
  0 siblings, 0 replies; 36+ messages in thread
From: Dragan Simic @ 2024-05-22  8:15 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Jeff King, Taylor Blau

On 2024-05-21 21:56, Junio C Hamano wrote:
> From: Jeff King <peff@peff.net>
> 
> 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 <peff@peff.net>
> Acked-by: Taylor Blau <me@ttaylorr.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Looking good to me.  Thanks for taking care of this issue.

Reviewed-by: Dragan Simic <dsimic@manjaro.org>

> ---
>  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 @@ sub get_patch_subject {
>  	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 <author@example.com>\$" msgtxt1 &&
> -		grep "^To: to@example.com\$" msgtxt1
> +		grep "^To: to@example.com\$" msgtxt1 &&
> +		grep "^In-Reply-To: <my-message-id@example.com>" msgtxt1
>  '
> 
>  test_expect_success $PREREQ,AUTOIDENT 'implicit ident is allowed' '

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 01/12] send-email: drop FakeTerm hack
  2024-05-21 19:56 ` [PATCH 01/12] send-email: drop FakeTerm hack Junio C Hamano
@ 2024-05-22  8:19   ` Dragan Simic
  0 siblings, 0 replies; 36+ messages in thread
From: Dragan Simic @ 2024-05-22  8:19 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Jeff King, Taylor Blau

On 2024-05-21 21:56, Junio C Hamano wrote:
> From: Jeff King <peff@peff.net>
> 
> 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 <peff@peff.net>
> Acked-by: Taylor Blau <me@ttaylorr.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Looking good to me.  Thanks for taking care of this issue.

Reviewed-by: Dragan Simic <dsimic@manjaro.org>

> ---
>  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 @@
> 
>  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 <<EOT;
>  git send-email' [<options>] <file|directory>
> @@ -930,16 +918,10 @@ sub get_patch_subject {
>  }
> 
>  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 {

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 00/12] Fix various overly aggressive protections in 2.45.1 and friends
  2024-05-21 19:56 [PATCH 00/12] Fix various overly aggressive protections in 2.45.1 and friends Junio C Hamano
                   ` (14 preceding siblings ...)
  2024-05-21 21:14 ` [PATCH 00/12] Fix various overly aggressive protections in 2.45.1 and friends Johannes Schindelin
@ 2024-05-22 10:01 ` Joey Hess
  2024-05-23  5:49   ` Junio C Hamano
  15 siblings, 1 reply; 36+ messages in thread
From: Joey Hess @ 2024-05-22 10:01 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

[-- Attachment #1: Type: text/plain, Size: 967 bytes --]

Junio C Hamano wrote:
> As people have seen, the latest "security fix" release turned out to
> be a mixed bag of good vulnerability fixes with a bit over-eager
> "layered defence" that broke real uses cases like git-lfs.

"fsck: warn about symlink pointing inside a gitdir"
(a33fea0886cfa016d313d2bd66bdd08615bffbc9) also broke pushing git-annex
repositories to eg Gitlab and has several other problems including dodgy
PATH_MAX checks that cause new OS interoperability problems. (I posted
details to an earlier thread but have now found this current one, oops.)

Please also revert it, or at least the portions for 
and symlinkPointsToGitDir and symlinkTargetLength. The
checks for symlinkTargetBlob and symlinkTargetMissing seem worth
keeping.

> Let's quickly get them in working order back first, with the vision that
> we will then rebuild layered defence more carefully in the open on
> top as necessary.

Exellent plan.

-- 
see shy jo

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 12/12] Revert "fetch/clone: detect dubious ownership of local repositories"
  2024-05-22  7:27     ` Johannes Schindelin
@ 2024-05-22 17:20       ` Junio C Hamano
  0 siblings, 0 replies; 36+ messages in thread
From: Junio C Hamano @ 2024-05-22 17:20 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: git

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> To https://github.com/dscho/git
>  + c6da96aa5f0...f71d7009814 maint-2.39 -> tentative/maint-2.39 (forced update)
>  + fff57b200d1...21dc6c4d521 maint-2.40 -> tentative/maint-2.40 (forced update)
>  + 616450032a0...0d21b3451cd maint-2.41 -> tentative/maint-2.41 (forced update)
>  + b1ea89bc2d6...e9bd0c8f8c4 maint-2.42 -> tentative/maint-2.42 (forced update)
>  + 093c42a6c6b...9926037ce8c maint-2.43 -> tentative/maint-2.43 (forced update)
>  + 3c7a7b923b3...aec5a9bf52c maint-2.44 -> tentative/maint-2.44 (forced update)
>  + aeddcb02756...d3c56966d13 maint-2.45 -> tentative/maint-2.45 (forced update)
>
> This command-line comes up with no differences (meaning: you resolved the
> merge conflicts, even the ones without conflict markers, in the same way
> as I did, which is good):

Yeah, but I am afraid that it is a bit too premature to worry about
the integration to merge them up.  What's your take on the symlink
stuff Joey raised recently?

Thanks.


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 00/12] Fix various overly aggressive protections in 2.45.1 and friends
  2024-05-22 10:01 ` Joey Hess
@ 2024-05-23  5:49   ` Junio C Hamano
  2024-05-23 16:31     ` Joey Hess
  2024-05-23 23:32     ` Junio C Hamano
  0 siblings, 2 replies; 36+ messages in thread
From: Junio C Hamano @ 2024-05-23  5:49 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: git, Joey Hess

Joey Hess <id@joeyh.name> writes:

> Junio C Hamano wrote:
>> As people have seen, the latest "security fix" release turned out to
>> be a mixed bag of good vulnerability fixes with a bit over-eager
>> "layered defence" that broke real uses cases like git-lfs.
>
> "fsck: warn about symlink pointing inside a gitdir"
> (a33fea0886cfa016d313d2bd66bdd08615bffbc9) also broke pushing git-annex
> repositories to eg Gitlab and has several other problems including dodgy
> PATH_MAX checks that cause new OS interoperability problems. (I posted
> details to an earlier thread but have now found this current one, oops.)
>
> Please also revert it, or at least the portions for 
> and symlinkPointsToGitDir and symlinkTargetLength. The
> checks for symlinkTargetBlob and symlinkTargetMissing seem worth
> keeping.

Looking at the change in question, in a33fea08 (fsck: warn about
symlink pointing inside a gitdir, 2024-04-10), you said:

> fsck: warn about symlink pointing inside a gitdir
> 
> In the wake of fixing a vulnerability where `git clone` mistakenly
> followed a symbolic link that it had just written while checking out
> files, writing into a gitdir, let's add some defense-in-depth by
> teaching `git fsck` to report symbolic links stored in its trees that
> point inside `.git/`.
> 
> Even though the Git project never made any promises about the exact
> shape of the `.git/` directory's contents, there are likely repositories
> out there containing symbolic links that point inside the gitdir. For
> that reason, let's only report these as warnings, not as errors.
> Security-conscious users are encouraged to configure
> `fsck.symlinkPointsToGitDir = error`.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

I can make a few observations (in addition to what Joey already
observed in the code around PATH_MAX, etc. [*]):

 - Yes, if git-annex wants to keep its private data in a private
   directory .git/annex/objects it carved out for itself and want to
   point at them from the working tree with symbolic links, the
   extra check would trigger as fsck would see the symbolic link
   contents pointing into the .git/ directory, so certainly they
   would be affected.

 - The extra check seems to have meant to target the symbolic links
   that point at objects, refs, config, and anything _we_ care
   about, as opposed to random garbage (from _our_ point of view)
   files third-parties throw into .git/ directory.  Would it have
   made a better trade-off if we tried to make the check more
   precise, only complaining about the things we care about (in
   other words, what _we_ use), or will it become too brittle
   because what we care about will change over time?

 - In any case, if it is merely warnings, not errors, these checks
   can be configured out.  Wouldn't that be an escape-hatch enough?

So, I am ambivalent.  I have prepared a revert queued on maint-2.39
and cascaded it up to the maintenance track, which I tentatively
queued on top of 'seen', to see how much damage such a reversion
involves (and it did not look too bad), but

 (1) I am not sure if this is such a huge deal that requires a
     revert;

 (2) I am not sure which one is more practical between ripping
     everything out and demoting these new fsck error types with
     FSCK_WARN to FSCK_IGNORE.  If the structure of the code to
     perform these checks is more or less good and the actual check
     the code implements is bad, the latter might give us a better
     foundation to build on than rebuilding everything from scratch.
     On the other hand, if we are redoing things in the open, it may
     be better to forget about the code in 2.45.1/2.39.4 and to build
     from scratch for those reviewers and developers who will offer
     help.

 (3) As I look at the change by a33fea08 again, it actually adds a
     few new fsck error types with FSCK_ERROR.  Perhaps that is a
     good enough reason to do a straight revert for now?

Thanks.  It is past my bedtime so I won't be pushing out the 'seen'
with the revert I prepared as a practice, at least tonight.

[Reference]

 * https://lore.kernel.org/git/Zk2_mJpE7tJgqxSp@kitenet.net/

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Reviewing merge commits, was Re: [rPATCH 13/12] Merge branch 'jc/fix-aggressive-protection-2.39'
  2024-05-21 20:45 ` [rPATCH 13/12] Merge branch 'jc/fix-aggressive-protection-2.39' Junio C Hamano
@ 2024-05-23 10:36   ` Johannes Schindelin
  2024-05-23 14:41     ` Junio C Hamano
  0 siblings, 1 reply; 36+ messages in thread
From: Johannes Schindelin @ 2024-05-23 10:36 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

Hi Junio,

On Tue, 21 May 2024, Junio C Hamano wrote:

> If this format looks reasonable to folks as a way to review the result
> of merging up fixes, I'll follow up with "patches" for more recent
> maintenance tracks.

Personally, I find this format quite hard to parse, and I am concerned
that the focus on the merge conflicts may distract too much from verifying
the correctness of the result. Much of what is tricky about these merges
happens outside conflict markers.

If it was up to me to verify such fixes, short of using Git and validating
the correctness by performing the merge independently instead of trying to
accomplish the validation by looking at a plain-text mail, I would compare
the diff of `maint-2.39` to the diff of `maint-2.40`. Something like this
[*1*]:

  $ git range-diff \
    mbox:<(git diff v2.45.1...gitster/jc/fix-2.45.1-and-friends-for-2.39) \
    mbox:<(git diff v2.45.1...gitster/fixes/2.45.1/2.40)
  1:  00000000000 ! 1:  00000000000
      @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)

          if (real_git_dir) {
       @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
      -   free(unborn_head);
          free(dir);
          free(path);
      +   free(repo_to_free);
       -  free(template_dir_dup);
      -   UNLEAK(repo);
      ++  UNLEAK(repo);
          junk_mode = JUNK_LEAVE_ALL;

      +   transport_ls_refs_options_release(&transport_ls_refs_options);

        ## cache.h ##
       @@ cache.h: int copy_fd(int ifd, int ofd);
      @@ t/t1450-fsck.sh: test_expect_success 'fsck error on gitattributes with excessive
        test_done

        ## t/t1800-hook.sh ##
      -@@ t/t1800-hook.sh: test_expect_success 'git hook run a hook with a bad shebang' '
      +@@ t/t1800-hook.sh: test_expect_success 'stdin to hooks' '
          test_cmp expect actual
        '
It does get a bit more confusing with the diff between `maint-2.40` and
`maint-2.41` because the declaration of `do_files_match()` moved from
`cache.h` to `copy.h`, and the hunks removing that declaration are at
different locations in the diffs:

  $ git range-diff \
    mbox:<(git diff v2.45.1...gitster/fixes/2.45.1/2.40) \
    mbox:<(git diff v2.45.1...gitster/fixes/2.45.1/2.41)
  1:  00000000000 ! 1:  00000000000
      @@ Makefile: exec-cmd.sp exec-cmd.s exec-cmd.o: EXTRA_CPPFLAGS = \

        ## builtin/clone.c ##
       @@ builtin/clone.c: 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;
      + 	int hash_algo;
       -	const char *template_dir;
       -	char *template_dir_dup = NULL;

      @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)

        	transport_ls_refs_options_release(&transport_ls_refs_options);

      - ## cache.h ##
      -@@ cache.h: 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);
      -
        ## ci/install-dependencies.sh ##
       @@ ci/install-dependencies.sh: macos-*)
        	export HOMEBREW_NO_AUTO_UPDATE=1 HOMEBREW_NO_INSTALL_CLEANUP=1
      @@ copy.c: int copy_file_with_time(const char *dst, const char *src, int mode)
       -	return ret;
       -}

      + ## copy.h ##
      +@@ copy.h: 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);
      +-
      + #endif /* COPY_H */
      +
        ## fsck.c ##
       @@ fsck.c: static int fsck_tree(const struct object_id *tree_oid,
        				retval += report(options, tree_oid, OBJ_TREE,
      @@ git-send-email.perl: sub get_patch_subject {

        ## hook.c ##
       @@
      - #include "run-command.h"
      - #include "config.h"
      -
      + #include "strbuf.h"
      + #include "environment.h"
      + #include "setup.h"
      +-#include "copy.h"
      +-
       -static int identical_to_template_hook(const char *name, const char *path)
       -{
       -	const char *env = getenv("GIT_CLONE_TEMPLATE_DIR");
      @@ hook.c
       -	strbuf_release(&template_path);
       -	return ret;
       -}
      --
      +
        const char *find_hook(const char *name)
        {
      - 	static struct strbuf path = STRBUF_INIT;
       @@ hook.c: const char *find_hook(const char *name)
        		}
        		return NULL;
      @@ t/t1350-config-hooks-path.sh: test_expect_success 'git rev-parse --git-path hook
        test_done

        ## t/t1450-fsck.sh ##
      -@@ t/t1450-fsck.sh: test_expect_success 'fsck error on gitattributes with excessive size' '
      - 	test_cmp expected actual
      +@@ t/t1450-fsck.sh: test_expect_success 'fsck reports problems in main index without filename' '
      + 	test_cmp expect actual
        '

       -test_expect_success 'fsck warning on symlink target with excessive length' '

Another thing that makes the review of these diffs-of-diffs harder than
necessary is the lack of color-coding in plain-text emails. When I look at
the color-coded version, my eyes are immediately drawn away from the
unimportant lines that start with a `-` or `+` but then continue in
uncolored text that indicates context line changes only. Instead, my eyes
shift immediately to the relevant parts, the blankets of red color.

Both the remerge-diff and the range-diff output do nothing, though, to
help verifying that no-longer-needed `#include`s are removed (you can see
that `#include "copy.h"` was removed from `hook.c`, but if that had been
missed there would be no indicator thereof).

So while I find this output more useful than the remerge diffs, it is
still far from ideal. I will therefore refrain from posting the
range-diffs that correspond to the remaining `maint-*` merges.

Ciao,
Johannes

Footnote *1*: This `mbox:` construct requires
https://github.com/gitgitgadget/git/pull/1420 (or `js/range-diff-mbox`) to
work as intended.

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: Reviewing merge commits, was Re: [rPATCH 13/12] Merge branch 'jc/fix-aggressive-protection-2.39'
  2024-05-23 10:36   ` Reviewing merge commits, was " Johannes Schindelin
@ 2024-05-23 14:41     ` Junio C Hamano
  0 siblings, 0 replies; 36+ messages in thread
From: Junio C Hamano @ 2024-05-23 14:41 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: git

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Much of what is tricky about these merges
> happens outside conflict markers.

Yes.  In other words, what you do not see in these outputs can be
leaving semantic conflicts unresolved.  If we made a change to a
location where there wasn't a textual conflict (i.e., making an evil
merge) in order to adjust for a semantic differences (e.g., they
added a callsite to a function whose signature we updated), any of
"git show --cc", "git range-diff", and "git show --remerge-diff"
would show.  If you failed to do so and introduced a bug (and worse
yet, unlike "oh, we now require one more argument to the function"
that compilers would catch for us, there are differences that
compilers would not notice), none of the three will show.

> If it was up to me to verify such fixes, short of using Git and validating
> the correctness by performing the merge independently instead of trying to
> accomplish the validation by looking at a plain-text mail, I would compare
> the diff of `maint-2.39` to the diff of `maint-2.40`. Something like this
> [*1*]:
> ...
> Both the remerge-diff and the range-diff output do nothing, though, to
> help verifying that no-longer-needed `#include`s are removed (you can see
> that `#include "copy.h"` was removed from `hook.c`, but if that had been
> missed there would be no indicator thereof).

In short, you are agreeing that between "remerge-diff" and "range-diff"
there is no difference in power to let you notice what is *not* there,
and I am agreeing to your "what is *not* there is often more problematic",
so we are on the same page.

I do not think I am married to "remerge-diff" output, but it being
the same form of the familiar "single-parent" patch, I personally
find it far easier to read than "diff of diff".  We may differ at
that point, but it does not matter, as neither of our preferred
format is adequate for the job.

;-)

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 00/12] Fix various overly aggressive protections in 2.45.1 and friends
  2024-05-23  5:49   ` Junio C Hamano
@ 2024-05-23 16:31     ` Joey Hess
  2024-05-27 19:51       ` Johannes Schindelin
  2024-05-23 23:32     ` Junio C Hamano
  1 sibling, 1 reply; 36+ messages in thread
From: Joey Hess @ 2024-05-23 16:31 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Johannes Schindelin, git

[-- Attachment #1: Type: text/plain, Size: 3276 bytes --]

Junio C Hamano wrote:
>  - The extra check seems to have meant to target the symbolic links
>    that point at objects, refs, config, and anything _we_ care
>    about, as opposed to random garbage (from _our_ point of view)
>    files third-parties throw into .git/ directory.  Would it have
>    made a better trade-off if we tried to make the check more
>    precise, only complaining about the things we care about (in
>    other words, what _we_ use)

I wondered about that possibility too. But it's not at all clear to
me how a symlink to .git/objects/foo risks any more security problem
to git than one to .git/annex/whatever, or indeed to /home/linus/.bashrc.

Git clearly has to get the security right of handling working tree files
that are symlinks.

The security hole that triggered this defense in depth, CVE-2024-32021,
involved an attacker with write access to .git/objects/ making a symlink
in there while another repo was cloning it. So it involved symlinks
inside a remote .git/objects/, which is very different than symlinks
into .git/objects/.

While it's understandable that dealing with such a symlink related
security hole may make one want to throw out the baby with the
bathwater, this fsck check is more like you've kept the bathwater and
only thrown out the baby. ;-)

>  - In any case, if it is merely warnings, not errors, these checks
>    can be configured out.  Wouldn't that be an escape-hatch enough?

The issue with that is, as we've experienced with Gitlab, git hosts that
choose to set receive.fsckObjects will prevent pushes of git-annex
repositories, and there's probably no way for a user to configure it
out. So every major git host that does it has to be approached to
configure it out, and some fraction probably won't. Which will be a
major impact and ongoing concern for git-annex users[1], all for
something that certainly adds no security to a bare repository on such a
host.

> I am not sure which one is more practical between ripping
> everything out and demoting these new fsck error types with
> FSCK_WARN to FSCK_IGNORE. 

It could indeed be beneficial to have some kind of symlink check that is
at FSCK_IGNORE by default. If someone is receiving a repository from an
untrusted source, and doesn't want to deal with the security risks of
symlinks in the working tree, they could configure it to be an error.
Such a symlink check would probably need to catch more symlinks than
only the ones into .git/ though. Having this available to git users
seems like it could prevent a much larger class of security holes.

As for the symlink length check, I do think it makes sense for fsck to
notice symlinks that are too long to make sense for any OS and so picking
some appropriate value, rather than the local PATH_MAX, could keep that one.

-- 
see shy jo

[1] I'm particularly concerned about the class of large institutional
    users who are managing more data with git-annex than the total size of
    all of Github[2]. They have a good reason to be risk averse,
    and it could be a major disruption to cross-organizational workflows
    and need updates to DOIs etc for them to switch hosting providers.
[2] https://hachyderm.io/@joeyh/112486445240754919

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 00/12] Fix various overly aggressive protections in 2.45.1 and friends
  2024-05-23  5:49   ` Junio C Hamano
  2024-05-23 16:31     ` Joey Hess
@ 2024-05-23 23:32     ` Junio C Hamano
  1 sibling, 0 replies; 36+ messages in thread
From: Junio C Hamano @ 2024-05-23 23:32 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: git, Joey Hess

Junio C Hamano <gitster@pobox.com> writes:

> Joey Hess <id@joeyh.name> writes:
>
>> Please also revert it, or at least the portions for 
>> and symlinkPointsToGitDir and symlinkTargetLength. The
>> checks for symlinkTargetBlob and symlinkTargetMissing seem worth
>> keeping.
>
> Looking at the change in question, in a33fea08 (fsck: warn about
> symlink pointing inside a gitdir, 2024-04-10), you said:
> ...
> So, I am ambivalent.  I have prepared a revert queued on maint-2.39
> and cascaded it up to the maintenance track, which I tentatively
> queued on top of 'seen', to see how much damage such a reversion
> involves (and it did not look too bad), but
>
>  (1) I am not sure if this is such a huge deal that requires a
>      revert;
>
>  (2) I am not sure which one is more practical between ripping
>      everything out and demoting these new fsck error types with
>      FSCK_WARN to FSCK_IGNORE.  If the structure of the code to
>      perform these checks is more or less good and the actual check
>      the code implements is bad, the latter might give us a better
>      foundation to build on than rebuilding everything from scratch.
>      On the other hand, if we are redoing things in the open, it may
>      be better to forget about the code in 2.45.1/2.39.4 and to build
>      from scratch for those reviewers and developers who will offer
>      help.
>
>  (3) As I look at the change by a33fea08 again, it actually adds a
>      few new fsck error types with FSCK_ERROR.  Perhaps that is a
>      good enough reason to do a straight revert for now?
>
> Thanks.  It is past my bedtime so I won't be pushing out the 'seen'
> with the revert I prepared as a practice, at least tonight.

So at least for now, I've queued a full revert of the change in
question in the "revert over-eager layering-on-top changes" pile,
but as we haven't really seen anybody give good input to help me
decide what to do with this step, the pile is still kept outside of
the 'next' branch.  At least it is visible on 'seen' as of tonight.

Thanks.

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 00/12] Fix various overly aggressive protections in 2.45.1 and friends
  2024-05-23 16:31     ` Joey Hess
@ 2024-05-27 19:51       ` Johannes Schindelin
  2024-05-28  2:25         ` Joey Hess
  2024-05-28 15:02         ` Phillip Wood
  0 siblings, 2 replies; 36+ messages in thread
From: Johannes Schindelin @ 2024-05-27 19:51 UTC (permalink / raw)
  To: Joey Hess; +Cc: Junio C Hamano, git

[-- Attachment #1: Type: text/plain, Size: 2097 bytes --]

Hi Joey,

On Thu, 23 May 2024, Joey Hess wrote:

> Junio C Hamano wrote:
> >  - The extra check seems to have meant to target the symbolic links
> >    that point at objects, refs, config, and anything _we_ care
> >    about, as opposed to random garbage (from _our_ point of view)
> >    files third-parties throw into .git/ directory.  Would it have
> >    made a better trade-off if we tried to make the check more
> >    precise, only complaining about the things we care about (in
> >    other words, what _we_ use)
>
> I wondered about that possibility too. But it's not at all clear to
> me how a symlink to .git/objects/foo risks any more security problem
> to git than one to .git/annex/whatever, or indeed to /home/linus/.bashrc.

It risks more security problems because `.git/objects/??/*` is not
re-hashed when it is being used by Git. That's a very easy way to slip in
unwanted file contents.

And there is a good reason _not_ to write stuff inside the `.git/`
directory unless you happen to be, well, Git itself: Git makes no
guarantees whatsoever that you can write into that directory whatever you
want. A future Git version might even write a file `.git/annex`, breaking
`git-annex`' assumptions, and that'd be totally within the guarantees Git
makes.

> Git clearly has to get the security right of handling working tree files
> that are symlinks.
>
> The security hole that triggered this defense in depth, CVE-2024-32021,
> involved an attacker with write access to .git/objects/ making a symlink
> in there while another repo was cloning it. So it involved symlinks
> inside a remote .git/objects/, which is very different than symlinks
> into .git/objects/.

No, the vulnerability that triggered this defense-in-depth was not
CVE-2024-32021, but instead CVE-2024-32002, a critical security issue.

Removing the defense-in-depth makes it more likely for otherwise
relatively harmless bugs à la "oh whoops, we wrote something we shouldn't
have" to escalate to full, critical Remote Code Execution vulnerabilities.

Ciao,
Johannes

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 00/12] Fix various overly aggressive protections in 2.45.1 and friends
  2024-05-27 19:51       ` Johannes Schindelin
@ 2024-05-28  2:25         ` Joey Hess
  2024-05-28 15:02         ` Phillip Wood
  1 sibling, 0 replies; 36+ messages in thread
From: Joey Hess @ 2024-05-28  2:25 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Junio C Hamano, git

[-- Attachment #1: Type: text/plain, Size: 1081 bytes --]

Johannes Schindelin wrote:
> And there is a good reason _not_ to write stuff inside the `.git/`
> directory unless you happen to be, well, Git itself: Git makes no
> guarantees whatsoever that you can write into that directory whatever you
> want. A future Git version might even write a file `.git/annex`, breaking
> `git-annex`' assumptions, and that'd be totally within the guarantees Git
> makes.

Well git-annex is hardly the only program to decide to carve out
part of .git/ for its own use. For example, git-lfs uses .git/lfs/
rather similarly.

Anyway, I hope I can ask nicely and not have tne git developers choose
to use .git/annex/ for something. Since it would cause a large amount of
pain to a large number of users, who would all have to rebase histories
of (often massive) git repos to update symlinks pointing there.

> No, the vulnerability that triggered this defense-in-depth was not
> CVE-2024-32021, but instead CVE-2024-32002, a critical security issue.

Ahh, thanks, I understand the concerns a little bit better now.

-- 
see shy jo

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 00/12] Fix various overly aggressive protections in 2.45.1 and friends
  2024-05-27 19:51       ` Johannes Schindelin
  2024-05-28  2:25         ` Joey Hess
@ 2024-05-28 15:02         ` Phillip Wood
  2024-05-28 16:13           ` Junio C Hamano
  2024-05-28 17:47           ` Junio C Hamano
  1 sibling, 2 replies; 36+ messages in thread
From: Phillip Wood @ 2024-05-28 15:02 UTC (permalink / raw)
  To: Johannes Schindelin, Joey Hess; +Cc: Junio C Hamano, git

Hi Johannes

On 27/05/2024 20:51, Johannes Schindelin wrote:
> Hi Joey,
> 
> On Thu, 23 May 2024, Joey Hess wrote:
> 
>> Junio C Hamano wrote:
>>>   - The extra check seems to have meant to target the symbolic links
>>>     that point at objects, refs, config, and anything _we_ care
>>>     about, as opposed to random garbage (from _our_ point of view)
>>>     files third-parties throw into .git/ directory.  Would it have
>>>     made a better trade-off if we tried to make the check more
>>>     precise, only complaining about the things we care about (in
>>>     other words, what _we_ use)
>>
>> I wondered about that possibility too. But it's not at all clear to
>> me how a symlink to .git/objects/foo risks any more security problem
>> to git than one to .git/annex/whatever, or indeed to /home/linus/.bashrc.
> 
> It risks more security problems because `.git/objects/??/*` is not
> re-hashed when it is being used by Git. That's a very easy way to slip in
> unwanted file contents.

What checks do we have in place to prevent git checking out blobs and 
gitlinks to paths under .git/? I'd have thought we should be applying 
the same restrictions to the target of symbolic links as we do to those.

> And there is a good reason _not_ to write stuff inside the `.git/`
> directory unless you happen to be, well, Git itself: Git makes no
> guarantees whatsoever that you can write into that directory whatever you
> want. A future Git version might even write a file `.git/annex`, breaking
> `git-annex`' assumptions, and that'd be totally within the guarantees Git
> makes.

This seems a bit harsh - many tools store their state under .git/ and I 
think it makes sense for them to do so as it avoids creating untracked 
files in the working copy. I would hope that we'd be considerate of 
widely used tools such as 'git annex' when adding new paths under .git/

Best Wishes

Phillip

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 00/12] Fix various overly aggressive protections in 2.45.1 and friends
  2024-05-28 15:02         ` Phillip Wood
@ 2024-05-28 16:13           ` Junio C Hamano
  2024-05-28 17:47           ` Junio C Hamano
  1 sibling, 0 replies; 36+ messages in thread
From: Junio C Hamano @ 2024-05-28 16:13 UTC (permalink / raw)
  To: Phillip Wood; +Cc: Johannes Schindelin, Joey Hess, git

Phillip Wood <phillip.wood123@gmail.com> writes:

> What checks do we have in place to prevent git checking out blobs and
> gitlinks to paths under .git/? I'd have thought we should be applying
> the same restrictions to the target of symbolic links as we do to
> those.

We do not even allow ".git" slip into the index (most likely from a
malicious tree object), so a direct "checkout" is not much of an
issue.  Of course you can introduce bugs to that regular mechanism
in the future but that is not the target for 2.45.1's check we are
going to revert.  I think what Dscho worries about in his message is
that we might by mistake write via a symbolic link in the working
tree.  If our procedure to update a checked out blob in the working
tree were open/truncate/write/close an existing file, a checkout
that switches from a version with a symbolic link at path F to a
version with a regular file at path F may end up overwriting the
target of F.  I think the idea was (Dscho can correct me if I am
misleading the log messge of a33fea08 (fsck: warn about symlink
pointing inside a gitdir, 2024-04-10)) that such a bug from
overwriting a file in our repository if we did not allow a symbolic
link F to point into our repository.

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 00/12] Fix various overly aggressive protections in 2.45.1 and friends
  2024-05-28 15:02         ` Phillip Wood
  2024-05-28 16:13           ` Junio C Hamano
@ 2024-05-28 17:47           ` Junio C Hamano
  1 sibling, 0 replies; 36+ messages in thread
From: Junio C Hamano @ 2024-05-28 17:47 UTC (permalink / raw)
  To: Phillip Wood; +Cc: Johannes Schindelin, Joey Hess, git

Phillip Wood <phillip.wood123@gmail.com> writes:

>> And there is a good reason _not_ to write stuff inside the `.git/`
>> directory unless you happen to be, well, Git itself: Git makes no
>> guarantees whatsoever that you can write into that directory whatever you
>> want. A future Git version might even write a file `.git/annex`, breaking
>> `git-annex`' assumptions, and that'd be totally within the guarantees Git
>> makes.
>
> This seems a bit harsh - many tools store their state under .git/ and
> I think it makes sense for them to do so as it avoids creating
> untracked files in the working copy. I would hope that we'd be
> considerate of widely used tools such as 'git annex' when adding new
> paths under .git/

Yes, a .git/annex file _can_ happen, but between civilized developer
groups, such a thing would not happen without a good reason.  If we
have no good reason (apparently you and I did not think of any) to
create such a file, "it can happen" is a poor straw-man, as we would
be aiming to work well together.

Yes, when we have a symbolic link as a tracked content, updating the
target of the link when we need to update it is simply a bug, and it
does not matter if it points at a file inside our own repository, or
a file inside a different and unrelated repository that is owned by
the same user, or a file in the user's home directory.  Our own
repository is not all that special from that perspective, and a
change to penalize symbolic links that point into our repository
specifically probably did make a bad choice.

Thanks.

^ permalink raw reply	[flat|nested] 36+ messages in thread

end of thread, other threads:[~2024-05-28 17:47 UTC | newest]

Thread overview: 36+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-21 19:56 [PATCH 00/12] Fix various overly aggressive protections in 2.45.1 and friends Junio C Hamano
2024-05-21 19:56 ` [PATCH 01/12] send-email: drop FakeTerm hack Junio C Hamano
2024-05-22  8:19   ` Dragan Simic
2024-05-21 19:56 ` [PATCH 02/12] send-email: avoid creating more than one Term::ReadLine object Junio C Hamano
2024-05-22  8:15   ` Dragan Simic
2024-05-21 19:56 ` [PATCH 03/12] ci: drop mention of BREW_INSTALL_PACKAGES variable Junio C Hamano
2024-05-21 19:56 ` [PATCH 04/12] ci: avoid bare "gcc" for osx-gcc job Junio C Hamano
2024-05-21 19:56 ` [PATCH 05/12] ci: stop installing "gcc-13" for osx-gcc Junio C Hamano
2024-05-21 19:56 ` [PATCH 06/12] hook: plug a new memory leak Junio C Hamano
2024-05-21 19:56 ` [PATCH 07/12] init: use the correct path of the templates directory again Junio C Hamano
2024-05-21 19:56 ` [PATCH 08/12] Revert "core.hooksPath: add some protection while cloning" Junio C Hamano
2024-05-21 19:56 ` [PATCH 09/12] tests: verify that `clone -c core.hooksPath=/dev/null` works again Junio C Hamano
2024-05-21 22:57   ` Brooke Kuhlmann
2024-05-21 19:56 ` [PATCH 10/12] clone: drop the protections where hooks aren't run Junio C Hamano
2024-05-21 19:56 ` [PATCH 11/12] Revert "Add a helper function to compare file contents" Junio C Hamano
2024-05-21 19:56 ` [PATCH 12/12] Revert "fetch/clone: detect dubious ownership of local repositories" Junio C Hamano
2024-05-21 20:43   ` Junio C Hamano
2024-05-22  7:27     ` Johannes Schindelin
2024-05-22 17:20       ` Junio C Hamano
2024-05-21 20:45 ` [rPATCH 13/12] Merge branch 'jc/fix-aggressive-protection-2.39' Junio C Hamano
2024-05-23 10:36   ` Reviewing merge commits, was " Johannes Schindelin
2024-05-23 14:41     ` Junio C Hamano
2024-05-21 20:45 ` [rPATCH 14/12] Merge branch 'jc/fix-aggressive-protection-2.40' Junio C Hamano
2024-05-21 21:33   ` Junio C Hamano
2024-05-21 21:14 ` [PATCH 00/12] Fix various overly aggressive protections in 2.45.1 and friends Johannes Schindelin
2024-05-21 21:46   ` Junio C Hamano
2024-05-21 22:13     ` Junio C Hamano
2024-05-22 10:01 ` Joey Hess
2024-05-23  5:49   ` Junio C Hamano
2024-05-23 16:31     ` Joey Hess
2024-05-27 19:51       ` Johannes Schindelin
2024-05-28  2:25         ` Joey Hess
2024-05-28 15:02         ` Phillip Wood
2024-05-28 16:13           ` Junio C Hamano
2024-05-28 17:47           ` Junio C Hamano
2024-05-23 23:32     ` Junio C Hamano

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).