bug report: "git pack-redundant --all" crash in minimize()

Hello,

I'm getting the following crash from "git pack-redundant --all" (output
from valgrind):

==14070== Invalid read of size 8
==14070==    at 0x18F165: minimize (pack-redundant.c:399)
==14070==    by 0x18F165: cmd_pack_redundant (pack-redundant.c:622)
==14070==    by 0x1242D3: run_builtin (git.c:444)
==14070==    by 0x1242D3: handle_builtin (git.c:674)
==14070==    by 0x125393: run_argv (git.c:741)
==14070==    by 0x125393: cmd_main (git.c:872)
==14070==    by 0x123E7D: main (common-main.c:52)
==14070==  Address 0x10 is not stack'd, malloc'd or (recently) free'd

Commands to reproduce:

mkdir new
cd new
git init
touch foo.txt
git add foo.txt
git commit -m "first commit"
git gc
git pack-redundant --all

The same happens with Ubuntu's git 2.25.1 as well as manually compiled
git 2.30.0.rc0 from current master.

The reason this showed up is that Yocto's build tool, bitbake, sometimes
invokes this git command on some of the repositories it downloaded. This
issue was apparently also reported here:
https://bugzilla.redhat.com/show_bug.cgi?id=1803506

Kind Regards,
Daniel

On Tue, Dec 15, 2020 at 06:34:53PM +0100, Daniel C. Klauer wrote:

I'm getting the following crash from "git pack-redundant --all" (output
from valgrind):

==14070== Invalid read of size 8
==14070==    at 0x18F165: minimize (pack-redundant.c:399)
==14070==    by 0x18F165: cmd_pack_redundant (pack-redundant.c:622)
==14070==    by 0x1242D3: run_builtin (git.c:444)
==14070==    by 0x1242D3: handle_builtin (git.c:674)
==14070==    by 0x125393: run_argv (git.c:741)
==14070==    by 0x125393: cmd_main (git.c:872)
==14070==    by 0x123E7D: main (common-main.c:52)
==14070==  Address 0x10 is not stack'd, malloc'd or (recently) free'd

Commands to reproduce:

mkdir new
cd new
git init
touch foo.txt
git add foo.txt
git commit -m "first commit"
git gc
git pack-redundant --all

Thanks for an easy reproduction. This bisects to 3011177640
(pack-redundant: delay creation of unique_objects, 2019-02-02).

I suspect the fix is just:

diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
index 3e70f2a4c1..68afcfeb7b 100644
--- a/builtin/pack-redundant.c
+++ b/builtin/pack-redundant.c

@@ -396,7 +396,7 @@ static void minimize(struct pack_list **min)
 
 	pl = local_packs;
 	while (pl) {
-		if (pl->unique_objects->size)
+		if (pl->unique_objects && pl->unique_objects->size)
 			pack_list_insert(&unique, pl);
 		else
 			pack_list_insert(&non_unique, pl);

but I didn't look closely (author cc'd).

The reason this showed up is that Yocto's build tool, bitbake, sometimes
invokes this git command on some of the repositories it downloaded. This
issue was apparently also reported here:
https://bugzilla.redhat.com/show_bug.cgi?id=1803506

We had a discussion not too long ago[1] about whether we should
deprecate and remove pack-redundant, as it's not generally useful. I'm
curious if you have any more context on why it's used in that tool.

-Peff

[1] https://lore.kernel.org/git/20200825172214.GC1414394@coredump.intra.peff.net/

Jeff King [off-list ref] 于2020年12月16日周三 上午2:24写道：

Will fix like this:

    pack-redundant: fix crash when one packfile in repo

    Command `git pack-redundant --all` will crash if there is only one
    packfile in the repository.  This is because, if there is only one
    packfile in local_packs, `cmp_local_packs` will do nothing and will
    leave `pl->unique_objects` as uninitialized.

    ... ...

diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
index 178e3409b7..9b0646a5e2 100644
--- a/builtin/pack-redundant.c
+++ b/builtin/pack-redundant.c

@@ -473,6 +473,12 @@ static void cmp_local_packs(void)
 {
        struct pack_list *subset, *pl = local_packs;

+       /* only one packfile */
+       if (!pl->next && !pl->unique_objects) {
+               llist_init(&pl->unique_objects);
+               return;
+       }
+
        while ((subset = pl)) {
                while ((subset = subset->next))
                        cmp_two_packs(pl, subset);

From: Jiang Xin <redacted>

Command `git pack-redundant --all` will crash if there is only one
packfile in the repository.  This is because, if there is only one
packfile in local_packs, `cmp_local_packs` will do nothing and will
leave `pl->unique_objects` as uninitialized.

Junio will add additional option for pack-redundant for preparing
its removal.  So replace `git pack-redundant` command with variable
`$git_pack_redundant` in t5323.

Reported-by: Daniel C. Klauer <redacted>
Signed-off-by: Jiang Xin <redacted>
---
 builtin/pack-redundant.c  |  6 ++++
 t/t5323-pack-redundant.sh | 65 +++++++++++++++++++++++++++++----------
 2 files changed, 54 insertions(+), 17 deletions(-)

diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
index 178e3409b7..690775fa82 100644
--- a/builtin/pack-redundant.c
+++ b/builtin/pack-redundant.c

@@ -473,6 +473,12 @@ static void cmp_local_packs(void)
 {
 	struct pack_list *subset, *pl = local_packs;
 
+	/* only one packfile */
+	if (!pl->next) {
+		llist_init(&pl->unique_objects);
+		return;
+	}
+
 	while ((subset = pl)) {
 		while ((subset = subset->next))
 			cmp_two_packs(pl, subset);

diff --git a/t/t5323-pack-redundant.sh b/t/t5323-pack-redundant.sh
index 6b4d1ca353..45436bd6e1 100755
--- a/t/t5323-pack-redundant.sh
+++ b/t/t5323-pack-redundant.sh

@@ -39,6 +39,8 @@ relationship between packs and objects is as follows:
 master_repo=master.git
 shared_repo=shared.git
 
+git_pack_redundant='git pack-redundant'
+
 # Create commits in <repo> and assign each commit's oid to shell variables
 # given in the arguments (A, B, and C). E.g.:
 #

@@ -112,19 +114,28 @@ test_expect_success 'setup master repo' '
 	create_commits_in "$master_repo" A B C D E F G H I J K L M N O P Q R
 '
 
+test_expect_success 'master: pack-redundant works with no packfile' '
+	(
+		cd "$master_repo" &&
+		cat >expect <<-EOF &&
+			fatal: Zero packs found!
+			EOF
+		test_must_fail $git_pack_redundant --all >actual 2>&1 &&
+		test_cmp expect actual
+	)
+'
+
 #############################################################################
 # Chart of packs and objects for this test case
 #
 #         | T A B C D E F G H I J K L M N O P Q R
 #     ----+--------------------------------------
 #     P1  | x x x x x x x                       x
-#     P2  |     x x x x   x x x
-#     P3  |             x     x x x x x
 #     ----+--------------------------------------
-#     ALL | x x x x x x x x x x x x x x         x
+#     ALL | x x x x x x x                       x
 #
 #############################################################################
-test_expect_success 'master: no redundant for pack 1, 2, 3' '
+test_expect_success 'master: pack-redundant works with one packfile' '
 	create_pack_in "$master_repo" P1 <<-EOF &&
 		$T
 		$A

@@ -135,6 +146,26 @@ test_expect_success 'master: no redundant for pack 1, 2, 3' '
 		$F
 		$R
 		EOF
+	(
+		cd "$master_repo" &&
+		$git_pack_redundant --all >out &&
+		test_must_be_empty out
+	)
+'
+
+#############################################################################
+# Chart of packs and objects for this test case
+#
+#         | T A B C D E F G H I J K L M N O P Q R
+#     ----+--------------------------------------
+#     P1  | x x x x x x x                       x
+#     P2  |     x x x x   x x x
+#     P3  |             x     x x x x x
+#     ----+--------------------------------------
+#     ALL | x x x x x x x x x x x x x x         x
+#
+#############################################################################
+test_expect_success 'master: no redundant for pack 1, 2, 3' '
 	create_pack_in "$master_repo" P2 <<-EOF &&
 		$B
 		$C

@@ -154,7 +185,7 @@ test_expect_success 'master: no redundant for pack 1, 2, 3' '
 		EOF
 	(
 		cd "$master_repo" &&
-		git pack-redundant --all >out &&
+		$git_pack_redundant --all >out &&
 		test_must_be_empty out
 	)
 '

@@ -192,7 +223,7 @@ test_expect_success 'master: one of pack-2/pack-3 is redundant' '
 		cat >expect <<-EOF &&
 			P3:$P3
 			EOF
-		git pack-redundant --all >out &&
+		$git_pack_redundant --all >out &&
 		format_packfiles <out >actual &&
 		test_cmp expect actual
 	)

@@ -231,7 +262,7 @@ test_expect_success 'master: pack 2, 4, and 6 are redundant' '
 			P4:$P4
 			P6:$P6
 			EOF
-		git pack-redundant --all >out &&
+		$git_pack_redundant --all >out &&
 		format_packfiles <out >actual &&
 		test_cmp expect actual
 	)

@@ -266,7 +297,7 @@ test_expect_success 'master: pack-8 (subset of pack-1) is also redundant' '
 			P6:$P6
 			P8:$P8
 			EOF
-		git pack-redundant --all >out &&
+		$git_pack_redundant --all >out &&
 		format_packfiles <out >actual &&
 		test_cmp expect actual
 	)

@@ -284,9 +315,9 @@ test_expect_success 'master: clean loose objects' '
 test_expect_success 'master: remove redundant packs and pass fsck' '
 	(
 		cd "$master_repo" &&
-		git pack-redundant --all | xargs rm &&
+		$git_pack_redundant --all | xargs rm &&
 		git fsck &&
-		git pack-redundant --all >out &&
+		$git_pack_redundant --all >out &&
 		test_must_be_empty out
 	)
 '

@@ -304,7 +335,7 @@ test_expect_success 'setup shared.git' '
 test_expect_success 'shared: all packs are redundant, but no output without --alt-odb' '
 	(
 		cd "$shared_repo" &&
-		git pack-redundant --all >out &&
+		$git_pack_redundant --all >out &&
 		test_must_be_empty out
 	)
 '

@@ -343,7 +374,7 @@ test_expect_success 'shared: show redundant packs in stderr for verbose mode' '
 			P5:$P5
 			P7:$P7
 			EOF
-		git pack-redundant --all --verbose >out 2>out.err &&
+		$git_pack_redundant --all --verbose >out 2>out.err &&
 		test_must_be_empty out &&
 		grep "pack$" out.err | format_packfiles >actual &&
 		test_cmp expect actual

@@ -356,9 +387,9 @@ test_expect_success 'shared: remove redundant packs, no packs left' '
 		cat >expect <<-EOF &&
 			fatal: Zero packs found!
 			EOF
-		git pack-redundant --all --alt-odb | xargs rm &&
+		$git_pack_redundant --all --alt-odb | xargs rm &&
 		git fsck &&
-		test_must_fail git pack-redundant --all --alt-odb >actual 2>&1 &&
+		test_must_fail $git_pack_redundant --all --alt-odb >actual 2>&1 &&
 		test_cmp expect actual
 	)
 '

@@ -386,7 +417,7 @@ test_expect_success 'shared: create new objects and packs' '
 test_expect_success 'shared: no redundant without --alt-odb' '
 	(
 		cd "$shared_repo" &&
-		git pack-redundant --all >out &&
+		$git_pack_redundant --all >out &&
 		test_must_be_empty out
 	)
 '

@@ -417,7 +448,7 @@ test_expect_success 'shared: no redundant without --alt-odb' '
 test_expect_success 'shared: one pack is redundant with --alt-odb' '
 	(
 		cd "$shared_repo" &&
-		git pack-redundant --all --alt-odb >out &&
+		$git_pack_redundant --all --alt-odb >out &&
 		format_packfiles <out >actual &&
 		test_line_count = 1 actual
 	)

@@ -454,7 +485,7 @@ test_expect_success 'shared: ignore unique objects and all two packs are redunda
 			Px1:$Px1
 			Px2:$Px2
 			EOF
-		git pack-redundant --all --alt-odb >out <<-EOF &&
+		$git_pack_redundant --all --alt-odb >out <<-EOF &&
 			$X
 			$Y
 			$Z

-- 
2.30.0.rc0

On 15.12.20 19:21, Jeff King wrote:

On Tue, Dec 15, 2020 at 06:34:53PM +0100, Daniel C. Klauer wrote:

quoted

The reason this showed up is that Yocto's build tool, bitbake, sometimes
invokes this git command on some of the repositories it downloaded. This
issue was apparently also reported here:
https://bugzilla.redhat.com/show_bug.cgi?id=1803506

We had a discussion not too long ago[1] about whether we should
deprecate and remove pack-redundant, as it's not generally useful. I'm
curious if you have any more context on why it's used in that tool.

Background: bitbake downloads git repositories during a build process
and supports caching them locally (in form of bare repos in some
user-defined directory). This prevents having to re-download them during
the next build, and also it is a convenient mirroring/backup system in
case the original URLs stop working.

As far as I can tell (since I'm not a bitbake developer) the git
pack-redundant invocation is one of multiple calls meant to improve
storage (probably minimize disk usage) of the locally cached git repos.
For reference, please take a look at the other git commands it's
invoking [1], and at the commit messages of the commits that added these
invocations [2] [3] [4].

If doing it that way seems wrong, I'll report the issue to bitbake
upstream too. Maybe there is a better way to do whatever bitbake wants
to do here?

Thanks!

[1]
http://git.openembedded.org/bitbake/tree/lib/bb/fetch2/git.py?h=1.48.1#n364
[2]
http://git.openembedded.org/bitbake/commit?id=d5d958744fff66cf5286022d78cbec1839fca2e2
[3]
http://git.openembedded.org/bitbake/commit?id=f8126aaf774186a6eaf0bd4067b89c074594886c
[4]
http://git.openembedded.org/bitbake/commit?id=7d55491f88ff90a4c16ad87b1483d55b1824acfc

On Wed, Dec 16, 2020 at 05:09:52AM -0500, Jiang Xin wrote:

Command `git pack-redundant --all` will crash if there is only one
packfile in the repository.  This is because, if there is only one
packfile in local_packs, `cmp_local_packs` will do nothing and will
leave `pl->unique_objects` as uninitialized.

Thanks, that makes sense. And I'm glad I waited to get the opinion of
somebody much more clueful. Mine was a band-aid, but yours is hitting
the root cause and will help any other downstream code that assumes
unique_objects isn't NULL.

 builtin/pack-redundant.c  |  6 ++++
 t/t5323-pack-redundant.sh | 65 +++++++++++++++++++++++++++++----------

The new test coverage makes sense, I think, though as Junio noted, it
should be done independent of the $git_pack_redundant change (which will
also make the diff a lot easier to review).

-Peff

On Wed, Dec 16, 2020 at 02:22:52PM +0100, Daniel Klauer wrote:

Background: bitbake downloads git repositories during a build process
and supports caching them locally (in form of bare repos in some
user-defined directory). This prevents having to re-download them during
the next build, and also it is a convenient mirroring/backup system in
case the original URLs stop working.

As far as I can tell (since I'm not a bitbake developer) the git
pack-redundant invocation is one of multiple calls meant to improve
storage (probably minimize disk usage) of the locally cached git repos.
For reference, please take a look at the other git commands it's
invoking [1], and at the commit messages of the commits that added these
invocations [2] [3] [4].

If doing it that way seems wrong, I'll report the issue to bitbake
upstream too. Maybe there is a better way to do whatever bitbake wants
to do here?

Thanks for that context.

I don't think it's _wrong_, in the sense that what they want to do
(remove redundant packs) is a reasonable thing to want. But in practice
I suspect that it rarely helps. It only makes sense if a pack is fully
made redundant by other packs. But that is unlikely to happen after a
fetch, because Git tries not to send objects that already exist. So
while there could be overlap, it's unlikely that full packs are
candidates for deletion. And if any are, then that is probably a sign
that fetch is not being given enough information (e.g., if there are
packs being copied into the repo behind the scenes, make sure that there
are matching refs pointing to their objects, so Git knows it has that
part of the object graph).

For saving space, "git repack -ad" is a much better option. It puts
everything reachable into a single pack, which means:

  - if two packs contain duplicates of an object, we'll end up with only
    a single copy, even if those packs also contained some unique
    objects

  - by putting all objects in the same pack, we have more opportunities
    for delta compression between similar objects

  - we'll drop any unreachable objects completely (presumably this is
    desirable here, but if they're trying to keep objects that don't
    have refs pointing at them as part of some caching scheme, they
    might not; passing "-k" will keep the unreachable objects, too)

Since they're doing other maintenance like "pack-refs", then running
"git gc" may be preferable, as it would cover that, too. Use
"--prune=now" to drop the unreachable objects immediately (as opposed to
giving them a 2-week grace period). Note that there's no equivalent to
repack's "-k" from git-gc", so if they need that, they'll have to invoke
git-repack directly.

-Peff