git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Erik Faye-Lund <kusmabite@googlemail.com>
Date
Mar 22, 2009, 12:22 UTC
Message-ID
<40aa078e0903220522g66cf2172l9f1a43ed562cc4d3@mail.gmail.com>
In-Reply-To
<7vd4cabffl.fsf@gitster.siamese.dyndns.org>
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.

Show 10 quoted lines
> 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
Previous: Junio C HamanoNext: Erik Faye-Lund
Message 3 of 6 in “builtin-fast-export.c: add default case to avoid crash on corrupt repo”
  1. builtin-fast-export.c: add default case to avoid crash on corrupt repoErik Faye-Lund, Mar 21, 2009
  2. Junio C HamanoMar 21, 2009
  3. Erik Faye-LundMar 22, 2009
  4. Erik Faye-LundMar 22, 2009
  5. Junio C HamanoMar 22, 2009
  6. Shawn O. PearceMar 23, 2009

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.