From: D. Ben Knoble Date: Thu, 20 Nov 2025 14:34:03 GMT Subject: Re: `git config get --type=path` results in segmentation fault on value starting with `:(optional)` Message-ID: In-Reply-To: <20251120075019.GA1283645@coredump.intra.peff.net> On Thu, Nov 20, 2025 at 2:52 AM Jeff King wrote: > > 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); > }[...] > Thanks for the diagnosis; just hit this myself and tracked down the same code. > 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). Yeah, unfortunately it doesn't look like string_list_insert considers NULL a no-op. Similarly I don't think strbuff_add can handle NULL because it calls strlen on the argument. > 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 -- D. Ben Knoble