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

From: Taylor Blau <me@ttaylorr.com>
Date: Jul 31, 2024, 21:36 UTC
Message-ID: <ZqqucpNgqSgZDPtA@nand.local>
In-Reply-To: <CANrWfmTek1xErBLrnoyhHN+gWU+rw14y6SQ+abZyzGoaBjmiKA@mail.gmail.com>

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.

Show 15 quoted lines

> 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