Jharrod LaFon <jlafon@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx> --- 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 && ( -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html