git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: git@vger.kernel.org
Subject: [PATCH 5/6] config: use size_t to store parsed variable baselen
Date: Fri, 10 Apr 2020 15:47:51 -0400	[thread overview]
Message-ID: <20200410194751.GE1363756@coredump.intra.peff.net> (raw)
In-Reply-To: <20200410194211.GA1363484@coredump.intra.peff.net>

Most of the config parsing infrastructure is limited in what it can
parse only by the size of memory, because it parses character by
character, building up strbufs for keys, values, etc. One exception is
the "baselen" value we keep in git_parse_source(), which is an int.

That stores the length of the section.subsection base, to which we can
then append individual key names (by truncating back to the baselen with
strbuf_setlen(), and then appending characters for the key name). But
because it's an int, if we see an absurdly long section or subsection,
we may overflow the integer, wrapping negative. That negative value is
then implicitly cast to a size_t when we pass it to strbuf_setlen(),
creating a very large value and triggering a BUG. For example:

  $ {
       printf '[foo "'
       perl -e 'print "a" x 2**31'
       echo '"]bar = value'
    } >huge
  $ git config --file=huge --list
  fatal: BUG: strbuf_setlen() beyond buffer

While this is obviously a silly case that we don't care about
supporting, it's worth fixing it by switching to a size_t for a few
reasons:

  - we should try to avoid hitting BUG assertions at all

  - avoiding integer truncation or overflow sets a good example and
    makes it easier to audit the code for more important issues

  - the BUG outcome is what happens in _this_ instance, because we wrap
    negative. If we used a 2**32 subsection, we'd wrap to a small
    positive value and actually generate wrong output (the subsection of
    our key would be truncated).

Signed-off-by: Jeff King <peff@peff.net>
---
 config.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/config.c b/config.c
index c48bb35dc0..1c25c94863 100644
--- a/config.c
+++ b/config.c
@@ -729,7 +729,7 @@ static int git_parse_source(config_fn_t fn, void *data,
 			    const struct config_options *opts)
 {
 	int comment = 0;
-	int baselen = 0;
+	size_t baselen = 0;
 	struct strbuf *var = &cf->var;
 	int error_return = 0;
 	char *error_msg = NULL;
-- 
2.26.0.414.ge3a6455e3d


  parent reply	other threads:[~2020-04-10 19:47 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 ` Jeff King [this message]
2020-04-10 19:50 ` [PATCH 6/6] config: reject parsing of files over INT_MAX Jeff King
2020-04-10 22:04   ` 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:
  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=20200410194751.GE1363756@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --subject='Re: [PATCH 5/6] config: use size_t to store parsed variable baselen' \
    /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

Code repositories for project(s) associated with this 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).