[PATCH] Git segmentation faults if submodule path is empty.

[PATCH] Git segmentation faults if submodule path is empty.

[Jharrod LaFon]

Git fails due to a segmentation fault if a submodule path is empty.
Here is an example .gitmodules that will cause a segmentation fault:
[submodule "foo-module"]
    path
    url = http://host/repo.git
$ git status
Segmentation fault (core dumped)

This occurs because in the function parse_submodule_config_option, the
variable 'value' is assumed not to be null, and when passed as an
argument to xstrdup a segmentation fault occurs if it is indeed null.
This is the case when using the .gitmodules example above.

This patch addresses the issue by returning from the function if
'value' is null before the call to xstrdup is made.

Signed-off-by: Jharrod LaFon <jlafon <at> eyesopen.com>
---
 submodule.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/submodule.c b/submodule.c
index 1821a5b..880f21b 100644
--- a/submodule.c
+++ b/submodule.c
@@ -130,7 +130,7 @@ int parse_submodule_config_option(const char *var, const char *value)
 	const char *name, *key;
 	int namelen;
 
-	if (parse_config_key(var, "submodule", &name, &namelen, &key) < 0 || !name)
+	if (parse_config_key(var, "submodule", &name, &namelen, &key) < 0 || !name || !value)
 		return 0;
 
 	if (!strcmp(key, "path")) {
-- 
1.7.9.5


--
Jharrod LaFon
OpenEye Scientific Software

Re: [PATCH] Git segmentation faults if submodule path is empty.

[Jens Lehmann]

Am 16.08.2013 03:51, schrieb Jharrod LaFon:
> Git fails due to a segmentation fault if a submodule path is empty.
> Here is an example .gitmodules that will cause a segmentation fault:
> [submodule "foo-module"]
>     path
>     url = http://host/repo.git
> $ git status
> Segmentation fault (core dumped)
> 
> This occurs because in the function parse_submodule_config_option, the
> variable 'value' is assumed not to be null, and when passed as an
> argument to xstrdup a segmentation fault occurs if it is indeed null.
> This is the case when using the .gitmodules example above.

Thanks for digging this up and describing it in a way that makes it
easy to reproduce and understand.

> This patch addresses the issue by returning from the function if
> 'value' is null before the call to xstrdup is made.

Hmm, I'm not sure silently ignoring the misconfiguration is the best
way to go. A submodule config having a path setting without a value
is broken (while a submodule setting without a subsection configures
something else, so the "|| !name" below is fine). So I believe we
should complain to the user when "value" is NULL.

On the other hand this should only happen for the three options we do
parse, as some users (e.g. git-submodule.sh) use other configurations
for which a missing value may be fine. Please see the "lacks value"
errors in read_merge_config() of ll-merge.c for an example of how to
deal with that.

And looking at other users of parse_config_key() I suspect there will
be other configuration options showing the same problem ...

> Signed-off-by: Jharrod LaFon <jlafon <at> eyesopen.com>
> ---
>  submodule.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/submodule.c b/submodule.c
> index 1821a5b..880f21b 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -130,7 +130,7 @@ int parse_submodule_config_option(const char *var, const char *value)
>  	const char *name, *key;
>  	int namelen;
>  
> -	if (parse_config_key(var, "submodule", &name, &namelen, &key) < 0 || !name)
> +	if (parse_config_key(var, "submodule", &name, &namelen, &key) < 0 || !name || !value)
>  		return 0;
>  
>  	if (!strcmp(key, "path")) {
> 

Re: [PATCH] Git segmentation faults if submodule path is empty.

[Jeff King]

On Fri, Aug 16, 2013 at 08:48:43AM +0200, Jens Lehmann wrote:

> > This patch addresses the issue by returning from the function if
> > 'value' is null before the call to xstrdup is made.
> 
> Hmm, I'm not sure silently ignoring the misconfiguration is the best
> way to go. A submodule config having a path setting without a value
> is broken (while a submodule setting without a subsection configures
> something else, so the "|| !name" below is fine). So I believe we
> should complain to the user when "value" is NULL.

Yeah, the usual behavior is to catch it and return config_error_nonbool
(because a key without a value is an implicit-true boolean).  Most code
does this via git_config_string, but what the submodule code is doing is
too tricky and custom to use that stock function.

> On the other hand this should only happen for the three options we do
> parse, as some users (e.g. git-submodule.sh) use other configurations
> for which a missing value may be fine. Please see the "lacks value"
> errors in read_merge_config() of ll-merge.c for an example of how to
> deal with that.

Those spots in ll-merge should probably be using config_error_nonbool,
if only for consistency with the rest of the code base.

> > -	if (parse_config_key(var, "submodule", &name, &namelen, &key) < 0 || !name)
> > +	if (parse_config_key(var, "submodule", &name, &namelen, &key) < 0 || !name || !value)
> >  		return 0;

I think this is also the wrong place to make the check, anyway. It is
saying that all values of submodule.X.Y must be non-NULL. But that is
not true. The submodule.X.fetchRecurseSubmodules option can be a
boolean, and:

  [submodule "foo"]
    fetchRecurseSubmodules

is perfectly valid (and is broken by this patch).

-Peff

Re: [PATCH] Git segmentation faults if submodule path is empty.

[Jeff King]

On Fri, Aug 16, 2013 at 09:09:58AM -0400, Jeff King wrote:

> > > -	if (parse_config_key(var, "submodule", &name, &namelen, &key) < 0 || !name)
> > > +	if (parse_config_key(var, "submodule", &name, &namelen, &key) < 0 || !name || !value)
> > >  		return 0;
> 
> I think this is also the wrong place to make the check, anyway. It is
> saying that all values of submodule.X.Y must be non-NULL. But that is
> not true. The submodule.X.fetchRecurseSubmodules option can be a
> boolean, and:
> 
>   [submodule "foo"]
>     fetchRecurseSubmodules
> 
> is perfectly valid (and is broken by this patch).

IOW, I think this is the right fix:

diff --git a/submodule.c b/submodule.c
index 3f0a3f9..c0f93c2 100644
--- a/submodule.c
+++ b/submodule.c
@@ -134,6 +134,9 @@ int parse_submodule_config_option(const char *var, const char *value)
 		return 0;
 
 	if (!strcmp(key, "path")) {
+		if (!value)
+			return config_error_nonbool(var);
+
 		config = unsorted_string_list_lookup(&config_name_for_path, value);
 		if (config)
 			free(config->util);
@@ -151,6 +154,9 @@ int parse_submodule_config_option(const char *var, const char *value)
 	} else if (!strcmp(key, "ignore")) {
 		char *name_cstr;
 
+		if (!value)
+			return config_error_nonbool(var);
+
 		if (strcmp(value, "untracked") && strcmp(value, "dirty") &&
 		    strcmp(value, "all") && strcmp(value, "none")) {
 			warning("Invalid parameter \"%s\" for config option \"submodule.%s.ignore\"", value, var);

And new options, as they are added, must decide whether they are boolean
or not (and check !value as appropriate).

-Peff

Re: [PATCH] Git segmentation faults if submodule path is empty.

[Jharrod LaFon]

OK,  I'll incorporate Jeff's changes, add a test and resubmit the patch.

Thanks,

--
Jharrod LaFon
OpenEye Scientific Software

On Aug 16, 2013, at 7:14 AM, Jeff King <peff@peff.net> wrote:

> On Fri, Aug 16, 2013 at 09:09:58AM -0400, Jeff King wrote:
> 
>>>> -	if (parse_config_key(var, "submodule", &name, &namelen, &key) < 0 || !name)
>>>> +	if (parse_config_key(var, "submodule", &name, &namelen, &key) < 0 || !name || !value)
>>>> 		return 0;
>> 
>> I think this is also the wrong place to make the check, anyway. It is
>> saying that all values of submodule.X.Y must be non-NULL. But that is
>> not true. The submodule.X.fetchRecurseSubmodules option can be a
>> boolean, and:
>> 
>>  [submodule "foo"]
>>    fetchRecurseSubmodules
>> 
>> is perfectly valid (and is broken by this patch).
> 
> IOW, I think this is the right fix:
> 
> diff --git a/submodule.c b/submodule.c
> index 3f0a3f9..c0f93c2 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -134,6 +134,9 @@ int parse_submodule_config_option(const char *var, const char *value)
> 		return 0;
> 
> 	if (!strcmp(key, "path")) {
> +		if (!value)
> +			return config_error_nonbool(var);
> +
> 		config = unsorted_string_list_lookup(&config_name_for_path, value);
> 		if (config)
> 			free(config->util);
> @@ -151,6 +154,9 @@ int parse_submodule_config_option(const char *var, const char *value)
> 	} else if (!strcmp(key, "ignore")) {
> 		char *name_cstr;
> 
> +		if (!value)
> +			return config_error_nonbool(var);
> +
> 		if (strcmp(value, "untracked") && strcmp(value, "dirty") &&
> 		    strcmp(value, "all") && strcmp(value, "none")) {
> 			warning("Invalid parameter \"%s\" for config option \"submodule.%s.ignore\"", value, var);
> 
> And new options, as they are added, must decide whether they are boolean
> or not (and check !value as appropriate).
> 
> -Peff

Re: [PATCH] Git segmentation faults if submodule path is empty.

[Jharrod LaFon]

Here is an updated patch with a test. 

Git fails due to a segmentation fault if a submodule path is empty.
Here is an example .gitmodules that will cause a segmentation fault:
[submodule "foo-module"]
  path
  url = http://host/repo.git
$ git status
Segmentation fault (core dumped)

This occurs because in the function parse_submodule_config_option, the
variable 'value' is assumed to be null, and when passed as an argument
to xstrdup a segmentation fault occurs if it is indeed null.
This is the case when using the .gitmodules example above.

This patch addresses the issue by checking to make sure 'value' is not
null before using it as an argument to xstrdup.  For some configuration
options, such as fetchRecurseSubmodules, an empty value is valid.  If
the option being read is 'path', an empty value is not valid, and so
an error message is printed.

Signed-off-by: Jharrod LaFon <jlafon <at> eyesopen.com>
---
 submodule.c                    |    6 ++++++
 t/t1307-null-submodule-path.sh |   16 ++++++++++++++++
 2 files changed, 22 insertions(+)
 create mode 100644 t/t1307-null-submodule-path.sh

diff --git a/submodule.c b/submodule.c
index 1821a5b..1e4acfd 100644
--- a/submodule.c
+++ b/submodule.c
@@ -134,6 +134,9 @@ int parse_submodule_config_option(const char *var, const char *value)
 		return 0;
 
 	if (!strcmp(key, "path")) {
+        if (!value)
+            return config_error_nonbool(var);
+
 		config = unsorted_string_list_lookup(&config_name_for_path, value);
 		if (config)
 			free(config->util);
@@ -151,6 +154,9 @@ int parse_submodule_config_option(const char *var, const char *value)
 	} else if (!strcmp(key, "ignore")) {
 		char *name_cstr;
 
+        if (!value)
+            return config_error_nonbool(var);
+
 		if (strcmp(value, "untracked") && strcmp(value, "dirty") &&
 		    strcmp(value, "all") && strcmp(value, "none")) {
 			warning("Invalid parameter \"%s\" for config option \"submodule.%s.ignore\"", value, var);
diff --git a/t/t1307-null-submodule-path.sh b/t/t1307-null-submodule-path.sh
new file mode 100644
index 0000000..eeda2cb
--- /dev/null
+++ b/t/t1307-null-submodule-path.sh
@@ -0,0 +1,16 @@
+#!/bin/sh
+
+test_description='test empty submodule path'
+. ./test-lib.sh
+
+setup() {
+    (printf "[submodule \"test\"]\n" && 
+    printf "  path\n" &&
+    printf "  url") >.gitmodules
+}
+
+test_expect_success 'git status with empty submodule path should not segfault' '
+    setup &&
+    test_must_fail git status
+'
+test_done
-- 
1.7.9.5



--
Jharrod LaFon
OpenEye Scientific Software

On Aug 16, 2013, at 9:12 AM, Jharrod LaFon <jlafon@eyesopen.com> wrote:

> OK,  I'll incorporate Jeff's changes, add a test and resubmit the patch.
> 
> Thanks,
> 
> --
> Jharrod LaFon
> OpenEye Scientific Software
> 
> On Aug 16, 2013, at 7:14 AM, Jeff King <peff@peff.net> wrote:
> 
>> On Fri, Aug 16, 2013 at 09:09:58AM -0400, Jeff King wrote:
>> 
>>>>> -	if (parse_config_key(var, "submodule", &name, &namelen, &key) < 0 || !name)
>>>>> +	if (parse_config_key(var, "submodule", &name, &namelen, &key) < 0 || !name || !value)
>>>>> 		return 0;
>>> 
>>> I think this is also the wrong place to make the check, anyway. It is
>>> saying that all values of submodule.X.Y must be non-NULL. But that is
>>> not true. The submodule.X.fetchRecurseSubmodules option can be a
>>> boolean, and:
>>> 
>>> [submodule "foo"]
>>>   fetchRecurseSubmodules
>>> 
>>> is perfectly valid (and is broken by this patch).
>> 
>> IOW, I think this is the right fix:
>> 
>> diff --git a/submodule.c b/submodule.c
>> index 3f0a3f9..c0f93c2 100644
>> --- a/submodule.c
>> +++ b/submodule.c
>> @@ -134,6 +134,9 @@ int parse_submodule_config_option(const char *var, const char *value)
>> 		return 0;
>> 
>> 	if (!strcmp(key, "path")) {
>> +		if (!value)
>> +			return config_error_nonbool(var);
>> +
>> 		config = unsorted_string_list_lookup(&config_name_for_path, value);
>> 		if (config)
>> 			free(config->util);
>> @@ -151,6 +154,9 @@ int parse_submodule_config_option(const char *var, const char *value)
>> 	} else if (!strcmp(key, "ignore")) {
>> 		char *name_cstr;
>> 
>> +		if (!value)
>> +			return config_error_nonbool(var);
>> +
>> 		if (strcmp(value, "untracked") && strcmp(value, "dirty") &&
>> 		    strcmp(value, "all") && strcmp(value, "none")) {
>> 			warning("Invalid parameter \"%s\" for config option \"submodule.%s.ignore\"", value, var);
>> 
>> And new options, as they are added, must decide whether they are boolean
>> or not (and check !value as appropriate).
>> 
>> -Peff
> 

Re: [PATCH] Git segmentation faults if submodule path is empty.

[Jeff King]

On Fri, Aug 16, 2013 at 10:59:35AM -0700, Jharrod LaFon wrote:

> Here is an updated patch with a test.

Bits like this that should not be part of the commit message should
either go after the "---" lines near the diffstat, or should come before
a scissors line, like:

  Here is my new patch.

  -- >8 --
  Git segmentation faults etc...

That way git-am can do the right thing, and the maintainer does not have
to fix up your patch by hand.

> diff --git a/submodule.c b/submodule.c
> index 1821a5b..1e4acfd 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -134,6 +134,9 @@ int parse_submodule_config_option(const char *var, const char *value)
>  		return 0;
>  
>  	if (!strcmp(key, "path")) {
> +        if (!value)
> +            return config_error_nonbool(var);

Your indentation looks like two spaces here, which does not match the
rest of the block. It should be one tab.

> @@ -151,6 +154,9 @@ int parse_submodule_config_option(const char *var, const char *value)
>  	} else if (!strcmp(key, "ignore")) {
>  		char *name_cstr;
>  
> +        if (!value)
> +            return config_error_nonbool(var);
> +

Ditto here.

> diff --git a/t/t1307-null-submodule-path.sh b/t/t1307-null-submodule-path.sh
> new file mode 100644
> index 0000000..eeda2cb
> --- /dev/null
> +++ b/t/t1307-null-submodule-path.sh
> @@ -0,0 +1,16 @@
> +#!/bin/sh
> +
> +test_description='test empty submodule path'
> +. ./test-lib.sh
> +
> +setup() {
> +    (printf "[submodule \"test\"]\n" && 
> +    printf "  path\n" &&
> +    printf "  url") >.gitmodules
> +}

You can use single-quotes to avoid having to backslash the embedded
double-quotes. And I do not see any reason to use printf rather than the
more readable echo, which can drop the "\n".

And is there any point in having the "url" field?  The presence of a
valueless bool "path" should be enough, no? It may be easier to see what
it is we are testing without the extraneous parameter.

With those changes, you could even put it all on one line:

  echo '[submodule "test"] path' >.gitmodules

-Peff

Re: [PATCH] Git segmentation faults if submodule path is empty.

[Jharrod LaFon]

Updated the patch and the patch submission.

 -- >8 --

Git segmentation faults if submodule path is empty.

    Git fails due to a segmentation fault if a submodule path is empty.
    Here is an example .gitmodules that will cause a segmentation fault:
    [submodule "foo-module"]
      path
      url = http://host/repo.git
    $ git status
    Segmentation fault (core dumped)

    This occurs because in the function parse_submodule_config_option, the
    variable 'value' is assumed to be null, and when passed as an argument
    to xstrdup a segmentation fault occurs if it is indeed null.
    This is the case when using the .gitmodules example above.

    This patch addresses the issue by checking to make sure 'value' is not
    null before using it as an argument to xstrdup.  For some configuration
    options, such as fetchRecurseSubmodules, an empty value is valid.  If
    the option being read is 'path', an empty value is not valid, and so
    an error message is printed.

Signed-off-by: Jharrod LaFon <jlafon <at> eyesopen.com>
---
 submodule.c                    |    6 ++++++
 t/t1307-null-submodule-path.sh |   14 ++++++++++++++
 2 files changed, 20 insertions(+)
 create mode 100755 t/t1307-null-submodule-path.sh

diff --git a/submodule.c b/submodule.c
index 1821a5b..1a2cf30 100644
--- a/submodule.c
+++ b/submodule.c
@@ -134,6 +134,9 @@ int parse_submodule_config_option(const char *var, const char *value)
 		return 0;
 
 	if (!strcmp(key, "path")) {
+		if (!value)
+			return config_error_nonbool(var);
+
 		config = unsorted_string_list_lookup(&config_name_for_path, value);
 		if (config)
 			free(config->util);
@@ -151,6 +154,9 @@ int parse_submodule_config_option(const char *var, const char *value)
 	} else if (!strcmp(key, "ignore")) {
 		char *name_cstr;
 
+		if (!value)
+			return config_error_nonbool(var);
+
 		if (strcmp(value, "untracked") && strcmp(value, "dirty") &&
 		    strcmp(value, "all") && strcmp(value, "none")) {
 			warning("Invalid parameter \"%s\" for config option \"submodule.%s.ignore\"", value, var);
diff --git a/t/t1307-null-submodule-path.sh b/t/t1307-null-submodule-path.sh
new file mode 100755
index 0000000..a4470a8
--- /dev/null
+++ b/t/t1307-null-submodule-path.sh
@@ -0,0 +1,14 @@
+#!/bin/sh
+
+test_description='test empty submodule path'
+. ./test-lib.sh
+
+setup() {
+    echo '[submodule "test"] path' > .gitmodules
+}
+
+test_expect_success 'git status with empty submodule path should not seg fault' '
+    setup &&
+    test_must_fail git status
+'
+test_done
-- 
1.7.9.5

 On Aug 16, 2013, at 2:52 PM, Jeff King <peff@peff.net> wrote:

> On Fri, Aug 16, 2013 at 10:59:35AM -0700, Jharrod LaFon wrote:
> 
>> Here is an updated patch with a test.
> 
> Bits like this that should not be part of the commit message should
> either go after the "---" lines near the diffstat, or should come before
> a scissors line, like:
> 
>  Here is my new patch.
> 
>  -- >8 --
>  Git segmentation faults etc...
> 
> That way git-am can do the right thing, and the maintainer does not have
> to fix up your patch by hand.
> 
>> diff --git a/submodule.c b/submodule.c
>> index 1821a5b..1e4acfd 100644
>> --- a/submodule.c
>> +++ b/submodule.c
>> @@ -134,6 +134,9 @@ int parse_submodule_config_option(const char *var, const char *value)
>> 		return 0;
>> 
>> 	if (!strcmp(key, "path")) {
>> +        if (!value)
>> +            return config_error_nonbool(var);
> 
> Your indentation looks like two spaces here, which does not match the
> rest of the block. It should be one tab.
> 
>> @@ -151,6 +154,9 @@ int parse_submodule_config_option(const char *var, const char *value)
>> 	} else if (!strcmp(key, "ignore")) {
>> 		char *name_cstr;
>> 
>> +        if (!value)
>> +            return config_error_nonbool(var);
>> +
> 
> Ditto here.
> 
>> diff --git a/t/t1307-null-submodule-path.sh b/t/t1307-null-submodule-path.sh
>> new file mode 100644
>> index 0000000..eeda2cb
>> --- /dev/null
>> +++ b/t/t1307-null-submodule-path.sh
>> @@ -0,0 +1,16 @@
>> +#!/bin/sh
>> +
>> +test_description='test empty submodule path'
>> +. ./test-lib.sh
>> +
>> +setup() {
>> +    (printf "[submodule \"test\"]\n" && 
>> +    printf "  path\n" &&
>> +    printf "  url") >.gitmodules
>> +}
> 
> You can use single-quotes to avoid having to backslash the embedded
> double-quotes. And I do not see any reason to use printf rather than the
> more readable echo, which can drop the "\n".
> 
> And is there any point in having the "url" field?  The presence of a
> valueless bool "path" should be enough, no? It may be easier to see what
> it is we are testing without the extraneous parameter.
> 
> With those changes, you could even put it all on one line:
> 
>  echo '[submodule "test"] path' >.gitmodules
> 
> -Peff

Re: [PATCH] Git segmentation faults if submodule path is empty.

[Junio C Hamano]

Jharrod LaFon <jlafon@eyesopen.com> writes:

> Updated the patch and the patch submission.
>

Getting a lot warmer ;-).

>  -- >8 --
>
> Git segmentation faults if submodule path is empty.

If this is meant to replace the MUA's Subject: line, then please add
"Subject: " prefix, like the example at the end of this message.

Our commit titles by convention omit the final full-stop.

>     Git fails due to a segmentation fault if a submodule path is empty.

Please do not indent the body of the commit log message.  Flush them
to the left.

>     Here is an example .gitmodules that will cause a segmentation fault:

Please have a blank line before an example added for illustration in
the log message.

>     [submodule "foo-module"]
>       path
>       url = http://host/repo.git
>     $ git status
>     Segmentation fault (core dumped)

Indenting such an illustration, and having a blank line after it,
are good things. Please continue doing so.

>     This occurs because in the function parse_submodule_config_option, the

Again, please do not indent the body text of the log message.

>     variable 'value' is assumed to be null, and when passed as an argument
>     to xstrdup a segmentation fault occurs if it is indeed null.
>     This is the case when using the .gitmodules example above.

It is not "assumed to be null", is it?

>     This patch addresses the issue by checking to make sure 'value' is not
>     null before using it as an argument to xstrdup.  For some configuration
>     options, such as fetchRecurseSubmodules, an empty value is valid.  If
>     the option being read is 'path', an empty value is not valid, and so
>     an error message is printed.

We like to write the log message in the imperative mood, as if we
are ordering the codebase to "make it so".

> Signed-off-by: Jharrod LaFon <jlafon <at> eyesopen.com>

Please do not do cute " <at> " here.  With a "Signed-off-by", you
are already agreeing to Developer's Certificate of Origin 1.1,
cluase (d):

	(d) I understand and agree that this project and the contribution
	    are public and that a record of the contribution (including all
	    personal information I submit with it, including my sign-off) is
	    maintained indefinitely and may be redistributed consistent with
	    this project or the open source license(s) involved.

> ---
>  submodule.c                    |    6 ++++++
>  t/t1307-null-submodule-path.sh |   14 ++++++++++++++

Can we have the test not in a brand new test script file, but at the
end of an existing one?

> diff --git a/t/t1307-null-submodule-path.sh b/t/t1307-null-submodule-path.sh
> new file mode 100755
> index 0000000..a4470a8
> --- /dev/null
> +++ b/t/t1307-null-submodule-path.sh
> @@ -0,0 +1,14 @@
> +#!/bin/sh
> +
> +test_description='test empty submodule path'
> +. ./test-lib.sh
> +
> +setup() {
> +    echo '[submodule "test"] path' > .gitmodules
> +}

No SP after redirection operator, i.e.

	echo '[submodule "test"] path' >.gitmodules

Also it does not make much sense to have a helper script that is
used only once (for that matter, it does not make much sense to add
a new script file to add a single two-liner test).

Here is to illustrate all the points mentioned above.

-- >8 --
From: Jharrod LaFon <jlafon@eyesopen.com>
Subject: avoid segfault on submodule.*.path set to an empty "true"
Date: Mon, 19 Aug 2013 09:26:56 -0700

Git fails due to a segmentation fault if a submodule path is empty.
Here is an example .gitmodules that will cause a segmentation fault:

    [submodule "foo-module"]
      path
      url = http://host/repo.git
    $ git status
    Segmentation fault (core dumped)

This is because the parsing of "submodule.*.path" is not prepared to
see a value-less "true" and assumes that the value is always
non-NULL (parsing of "ignore" has the same problem).

Fix it by checking the NULL-ness of value and complain with
config_error_nonbool().

Signed-off-by: Jharrod LaFon <jlafon@eyesopen.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 submodule.c                |  6 ++++++
 t/t7400-submodule-basic.sh | 10 ++++++++++
 2 files changed, 16 insertions(+)

diff --git a/submodule.c b/submodule.c
index 3f0a3f9..c0f93c2 100644
--- a/submodule.c
+++ b/submodule.c
@@ -134,6 +134,9 @@ int parse_submodule_config_option(const char *var, const char *value)
 		return 0;
 
 	if (!strcmp(key, "path")) {
+		if (!value)
+			return config_error_nonbool(var);
+
 		config = unsorted_string_list_lookup(&config_name_for_path, value);
 		if (config)
 			free(config->util);
@@ -151,6 +154,9 @@ int parse_submodule_config_option(const char *var, const char *value)
 	} else if (!strcmp(key, "ignore")) {
 		char *name_cstr;
 
+		if (!value)
+			return config_error_nonbool(var);
+
 		if (strcmp(value, "untracked") && strcmp(value, "dirty") &&
 		    strcmp(value, "all") && strcmp(value, "none")) {
 			warning("Invalid parameter \"%s\" for config option \"submodule.%s.ignore\"", value, var);
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 5ee97b0..a39d074 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -18,6 +18,16 @@ test_expect_success 'setup - initial commit' '
 	git branch initial
 '
 
+test_expect_success 'configuration parsing' '
+	test_when_finished "rm -f .gitmodules" &&
+	cat >.gitmodules <<-\EOF &&
+	[submodule "s"]
+		path
+		ignore
+	EOF
+	test_must_fail git status
+'
+
 test_expect_success 'setup - repository in init subdirectory' '
 	mkdir init &&
 	(

Re: [PATCH] Git segmentation faults if submodule path is empty.

[Jeff King]

On Mon, Aug 19, 2013 at 11:56:17AM -0700, Junio C Hamano wrote:

> Jharrod LaFon <jlafon@eyesopen.com> writes:
> 
> > Updated the patch and the patch submission.
> >
> 
> Getting a lot warmer ;-).

Thanks, I agree with all of the stuff you said. The end result that you
included looks good to me.

-Peff

Re: [PATCH] Git segmentation faults if submodule path is empty.

[Junio C Hamano]

Jharrod LaFon <jlafon@eyesopen.com> writes:

> I will keep trying this until it's perfect, and I thank you for
> the help.  When I resubmit this, would you like me to include your
> sign-off line as well?

If the one I attached at the end of the message you are responding
to looks fine to you, I'd just apply it without having you to
reroll.

> Also, the end of the test script was not included in your message.  

Sorry, but I am not sure what you meant by this.

I reworked your example to make it the second test in an existing
test script.  There are many existing tests after that so it is
natural that we wouldn't see the end of the file in the patch
context.

>> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
>> index 5ee97b0..a39d074 100755
>> --- a/t/t7400-submodule-basic.sh
>> +++ b/t/t7400-submodule-basic.sh
>> @@ -18,6 +18,16 @@ test_expect_success 'setup - initial commit' '
>> 	git branch initial
>> '
>> 
>> +test_expect_success 'configuration parsing' '
>> +	test_when_finished "rm -f .gitmodules" &&
>> +	cat >.gitmodules <<-\EOF &&
>> +	[submodule "s"]
>> +		path
>> +		ignore
>> +	EOF
>> +	test_must_fail git status
>> +'
>> +
>> test_expect_success 'setup - repository in init subdirectory' '
>> 	mkdir init &&
>> 	(

Re: [PATCH] Git segmentation faults if submodule path is empty.

[Jharrod LaFon]

It looks great to me.

Thanks,

--
Jharrod LaFon
OpenEye Scientific Software

On Aug 19, 2013, at 2:54 PM, Junio C Hamano <gitster@pobox.com> wrote:

> Jharrod LaFon <jlafon@eyesopen.com> writes:
> 
>> I will keep trying this until it's perfect, and I thank you for
>> the help.  When I resubmit this, would you like me to include your
>> sign-off line as well?
> 
> If the one I attached at the end of the message you are responding
> to looks fine to you, I'd just apply it without having you to
> reroll.
> 
>> Also, the end of the test script was not included in your message.  
> 
> Sorry, but I am not sure what you meant by this.
> 
> I reworked your example to make it the second test in an existing
> test script.  There are many existing tests after that so it is
> natural that we wouldn't see the end of the file in the patch
> context.
> 
>>> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
>>> index 5ee97b0..a39d074 100755
>>> --- a/t/t7400-submodule-basic.sh
>>> +++ b/t/t7400-submodule-basic.sh
>>> @@ -18,6 +18,16 @@ test_expect_success 'setup - initial commit' '
>>> 	git branch initial
>>> '
>>> 
>>> +test_expect_success 'configuration parsing' '
>>> +	test_when_finished "rm -f .gitmodules" &&
>>> +	cat >.gitmodules <<-\EOF &&
>>> +	[submodule "s"]
>>> +		path
>>> +		ignore
>>> +	EOF
>>> +	test_must_fail git status
>>> +'
>>> +
>>> test_expect_success 'setup - repository in init subdirectory' '
>>> 	mkdir init &&
>>> 	(

Re: [PATCH] Git segmentation faults if submodule path is empty.

[Thomas Rast]

Jharrod LaFon <jlafon@eyesopen.com> writes:

> Git fails due to a segmentation fault if a submodule path is empty.
> Here is an example .gitmodules that will cause a segmentation fault:
> [submodule "foo-module"]
>     path
>     url = http://host/repo.git
> $ git status
> Segmentation fault (core dumped)

Can you turn this into a test to guard against the bug reappearing?

> This occurs because in the function parse_submodule_config_option, the
> variable 'value' is assumed not to be null, and when passed as an
> argument to xstrdup a segmentation fault occurs if it is indeed null.
> This is the case when using the .gitmodules example above.
>
> This patch addresses the issue by returning from the function if
> 'value' is null before the call to xstrdup is made.
>
> Signed-off-by: Jharrod LaFon <jlafon <at> eyesopen.com>
> ---
>  submodule.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/submodule.c b/submodule.c
> index 1821a5b..880f21b 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -130,7 +130,7 @@ int parse_submodule_config_option(const char *var, const char *value)
>  	const char *name, *key;
>  	int namelen;
>  
> -	if (parse_config_key(var, "submodule", &name, &namelen, &key) < 0 || !name)
> +	if (parse_config_key(var, "submodule", &name, &namelen, &key) < 0 || !name || !value)
>  		return 0;
>  
>  	if (!strcmp(key, "path")) {

-- 
Thomas Rast
trast@{inf,student}.ethz.ch