Bug: segfault during git-prune

Bug: segfault during git-prune

[Andy Parkins]

Hello,

I ran git-prune on a repository and got this:

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

git-show of the object shows it looks okay.  git-fsck just shows a load
of dangling objects - which isn't a surprise, that's why I was running
git-prune.

Here's the backtrace:

parse_commit_buffer (item=0x0, buffer=0x81124b8, size=421) at commit.c:292
#1  0x0808ad22 in parse_object_buffer (sha1=0x8111ac5 "\"\217\200e¹0\022\0165ü\f\025L#t\207«\002ÖJ", type=OBJ_COMMIT, size=421, buffer=0x81124b8,
    eaten_p=0xafdc6c40) at object.c:152
#2  0x0808adb7 in parse_object (sha1=0x8111ac5 "\"\217\200e¹0\022\0165ü\f\025L#t\207«\002ÖJ") at object.c:187
#3  0x0808d1a2 in add_one_ref (path=0x8111aed "refs/remotes/libswscale-svn", sha1=0x8111ac5 "\"\217\200e¹0\022\0165ü\f\025L#t\207«\002ÖJ", flag=6,
    cb_data=0xafdc6d40) at reachable.c:124
#4  0x0809133e in do_one_ref (base=<value optimized out>, fn=0x808d190 <add_one_ref>, trim=0, cb_data=0xafdc6d40, entry=0x8111ac0) at refs.c:478
#5  0x08092be4 in do_for_each_ref (base=0x80c8e76 "refs/", fn=0x808d190 <add_one_ref>, trim=0, cb_data=0xafdc6d40) at refs.c:545
#6  0x0808cff4 in mark_reachable_objects (revs=0xafdc6d40, mark_reflog=1) at reachable.c:188
#7  0x08071c90 in cmd_prune (argc=1, argv=0xafdc7074, prefix=0x0) at builtin-prune.c:93
#8  0x0804aa24 in handle_internal_command (argc=1, argv=0xafdc7074, envp=<value optimized out>) at git.c:348
#9  0x0804b4c9 in main (argc=1, argv=Cannot access memory at address 0xe

So, I think there are two faults: git-prune is deciding that the object is
a blob, when it's actually a commit; and git-prune's error handling is broken
in that case, because it's continuing with the NULL pointer returned by
check_commit() when obj->type != OBJ_COMMIT.



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

Re: Bug: segfault during git-prune

[Andy Parkins]

On Thursday 2007 June 28, Andy Parkins wrote:

> So, I think there are two faults: git-prune is deciding that the object is
> a blob, when it's actually a commit; and git-prune's error handling is
> broken in that case, because it's continuing with the NULL pointer returned
> by check_commit() when obj->type != OBJ_COMMIT.

I can't figure it out I'm afraid.  I've done a bit of back tracing and found
the block that's generating the error; object.c:150

  } else if (type == OBJ_COMMIT) {
    struct commit *commit = lookup_commit(sha1);
    parse_commit_buffer(commit, buffer, size);
    if (!commit->buffer) {
      commit->buffer = buffer;
      eaten = 1;
    }
    obj = &commit->object;
  }
	
Now, lookup_commit() is returning NULL because check_commit() is being called
with a struct object whose type field is not OBJ_COMMIT.  Which it presumably
got from lookup_object().  gdb tells me the object returned by lookup_object() 
is

 {parsed = 0, used = 0, type = 3, flags = 0,
  sha1 = "\"\217\200e¹0\022\0165ü\f\025L#t\207«\002ÖJ"}

Which seems to be the right hash, 228f8065b930120e35fc0c154c237487ab02d64a.

What should I do next?  I don't understand why lookup_object() is returning
an object with type blob instead of with type commit, when git-cat-file says 
that same object is a commit.



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

Re: Bug: segfault during git-prune

[Linus Torvalds]

On Thu, 28 Jun 2007, Andy Parkins wrote:
> 
> I ran git-prune on a repository and got this:
> 
> $ git-prune
> error: Object 228f8065b930120e35fc0c154c237487ab02d64a is a blob, not a commit
> Segmentation fault (core dumped)

Do you have subprojects in that git repo?

What happens is that all git object handling *refuses* (correctly) to 
touch an object of the wrong type. If it's been marked as a commit at some 
point, it had better be looked up as a commit the *next* time we see that 
same object too.

And every normal git object traversal will specify in advance what kind of 
object it expects (except for the initial refs lookups, that don't know, 
and use a totally different interface for "give me any kind of object, and 
figure it out").

So the error in this case is that we decided it was a commit once, and a 
blob once, and git is very unhappy.

And the only case I know of that does that is using an old git binary, or 
a unconverted git code-path program, on a repository with subprojects when 
the code-path doesn't understand that a tree can contain pointers to 
commits.

So what happens is that something traverses a tree object, looks at each 
entry, sees that it's not a tree, and tries to look it up as a blob. But 
subprojects are commits, not blobs, and then when you look at the object 
more closely, you get the above kind of object type confusion.

And I did that very early on, and wanted to make sure that git objects 
were "strongly typed", and you get a big fat error if you try to use the 
wrong type.

> $ git-cat-file -t 228f8065b930120e35fc0c154c237487ab02d64a
> commit
> 
> git-show of the object shows it looks okay.  git-fsck just shows a load
> of dangling objects - which isn't a surprise, that's why I was running
> git-prune.

Yeah, git-fsck knows about subprojects. I bet git-prune just doesn't. 

And indeed.. Here's a patch for it, but the fact is, you really should 
*not* prune that repository, because you'll prune away all the subproject 
data, which you seem to have in the same repo!

(General rule: never *ever* prune a shared object repository!)

		Linus

---
 reachable.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/reachable.c b/reachable.c
index ff3dd34..17ff929 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,
@@ -47,6 +56,8 @@ static void process_tree(struct tree *tree,
 	while (tree_entry(&desc, &entry)) {
 		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);
 	}

Re: Bug: segfault during git-prune

[Linus Torvalds]

On Thu, 28 Jun 2007, Linus Torvalds wrote:
> 
> Yeah, git-fsck knows about subprojects. I bet git-prune just doesn't. 
> 
> And indeed.. Here's a patch for it, but the fact is, you really should 
> *not* prune that repository, because you'll prune away all the subproject 
> data, which you seem to have in the same repo!
> 
> (General rule: never *ever* prune a shared object repository!)

Btw, we could make the "reachable" code say that *if* you have the commit 
of a subproject in your current object directory, it will follow that 
commit recursively, and thus sharing an object store at the superproject 
level is "ok" as far as "git prune" is concerned.

We don't currently even have that kind of function, though. Calling 
"parse_commit()" will complain very loudly if the commit doesn't exist, 
but it's actually perfectly fine to not exist if it's in a separate 
subproject.

So if you actually want to use subprojects *and* prune them, *and* you 
want to keep all the subproject objects in the same big object database, 
you'd have to add some commit object "parse-if-you-can-find-it" thing, and 
use that.

It shouldn't be fundamentally hard, but it's _not_ what my patch did. My 
patch just says: "subprojects are separate projects, and we don't do 
reachability from superproject to subproject".

		Linus

Re: Bug: segfault during git-prune

[Andy Parkins]

On Thursday 2007, June 28, Linus Torvalds wrote:
> On Thu, 28 Jun 2007, Andy Parkins wrote:
> > I ran git-prune on a repository and got this:
> >
> > $ git-prune
> > error: Object 228f8065b930120e35fc0c154c237487ab02d64a is a blob, not a
> > commit Segmentation fault (core dumped)
>
> Do you have subprojects in that git repo?

Yes.  I'm also doing something that is possibly very naughty; and I'm sure 
you're going to say "what on earth do you expect when you've done _that_"

The subproject is the same repository...  It's a git conversion of the 
ffmpeg history; ffmpeg uses svn:externals for the libswscale directory; so 
I set that as an independent branch in the same repository, fetched with 
git-svn as well.  Then I cloned the repository into a subdirectory of 
itself.

 $ git clone -n . libswscale

Then I went into libswscale/.git/ and ln -s objects, refs, info, and logs.  
In fact the only thing that isn't shared is HEAD.

Then I changed into libswscale and checked out the libswscale branch.  Back 
in the ffmpeg repository I git-add the libswscale directory and everything 
seems to be working wonderfully.

> And the only case I know of that does that is using an old git binary, or
> a unconverted git code-path program, on a repository with subprojects
> when the code-path doesn't understand that a tree can contain pointers to
> commits.

Sounds like the last one to me.  I tend to be only a few days behind 
upstream git.

> Yeah, git-fsck knows about subprojects. I bet git-prune just doesn't.
>
> And indeed.. Here's a patch for it, but the fact is, you really should
> *not* prune that repository, because you'll prune away all the subproject
> data, which you seem to have in the same repo!

Correct; you did well figuring that out from the meagre information I gave.

I had hoped that git-prune wouldn't be a risk because I have:

 * -- * -- * -- * -- * (ffmpeg-svn)

      * -- * -- * -- * (libswscale-svn)

Then I forked master from ffmpeg-svn and added the libswscale-svn branch as 
a submodule as described above.  Now, because the submodule always refers 
to commits that are ancestors of libswscale-svn, they'll never be seen as 
dangling and pruned?

> (General rule: never *ever* prune a shared object repository!)

Even when I'm sure that every object of interest is behind a head ref?

> ---
>  reachable.c |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)

The repository in question is on my work computer, so I won't be able to try 
this patch until Monday.  I'll report back then.


Andy

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

Re: Bug: segfault during git-prune

[Linus Torvalds]

On Thu, 28 Jun 2007, Andy Parkins wrote:
> 
> I had hoped that git-prune wouldn't be a risk because I have:
> 
>  * -- * -- * -- * -- * (ffmpeg-svn)
> 
>       * -- * -- * -- * (libswscale-svn)

Ok. If all subproject branches are also visible in the superproject as 
refs, then "git prune" should work fine, and you can apply my patch and it 
should just work very naturally: the reachability analysis will find the 
subprojects not through the superproject link, but simply through the 
direct refs to the subproject.

> > (General rule: never *ever* prune a shared object repository!)
> 
> Even when I'm sure that every object of interest is behind a head ref?

So yes, in that case, you're ok.

This is not unlike just having two different repositories sharing the same 
object directory: as long as the two different repositories both have the 
appropriate refs, pruning is fine. In other words, you can see them as 
just independent branches in the same repo.

And in fact, subprojects are obviously very much *designed* to work that 
way: a subproject is basically a "different repository". So the basic rule 
is that if it would work with totally independent repos, it works with 
subprojects.

[ That's all aside from the kind of bug that you found, where some code 
  that parses the tree structures hadn't been updated for subprojects, of 
  course ]

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

		Linus

Re: Bug: segfault during git-prune

[Andy Parkins]

On Thursday 2007, June 28, Linus Torvalds wrote:
> On Thu, 28 Jun 2007, Andy Parkins wrote:
> > I had hoped that git-prune wouldn't be a risk because I have:
> >
> >  * -- * -- * -- * -- * (ffmpeg-svn)
> >
> >       * -- * -- * -- * (libswscale-svn)
>
> Ok. If all subproject branches are also visible in the superproject as
> refs, then "git prune" should work fine, and you can apply my patch and
> it should just work very naturally: the reachability analysis will find
> the subprojects not through the superproject link, but simply through the
> direct refs to the subproject.

Excellent.  That's what I'd hoped.

I'm actually really impressed that git is robust enough that my symbolic 
linking of all the .git subdirectories works so well.  It's actually turned 
out to be a really natural way of using a repository for strongly connected 
submodules.

> This is not unlike just having two different repositories sharing the
> same object directory: as long as the two different repositories both
> have the appropriate refs, pruning is fine. In other words, you can see
> them as just independent branches in the same repo.

Absolutely.  With my poor-mans submodule script that I was using before git 
had it's own submodule support, I had the script make a refs/superrefs 
directory, and every time I committed to the supermodule the script would 
write $subhash to $submodule/.git/refs/heads/superrefs/$subhash.  The it 
was safe to git-prune the submodules as well.

> And in fact, subprojects are obviously very much *designed* to work that
> way: a subproject is basically a "different repository". So the basic
> rule is that if it would work with totally independent repos, it works
> with subprojects.

I certainly agree, it's an extremely elegant way of working.  I can't 
imagine that any other VCS is capable of this sort of manipulation.

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

Will do.  I'll certainly test it, and am happy to forward it on if that test 
is successful.


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

Re: Bug: segfault during git-prune

[Andy Parkins]

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

Re: Bug: segfault during git-prune

[Linus Torvalds]

On Mon, 2 Jul 2007, Andy Parkins wrote:
> 
> 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:

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

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

Re: Bug: segfault during git-prune

[Andy Parkins]

On Monday 2007 July 02, Linus Torvalds wrote:

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

I've pushed it out as-is as a bug fix patch now; and will happilly make the 
above change on top.  However, I've not understood what all the parameters 
are for in the process_XXXX() functions:

static void process_gitlink(const unsigned char *sha1,
			    struct object_array *p,
			    struct name_path *path,
			    const char *name)

the sha1 and name I can read out of the cache_entry structure, but I'm not 
sure about the object_array and name_path structures in the context of 
add_cache_refs().



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

Re: Bug: segfault during git-prune

[Linus Torvalds]

On Mon, 2 Jul 2007, Andy Parkins wrote:
> 
> I've pushed it out as-is as a bug fix patch now; and will happilly make the 
> above change on top.  However, I've not understood what all the parameters 
> are for in the process_XXXX() functions:

Yeah, never mind, your patch is fine. The process_xxx() parameters are 
really only there so that process_tree() can generate the proper path and 
names, and they are passed to process_blob/gitlink() just to make all the 
processing look the same.

But it's nonsensical to do for the case of the index (although you could 
pass a NULL path and the name from the index), and we haven't created the 
object array there, so just ignore my suggestion - your original patch is 
fine.

		Linus