[PATCH] builtin-fast-export.c: add default case to avoid crash on corrupt repo

[PATCH] builtin-fast-export.c: add default case to avoid crash on corrupt repo

[Erik Faye-Lund]

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

diff --git a/builtin-fast-export.c b/builtin-fast-export.c
index fdf4ae9..26b2a93 100644
--- a/builtin-fast-export.c
+++ b/builtin-fast-export.c
@@ -375,6 +375,9 @@ static void get_tags_and_duplicates(struct object_array *pending,
 			case OBJ_BLOB:
 				handle_object(tag->object.sha1);
 				continue;
+			default:
+				die ("Unexpected object of type %s",
+				     typename(tag->object.type));
 			}
 			break;
 		default:
-- 
1.6.2.1.215.gf786.dirty

Re: [PATCH] builtin-fast-export.c: add default case to avoid crash on corrupt repo

[Junio C Hamano]

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

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

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.

Re: [PATCH] builtin-fast-export.c: add default case to avoid crash on corrupt repo

[Erik Faye-Lund]

On Sun, Mar 22, 2009 at 12:54 AM, Junio C Hamano <gitster@pobox.com> wrote:
> A tag can point at anything, so this is not an issue about "crash on a
> _corrupt_ repository".

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.

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

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.

-- 
Erik "kusma" Faye-Lund
kusmabite@gmail.com
(+47) 986 59 656

Re: [PATCH] builtin-fast-export.c: add default case to avoid crash on corrupt repo

[Erik Faye-Lund]

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

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


-- 
Erik "kusma" Faye-Lund
kusmabite@gmail.com
(+47) 986 59 656

Re: [PATCH] builtin-fast-export.c: add default case to avoid crash on corrupt repo

[Junio C Hamano]

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

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

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.

Re: [PATCH] builtin-fast-export.c: add default case to avoid crash on corrupt repo

[Shawn O. Pearce]

Junio C Hamano <gitster@pobox.com> wrote:
> 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.

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.

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

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

-- 
Shawn.