git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: Ramsay Jones <ramsay@ramsayjones.plus.com>
Cc: Junio C Hamano <gitster@pobox.com>,
	Jason@zx2c4.com, GIT Mailing-list <git@vger.kernel.org>
Subject: Re: [PATCH 4/4] fsck: silence stderr when parsing .gitmodules
Date: Thu, 28 Jun 2018 18:12:44 -0400	[thread overview]
Message-ID: <20180628221244.GA11462@sigill.intra.peff.net> (raw)
In-Reply-To: <20180628220603.GD5524@sigill.intra.peff.net>

On Thu, Jun 28, 2018 at 06:06:03PM -0400, Jeff King wrote:

> Note that we didn't test this case at all, so I've added
> coverage in t7415. We may end up toning down or removing
> this fsck check in the future. So take this test as checking
> what happens now with a focus on stderr, and not any
> ironclad guarantee that we must detect and report parse
> failures in the future.

And such a patch _could_ look something like this. Though we could also
perhaps leave it in place and tone it down to "ignore" by default.

There's another case that triggers gitmodulesParse, too, which is a blob
so gigantic that we try not to hold it in memory. Technically that could
also happen due to somebody using .gitmodules for something unrelated.
But that seems even more far-fetched. And it _is_ dangerous to leave,
because I think existing vulnerable clients will try to load a 500MB
.gitmodules file in memory and parse it.

---
diff --git a/fsck.c b/fsck.c
index 87b0e228bd..296e8a8a7c 100644
--- a/fsck.c
+++ b/fsck.c
@@ -1013,11 +1013,9 @@ static int fsck_blob(struct blob *blob, const char *buf,
 	data.options = options;
 	data.ret = 0;
 	config_opts.error_action = CONFIG_ERROR_SILENT;
-	if (git_config_from_mem(fsck_gitmodules_fn, CONFIG_ORIGIN_BLOB,
-				".gitmodules", buf, size, &data, &config_opts))
-		data.ret |= report(options, &blob->object,
-				   FSCK_MSG_GITMODULES_PARSE,
-				   "could not parse gitmodules blob");
+	/* ignore errors */
+	git_config_from_mem(fsck_gitmodules_fn, CONFIG_ORIGIN_BLOB,
+			    ".gitmodules", buf, size, &data, &config_opts);
 
 	return data.ret;
 }
diff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh
index ba8af785a5..9a7dae88a5 100755
--- a/t/t7415-submodule-names.sh
+++ b/t/t7415-submodule-names.sh
@@ -176,7 +176,7 @@ test_expect_success 'fsck detects non-blob .gitmodules' '
 	)
 '
 
-test_expect_success 'fsck detects corrupt .gitmodules' '
+test_expect_success 'fsck does not complain about corrupt .gitmodules' '
 	git init corrupt &&
 	(
 		cd corrupt &&
@@ -185,9 +185,8 @@ test_expect_success 'fsck detects corrupt .gitmodules' '
 		git add .gitmodules &&
 		git commit -m "broken gitmodules" &&
 
-		test_must_fail git fsck 2>output &&
-		grep gitmodulesParse output &&
-		test_i18ngrep ! "bad config" output
+		git fsck 2>output &&
+		! test -s output
 	)
 '
 

  reply	other threads:[~2018-06-28 22:12 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-06-27 18:39 [PATCH] fsck: check skiplist for object in fsck_blob() Ramsay Jones
2018-06-28 11:49 ` Jeff King
2018-06-28 16:39   ` Junio C Hamano
2018-06-28 17:30     ` Jeff King
2018-06-28 16:56   ` Ramsay Jones
2018-06-28 17:28     ` Junio C Hamano
2018-06-28 17:45     ` Jeff King
2018-06-28 18:53       ` Ramsay Jones
2018-06-28 22:03         ` Jeff King
2018-06-28 22:05           ` [PATCH 1/4] config: turn die_on_error into caller-facing enum Jeff King
2018-06-28 22:05           ` [PATCH 2/4] config: add CONFIG_ERROR_SILENT handler Jeff King
2018-06-28 22:05           ` [PATCH 3/4] config: add options parameter to git_config_from_mem Jeff King
2018-06-28 22:06           ` [PATCH 4/4] fsck: silence stderr when parsing .gitmodules Jeff King
2018-06-28 22:12             ` Jeff King [this message]
2018-06-29  1:14               ` Ramsay Jones
2018-06-29  1:10           ` [PATCH] fsck: check skiplist for object in fsck_blob() Ramsay Jones
2018-07-03 14:34             ` Jeff King
2018-07-04  0:12               ` Ramsay Jones
2018-07-07  1:32                 ` Jeff King
2018-07-11 19:31                   ` Ramsay Jones
2018-07-13 19:37                     ` Ramsay Jones
2018-07-13 19:41                       ` Jeff King
2018-07-13 19:46                         ` Jeff King
2018-07-13 20:08                           ` Ramsay Jones
2018-07-13 19:38                     ` Jeff King
2018-07-13 19:39                       ` [PATCH 1/2] fsck: split ".gitmodules too large" error from parse failure Jeff King
2018-07-13 19:39                       ` [PATCH 2/2] fsck: downgrade gitmodulesParse default to "info" Jeff King
2018-07-13 20:21                         ` Stefan Beller
2018-07-16 18:04                         ` Junio C Hamano
2018-07-16 18:30                           ` Jeff King
2018-07-16 21:08                             ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180628221244.GA11462@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=Jason@zx2c4.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=ramsay@ramsayjones.plus.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).