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

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

[Jesse Kasky]

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>

What did you expect to happen? (Expected behavior)

Expected the fetch to complete

What happened instead? (Actual behavior)

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

What's different between what you expected and what actually happened?

Did not expect to encounter an abort.

Anything else you want to add:

Two items I came across while trying to troubleshoot:
https://github.com/git/git/commit/9828453ff00b330c57daa3a8b672cbb5f0cdce34
https://github.com/Homebrew/homebrew-cask-fonts/issues/7718

Please review the rest of the bug report below.
You can delete any lines you don't wish to share.


[System Info]
git version:
git version 2.41.0
cpu: arm64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
feature: fsmonitor--daemon
uname: Darwin 22.5.0 Darwin Kernel Version 22.5.0: Mon Apr 24 20:52:24
PDT 2023; root:xnu-8796.121.2~5/RELEASE_ARM64_T6000 arm64
compiler info: clang: 14.0.3 (clang-1403.0.22.14.1)
libc info: no libc information available
$SHELL (typically, interactive shell): /bin/zsh


[Enabled Hooks]

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

[Derrick Stolee]

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>

I'll note the oddity of using a blobless clone filter alongside a
shallow clone. This is something we are not super-likely to do
very often, so I could see how it is skipped by our test suite.

> cd <dir>
> git sparse-checkout add <dir1> <dir2>
> git fetch --depth=1 origin <commit>

Especially this fetch after the clone. Shallow clones are better to
only clone, then throw away. Don't fetch from them again. That said,
we shouldn't be hitting a BUG statement.

> 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

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.

That's where I would start investigating, but don't have time to
dig in more than to contribute my experience with this BUG
statement.

> Two items I came across while trying to troubleshoot:
> https://github.com/git/git/commit/9828453ff00b330c57daa3a8b672cbb5f0cdce34

I have CC'd Glen, the author of that commit, and an experienced
contributor in this space.

Thanks,
-Stolee

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

[Glen Choo]

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

Derrick Stolee <derrickstolee@github.com> writes:

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

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