Re: Bug: segfault during git-prune

From

APAndy Parkins <andyparkins@gmail.com>

Date

Jul 2, 2007, 10:00 UTC

Message-ID

<200707021100.16610.andyparkins@gmail.com>

In-Reply-To

<alpine.LFD.0.98.0706281525460.8675@woody.linux-foundation.org>

On Thursday 2007 June 28, Linus Torvalds wrote:

> Anyway, if that patch works for you, I'd suggest you just pass it on to
> Junio (and feel free to add my "Signed-off-by:" on it - but conditional on
> you having actually tested it).

Okay; tested with this patch, but no change in behaviour.

Looking at your patch: is it possible that S_ISDIR() is true for gitlinks as 
well as S_ISGITLINK()?  S_ISDIR() is from unistd.h; and is presumably 
something like:
 
 S_ISDIR() { return mode & S_IFDIR; }

Given the GITLINK mode is S_IFLINK | S_IFDIR; then S_ISDIR() will be true and

        if (S_ISDIR(entry.mode))
            process_tree(lookup_tree(entry.sha1), p, &me, entry.path);
+       else if (S_ISGITLINK(entry.mode))
+           process_gitlink(entry.sha1, p, &me, entry.path);
        else
            process_blob(lookup_blob(entry.sha1), p, &me, entry.path);

will never get to the process_gitlink() call.

 - mark_reachable_objects() calls add_cache_refs()
 - which uses lookup_blob() to mark every hash in the index as an OBJ_BLOB 
   type of hash; including the GITLINK entries.
 - mark_reachable_objects() calls add_one_ref() for_each_ref(), which finds
   a ref pointing to one of the GITLINK entries, and via 
   parse_object_buffer(), tries to lookup_commit(), which finds the GITLINKed
   object using lookup_object() only it is not an OBJ_COMMIT, it's an OBJ_BLOB
 - all hell breaks loose

diff --git a/reachable.c b/reachable.c
index ff3dd34..ffc8d0a 100644
--- a/reachable.c
+++ b/reachable.c
@@ -21,6 +21,15 @@ static void process_blob(struct blob *blob,
    /* Nothing to do, really .. The blob lookup was the important part */
 }
 
+static void process_gitlink(const unsigned char *sha1,
+               struct object_array *p,
+               struct name_path *path,
+               const char *name)
+{
+   /* I don't think we want to recurse into this, really. */
+}
+
+
 static void process_tree(struct tree *tree,
             struct object_array *p,
             struct name_path *path,
@@ -45,7 +54,9 @@ static void process_tree(struct tree *tree,
    init_tree_desc(&desc, tree->buffer, tree->size);
 
    while (tree_entry(&desc, &entry)) {
-       if (S_ISDIR(entry.mode))
+       if (S_ISGITLINK(entry.mode))
+           process_gitlink(entry.sha1, p, &me, entry.path);
+       else if (S_ISDIR(entry.mode))
            process_tree(lookup_tree(entry.sha1), p, &me, entry.path);
        else
            process_blob(lookup_blob(entry.sha1), p, &me, entry.path);
@@ -159,6 +170,16 @@ static void add_cache_refs(struct rev_info *revs)
 
    read_cache();
    for (i = 0; i < active_nr; i++) {
+       /*
+        * The index can contain blobs and GITLINKs, GITLINKs are hashes
+        * that don't actually point to objects in the repository, it's
+        * almost guaranteed that they are NOT blobs, so we don't call
+        * lookup_blob() on them, to avoid populating the hash table
+        * with invalid information
+        */
+       if (S_ISGITLINK(ntohl(active_cache[i]->ce_mode)))
+           continue;
+
        lookup_blob(active_cache[i]->sha1);
        /*
         * We could add the blobs to the pending list, but quite

If you think I'm on the right lines with this, I'll make better patches for 
Junio.


Andy

-- 
Dr Andy Parkins, M Eng (hons), MIET
andyparkins@gmail.com