Re: Bug: segfault during git-prune

[Andy Parkins <andyparkins@gmail.com>]

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.

$ git-prune
error: Object 228f8065b930120e35fc0c154c237487ab02d64a is a blob, not a commit
Segmentation fault (core dumped)

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.

However; I tried fixing this by swapping the order of the tests and the 
problem hasn't gone away.  I'm not sure that it's even getting as far as 
process_tree().  (incidentally I think the same fault exists in 
list-objects.c's process_tree).

Given the hints you gave me in your previous reply, I've looked at the 
backtrace again and understood more what's happening.

 - 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

I think the fault is in add_cache_refs() which assumes that every hash in the 
index is an OBJ_BLOB.  I think that add_cache_refs() shouldn't be calling 
lookup_blob() for S_ISGITLINK() index entries.  Therefore I think this patch 
is the right one; what do you reckon?

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