patchbuiltin-fast-export.c: add default case to avoid crash on corrupt repo

This small issue was discovered by Benjamin Kramers Clang-runs on the git code-base. If a tag object points to an object that is not a commit or a blob, an invalid pointer is dereferenced in the code that followed.

This patch fixes this issue, by giving an error instead. This should also be more useful when debugging corrupted repos.

Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
 builtin-fast-export.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

Erik Faye-Lund <kusmabite@gmail.com> writes:

A tag can point at anything, so this is not an issue about "crash on a _corrupt_ repository".

I am not very familiar with this program, but the codepath involved should be prepared to _see_ any type of object instead of dying.

What to do after _seeing_ a type of object is a different matter. It appears that there is no way to feed a tree object to fast-import, but I think the fast-import language can represent a tag that points at another tag just fine. So the best you can do is perhaps to issue a warning "skipping a tag that points at a tree object" and impoement a proper handling of a tag that points at a tag.

On Sun, Mar 22, 2009 at 12:54 AM, Junio C Hamano <gitster@pobox.com> wrote:

Ah, my bad. I wrongly assumed corrupted repos were the only ways of triggering this issue. I quite easily managed to reproduce the crash by setting up some tags to trees and tag objects to trees.

My patch simply applied the same error that was already present for tags to tag objects, but yeah. Handling tagged tags and warning instead of erroring-out makes more sense to me as well. I'll see if I can write it up, and resubmit a patch.

After looking some more at the code, it seems that there's an attempt to handle tags to tags there already, but it doesn't seem to work properly; the program error out with a "fatal: Tag [tagname] points nowhere?". This seems to be because the tagged-pointer of the second tag-object is NULL. Now, I'm no expert, but from browsing some code, it seems that "parse_object(tag->object.sha1)" should have been performed on the tag before looking up the tagged object. Does this make sense?

Also, I guess this calls for a couple of test-cases or something. I haven't written any tests yet, so I might need some time to figuring it out.

Anyway, I guess this makes the most sense as a four-patch series:
1) Add test-cases for tags of tag objects, tag objects of tag objects,
tags of trees and tag objects of trees.
2) Turn the existing error into a warning
3) Add the missing warning and remove the crash
4) Fix fast-export to export tags pointing to tags.

Makes sense?

Additionally, to reduce the chance of similar bugs in the future, the code could be refactored a bit to have a handle_commit()-function, so what goes on becomes a bit more obvious. Since this doesn't really change any functionality, I guess it could be handed in as a separate patch.

I guess I forgot to mention that adding a call to parse_object() does make fast-exporting nested tags appear to work.

Erik Faye-Lund <kusmabite@googlemail.com> writes:

Yes.

I suspect we will hear an argument that say the dying is deliberate because pretending to have produced a faithful export while some information is lacking is bad and warnings are easy to miss, and I would certainly understand that argument.

We have similar issue with signed tags and filter-branch, which strips the signature when it rewrites the history. I recall the code originally died for the same reasoning, but asking "it died; what can the user do now?" made us realize that it is the best we can do to make best-effort reproduction with a warning when losing information.

This falls into the same category. I think "we die by default if we cannot give a faithful reproduction, and the user needs to give an explicit permission (e.g. --force option) to lose information", is a very sensible thing to do, but "we die if we cannot export some things, and we refuse to produce an approximation." without an escape hatch is not.

Junio C Hamano <gitster@pobox.com> wrote:

Correct.

This area of the langauge was built around the basic CVS->Git sort of conversion, where we just wanted to attach the CVS "tag" symbol onto a Git snapshot that seemed to match it. Annotated tags were used only because tools like git fetch and git describe prefer to work with them, and these were (at some point in time) meaningful labels for these revisions so the project post-conversion should retain those same labels.

In hindsight, parts of the fast-import langauge are problematic as they don't fully represent the Git object graph, and this is one of those areas. It wasn't designed to represent the same range of structures as Git can represent.

Or, patch fast-import to expand its language. You could relax the "from <committish>" rule to be "from <objectish>" and then create tag a tree using a temporary branch and a from line of "from temp-branch^{tree}".