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

2013-08-19 Thread 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 000..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 p...@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 000..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

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at

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

2013-08-19 Thread Junio C Hamano 
Jharrod LaFon jla...@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 000..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 jla...@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 jla...@eyesopen.com
Signed-off-by: Junio C Hamano gits...@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)) {

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

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

 Jharrod LaFon jla...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-08-19 Thread Junio C Hamano 
Jharrod LaFon jla...@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 
  (
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-08-19 Thread Jharrod LaFon 
It looks great to me.

Thanks,

--
Jharrod LaFon
OpenEye Scientific Software

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

 Jharrod LaFon jla...@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 
 (

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-08-16 Thread 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)) {
 

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-08-16 Thread Thomas Rast 
Jharrod LaFon jla...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-08-16 Thread 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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-08-16 Thread 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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-08-16 Thread 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 p...@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

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-08-16 Thread 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 000..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 jla...@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 p...@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
 

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-08-16 Thread 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 000..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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-08-15 Thread 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

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html