Re: [BUG] realloc failed

From: Junio C Hamano <gitster@pobox.com>
Date: May 22, 2011, 19:34 UTC
Message-ID: <7v8vtyy1ft.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20110521145056.E3F5.BA9123DE@callcc.net>

Kazuki Tsujimoto <kazuki@callcc.net> writes:

Show 9 quoted lines

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

Show 24 quoted lines

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