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

Jharrod LaFon [off-list ref] 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?

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 <redacted>
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 <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
 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 &&
 	(

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

Jharrod LaFon [off-list ref] writes:

quoted

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