list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <>
Subject: [PATCH 6/6] config: reject parsing of files over INT_MAX
Date: Fri, 10 Apr 2020 15:50:07 -0400	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

While the last few commits have made it possible for the config parser
to handle config files up to the limits of size_t, the rest of the code
isn't really ready for this. In particular, we often feed the keys as
strings into printf "%s" format specifiers. And because the printf
family of functions must return an int to specify the result, they
complain. Here are two concrete examples (using glibc; we're in
uncharted territory here so results may vary):

Generate a gigantic .gitmodules file like this:

   git submodule add /some/other/repo foo
           printf '[submodule "'
           perl -e 'print "a" x 2**31'
	   echo '"]path = foo'
   } >.gitmodules
   git commit -m 'huge gitmodule'

then try this:

   $ git show
   BUG: strbuf.c:397: your vsnprintf is broken (returned -1)

The problem is that we end up calling:

   strbuf_addf(&sb, "submodule.%s.ignore", submodule_name);

which relies on vsnprintf(), and that function has no way to report back
a size larger than INT_MAX.

Taking that same file, try this:

  git config --file=.gitmodules --list --name-only

On my system it produces an output with exactly 4GB of spaces. I
confirmed in a debugger that we reach the config callback with the key
intact: it's 2147483663 bytes and full of a's. But when we print it with
this call:

  printf("%s%c", key_, term);

we just get the spaces.

So given the fact that these are insane cases which we have no need to
support, the weird behavior from feeding the results to printf even if
the code is careful, and the possibility of uncareful code introducing
its own integer truncation issues, let's just declare INT_MAX as a limit
for parsing config files.

We'll enforce the limit in get_next_char(), which generalizes over all
sources (blobs, files, etc) and covers any element we're parsing
(whether section, key, value, etc). For simplicity, the limit is over
the length of the _whole_ file, so you couldn't have two 1GB values in
the same file. This should be perfectly fine, as the expected size for
config files is generally kilobytes at most.

With this patch both cases above will yield:

  fatal: bad config line 1 in file .gitmodules

That's not an amazing error message, but the parser isn't set up to
provide specific messages (it just breaks out of the parsing loop and
gives that generic error even if see a syntactic issue). And we really
wouldn't expect to see this case outside of somebody maliciously probing
the limits of the config system.

Signed-off-by: Jeff King <>
 config.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/config.c b/config.c
index 1c25c94863..8db9c77098 100644
--- a/config.c
+++ b/config.c
@@ -37,6 +37,7 @@ struct config_source {
 	enum config_error_action default_error_action;
 	int linenr;
 	int eof;
+	size_t total_len;
 	struct strbuf value;
 	struct strbuf var;
 	unsigned subsection_case_sensitive : 1;
@@ -524,6 +525,19 @@ static int get_next_char(void)
 			c = '\r';
+	if (c != EOF && ++cf->total_len > INT_MAX) {
+		/*
+		 * This is an absurdly long config file; refuse to parse
+		 * further in order to protect downstream code from integer
+		 * overflows. Note that we can't return an error specifically,
+		 * but we can mark EOF and put trash in the return value,
+		 * which will trigger a parse error.
+		 */
+		cf->eof = 1;
+		return 0;
+	}
 	if (c == '\n')
 	if (c == EOF) {
@@ -1540,6 +1554,7 @@ static int do_config_from(struct config_source *top, config_fn_t fn, void *data,
 	top->prev = cf;
 	top->linenr = 1;
 	top->eof = 0;
+	top->total_len = 0;
 	strbuf_init(&top->value, 1024);
 	strbuf_init(&top->var, 1024);
 	cf = top;

  parent reply	other threads:[~2020-04-10 19:50 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-10 19:42 [PATCH 0/6] better handling of gigantic config files Jeff King
2020-04-10 19:43 ` [PATCH 1/6] remote: drop auto-strlen behavior of make_branch() and make_rewrite() Jeff King
2020-04-10 21:44   ` Junio C Hamano
2020-04-10 19:44 ` [PATCH 2/6] parse_config_key(): return subsection len as size_t Jeff King
2020-04-10 19:44 ` [PATCH 3/6] config: drop useless length variable in write_pair() Jeff King
2020-04-10 21:51   ` Junio C Hamano
2020-04-10 19:46 ` [PATCH 4/6] git_config_parse_key(): return baselen as size_t Jeff King
2020-04-10 19:47 ` [PATCH 5/6] config: use size_t to store parsed variable baselen Jeff King
2020-04-10 19:50 ` Jeff King [this message]
2020-04-10 22:04   ` [PATCH 6/6] config: reject parsing of files over INT_MAX Junio C Hamano
2020-04-10 22:15     ` Jeff King
2020-04-13  0:47       ` Taylor Blau
2020-04-13 22:14         ` Junio C Hamano
2020-04-13  0:49 ` [PATCH 0/6] better handling of gigantic config files Taylor Blau
2020-04-13 17:20   ` Jeff King
2020-04-13 17:23     ` Taylor Blau

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:

  List information:

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

  git send-email \ \ \ \
    --subject='Re: [PATCH 6/6] config: reject parsing of files over INT_MAX' \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

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).