Re: `git config get --type=path` results in segmentation fault on value starting with `:(optional)`

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