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

From: Jens Lehmann <jens.lehmann@web.de>
Date: Aug 16, 2013, 06:48 UTC
Message-ID: <520DCB4B.6090309@web.de>
In-Reply-To: <277BEB82-D618-48D9-A276-4B0E76A11A38@eyesopen.com>

Am 16.08.2013 03:51, schrieb Jharrod LaFon:

Show 12 quoted lines

> 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 ...

Show 19 quoted lines

> 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")) {
>