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

[Taylor Blau <me@ttaylorr.com>]

Hi Peff,

On Fri, Apr 10, 2020 at 03:42:11PM -0400, Jeff King wrote:
> 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