Re: BUG: config.c:129: kvi should not be set while parsing a config source

From: Glen Choo <chooglen@google.com>
Date: Jun 23, 2023, 19:22 UTC
Message-ID: <kl6lh6qyrnjm.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <6961779f-9fd8-28cb-6046-cc24b6869cbc@github.com>

Thanks Jesse for the bug report, and thanks Stolee for looping me in!

Derrick Stolee <derrickstolee@github.com> writes:

Show 14 quoted lines

> On 6/21/2023 7:45 PM, Jesse Kasky wrote:
>> Thank you for filling out a Git bug report!
>> Please answer the following questions to help us understand your issue.
>> 
>> What did you do before the bug happened? (Steps to reproduce your issue)
>> 
>> git clone --no-checkout --sparse --filter=blob:none --depth=1 <repo> <dir>
>> cd <dir>
>> git sparse-checkout add <dir1> <dir2>
>> git fetch --depth=1 origin <commit>
>> Received the following error:
>> 
>> BUG: config.c:129: kvi should not be set while parsing a config source
>> [1]    5842 abort      /opt/homebrew/bin/git fetch --depth=1 origin

It took me some fiddling around to find a reproducing case, because, as it turns out, the choice of repo is important. This bug occurs when you have submodules + partial clone, _and_ .gitmodules has not been fetched yet (e.g. because --no-checkout skipped the fetch).

E.g. you can see this for yourself with:

  rm -fr test-breakage &&
  git clone --filter=blob:none --no-checkout https://github.com/git/git test-breakage &&
  cd test-breakage &&
  git fetch

This BUG() assertion is meant to guard against reading config in an unsafe nested way - the information about the config source we are reading (i.e. the key_value_info/kvi) is global state in the config machinery, and certain kinds of nesting will produce undefined behavior if we try to read it.

What happens is that "git fetch" tries to read .gitmodules, which triggers a partial clone fetch because .gitmodules is missing. But to do this, the partial clone code reads config to figure out where to fetch from, leading to a nested config-read-in-a-config-read.

For Git devs: the 'unsafe' thing is reading a config set while reading a config file/params/blob/etc. In this case, we are trying to read repo_config() (the config set in the_repository->config) while reading .gitmodules from a blob. Reading from files while reading files is safe (e.g. that's what we do when evaluating includes).

This should go away in my in-flight series:

  https://lore.kernel.org/git/pull.1497.v3.git.git.1687290231.gitgitgadget@gmail.com

because that removes all of the problematic global state. That's a very big refactor, so I'm not sure we should wait until that goes in to fix this bug. And while I'd like to fix this bug now, I don't see a point in making the existing code 'safer' by making small changes in the machinery that will just go away anyway.

So I think the best fix is for me to get rid of the BUG() check and go back to the 'unsafe' behavior. It won't matter in this specific scenario (since we don't actually need the config source information AFAICT), and at least it doesn't make _other_ use cases worse off than before.

Show 7 quoted lines

> I've come across this error while playing around with things in
> the config space, and the case I figured out was due to nested
> iterations over config.
>
> In my case, I was adding dynamic config loading when certain
> global variables were used, and some were used during config
> parsing causing this nesting.

Presumably this is how you were coming across this bug too: since your experiments added config set lookups, maybe some of them were happening in the middle of reading a config file.