From: Jeff King <peff@peff•net>
To: Han Jiang <jhcarl0814@gmail•com>
Cc: Junio C Hamano <gitster@pobox•com>,
Git Mailing List <git@vger•kernel.org>
Subject: Re: `git config get --type=path` results in segmentation fault on value starting with `:(optional)`
Date: Thu, 20 Nov 2025 02:50:19 -0500 [thread overview]
Message-ID: <20251120075019.GA1283645@coredump.intra.peff.net> (raw)
In-Reply-To: <CANrWfmQUuGKWPc6JCzeCaa9t98ag_Lyk0G_Prtd8YmqP-TiRpg@mail.gmail.com>
On Thu, Nov 20, 2025 at 07:46:42PM +1300, Han Jiang wrote:
> What did you do before the bug happened? (Steps to reproduce your issue)
> git -c 'section.key-path=/nonexistent' config get --show-origin
> --show-scope --all --type=path 'section.key-path'
> git -c 'section.key-path=:(optional)/nonexistent' config get
> --show-origin --show-scope --all --type=path 'section.key-path'
>
> What did you expect to happen? (Expected behavior)
>
> 1st command outputs "command command line: C:/Program Files/Git/nonexistent";
> 2nd command outputs nothing, $?=1;
>
> What happened instead? (Actual behavior)
>
> 1st command outputs "command command line: C:/Program Files/Git/nonexistent";
> 2nd command outputs "Segmentation fault", $?=139;
The issue is that git_config_pathname(), when it sees the ":(optional)"
marker, may return success (0) to the caller without actually setting
the "dest" parameter. So if we are lucky, we get a NULL and segfault,
but we may get any random data from the uninitialized pointer. Here's
another caller which exhibits similar problems:
$ git -c blame.ignorerevsfile=':(optional)foo' blame
double free or corruption (out)
Aborted git -c blame.ignorerevsfile=':(optional)foo' blame
This is all due to 749d6d166d (config: values of pathname type can be
prefixed with :(optional), 2025-09-28), which changed the contract for
git_config_pathname(). Before that patch, if the function returned 0,
then "dest" was guaranteed to point to a string. Now the caller must:
- set the dest parameter to some known value like NULL before the call
- after seeing success, check whether dest points to a string (if they
want to know whether we actually got a path).
This more or less[*] does the right thing when the dest points to a
static global, and we call it from a config callback. In that case the
destination is initialized to NULL, and anybody who looks at the
variables assumes that NULL means "it was never set at all". And that's
the case for commit.template, which is what the test from 749d6d166d
covers.
But many other callers are broken. E.g., blame.ignorerevsfile does this:
if (!strcmp(var, "blame.ignorerevsfile")) {
char *str;
int ret;
ret = git_config_pathname(&str, var, value);
if (ret)
return ret;
string_list_insert(&ignore_revs_file_list, str);
free(str);
return 0;
}
which tries to insert (and then free!) uninitialized bytes from "str".
Likewise git-config does:
} else if (opts->type == TYPE_PATH) {
char *v;
if (git_config_pathname(&v, key_, value_) < 0)
return -1;
strbuf_addstr(buf, v);
free((char *)v);
}[...]
Those (and some others) all need to be updated to the new semantics.
Something like this would fix the blame one:
diff --git a/builtin/blame.c b/builtin/blame.c
index 2703820258..15d719aec3 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -733,13 +733,14 @@ static int git_blame_config(const char *var, const char *value,
return 0;
}
if (!strcmp(var, "blame.ignorerevsfile")) {
- char *str;
+ char *str = NULL;
int ret;
ret = git_config_pathname(&str, var, value);
if (ret)
return ret;
- string_list_insert(&ignore_revs_file_list, str);
+ if (str)
+ string_list_insert(&ignore_revs_file_list, str);
free(str);
return 0;
}
I am tempted to say that git_config_pathname() should set the dest to
NULL itself in this case, but it is really only half the battle (callers
still need to check for NULL before looking at the value).
I am not sure about the git-config one, though. What should it print for
an optional path that is not there? The empty string? Is it an error?
I put a [*] above on "more or less does the right thing" because there's
another corner case, even for callers like commit.template. What should
this:
[commit]
template = :(optional)does-exist
template = :(optional)does-not-exist
With the current code, we will ignore the second config entry entirely,
and the result will point to "does-exist". But that feels surprising to
me. I'd expect the "optional" marker to set the value unconditionally,
but with an annotation that the entry does not need to exist. And that's
something only the caller can interpret (for commit.template, it means
setting it back to NULL, but for blame.ignorerevsfile, it means skipping
the string list insertion when it's not there).
I kind of wonder if git_config_pathname() ought to be returning more
data to the caller, like:
struct config_pathname {
char *path; /* never NULL */
unsigned missing : 1;
};
That would change the interface of git_config_pathname(), but that would
also force us to make the appropriate changes in each caller.
-Peff
next prev parent reply other threads:[~2025-11-20 7:50 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-11-20 6:46 `git config get --type=path` results in segmentation fault on value starting with `:(optional)` Han Jiang
2025-11-20 7:50 ` Jeff King [this message]
2025-11-20 14:34 ` D. Ben Knoble
2025-11-20 16:46 ` Junio C Hamano
2025-11-25 0:28 ` Jeff King
2025-11-25 0:57 ` Junio C Hamano
2025-11-26 15:13 ` Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20251120075019.GA1283645@coredump.intra.peff.net \
--to=peff@peff$(echo .)net \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=jhcarl0814@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox