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

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

[Han Jiang]

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;

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.52.0.windows.1
cpu: x86_64
built from commit: 2912d8e9b8253723974b7baf1c890273b1a1c5bd
sizeof-long: 4
sizeof-size_t: 8
shell-path: D:/git-sdk-64-build-installers/usr/bin/sh
rust: disabled
feature: fsmonitor--daemon
libcurl: 8.17.0
OpenSSL: OpenSSL 3.5.4 30 Sep 2025
zlib: 1.3.1
SHA-1: SHA1_DC
SHA-256: SHA256_BLK
default-ref-format: files
default-hash: sha1
uname: Windows 10.0 26200
compiler info: gnuc: 15.2
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 get --type=path` results in segmentation fault on value starting with `:(optional)`

[Jeff King]

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

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

[D. Ben Knoble]

On Thu, Nov 20, 2025 at 2:52 AM Jeff King <peff@peff.net> 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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

Thanks for analysing all of the above (omitted); I was doing the
same on the bus but couldn't finish it and then when I reached the
office, you've nicely done everything necessary ;-)

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

The documentation says

	If prefixed with :(optional), the configuration variable is
	treated as if it does not exist, if the named path does not
	exist.

and when I wrote it, by "the configuration variable", I meant the
second "template = ..." line above, not the configuration variable
commit.template, that the machinery pretends not to exist.  So the
result pointing at does-exist matches my expectation.

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

The problem is that there is no mechanism for the function to say
"success" without setting *dest to the discovered value.  We could
introduce multiple kinds of "failure", and have callers react to the
differences, but then it is like setting NULL in *dest and having
callers react to it, so I am not sure how much benefit we would be
gaining by changing its interface.

On the other hand, builtin/config.c::format_config() probably needs
a richer set of return values.  When used from collect_config(), it
needs to be able to say "no, pretend that the key/value pair you fed
me did not exist" in addition to "that value is bogus---you have an
error (e.g., config_error_nonbool())".

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

[Jeff King]

On Thu, Nov 20, 2025 at 08:46:17AM -0800, Junio C Hamano wrote:

> > 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.
> 
> The documentation says
> 
> 	If prefixed with :(optional), the configuration variable is
> 	treated as if it does not exist, if the named path does not
> 	exist.
> 
> and when I wrote it, by "the configuration variable", I meant the
> second "template = ..." line above, not the configuration variable
> commit.template, that the machinery pretends not to exist.  So the
> result pointing at does-exist matches my expectation.

I confess that I did not read the documentation at all, and was only
going on what I'd expect ":(optional)" to do. So you can take what you
will from that. ;) It does feel to me like the user-facing behavior is
driven by ease of implementation, not what users would necessarily want.
But it probably is not worth revisiting at this point (especially
because it is kind of a corner case for the distinction to matter at
all).

(I do agree that the documentation you quoted clearly covers the current
behavior).

> > 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.
> 
> The problem is that there is no mechanism for the function to say
> "success" without setting *dest to the discovered value.  We could
> introduce multiple kinds of "failure", and have callers react to the
> differences, but then it is like setting NULL in *dest and having
> callers react to it, so I am not sure how much benefit we would be
> gaining by changing its interface.

In my mind, we'd still return "0" as long as there was any string at all
(i.e., the only error is the non-bool case). And then the caller would
have to pick the results out of the struct above. I agree that setting
*dest to NULL is mostly equivalent to what I'm proposing. The main
advantages of the struct are:

  1. The caller gets to actually see what the value is. This may or may
     not be useful for stuff like format_config(). See below.

  2. The interface change is a feature, since it requires examining and
     updating each caller (enforced by the compiler).

     It looks like you already produced a patch to update the existing
     callers, and I'll assume you caught them all. It does leave any
     topics-in-flight potentially buggy, though. As somebody who used to
     maintain a long-running fork, and who has a years-long backlog of
     random topics, I do not consider "all of the branches in
     gitster/git.git" to necessarily be all topics in flight. ;)

     (I did check all of my topics and didn't have any new callers,
     though).

> On the other hand, builtin/config.c::format_config() probably needs
> a richer set of return values.  When used from collect_config(), it
> needs to be able to say "no, pretend that the key/value pair you fed
> me did not exist" in addition to "that value is bogus---you have an
> error (e.g., config_error_nonbool())".

I was thinking that we might need some way for format_config() to show
the original value (minus the ":(optional)" meta-tag). The same way that
we may show include.path both as its own config variable, and as a
mechanism that triggers an include. I.e., would somebody ask git-config
about "commit.template" not as a path, but as a string?

But the way to do that is to avoid saying "--type=path" in the first
place, and get the full string (including the optional tag). If we had
some kind of "--type=path --show-missing-paths" option, then we'd need
to be able to see the missing name (like my struct proposal above). But
we don't, and nobody is asking for it, so I think we can punt on it for
now.

I did wonder also if format_config() would need to roll back any output
for something like:

  git -c foo.bar=':(optional)/no-such-file' \
    config --type=path --get-regexp --show-scope foo.bar

which would show the key name and scope before even looking at the
value. But because we assemble it all in a strbuf, we can just throw
away the result.  And it looks like your patches handle that. It doesn't
look like the tests cover it, though.

Looks your topic isn't in 'next' yet, so possibly squash this in?

diff --git a/t/t1311-config-optional.sh b/t/t1311-config-optional.sh
index 766693387f..fbbacfc67b 100755
--- a/t/t1311-config-optional.sh
+++ b/t/t1311-config-optional.sh
@@ -18,7 +18,9 @@ test_expect_success 'var=:(optional)path-exists' '
 
 test_expect_success 'missing optional value is ignored' '
 	test_config a.path ":(optional)no-such-path" &&
-	test_must_fail git config get --path a.path >actual &&
+	# Using --show-scope ensures we skip writing not only the value
+	# but also any meta-information about the ignored key.
+	test_must_fail git config get --show-scope --path a.path >actual &&
 	test_line_count = 0 actual
 '
 

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I confess that I did not read the documentation at all, and was only
> going on what I'd expect ":(optional)" to do. So you can take what you
> will from that. ;) It does feel to me like the user-facing behavior is
> driven by ease of implementation, not what users would necessarily want.
> But it probably is not worth revisiting at this point (especially
> because it is kind of a corner case for the distinction to matter at
> all).

Hmph, I tend to disagree; this was not driven by ease of
implementation at all.  Rather, :(optional) cannot be an attribute
of a variable; it is an attribute of individual setting of a variable.

For example, imagine that you want to say "the system wide fallback
is in this file in /etc, but you can override it with a file in your
home directory", and you want to say that only once in the system
wide configuration file so that it applies to all users, without
each end user having to specify that they do want to override it in
their Git configuration file.

You can write this in /etc/gitconfig

    [default]
	editorConfig = /etc/editorConfig
	editorConfig = ':(optional)~/.editorConfig'

and ask what path default.editorConfig file is.  As long as large
enough user population agrees what the name of the file under their
$HOME to control the behaviour, this would work better than telling
them "you can override default.editorCondfig in your per-user
configuration file", as it is one fewer thing to configure.

And this is possible only if we consider that what the system
pretends not to have seen is per :(optional) definition.

> But the way to do that is to avoid saying "--type=path" in the first
> place, and get the full string (including the optional tag). If we had
> some kind of "--type=path --show-missing-paths" option, then we'd need
> to be able to see the missing name (like my struct proposal above). But
> we don't, and nobody is asking for it, so I think we can punt on it for
> now.

;-).

> I did wonder also if format_config() would need to roll back any output
> for something like:
>
>   git -c foo.bar=':(optional)/no-such-file' \
>     config --type=path --get-regexp --show-scope foo.bar
>
> which would show the key name and scope before even looking at the
> value. But because we assemble it all in a strbuf, we can just throw
> away the result.  And it looks like your patches handle that. It doesn't
> look like the tests cover it, though.

Didn't think about that case, but then we seem to be lucky ;-).

> Looks your topic isn't in 'next' yet, so possibly squash this in?
>
> diff --git a/t/t1311-config-optional.sh b/t/t1311-config-optional.sh
> index 766693387f..fbbacfc67b 100755
> --- a/t/t1311-config-optional.sh
> +++ b/t/t1311-config-optional.sh
> @@ -18,7 +18,9 @@ test_expect_success 'var=:(optional)path-exists' '
>  
>  test_expect_success 'missing optional value is ignored' '
>  	test_config a.path ":(optional)no-such-path" &&
> -	test_must_fail git config get --path a.path >actual &&
> +	# Using --show-scope ensures we skip writing not only the value
> +	# but also any meta-information about the ignored key.
> +	test_must_fail git config get --show-scope --path a.path >actual &&
>  	test_line_count = 0 actual
>  '

Nice ;-).

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

[Jeff King]

On Mon, Nov 24, 2025 at 04:57:35PM -0800, Junio C Hamano wrote:

> Hmph, I tend to disagree; this was not driven by ease of
> implementation at all.  Rather, :(optional) cannot be an attribute
> of a variable; it is an attribute of individual setting of a variable.
> 
> For example, imagine that you want to say "the system wide fallback
> is in this file in /etc, but you can override it with a file in your
> home directory", and you want to say that only once in the system
> wide configuration file so that it applies to all users, without
> each end user having to specify that they do want to override it in
> their Git configuration file.
> 
> You can write this in /etc/gitconfig
> 
>     [default]
> 	editorConfig = /etc/editorConfig
> 	editorConfig = ':(optional)~/.editorConfig'
> 
> and ask what path default.editorConfig file is.  As long as large
> enough user population agrees what the name of the file under their
> $HOME to control the behaviour, this would work better than telling
> them "you can override default.editorCondfig in your per-user
> configuration file", as it is one fewer thing to configure.
> 
> And this is possible only if we consider that what the system
> pretends not to have seen is per :(optional) definition.

Yes, I agree that the code as-is opens up that workflow. But it forbids
the flipside, which is: "the sysadmin set up a path in /etc, but I do not
ever want to use that; I want to use my file if present, or nothing".

Now which is more likely, I don't know. I've never wanted to do either. ;)

The workflow I suggest would also perhaps be more elegant if there was a
way to "unset" a variable. We allow that in some cases for list-like
variables, with an empty entry to reset the list. But usually for
single-valued variables, we assume that last-one-wins is enough.

-Peff