[PATCH] remote.c: guard config parser from value=NULL

[PATCH] remote.c: guard config parser from value=NULL

[Govind Salinas]

Note:Moved multiple checks for null to one place at the top of the function.
Comment in this section conflicted with Junio's request to remove this
behavior.

Signed-off-by: Govind Salinas <blix@sophiasuchtig.com>
---
 remote.c |   19 ++-----------------
 1 files changed, 2 insertions(+), 17 deletions(-)

diff --git a/remote.c b/remote.c
index 0e00680..27cdaa7 100644
--- a/remote.c
+++ b/remote.c
@@ -217,13 +217,13 @@ static int handle_config(const char *key, const
char *value)
 	const char *subkey;
 	struct remote *remote;
 	struct branch *branch;
+	if (!value)
+		return 0;
 	if (!prefixcmp(key, "branch.")) {
 		name = key + 7;
 		subkey = strrchr(name, '.');
 		if (!subkey)
 			return 0;
-		if (!value)
-			return 0;
 		branch = make_branch(name, subkey - name);
 		if (!strcmp(subkey, ".remote")) {
 			branch->remote_name = xstrdup(value);
@@ -244,21 +244,6 @@ static int handle_config(const char *key, const
char *value)
 		return 0;
 	}
 	remote = make_remote(name, subkey - name);
-	if (!value) {
-		/* if we ever have a boolean variable, e.g. "remote.*.disabled"
-		 * [remote "frotz"]
-		 *      disabled
-		 * is a valid way to set it to true; we get NULL in value so
-		 * we need to handle it here.
-		 *
-		 * if (!strcmp(subkey, ".disabled")) {
-		 *      val = git_config_bool(key, value);
-		 *      return 0;
-		 * } else
-		 *
-		 */
-		return 0; /* ignore unknown booleans */
-	}
 	if (!strcmp(subkey, ".url")) {
 		add_url(remote, xstrdup(value));
 	} else if (!strcmp(subkey, ".push")) {
-- 
1.5.4.36.g9af61

[Janitors] value could be NULL in config parser

[Junio C Hamano]

If somebody wants to dip his or her toe in git hacking, and is
tempted to send in a "clean up" patch (e.g. whitespace, coding
style) that does not really _fix_ anything, please don't.

I have a task of similar complexity (meaning, reasonably easy)
that is much more useful and appreciated than clean-up patches
for you.

The callback functions that are passed to git_config() need to
be audited so that they do not barf when given NULL.  Currently,
many of them are not safe.

A callback function of git_config() is called when the command
reads value from .git/config and friends.  The function takes
two parameters, var and value.  var is never NULL and it is the
name of the configuration variable found in the file being
read.  value could be either string or NULL.

A NULL value is boolean "true".  For example, on MS-DOS, you may
have something like this:

	[core]
 		autocrlf

and your callback will be called with var = "core.autocrlf" and
value = NULL in such a case.

If you want to fix them (you do not have to do all of them, and
if you would like to help, please make one patch per function
fixed), the procedure is:

 (1) Find calling sites for git_config().  For example, we find
     one in archive-tar.c::write_tar_archive().

        int write_tar_archive(struct archiver_args *args)
        {
                int plen = args->base ? strlen(args->base) : 0;

                git_config(git_tar_config);

                archive_time = args->time;
                verbose = args->verbose;
	...

 (2) Look at the function that is passed to git_config().

        static int git_tar_config(const char *var, const char *value)
        {
                if (!strcmp(var, "tar.umask")) {
                        if (!strcmp(value, "user")) {
                                tar_umask = umask(0);
                                umask(tar_umask);
                        } else {
                                tar_umask = git_config_int(var, value);
                        }
                        return 0;
                }
                return git_default_config(var, value);
        }

 (3) Let's fix it.  If the user's configuration has:

	[tar]
        	umask

     it is an illegal configuration, but the code above does not
     check for NULL, and the second strcmp() would fail.  If we
     guard that strcmp() with a check against NULL, we would be
     Ok.  git_config_int() will correctly barf telling the user
     that "tar.umask" configuration is wrong.

 (4) Then send in a patch.  Again, one patch per fixed function,
     please.  The message may look like this:

-- >8 --
[PATCH] archive-tar.c: guard config parser from value=NULL

Signed-off-by: A U Thor <author@example.com>

 archive-tar.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/archive-tar.c b/archive-tar.c
index e1bced5..30aa2e2 100644
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -222,7 +222,7 @@ static void write_global_extended_header(const unsigned char *sha1)
 static int git_tar_config(const char *var, const char *value)
 {
 	if (!strcmp(var, "tar.umask")) {
-		if (!strcmp(value, "user")) {
+		if (value && !strcmp(value, "user")) {
 			tar_umask = umask(0);
 			umask(tar_umask);
 		} else {

[PATCH] remote.c: guard config parser from value=NULL

[Miklos Vajna]

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 remote.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/remote.c b/remote.c
index 0e00680..4765815 100644
--- a/remote.c
+++ b/remote.c
@@ -276,7 +276,7 @@ static int handle_config(const char *key, const char *value)
 		else
 			error("more than one uploadpack given, using the first");
 	} else if (!strcmp(subkey, ".tagopt")) {
-		if (!strcmp(value, "--no-tags"))
+		if (value && !strcmp(value, "--no-tags"))
 			remote->fetch_tags = -1;
 	} else if (!strcmp(subkey, ".proxy")) {
 		remote->http_proxy = xstrdup(value);
-- 
1.5.4

Re: [PATCH] remote.c: guard config parser from value=NULL

[Michele Ballabio]

On Friday 08 February 2008, Miklos Vajna wrote:
> Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
> ---
>  remote.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/remote.c b/remote.c
> index 0e00680..4765815 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -276,7 +276,7 @@ static int handle_config(const char *key, const char *value)
>  		else
>  			error("more than one uploadpack given, using the first");
>  	} else if (!strcmp(subkey, ".tagopt")) {
> -		if (!strcmp(value, "--no-tags"))
> +		if (value && !strcmp(value, "--no-tags"))
>  			remote->fetch_tags = -1;
>  	} else if (!strcmp(subkey, ".proxy")) {
>  		remote->http_proxy = xstrdup(value);

Function handle_config() has already returned 0 at this point.