git 2.46.0 crashes when trying to verify-pack outside of a repo

git 2.46.0 crashes when trying to verify-pack outside of a repo

[Ilya K]

Hello folks!

We've updated to Git 2.46.0 in NixOS, and encountered an issue with Dulwich (a Python Git implementation) tests failing[0] because it attempts to call `git verify-pack` on a bare pack, with no surrounding repo. This used to work in Git 2.45.x, but in 2.46 it simply prints "error: index-pack died of signal 11". This seems to happen on any pack, including ones generated by Git itself (at least on a random pack from a random checkout I tested).

Here's a traceback from the crash:

    (gdb) bt
    #0  0x00000000005d93ef in add_packed_git (
        path=path@entry=0x7dd8f0 "pack-a94057e69dcef307b749cd32232e845c48bb32c9.idx", path_len=45,
        local=local@entry=1) at packfile.c:754
    754             if (path_len < the_hash_algo->hexsz ||
    #1  0x000000000046bd38 in read_idx_option (
        pack_name=0x7dd8f0 "pack-a94057e69dcef307b749cd32232e845c48bb32c9.idx", opts=0x7fffffffcd00)
        at builtin/index-pack.c:1652
    1652            struct packed_git *p = add_packed_git(pack_name, strlen(pack_name), 1);
    #2  cmd_index_pack (argc=3, argv=<optimized out>, prefix=<optimized out>)
        at builtin/index-pack.c:1883
    1883                    read_idx_option(&opts, index_name);
    #3  0x000000000040651d in run_builtin (argv=0x7fffffffd270, argc=3, p=0x7a0560 <commands+1344>)
        at git.c:476
    476             status = p->fn(argc, argv, prefix);
    #4  handle_builtin (argc=3, argv=0x7fffffffd270) at git.c:732
    732                     exit(run_builtin(builtin, argc, argv));
    #5  0x0000000000407690 in run_argv (argcp=argcp@entry=0x7fffffffd04c,
        argv=argv@entry=0x7fffffffd040) at git.c:796
    796                             handle_builtin(*argcp, *argv);
    #6  0x000000000040812c in cmd_main (argc=<optimized out>, argc@entry=4, argv=<optimized out>,
        argv@entry=0x7fffffffd268) at git.c:931
    931                     int was_alias = run_argv(&argc, &argv);
    #7  0x00000000004061fd in main (argc=4, argv=0x7fffffffd268) at common-main.c:64
    64              result = cmd_main(argc, argv);

It seems like `the_hash_algo` (which, AFAIUI, is macroed to `the_repository->hash_algo`) may be uninitialized here, but I'm not familiar enough with the codebase to really tell if that's the issue.

If you want to reproduce it, simply copy any pack file from a repo's .git/objects/pack/ to a directory that's not a repo, then `git verify-pack <the-file-you-just-copied>`.

If you need any more information, please contact me.

Thanks in advance!

[0]: https://github.com/jelmer/dulwich/issues/1359

Re: git 2.46.0 crashes when trying to verify-pack outside of a repo

[Junio C Hamano]

Ilya K <me@0upti.me> writes:

> We've updated to Git 2.46.0 in NixOS, and encountered an issue
> with Dulwich (a Python Git implementation) tests failing[0]
> because it attempts to call `git verify-pack` on a bare pack, with
> no surrounding repo. This used to work in Git 2.45.x, but in 2.46
> it simply prints "error: index-pack died of signal 11".

Thanks.  This is a fallout from code-wide clean-up in 2.46.0 where
we do not assume that everybody runs SHA-1.

------- >8 -------
Subject: verify-pack: fall back to SHA-1 outside a repo

In c8aed5e8da (repository: stop setting SHA1 as the default object hash,
2024-05-07), we have stopped setting the default hash algorithm for
`the_repository`. Consequently, code that relies on `the_hash_algo` will
now crash when it hasn't explicitly been initialized, which may be the
case when running outside of a Git repository.

As the verify-pack command ought to be able to infer what algorithm
is used in the input file (and if the input file does not have such
an information, that by itself is a problem), and the command allows
an option to explicitly tell what algorithm to use in case it cannot
be guessed from the input file, in theory we shouldn't have to use
the default algorithm anywhere in the operation of the command, but
we fail fairly early in the process when run outside a repository
without any default algorithm set.

Resurrect the setting of the default algorithm just like we used to
do before 2.46.0

Reported-by: Ilya K <me@0upti.me>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/verify-pack.c  | 4 ++++
 t/t5300-pack-object.sh | 4 ++++
 2 files changed, 8 insertions(+)

diff --git c/builtin/verify-pack.c w/builtin/verify-pack.c
index 011dddd2dc..5b663905ae 100644
--- c/builtin/verify-pack.c
+++ w/builtin/verify-pack.c
@@ -1,6 +1,7 @@
 #include "builtin.h"
 #include "config.h"
 #include "gettext.h"
+#include "hash.h"
 #include "run-command.h"
 #include "parse-options.h"
 #include "strbuf.h"
@@ -77,6 +78,9 @@ int cmd_verify_pack(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
+	if (!the_hash_algo)
+		repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
+
 	git_config(git_default_config, NULL);
 	argc = parse_options(argc, argv, prefix, verify_pack_options,
 			     verify_pack_usage, 0);
diff --git c/t/t5300-pack-object.sh w/t/t5300-pack-object.sh
index 4ad023c846..d6f45d8923 100755
--- c/t/t5300-pack-object.sh
+++ w/t/t5300-pack-object.sh
@@ -322,6 +322,10 @@ test_expect_success 'verify-pack catches a corrupted sum of the index file itsel
 	fi
 '
 
+test_expect_success 'verify-pack outside a repository' '
+	nongit git verify-pack -v "$(pwd)/test-1-${packname_1}.idx"
+'
+
 test_expect_success 'build pack index for an existing pack' '
 	cat test-1-${packname_1}.pack >test-3.pack &&
 	git index-pack -o tmp.idx test-3.pack &&

Re: git 2.46.0 crashes when trying to verify-pack outside of a repo

[Patrick Steinhardt]

On Sun, Sep 01, 2024 at 08:26:08AM -0700, Junio C Hamano wrote:
> Ilya K <me@0upti.me> writes:
> 
> > We've updated to Git 2.46.0 in NixOS, and encountered an issue
> > with Dulwich (a Python Git implementation) tests failing[0]
> > because it attempts to call `git verify-pack` on a bare pack, with
> > no surrounding repo. This used to work in Git 2.45.x, but in 2.46
> > it simply prints "error: index-pack died of signal 11".
> 
> Thanks.  This is a fallout from code-wide clean-up in 2.46.0 where
> we do not assume that everybody runs SHA-1.

Yup, indeed. The problem lies deeper than what the below patch fixes
though. The issue isn't in git-verify-pack(1), but in git-index-pack(1),
and can be fixed like this:

    diff --git a/builtin/index-pack.c b/builtin/index-pack.c
    index fd968d673d2..e6edd96d099 100644
    --- a/builtin/index-pack.c
    +++ b/builtin/index-pack.c
    @@ -1733,7 +1733,7 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
        unsigned char pack_hash[GIT_MAX_RAWSZ];
        unsigned foreign_nr = 1;	/* zero is a "good" value, assume bad */
        int report_end_of_input = 0;
    -	int hash_algo = 0;
    +	int hash_algo = GIT_HASH_UNKNOWN;;
     
        /*
         * index-pack never needs to fetch missing objects except when
    @@ -1857,6 +1857,9 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
            pack_name = arg;
        }
     
    +	if (!the_repository->hash_algo && hash_algo == GIT_HASH_UNKNOWN)
    +		repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
    +
        if (!pack_name && !from_stdin)
            usage(index_pack_usage);
        if (fix_thin_pack && !from_stdin)

Unfortunately, this once again uncovers a deeper issue: neither the
packfile nor their index encode the object format they use. So while
falling back to SHA1 papers over the issue, it means that we misparse
SHA256 indices. Also, we misparse SHA1 indices if we happen to be in a
SHA256 repository. E.g. when parsing a SHA256 file in a SHA1 repo:

    $ git index-pack --verify '/tmp/git-tests/trash directory.t5300-pack-object/repo/.git/objects/pack/pack-aa45f7f08f043c9f0388f1844a2a797587254e249919b35ac9dc2b52c1aada29.pack'
    error: wrong index v2 file size in /tmp/git-tests/trash directory.t5300-pack-object/repo/.git/objects/pack/pack-aa45f7f08f043c9f0388f1844a2a797587254e249919b35ac9dc2b52c1aada29.idx
    fatal: Cannot open existing pack idx file for '/tmp/git-tests/trash directory.t5300-pack-object/repo/.git/objects/pack/pack-aa45f7f08f043c9f0388f1844a2a797587254e249919b35ac9dc2b52c1aada29.idx'

The error message isn't even properly indicating what the actual issue
is.

One potential solution would be to try and derive the object format from
the hash that the packfile index name has. But that is quite roundabout
and rather ugly, and packfiles may not necessarily have that hash in the
first place. It would also become potentially ambiguous in the future if
we were to ever adopt another hash that has the same length as either
SHA1 or SHA256.

So we basically have three different options:

  - Accept that we just don't handle this case correctly and let the
    code error out. This pessimizes all hashes but SHA256.

  - Bail out when outside of a repository when `--object-format=` wasn't
    given. This pessimizes all hashes, but gives a clear indicator to
    the user why things don't work.

  - Introduce packfiles v3 and encode the object format into the header.
    Then do either (1) or (2) on top.

The last option is of course the cleanest, but also the most involved.

Patrick

> ------- >8 -------
> Subject: verify-pack: fall back to SHA-1 outside a repo
> 
> In c8aed5e8da (repository: stop setting SHA1 as the default object hash,
> 2024-05-07), we have stopped setting the default hash algorithm for
> `the_repository`. Consequently, code that relies on `the_hash_algo` will
> now crash when it hasn't explicitly been initialized, which may be the
> case when running outside of a Git repository.
> 
> As the verify-pack command ought to be able to infer what algorithm
> is used in the input file (and if the input file does not have such
> an information, that by itself is a problem), and the command allows
> an option to explicitly tell what algorithm to use in case it cannot
> be guessed from the input file, in theory we shouldn't have to use
> the default algorithm anywhere in the operation of the command, but
> we fail fairly early in the process when run outside a repository
> without any default algorithm set.
> 
> Resurrect the setting of the default algorithm just like we used to
> do before 2.46.0
> 
> Reported-by: Ilya K <me@0upti.me>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  builtin/verify-pack.c  | 4 ++++
>  t/t5300-pack-object.sh | 4 ++++
>  2 files changed, 8 insertions(+)
> 
> diff --git c/builtin/verify-pack.c w/builtin/verify-pack.c
> index 011dddd2dc..5b663905ae 100644
> --- c/builtin/verify-pack.c
> +++ w/builtin/verify-pack.c
> @@ -1,6 +1,7 @@
>  #include "builtin.h"
>  #include "config.h"
>  #include "gettext.h"
> +#include "hash.h"
>  #include "run-command.h"
>  #include "parse-options.h"
>  #include "strbuf.h"
> @@ -77,6 +78,9 @@ int cmd_verify_pack(int argc, const char **argv, const char *prefix)
>  		OPT_END()
>  	};
>  
> +	if (!the_hash_algo)
> +		repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
> +
>  	git_config(git_default_config, NULL);
>  	argc = parse_options(argc, argv, prefix, verify_pack_options,
>  			     verify_pack_usage, 0);
> diff --git c/t/t5300-pack-object.sh w/t/t5300-pack-object.sh
> index 4ad023c846..d6f45d8923 100755
> --- c/t/t5300-pack-object.sh
> +++ w/t/t5300-pack-object.sh
> @@ -322,6 +322,10 @@ test_expect_success 'verify-pack catches a corrupted sum of the index file itsel
>  	fi
>  '
>  
> +test_expect_success 'verify-pack outside a repository' '
> +	nongit git verify-pack -v "$(pwd)/test-1-${packname_1}.idx"
> +'
> +
>  test_expect_success 'build pack index for an existing pack' '
>  	cat test-1-${packname_1}.pack >test-3.pack &&
>  	git index-pack -o tmp.idx test-3.pack &&

Re: git 2.46.0 crashes when trying to verify-pack outside of a repo

[brian m. carlson]

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

On 2024-09-01 at 23:45:43, Patrick Steinhardt wrote:
> Unfortunately, this once again uncovers a deeper issue: neither the
> packfile nor their index encode the object format they use. So while
> falling back to SHA1 papers over the issue, it means that we misparse
> SHA256 indices. Also, we misparse SHA1 indices if we happen to be in a
> SHA256 repository. E.g. when parsing a SHA256 file in a SHA1 repo:
> 
>     $ git index-pack --verify '/tmp/git-tests/trash directory.t5300-pack-object/repo/.git/objects/pack/pack-aa45f7f08f043c9f0388f1844a2a797587254e249919b35ac9dc2b52c1aada29.pack'
>     error: wrong index v2 file size in /tmp/git-tests/trash directory.t5300-pack-object/repo/.git/objects/pack/pack-aa45f7f08f043c9f0388f1844a2a797587254e249919b35ac9dc2b52c1aada29.idx
>     fatal: Cannot open existing pack idx file for '/tmp/git-tests/trash directory.t5300-pack-object/repo/.git/objects/pack/pack-aa45f7f08f043c9f0388f1844a2a797587254e249919b35ac9dc2b52c1aada29.idx'
> 
> The error message isn't even properly indicating what the actual issue
> is.

Yes, this is also true of other formats like the index as well, but
there we know it must be of the same format as the rest of the
repository.

I noticed this during writing the SHA-256 series, and it's inconvenient.
If you blame some of the tests that add the `--object-format` entry,
I wrote them.

> One potential solution would be to try and derive the object format from
> the hash that the packfile index name has. But that is quite roundabout
> and rather ugly, and packfiles may not necessarily have that hash in the
> first place. It would also become potentially ambiguous in the future if
> we were to ever adopt another hash that has the same length as either
> SHA1 or SHA256.

Yes, we've decided not to derive things by their length except in the
dumb HTTP protocol for this reason.

> So we basically have three different options:
> 
>   - Accept that we just don't handle this case correctly and let the
>     code error out. This pessimizes all hashes but SHA256.
> 
>   - Bail out when outside of a repository when `--object-format=` wasn't
>     given. This pessimizes all hashes, but gives a clear indicator to
>     the user why things don't work.

This is what I would recommend.

>   - Introduce packfiles v3 and encode the object format into the header.
>     Then do either (1) or (2) on top.

I think we have pack v3 already (which is the same as v2), and v4 was
for an experimental format that never landed fully.  Maybe v5?

If you wanted to do this, you could add support for arbitrary chunks,
like with multi-pack indexes, that would allow for extensibility in the
future.  However, you'd also need some protocol capabilities if you
want to send pack v5 or certain chunks over the protocol.

> The last option is of course the cleanest, but also the most involved.

I'd personally recommend just requiring the `--object-format=` option,
but of course if you want to write pack v5, don't let me stop you.
-- 
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA

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

Re: git 2.46.0 crashes when trying to verify-pack outside of a repo

[Patrick Steinhardt]

On Mon, Sep 02, 2024 at 01:18:38PM +0000, brian m. carlson wrote:
> On 2024-09-01 at 23:45:43, Patrick Steinhardt wrote:
> > So we basically have three different options:
> > 
> >   - Accept that we just don't handle this case correctly and let the
> >     code error out. This pessimizes all hashes but SHA256.
> > 
> >   - Bail out when outside of a repository when `--object-format=` wasn't
> >     given. This pessimizes all hashes, but gives a clear indicator to
> >     the user why things don't work.
> 
> This is what I would recommend.

I'm also leaning into this direction. I want us to move to a world where
SHA1 and SHA256 are equal citizens. We won't ever get folks to move on
to SHA256 repositories when it continues to be an afterthought that
behaves worse than SHA1 repositories.

That's basically where this whole exercise to stop setting the default
hash came from. GitLab nowadays has support for SHA256 repositories, so
I want to ensure that it can be used as a drop-in replacement for SHA1
in basically all ways.

> >   - Introduce packfiles v3 and encode the object format into the header.
> >     Then do either (1) or (2) on top.
> 
> I think we have pack v3 already (which is the same as v2), and v4 was
> for an experimental format that never landed fully.  Maybe v5?

Ah, fair enough.

> If you wanted to do this, you could add support for arbitrary chunks,
> like with multi-pack indexes, that would allow for extensibility in the
> future.  However, you'd also need some protocol capabilities if you
> want to send pack v5 or certain chunks over the protocol.
> 
> > The last option is of course the cleanest, but also the most involved.
> 
> I'd personally recommend just requiring the `--object-format=` option,
> but of course if you want to write pack v5, don't let me stop you.

Well, in the context of this issue I'd definitely aim for the easier fix
first. Regardless of whether or not we introduce v5, we'd still have to
address the underlying issue for repositories that do not (yet) have v5
packfiles.

Patrick

Re: git 2.46.0 crashes when trying to verify-pack outside of a repo

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

>> I'd personally recommend just requiring the `--object-format=` option,
>> but of course if you want to write pack v5, don't let me stop you.
>
> Well, in the context of this issue I'd definitely aim for the easier fix
> first. Regardless of whether or not we introduce v5, we'd still have to
> address the underlying issue for repositories that do not (yet) have v5
> packfiles.

I would be hesitant to reroactively tightening the rules, though.
If a tool has worked well for those who can and wants to assume a
hash function (because it has been the default, or perhaps because
the user configured the tool as such) even outside a repository,
such a tightening is a regression.  If such a change is done to
avoid triggering a data corrupting bug, we may be able to come up
with a valid justification, but on the other hand, to discourage
certain uses of the tool, even if the discouraged use is an insecure
one, feels a bit too opinionated for a tool.  It has the same smell
as updating the "md5sum" tool to discourage the use of the function
so that it always exits with 1 (after computing and showing the
hash) or something silly like that.

I am not saying that it is bad for a tool to be opinionated.  When
we design a new feature, it is part of the design process to make
sure that use of the feature encourages use of better workflows.

But disabling what has worked to user's expectation so far is a bit
different story.

[PATCH] builtin/index-pack: fix segfaults when running outside of a repo

[Patrick Steinhardt]

It was reported that git-verify-pack(1) has started to crash with Git
v2.46.0 when run outside of a repository. This is another fallout from
c8aed5e8da (repository: stop setting SHA1 as the default object hash,
2024-05-07), where we have stopped setting the default hash algorithm
for `the_repository`. Consequently, code that relies on `the_hash_algo`
will now crash when it hasn't explicitly been initialized, which may be
the case when running outside of a Git repository.

The crash is not in git-verify-pack(1) but instead in git-index-pack(1),
which gets called by the former. Ideally, both of these programs should
be able to identify the hash algorithm used by the packfile and index
without having to rely on external information. But unfortunately, the
format for neither of them is completely self-describing, so it is not
possible to derive that information. This is a design issue that we
should address by introducing a new packfile version that encodes its
object hash.

For now though the more important fix is to not make either of these
programs crash anymore, which we do by falling back to SHA1 when the
object hash is unconfigured. This pessimizes reading packfiles which
use a different hash than SHA1, but restores previous behaviour.

Reported-by: Ilya K <me@0upti.me>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/index-pack.c   |  9 +++++++++
 t/t5300-pack-object.sh | 39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index fd968d673d2..763b01372aa 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1868,6 +1868,15 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
 	if (!index_name && pack_name)
 		index_name = derive_filename(pack_name, "pack", "idx", &index_name_buf);
 
+	/*
+	 * Packfiles and indices do not carry enough information to be able to
+	 * identify their object hash. So when we are neither in a repository
+	 * nor has the user told us which object hash to use we have no other
+	 * choice but to guess the object hash.
+	 */
+	if (!the_repository->hash_algo)
+		repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
+
 	opts.flags &= ~(WRITE_REV | WRITE_REV_VERIFY);
 	if (rev_index) {
 		opts.flags |= verify ? WRITE_REV_VERIFY : WRITE_REV;
diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
index 4ad023c846d..3b9dae331a5 100755
--- a/t/t5300-pack-object.sh
+++ b/t/t5300-pack-object.sh
@@ -635,4 +635,43 @@ test_expect_success 'negative window clamps to 0' '
 	check_deltas stderr = 0
 '
 
+for hash in sha1 sha256
+do
+	test_expect_success "verify-pack with $hash packfile" '
+		test_when_finished "rm -rf repo" &&
+		git init --object-format=$hash repo &&
+		test_commit -C repo initial &&
+		git -C repo repack -ad &&
+		git -C repo verify-pack "$(pwd)"/repo/.git/objects/pack/*.idx &&
+		if test $hash = sha1
+		then
+			nongit git verify-pack "$(pwd)"/repo/.git/objects/pack/*.idx
+		else
+			# We have no way to identify the hash used by packfiles
+			# or indices, so we always fall back to SHA1.
+			nongit test_must_fail git verify-pack "$(pwd)"/repo/.git/objects/pack/*.idx &&
+			# But with an explicit object format we should succeed.
+			nongit git verify-pack --object-format=$hash "$(pwd)"/repo/.git/objects/pack/*.idx
+		fi
+	'
+
+	test_expect_success "index-pack outside of a $hash repository" '
+		test_when_finished "rm -rf repo" &&
+		git init --object-format=$hash repo &&
+		test_commit -C repo initial &&
+		git -C repo repack -ad &&
+		git -C repo index-pack --verify "$(pwd)"/repo/.git/objects/pack/*.pack &&
+		if test $hash = sha1
+		then
+			nongit git index-pack --verify "$(pwd)"/repo/.git/objects/pack/*.pack
+		else
+			# We have no way to identify the hash used by packfiles
+			# or indices, so we always fall back to SHA1.
+			nongit test_must_fail git index-pack --verify "$(pwd)"/repo/.git/objects/pack/*.pack 2>err &&
+			# But with an explicit object format we should succeed.
+			nongit git index-pack --object-format=$hash --verify "$(pwd)"/repo/.git/objects/pack/*.pack
+		fi
+	'
+done
+
 test_done
-- 
2.46.0.421.g159f2d50e7.dirty