git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 0/6] better handling of gigantic config files

From
Taylor Blau <me@ttaylorr.com>
Date
Apr 13, 2020, 00:49 UTC
Message-ID
<20200413004927.GC55122@syl.local>
In-Reply-To
<20200410194211.GA1363484@coredump.intra.peff.net>
Hi Peff,
On Fri, Apr 10, 2020 at 03:42:11PM -0400, Jeff King wrote:
Show 41 quoted lines
> The fact that parse_config_key() requires its callers to use an "int"
> for a string length has bugged me for a while, and it re-bugged me when
> looking at it today. So I finally decided to do something about it,
> which led to an odyssey of other small fixes and cleanups.
>
> In particular, I was curious what kinds of bad behavior you could
> provoke by having a key name larger than 2GB (especially because we use
> the same parser for .gitmodules files, which might not be trusted). It
> turns out: basically none, because the config parser chokes immediately
> dues to its own int/size_t confusion.
>
> After patch 5, the config system _can_ actually handle stupidly-sized
> config keys, but in the end I decided to explicitly disallow them.
> There's downstream code that would be impossible to fix, and nobody
> actually cares about this case working anyway. See patch 6 for more
> discussion. I do still think the other patches are worth having as a
> cleanup; the more code that is safe from unexpected integer truncation
> the better.
>
>   [1/6]: remote: drop auto-strlen behavior of make_branch() and make_rewrite()
>   [2/6]: parse_config_key(): return subsection len as size_t
>   [3/6]: config: drop useless length variable in write_pair()
>   [4/6]: git_config_parse_key(): return baselen as size_t
>   [5/6]: config: use size_t to store parsed variable baselen
>   [6/6]: config: reject parsing of files over INT_MAX
>
>  archive-tar.c      |  4 ++--
>  builtin/help.c     |  2 +-
>  builtin/reflog.c   |  2 +-
>  config.c           | 42 +++++++++++++++++++++++++++++-------------
>  config.h           |  4 ++--
>  convert.c          |  2 +-
>  fsck.c             |  2 +-
>  ll-merge.c         |  2 +-
>  promisor-remote.c  |  2 +-
>  remote.c           | 37 +++++++++++++------------------------
>  submodule-config.c |  3 ++-
>  userdiff.c         |  4 ++--
>  12 files changed, 56 insertions(+), 50 deletions(-)
>
> -Peff

Thanks for doing this. I knew that it rang a bell for some reason, but it was because of the upload-pack changes to limit the set of allowed object filter choices that I'd sent as an RFC somewhere.

I was using 'parse_config_key()', and I think that you noted somewhere that it was odd that it filled an int and not a size_t. So, thanks very much for fixing that case.

This series looks very good and straightforward to me. So,
  Reviewed-by: Taylor Blau <me@ttaylorr.com>

Thanks, Taylor

Previous: Junio C HamanoNext: Jeff King
Message 14 of 16 in “better handling of gigantic config files”
  1. 0/6 better handling of gigantic config filesJeff King, Apr 10, 2020
  2. 1/6 remote: drop auto-strlen behavior of make_branch() and make_rewrite()Jeff King, Apr 10, 2020
  3. Junio C HamanoApr 10, 2020
  4. 2/6 parse_config_key(): return subsection len as size_tJeff King, Apr 10, 2020
  5. 3/6 config: drop useless length variable in write_pair()Jeff King, Apr 10, 2020
  6. Junio C HamanoApr 10, 2020
  7. 4/6 git_config_parse_key(): return baselen as size_tJeff King, Apr 10, 2020
  8. 5/6 config: use size_t to store parsed variable baselenJeff King, Apr 10, 2020
  9. 6/6 config: reject parsing of files over INT_MAXJeff King, Apr 10, 2020
  10. Junio C HamanoApr 10, 2020
  11. Jeff KingApr 10, 2020
  12. Taylor BlauApr 13, 2020
  13. Junio C HamanoApr 13, 2020
  14. Taylor BlauApr 13, 2020
  15. Jeff KingApr 13, 2020
  16. Taylor BlauApr 13, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.