[BUG] realloc failed

[BUG] realloc failed

[Kazuki Tsujimoto]

The following command causes "fatal: Out of memory, realloc failed" error.

$ ./git --version
git version 1.7.5.GIT

$ cat ~/.gitconfig
[alias]
    a = -c n=v status

$ MALLOC_CHECK_=0 ./git a
fatal: Out of memory, realloc failed

$ gdb --args ./git a
(gdb) run
*** glibc detected *** /tmp/git/git: realloc(): invalid pointer: 0x00000000007cd328 ***
...snip...
(gdb) bt
#0  0x00007ffff72c8a75 in raise () from /lib/libc.so.6
#1  0x00007ffff72cc5c0 in abort () from /lib/libc.so.6
#2  0x00007ffff73024fb in ?? () from /lib/libc.so.6
#3  0x00007ffff730c5b6 in ?? () from /lib/libc.so.6
#4  0x00007ffff73132e2 in realloc () from /lib/libc.so.6
#5  0x0000000000510bfb in xrealloc (ptr=0x7cd328, size=16) at wrapper.c:82
#6  0x0000000000405013 in handle_alias (argcp=0x7fffffffdfdc, argv=0x7fffffffdfd0) at git.c:236
#7  0x000000000040550a in run_argv (argcp=0x7fffffffdfdc, argv=0x7fffffffdfd0) at git.c:515
#8  0x000000000040566a in main (argc=1, argv=0x7fffffffe0f0) at git.c:579


When the "-c" option is specified, setenv will be called in git_config_push_parameter.
So it seems "envchanged" flag must be set to true in this case.

 git.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/git.c b/git.c
index a5ef3c6..e04e4d4 100644
--- a/git.c
+++ b/git.c
@@ -153,6 +153,8 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 				usage(git_usage_string);
 			}
 			git_config_push_parameter((*argv)[1]);
+			if (envchanged)
+				*envchanged = 1;
 			(*argv)++;
 			(*argc)--;
 		} else {

After applying this patch, it works.

$ ./git a
fatal: alias 'a' changes environment variables
You can use '!git' in the alias to do this.

$ vi ~/.gitconfig
[alias]
    a = !git -c n=v status
        ~~~~

$ ./git a
# On branch master
nothing to commit (working directory clean)

-- 
Kazuki Tsujimoto

Re: [BUG] realloc failed

[Junio C Hamano]

Kazuki Tsujimoto <kazuki@callcc.net> writes:

> The following command causes "fatal: Out of memory, realloc failed" error.
> ...

Care to send a patch with test (see Documentation/SubmittingPatches)?

Thanks.

Re: [BUG] realloc failed

[Kazuki Tsujimoto]

From: Junio C Hamano <gitster@pobox.com>
Date: Fri, 20 May 2011 18:35:58 -0700

> Care to send a patch with test (see Documentation/SubmittingPatches)?

Thanks for your comment.
Here is a patch with test.


[PATCH] Avoid "realloc failed" error in alias expansion including -c option

When the -c option is specified, setenv will be called.
Therefore, set envchanged flag to true so that git exits with
"alias '%s' changes environment variables" error.

Signed-off-by: Kazuki Tsujimoto <kazuki@callcc.net>
---
 git.c                   |    2 ++
 t/t1020-subdirectory.sh |   15 +++++++++++++++
 2 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/git.c b/git.c
index a5ef3c6..e04e4d4 100644
--- a/git.c
+++ b/git.c
@@ -153,6 +153,8 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 				usage(git_usage_string);
 			}
 			git_config_push_parameter((*argv)[1]);
+			if (envchanged)
+				*envchanged = 1;
 			(*argv)++;
 			(*argc)--;
 		} else {
diff --git a/t/t1020-subdirectory.sh b/t/t1020-subdirectory.sh
index ddc3921..94fed9c 100755
--- a/t/t1020-subdirectory.sh
+++ b/t/t1020-subdirectory.sh
@@ -119,6 +119,21 @@ test_expect_success 'alias expansion' '
 	)
 '
 
+test_expect_success 'alias expansion including "-c <name>=<value>" option' '
+	printf "%s\n%s\n" \
+	       "fatal: alias '"'ss'"' changes environment variables" \
+	       "You can use '"'!git'"' in the alias to do this.">expect &&
+	(
+		cat > .git/config <<EOF &&
+[alias]
+ss = -c name=value status
+EOF
+		cd dir &&
+		test_must_fail git ss 2> ../actual
+	) &&
+	test_cmp expect actual
+'
+
 test_expect_success '!alias expansion' '
 	pwd >expect &&
 	(


-- 
Kazuki Tsujimoto

Re: [BUG] realloc failed

[Junio C Hamano]

Kazuki Tsujimoto <kazuki@callcc.net> writes:

> From: Junio C Hamano <gitster@pobox.com>
> Date: Fri, 20 May 2011 18:35:58 -0700
>
>> Care to send a patch with test (see Documentation/SubmittingPatches)?
>
> Thanks for your comment.
> Here is a patch with test.
>
> [PATCH] Avoid "realloc failed" error in alias expansion including -c option

Just for your future reference.

Retitle the "Subject:" to the above line, and move everything above after
the "---" line. Another style that is usable in a discussion thread is to
use a scissors-line "-- >8 --" to say "ignore everything above this line"
(I'll use this message as a demonstration), but we didn't have a long
thread here, so just a straight patch with comment after "---" is preferred
in this case.

>
> When the -c option is specified, setenv will be called.
> Therefore, set envchanged flag to true so that git exits with
> "alias '%s' changes environment variables" error.
>
> Signed-off-by: Kazuki Tsujimoto <kazuki@callcc.net>
> ---
>  git.c                   |    2 ++
>  t/t1020-subdirectory.sh |   15 +++++++++++++++
>  2 files changed, 17 insertions(+), 0 deletions(-)
>
> diff --git a/git.c b/git.c
> index a5ef3c6..e04e4d4 100644
> --- a/git.c
> +++ b/git.c
> @@ -153,6 +153,8 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
>  				usage(git_usage_string);
>  			}
>  			git_config_push_parameter((*argv)[1]);
> +			if (envchanged)
> +				*envchanged = 1;
>  			(*argv)++;
>  			(*argc)--;
>  		} else {

Sorry, but I have to say that you are solving a wrong problem with a wrong
approach.

The realloc() failure you saw is caused by this sequence:

        option_count = handle_options(&new_argv, &count, &envchanged);
        ...
        new_argv -= option_count;
        ...
        new_argv = xrealloc(new_argv, sizeof(char *) * (count + *argcp));

The "handle_options()" function advances new_argv, trying to strip the
earlier items it recognized, and indicates how many it stripped by its
return value. When the caller wants to resize the arguments array, it
no longer points at the beginning of the buffer returned by malloc(),
and compensates what handle_options() did by subtracting option_count
before calling realloc().

The immediate cause of the problem is that handle_options() returns a
wrong count when it sees a "-c <config-specification>". If you look at its
implementation, you see "handled++" paired with "(*argv)++" everywhere but
the place it parsed "-c".

There is no inherent reason to forbid "-c <config-specification>" in
the alias, e.g. "-c ui.color=no diff". Your patch may avoid the failure
from the realloc, but breaks uses of such aliases.

The "envchanged" thing is there to avoid a situation like this:

 * You have an alias "foo" in .git/config;

 * You run "git --git-dir=/some/where/else foo";

 * We may read that "foo" alias from .git/config in your current
   directory, or we may notice the "--git-dir" on the command line, and
   may not to even read from that particular .git/config file that has
   "foo". Depending on the implementation of git (and its vintage), this
   will lead to an unexpected behaviour.

So a patch to fix the "immediate cause" would probably look like this
instead.

-- >8 --
Subject: handle_options(): do not miscount how many arguments were used

The handle_options() function advances the base of the argument array and
returns the number of arguments it used. The caller in handle_alias()
wants to reallocate the argv array it passes to this function, and
attempts to do so by subtracting the returned value to compensate for the
change handle_options() makes to the new_argv.

But handle_options() did not correctly count when "-c <config=value>" is
given, causing a wrong pointer to be passed to realloc().

Fix it by saving the original argv at the beginning of handle_options(),
and return the difference between the final value of argv, which will
relieve the places that move the array pointer from the additional burden
of keeping track of "handled" counter.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/git.c b/git.c
index ef598c3..df4306d 100644
--- a/git.c
+++ b/git.c
@@ -66,7 +66,7 @@ static void commit_pager_choice(void) {
 
 static int handle_options(const char ***argv, int *argc, int *envchanged)
 {
-	int handled = 0;
+	const char **orig_argv = *argv;
 
 	while (*argc > 0) {
 		const char *cmd = (*argv)[0];
@@ -116,7 +116,6 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 				*envchanged = 1;
 			(*argv)++;
 			(*argc)--;
-			handled++;
 		} else if (!prefixcmp(cmd, "--git-dir=")) {
 			setenv(GIT_DIR_ENVIRONMENT, cmd + 10, 1);
 			if (envchanged)
@@ -156,9 +155,8 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 
 		(*argv)++;
 		(*argc)--;
-		handled++;
 	}
-	return handled;
+	return (*argv) - orig_argv;
 }
 
 static int handle_alias(int *argcp, const char ***argv)

Re: [BUG] realloc failed

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> The immediate cause of the problem is that handle_options() returns a
> wrong count when it sees a "-c <config-specification>". If you look at its
> implementation, you see "handled++" paired with "(*argv)++" everywhere but
> the place it parsed "-c".

I said "immediate" above, because I think the "real" reason why we
suffered is because handle_options() helper function was created by
refactoring existing code with a poor taste in designing an API.

It is understandable that it is more convenent for one callsite, which is
git.c::main(), to make the helper automatically move the argc/argv
variables of the caller, and it certainly would have made the initial
patch look smaller when the helper was introduced, but as an API for a
more general callers, it is easier to follow the resulting code of the
caller if it didn't.

Refactored in a more reasonable way, the result may look like this, on top
of the previous patch.

*CAUTION* I was not very careful checking sites that increment these
variables and rely on the result when I was doing this, so there may be
places that need to be adjusted by adding or subtracting option_count)


 git.c |   51 +++++++++++++++++++++++++++------------------------
 1 files changed, 27 insertions(+), 24 deletions(-)

diff --git a/git.c b/git.c
index df4306d..3596f5b 100644
--- a/git.c
+++ b/git.c
@@ -64,12 +64,12 @@ static void commit_pager_choice(void) {
 	}
 }
 
-static int handle_options(const char ***argv, int *argc, int *envchanged)
+static int handle_options(int argc, const char **argv, int *envchanged)
 {
-	const char **orig_argv = *argv;
+	const char **orig_argv = argv;
 
-	while (*argc > 0) {
-		const char *cmd = (*argv)[0];
+	while (argc > 0) {
+		const char *cmd = argv[0];
 		if (cmd[0] != '-')
 			break;
 
@@ -107,29 +107,29 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 			if (envchanged)
 				*envchanged = 1;
 		} else if (!strcmp(cmd, "--git-dir")) {
-			if (*argc < 2) {
+			if (argc < 2) {
 				fprintf(stderr, "No directory given for --git-dir.\n" );
 				usage(git_usage_string);
 			}
-			setenv(GIT_DIR_ENVIRONMENT, (*argv)[1], 1);
+			setenv(GIT_DIR_ENVIRONMENT, argv[1], 1);
 			if (envchanged)
 				*envchanged = 1;
-			(*argv)++;
-			(*argc)--;
+			argv++;
+			argc--;
 		} else if (!prefixcmp(cmd, "--git-dir=")) {
 			setenv(GIT_DIR_ENVIRONMENT, cmd + 10, 1);
 			if (envchanged)
 				*envchanged = 1;
 		} else if (!strcmp(cmd, "--work-tree")) {
-			if (*argc < 2) {
+			if (argc < 2) {
 				fprintf(stderr, "No directory given for --work-tree.\n" );
 				usage(git_usage_string);
 			}
-			setenv(GIT_WORK_TREE_ENVIRONMENT, (*argv)[1], 1);
+			setenv(GIT_WORK_TREE_ENVIRONMENT, argv[1], 1);
 			if (envchanged)
 				*envchanged = 1;
-			(*argv)++;
-			(*argc)--;
+			argv++;
+			argc--;
 		} else if (!prefixcmp(cmd, "--work-tree=")) {
 			setenv(GIT_WORK_TREE_ENVIRONMENT, cmd + 12, 1);
 			if (envchanged)
@@ -141,22 +141,22 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 			if (envchanged)
 				*envchanged = 1;
 		} else if (!strcmp(cmd, "-c")) {
-			if (*argc < 2) {
+			if (argc < 2) {
 				fprintf(stderr, "-c expects a configuration string\n" );
 				usage(git_usage_string);
 			}
-			git_config_push_parameter((*argv)[1]);
-			(*argv)++;
-			(*argc)--;
+			git_config_push_parameter(argv[1]);
+			argv++;
+			argc--;
 		} else {
 			fprintf(stderr, "Unknown option: %s\n", cmd);
 			usage(git_usage_string);
 		}
 
-		(*argv)++;
-		(*argc)--;
+		argv++;
+		argc--;
 	}
-	return (*argv) - orig_argv;
+	return argv - orig_argv;
 }
 
 static int handle_alias(int *argcp, const char ***argv)
@@ -198,14 +198,14 @@ static int handle_alias(int *argcp, const char ***argv)
 		if (count < 0)
 			die("Bad alias.%s string: %s", alias_command,
 			    split_cmdline_strerror(count));
-		option_count = handle_options(&new_argv, &count, &envchanged);
+		option_count = handle_options(count, new_argv, &envchanged);
+		count -= option_count;
 		if (envchanged)
 			die("alias '%s' changes environment variables\n"
 				 "You can use '!git' in the alias to do this.",
 				 alias_command);
-		memmove(new_argv - option_count, new_argv,
-				count * sizeof(char *));
-		new_argv -= option_count;
+		memmove(new_argv, new_argv + option_count,
+			count * sizeof(char *));
 
 		if (count < 1)
 			die("empty alias for %s", alias_command);
@@ -508,6 +508,7 @@ static int run_argv(int *argcp, const char ***argv)
 int main(int argc, const char **argv)
 {
 	const char *cmd;
+	int option_count;
 
 	startup_info = &git_startup_info;
 
@@ -535,7 +536,9 @@ int main(int argc, const char **argv)
 	/* Look for flags.. */
 	argv++;
 	argc--;
-	handle_options(&argv, &argc, NULL);
+	option_count = handle_options(argc, argv, NULL);
+	argv += option_count;
+	argc -= option_count;
 	if (argc > 0) {
 		if (!prefixcmp(argv[0], "--"))
 			argv[0] += 2;

Re: [BUG] realloc failed

[Kazuki Tsujimoto]

Subject: Re: [BUG] realloc failed
From: Junio C Hamano <gitster@pobox.com>
Date: Sun, 22 May 2011 12:34:14 -0700

> Just for your future reference.
> ...snip...

Thanks for useful information, and reviewing the patch.
I got it.

-- 
Kazuki Tsujimoto