[PATCH] 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 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
Re: [PATCH] Git segmentation faults if submodule path is empty.
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.
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.
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 http://vger.kernel.org/majordomo
Re: [PATCH] Git segmentation faults if submodule path is empty.
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
