[PATCH] fsck: do not crash on tag objects which do not contain an empty line

[PATCH] fsck: do not crash on tag objects which do not contain an empty line

[Johannes Schindelin]

The first empty line in a tag object separates the header from the
message. If the tag object has no empty line, do not crash, but
complain loudly instead.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	I have no idea how this tag crept into one of my repos, but it is 
	no good to crash for git-fsck.

 builtin-fsck.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/builtin-fsck.c b/builtin-fsck.c
index 7a92e47..607136a 100644
--- a/builtin-fsck.c
+++ b/builtin-fsck.c
@@ -545,9 +545,14 @@ static void fsck_verify_ref_to_tag_object(const char *refname, struct object *ob
 {
 	/* Verify that refname matches the name stored in obj's "tag" header */
 	struct tag *tagobj = (struct tag *) parse_object(obj->sha1);
-	size_t tagname_len = strlen(tagobj->tag);
+	size_t tagname_len;
 	size_t refname_len = strlen(refname);
 
+	if (!tagobj->tag) {
+		error("tag %s does not contain any tag?", refname);
+		return;
+	}
+	tagname_len = strlen(tagobj->tag);
 	if (!tagname_len) return; /* No tag name stored in tagobj. Nothing to do. */
 
 	if (tagname_len < refname_len &&
-- 
1.5.2.1.2683.gab86-dirty

Re: [PATCH] fsck: do not crash on tag objects which do not contain an empty line

[Johan Herland]

On Friday 08 June 2007, Johannes Schindelin wrote:
> 
> The first empty line in a tag object separates the header from the
> message. If the tag object has no empty line, do not crash, but
> complain loudly instead.

Aren't tag objects _required_ to have an empty line separating the headers 
from the body? At least I wrote the new tag code with that assumption in 
mind.

Could this be related to the "error: char103: premature end of data" you're 
seeing?


...Johan


-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCH] fsck: do not crash on tag objects which do not contain an empty line

[Johannes Schindelin]

Hi,

On Fri, 8 Jun 2007, Johan Herland wrote:

> On Friday 08 June 2007, Johannes Schindelin wrote:
> > 
> > The first empty line in a tag object separates the header from the 
> > message. If the tag object has no empty line, do not crash, but 
> > complain loudly instead.
> 
> Aren't tag objects _required_ to have an empty line separating the 
> headers from the body? At least I wrote the new tag code with that 
> assumption in mind.

Yes, evidently you did.

But even then, isn't it always better to not rely on such assumptions, but 
fail gracefully? The rest of Git's source code seems to be nicer to 
failures as this one, IMHO.

> Could this be related to the "error: char103: premature end of data" 
> you're seeing?

Definitely. It breaks even _fetching_.

Ciao,
Dscho

Re: [PATCH] fsck: do not crash on tag objects which do not contain an empty line

[Johan Herland]

On Friday 08 June 2007, Johannes Schindelin wrote:
> Hi,
> 
> On Fri, 8 Jun 2007, Johan Herland wrote:
> 
> > On Friday 08 June 2007, Johannes Schindelin wrote:
> > > 
> > > The first empty line in a tag object separates the header from the 
> > > message. If the tag object has no empty line, do not crash, but 
> > > complain loudly instead.
> > 
> > Aren't tag objects _required_ to have an empty line separating the 
> > headers from the body? At least I wrote the new tag code with that 
> > assumption in mind.
> 
> Yes, evidently you did.
> 
> But even then, isn't it always better to not rely on such assumptions, but 
> fail gracefully? The rest of Git's source code seems to be nicer to 
> failures as this one, IMHO.

I agree that we should fail gracefully, and my code is clearly not doing 
that in this case. My bad.

But the code should also detect invalid tag objects, and in this case I'm 
not yet convinced that the tag object causing the failure is in fact valid. 
If someone can convince me that the blank line after headers is optional, 
then I'll gladly fix the code.

> > Could this be related to the "error: char103: premature end of data" 
> > you're seeing?
> 
> Definitely. It breaks even _fetching_.

Sorry again. Still, if I could get a look at the object that'd help me alot 
in debugging.


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCH] fsck: do not crash on tag objects which do not contain an empty line

[Johannes Schindelin]

Hi,

On Fri, 8 Jun 2007, Johan Herland wrote:

> On Friday 08 June 2007, Johannes Schindelin wrote:
> > 
> > On Fri, 8 Jun 2007, Johan Herland wrote:
> > 
> > > On Friday 08 June 2007, Johannes Schindelin wrote:
> > > > 
> > > > The first empty line in a tag object separates the header from the 
> > > > message. If the tag object has no empty line, do not crash, but 
> > > > complain loudly instead.
> > > 
> > > Aren't tag objects _required_ to have an empty line separating the 
> > > headers from the body? At least I wrote the new tag code with that 
> > > assumption in mind.
> > 
> > Yes, evidently you did.
> > 
> > But even then, isn't it always better to not rely on such assumptions, but 
> > fail gracefully? The rest of Git's source code seems to be nicer to 
> > failures as this one, IMHO.
> 
> I agree that we should fail gracefully, and my code is clearly not doing 
> that in this case. My bad.
> 
> But the code should also detect invalid tag objects, and in this case I'm 
> not yet convinced that the tag object causing the failure is in fact valid. 

IT BROKE GIT IN A REPO THAT WAS PERFECTLY VALID BEFORE, AND I COULD NOT DO 
ANYTHING AFTER THAT CHANGE.

Thank you very much again.

> If someone can convince me that the blank line after headers is optional, 
> then I'll gladly fix the code.

Convincing enough?

> > > Could this be related to the "error: char103: premature end of data" 
> > > you're seeing?
> > 
> > Definitely. It breaks even _fetching_.
> 
> Sorry again. Still, if I could get a look at the object that'd help me alot 
> in debugging.

object f90084c7b53b1c2fb4606acafd84ef8a748a7d78
type commit
tag start
tagger me <me>

But I have to say that I am unlikely to review any fix you make, if that 
fix is as unreadable as those mega long lines with funny spaces in the 
middle of the line in that mega patch that unfortunately was already 
applied in next.

I need a beer,
Dscho

Re: [PATCH] fsck: do not crash on tag objects which do not contain an empty line

[Nicolas Pitre]

On Fri, 8 Jun 2007, Johan Herland wrote:

> I agree that we should fail gracefully, and my code is clearly not doing 
> that in this case. My bad.
> 
> But the code should also detect invalid tag objects, and in this case I'm 
> not yet convinced that the tag object causing the failure is in fact valid. 
> If someone can convince me that the blank line after headers is optional, 
> then I'll gladly fix the code.

That's irrelevant.

Because you are fed invalid data is no excuse for crashing.

Especially in a tool like fsck, you should _expect_ and cope with 
invalid data. That's why it exists in the first place: to identify such
data.


Nicolas

Re: [PATCH] fsck: do not crash on tag objects which do not contain an empty line

[Johan Herland]

On Friday 08 June 2007, Nicolas Pitre wrote:
> On Fri, 8 Jun 2007, Johan Herland wrote:
> 
> > I agree that we should fail gracefully, and my code is clearly not doing 
> > that in this case. My bad.
> > 
> > But the code should also detect invalid tag objects, and in this case I'm 
> > not yet convinced that the tag object causing the failure is in fact valid. 
> > If someone can convince me that the blank line after headers is optional, 
> > then I'll gladly fix the code.
> 
> That's irrelevant.
> 
> Because you are fed invalid data is no excuse for crashing.
> 
> Especially in a tool like fsck, you should _expect_ and cope with 
> invalid data. That's why it exists in the first place: to identify such
> data.

Of course. If you read my first sentence in the quote above, you will see
that I agree with you 100%.


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCH] fsck: do not crash on tag objects which do not contain an empty line

[Junio C Hamano]

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

> The first empty line in a tag object separates the header from the
> message. If the tag object has no empty line, do not crash, but
> complain loudly instead.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>
> 	I have no idea how this tag crept into one of my repos, but it is 
> 	no good to crash for git-fsck.

Absolutely.  git-fsck is to find suspicious data and should not
crash on them.

Thanks.

But it _does_ make me wonder how.  Recent git-tag does this

    ( printf 'object %s\ntype %s\ntag %s\ntagger %s\n\n' \
	"$object" "$type" "$name" "$tagger";
      cat "$GIT_DIR"/TAG_FINALMSG ) >"$GIT_DIR"/TAG_TMP

so even if TAG_FINALMSG (which is -m or edited message after
filtering out the comments and git-stripspace) is empty, you
would have the two LFs at the end of the header.  Ancient
"git-tag-script" did this, which is a moral equivalent with echo
and has the same effect:

  ( echo -e "object $object\ntype $type\ntag $name\ntagger $tagger\n";
    cat .tagmsg ) > .tmp-t

Did this tag come from cvsimport (or svnimport) perhaps?  We are
in the process of updating its use of git-mktag to git-tag, but
they use git-mktag and do not leave a blank line after the
header (and they do not add any body).

I notice that gitk creates lightweight tag by hand (it directly
goes to the filesystem)  It should probably use git-tag as well.

I agree with you that if something does not have body, we should
not require an empty trailing line after the header.

Re: [PATCH] fsck: do not crash on tag objects which do not contain an empty line

[Johannes Schindelin]

Hi,

On Thu, 7 Jun 2007, Junio C Hamano wrote:

> Recent git-tag does this
> 
>     ( printf 'object %s\ntype %s\ntag %s\ntagger %s\n\n' \
> 	"$object" "$type" "$name" "$tagger";
>       cat "$GIT_DIR"/TAG_FINALMSG ) >"$GIT_DIR"/TAG_TMP
> 
> so even if TAG_FINALMSG (which is -m or edited message after
> filtering out the comments and git-stripspace) is empty, you
> would have the two LFs at the end of the header.

I could imagine that there is still a git-stripspace after that. Undoing 
the last LF.

> Did this tag come from cvsimport (or svnimport) perhaps?

Yep. cvsimport.

But nevertheless, I know the convention of "LFLF" being the end of the 
header from several places: HTTP, mail, proxies.

_None_ of them fail as badly as what I experienced. Heck, Git used to be 
the _most_ stable software I ever used, _including_ the Linux kernel. And 
all of a sudden, when I really needed it just to work properly, and did 
not bother to install a "stable" version, but just took "next", it failed 
on me.

I don't like this at all.

And I have to say, even if I was responsible for my share of 
not-quite-perfect patches, this patch _series_ is bad IMHO.

Not only does already the _first_ patch contain a serious error. I did not 
even bother to read it in total, because it was _huge_, contained 
_white-space_ fixes in combination with some dubious comment changes, did 
not clean up _obvious_ errors (like lookup_object(sha1)->object!!), but at 
the same time _changed_ behaviour.

And it is totally unreadable, with 120+ characters per line.

Frankly, I am angry at myself that I did not have a look and shot that 
patch down on the merits of its _line size_ alone.

I have come to expect bad code from bad style and I would have been right 
again, had I read the patch in time.

> I agree with you that if something does not have body, we should not 
> require an empty trailing line after the header.

It is just _one_ issue about it.

We should not fail to process a fetch _at all_, just because a single 
_tag_ is not really conforming to some obscure convention.

Ciao,
Dscho "still somewhat upset"

Re: [PATCH] fsck: do not crash on tag objects which do not contain an empty line

[Andy Parkins]

On Thursday 2007 June 07, Johannes Schindelin wrote:

> +		error("tag %s does not contain any tag?", refname);

Seems a bad error message to me.  Also, why is it a question?

 git: tag does not contain a tag
 user: WTF?

Perhaps, "The annotated tag, %s, does not contain a message", or similar?

Also - is this really something that should be complained about?  I'm 
wondering why it is important that tag objects contain a message?  For 
example, tagging a release as v1.0.0 - it's important to record who tagged 
it, and when it was tagged, but I can certainly envisage there not being a 
message to go with it.  Most tags in the git repository are just a repeat of 
the tag name in the message.




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