[PATCH] Fix segfaults caused by invalid tags

[PATCH] Fix segfaults caused by invalid tags

[Petr Baudis]

On many places, we didn't bother checking deref_tag() return value against
NULL, causing GIT segfaulting e.g. on some old Cogito checkouts containing
residual tags from GIT.

Signed-off-by: Petr Baudis <pasky@suse.cz>
---

 fetch-pack.c  |    2 +-
 server-info.c |    5 +++--
 upload-pack.c |    3 ++-
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index 3df9911..1fae211 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -40,7 +40,7 @@ static int rev_list_insert_ref(const cha
 {
 	struct object *o = deref_tag(parse_object(sha1));
 
-	if (o->type == commit_type)
+	if (o && o->type == commit_type)
 		rev_list_push((struct commit *)o, SEEN);
 
 	return 0;
diff --git a/server-info.c b/server-info.c
index ba53591..8e13c94 100644
--- a/server-info.c
+++ b/server-info.c
@@ -14,8 +14,9 @@ static int add_info_ref(const char *path
 	fprintf(info_ref_fp, "%s	%s\n", sha1_to_hex(sha1), path);
 	if (o->type == tag_type) {
 		o = deref_tag(o);
-		fprintf(info_ref_fp, "%s	%s^{}\n",
-			sha1_to_hex(o->sha1), path);
+		if (o)
+			fprintf(info_ref_fp, "%s	%s^{}\n",
+				sha1_to_hex(o->sha1), path);
 	}
 	return 0;
 }
diff --git a/upload-pack.c b/upload-pack.c
index c5eff21..f0b8e63 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -227,7 +227,8 @@ static int send_ref(const char *refname,
 	}
 	if (o->type == tag_type) {
 		o = deref_tag(o);
-		packet_write(1, "%s %s^{}\n", sha1_to_hex(o->sha1), refname);
+		if (o)
+			packet_write(1, "%s %s^{}\n", sha1_to_hex(o->sha1), refname);
 	}
 	return 0;
 }

Re: [PATCH] Fix segfaults caused by invalid tags

[Johannes Schindelin]

Hi,

On Wed, 2 Nov 2005, Petr Baudis wrote:

> On many places, we didn't bother checking deref_tag() return value against
> NULL, causing GIT segfaulting e.g. on some old Cogito checkouts containing
> residual tags from GIT.

Shouldn't a warning be issued in every of those cases? I don't know, but 
those tags pointing nowhere don't seem useful to me.

Ciao,
Dscho

Re: [PATCH] Fix segfaults caused by invalid tags

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Shouldn't a warning be issued in every of those cases? I don't know, but 
> those tags pointing nowhere don't seem useful to me.

Yes they should warn.

It is not just "don't seem useful"; it is a corrupt repository.

[PATCH] Warn when calling deref_tag() on broken tags

[Petr Baudis]

Dear diary, on Wed, Nov 02, 2005 at 09:16:13PM CET, I got a letter
where Johannes Schindelin <Johannes.Schindelin@gmx.de> told me that...
> On Wed, 2 Nov 2005, Petr Baudis wrote:
> 
> > On many places, we didn't bother checking deref_tag() return value against
> > NULL, causing GIT segfaulting e.g. on some old Cogito checkouts containing
> > residual tags from GIT.
> 
> Shouldn't a warning be issued in every of those cases? I don't know, but 
> those tags pointing nowhere don't seem useful to me.

Good idea. Not that I would be excited by the awing elegancy of the
patch...

---

When we hit a broken tag by deref_tag(), warn the user.

Signed-off-by: Petr Baudis <pasky@suse.cz>
---

 commit.c      |    2 +-
 fetch-pack.c  |    4 ++--
 name-rev.c    |    2 +-
 send-pack.c   |    4 ++--
 server-info.c |    2 +-
 sha1_name.c   |    2 +-
 tag.c         |   12 ++++++++++--
 tag.h         |    2 +-
 upload-pack.c |    2 +-
 9 files changed, 20 insertions(+), 12 deletions(-)

diff --git a/commit.c b/commit.c
index 8f40318..78aac42 100644
--- a/commit.c
+++ b/commit.c
@@ -55,7 +55,7 @@ static struct commit *check_commit(struc
 struct commit *lookup_commit_reference_gently(const unsigned char *sha1,
 					      int quiet)
 {
-	struct object *obj = deref_tag(parse_object(sha1));
+	struct object *obj = deref_tag(parse_object(sha1), NULL);
 
 	if (!obj)
 		return NULL;
diff --git a/fetch-pack.c b/fetch-pack.c
index 1fae211..14b5918 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -38,7 +38,7 @@ static void rev_list_push(struct commit 
 
 static int rev_list_insert_ref(const char *path, const unsigned char *sha1)
 {
-	struct object *o = deref_tag(parse_object(sha1));
+	struct object *o = deref_tag(parse_object(sha1), path);
 
 	if (o && o->type == commit_type)
 		rev_list_push((struct commit *)o, SEEN);
@@ -317,7 +317,7 @@ static int everything_local(struct ref *
 	 * Don't mark them common yet; the server has to be told so first.
 	 */
 	for (ref = *refs; ref; ref = ref->next) {
-		struct object *o = deref_tag(lookup_object(ref->old_sha1));
+		struct object *o = deref_tag(lookup_object(ref->old_sha1), ref->name);
 
 		if (!o || o->type != commit_type || !(o->flags & COMPLETE))
 			continue;
diff --git a/name-rev.c b/name-rev.c
index 21fecdf..8ac2d72 100644
--- a/name-rev.c
+++ b/name-rev.c
@@ -164,7 +164,7 @@ int main(int argc, char **argv)
 			continue;
 		}
 
-		o = deref_tag(parse_object(sha1));
+		o = deref_tag(parse_object(sha1), *argv);
 		if (!o || o->type != commit_type) {
 			fprintf(stderr, "Could not get commit for %s. Skipping.\n",
 					*argv);
diff --git a/send-pack.c b/send-pack.c
index 9f9a6e7..3ee6794 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -126,12 +126,12 @@ static int ref_newer(const unsigned char
 	/* Both new and old must be commit-ish and new is descendant of
 	 * old.  Otherwise we require --force.
 	 */
-	o = deref_tag(parse_object(old_sha1));
+	o = deref_tag(parse_object(old_sha1), NULL);
 	if (!o || o->type != commit_type)
 		return 0;
 	old = (struct commit *) o;
 
-	o = deref_tag(parse_object(new_sha1));
+	o = deref_tag(parse_object(new_sha1), NULL);
 	if (!o || o->type != commit_type)
 		return 0;
 	new = (struct commit *) o;
diff --git a/server-info.c b/server-info.c
index 8e13c94..23d89fb 100644
--- a/server-info.c
+++ b/server-info.c
@@ -13,7 +13,7 @@ static int add_info_ref(const char *path
 
 	fprintf(info_ref_fp, "%s	%s\n", sha1_to_hex(sha1), path);
 	if (o->type == tag_type) {
-		o = deref_tag(o);
+		o = deref_tag(o, path);
 		if (o)
 			fprintf(info_ref_fp, "%s	%s^{}\n",
 				sha1_to_hex(o->sha1), path);
diff --git a/sha1_name.c b/sha1_name.c
index fe409fb..ae4c801 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -349,7 +349,7 @@ static int peel_onion(const char *name, 
 	if (!o)
 		return -1;
 	if (!type_string) {
-		o = deref_tag(o);
+		o = deref_tag(o, NULL);
 		if (!o || (!o->parsed && !parse_object(o->sha1)))
 			return -1;
 		memcpy(sha1, o->sha1, 20);
diff --git a/tag.c b/tag.c
index b1ab75f..9cd09cf 100644
--- a/tag.c
+++ b/tag.c
@@ -3,10 +3,18 @@
 
 const char *tag_type = "tag";
 
-struct object *deref_tag(struct object *o)
+/* The refname argument is optional and suitable to use only if
+ * we use this to resolve an actual tag ref. */
+struct object *deref_tag(struct object *o, const char *refname)
 {
+	struct object *o2 = o;
 	while (o && o->type == tag_type)
-		o = parse_object(((struct tag *)o)->tagged->sha1);
+		o = parse_object(((struct tag *)(o2 = o))->tagged->sha1);
+	if (!o)
+		fprintf(stderr, "warning: invalid tag %s\n",
+		        refname ? refname
+		        : (o2 ? sha1_to_hex(o2->sha1)
+		           : "<unknown> (this is probably internal GIT error)"));
 	return o;
 }
 
diff --git a/tag.h b/tag.h
index 36e5324..2b60b45 100644
--- a/tag.h
+++ b/tag.h
@@ -15,6 +15,6 @@ struct tag {
 extern struct tag *lookup_tag(const unsigned char *sha1);
 extern int parse_tag_buffer(struct tag *item, void *data, unsigned long size);
 extern int parse_tag(struct tag *item);
-extern struct object *deref_tag(struct object *);
+extern struct object *deref_tag(struct object *, const char *refname);
 
 #endif /* TAG_H */
diff --git a/upload-pack.c b/upload-pack.c
index f0b8e63..cc5b3e3 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -226,7 +226,7 @@ static int send_ref(const char *refname,
 		nr_our_refs++;
 	}
 	if (o->type == tag_type) {
-		o = deref_tag(o);
+		o = deref_tag(o, refname);
 		if (o)
 			packet_write(1, "%s %s^{}\n", sha1_to_hex(o->sha1), refname);
 	}

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
VI has two modes: the one in which it beeps and the one in which
it doesn't.

Re: [PATCH] Warn when calling deref_tag() on broken tags

[Johannes Schindelin]

Hi,

On Wed, 2 Nov 2005, Petr Baudis wrote:

> diff --git a/tag.c b/tag.c
> index b1ab75f..9cd09cf 100644
> --- a/tag.c
> +++ b/tag.c
> @@ -3,10 +3,18 @@
>  
>  const char *tag_type = "tag";
>  
> -struct object *deref_tag(struct object *o)
> +/* The refname argument is optional and suitable to use only if
> + * we use this to resolve an actual tag ref. */
> +struct object *deref_tag(struct object *o, const char *refname)

How about leaving dereg_tag's signature alone, and warn something like 

	"You have a broken tag (sha1=%s).\n"
	"To find out which name(s) it corresponds to, execute\n"
	"\tgit-name-rev --tags %s\n"

What do you think?

Ciao,
Dscho

Re: [PATCH] Warn when calling deref_tag() on broken tags

[Petr Baudis]

Dear diary, on Wed, Nov 02, 2005 at 09:47:46PM CET, I got a letter
where Johannes Schindelin <Johannes.Schindelin@gmx.de> told me that...
> On Wed, 2 Nov 2005, Petr Baudis wrote:
> > diff --git a/tag.c b/tag.c
> > index b1ab75f..9cd09cf 100644
> > --- a/tag.c
> > +++ b/tag.c
> > @@ -3,10 +3,18 @@
> >  
> >  const char *tag_type = "tag";
> >  
> > -struct object *deref_tag(struct object *o)
> > +/* The refname argument is optional and suitable to use only if
> > + * we use this to resolve an actual tag ref. */
> > +struct object *deref_tag(struct object *o, const char *refname)
> 
> How about leaving dereg_tag's signature alone, and warn something like 
> 
> 	"You have a broken tag (sha1=%s).\n"
> 	"To find out which name(s) it corresponds to, execute\n"
> 	"\tgit-name-rev --tags %s\n"
> 
> What do you think?

I don't know. For multiple broken tags, or multiple invocations of
deref_tag() on the same tag, this will fill a lot of screen estate. And
if we mostly actually have this information, why not show it to the user
right aay?

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
VI has two modes: the one in which it beeps and the one in which
it doesn't.

Re: [PATCH] Warn when calling deref_tag() on broken tags

[Junio C Hamano]

Petr Baudis <pasky@suse.cz> writes:

> +struct object *deref_tag(struct object *o, const char *refname)
>  {
> +	struct object *o2 = o;
>  	while (o && o->type == tag_type)
> -		o = parse_object(((struct tag *)o)->tagged->sha1);
> +		o = parse_object(((struct tag *)(o2 = o))->tagged->sha1);
> +	if (!o)
> +		fprintf(stderr, "warning: invalid tag %s\n",
> +		        refname ? refname
> +		        : (o2 ? sha1_to_hex(o2->sha1)
> +		           : "<unknown> (this is probably internal GIT error)"));
>  	return o;
>  }

I wonder if it would make more sense to keep the root of the
traversal for this function (i.e. the original value of o) for
error reporting purposes, like this:

        struct object *deref_tag(struct object *o, const char *refname)
        {
                struct object *o2 = o;
                while (o && o->type == tag_type)
                        o = parse_object(((struct tag *)o)->tagged->sha1);
                if (!o)
                        fprintf(stderr, "warning: invalid tag %s\n",
                                refname ? refname
                                : (o2 ? sha1_to_hex(o2->sha1)
                                   : "<unknown> (this is probably internal GIT error)"));
                return o;
        }

What do you think?

Other than that, the patch is OK by me.  Thanks.

Re: [PATCH] Warn when calling deref_tag() on broken tags

[Junio C Hamano]

Petr Baudis <pasky@suse.cz> writes:

> Good idea. Not that I would be excited by the awing elegancy of the
> patch...

It turns out to be not so good idea.  Some places call deref_tag
just to see if we can unwrap it but not having the referenced
object but having the tag object does not necessarily mean a
corrupt repository.

For example, after a fetch that retrieved and stored a tag
object but was interrupted before retrieving and storing the
object the tag refers to, we do not update the refs of our end
(which is correct).  After this, imagine fetching from the same
remote for the same tag.  fetch-pack.c::everything_local will
find that we do have the tag object the remote says it has,
hoping that we can mark it one of the common refs, if that tag
is something reachable from our refs (it is not).  The caller of
the deref_tag() there is careful not to assume the tag is
complete, but it should not bark -- we should just ignore and
pretend that dangling tag object is something we do not have.

I've done it a slightly differently.  Does this look OK to you?

-- >8 -- cut here -- >8 --

Subject: [PATCH] Be careful when dereferencing tags.

One caller of deref_tag() was not careful enough to make sure
what deref_tag() returned was not NULL (i.e. we found a tag
object that points at an object we do not have).  Fix it, and
warn about refs that point at such an incomplete tag where
needed.

Signed-off-by: Junio C Hamano <junkio@cox.net>

---

 commit.c      |    2 +-
 fetch-pack.c  |    7 ++++---
 name-rev.c    |    2 +-
 send-pack.c   |    4 ++--
 server-info.c |    7 ++++---
 sha1_name.c   |    2 +-
 tag.c         |    7 ++++++-
 tag.h         |    2 +-
 upload-pack.c |    2 +-
 9 files changed, 21 insertions(+), 14 deletions(-)

applies-to: 003cf1e61da5c5fe13ed574ae3630c1ee8b56cae
30a28f848f666ae6f6641baa2917a8ea15ad3160
diff --git a/commit.c b/commit.c
index 8f40318..a8c9bfc 100644
--- a/commit.c
+++ b/commit.c
@@ -55,7 +55,7 @@ static struct commit *check_commit(struc
 struct commit *lookup_commit_reference_gently(const unsigned char *sha1,
 					      int quiet)
 {
-	struct object *obj = deref_tag(parse_object(sha1));
+	struct object *obj = deref_tag(parse_object(sha1), NULL, 0);
 
 	if (!obj)
 		return NULL;
diff --git a/fetch-pack.c b/fetch-pack.c
index 3df9911..cb21715 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -38,9 +38,9 @@ static void rev_list_push(struct commit 
 
 static int rev_list_insert_ref(const char *path, const unsigned char *sha1)
 {
-	struct object *o = deref_tag(parse_object(sha1));
+	struct object *o = deref_tag(parse_object(sha1), path, 0);
 
-	if (o->type == commit_type)
+	if (o && o->type == commit_type)
 		rev_list_push((struct commit *)o, SEEN);
 
 	return 0;
@@ -317,7 +317,8 @@ static int everything_local(struct ref *
 	 * Don't mark them common yet; the server has to be told so first.
 	 */
 	for (ref = *refs; ref; ref = ref->next) {
-		struct object *o = deref_tag(lookup_object(ref->old_sha1));
+		struct object *o = deref_tag(lookup_object(ref->old_sha1),
+					     NULL, 0);
 
 		if (!o || o->type != commit_type || !(o->flags & COMPLETE))
 			continue;
diff --git a/name-rev.c b/name-rev.c
index 21fecdf..59194f1 100644
--- a/name-rev.c
+++ b/name-rev.c
@@ -164,7 +164,7 @@ int main(int argc, char **argv)
 			continue;
 		}
 
-		o = deref_tag(parse_object(sha1));
+		o = deref_tag(parse_object(sha1), *argv, 0);
 		if (!o || o->type != commit_type) {
 			fprintf(stderr, "Could not get commit for %s. Skipping.\n",
 					*argv);
diff --git a/send-pack.c b/send-pack.c
index 9f9a6e7..3eeb18f 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -126,12 +126,12 @@ static int ref_newer(const unsigned char
 	/* Both new and old must be commit-ish and new is descendant of
 	 * old.  Otherwise we require --force.
 	 */
-	o = deref_tag(parse_object(old_sha1));
+	o = deref_tag(parse_object(old_sha1), NULL, 0);
 	if (!o || o->type != commit_type)
 		return 0;
 	old = (struct commit *) o;
 
-	o = deref_tag(parse_object(new_sha1));
+	o = deref_tag(parse_object(new_sha1), NULL, 0);
 	if (!o || o->type != commit_type)
 		return 0;
 	new = (struct commit *) o;
diff --git a/server-info.c b/server-info.c
index ba53591..0cba8e1 100644
--- a/server-info.c
+++ b/server-info.c
@@ -13,9 +13,10 @@ static int add_info_ref(const char *path
 
 	fprintf(info_ref_fp, "%s	%s\n", sha1_to_hex(sha1), path);
 	if (o->type == tag_type) {
-		o = deref_tag(o);
-		fprintf(info_ref_fp, "%s	%s^{}\n",
-			sha1_to_hex(o->sha1), path);
+		o = deref_tag(o, path, 0);
+		if (o)
+			fprintf(info_ref_fp, "%s	%s^{}\n",
+				sha1_to_hex(o->sha1), path);
 	}
 	return 0;
 }
diff --git a/sha1_name.c b/sha1_name.c
index fe409fb..be1755a 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -349,7 +349,7 @@ static int peel_onion(const char *name, 
 	if (!o)
 		return -1;
 	if (!type_string) {
-		o = deref_tag(o);
+		o = deref_tag(o, name, sp - name - 2);
 		if (!o || (!o->parsed && !parse_object(o->sha1)))
 			return -1;
 		memcpy(sha1, o->sha1, 20);
diff --git a/tag.c b/tag.c
index b1ab75f..e574c4b 100644
--- a/tag.c
+++ b/tag.c
@@ -3,10 +3,15 @@
 
 const char *tag_type = "tag";
 
-struct object *deref_tag(struct object *o)
+struct object *deref_tag(struct object *o, const char *warn, int warnlen)
 {
 	while (o && o->type == tag_type)
 		o = parse_object(((struct tag *)o)->tagged->sha1);
+	if (!o && warn) {
+		if (!warnlen)
+			warnlen = strlen(warn);
+		error("missing object referenced by '%.*s'", warnlen, warn);
+	}
 	return o;
 }
 
diff --git a/tag.h b/tag.h
index 36e5324..7a0cb00 100644
--- a/tag.h
+++ b/tag.h
@@ -15,6 +15,6 @@ struct tag {
 extern struct tag *lookup_tag(const unsigned char *sha1);
 extern int parse_tag_buffer(struct tag *item, void *data, unsigned long size);
 extern int parse_tag(struct tag *item);
-extern struct object *deref_tag(struct object *);
+extern struct object *deref_tag(struct object *, const char *, int);
 
 #endif /* TAG_H */
diff --git a/upload-pack.c b/upload-pack.c
index c5eff21..be63132 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -226,7 +226,7 @@ static int send_ref(const char *refname,
 		nr_our_refs++;
 	}
 	if (o->type == tag_type) {
-		o = deref_tag(o);
+		o = deref_tag(o, refname, 0);
 		packet_write(1, "%s %s^{}\n", sha1_to_hex(o->sha1), refname);
 	}
 	return 0;
---
0.99.9.GIT

Re: [PATCH] Warn when calling deref_tag() on broken tags

[Petr Baudis]

Dear diary, on Thu, Nov 03, 2005 at 12:21:01AM CET, I got a letter
where Junio C Hamano <junkio@cox.net> told me that...
> Petr Baudis <pasky@suse.cz> writes:
> 
> > Good idea. Not that I would be excited by the awing elegancy of the
> > patch...
> 
> It turns out to be not so good idea.  Some places call deref_tag
> just to see if we can unwrap it but not having the referenced
> object but having the tag object does not necessarily mean a
> corrupt repository.

Aha, that didn't occur to me initially. Yes, what you did is fine by me
(not that I would be overly proficient in this part of code).

> diff --git a/tag.c b/tag.c
> index b1ab75f..e574c4b 100644
> --- a/tag.c
> +++ b/tag.c
> @@ -3,10 +3,15 @@
>  
>  const char *tag_type = "tag";
>  
> -struct object *deref_tag(struct object *o)
> +struct object *deref_tag(struct object *o, const char *warn, int warnlen)
>  {
>  	while (o && o->type == tag_type)
>  		o = parse_object(((struct tag *)o)->tagged->sha1);
> +	if (!o && warn) {
> +		if (!warnlen)
> +			warnlen = strlen(warn);
> +		error("missing object referenced by '%.*s'", warnlen, warn);
> +	}
>  	return o;
>  }
>  

It should still be a warning, not an error, though. I'd also mention the
"tag" keyword in the message.

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
VI has two modes: the one in which it beeps and the one in which
it doesn't.