Re: Bug: segfault during git-prune

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Jul 2, 2007, 11:45 UTC
Message-ID: <alpine.LFD.0.98.0707020441460.3323@woody.linux-foundation.org>
In-Reply-To: <200707021100.16610.andyparkins@gmail.com>

On Mon, 2 Jul 2007, Andy Parkins wrote:

Show 6 quoted lines

> 
> 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; }

No, it's going to be something like "(mode & S_IFMASK) == S_IFDIR", so that part should be ok.

BUT:

Show 6 quoted lines

> 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.

Yes, this sounds like the real issue (or the remaining issue, at least).

> 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?

Looks good to me.

Show 9 quoted lines

> +       /*
> +        * 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;

Just for clarity, I'd use "process_gitlink()" here (and it would probably be a good idea to use process_blob() on the blob we look up too), and the proper tab indentation, but yes, I think you're otherwise exactly right.

		Linus