Re: [PATCH 0/5] add pack index v2 reading capability to git v1.4.4.4

Linus Torvalds [off-list ref] writes:

On Wed, 16 Jul 2008, Junio C Hamano wrote:

quoted

I do not think it should SEGV.  The pack-idx signature was chosen rather
carefully to allow older ones to die gracefully.

Well, Pasky reported differently.

quoted

    error: non-monotonic index
    error: Could not read 4a588075c54cd5902e5f4d43b9d6b0c31d0f9769

Pasky's report was

	error: non-monotonic index
	/usr/bin/git-fetch: line 297: 30402 Segmentation fault git-http-fetch -v -a "$head" "$remote/"

but maybe that was something specific to his case.

It is caused by the http walker not being careful.  In v1.4.4.5
http-fetch.c, this code appears unmodified since v1.4.4.4, and an
equivalent code is still in http-walker.c in more recent versions:

    static int setup_index(struct alt_base *repo, unsigned char *sha1)
    {
            struct packed_git *new_pack;
            if (has_pack_file(sha1))
                    return 0; /* don't list this as something we can get */

            if (fetch_index(repo, sha1))
                    return -1;

            new_pack = parse_pack_index(sha1);
            new_pack->next = repo->packs;
            repo->packs = new_pack;
            return 0;
    }

Nico taught parse_pack_index() what v2 pack idx file looks like, but when
the code hits unknown idx file (or a corrupt one), the function signals
error by returning NULL; assigning to new_pack->next without checking
would segfault.

We would need this fix to futureproof ourselves for pack idx v3 and later,
and also for protecting from a corrupt idx file coming over the wire.

---

 http-walker.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/http-walker.c b/http-walker.c
index 51c18f2..9dc6b27 100644
--- a/http-walker.c
+++ b/http-walker.c

@@ -442,6 +442,8 @@ static int setup_index(struct walker *walker, struct alt_base *repo, unsigned ch
 		return -1;
 
 	new_pack = parse_pack_index(sha1);
+	if (!new_pack)
+		return -1; /* parse_pack_index() already issued an error message */
 	new_pack->next = repo->packs;
 	repo->packs = new_pack;
 	return 0;