git config set --file, key-value pair without '= value', gives Segmentation fault

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)

input: cd '/'; cd '/'; rm --force --recursive -- './test_git'; mkdir "$_"; cd "$_"; cat >'./config.txt' <<'EOF' [section] key #key = key = value1 key = value2 key = value3 key = value4 EOF git config set --file './config.txt' --value='[2]' --fixed-value 'section.key' 'value6'

output: Segmentation fault $?: 139

What did you expect to happen? (Expected behavior)

'./config.txt' has 'key = value6' appended to its end

What happened instead? (Actual behavior)

output: Segmentation fault $?: 139

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

Anything else you want to add:

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.46.0.windows.1
cpu: x86_64
built from commit: 2e6a859ffc0471f60f79c1256f766042b0d5d17d
sizeof-long: 4
sizeof-size_t: 8
shell-path: D:/git-sdk-64-build-installers/usr/bin/sh
feature: fsmonitor--daemon
libcurl: 8.9.0
OpenSSL: OpenSSL 3.2.2 4 Jun 2024
zlib: 1.3.1
uname: Windows 10.0 22631
compiler info: gnuc: 14.1
libc info: no libc information available
$SHELL (typically, interactive shell): C:\Program Files\Git\usr\bin\bash.exe

[Enabled Hooks] not run from a git repository - no hooks to show

On Wed, Jul 31, 2024 at 11:55:06PM +1200, Han Jiang wrote:

Thanks for the bugreport.

I was able to trim reproduction script slightly to the following:

--- 8< --- set -x

rm -f config.txt*

cat >'./config.txt' <<EOF [section] key EOF

git.compile config set --file ./config.txt --value='[2]' --fixed-value section.key value6 --- >8 ---

, which just relies on having --value, --fixed-value, and a key without an explicit value.

The script you provided bisects to 00bbdde141 (builtin/config: introduce "set" subcommand, 2024-05-06), which is the commit which introduced the new 'git config set' invocation.

But that appears to be a red herring, since the segfault happens in config.c::matches() here:

    (gdb) up
    #1  0x000055b3e8b06022 in matches (key=0x55b3ea894360 "section.key", value=0x0,
        store=0x7ffe99076eb0) at config.c:2884
    2884			return !strcmp(store->fixed_value, value);

where we are trying to compare the `--fixed-value` argument to `value`, which is NULL.

So I think that the behavior dates back to c90702a1f6 (config: plumb --fixed-value into config API, 2020-11-25). I think that the fix looks something like:

--- 8< ---

Hmph. fixed_value is about the string given on the command line being not a pattern. And value that is NULL is the valueless "true" in this case.

I would have actually expected the fix that follow your analysis (by the way, I found it really well done) would say something more like

	if (strcmp(key, store->key))
		return 0; /* not ours */
+	if (!value)
+		value = "true";
	if (store->fixed_value)
		return !strcmp(store->fixed_value, value);

but I am not sure exactly how we want to handle synonymous Boolean values here. Regardless of how "true" value is spelled in the configuration file, e.g.

        [section]               [section]
                key                     key = true

I wonder if "git config set --value=yes [--fixed-value] section.key false" should replace either of them to false.

It would especially true if the command is invoked with --type=bool but it seems that the --type option does not participate in the matching with the current value.

On Wed, Jul 31, 2024 at 03:47:04PM -0700, Junio C Hamano wrote:

I think it gets tricky when we talk about the implicit true value here for exactly that reason. Do we take true to mean just "true"? Or "true" and "yes", "1", "", and so on?

I tried to match the behavior a few lines down in that function, where we only call regexec() if value is non-NULL.

I think I could be convinced that --type=bool should treat "", "true", "1", "on", and so on as matching here, but I'm not sure.

Thanks, Taylor

'key' can be selected by `--value='!'` 'key =' can be selected by `--value='^$'` or `--value='' --fixed-value` commands for demonstration:

cd '/'; cd '/'; rm --force --recursive -- './test_git'; mkdir "$_"; cd "$_";
cat >'./config.txt' <<'EOF'
[section]
    key
    key =
    key = value1
    key = value2
EOF
git config set --file './config.txt' --value='^$' 'section.key' 'value3'
git config get --file './config.txt' --show-origin --show-scope --all
'section.key'
git config get --file './config.txt' --show-origin --show-scope 'section.key'

cat >'./config.txt' <<'EOF'
[section]
    key
    key =
    key = value1
    key = value2
EOF
git config set --file './config.txt' --value='^$' --fixed-value
'section.key' 'value4'
git config get --file './config.txt' --show-origin --show-scope --all
'section.key'
git config get --file './config.txt' --show-origin --show-scope 'section.key'

cat >'./config.txt' <<'EOF'
[section]
    key
    key =
    key = value1
    key = value2
EOF
git config set --file './config.txt' --value='' 'section.key' 'value5'
git config get --file './config.txt' --show-origin --show-scope --all
'section.key'
git config get --file './config.txt' --show-origin --show-scope 'section.key'

cat >'./config.txt' <<'EOF'
[section]
    key
    key =
    key = value1
    key = value2
EOF
git config set --file './config.txt' --value='' --fixed-value
'section.key' 'value6'
git config get --file './config.txt' --show-origin --show-scope --all
'section.key'
git config get --file './config.txt' --show-origin --show-scope 'section.key'

cat >'./config.txt' <<'EOF'
[section]
    key
    key =
    key = value1
    key = value2
EOF
git config set --file './config.txt' --value='!' 'section.key' 'value7'
git config get --file './config.txt' --show-origin --show-scope --all
'section.key'
git config get --file './config.txt' --show-origin --show-scope 'section.key'

It seems that `--regexp` and `--value=<regexp>` and `--fixed-value` take effect before `--default=<default>` which is before `--type=<type>`, so creating a special case (treating 'key' as 'key = true' if `--type=bool` is present in downstream process, and when `--default=<default>` is present, requiring users to remember which has higher priority or which takes effect first) would be too surprising. (Currently you can use `--value=abcd` to kick 'key' away then `--default=false` to turn 'key' into false, use `--value=abcd` to kick 'key=' away then `--default=true` to turn 'key=' into true.)

On Thu, Aug 1, 2024 at 11:36 AM Taylor Blau <me@ttaylorr.com> wrote:

Taylor Blau <me@ttaylorr.com> writes:

Yes, that is exactly what made me question the code. With the current code, valueless true cannot be updated with "--value" restriction, ever, because it will never match anything.

In any case, the patch is a strict improvement over the current code that segfaults. Making it aware of "--type=bool" and have it treat synonymous values of true/false properly would be better left as a follow-up topic.

Thanks.