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

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

[Han Jiang]

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

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

[Taylor Blau]

On Wed, Jul 31, 2024 at 11:55:06PM +1200, Han Jiang wrote:
> Thank you for filling out a Git bug report!

Thanks for the bugreport.

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

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< ---
diff --git a/config.c b/config.c
index 6421894614..05f369ec0d 100644
--- a/config.c
+++ b/config.c
@@ -2914,7 +2914,7 @@ static int matches(const char *key, const char *value,
 {
 	if (strcmp(key, store->key))
 		return 0; /* not ours */
-	if (store->fixed_value)
+	if (store->fixed_value && value)
 		return !strcmp(store->fixed_value, value);
 	if (!store->value_pattern)
 		return 1; /* always matches */
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 9de2d95f06..f13277c8f3 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -2704,6 +2704,15 @@ test_expect_success '--get and --get-all with --fixed-value' '
 	test_must_fail git config --file=config --get-regexp --fixed-value fixed+ non-existent
 '

+test_expect_success '--fixed-value with value-less configuration' '
+	test_when_finished rm -f config &&
+	cat >config <<-\EOF &&
+	[section]
+		key
+	EOF
+	git config --file=config --fixed-value section.key value pattern
+'
+
 test_expect_success 'includeIf.hasconfig:remote.*.url' '
 	git init hasremoteurlTest &&
 	test_when_finished "rm -rf hasremoteurlTest" &&
--- >8 ---

I'd like to hear from Stolee (CC'd), who is the author of c90702a1f6
before submitting this as a standalone patch.

Thanks,
Taylor

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

[Junio C Hamano]

> 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< ---
> diff --git a/config.c b/config.c
> index 6421894614..05f369ec0d 100644
> --- a/config.c
> +++ b/config.c
> @@ -2914,7 +2914,7 @@ static int matches(const char *key, const char *value,
>  {
>  	if (strcmp(key, store->key))
>  		return 0; /* not ours */
> -	if (store->fixed_value)
> +	if (store->fixed_value && value)
>  		return !strcmp(store->fixed_value, value);
>  	if (!store->value_pattern)
>  		return 1; /* always matches */

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.

> I'd like to hear from Stolee (CC'd), who is the author of c90702a1f6
> before submitting this as a standalone patch.

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

[Taylor Blau]

On Wed, Jul 31, 2024 at 03:47:04PM -0700, Junio C Hamano wrote:
> 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.

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.

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

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.

> > I'd like to hear from Stolee (CC'd), who is the author of c90702a1f6
> > before submitting this as a standalone patch.

Thanks,
Taylor

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

[Han Jiang]

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

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

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

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:
>
> On Wed, Jul 31, 2024 at 03:47:04PM -0700, Junio C Hamano wrote:
> > 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.
>
> 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.
>
> > 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.
>
> 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.
>
> > > I'd like to hear from Stolee (CC'd), who is the author of c90702a1f6
> > > before submitting this as a standalone patch.
>
> Thanks,
> Taylor

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

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

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

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.