{"thread":{"id":"20579","subject":"[BUG] Submodules problem with subdirectories and pushing","startedAt":"2009-08-13T10:32:31Z","lastAt":"2009-08-14T09:30:27Z","messageCount":9,"participants":["Frank Lichtenheld","Junio C Hamano","Martin Koegler"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"120508","messageId":"20090813103231.GY14475@mail-vs.djpig.de","threadId":"20579","inReplyTo":null,"subject":"[BUG] Submodules problem with subdirectories and pushing","fromName":"Frank Lichtenheld","fromEmail":"frank@lichtenheld.de","sentAt":"2009-08-13T10:32:31Z","receivedAt":"2009-08-13T10:32:31Z","isPatch":false,"sender":{"key":"frank@lichtenheld.de","avatar":"https://gravatar.com/avatar/b9f1d4b120e138f157c9e480d0818197c474628923786adb98f30017cdb99c3c?d=mp&s=160"},"body":"Hi.\n\nI have a git repository where I include several submodules. This seemed to\nwork fine until the server I push to got (finally) updated from 1.5.something\nto 1.6.4. Now I get an error if I try to push.\n\nThe issue is easily reproducible with a minimal repository for me:\n\nCreating an empty repository on server:\n\nflichtenheld@git-test:~$ git version\ngit version 1.6.4    <---- Directly compiled from git\nflichtenheld@git-test:~$ mkdir test.git\nflichtenheld@git-test:~$ cd test.git/\nflichtenheld@git-test:~/test.git$ git init --bare\nInitialized empty Git repository in /home/flichtenheld/test.git/\n\nCreating repository on client:\n\nfrl@dhcp-rnd-054:~/tmp$ git version\ngit version 1.6.3.3   <--- From Debian Package\nfrl@dhcp-rnd-054:~/tmp$ mkdir test\nfrl@dhcp-rnd-054:~/tmp$ cd test/\nfrl@dhcp-rnd-054:~/tmp/test$ git init\nInitialized empty Git repository in /home/frl/tmp/test/.git/\n\nAdd a random submodule:\n\nfrl@dhcp-rnd-054:~/tmp/test$ git submodule add git://repo.or.cz/git-browser.git subdir/git-browser\nInitialized empty Git repository in /home/frl/tmp/test/subdir/git-browser/.git/\nremote: Counting objects: 131, done.\nremote: Compressing objects: 100% (67/67), done.\nremote: Total 131 (delta 63), reused 131 (delta 63)\nReceiving objects: 100% (131/131), 105.98 KiB, done.\nResolving deltas: 100% (63/63), done.\nfrl@dhcp-rnd-054:~/tmp/test$ git commit -a\n[master (root-commit) 388b975] Add submodule\n 2 files changed, 4 insertions(+), 0 deletions(-)\n create mode 100644 .gitmodules\n create mode 160000 subdir/git-browser\n\nTry to push:\n\nfrl@dhcp-rnd-054:~/tmp/test$ git remote add origin ssh://gitadm/home/flichtenheld/test.git\nfrl@dhcp-rnd-054:~/tmp/test$ git push origin master\nCounting objects: 4, done.\nDelta compression using up to 4 threads.\nCompressing objects: 100% (3/3), done.\nWriting objects: 100% (4/4), 372 bytes, done.\nTotal 4 (delta 0), reused 0 (delta 0)\nfatal: Error on reachable objects of 9664402120f411181d05a2f51ee06a475fb73d9b\nerror: unpack-objects exited with error code 128\nerror: unpack failed: unpack-objects abnormal exit\nTo ssh://gitadm/home/flichtenheld/test.git\n ! [remote rejected] master -> master (n/a (unpacker error))\nerror: failed to push some refs to 'ssh://gitadm/home/flichtenheld/test.git'\nfrl@dhcp-rnd-054:~/tmp/test$ git show 9664402120f411181d05a2f51ee06a475fb73d9b\ntree 9664402120f411181d05a2f51ee06a475fb73d9b\n\ngit-browser\n\nAll seems to work fine if I add the submodule as git-browser instead of as subdir/git-browser.\n\nGruesse,\n-- \nFrank Lichtenheld <frank@lichtenheld.de>\nwww: http://www.djpig.de/\n"},{"id":"120509","messageId":"20090813111933.GZ14475@mail-vs.djpig.de","threadId":"20579","inReplyTo":"20090813103231.GY14475@mail-vs.djpig.de","subject":"Re: [BUG] Submodules problem with subdirectories and pushing","fromName":"Frank Lichtenheld","fromEmail":"frank@lichtenheld.de","sentAt":"2009-08-13T11:19:33Z","receivedAt":"2009-08-13T11:19:33Z","isPatch":false,"sender":{"key":"frank@lichtenheld.de","avatar":"https://gravatar.com/avatar/b9f1d4b120e138f157c9e480d0818197c474628923786adb98f30017cdb99c3c?d=mp&s=160"},"body":"On Thu, Aug 13, 2009 at 12:32:31PM +0200, Frank Lichtenheld wrote:\n> Hi.\n> \n> I have a git repository where I include several submodules. This seemed to\n> work fine until the server I push to got (finally) updated from 1.5.something\n> to 1.6.4. Now I get an error if I try to push.\n> \n> The issue is easily reproducible with a minimal repository for me:\n> \n> Creating an empty repository on server:\n> \n> flichtenheld@git-test:~$ git version\n> git version 1.6.4    <---- Directly compiled from git\n> flichtenheld@git-test:~$ mkdir test.git\n> flichtenheld@git-test:~$ cd test.git/\n> flichtenheld@git-test:~/test.git$ git init --bare\n> Initialized empty Git repository in /home/flichtenheld/test.git/\n\nHere is a \"git config receive.fsckObjects true\" missing. I have\nthis in my default config, and without it the error will not be\ntriggered.\n\nGruesse,\n-- \nFrank Lichtenheld <frank@lichtenheld.de>\nwww: http://www.djpig.de/\n"},{"id":"120565","messageId":"7vd46zbjae.fsf@alter.siamese.dyndns.org","threadId":"20579","inReplyTo":"20090813111933.GZ14475@mail-vs.djpig.de","subject":"[PATCH] Fix \"unpack-objects --strict\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-08-13T19:33:45Z","receivedAt":"2009-08-13T19:33:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When unpack-objects is run under the --strict option, objects that have\npointers to other objects are verified for the reachability at the end, by\ncalling check_object() on each of them, and letting check_object to walk\nthe reachable objects from them using fsck_walk() recursively.\n\nThe function however misunderstands the semantics of fsck_walk() function\nwhen it makes a call to it, setting itself as the callback.  fsck_walk()\nexpects the callback function to return a non-zero value to signal an\nerror (negative value causes an immediate abort, positive value is still\nan error but allows further checks on sibling objects) and return zero to\nsignal a success.  The function however returned 1 on some non error\ncases, and to cover up this mistake, complained only when fsck_walk() did\nnot detect any error.\n\nTo fix this double-bug, make the function return zero on all success\ncases, and also check for non-zero return from fsck_walk() for an error.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\nCaused by b41860b (unpack-objects: prevent writing of inconsistent\nobjects, 2008-02-25), which introduced these checks and also the code to\nkeep unverified objects in core until check_objects() verifies their\nreachability.  While I think it is a good idea to check for incomplete\npack data, I do not think it is necessary to keep them in core.  We can\nsimply error out to signal the caller not to update the refs.\n\nWe probably should write everything as they become unpackable (i.e. as\ntheir delta bases becomes available) while keeping track of object names\n(but not data) of structured objects that we received, and running only\none level of reachability check on them at the end.  That would certainly\nreduce the memory consumption and may simplify the complexity of the code\nat the same time.\n\nBut I'll leave that to other people.  Hint, hint...\n\n builtin-unpack-objects.c       |    8 ++++----\n t/t5531-deep-submodule-push.sh |   32 ++++++++++++++++++++++++++++++++\n 2 files changed, 36 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin-unpack-objects.c b/builtin-unpack-objects.c\nindex 557148a..109b7c8 100644\n--- a/builtin-unpack-objects.c\n+++ b/builtin-unpack-objects.c\n@@ -184,7 +184,7 @@ static int check_object(struct object *obj, int type, void *data)\n \t\treturn 0;\n \n \tif (obj->flags & FLAG_WRITTEN)\n-\t\treturn 1;\n+\t\treturn 0;\n \n \tif (type != OBJ_ANY && obj->type != type)\n \t\tdie(\"object type mismatch\");\n@@ -195,15 +195,15 @@ static int check_object(struct object *obj, int type, void *data)\n \t\tif (type != obj->type || type <= 0)\n \t\t\tdie(\"object of unexpected type\");\n \t\tobj->flags |= FLAG_WRITTEN;\n-\t\treturn 1;\n+\t\treturn 0;\n \t}\n \n \tif (fsck_object(obj, 1, fsck_error_function))\n \t\tdie(\"Error in object\");\n-\tif (!fsck_walk(obj, check_object, NULL))\n+\tif (fsck_walk(obj, check_object, NULL))\n \t\tdie(\"Error on reachable objects of %s\", sha1_to_hex(obj->sha1));\n \twrite_cached_object(obj);\n-\treturn 1;\n+\treturn 0;\n }\n \n static void write_rest(void)\ndiff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh\nnew file mode 100755\nindex 0000000..13b8e40\n--- /dev/null\n+++ b/t/t5531-deep-submodule-push.sh\n@@ -0,0 +1,32 @@\n+#!/bin/sh\n+\n+test_description='unpack-objects'\n+\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\tgit init --bare pub.git &&\n+\tGIT_DIR=pub.git git config receive.fsckobjects true &&\n+\tgit init work &&\n+\t(\n+\t\tcd work &&\n+\t\tgit init gar/bage &&\n+\t\t(\n+\t\t\tcd gar/bage &&\n+\t\t\t>junk &&\n+\t\t\tgit add junk &&\n+\t\t\tgit commit -m \"Initial junk\"\n+\t\t) &&\n+\t\tgit add gar/bage &&\n+\t\tgit commit -m \"Initial superproject\"\n+\t)\n+'\n+\n+test_expect_failure push '\n+\t(\n+\t\tcd work &&\n+\t\tgit push ../pub.git master\n+\t)\n+'\n+\n+test_done\n"},{"id":"120605","messageId":"20090814060307.GA31721@auto.tuwien.ac.at","threadId":"20579","inReplyTo":"7vd46zbjae.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Fix \"unpack-objects --strict\"","fromName":"Martin Koegler","fromEmail":"mkoegler@auto.tuwien.ac.at","sentAt":"2009-08-14T06:03:07Z","receivedAt":"2009-08-14T06:03:07Z","isPatch":true,"sender":{"key":"mkoegler@auto.tuwien.ac.at","avatar":null},"body":"On Thu, Aug 13, 2009 at 12:33:45PM -0700, Junio C Hamano wrote:\n> diff --git a/builtin-unpack-objects.c b/builtin-unpack-objects.c\n> index 557148a..109b7c8 100644\n> --- a/builtin-unpack-objects.c\n> +++ b/builtin-unpack-objects.c\n\nWhat about this check:\n> @@ -184,7 +184,7 @@ static int check_object(struct object *obj, int type, void *data)\n>       if (!obj)\n>  \t\treturn 0;\n\nThis is neccessary to skip already written objects (eg. blobs,\nobj_list[i].obj == NULL). The return code is not important in this\ncase.\n\nI'm not sure, if fsck_walk can call check_object with obj == NULL\nunder some (rare) conditions. If yes, the return code should be\nchanged to 1.\n\n> We probably should write everything as they become unpackable (i.e. as\n> their delta bases becomes available) while keeping track of object names\n> (but not data) of structured objects that we received, and running only\n> one level of reachability check on them at the end.  That would certainly\n> reduce the memory consumption and may simplify the complexity of the code\n> at the same time.\n\nThis would defeat the whole idea of the this check: If the\nprecondition (fsck returns OK for a repository) is met, unpack-objects\n(and index-pack) with --strict should gurantee, that this is still\ntrue after receiving the objects (even if somebody intentionally tries\nto corrupt the repository).\n\nAs we assume, that all objects (and all their ancestors) already\npresent in the repository are OK, we only have to check new objects\nand verify, that linked objects in the repository are of the correct\ntype.\n\nAs soon as we start to write objects without all linked objects\nalready present in the repository, the repository can get inconsistant.\n\nLets assume, unpack-objects/receive-pack is changed according to your proposal:\n* writeout all objects, as received\n* checking reachability of new objects\n* update refs, if everything is OK\n\nThen a corruption HOWTO would be:\n\nTo introduce a object with one of its linked objects missing, left it\nout of the pack and push it into the repository. unpack-objects will\nunpack all objects and fail updating the ref (but leave all objects in\nthe repository). As second step, simply send a ref update request,\nwhich should succed, as the object is present in the repository.\n\nA variant: set the SHA1 of the missing object to the SHA1 of a object\nof another type. In the second step, you can sent this object to\nunpack-objects, which will unpack it, as it does not know, that the\nobject is already referenced in the repository.\n\nDeleting objects in the case of an error is also not an option, as a\nparallel push operation could have already used the object.\n\nmfg Martin Köger\n"},{"id":"120609","messageId":"7vocqiucpw.fsf@alter.siamese.dyndns.org","threadId":"20579","inReplyTo":"20090814060307.GA31721@auto.tuwien.ac.at","subject":"Re: [PATCH] Fix \"unpack-objects --strict\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-08-14T06:32:59Z","receivedAt":"2009-08-14T06:32:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Koegler <mkoegler@auto.tuwien.ac.at> writes:\n\n> To introduce a object with one of its linked objects missing, left it\n> out of the pack and push it into the repository. unpack-objects will\n> unpack all objects and fail updating the ref (but leave all objects in\n> the repository). As second step, simply send a ref update request,\n> which should succed, as the object is present in the repository.\n\nYour \"ref update request\" exploit does not work because your understanding\nof how we decide to allow updating a ref is flawed.\n\nWe do not blindly update a ref to a commit only because we happen to have\nthat commit.  We require that commit to reach existing tips of refs\nwithout break.  The logic is in quickfetch() in builtin-fetch.c.\n\nThis stronger validation is necessary to deal with any failed transfer by\nhttp walkers, so it is nothing unusual nor new.  They walk from the latest\ncommits that exist on the other side, and their object transfer can be\ninterrupted before they transfer enough older commits to make the history\nconnected with what we already had.  In such a case we obviously do not\nupdate any ref.  And when we re-run the same request, we do not say \"Ah,\nwe have the tip in the object database, so we will update the ref to it\".\n\nThat is why I said requiring connectivity is a good idea, but keeping them\nin core is a misguided waste of memory.\n"},{"id":"120618","messageId":"20090814071949.GA2342@auto.tuwien.ac.at","threadId":"20579","inReplyTo":"7vocqiucpw.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Fix \"unpack-objects --strict\"","fromName":"Martin Koegler","fromEmail":"mkoegler@auto.tuwien.ac.at","sentAt":"2009-08-14T07:19:49Z","receivedAt":"2009-08-14T07:19:49Z","isPatch":true,"sender":{"key":"mkoegler@auto.tuwien.ac.at","avatar":null},"body":"On Thu, Aug 13, 2009 at 11:32:59PM -0700, Junio C Hamano wrote:\n> Martin Koegler <mkoegler@auto.tuwien.ac.at> writes:\n> > To introduce a object with one of its linked objects missing, left it\n> > out of the pack and push it into the repository. unpack-objects will\n> > unpack all objects and fail updating the ref (but leave all objects in\n> > the repository). As second step, simply send a ref update request,\n> > which should succed, as the object is present in the repository.\n> \n> Your \"ref update request\" exploit does not work because your understanding\n> of how we decide to allow updating a ref is flawed.\n> \n> We do not blindly update a ref to a commit only because we happen to have\n> that commit.  We require that commit to reach existing tips of refs\n> without break.  The logic is in quickfetch() in builtin-fetch.c.\n\nI'm talking on the server side of a push operation (receive-pack), not\nthe client side. The patchset should prevent invalid data from\nentering the repository, thereby preventing upload-pack (during further\nfetch operation) and other git programs (eg. called from gitweb) from\nfailing/segfaulting.\n\nmfg Martin Kögler\n"},{"id":"120621","messageId":"7vocqisvfk.fsf@alter.siamese.dyndns.org","threadId":"20579","inReplyTo":"20090814071949.GA2342@auto.tuwien.ac.at","subject":"Re: [PATCH] Fix \"unpack-objects --strict\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-08-14T07:31:43Z","receivedAt":"2009-08-14T07:31:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Koegler <mkoegler@auto.tuwien.ac.at> writes:\n\n> I'm talking on the server side of a push operation (receive-pack), not\n> the client side. The patchset should prevent invalid data from\n> entering the repository, thereby preventing upload-pack (during further\n> fetch operation) and other git programs (eg. called from gitweb) from\n> failing/segfaulting.\n\nupload-pack won't feed starting from a random object for a reason, so your\nworry is unfounded.\n\nI'd agree gitweb may be a problem.  Ideally it should restrict itself from\nanything reachable from the refs of he repository---patches welcome.\n"},{"id":"120622","messageId":"7vhbwasuz8.fsf@alter.siamese.dyndns.org","threadId":"20579","inReplyTo":"20090814060307.GA31721@auto.tuwien.ac.at","subject":"Re: [PATCH] Fix \"unpack-objects --strict\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-08-14T07:41:31Z","receivedAt":"2009-08-14T07:41:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Koegler <mkoegler@auto.tuwien.ac.at> writes:\n\n> What about this check:\n>> @@ -184,7 +184,7 @@ static int check_object(struct object *obj, int type, void *data)\n>>       if (!obj)\n>>  \t\treturn 0;\n>\n> This is neccessary to skip already written objects (eg. blobs,\n> obj_list[i].obj == NULL).\n\nYou can fix that issue by teaching write_rest() to check what it feeds\ncheck_object(), can't you?\n\n> I'm not sure, if fsck_walk can call check_object with obj == NULL\n> under some (rare) conditions. If yes, the return code should be\n> changed to 1.\n\nI think that is a sensible change to signal an error regardless.  For\nexample, fsck_walk_tree() will make a callback to you (meaning, walk()\nfunction pointer points at your check_object() function) like this:\n\n\twhile (tree_entry(&desc, &entry)) {\n\t\tint result;\n\n\t\tif (S_ISGITLINK(entry.mode))\n\t\t\tcontinue;\n\t\tif (S_ISDIR(entry.mode))\n\t\t\tresult = walk(&lookup_tree(entry.sha1)->object, OBJ_TREE, data);\n\nso while you are checking a tree object you received, upon hitting a\nsubtree of that tree, it will lookup_tree() it, and if that tree is\nmissing, you will be called with NULL.\n\nOn top of the previous patch, a fix would look like this, I think, but\nplease double check.\n\nThanks.\n\n builtin-unpack-objects.c |    8 +++++---\n 1 files changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin-unpack-objects.c b/builtin-unpack-objects.c\nindex 2522c2d..bae00ea 100644\n--- a/builtin-unpack-objects.c\n+++ b/builtin-unpack-objects.c\n@@ -181,7 +181,7 @@ static void write_cached_object(struct object *obj)\n static int check_object(struct object *obj, int type, void *data)\n {\n \tif (!obj)\n-\t\treturn 0;\n+\t\treturn 1;\n \n \tif (obj->flags & FLAG_WRITTEN)\n \t\treturn 0;\n@@ -209,8 +209,10 @@ static int check_object(struct object *obj, int type, void *data)\n static void write_rest(void)\n {\n \tunsigned i;\n-\tfor (i = 0; i < nr_objects; i++)\n-\t\tcheck_object(obj_list[i].obj, OBJ_ANY, 0);\n+\tfor (i = 0; i < nr_objects; i++) {\n+\t\tif (obj_list[i].obj)\n+\t\t\tcheck_object(obj_list[i].obj, OBJ_ANY, 0);\n+\t}\n }\n \n static void added_object(unsigned nr, enum object_type type,\n"},{"id":"120628","messageId":"20090814093027.GA14475@mail-vs.djpig.de","threadId":"20579","inReplyTo":"7vd46zbjae.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Fix \"unpack-objects --strict\"","fromName":"Frank Lichtenheld","fromEmail":"frank@lichtenheld.de","sentAt":"2009-08-14T09:30:27Z","receivedAt":"2009-08-14T09:30:27Z","isPatch":true,"sender":{"key":"frank@lichtenheld.de","avatar":"https://gravatar.com/avatar/b9f1d4b120e138f157c9e480d0818197c474628923786adb98f30017cdb99c3c?d=mp&s=160"},"body":"On Thu, Aug 13, 2009 at 12:33:45PM -0700, Junio C Hamano wrote:\n> When unpack-objects is run under the --strict option, objects that have\n> pointers to other objects are verified for the reachability at the end, by\n> calling check_object() on each of them, and letting check_object to walk\n> the reachable objects from them using fsck_walk() recursively.\n> \n> The function however misunderstands the semantics of fsck_walk() function\n> when it makes a call to it, setting itself as the callback.  fsck_walk()\n> expects the callback function to return a non-zero value to signal an\n> error (negative value causes an immediate abort, positive value is still\n> an error but allows further checks on sibling objects) and return zero to\n> signal a success.  The function however returned 1 on some non error\n> cases, and to cover up this mistake, complained only when fsck_walk() did\n> not detect any error.\n> \n> To fix this double-bug, make the function return zero on all success\n> cases, and also check for non-zero return from fsck_walk() for an error.\n\nI've applied this patch and your small follow-up patch here and the error\nindeed disappears. I can't comment on the semantical correctness of the\npatch.\n\nThanks,\n-- \nFrank Lichtenheld <frank@lichtenheld.de>\nwww: http://www.djpig.de/\n"}]}