{"thread":{"id":"48791","subject":"[PATCH] fsck: check skiplist for object in fsck_blob()","startedAt":"2018-06-27T18:40:00Z","lastAt":"2018-07-16T21:08:45Z","messageCount":31,"participants":["Ramsay Jones","Jeff King","Junio C Hamano","Stefan Beller"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"351149","messageId":"2fc2d53f-e193-2a2a-9f8f-b3e1d256d940@ramsayjones.plus.com","threadId":"48791","inReplyTo":null,"subject":"[PATCH] fsck: check skiplist for object in fsck_blob()","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2018-06-27T18:39:53Z","receivedAt":"2018-06-27T18:40:00Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nSince commit ed8b10f631 (\"fsck: check .gitmodules content\", 2018-05-02),\nfsck will issue an error message for '.gitmodules' content that cannot\nbe parsed correctly. This is the case, even when the corresponding blob\nobject has been included on the skiplist. For example, using the cgit\nrepository, we see the following:\n\n $ git fsck\n Checking object directories: 100% (256/256), done.\n error: bad config line 5 in blob .gitmodules\n error in blob 51dd1eff1edc663674df9ab85d2786a40f7ae3a5: gitmodulesParse: could not parse gitmodules blob\n Checking objects: 100% (6626/6626), done.\n $\n\n $ git config fsck.skiplist '.git/skip'\n $ echo 51dd1eff1edc663674df9ab85d2786a40f7ae3a5 >.git/skip\n $\n\n $ git fsck\n Checking object directories: 100% (256/256), done.\n error: bad config line 5 in blob .gitmodules\n Checking objects: 100% (6626/6626), done.\n $\n\nNote that the error message issued by the config parser is still\npresent, despite adding the object-id of the blob to the skiplist.\n\nOne solution would be to provide a means of suppressing the messages\nissued by the config parser. However, given that (logically) we are\nasking fsck to ignore this object, a simpler approach is to just not\ncall the config parser if the object is to be skipped. Add a check to\nthe 'fsck_blob()' processing function, to determine if the object is\non the skiplist and, if so, exit the function early.\n\nSigned-off-by: Ramsay Jones \n---\n\nHi Junio,\n\nI noticed recently that the 'cgit.git' repo was complaining when\ndoing an 'git fsck' ...\n\nNote that 'cgit' had a '.gitmodules' file and a 'submodule.sh'\nscript back in 2007, which had nothing to do with the current\ngit submodule facilities, ... ;-)\n\nViz:\n\n $ git show 51dd1eff1e\n # This file maps a submodule path to an url from where the submodule\n # can be obtained. The script \"submodules.sh\" finds the url in this file\n # when invoked with -i to clone the submodules.\n\n git git://git.kernel.org/pub/scm/git/git.git\n $ \n\nI just remembered that I had intended to review the name of the\nfunction that this patch introduces before sending, but totally\nforgot! :(\n\n[Hmm, 'to_be_skipped' -> object_to_be_skipped, object_on_skiplist,\netc., ... suggestions welcome!]\n\nATB,\nRamsay Jones\n\n\n fsck.c | 13 +++++++++++--\n 1 file changed, 11 insertions(+), 2 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 48e7e3686..702ceb629 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -281,6 +281,13 @@ static void append_msg_id(struct strbuf *sb, const char *msg_id)\n \tstrbuf_addstr(sb, \": \");\n }\n \n+static int to_be_skipped(struct fsck_options *opts, struct object *obj)\n+{\n+\tif (opts && opts->skiplist && obj)\n+\t\treturn oid_array_lookup(opts->skiplist, &obj->oid) >= 0;\n+\treturn 0;\n+}\n+\n __attribute__((format (printf, 4, 5)))\n static int report(struct fsck_options *options, struct object *object,\n \tenum fsck_msg_id id, const char *fmt, ...)\n@@ -292,8 +299,7 @@ static int report(struct fsck_options *options, struct object *object,\n \tif (msg_type == FSCK_IGNORE)\n \t\treturn 0;\n \n-\tif (options->skiplist && object &&\n-\t\t\toid_array_lookup(options->skiplist, &object->oid) >= 0)\n+\tif (to_be_skipped(options, object))\n \t\treturn 0;\n \n \tif (msg_type == FSCK_FATAL)\n@@ -963,6 +969,9 @@ static int fsck_blob(struct blob *blob, const char *buf,\n \t\treturn 0;\n \toidset_insert(&gitmodules_done, &blob->object.oid);\n \n+\tif (to_be_skipped(options, &blob->object))\n+\t\treturn 0;\n+\n \tif (!buf) {\n \t\t/*\n \t\t * A missing buffer here is a sign that the caller found the\n-- \n2.18.0\n"},{"id":"351200","messageId":"20180628114912.GA12901@sigill.intra.peff.net","threadId":"48791","inReplyTo":"2fc2d53f-e193-2a2a-9f8f-b3e1d256d940@ramsayjones.plus.com","subject":"Re: [PATCH] fsck: check skiplist for object in fsck_blob()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-28T11:49:12Z","receivedAt":"2018-06-28T11:49:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 27, 2018 at 07:39:53PM +0100, Ramsay Jones wrote:\n\n> Since commit ed8b10f631 (\"fsck: check .gitmodules content\", 2018-05-02),\n> fsck will issue an error message for '.gitmodules' content that cannot\n> be parsed correctly. This is the case, even when the corresponding blob\n> object has been included on the skiplist. For example, using the cgit\n> repository, we see the following:\n> \n> $ git fsck\n> Checking object directories: 100% (256/256), done.\n> error: bad config line 5 in blob .gitmodules\n> error in blob 51dd1eff1edc663674df9ab85d2786a40f7ae3a5: gitmodulesParse: could not parse gitmodules blob\n> Checking objects: 100% (6626/6626), done.\n> $\n> \n> $ git config fsck.skiplist '.git/skip'\n> $ echo 51dd1eff1edc663674df9ab85d2786a40f7ae3a5 >.git/skip\n> $\n> \n> $ git fsck\n> Checking object directories: 100% (256/256), done.\n> error: bad config line 5 in blob .gitmodules\n> Checking objects: 100% (6626/6626), done.\n> $\n> \n> Note that the error message issued by the config parser is still\n> present, despite adding the object-id of the blob to the skiplist.\n> \n> One solution would be to provide a means of suppressing the messages\n> issued by the config parser. However, given that (logically) we are\n> asking fsck to ignore this object, a simpler approach is to just not\n> call the config parser if the object is to be skipped. Add a check to\n> the 'fsck_blob()' processing function, to determine if the object is\n> on the skiplist and, if so, exit the function early.\n\nYeah, this solution seems sensible. Given that we would never report any\nerror for that blob, there is no point in even looking at it. I wonder\nif we ought to do the same for other types, too. Is there any point in\nopening a tree that is in the skiplist?\n\n> I noticed recently that the 'cgit.git' repo was complaining when\n> doing an 'git fsck' ...\n> \n> Note that 'cgit' had a '.gitmodules' file and a 'submodule.sh'\n> script back in 2007, which had nothing to do with the current\n> git submodule facilities, ... ;-)\n\nYikes. I worried about that sort of regression when adding the\n.gitmodules checks. But this _is_ a pretty unique case: somebody was\nimplementing their own version of submodules (pre-git-submodule) and\ndecided to use the same name. So I'm not sure if this is a sign that we\nneed to think through the regression, or a sign that it really is rare.\n\nOne thing we could do is turn the parse failure into a noop. The main\npoint of the check is to protect people against the malicious\n.gitmodules bug. If the file can't be parsed, then it also can't be an\nattack vector (assuming the parser used for the fsck check and the\nparser used by the victim behave identically).\n\nThat wouldn't help with your stray message, of course, but it would\neliminate the need to deal with the skiplist here (and skiplists aren't\nalways easy to do -- for instance, pushing up a non-fork of cgit to\nGitHub would now be rejected because of this old file, and you'd have to\ncontact support to resolve it).\n\n> I just remembered that I had intended to review the name of the\n> function that this patch introduces before sending, but totally\n> forgot! :(\n> \n> [Hmm, 'to_be_skipped' -> object_to_be_skipped, object_on_skiplist,\n> etc., ... suggestions welcome!]\n\nThe current name is OK to be, but object_on_skiplist() also seems quite\nobvious.\n\n-Peff\n"},{"id":"351234","messageId":"xmqqr2kqyies.fsf@gitster-ct.c.googlers.com","threadId":"48791","inReplyTo":"20180628114912.GA12901@sigill.intra.peff.net","subject":"Re: [PATCH] fsck: check skiplist for object in fsck_blob()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-28T16:39:39Z","receivedAt":"2018-06-28T16:40:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Yeah, this solution seems sensible. Given that we would never report any\n> error for that blob, there is no point in even looking at it. I wonder\n> if we ought to do the same for other types, too. Is there any point in\n> opening a tree that is in the skiplist?\n\nSuppose the tree is listed there only because it has one entry for a\nsubtree with leading 0 in its mode. We do want to ignore that\nformat violation, but we still want to learn the fact that the\nsubtree it points at and its contents are connected and not\ndangling, so we do need to open it. Is that open done in a separate\nphase?\n"},{"id":"351237","messageId":"0a18acbd-0124-1c92-0046-05b8b035dd28@ramsayjones.plus.com","threadId":"48791","inReplyTo":"20180628114912.GA12901@sigill.intra.peff.net","subject":"Re: [PATCH] fsck: check skiplist for object in fsck_blob()","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2018-06-28T16:56:18Z","receivedAt":"2018-06-28T16:56:26Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 28/06/18 12:49, Jeff King wrote:\n> On Wed, Jun 27, 2018 at 07:39:53PM +0100, Ramsay Jones wrote:\n> \n>> Since commit ed8b10f631 (\"fsck: check .gitmodules content\", 2018-05-02),\n>> fsck will issue an error message for '.gitmodules' content that cannot\n>> be parsed correctly. This is the case, even when the corresponding blob\n>> object has been included on the skiplist. For example, using the cgit\n>> repository, we see the following:\n>>\n>> $ git fsck\n>> Checking object directories: 100% (256/256), done.\n>> error: bad config line 5 in blob .gitmodules\n>> error in blob 51dd1eff1edc663674df9ab85d2786a40f7ae3a5: gitmodulesParse: could not parse gitmodules blob\n>> Checking objects: 100% (6626/6626), done.\n>> $\n>>\n>> $ git config fsck.skiplist '.git/skip'\n>> $ echo 51dd1eff1edc663674df9ab85d2786a40f7ae3a5 >.git/skip\n>> $\n>>\n>> $ git fsck\n>> Checking object directories: 100% (256/256), done.\n>> error: bad config line 5 in blob .gitmodules\n>> Checking objects: 100% (6626/6626), done.\n>> $\n>>\n>> Note that the error message issued by the config parser is still\n>> present, despite adding the object-id of the blob to the skiplist.\n>>\n>> One solution would be to provide a means of suppressing the messages\n>> issued by the config parser. However, given that (logically) we are\n>> asking fsck to ignore this object, a simpler approach is to just not\n>> call the config parser if the object is to be skipped. Add a check to\n>> the 'fsck_blob()' processing function, to determine if the object is\n>> on the skiplist and, if so, exit the function early.\n> \n> Yeah, this solution seems sensible. Given that we would never report any\n> error for that blob, there is no point in even looking at it. I wonder\n> if we ought to do the same for other types, too. Is there any point in\n> opening a tree that is in the skiplist?\n\nNote that the 'blob' object has already been 'loaded' at this\npoint anyway (and the basic 'object' checks have been done).\n\nI did think about this, briefly, but decided that we should\nonly 'skip' the leaf nodes (blobs). (So, when processing\ncommits, trees and tags, we should not report an error for\nthat object-id, but that should not stop us from checking\nthe tree object associated with a commit, just because of\na problem with the commit message).\n\n[Oh, wait - Hmm, each object could be checked independently\nof all others and not used for any object traversal right?\n(e.g. using packfile index). I saw fsck_walk() and didn't\nlook any further ... Ah, broken link check, ... I obviously\nneed to read the code some more ... :-D ]\n\n>> I noticed recently that the 'cgit.git' repo was complaining when\n>> doing an 'git fsck' ...\n>>\n>> Note that 'cgit' had a '.gitmodules' file and a 'submodule.sh'\n>> script back in 2007, which had nothing to do with the current\n>> git submodule facilities, ... ;-)\n> \n> Yikes. I worried about that sort of regression when adding the\n> .gitmodules checks. But this _is_ a pretty unique case: somebody was\n> implementing their own version of submodules (pre-git-submodule) and\n> decided to use the same name. So I'm not sure if this is a sign that we\n> need to think through the regression, or a sign that it really is rare.\n\nI don't have any numbers, but my gut tells me that this would\nbe very rare indeed. Of course, my gut has been wrong before ... :-D\n\n> One thing we could do is turn the parse failure into a noop. The main\n> point of the check is to protect people against the malicious\n> .gitmodules bug. If the file can't be parsed, then it also can't be an\n> attack vector (assuming the parser used for the fsck check and the\n> parser used by the victim behave identically).\n\nHmm, yeah, but I would have to provide a means of suppressing\nthe config parser error messages. Something I wanted to avoid. :(\n\n> That wouldn't help with your stray message, of course, but it would\n> eliminate the need to deal with the skiplist here (and skiplists aren't\n> always easy to do -- for instance, pushing up a non-fork of cgit to\n> GitHub would now be rejected because of this old file, and you'd have to\n> contact support to resolve it).\n\nGood point.\n\n>> I just remembered that I had intended to review the name of the\n>> function that this patch introduces before sending, but totally\n>> forgot! :(\n>>\n>> [Hmm, 'to_be_skipped' -> object_to_be_skipped, object_on_skiplist,\n>> etc., ... suggestions welcome!]\n> \n> The current name is OK to be, but object_on_skiplist() also seems quite\n> obvious.\n\nobject_on_skiplist() it is!\n\nJunio, do you want me to address the above 'rejected push'\nissue in this patch, or with a follow-up patch? (It should\nbe a pretty rare problem - famous last words!)\n\nATB,\nRamsay Jones\n\n\n\n"},{"id":"351245","messageId":"xmqq6022yg4k.fsf@gitster-ct.c.googlers.com","threadId":"48791","inReplyTo":"0a18acbd-0124-1c92-0046-05b8b035dd28@ramsayjones.plus.com","subject":"Re: [PATCH] fsck: check skiplist for object in fsck_blob()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-28T17:28:59Z","receivedAt":"2018-06-28T17:29:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones writes:\n\n> Junio, do you want me to address the above 'rejected push'\n> issue in this patch, or with a follow-up patch? (It should\n> be a pretty rare problem - famous last words!)\n\nIf you feel the need to say \"famous last words\", it is an indication\nthat it is better done as a follow-up, I would think ;-)\n\nThanks for spotting and addressing this issue.\n"},{"id":"351246","messageId":"20180628173014.GB31766@sigill.intra.peff.net","threadId":"48791","inReplyTo":"xmqqr2kqyies.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] fsck: check skiplist for object in fsck_blob()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-28T17:30:14Z","receivedAt":"2018-06-28T17:30:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 28, 2018 at 09:39:39AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Yeah, this solution seems sensible. Given that we would never report any\n> > error for that blob, there is no point in even looking at it. I wonder\n> > if we ought to do the same for other types, too. Is there any point in\n> > opening a tree that is in the skiplist?\n> \n> Suppose the tree is listed there only because it has one entry for a\n> subtree with leading 0 in its mode. We do want to ignore that\n> format violation, but we still want to learn the fact that the\n> subtree it points at and its contents are connected and not\n> dangling, so we do need to open it. Is that open done in a separate\n> phase?\n\nTo be honest, I'm not sure. There _is_ a separate phase for checking\nreachability, but I think there may be some dependencies between the\nphases. Once upon a time they were communicated by the existence of\nentries in obj_hash (blech!) but I think these days it happens using a\na bit in object->flags.\n\nThere is at least one case of interest just in this phase, though: we\nhave to read the surrounding tree to find out that a particular blob is\na .gitmodules file. So if you skiplist'd a tree, that would also mean we\nfail to mark its .gitmodules (if any) as such. I'm not sure if that\nwould be a bug or a feature, though.\n\n-Peff\n"},{"id":"351247","messageId":"20180628174501.GC31766@sigill.intra.peff.net","threadId":"48791","inReplyTo":"0a18acbd-0124-1c92-0046-05b8b035dd28@ramsayjones.plus.com","subject":"Re: [PATCH] fsck: check skiplist for object in fsck_blob()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-28T17:45:01Z","receivedAt":"2018-06-28T17:45:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 28, 2018 at 05:56:18PM +0100, Ramsay Jones wrote:\n\n> > Yeah, this solution seems sensible. Given that we would never report any\n> > error for that blob, there is no point in even looking at it. I wonder\n> > if we ought to do the same for other types, too. Is there any point in\n> > opening a tree that is in the skiplist?\n> \n> Note that the 'blob' object has already been 'loaded' at this\n> point anyway (and the basic 'object' checks have been done).\n\nYeah, you're right. If we wanted to avoid that, we should prevent it\nfrom entering the gitmodules_found list in the first place.\n\nOf course, we'd generally still load it once anyway in order to check\nthe sha1. So really, the most we could do is prevent it from being\nloaded a _second_ time for no reason during the fsck_finish() stage.\n\nBut for the reasons I gave in the fsck_finish() patches (like pack\nordering), we will quite often end up hitting it in the first pass\nanyway. So it's probably not worth spending too much time trying to\noptimize it.\n\nAnd thinking on that, my \"should we do this for trees\" is pretty dumb,\ntoo. We load them and compute their sha1 anyway (I _think_ bitrot checks\nlike that are totally independent of skiplist). So all we'd be saving is\nwalking the buffer entries.\n\n> I did think about this, briefly, but decided that we should\n> only 'skip' the leaf nodes (blobs). (So, when processing\n> commits, trees and tags, we should not report an error for\n> that object-id, but that should not stop us from checking\n> the tree object associated with a commit, just because of\n> a problem with the commit message).\n> \n> [Oh, wait - Hmm, each object could be checked independently\n> of all others and not used for any object traversal right?\n> (e.g. using packfile index). I saw fsck_walk() and didn't\n> look any further ... Ah, broken link check, ... I obviously\n> need to read the code some more ... :-D ]\n\nYes, I've been confused by this code before. I'm still not sure I\ntotally understand it. ;)\n\n> > One thing we could do is turn the parse failure into a noop. The main\n> > point of the check is to protect people against the malicious\n> > .gitmodules bug. If the file can't be parsed, then it also can't be an\n> > attack vector (assuming the parser used for the fsck check and the\n> > parser used by the victim behave identically).\n> \n> Hmm, yeah, but I would have to provide a means of suppressing\n> the config parser error messages. Something I wanted to avoid. :(\n\nYes, though I don't think it's too bad. We already have a \"die_on_error\"\nflag in the config code. I think it just needs to become a tristate:\ndie/error/silent (and probably get passed in via config_options, since I\nthink we tie it right now to the file/blob source).\n\n> Junio, do you want me to address the above 'rejected push'\n> issue in this patch, or with a follow-up patch? (It should\n> be a pretty rare problem - famous last words!)\n\nHmm, if we end up doing the config thing above, then this patch would\nbecome unnecessary.\n\nAnd I think doing that would help _all_ cases, even ones without a\nskiplist. They don't need to see random config error messages either,\neven if we do eventually report an fsck error.\n\n-Peff\n"},{"id":"351256","messageId":"db7683ab-1025-d7bb-d0ce-fc4ee28681e1@ramsayjones.plus.com","threadId":"48791","inReplyTo":"20180628174501.GC31766@sigill.intra.peff.net","subject":"Re: [PATCH] fsck: check skiplist for object in fsck_blob()","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2018-06-28T18:53:27Z","receivedAt":"2018-06-28T18:53:33Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 28/06/18 18:45, Jeff King wrote:\n> On Thu, Jun 28, 2018 at 05:56:18PM +0100, Ramsay Jones wrote:\n[snip]\n>>> One thing we could do is turn the parse failure into a noop. The main\n>>> point of the check is to protect people against the malicious\n>>> .gitmodules bug. If the file can't be parsed, then it also can't be an\n>>> attack vector (assuming the parser used for the fsck check and the\n>>> parser used by the victim behave identically).\n>>\n>> Hmm, yeah, but I would have to provide a means of suppressing\n>> the config parser error messages. Something I wanted to avoid. :(\n> \n> Yes, though I don't think it's too bad. We already have a \"die_on_error\"\n> flag in the config code. I think it just needs to become a tristate:\n> die/error/silent (and probably get passed in via config_options, since I\n> think we tie it right now to the file/blob source).\n\nYes, but this code is already very crufty - and I'm just\nwaiting for someone to want to have per-repo/submodule\nconfig parsing i... ;-)\n\n>> Junio, do you want me to address the above 'rejected push'\n>> issue in this patch, or with a follow-up patch? (It should\n>> be a pretty rare problem - famous last words!)\n> \n> Hmm, if we end up doing the config thing above, then this patch would\n> become unnecessary.\n\nI was thinking of timing - the current patch could go\nin quickly to solve the immediate problem (eg. in maint).\n\nAlso, it does not hurt to do this _as well as_ suppress\nthe config errors.\n\n> And I think doing that would help _all_ cases, even ones without a\n> skiplist. They don't need to see random config error messages either,\n> even if we do eventually report an fsck error.\n\nOh, yes, I agree. You will have noticed that it was my\nfirst suggested solution. (I have started writing that\npatch a few times, but it just makes me want to throw\nthe current code away and start again)!\n\nHmm, BTW, the 'rejected push' problem would include *any*\n'.gitmodules' blob that contained a syntax error, right?\n\nMaybe it won't be as rare as all that! (Imagine not being\nable to push due to a compiler error/warning in source files.\nHow irritating would that be! :-P ).\n\n(if we fix this, you could hide some nefarious settings\nafter an obvious syntax error - then get the victim to\nfix the syntax error ...)\n\nATB,\nRamsay Jones\n\n"},{"id":"351263","messageId":"20180628220332.GA5128@sigill.intra.peff.net","threadId":"48791","inReplyTo":"db7683ab-1025-d7bb-d0ce-fc4ee28681e1@ramsayjones.plus.com","subject":"Re: [PATCH] fsck: check skiplist for object in fsck_blob()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-28T22:03:33Z","receivedAt":"2018-06-28T22:03:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 28, 2018 at 07:53:27PM +0100, Ramsay Jones wrote:\n\n> > Yes, though I don't think it's too bad. We already have a \"die_on_error\"\n> > flag in the config code. I think it just needs to become a tristate:\n> > die/error/silent (and probably get passed in via config_options, since I\n> > think we tie it right now to the file/blob source).\n> \n> Yes, but this code is already very crufty - and I'm just\n> waiting for someone to want to have per-repo/submodule\n> config parsing i... ;-)\n\nIt is crufty, but I think we actually handle that part OK; the flag gets\nattached to the stack.\n\n> > Hmm, if we end up doing the config thing above, then this patch would\n> > become unnecessary.\n> \n> I was thinking of timing - the current patch could go\n> in quickly to solve the immediate problem (eg. in maint).\n> \n> Also, it does not hurt to do this _as well as_ suppress\n> the config errors.\n\nYes, it can go in quickly. But I'd prefer not to keep it in the long\nterm if it's literally doing nothing.\n\nI have some patches which I think solve your problem. They apply on\nv2.18.0, but not on v2.17.1 (because they rely on Dscho's increased\npassing of config_options in v2.18). Is that good enough?\n\n> > And I think doing that would help _all_ cases, even ones without a\n> > skiplist. They don't need to see random config error messages either,\n> > even if we do eventually report an fsck error.\n> \n> Oh, yes, I agree. You will have noticed that it was my\n> first suggested solution. (I have started writing that\n> patch a few times, but it just makes me want to throw\n> the current code away and start again)!\n> \n> Hmm, BTW, the 'rejected push' problem would include *any*\n> '.gitmodules' blob that contained a syntax error, right?\n> \n> Maybe it won't be as rare as all that! (Imagine not being\n> able to push due to a compiler error/warning in source files.\n> How irritating would that be! :-P ).\n\nYes, it would include any syntax error. I also have a slight worry about\nthat, but nobody seems to have screamed _yet_. :)\n\n> (if we fix this, you could hide some nefarious settings\n> after an obvious syntax error - then get the victim to\n> fix the syntax error ...)\n\nYou can also usually get the victim to type \"make\", which is even\nsimpler. :)\n\nHere are the patches I came up with.\n\nNote that the config_options struct has a bit of a dual-nature. Some\noptions are respected only via config_from_options(), and some only from\ngit_config_from_file(). I think this should probably be remedied in the\nlong run, but I stopped here in the interest of keeping this\nmaint-worthy.\n\n [1/4]: config: turn die_on_error into caller-facing enum\n [2/4]: config: add CONFIG_ERROR_SILENT handler\n [3/4]: config: add options parameter to git_config_from_mem\n [4/4]: fsck: silence stderr when parsing .gitmodules\n\n config.c | 32 +++++++++++++++++++++++---------\n config.h | 13 +++++++++++--\n fsck.c | 4 +++-\n submodule-config.c | 2 +-\n t/t7415-submodule-names.sh | 15 +++++++++++++++\n 5 files changed, 53 insertions(+), 13 deletions(-)\n\n-Peff\n"},{"id":"351264","messageId":"20180628220500.GA5524@sigill.intra.peff.net","threadId":"48791","inReplyTo":"20180628220332.GA5128@sigill.intra.peff.net","subject":"[PATCH 1/4] config: turn die_on_error into caller-facing enum","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-28T22:05:00Z","receivedAt":"2018-06-28T22:05:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The config code has a die_on_error flag, which lets us emit\nan error() instead of dying when we see a bogus config file.\nBut there's no way for a caller of the config code to set\nthis: it's auto-set based on whether we're reading a file or\na blob.\n\nInstead, let's add it to the config_options struct. When\nit's not set (or we have no options) we'll continue to fall\nback to the existing file/blob behavior.\n\nSigned-off-by: Jeff King \n---\n config.c | 18 +++++++++++++-----\n config.h | 5 +++++\n 2 files changed, 18 insertions(+), 5 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex a0a6ae1980..b5c23369d0 100644\n--- a/config.c\n+++ b/config.c\n@@ -31,7 +31,7 @@ struct config_source {\n \tenum config_origin_type origin_type;\n \tconst char *name;\n \tconst char *path;\n-\tint die_on_error;\n+\tenum config_error_action default_error_action;\n \tint linenr;\n \tint eof;\n \tstruct strbuf value;\n@@ -809,10 +809,18 @@ static int git_parse_source(config_fn_t fn, void *data,\n \t\t\t\t cf->linenr, cf->name);\n \t}\n \n-\tif (cf->die_on_error)\n+\tswitch (opts && opts->error_action ?\n+\t\topts->error_action :\n+\t\tcf->default_error_action) {\n+\tcase CONFIG_ERROR_DIE:\n \t\tdie(\"%s\", error_msg);\n-\telse\n+\t\tbreak;\n+\tcase CONFIG_ERROR_ERROR:\n \t\terror_return = error(\"%s\", error_msg);\n+\t\tbreak;\n+\tcase CONFIG_ERROR_UNSET:\n+\t\tBUG(\"config error action unset\");\n+\t}\n \n \tfree(error_msg);\n \treturn error_return;\n@@ -1520,7 +1528,7 @@ static int do_config_from_file(config_fn_t fn,\n \ttop.origin_type = origin_type;\n \ttop.name = name;\n \ttop.path = path;\n-\ttop.die_on_error = 1;\n+\ttop.default_error_action = CONFIG_ERROR_DIE;\n \ttop.do_fgetc = config_file_fgetc;\n \ttop.do_ungetc = config_file_ungetc;\n \ttop.do_ftell = config_file_ftell;\n@@ -1569,7 +1577,7 @@ int git_config_from_mem(config_fn_t fn, const enum config_origin_type origin_typ\n \ttop.origin_type = origin_type;\n \ttop.name = name;\n \ttop.path = NULL;\n-\ttop.die_on_error = 0;\n+\ttop.default_error_action = CONFIG_ERROR_ERROR;\n \ttop.do_fgetc = config_buf_fgetc;\n \ttop.do_ungetc = config_buf_ungetc;\n \ttop.do_ftell = config_buf_ftell;\ndiff --git a/config.h b/config.h\nindex 626d4654bd..ce75bf1e5c 100644\n--- a/config.h\n+++ b/config.h\n@@ -54,6 +54,11 @@ struct config_options {\n \tconst char *git_dir;\n \tconfig_parser_event_fn_t event_fn;\n \tvoid *event_fn_data;\n+\tenum config_error_action {\n+\t\tCONFIG_ERROR_UNSET = 0, /* use source-specific default */\n+\t\tCONFIG_ERROR_DIE, /* die() on error */\n+\t\tCONFIG_ERROR_ERROR, /* error() on error, return -1 */\n+\t} error_action;\n };\n \n typedef int (*config_fn_t)(const char *, const char *, void *);\n-- \n2.18.0.604.g8c4f59c959\n\n"},{"id":"351265","messageId":"20180628220509.GB5524@sigill.intra.peff.net","threadId":"48791","inReplyTo":"20180628220332.GA5128@sigill.intra.peff.net","subject":"[PATCH 2/4] config: add CONFIG_ERROR_SILENT handler","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-28T22:05:09Z","receivedAt":"2018-06-28T22:05:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We can currently die() or error(), but there's not yet any\nway for callers to ask us just to quietly return an error.\nLet's give them one.\n\nSigned-off-by: Jeff King \n---\n config.c | 3 +++\n config.h | 1 +\n 2 files changed, 4 insertions(+)\n\ndiff --git a/config.c b/config.c\nindex b5c23369d0..0797e284f4 100644\n--- a/config.c\n+++ b/config.c\n@@ -818,6 +818,9 @@ static int git_parse_source(config_fn_t fn, void *data,\n \tcase CONFIG_ERROR_ERROR:\n \t\terror_return = error(\"%s\", error_msg);\n \t\tbreak;\n+\tcase CONFIG_ERROR_SILENT:\n+\t\terror_return = -1;\n+\t\tbreak;\n \tcase CONFIG_ERROR_UNSET:\n \t\tBUG(\"config error action unset\");\n \t}\ndiff --git a/config.h b/config.h\nindex ce75bf1e5c..c02809ffdc 100644\n--- a/config.h\n+++ b/config.h\n@@ -58,6 +58,7 @@ struct config_options {\n \t\tCONFIG_ERROR_UNSET = 0, /* use source-specific default */\n \t\tCONFIG_ERROR_DIE, /* die() on error */\n \t\tCONFIG_ERROR_ERROR, /* error() on error, return -1 */\n+\t\tCONFIG_ERROR_SILENT, /* return -1 */\n \t} error_action;\n };\n \n-- \n2.18.0.604.g8c4f59c959\n\n"},{"id":"351266","messageId":"20180628220523.GC5524@sigill.intra.peff.net","threadId":"48791","inReplyTo":"20180628220332.GA5128@sigill.intra.peff.net","subject":"[PATCH 3/4] config: add options parameter to git_config_from_mem","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-28T22:05:24Z","receivedAt":"2018-06-28T22:05:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The underlying config parser knows how to handle a\nconfig_options struct, but git_config_from_mem() always\npasses NULL. Let's allow our callers to specify the options\nstruct.\n\nWe could add a \"_with_options\" variant, but since there are\nonly a handful of callers, let's just update them to pass\nNULL.\n\nSigned-off-by: Jeff King \n---\n config.c | 11 +++++++----\n config.h | 7 +++++--\n fsck.c | 2 +-\n submodule-config.c | 2 +-\n 4 files changed, 14 insertions(+), 8 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 0797e284f4..4548edb1e5 100644\n--- a/config.c\n+++ b/config.c\n@@ -1569,8 +1569,10 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)\n \treturn git_config_from_file_with_options(fn, filename, data, NULL);\n }\n \n-int git_config_from_mem(config_fn_t fn, const enum config_origin_type origin_type,\n-\t\t\tconst char *name, const char *buf, size_t len, void *data)\n+int git_config_from_mem(config_fn_t fn,\n+\t\t\tconst enum config_origin_type origin_type,\n+\t\t\tconst char *name, const char *buf, size_t len,\n+\t\t\tvoid *data, const struct config_options *opts)\n {\n \tstruct config_source top;\n \n@@ -1585,7 +1587,7 @@ int git_config_from_mem(config_fn_t fn, const enum config_origin_type origin_typ\n \ttop.do_ungetc = config_buf_ungetc;\n \ttop.do_ftell = config_buf_ftell;\n \n-\treturn do_config_from(&top, fn, data, NULL);\n+\treturn do_config_from(&top, fn, data, opts);\n }\n \n int git_config_from_blob_oid(config_fn_t fn,\n@@ -1606,7 +1608,8 @@ int git_config_from_blob_oid(config_fn_t fn,\n \t\treturn error(\"reference '%s' does not point to a blob\", name);\n \t}\n \n-\tret = git_config_from_mem(fn, CONFIG_ORIGIN_BLOB, name, buf, size, data);\n+\tret = git_config_from_mem(fn, CONFIG_ORIGIN_BLOB, name, buf, size,\n+\t\t\t\t data, NULL);\n \tfree(buf);\n \n \treturn ret;\ndiff --git a/config.h b/config.h\nindex c02809ffdc..f2063ceb86 100644\n--- a/config.h\n+++ b/config.h\n@@ -68,8 +68,11 @@ extern int git_config_from_file(config_fn_t fn, const char *, void *);\n extern int git_config_from_file_with_options(config_fn_t fn, const char *,\n \t\t\t\t\t void *,\n \t\t\t\t\t const struct config_options *);\n-extern int git_config_from_mem(config_fn_t fn, const enum config_origin_type,\n-\t\t\t\t\tconst char *name, const char *buf, size_t len, void *data);\n+extern int git_config_from_mem(config_fn_t fn,\n+\t\t\t const enum config_origin_type,\n+\t\t\t const char *name,\n+\t\t\t const char *buf, size_t len,\n+\t\t\t void *data, const struct config_options *opts);\n extern int git_config_from_blob_oid(config_fn_t fn, const char *name,\n \t\t\t\t const struct object_id *oid, void *data);\n extern void git_config_push_parameter(const char *text);\ndiff --git a/fsck.c b/fsck.c\nindex 0b8b20b6c4..aa7a52cc80 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -1012,7 +1012,7 @@ static int fsck_blob(struct blob *blob, const char *buf,\n \tdata.options = options;\n \tdata.ret = 0;\n \tif (git_config_from_mem(fsck_gitmodules_fn, CONFIG_ORIGIN_BLOB,\n-\t\t\t\t\".gitmodules\", buf, size, &data))\n+\t\t\t\t\".gitmodules\", buf, size, &data, NULL))\n \t\tdata.ret |= report(options, &blob->object,\n \t\t\t\t FSCK_MSG_GITMODULES_PARSE,\n \t\t\t\t \"could not parse gitmodules blob\");\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 388ef1f892..2ca3272dd1 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -561,7 +561,7 @@ static const struct submodule *config_from(struct submodule_cache *cache,\n \tparameter.gitmodules_oid = &oid;\n \tparameter.overwrite = 0;\n \tgit_config_from_mem(parse_config, CONFIG_ORIGIN_SUBMODULE_BLOB, rev.buf,\n-\t\t\tconfig, config_size, ¶meter);\n+\t\t\tconfig, config_size, ¶meter, NULL);\n \tstrbuf_release(&rev);\n \tfree(config);\n \n-- \n2.18.0.604.g8c4f59c959\n\n"},{"id":"351267","messageId":"20180628220603.GD5524@sigill.intra.peff.net","threadId":"48791","inReplyTo":"20180628220332.GA5128@sigill.intra.peff.net","subject":"[PATCH 4/4] fsck: silence stderr when parsing .gitmodules","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-28T22:06:04Z","receivedAt":"2018-06-28T22:06:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If there's a parsing error we'll already report it via the\nusual fsck report() function (or not, if the user has asked\nto skip this object or warning type). The error message from\nthe config parser just adds confusion. Let's suppress it.\n\nNote that we didn't test this case at all, so I've added\ncoverage in t7415. We may end up toning down or removing\nthis fsck check in the future. So take this test as checking\nwhat happens now with a focus on stderr, and not any\nironclad guarantee that we must detect and report parse\nfailures in the future.\n\nSigned-off-by: Jeff King \n---\n fsck.c | 4 +++-\n t/t7415-submodule-names.sh | 15 +++++++++++++++\n 2 files changed, 18 insertions(+), 1 deletion(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex aa7a52cc80..87b0e228bd 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -992,6 +992,7 @@ static int fsck_blob(struct blob *blob, const char *buf,\n \t\t unsigned long size, struct fsck_options *options)\n {\n \tstruct fsck_gitmodules_data data;\n+\tstruct config_options config_opts = { 0 };\n \n \tif (!oidset_contains(&gitmodules_found, &blob->object.oid))\n \t\treturn 0;\n@@ -1011,8 +1012,9 @@ static int fsck_blob(struct blob *blob, const char *buf,\n \tdata.obj = &blob->object;\n \tdata.options = options;\n \tdata.ret = 0;\n+\tconfig_opts.error_action = CONFIG_ERROR_SILENT;\n \tif (git_config_from_mem(fsck_gitmodules_fn, CONFIG_ORIGIN_BLOB,\n-\t\t\t\t\".gitmodules\", buf, size, &data, NULL))\n+\t\t\t\t\".gitmodules\", buf, size, &data, &config_opts))\n \t\tdata.ret |= report(options, &blob->object,\n \t\t\t\t FSCK_MSG_GITMODULES_PARSE,\n \t\t\t\t \"could not parse gitmodules blob\");\ndiff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh\nindex b68c5f5e85..ba8af785a5 100755\n--- a/t/t7415-submodule-names.sh\n+++ b/t/t7415-submodule-names.sh\n@@ -176,4 +176,19 @@ test_expect_success 'fsck detects non-blob .gitmodules' '\n \t)\n '\n \n+test_expect_success 'fsck detects corrupt .gitmodules' '\n+\tgit init corrupt &&\n+\t(\n+\t\tcd corrupt &&\n+\n+\t\techo \"[broken\" >.gitmodules &&\n+\t\tgit add .gitmodules &&\n+\t\tgit commit -m \"broken gitmodules\" &&\n+\n+\t\ttest_must_fail git fsck 2>output &&\n+\t\tgrep gitmodulesParse output &&\n+\t\ttest_i18ngrep ! \"bad config\" output\n+\t)\n+'\n+\n test_done\n-- \n2.18.0.604.g8c4f59c959\n"},{"id":"351268","messageId":"20180628221244.GA11462@sigill.intra.peff.net","threadId":"48791","inReplyTo":"20180628220603.GD5524@sigill.intra.peff.net","subject":"Re: [PATCH 4/4] fsck: silence stderr when parsing .gitmodules","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-28T22:12:44Z","receivedAt":"2018-06-28T22:12:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 28, 2018 at 06:06:03PM -0400, Jeff King wrote:\n\n> Note that we didn't test this case at all, so I've added\n> coverage in t7415. We may end up toning down or removing\n> this fsck check in the future. So take this test as checking\n> what happens now with a focus on stderr, and not any\n> ironclad guarantee that we must detect and report parse\n> failures in the future.\n\nAnd such a patch _could_ look something like this. Though we could also\nperhaps leave it in place and tone it down to \"ignore\" by default.\n\nThere's another case that triggers gitmodulesParse, too, which is a blob\nso gigantic that we try not to hold it in memory. Technically that could\nalso happen due to somebody using .gitmodules for something unrelated.\nBut that seems even more far-fetched. And it _is_ dangerous to leave,\nbecause I think existing vulnerable clients will try to load a 500MB\n.gitmodules file in memory and parse it.\n\n---\ndiff --git a/fsck.c b/fsck.c\nindex 87b0e228bd..296e8a8a7c 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -1013,11 +1013,9 @@ static int fsck_blob(struct blob *blob, const char *buf,\n \tdata.options = options;\n \tdata.ret = 0;\n \tconfig_opts.error_action = CONFIG_ERROR_SILENT;\n-\tif (git_config_from_mem(fsck_gitmodules_fn, CONFIG_ORIGIN_BLOB,\n-\t\t\t\t\".gitmodules\", buf, size, &data, &config_opts))\n-\t\tdata.ret |= report(options, &blob->object,\n-\t\t\t\t FSCK_MSG_GITMODULES_PARSE,\n-\t\t\t\t \"could not parse gitmodules blob\");\n+\t/* ignore errors */\n+\tgit_config_from_mem(fsck_gitmodules_fn, CONFIG_ORIGIN_BLOB,\n+\t\t\t \".gitmodules\", buf, size, &data, &config_opts);\n \n \treturn data.ret;\n }\ndiff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh\nindex ba8af785a5..9a7dae88a5 100755\n--- a/t/t7415-submodule-names.sh\n+++ b/t/t7415-submodule-names.sh\n@@ -176,7 +176,7 @@ test_expect_success 'fsck detects non-blob .gitmodules' '\n \t)\n '\n \n-test_expect_success 'fsck detects corrupt .gitmodules' '\n+test_expect_success 'fsck does not complain about corrupt .gitmodules' '\n \tgit init corrupt &&\n \t(\n \t\tcd corrupt &&\n@@ -185,9 +185,8 @@ test_expect_success 'fsck detects corrupt .gitmodules' '\n \t\tgit add .gitmodules &&\n \t\tgit commit -m \"broken gitmodules\" &&\n \n-\t\ttest_must_fail git fsck 2>output &&\n-\t\tgrep gitmodulesParse output &&\n-\t\ttest_i18ngrep ! \"bad config\" output\n+\t\tgit fsck 2>output &&\n+\t\t! test -s output\n \t)\n '\n \n"},{"id":"351288","messageId":"9162ed69-d245-8b2f-0dcc-3b345264b029@ramsayjones.plus.com","threadId":"48791","inReplyTo":"20180628220332.GA5128@sigill.intra.peff.net","subject":"Re: [PATCH] fsck: check skiplist for object in fsck_blob()","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2018-06-29T01:10:59Z","receivedAt":"2018-06-29T01:11:11Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 28/06/18 23:03, Jeff King wrote:\n> On Thu, Jun 28, 2018 at 07:53:27PM +0100, Ramsay Jones wrote:\n[snip]\n> Yes, it can go in quickly. But I'd prefer not to keep it in the long\n> term if it's literally doing nothing.\n\nHmm, I don't think you can say its doing nothing!\n\n \"Yeah, this solution seems sensible. Given that we would\n never report any error for that blob, there is no point\n in even looking at it.\"\n\n... is no less true, with or without additional patches! ;-)\n\n> I have some patches which I think solve your problem. They apply on\n> v2.18.0, but not on v2.17.1 (because they rely on Dscho's increased\n> passing of config_options in v2.18). Is that good enough?\n\nHeh, I was also writing patches to address this tonight (but\nI was also watching the football, so I was somewhat behind you).\nMy patches were not too dissimilar to yours, except I was aiming\nto allow even do_config_from_file() etc., to suppress errors.\n\nYour patches were cleaner and more focused than mine. (Instead of\nturning die_on_error into an enum, I added an additional 'quiet'\nflag. When pushing the stack (eg. for include files), I had to\ncopy the quiet flag from the parent struct, etc, ... ;-) ).\n\n> Yes, it would include any syntax error. I also have a slight worry about\n> that, but nobody seems to have screamed _yet_. :)\n\nHmm, I don't think we can ignore this. :(\n\n> Here are the patches I came up with.\n\nYes, I applied these locally and tested them. All OK here.\n\nSo, FWIW, Ack!\n\n[I still think my original patch, with the 'to_be_skipped'\nfunction name changed to 'object_on_skiplist', should be\nthe first patch of the series!]\n\nATB,\nRamsay Jones\n"},{"id":"351289","messageId":"0859e051-d78c-6d6f-17e4-c2c513341eeb@ramsayjones.plus.com","threadId":"48791","inReplyTo":"20180628221244.GA11462@sigill.intra.peff.net","subject":"Re: [PATCH 4/4] fsck: silence stderr when parsing .gitmodules","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2018-06-29T01:14:33Z","receivedAt":"2018-06-29T01:14:39Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 28/06/18 23:12, Jeff King wrote:\n> On Thu, Jun 28, 2018 at 06:06:03PM -0400, Jeff King wrote:\n> \n>> Note that we didn't test this case at all, so I've added\n>> coverage in t7415. We may end up toning down or removing\n>> this fsck check in the future. So take this test as checking\n>> what happens now with a focus on stderr, and not any\n>> ironclad guarantee that we must detect and report parse\n>> failures in the future.\n> \n> And such a patch _could_ look something like this. Though we could also\n> perhaps leave it in place and tone it down to \"ignore\" by default.\n> \n> There's another case that triggers gitmodulesParse, too, which is a blob\n> so gigantic that we try not to hold it in memory. Technically that could\n> also happen due to somebody using .gitmodules for something unrelated.\n> But that seems even more far-fetched. And it _is_ dangerous to leave,\n> because I think existing vulnerable clients will try to load a 500MB\n> .gitmodules file in memory and parse it.\n\nI also applied and tested the patch below. I think this patch\nmust be included in the series.\n\nATB,\nRamsay Jones\n\n> ---\n> diff --git a/fsck.c b/fsck.c\n> index 87b0e228bd..296e8a8a7c 100644\n> --- a/fsck.c\n> +++ b/fsck.c\n> @@ -1013,11 +1013,9 @@ static int fsck_blob(struct blob *blob, const char *buf,\n> \tdata.options = options;\n> \tdata.ret = 0;\n> \tconfig_opts.error_action = CONFIG_ERROR_SILENT;\n> -\tif (git_config_from_mem(fsck_gitmodules_fn, CONFIG_ORIGIN_BLOB,\n> -\t\t\t\t\".gitmodules\", buf, size, &data, &config_opts))\n> -\t\tdata.ret |= report(options, &blob->object,\n> -\t\t\t\t FSCK_MSG_GITMODULES_PARSE,\n> -\t\t\t\t \"could not parse gitmodules blob\");\n> +\t/* ignore errors */\n> +\tgit_config_from_mem(fsck_gitmodules_fn, CONFIG_ORIGIN_BLOB,\n> +\t\t\t \".gitmodules\", buf, size, &data, &config_opts);\n> \n> \treturn data.ret;\n> }\n> diff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh\n> index ba8af785a5..9a7dae88a5 100755\n> --- a/t/t7415-submodule-names.sh\n> +++ b/t/t7415-submodule-names.sh\n> @@ -176,7 +176,7 @@ test_expect_success 'fsck detects non-blob .gitmodules' '\n> \t)\n> '\n> \n> -test_expect_success 'fsck detects corrupt .gitmodules' '\n> +test_expect_success 'fsck does not complain about corrupt .gitmodules' '\n> \tgit init corrupt &&\n> \t(\n> \t\tcd corrupt &&\n> @@ -185,9 +185,8 @@ test_expect_success 'fsck detects corrupt .gitmodules' '\n> \t\tgit add .gitmodules &&\n> \t\tgit commit -m \"broken gitmodules\" &&\n> \n> -\t\ttest_must_fail git fsck 2>output &&\n> -\t\tgrep gitmodulesParse output &&\n> -\t\ttest_i18ngrep ! \"bad config\" output\n> +\t\tgit fsck 2>output &&\n> +\t\t! test -s output\n> \t)\n> '\n> \n> \n"},{"id":"351624","messageId":"20180703143416.GA23556@sigill.intra.peff.net","threadId":"48791","inReplyTo":"9162ed69-d245-8b2f-0dcc-3b345264b029@ramsayjones.plus.com","subject":"Re: [PATCH] fsck: check skiplist for object in fsck_blob()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-03T14:34:16Z","receivedAt":"2018-07-03T14:34:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 29, 2018 at 02:10:59AM +0100, Ramsay Jones wrote:\n\n> On 28/06/18 23:03, Jeff King wrote:\n> > On Thu, Jun 28, 2018 at 07:53:27PM +0100, Ramsay Jones wrote:\n> [snip]\n> > Yes, it can go in quickly. But I'd prefer not to keep it in the long\n> > term if it's literally doing nothing.\n> \n> Hmm, I don't think you can say its doing nothing!\n> \n> \"Yeah, this solution seems sensible. Given that we would\n> never report any error for that blob, there is no point\n> in even looking at it.\"\n> \n> ... is no less true, with or without additional patches! ;-)\n\nTrue that we don't even bother doing the parsing with your patch. But I\nthink I talked myself out of that part being a significant savings\nelsewhere.\n\nI guess it would be OK to leave it in. It just feels like it would be\nvestigial after the rest of the patches.\n\n> > I have some patches which I think solve your problem. They apply on\n> > v2.18.0, but not on v2.17.1 (because they rely on Dscho's increased\n> > passing of config_options in v2.18). Is that good enough?\n> \n> Heh, I was also writing patches to address this tonight (but\n> I was also watching the football, so I was somewhat behind you).\n> My patches were not too dissimilar to yours, except I was aiming\n> to allow even do_config_from_file() etc., to suppress errors.\n\nI think this should work via do_config_from_file(). The thing it really\nmisses is that git_config_with_options() will not respect it, but the\nhandling of options there is already a bug (well, I don't think there's\nanything triggerable either before or after my patches, but it feels\nlike a bug waiting to happen).\n\n> Your patches were cleaner and more focused than mine. (Instead of\n> turning die_on_error into an enum, I added an additional 'quiet'\n> flag. When pushing the stack (eg. for include files), I had to\n> copy the quiet flag from the parent struct, etc, ... ;-) ).\n\nYes, I think that's what you have to do pre-v2.18, where we don't pass\nthe options struct around.\n\n> > Yes, it would include any syntax error. I also have a slight worry about\n> > that, but nobody seems to have screamed _yet_. :)\n> \n> Hmm, I don't think we can ignore this. :(\n\nI'm not sure. This has been running on every push to GitHub for the past\n6 weeks, and this is the first report. It's hard to say what that means,\nand technically speaking of course this _is_ a regression.\n\nThere's a nearby thread of interest, too, which I cc'd you on:\n\n https://public-inbox.org/git/20180703070650.b3drk5a6kb4k4tnp@glandium.org/\n\n> > Here are the patches I came up with.\n> \n> Yes, I applied these locally and tested them. All OK here.\n> \n> So, FWIW, Ack!\n> \n> [I still think my original patch, with the 'to_be_skipped'\n> function name changed to 'object_on_skiplist', should be\n> the first patch of the series!]\n\nThanks. If we're going to do any loosening, I think we may want to\naddress that _first_, so it can go directly on top of the patches in\nv2.17.1 (because it's a bigger issue than the stray message, IMHO).\n\n-Peff\n"},{"id":"351676","messageId":"80fad203-8196-c4b6-ed9e-10def0890d59@ramsayjones.plus.com","threadId":"48791","inReplyTo":"20180703143416.GA23556@sigill.intra.peff.net","subject":"Re: [PATCH] fsck: check skiplist for object in fsck_blob()","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2018-07-04T00:12:40Z","receivedAt":"2018-07-04T00:12:50Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 03/07/18 15:34, Jeff King wrote:\n> On Fri, Jun 29, 2018 at 02:10:59AM +0100, Ramsay Jones wrote:\n> \n>> On 28/06/18 23:03, Jeff King wrote:\n>>> On Thu, Jun 28, 2018 at 07:53:27PM +0100, Ramsay Jones wrote:\n>> [snip]\n>>> Yes, it can go in quickly. But I'd prefer not to keep it in the long\n>>> term if it's literally doing nothing.\n>>\n>> Hmm, I don't think you can say its doing nothing!\n>>\n>> \"Yeah, this solution seems sensible. Given that we would\n>> never report any error for that blob, there is no point\n>> in even looking at it.\"\n>>\n>> ... is no less true, with or without additional patches! ;-)\n> \n> True that we don't even bother doing the parsing with your patch. But I\n> think I talked myself out of that part being a significant savings\n> elsewhere.\n\n[Sorry for the late reply - watching football again!]\n\nI'm not interested in any savings - it would have to be a pretty\nwacky repo for there to be much in the way of savings!\n\nSimply, I have found (for many different reasons) that, if there\nis no good reason to execute some code, it is _far_ better to not\ndo so! ;-)\n\n> I guess it would be OK to leave it in. It just feels like it would be\n> vestigial after the rest of the patches.\n> \n[snip]\n\n>>> Yes, it would include any syntax error. I also have a slight worry about\n>>> that, but nobody seems to have screamed _yet_. :)\n>>\n>> Hmm, I don't think we can ignore this. :(\n> \n> I'm not sure. This has been running on every push to GitHub for the past\n> 6 weeks, and this is the first report. It's hard to say what that means,\n> and technically speaking of course this _is_ a regression.\n\nHmm, are you concerned about old clients 'transmitting' the\nbad data via large hosting sites? (New clients would complain\nabout a dodgy .gitmodules file, no matter were it came from,\nright?)\n\nHas the definition of the config file syntax changed recently?\nIf not, then old client versions will see the same syntax errors\nas recently 'fixed' versions. So they should error out without\n'looking' at the bad data, right? (ignoring the 'lets fix the\nobvious syntax error' idea).\n\n> There's a nearby thread of interest, too, which I cc'd you on:\n> \n> https://public-inbox.org/git/20180703070650.b3drk5a6kb4k4tnp@glandium.org/\n\nYeah, I don't quite follow what's going on there - I would have\nto read up some more. ;-)\n\n>> So, FWIW, Ack!\n>>\n>> [I still think my original patch, with the 'to_be_skipped'\n>> function name changed to 'object_on_skiplist', should be\n>> the first patch of the series!]\n> \n> Thanks. If we're going to do any loosening, I think we may want to\n> address that _first_, so it can go directly on top of the patches in\n> v2.17.1 (because it's a bigger issue than the stray message, IMHO).\n\nAgreed. I probably haven't given it sufficient thought, but my\nimmediate reaction is to loosen the check - I don't see how\nthis could be exploited to significantly reduce security. (My lack\nof imagination has been noted several times in the past, however!)\n\nHaving said that, I am no security expert, so I will let those who\nhave security expertise decide what is best to do in this situation.\n\nThanks!\n\nATB,\nRamsay Jones\n\n\n\n\n"},{"id":"351835","messageId":"20180707013239.GA4687@sigill.intra.peff.net","threadId":"48791","inReplyTo":"80fad203-8196-c4b6-ed9e-10def0890d59@ramsayjones.plus.com","subject":"Re: [PATCH] fsck: check skiplist for object in fsck_blob()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-07T01:32:40Z","receivedAt":"2018-07-07T01:32:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 04, 2018 at 01:12:40AM +0100, Ramsay Jones wrote:\n\n> > True that we don't even bother doing the parsing with your patch. But I\n> > think I talked myself out of that part being a significant savings\n> > elsewhere.\n> \n> [Sorry for the late reply - watching football again!]\n> \n> I'm not interested in any savings - it would have to be a pretty\n> wacky repo for there to be much in the way of savings!\n> \n> Simply, I have found (for many different reasons) that, if there\n> is no good reason to execute some code, it is _far_ better to not\n> do so! ;-)\n\nHeh. I also agree with that as a guiding principle. But I _also_ like\nthe principle of \"if you do not need to do add this code, do not add\nit\". So the two are a little at odds here. :)\n\n> > I'm not sure. This has been running on every push to GitHub for the past\n> > 6 weeks, and this is the first report. It's hard to say what that means,\n> > and technically speaking of course this _is_ a regression.\n> \n> Hmm, are you concerned about old clients 'transmitting' the\n> bad data via large hosting sites? (New clients would complain\n> about a dodgy .gitmodules file, no matter were it came from,\n> right?)\n\nI just meant above that anybody with a broken .gitmodules would have had\ntheir push rejected, and we haven't gotten any such reports beyond yours\nand Mike's. So that's some evidence that it's relatively rare.\n\nAs far as why those fsck checks are there in the first place, yes, it's\nabout transmitting bad data to unpatched clients. Ultimately the\nresponsibility for not being vulnerable is on the clients themselves.\nBut in practice large hosting sites can help by not being vectors.\n\n> Has the definition of the config file syntax changed recently?\n> If not, then old client versions will see the same syntax errors\n> as recently 'fixed' versions. So they should error out without\n> 'looking' at the bad data, right? (ignoring the 'lets fix the\n> obvious syntax error' idea).\n\nNo, it hasn't change. So as far as I know, loosening the syntax check\ndoes not impact _this_ vulnerability, because it requires a file that\ncan be parsed, and the parser is the same for both cases. It might\naffect future vulnerabilities. We could also tighten when those come to\nlight, or tighten in a way that blocks the specific bug. But there's\npotentially some value in putting our foot down _now_ and saying that\nwe're going to enforce certain properties of special files, before it\ngets any further out-of-hand.\n\nAnd of course I could be wrong. We do occasionally fix bugs in the\nparser, so I won't be surprised if there's some obscure case where git\n<2.0 would not be protected or something like that. But frankly,\nanything that old is probably already vulnerable to other ancient bugs,\ntoo.\n\n> > Thanks. If we're going to do any loosening, I think we may want to\n> > address that _first_, so it can go directly on top of the patches in\n> > v2.17.1 (because it's a bigger issue than the stray message, IMHO).\n> \n> Agreed. I probably haven't given it sufficient thought, but my\n> immediate reaction is to loosen the check - I don't see how\n> this could be exploited to significantly reduce security. (My lack\n> of imagination has been noted several times in the past, however!)\n> \n> Having said that, I am no security expert, so I will let those who\n> have security expertise decide what is best to do in this situation.\n\nI think you have a grasp on the situation from what you wrote above.\n\nWhat next? I was kind of leaning towards loosening, but it sounded like\nJunio thought the opposite. One thing I didn't like about the patch I\nsent earlier is that it removes the option for the admin to say \"no, I\nreally do want to enforce this\". I don't know if that's of value or not.\n\nIn an ideal world, I think we'd detect the problem and then react\naccording to the receiver's receive.fsck.* config. And we could just\nflip the default for receive.fsck.gitmodulesParse to \"warning\". But\nIIRC, the fsck code in index-pack does not bother distinguishing between\nwarnings and errors. I think it ought to, but that's too big a change to\ngo on maint.\n\nIt _might_ work to just flip the default to \"ignore\". That leaves the\nparanoid admin with a way to re-enable it if they want, and I _think_ it\nwould be respected by index-pack.\n\n[goes and looks at the code]\n\nHmm, we seem to have \"info\" these days, so maybe that would do what I\nwant. I.e., I wonder if the patch below does everything we'd want. It's\nlate here and I probably won't get back to this until Monday, but you\nmay want to play with it in the meantime.\n\ndiff --git a/fsck.c b/fsck.c\nindex 48e7e36869..0b0003055e 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -61,7 +61,6 @@ static struct oidset gitmodules_done = OIDSET_INIT;\n \tFUNC(ZERO_PADDED_DATE, ERROR) \\\n \tFUNC(GITMODULES_MISSING, ERROR) \\\n \tFUNC(GITMODULES_BLOB, ERROR) \\\n-\tFUNC(GITMODULES_PARSE, ERROR) \\\n \tFUNC(GITMODULES_NAME, ERROR) \\\n \tFUNC(GITMODULES_SYMLINK, ERROR) \\\n \t/* warnings */ \\\n@@ -76,6 +75,7 @@ static struct oidset gitmodules_done = OIDSET_INIT;\n \tFUNC(NUL_IN_COMMIT, WARN) \\\n \t/* infos (reported as warnings, but ignored by default) */ \\\n \tFUNC(BAD_TAG_NAME, INFO) \\\n+\tFUNC(GITMODULES_PARSE, INFO) \\\n \tFUNC(MISSING_TAGGER_ENTRY, INFO)\n \n #define MSG_ID(id, msg_type) FSCK_MSG_##id,\n"},{"id":"352290","messageId":"2ad1b00c-70ff-c4b2-8cbc-9ef55c174221@ramsayjones.plus.com","threadId":"48791","inReplyTo":"20180707013239.GA4687@sigill.intra.peff.net","subject":"Re: [PATCH] fsck: check skiplist for object in fsck_blob()","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2018-07-11T19:31:34Z","receivedAt":"2018-07-11T19:31:42Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 07/07/18 02:32, Jeff King wrote:\n[snip]\n>> I'm not interested in any savings - it would have to be a pretty\n>> wacky repo for there to be much in the way of savings!\n>>\n>> Simply, I have found (for many different reasons) that, if there\n>> is no good reason to execute some code, it is _far_ better to not\n>> do so! ;-)\n> \n> Heh. I also agree with that as a guiding principle. But I _also_ like\n> the principle of \"if you do not need to do add this code, do not add\n> it\". So the two are a little at odds here. :)\n\nI agree with that also! ;-) However, in this case, I can't\nimagine having to do less, to do nothing - if you see what\nI mean! So, I think \"don't execute code you don't need to\"\ntrumps \"don't add code you don't need to\" here.\n\n[snip]\n> What next? I was kind of leaning towards loosening, but it sounded like\n> Junio thought the opposite. One thing I didn't like about the patch I\n> sent earlier is that it removes the option for the admin to say \"no, I\n> really do want to enforce this\". I don't know if that's of value or not.\n\nYes, it would be good to let the admin set the policy.\n\n> In an ideal world, I think we'd detect the problem and then react\n> according to the receiver's receive.fsck.* config. And we could just\n> flip the default for receive.fsck.gitmodulesParse to \"warning\". But\n> IIRC, the fsck code in index-pack does not bother distinguishing between\n> warnings and errors. I think it ought to, but that's too big a change to\n> go on maint.\n> \n> It _might_ work to just flip the default to \"ignore\". That leaves the\n> paranoid admin with a way to re-enable it if they want, and I _think_ it\n> would be respected by index-pack.\n\nAh, that would be good, if it works.\n\n> [goes and looks at the code]\n> \n> Hmm, we seem to have \"info\" these days, so maybe that would do what I\n> want. I.e., I wonder if the patch below does everything we'd want. It's\n> late here and I probably won't get back to this until Monday, but you\n> may want to play with it in the meantime.\n\nSorry, I've been busy with other things and have not had the\ntime to try the patch below (still trying to catch up with\nthe mailing-list emails!).\n\n> diff --git a/fsck.c b/fsck.c\n> index 48e7e36869..0b0003055e 100644\n> --- a/fsck.c\n> +++ b/fsck.c\n> @@ -61,7 +61,6 @@ static struct oidset gitmodules_done = OIDSET_INIT;\n> \tFUNC(ZERO_PADDED_DATE, ERROR) \\\n> \tFUNC(GITMODULES_MISSING, ERROR) \\\n> \tFUNC(GITMODULES_BLOB, ERROR) \\\n> -\tFUNC(GITMODULES_PARSE, ERROR) \\\n> \tFUNC(GITMODULES_NAME, ERROR) \\\n> \tFUNC(GITMODULES_SYMLINK, ERROR) \\\n> \t/* warnings */ \\\n> @@ -76,6 +75,7 @@ static struct oidset gitmodules_done = OIDSET_INIT;\n> \tFUNC(NUL_IN_COMMIT, WARN) \\\n> \t/* infos (reported as warnings, but ignored by default) */ \\\n> \tFUNC(BAD_TAG_NAME, INFO) \\\n> +\tFUNC(GITMODULES_PARSE, INFO) \\\n> \tFUNC(MISSING_TAGGER_ENTRY, INFO)\n> \n> #define MSG_ID(id, msg_type) FSCK_MSG_##id,\n> \n\nSo, just squinting at this in my email client, if this allowed\na push/fetch to succeed (along with an 'info' message), while\nproviding an admin the means to configure it to loudly deny\nthe push/fetch - then I think we have a winner! ;-)\n\nSorry for not testing the patch.\n\nATB,\nRamsay Jones\n\n"},{"id":"352522","messageId":"6b323eff-a0a6-d8d3-1e40-70af8299db5f@ramsayjones.plus.com","threadId":"48791","inReplyTo":"2ad1b00c-70ff-c4b2-8cbc-9ef55c174221@ramsayjones.plus.com","subject":"Re: [PATCH] fsck: check skiplist for object in fsck_blob()","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2018-07-13T19:37:46Z","receivedAt":"2018-07-13T19:37:52Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 11/07/18 20:31, Ramsay Jones wrote:\n> On 07/07/18 02:32, Jeff King wrote:\n[snip]\n>> Hmm, we seem to have \"info\" these days, so maybe that would do what I\n>> want. I.e., I wonder if the patch below does everything we'd want. It's\n>> late here and I probably won't get back to this until Monday, but you\n>> may want to play with it in the meantime.\n> \n> Sorry, I've been busy with other things and have not had the\n> time to try the patch below (still trying to catch up with\n> the mailing-list emails!).\n> \n>> diff --git a/fsck.c b/fsck.c\n>> index 48e7e36869..0b0003055e 100644\n>> --- a/fsck.c\n>> +++ b/fsck.c\n>> @@ -61,7 +61,6 @@ static struct oidset gitmodules_done = OIDSET_INIT;\n>> \tFUNC(ZERO_PADDED_DATE, ERROR) \\\n>> \tFUNC(GITMODULES_MISSING, ERROR) \\\n>> \tFUNC(GITMODULES_BLOB, ERROR) \\\n>> -\tFUNC(GITMODULES_PARSE, ERROR) \\\n>> \tFUNC(GITMODULES_NAME, ERROR) \\\n>> \tFUNC(GITMODULES_SYMLINK, ERROR) \\\n>> \t/* warnings */ \\\n>> @@ -76,6 +75,7 @@ static struct oidset gitmodules_done = OIDSET_INIT;\n>> \tFUNC(NUL_IN_COMMIT, WARN) \\\n>> \t/* infos (reported as warnings, but ignored by default) */ \\\n>> \tFUNC(BAD_TAG_NAME, INFO) \\\n>> +\tFUNC(GITMODULES_PARSE, INFO) \\\n>> \tFUNC(MISSING_TAGGER_ENTRY, INFO)\n>> \n>> #define MSG_ID(id, msg_type) FSCK_MSG_##id,\n>>\n> \n> So, just squinting at this in my email client, if this allowed\n> a push/fetch to succeed (along with an 'info' message), while\n> providing an admin the means to configure it to loudly deny\n> the push/fetch - then I think we have a winner! ;-)\n> \n> Sorry for not testing the patch.\n\nOK, so I found some time to test this tonight. It is not good\nnews (assuming that I haven't messed up the testing, of course). :(\n\nOn top of 'pu' (@9026cfc855), I reverted commit d4c5675233\n(\"fsck: silence stderr when parsing .gitmodules\", 2018-06-28) and\nadded the patch given below.\n\nUnfortunately, the final test fails, thus:\n\n $ cd t\n $ ./t7415-submodule-names.sh\n ok 1 - check names\n ok 2 - create innocent subrepo\n ok 3 - submodule add refuses invalid names\n ok 4 - add evil submodule\n ok 5 - add other submodule\n ok 6 - clone evil superproject\n ok 7 - fsck detects evil superproject\n ok 8 - transfer.fsckObjects detects evil superproject (unpack)\n ok 9 - transfer.fsckObjects detects evil superproject (index)\n ok 10 - create oddly ordered pack\n ok 11 - transfer.fsckObjects handles odd pack (unpack)\n ok 12 - transfer.fsckObjects handles odd pack (index)\n ok 13 - index-pack --strict works for non-repo pack\n ok 14 - fsck detects symlinked .gitmodules file\n ok 15 - fsck detects non-blob .gitmodules\n ok 16 - fsck detects corrupt .gitmodules\n ok 17 - push warns about corrupt .gitmodules\n not ok 18 - push rejects corrupt .gitmodules (policy)\n #\t\n #\t\trm -rf dst.git &&\n #\t\tgit init --bare dst.git &&\n #\t\tgit -C dst.git config transfer.fsckObjects true &&\n #\t\tgit -C dst.git config fsck.gitmodulesParse error &&\n #\t\ttest_must_fail git -C corrupt push ../dst.git HEAD 2>output &&\n #\t\tgrep gitmodulesParse output &&\n #\t\ttest_i18ngrep ! \"bad config\" output\n #\t\n # failed 1 among 18 test(s)\n 1..18\n $ \n\ni.e. the test_must_fail doesn't! ;-)\n\nI have to go out now, but I will hopefully take a look at\nthis again tomorrow. (Do the test additions look OK?)\n\nATB,\nRamsay Jones\n\n-- >8 --\nSubject: [PATCH] WIP: try jeff's last patch\n\nSigned-off-by: Ramsay Jones \n---\n fsck.c | 2 +-\n t/t7415-submodule-names.sh | 26 ++++++++++++++++++++++++++\n 2 files changed, 27 insertions(+), 1 deletion(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 17b3a51fa..b74856cee 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -64,7 +64,6 @@ static struct oidset gitmodules_done = OIDSET_INIT;\n \tFUNC(ZERO_PADDED_DATE, ERROR) \\\n \tFUNC(GITMODULES_MISSING, ERROR) \\\n \tFUNC(GITMODULES_BLOB, ERROR) \\\n-\tFUNC(GITMODULES_PARSE, ERROR) \\\n \tFUNC(GITMODULES_NAME, ERROR) \\\n \tFUNC(GITMODULES_SYMLINK, ERROR) \\\n \t/* warnings */ \\\n@@ -79,6 +78,7 @@ static struct oidset gitmodules_done = OIDSET_INIT;\n \tFUNC(NUL_IN_COMMIT, WARN) \\\n \t/* infos (reported as warnings, but ignored by default) */ \\\n \tFUNC(BAD_TAG_NAME, INFO) \\\n+\tFUNC(GITMODULES_PARSE, INFO) \\\n \tFUNC(MISSING_TAGGER_ENTRY, INFO)\n \n #define MSG_ID(id, msg_type) FSCK_MSG_##id,\ndiff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh\nindex ba8af785a..ef9535a44 100755\n--- a/t/t7415-submodule-names.sh\n+++ b/t/t7415-submodule-names.sh\n@@ -185,10 +185,36 @@ test_expect_success 'fsck detects corrupt .gitmodules' '\n \t\tgit add .gitmodules &&\n \t\tgit commit -m \"broken gitmodules\" &&\n \n+\t\t# issues an \"info\" warning, but does not fail\n+\t\tgit fsck 2>output &&\n+\t\tgrep gitmodulesParse output &&\n+\t\ttest_i18ngrep ! \"bad config\" output &&\n+\n+\t\t# up-rate gitmodulesParse to error\n+\t\tgit config fsck.gitmodulesParse error &&\n \t\ttest_must_fail git fsck 2>output &&\n \t\tgrep gitmodulesParse output &&\n \t\ttest_i18ngrep ! \"bad config\" output\n \t)\n '\n \n+test_expect_success 'push warns about corrupt .gitmodules' '\n+\trm -rf dst.git &&\n+\tgit init --bare dst.git &&\n+\tgit -C dst.git config transfer.fsckObjects true &&\n+\tgit -C corrupt push ../dst.git HEAD 2>output &&\n+\tgrep gitmodulesParse output &&\n+\ttest_i18ngrep ! \"bad config\" output\n+'\n+\n+test_expect_success 'push rejects corrupt .gitmodules (policy)' '\n+\trm -rf dst.git &&\n+\tgit init --bare dst.git &&\n+\tgit -C dst.git config transfer.fsckObjects true &&\n+\tgit -C dst.git config fsck.gitmodulesParse error &&\n+\ttest_must_fail git -C corrupt push ../dst.git HEAD 2>output &&\n+\tgrep gitmodulesParse output &&\n+\ttest_i18ngrep ! \"bad config\" output\n+'\n+\n test_done\n-- \n2.18.0\n"},{"id":"352523","messageId":"20180713193759.GB10354@sigill.intra.peff.net","threadId":"48791","inReplyTo":"2ad1b00c-70ff-c4b2-8cbc-9ef55c174221@ramsayjones.plus.com","subject":"Re: [PATCH] fsck: check skiplist for object in fsck_blob()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-13T19:38:00Z","receivedAt":"2018-07-13T19:38:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 11, 2018 at 08:31:34PM +0100, Ramsay Jones wrote:\n\n> >> Simply, I have found (for many different reasons) that, if there\n> >> is no good reason to execute some code, it is _far_ better to not\n> >> do so! ;-)\n> > \n> > Heh. I also agree with that as a guiding principle. But I _also_ like\n> > the principle of \"if you do not need to do add this code, do not add\n> > it\". So the two are a little at odds here. :)\n> \n> I agree with that also! ;-) However, in this case, I can't\n> imagine having to do less, to do nothing - if you see what\n> I mean! So, I think \"don't execute code you don't need to\"\n> trumps \"don't add code you don't need to\" here.\n\nFair enough. I'm OK with it either way, then.\n\n> > @@ -76,6 +75,7 @@ static struct oidset gitmodules_done = OIDSET_INIT;\n> > \tFUNC(NUL_IN_COMMIT, WARN) \\\n> > \t/* infos (reported as warnings, but ignored by default) */ \\\n> > \tFUNC(BAD_TAG_NAME, INFO) \\\n> > +\tFUNC(GITMODULES_PARSE, INFO) \\\n> > \tFUNC(MISSING_TAGGER_ENTRY, INFO)\n> > \n> > #define MSG_ID(id, msg_type) FSCK_MSG_##id,\n> > \n> \n> So, just squinting at this in my email client, if this allowed\n> a push/fetch to succeed (along with an 'info' message), while\n> providing an admin the means to configure it to loudly deny\n> the push/fetch - then I think we have a winner! ;-)\n> \n> Sorry for not testing the patch.\n\nNo problem. I didn't get back to it until today. And indeed, the patch\nworks as advertised, but there's one additional bit needed (in the\npreparatory patch below).\n\nSo here's what I came up with, which I think is pretty reasonable. The\ncommit message for the second one is quite long, but I tried to lay out\nthe pros and cons from our discussion. And I think what we discussed\nhere may end up being the blueprint for how we consider similar cases in\nthe future, so I tried to be exhaustive.\n\nI built these two on top of my earlier four patches (which Junio has\nqueued as jk/fsck-gitmodules-gently). The code change itself is\northogonal to silencing the config code, but I built on top of the test\nadded earlier. If we want to back-port this to v2.17 or earlier, I can\nbuild it the other way around.\n\nIf this is what we decide to do upstream, then I'll lobby to flip\nGitHub's defaults to match. Other hosters (especially ones using other\nimplementations) may consider doing the same.\n\n [1/2]: fsck: split \".gitmodules too large\" error from parse failure\n [2/2]: fsck: downgrade gitmodulesParse default to \"info\"\n\n fsck.c | 5 +++--\n t/t7415-submodule-names.sh | 2 +-\n 2 files changed, 4 insertions(+), 3 deletions(-)\n\n-Peff\n"},{"id":"352524","messageId":"20180713193952.GA12162@sigill.intra.peff.net","threadId":"48791","inReplyTo":"20180713193759.GB10354@sigill.intra.peff.net","subject":"[PATCH 1/2] fsck: split \".gitmodules too large\" error from parse failure","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-13T19:39:53Z","receivedAt":"2018-07-13T19:39:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since ed8b10f631 (fsck: check .gitmodules content,\n2018-05-02), we'll report a gitmodulesParse error for two\nconditions:\n\n - a .gitmodules entry is not syntactically valid\n\n - a .gitmodules entry is larger than core.bigFileThreshold\n\nwith the intent that we can detect malicious files and\nprotect downstream clients. E.g., from the issue in\n0383bbb901 (submodule-config: verify submodule names as\npaths, 2018-04-30).\n\nBut these conditions are actually quite different with\nrespect to that bug:\n\n - a syntactically invalid file cannot trigger the problem,\n as the victim would barf before hitting the problematic\n code\n\n - a too-big .gitmodules _can_ trigger the problem. Even\n though it is obviously silly to have a 500MB .gitmodules\n file, the submodule code will happily parse it if you\n have enough memory.\n\nSo it may be reasonable to configure their severity\nseparately. Let's add a new class for the \"too large\" case\nto allow that.\n\nSigned-off-by: Jeff King \n---\n fsck.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 87b0e228bd..4129935d86 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -63,6 +63,7 @@ static struct oidset gitmodules_done = OIDSET_INIT;\n \tFUNC(GITMODULES_MISSING, ERROR) \\\n \tFUNC(GITMODULES_BLOB, ERROR) \\\n \tFUNC(GITMODULES_PARSE, ERROR) \\\n+\tFUNC(GITMODULES_LARGE, ERROR) \\\n \tFUNC(GITMODULES_NAME, ERROR) \\\n \tFUNC(GITMODULES_SYMLINK, ERROR) \\\n \t/* warnings */ \\\n@@ -1005,7 +1006,7 @@ static int fsck_blob(struct blob *blob, const char *buf,\n \t\t * that an error.\n \t\t */\n \t\treturn report(options, &blob->object,\n-\t\t\t FSCK_MSG_GITMODULES_PARSE,\n+\t\t\t FSCK_MSG_GITMODULES_LARGE,\n \t\t\t \".gitmodules too large to parse\");\n \t}\n \n-- \n2.18.0.433.gb9621797ee\n\n"},{"id":"352525","messageId":"20180713193958.GB12162@sigill.intra.peff.net","threadId":"48791","inReplyTo":"20180713193759.GB10354@sigill.intra.peff.net","subject":"[PATCH 2/2] fsck: downgrade gitmodulesParse default to \"info\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-13T19:39:58Z","receivedAt":"2018-07-13T19:40:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We added an fsck check in ed8b10f631 (fsck: check\n.gitmodules content, 2018-05-02) as a defense against the\nvulnerability from 0383bbb901 (submodule-config: verify\nsubmodule names as paths, 2018-04-30). With the idea that\nup-to-date hosting sites could protect downstream unpatched\nclients that fetch from them.\n\nAs part of that defense, we reject any \".gitmodules\" entry\nthat is not syntactically valid. The theory is that if we\ncannot even parse the file, we cannot accurately check it\nfor vulnerabilities. And anybody with a broken .gitmodules\nfile would eventually want to know anyway.\n\nBut there are a few reasons this is a bad tradeoff in\npractice:\n\n - for this particular vulnerability, the client has to be\n able to parse the file. So you cannot sneak an attack\n through using a broken file, assuming the config parsers\n for the process running fsck and the eventual victim are\n functionally equivalent.\n\n - a broken .gitmodules file is not necessarily a problem.\n Our fsck check detects .gitmodules in _any_ tree, not\n just at the root. And the presence of a .gitmodules file\n does not necessarily mean it will be used; you'd have to\n also have gitlinks in the tree. The cgit repository, for\n example, has a file named .gitmodules from a\n pre-submodule attempt at sharing code, but does not\n actually have any gitlinks.\n\n - when the fsck check is used to reject a push, it's often\n hard to work around. The pusher may not have full control\n over the destination repository (e.g., if it's on a\n hosting server, they may need to contact the hosting\n site's support). And the broken .gitmodules may be too\n far back in history for rewriting to be feasible (again,\n this is an issue for cgit).\n\nSo we're being unnecessarily restrictive without actually\nimproving the security in a meaningful way. It would be more\nconvenient to downgrade this check to \"info\", which means\nwe'd still comment on it, but not reject a push. Site admins\ncan already do this via config, but we should ship sensible\ndefaults.\n\nThere are a few counterpoints to consider in favor of\nkeeping the check as an error:\n\n - the first point above assumes that the config parsers for\n the victim and the fsck process are equivalent. This is\n pretty true now, but as time goes on will become less so.\n Hosting sites are likely to upgrade their version of Git,\n whereas vulnerable clients will be stagnant (if they did\n upgrade, they'd cease to be vulnerable!). So in theory we\n may see drift over time between what two config parsers\n will accept.\n\n In practice, this is probably OK. The config format is\n pretty established at this point and shouldn't change a\n lot. And the farther we get from the announcement of the\n vulnerability, the less interesting this extra layer of\n protection becomes. I.e., it was _most_ valuable on day\n 0, when everybody's client was still vulnerable and\n hosting sites could protect people. But as time goes on\n and people upgrade, the population of vulnerable clients\n becomes smaller and smaller.\n\n - In theory this could protect us from other\n vulnerabilities in the future. E.g., .gitmodules are the\n only way for a malicious repository to feed data to the\n config parser, so this check could similarly protect\n clients from a future (to-be-found) bug there.\n\n But that's trading a hypothetical case for real-world\n pain today. If we do find such a bug, the hosting site\n would need to be updated to fix it, too. At which point\n we could figure out whether it's possible to detect\n _just_ the malicious case without hurting existing\n broken-but-not-evil cases.\n\n - Until recently, we hadn't made any restrictions on\n .gitmodules content. So now in tightening that we're\n hitting cases where certain things used to work, but\n don't anymore. There's some moderate pain now. But as\n time goes on, we'll see more (and more varied) cases that\n will make tightening harder in the future. So there's\n some argument for putting rules in place _now_, before\n users grow more cases that violate them.\n\n Again, this is trading pain now for hypothetical benefit\n in the future. And if we try hard in the future to keep\n our tightening to a minimum (i.e., rejecting true\n maliciousness without hurting broken-but-not-evil repos),\n then that reduces even the hypothetical benefit.\n\nConsidering both sets of arguments, it makes sense to loosen\nthis check for now.\n\nNote that we have to tweak the test in t7415 since fsck will\nno longer consider this a fatal error. But we still check\nthat it reports the warning, and that we don't get the\nspurious error from the config code.\n\nSigned-off-by: Jeff King \n---\n fsck.c | 2 +-\n t/t7415-submodule-names.sh | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 4129935d86..69ea8d5321 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -62,7 +62,6 @@ static struct oidset gitmodules_done = OIDSET_INIT;\n \tFUNC(ZERO_PADDED_DATE, ERROR) \\\n \tFUNC(GITMODULES_MISSING, ERROR) \\\n \tFUNC(GITMODULES_BLOB, ERROR) \\\n-\tFUNC(GITMODULES_PARSE, ERROR) \\\n \tFUNC(GITMODULES_LARGE, ERROR) \\\n \tFUNC(GITMODULES_NAME, ERROR) \\\n \tFUNC(GITMODULES_SYMLINK, ERROR) \\\n@@ -77,6 +76,7 @@ static struct oidset gitmodules_done = OIDSET_INIT;\n \tFUNC(ZERO_PADDED_FILEMODE, WARN) \\\n \tFUNC(NUL_IN_COMMIT, WARN) \\\n \t/* infos (reported as warnings, but ignored by default) */ \\\n+\tFUNC(GITMODULES_PARSE, INFO) \\\n \tFUNC(BAD_TAG_NAME, INFO) \\\n \tFUNC(MISSING_TAGGER_ENTRY, INFO)\n \ndiff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh\nindex ba8af785a5..293e2e1963 100755\n--- a/t/t7415-submodule-names.sh\n+++ b/t/t7415-submodule-names.sh\n@@ -185,7 +185,7 @@ test_expect_success 'fsck detects corrupt .gitmodules' '\n \t\tgit add .gitmodules &&\n \t\tgit commit -m \"broken gitmodules\" &&\n \n-\t\ttest_must_fail git fsck 2>output &&\n+\t\tgit fsck 2>output &&\n \t\tgrep gitmodulesParse output &&\n \t\ttest_i18ngrep ! \"bad config\" output\n \t)\n-- \n2.18.0.433.gb9621797ee\n"},{"id":"352527","messageId":"20180713194119.GC10354@sigill.intra.peff.net","threadId":"48791","inReplyTo":"6b323eff-a0a6-d8d3-1e40-70af8299db5f@ramsayjones.plus.com","subject":"Re: [PATCH] fsck: check skiplist for object in fsck_blob()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-13T19:41:19Z","receivedAt":"2018-07-13T19:41:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 13, 2018 at 08:37:46PM +0100, Ramsay Jones wrote:\n\n> OK, so I found some time to test this tonight. It is not good\n> news (assuming that I haven't messed up the testing, of course). :(\n\nI think you may have. :)\n\n> not ok 18 - push rejects corrupt .gitmodules (policy)\n> #\t\n> #\t\trm -rf dst.git &&\n> #\t\tgit init --bare dst.git &&\n> #\t\tgit -C dst.git config transfer.fsckObjects true &&\n> #\t\tgit -C dst.git config fsck.gitmodulesParse error &&\n> #\t\ttest_must_fail git -C corrupt push ../dst.git HEAD 2>output &&\n> #\t\tgrep gitmodulesParse output &&\n> #\t\ttest_i18ngrep ! \"bad config\" output\n\nThere are separate config slots for local fsck versus receiving objects.\nSo I think you need to be setting receive.fsck.gitmodulesParse.\n\n-Peff\n"},{"id":"352528","messageId":"20180713194609.GA16745@sigill.intra.peff.net","threadId":"48791","inReplyTo":"20180713194119.GC10354@sigill.intra.peff.net","subject":"Re: [PATCH] fsck: check skiplist for object in fsck_blob()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-13T19:46:10Z","receivedAt":"2018-07-13T19:46:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 13, 2018 at 03:41:19PM -0400, Jeff King wrote:\n\n> > not ok 18 - push rejects corrupt .gitmodules (policy)\n> > #\t\n> > #\t\trm -rf dst.git &&\n> > #\t\tgit init --bare dst.git &&\n> > #\t\tgit -C dst.git config transfer.fsckObjects true &&\n> > #\t\tgit -C dst.git config fsck.gitmodulesParse error &&\n> > #\t\ttest_must_fail git -C corrupt push ../dst.git HEAD 2>output &&\n> > #\t\tgrep gitmodulesParse output &&\n> > #\t\ttest_i18ngrep ! \"bad config\" output\n> \n> There are separate config slots for local fsck versus receiving objects.\n> So I think you need to be setting receive.fsck.gitmodulesParse.\n\nI confirmed that s/fsck/receive.fsck/ in your patch makes the tests\npass.\n\nI didn't bother adding extra push tests in the patch I just sent, since\nupgrading of config error classes is already covered elsewhere in t5504.\n\nThat said, I'm not opposed to adding more tests on top even if they are\nslightly redundant (well, not redundant if you're into black-box\ntesting, but our current tests are usually written with an assumption of\nwhere the module boundaries are, and what is likely to be a problem).\n\n-Peff\n"},{"id":"352530","messageId":"6e3ed7b0-105f-f527-65af-6fc217bc826d@ramsayjones.plus.com","threadId":"48791","inReplyTo":"20180713194609.GA16745@sigill.intra.peff.net","subject":"Re: [PATCH] fsck: check skiplist for object in fsck_blob()","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2018-07-13T20:08:02Z","receivedAt":"2018-07-13T20:08:06Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 13/07/18 20:46, Jeff King wrote:\n> On Fri, Jul 13, 2018 at 03:41:19PM -0400, Jeff King wrote:\n> \n>>> not ok 18 - push rejects corrupt .gitmodules (policy)\n>>> #\t\n>>> #\t\trm -rf dst.git &&\n>>> #\t\tgit init --bare dst.git &&\n>>> #\t\tgit -C dst.git config transfer.fsckObjects true &&\n>>> #\t\tgit -C dst.git config fsck.gitmodulesParse error &&\n>>> #\t\ttest_must_fail git -C corrupt push ../dst.git HEAD 2>output &&\n>>> #\t\tgrep gitmodulesParse output &&\n>>> #\t\ttest_i18ngrep ! \"bad config\" output\n>>\n>> There are separate config slots for local fsck versus receiving objects.\n>> So I think you need to be setting receive.fsck.gitmodulesParse.\n> \n> I confirmed that s/fsck/receive.fsck/ in your patch makes the tests\n> pass.\n\nDoh! Thanks for catching my stupid mistake! I was rushing a bit\njust before going out (yes, I'm going to be late now!).\n\n> I didn't bother adding extra push tests in the patch I just sent, since\n> upgrading of config error classes is already covered elsewhere in t5504.\n\nyeah, I like to 'test' by adding tests if I can (makes repeating\nthe steps less effort ...). So, I was just 'showing my working',\nas it were.\n\n> That said, I'm not opposed to adding more tests on top even if they are\n> slightly redundant (well, not redundant if you're into black-box\n> testing, but our current tests are usually written with an assumption of\n> where the module boundaries are, and what is likely to be a problem).\n\nI don't mind either way. I will let you and Junio decide.\n\nThanks!\n\nATB,\nRamsay Jones\n\n\n"},{"id":"352535","messageId":"CAGZ79kY1+tH0foab-pjWp4pW_gBxi5tOD_yYPsu03Q5A2oLUag@mail.gmail.com","threadId":"48791","inReplyTo":"20180713193958.GB12162@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] fsck: downgrade gitmodulesParse default to \"info\"","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-13T20:21:15Z","receivedAt":"2018-07-13T20:21:28Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> Considering both sets of arguments, it makes sense to loosen\n> this check for now.\n>\n\nI agree with this reasoning,\n\n>\n> Signed-off-by: Jeff King \n\nThis and the previous patch are\nReviewed-by: Stefan Beller \n"},{"id":"352639","messageId":"xmqqwotvt5uz.fsf@gitster-ct.c.googlers.com","threadId":"48791","inReplyTo":"20180713193958.GB12162@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] fsck: downgrade gitmodulesParse default to \"info\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-16T18:04:04Z","receivedAt":"2018-07-16T18:04:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> site's support). And the broken .gitmodules may be too\n> far back in history for rewriting to be feasible (again,\n> this is an issue for cgit).\n\n\"again\" but this is the first mention that hints cgit has some\nproblem (but not exactly what problem). Is that the \"cgit has a\nfile called .gitmodules that predates the submodule support on our\nside?\" thing?\n\n> So we're being unnecessarily restrictive without actually\n> improving the security in a meaningful way. It would be more\n> convenient to downgrade this check to \"info\", which means\n> we'd still comment on it, but not reject a push. Site admins\n> can already do this via config, but we should ship sensible\n> defaults.\n> ...\n> Considering both sets of arguments, it makes sense to loosen\n> this check for now.\n>\n> Note that we have to tweak the test in t7415 since fsck will\n> no longer consider this a fatal error. But we still check\n> that it reports the warning, and that we don't get the\n> spurious error from the config code.\n>\n> Signed-off-by: Jeff King \n> ---\n\nThanks.\n\n> fsck.c | 2 +-\n> t/t7415-submodule-names.sh | 2 +-\n> 2 files changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/fsck.c b/fsck.c\n> index 4129935d86..69ea8d5321 100644\n> --- a/fsck.c\n> +++ b/fsck.c\n> @@ -62,7 +62,6 @@ static struct oidset gitmodules_done = OIDSET_INIT;\n> \tFUNC(ZERO_PADDED_DATE, ERROR) \\\n> \tFUNC(GITMODULES_MISSING, ERROR) \\\n> \tFUNC(GITMODULES_BLOB, ERROR) \\\n> -\tFUNC(GITMODULES_PARSE, ERROR) \\\n> \tFUNC(GITMODULES_LARGE, ERROR) \\\n> \tFUNC(GITMODULES_NAME, ERROR) \\\n> \tFUNC(GITMODULES_SYMLINK, ERROR) \\\n> @@ -77,6 +76,7 @@ static struct oidset gitmodules_done = OIDSET_INIT;\n> \tFUNC(ZERO_PADDED_FILEMODE, WARN) \\\n> \tFUNC(NUL_IN_COMMIT, WARN) \\\n> \t/* infos (reported as warnings, but ignored by default) */ \\\n> +\tFUNC(GITMODULES_PARSE, INFO) \\\n> \tFUNC(BAD_TAG_NAME, INFO) \\\n> \tFUNC(MISSING_TAGGER_ENTRY, INFO)\n> \n> diff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh\n> index ba8af785a5..293e2e1963 100755\n> --- a/t/t7415-submodule-names.sh\n> +++ b/t/t7415-submodule-names.sh\n> @@ -185,7 +185,7 @@ test_expect_success 'fsck detects corrupt .gitmodules' '\n> \t\tgit add .gitmodules &&\n> \t\tgit commit -m \"broken gitmodules\" &&\n> \n> -\t\ttest_must_fail git fsck 2>output &&\n> +\t\tgit fsck 2>output &&\n> \t\tgrep gitmodulesParse output &&\n> \t\ttest_i18ngrep ! \"bad config\" output\n> \t)\n"},{"id":"352643","messageId":"20180716183033.GA22298@sigill.intra.peff.net","threadId":"48791","inReplyTo":"xmqqwotvt5uz.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/2] fsck: downgrade gitmodulesParse default to \"info\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-16T18:30:34Z","receivedAt":"2018-07-16T18:30:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 16, 2018 at 11:04:04AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > site's support). And the broken .gitmodules may be too\n> > far back in history for rewriting to be feasible (again,\n> > this is an issue for cgit).\n> \n> \"again\" but this is the first mention that hints cgit has some\n> problem (but not exactly what problem). Is that the \"cgit has a\n> file called .gitmodules that predates the submodule support on our\n> side?\" thing?\n\nI think you missed it. In the paragraph above the one you\nquoted, I said:\n\n The cgit repository, for example, has a file named\n .gitmodules from a pre-submodule attempt at sharing code,\n but does not actually have any gitlinks.\n\n> > So we're being unnecessarily restrictive without actually\n> > improving the security in a meaningful way. It would be more\n> > convenient to downgrade this check to \"info\", which means\n> > we'd still comment on it, but not reject a push. Site admins\n> > can already do this via config, but we should ship sensible\n> > defaults.\n> > ...\n> > Considering both sets of arguments, it makes sense to loosen\n> > this check for now.\n> >\n> > Note that we have to tweak the test in t7415 since fsck will\n> > no longer consider this a fatal error. But we still check\n> > that it reports the warning, and that we don't get the\n> > spurious error from the config code.\n> >\n> > Signed-off-by: Jeff King \n> > ---\n> \n> Thanks.\n\nSo I'm curious if you found the argument in my commit\nmessage compelling. :)\n\nMy recollection from the earlier discussion was that you\nwere more in favor of keeping things tight. E.g.,:\n\n https://public-inbox.org/git/xmqqh8lgrz5c.fsf@gitster-ct.c.googlers.com/\n\nbut reading it again:\n\n - there we were talking about non-blob objects as\n .gitmodules\n\n - I think your main concern was that there be a way for the\n user to loosen/tighten, which there is.\n\n-Peff\n"},{"id":"352680","messageId":"xmqqin5esxbb.fsf@gitster-ct.c.googlers.com","threadId":"48791","inReplyTo":"20180716183033.GA22298@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] fsck: downgrade gitmodulesParse default to \"info\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-16T21:08:40Z","receivedAt":"2018-07-16T21:08:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I think you missed it. In the paragraph above the one you\n> quoted, I said:\n>\n> The cgit repository, for example, has a file named\n> .gitmodules from a pre-submodule attempt at sharing code,\n> but does not actually have any gitlinks.\n\nIndeed.\n\n> So I'm curious if you found the argument in my commit\n> message compelling. :)\n> ...\n> - I think your main concern was that there be a way for the\n> user to loosen/tighten, which there is.\n\nYeah, I think the solution looks good.\n"}]}