Re: [PATCH 2/4] make verify_one_pack() a bit less wrong wrt packed_git structure

From: Nicolas Pitre <nico@cam.org>
Date: Mar 1, 2008, 08:18 UTC
Message-ID: <alpine.LFD.1.00.0803010305230.2899@xanadu.home>
In-Reply-To: <7vk5kn7znz.fsf@gitster.siamese.dyndns.org>

On Fri, 29 Feb 2008, Junio C Hamano wrote:

Show 7 quoted lines

> Nicolas Pitre <nico@cam.org> writes:
> 
> > ..., but it is still
> > a bit more "correct" to leak it implicitly rather than explicitly.
> 
> I do not follow this logic to debate which incorrectness is more
> correct, but I do not mind the removal of free() there.

Freeing the pack structure here without cleaning the resources it still holds is plain wrong. Not freeing it might be a bit wrong as well, but it is inline with the rest of Git which relies on program termination to clean everything.

Show 16 quoted lines

> I am not sure about the install_packed_git() piece, though.
> 
> This part of the code predates Shawn's windowed mmap and all
> other recent code improvements, but the original motivation of
> not installing the pack was to make sure that codepaths outside
> verify_packfile() would not see the objects from the pack being
> verified at all.  IOW, the omission originally was intentional.
> 
> I just quickly looked at verify_packfile() after applying your
> series, and it seems that nothing tries to access objects with
> only their SHA-1 names without explicitly telling which pack to
> read from, so it should still be safe even if we did not install
> the packed git (iow, the normal codepath would not try to pick
> up objects from the suspect pack that is being validated).
> 
> But it made me feel a bit worried.

The problem is with the patch that follows, which calls init_revindex(). Maybe the following patch (on top of the other ones) should bring back that isolation property for the actual verification pass, and install_packed_git() called only for the verbose output.

diff --git a/builtin-verify-pack.c b/builtin-verify-pack.c
index 4958bbb..cefa4d7 100644
--- a/builtin-verify-pack.c
+++ b/builtin-verify-pack.c
@@ -40,7 +40,6 @@ static int verify_one_pack(const char *path, int verbose)
 	if (!pack)
 		return error("packfile %s not found.", arg);
 
-	install_packed_git(pack);
 	err = verify_pack(pack, verbose);
 
 	return err;
diff --git a/pack-check.c b/pack-check.c
index 0f8ad2c..32176b2 100644
--- a/pack-check.c
+++ b/pack-check.c
@@ -105,6 +105,7 @@ static void show_pack_info(struct packed_git *p)
 
 	nr_objects = p->num_objects;
 	memset(chain_histogram, 0, sizeof(chain_histogram));
+	install_packed_git(p);
 	init_pack_revindex();
 
 	for (i = 0; i < nr_objects; i++) {