{"thread":{"id":"18459","subject":"[PATCH] builtin-fast-export.c: add default case to avoid crash on corrupt repo","startedAt":"2009-03-21T22:37:31Z","lastAt":"2009-03-23T00:26:36Z","messageCount":6,"participants":["Erik Faye-Lund","Junio C Hamano","Shawn O. Pearce"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"108855","messageId":"1237675051-6688-1-git-send-email-kusmabite@gmail.com","threadId":"18459","inReplyTo":null,"subject":"[PATCH] builtin-fast-export.c: add default case to avoid crash on corrupt repo","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2009-03-21T22:37:31Z","receivedAt":"2009-03-21T22:37:31Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"This small issue was discovered by Benjamin Kramers Clang-runs on the git\ncode-base. If a tag object points to an object that is not a commit or a blob,\nan invalid pointer is dereferenced in the code that followed.\n\nThis patch fixes this issue, by giving an error instead. This should also be\nmore useful when debugging corrupted repos.\n\nSigned-off-by: Erik Faye-Lund \n---\n builtin-fast-export.c | 3 +++\n 1 files changed, 3 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-fast-export.c b/builtin-fast-export.c\nindex fdf4ae9..26b2a93 100644\n--- a/builtin-fast-export.c\n+++ b/builtin-fast-export.c\n@@ -375,6 +375,9 @@ static void get_tags_and_duplicates(struct object_array *pending,\n \t\t\tcase OBJ_BLOB:\n \t\t\t\thandle_object(tag->object.sha1);\n \t\t\t\tcontinue;\n+\t\t\tdefault:\n+\t\t\t\tdie (\"Unexpected object of type %s\",\n+\t\t\t\t typename(tag->object.type));\n \t\t\t}\n \t\t\tbreak;\n \t\tdefault:\n-- \n1.6.2.1.215.gf786.dirty\n"},{"id":"108858","messageId":"7vd4cabffl.fsf@gitster.siamese.dyndns.org","threadId":"18459","inReplyTo":"1237675051-6688-1-git-send-email-kusmabite@gmail.com","subject":"Re: [PATCH] builtin-fast-export.c: add default case to avoid crash on corrupt repo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-21T23:54:06Z","receivedAt":"2009-03-21T23:54:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Erik Faye-Lund writes:\n\n> This small issue was discovered by Benjamin Kramers Clang-runs on the\n> git code-base. If a tag object points to an object that is not a commit\n> or a blob, an invalid pointer is dereferenced in the code that followed.\n\nA tag can point at anything, so this is not an issue about \"crash on a\n_corrupt_ repository\".\n\nI am not very familiar with this program, but the codepath involved should\nbe prepared to _see_ any type of object instead of dying.\n\nWhat to do after _seeing_ a type of object is a different matter. It\nappears that there is no way to feed a tree object to fast-import, but I\nthink the fast-import language can represent a tag that points at another\ntag just fine. So the best you can do is perhaps to issue a warning\n\"skipping a tag that points at a tree object\" and impoement a proper\nhandling of a tag that points at a tag.\n"},{"id":"108893","messageId":"40aa078e0903220522g66cf2172l9f1a43ed562cc4d3@mail.gmail.com","threadId":"18459","inReplyTo":"7vd4cabffl.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] builtin-fast-export.c: add default case to avoid crash on corrupt repo","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2009-03-22T12:22:39Z","receivedAt":"2009-03-22T12:22:39Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Sun, Mar 22, 2009 at 12:54 AM, Junio C Hamano wrote:\n> A tag can point at anything, so this is not an issue about \"crash on a\n> _corrupt_ repository\".\n\nAh, my bad. I wrongly assumed corrupted repos were the only ways of\ntriggering this issue. I quite easily managed to reproduce the crash\nby setting up some tags to trees and tag objects to trees.\n\n> I am not very familiar with this program, but the codepath involved should\n> be prepared to _see_ any type of object instead of dying.\n>\n> What to do after _seeing_ a type of object is a different matter. It\n> appears that there is no way to feed a tree object to fast-import, but I\n> think the fast-import language can represent a tag that points at another\n> tag just fine. So the best you can do is perhaps to issue a warning\n> \"skipping a tag that points at a tree object\" and impoement a proper\n> handling of a tag that points at a tag.\n>\n\nMy patch simply applied the same error that was already present for\ntags to tag objects, but yeah. Handling tagged tags and warning\ninstead of erroring-out makes more sense to me as well. I'll see if I\ncan write it up, and resubmit a patch.\n\nAfter looking some more at the code, it seems that there's an attempt\nto handle tags to tags there already, but it doesn't seem to work\nproperly; the program error out with a \"fatal: Tag [tagname] points\nnowhere?\". This seems to be because the tagged-pointer of the second\ntag-object is NULL. Now, I'm no expert, but from browsing some code,\nit seems that \"parse_object(tag->object.sha1)\" should have been\nperformed on the tag before looking up the tagged object. Does this\nmake sense?\n\nAlso, I guess this calls for a couple of test-cases or something. I\nhaven't written any tests yet, so I might need some time to figuring\nit out.\n\nAnyway, I guess this makes the most sense as a four-patch series:\n1) Add test-cases for tags of tag objects, tag objects of tag objects,\ntags of trees and tag objects of trees.\n2) Turn the existing error into a warning\n3) Add the missing warning and remove the crash\n4) Fix fast-export to export tags pointing to tags.\n\nMakes sense?\n\nAdditionally, to reduce the chance of similar bugs in the future, the\ncode could be refactored a bit to have a handle_commit()-function, so\nwhat goes on becomes a bit more obvious. Since this doesn't really\nchange any functionality, I guess it could be handed in as a separate\npatch.\n\n-- \nErik \"kusma\" Faye-Lund\nkusmabite@gmail.com\n(+47) 986 59 656\n"},{"id":"108894","messageId":"40aa078e0903220532n41895acemff175e4012e28ea6@mail.gmail.com","threadId":"18459","inReplyTo":"40aa078e0903220522g66cf2172l9f1a43ed562cc4d3@mail.gmail.com","subject":"Re: [PATCH] builtin-fast-export.c: add default case to avoid crash on corrupt repo","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2009-03-22T12:32:58Z","receivedAt":"2009-03-22T12:32:58Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"> tag-object is NULL. Now, I'm no expert, but from browsing some code,\n> it seems that \"parse_object(tag->object.sha1)\" should have been\n> performed on the tag before looking up the tagged object. Does this\n> make sense?\n\nI guess I forgot to mention that adding a call to parse_object() does\nmake fast-exporting nested tags appear to work.\n\n\n-- \nErik \"kusma\" Faye-Lund\nkusmabite@gmail.com\n(+47) 986 59 656\n"},{"id":"108937","messageId":"7vwsahcj8i.fsf@gitster.siamese.dyndns.org","threadId":"18459","inReplyTo":"40aa078e0903220522g66cf2172l9f1a43ed562cc4d3@mail.gmail.com","subject":"Re: [PATCH] builtin-fast-export.c: add default case to avoid crash on corrupt repo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-22T21:58:53Z","receivedAt":"2009-03-22T21:58:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Erik Faye-Lund writes:\n\n> Anyway, I guess this makes the most sense as a four-patch series:\n> 1) Add test-cases for tags of tag objects, tag objects of tag objects,\n> tags of trees and tag objects of trees.\n> 2) Turn the existing error into a warning\n> 3) Add the missing warning and remove the crash\n> 4) Fix fast-export to export tags pointing to tags.\n>\n> Makes sense?\n\nYes.\n\nI suspect we will hear an argument that say the dying is deliberate\nbecause pretending to have produced a faithful export while some\ninformation is lacking is bad and warnings are easy to miss, and I would\ncertainly understand that argument.\n\nWe have similar issue with signed tags and filter-branch, which strips the\nsignature when it rewrites the history. I recall the code originally died\nfor the same reasoning, but asking \"it died; what can the user do now?\"\nmade us realize that it is the best we can do to make best-effort\nreproduction with a warning when losing information.\n\nThis falls into the same category. I think \"we die by default if we\ncannot give a faithful reproduction, and the user needs to give an\nexplicit permission (e.g. --force option) to lose information\", is a very\nsensible thing to do, but \"we die if we cannot export some things, and we\nrefuse to produce an approximation.\" without an escape hatch is not.\n"},{"id":"108967","messageId":"20090323002636.GI23521@spearce.org","threadId":"18459","inReplyTo":"7vd4cabffl.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] builtin-fast-export.c: add default case to avoid crash on corrupt repo","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-03-23T00:26:36Z","receivedAt":"2009-03-23T00:26:36Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano wrote:\n> It\n> appears that there is no way to feed a tree object to fast-import, but I\n> think the fast-import language can represent a tag that points at another\n> tag just fine.\n\nCorrect.\n\nThis area of the langauge was built around the basic CVS->Git sort\nof conversion, where we just wanted to attach the CVS \"tag\" symbol\nonto a Git snapshot that seemed to match it. Annotated tags were\nused only because tools like git fetch and git describe prefer to\nwork with them, and these were (at some point in time) meaningful\nlabels for these revisions so the project post-conversion should\nretain those same labels.\n\nIn hindsight, parts of the fast-import langauge are problematic as\nthey don't fully represent the Git object graph, and this is one\nof those areas. It wasn't designed to represent the same range\nof structures as Git can represent.\n\n> So the best you can do is perhaps to issue a warning\n> \"skipping a tag that points at a tree object\" and impoement a proper\n> handling of a tag that points at a tag.\n\nOr, patch fast-import to expand its language. You could relax\nthe \"from \" rule to be \"from \" and then\ncreate tag a tree using a temporary branch and a from line of\n\"from temp-branch^{tree}\".\n\n-- \nShawn.\n"}]}