Re: [PATCH] git.c: fix help.autocorrect after 57ea712 breaks it

[Junio C Hamano]

Duy Nguyen  writes:

> On Wed, Jan 27, 2016 at 1:49 PM, Junio C Hamano  wrote:
>> Junio C Hamano  writes:
>>
>>> I spoke too soon, I am afraid.
>>> ...
>>> I wonder if this would be better done as a multi-part series that
>>> goes like this:
>>> ...
>>
>> So here is the first of the three-patch series to replace it.
>
> This is much better (the whole series, not just the first patch). We
> probably should add a test about help.autocorrect though, maybe in the
> first patch, because it's not tested at all in the test suite.

Patches welcome.  Thanks.
--
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.c: fix help.autocorrect after 57ea712 breaks it

[Duy Nguyen]

On Wed, Jan 27, 2016 at 1:49 PM, Junio C Hamano  wrote:
> Junio C Hamano  writes:
>
>> I spoke too soon, I am afraid.
>> ...
>> I wonder if this would be better done as a multi-part series that
>> goes like this:
>> ...
>
> So here is the first of the three-patch series to replace it.

This is much better (the whole series, not just the first patch). We
probably should add a test about help.autocorrect though, maybe in the
first patch, because it's not tested at all in the test suite.
-- 
Duy
--
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.c: fix help.autocorrect after 57ea712 breaks it

[Junio C Hamano]

Junio C Hamano  writes:

> I spoke too soon, I am afraid.
> ...
> I wonder if this would be better done as a multi-part series that
> goes like this:
> ...

So here is the first of the three-patch series to replace it.

-- >8 --
From: Junio C Hamano 
Date: Tue, 26 Jan 2016 11:46:53 -0800
Subject: [PATCH 1/3] git: remove an early return from save_env_before_alias()

When help.autocorrect is in effect, an attempt to auto-execute an
uniquely corrected result of a misspelled alias will result in an
irrelevant error message.  The codepath that causes this issue calls
save_env_before_alias() and restore_env() as a pair in handle_alias(),
and that happens twice.  A global variable orig_cwd is allocated and
holds the return value of getcwd() in save_env_before_alias(), which
is used in restore_env() to go back to that directory and freed.

However, save_env_before_alias() is not prepared to be called twice.
It returns early when it knows it has already been called, leaving
orig_cwd undefined, which is then checked in the second call to
restore_env(), and by that time, the memory that used to hold the
contents of orig_cwd is either freed or reused to hold something else,
and this is fed to chdir(), causing it to fail.

Fix this by making sure save_env() does do the saving when called.

Signed-off-by: Junio C Hamano 
---
 git.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/git.c b/git.c
index 98d4412..a57a4cb 100644
--- a/git.c
+++ b/git.c
@@ -30,8 +30,6 @@ static int saved_env_before_alias;
 static void save_env_before_alias(void)
 {
int i;
-   if (saved_env_before_alias)
-   return;
saved_env_before_alias = 1;
orig_cwd = xgetcwd();
for (i = 0; i < ARRAY_SIZE(env_names); i++) {
-- 
2.7.0-366-g065efda

--
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.c: fix help.autocorrect after 57ea712 breaks it

[Michael J Gruber]

Nguyễn Thái Ngọc Duy venit, vidit, dixit 26.01.2016 14:26:
> Commit 57ea712 (git.c: make sure we do not leak GIT_* to alias scripts -
> 2015-12-20) does not realize that handle_alias() can be called multiple
> times because of the forever loop in run_argv(). The commit breaks alias
> chains.
> 
> Suppose you have an alias "abc" that resolves to another alias "def",
> which finally resolve to "git status". handle_alias() is called twice.
> save_env() and restore_env() are also called twice. But because of the
> check save_env_before_alias in save_env(), we save once while trying to
> restore twice. Consequences are left for the reader's imagination.
> 
> Fortunately, you cannot make an alias of another alias. At least not
> yet. Unfortunately it can still happen with help.autocorrect, where your
> alias typo is treated as the first "alias", and it can be resolved to
> the second alias. Then boom.
> 
> Make sure we call save_env() and restore_env() in pairs. While at there,
> set orig_cwd to NULL after freeing it for hygiene.
> 
> Reported-by: Michael J Gruber 
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---

The patch fixes it for me, thanks!

Michael
--
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.c: fix help.autocorrect after 57ea712 breaks it

[Junio C Hamano]

Michael J Gruber  writes:

> Nguyễn Thái Ngọc Duy venit, vidit, dixit 26.01.2016 14:26:
>> Commit 57ea712 (git.c: make sure we do not leak GIT_* to alias scripts -
>> 2015-12-20) does not realize that handle_alias() can be called multiple
>> times because of the forever loop in run_argv(). The commit breaks alias
>> chains.
>> 
>> Suppose you have an alias "abc" that resolves to another alias "def",
>> which finally resolve to "git status". handle_alias() is called twice.
>> save_env() and restore_env() are also called twice. But because of the
>> check save_env_before_alias in save_env(), we save once while trying to
>> restore twice. Consequences are left for the reader's imagination.
>> 
>> Fortunately, you cannot make an alias of another alias. At least not
>> yet. Unfortunately it can still happen with help.autocorrect, where your
>> alias typo is treated as the first "alias", and it can be resolved to
>> the second alias. Then boom.
>> 
>> Make sure we call save_env() and restore_env() in pairs. While at there,
>> set orig_cwd to NULL after freeing it for hygiene.
>> 
>> Reported-by: Michael J Gruber 
>> Signed-off-by: Nguyễn Thái Ngọc Duy 
>> ---
>
> The patch fixes it for me, thanks!
>
> Michael

Thanks for noticing, and thanks for describing the problem very
clearly and fixing it quickly.

--
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.c: fix help.autocorrect after 57ea712 breaks it

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   writes:

> diff --git a/git.c b/git.c
> index da278c3..cd733f7 100644
> --- a/git.c
> +++ b/git.c
> @@ -25,14 +25,15 @@ static const char *env_names[] = {
>   GIT_PREFIX_ENVIRONMENT
>  };
>  static char *orig_env[4];
> -static int saved_env_before_alias;
> +static int saved_env_before_alias, saved;

Even for a file-local static, this name is a bit too generic, isn't
it?  saved_env_count or something perhaps?


>  static void save_env_before_alias(void)
>  {
>   int i;
> - if (saved_env_before_alias)
> - return;
> + if (saved)
> + die("BUG: uneven pair of save_env/restore_env calls");
>   saved_env_before_alias = 1;
> + saved = 1;
>   orig_cwd = xgetcwd();
>   for (i = 0; i < ARRAY_SIZE(env_names); i++) {
>   orig_env[i] = getenv(env_names[i]);
> @@ -44,9 +45,13 @@ static void save_env_before_alias(void)
>  static void restore_env(int external_alias)
>  {
>   int i;
> + if (saved != 1)
> + die("BUG: uneven pair of save_env/restore_env calls");
>   if (!external_alias && orig_cwd && chdir(orig_cwd))
>   die_errno("could not move to %s", orig_cwd);
>   free(orig_cwd);
> + orig_cwd = NULL;
> + saved = 0;
>   for (i = 0; i < ARRAY_SIZE(env_names); i++) {
>   if (external_alias &&
>   !strcmp(env_names[i], GIT_PREFIX_ENVIRONMENT))
> diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
> index 52678e7..3f95285 100755
> --- a/t/t1300-repo-config.sh
> +++ b/t/t1300-repo-config.sh
> @@ -1201,4 +1201,12 @@ test_expect_success POSIXPERM,PERL 'preserves existing 
> permissions' '
> "die q(badrename) if ((stat(q(.git/config)))[2] & 0) != 0600"
>  '
>  
> +test_expect_success 'autocorrect and save_env/restore_env' '
> + git config alias.ss status &&
> + git config help.autocorrect 1 &&
> + git sss --porcelain | grep actual >actual &&
> + echo "?? actual" >expected &&
> + test_cmp expected actual
> +'
> +
>  test_done
--
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.c: fix help.autocorrect after 57ea712 breaks it

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   writes:

> Commit 57ea712 (git.c: make sure we do not leak GIT_* to alias scripts -
> 2015-12-20) does not realize that handle_alias() can be called multiple
> times because of the forever loop in run_argv(). The commit breaks alias
> chains.
>
> Suppose you have an alias "abc" that resolves to another alias "def",
> which finally resolve to "git status". handle_alias() is called twice.
> save_env() and restore_env() are also called twice. But because of the
> check save_env_before_alias in save_env(), we save once while trying to
> restore twice. Consequences are left for the reader's imagination.
>
> Fortunately, you cannot make an alias of another alias. At least not
> yet. Unfortunately it can still happen with help.autocorrect, where your
> alias typo is treated as the first "alias", and it can be resolved to
> the second alias. Then boom.
>
> Make sure we call save_env() and restore_env() in pairs. While at there,
> set orig_cwd to NULL after freeing it for hygiene.
>
> Reported-by: Michael J Gruber 
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---

I spoke too soon, I am afraid.

The log message talks about "we save once while trying to restore
twice" being bad, and the fix is to make sure we save and restore in
pairs.  Before or after the patch, however, there is no change in
the callers of save_env_before_alias() and restore_env(), and it is
unclear how that is ensured.

It turns out that the callers are doing the right thing (assuming
that calling save and restore in pairs is the right solution), and
the culprit is in the save_env_before_alias() function that returns
without saving when we were called.  That is mentioned in the log,
but the description of last paragraph leaves the impression that it
was callers that needed fixing, which is misleading.

After the patch, we have "saved" and "saved_env_before_alias".

 - The former is a protection against committing similar programming
   errors in the future (i.e. glorified assert()).  Can we have it
   in a separate commit, perhaps on top of the real fix, to make the
   change for the real fix easier to understand?

 - Do we still need the latter?  saved_env_before_alias is set to 1
   if we called save_env_before_alias() even once, which happens
   always (and only) at the beginning of handle_alias(), so that is
   equivalent to saying "have we ever called handle_alias()?"

   And there is only one caller of handle_alias, in run_argv(), and
   it maintains yet another variable "done_alias" to keep track of
   the same information.

   The only code that cares about saved_env_before_alias is
   handle_builtin(), but it is a glorified no-op if
   saved_env_before_alias() is set.  And the only caller of this
   handle_builtin(), for which saved_env_before_alias matters, is
   the same run_argv().

I wonder if this would be better done as a multi-part series that
goes like this:

 [1/3] git: remove an early return from save_env_before_alias()

   whose description would be the first two paragraphs of your
   patch, with two line removal, i.e.

diff --git a/git.c b/git.c
index 98d4412..a57a4cb 100644
--- a/git.c
+++ b/git.c
@@ -30,8 +30,6 @@ static int saved_env_before_alias;
 static void save_env_before_alias(void)
 {
int i;
-   if (saved_env_before_alias)
-   return;
saved_env_before_alias = 1;
orig_cwd = xgetcwd();
for (i = 0; i < ARRAY_SIZE(env_names); i++) {

   that should be a sufficient "fix" to the issue.

 [2/3] git: protect against unbalanced calls to {save,restore}_env()

   We made sure that save_env_before_alias() does not skip
   saving the environment when asked to (which led to double
   free of orig_cwd in restore_env()) with the previous step.
   Protect against future breakage where somebody adds new
   callers of these functions in an unbalanced fashion.
   
diff --git a/git.c b/git.c
index a57a4cb..e39b972 100644
--- a/git.c
+++ b/git.c
@@ -26,11 +26,15 @@ static const char *env_names[] = {
 };
 static char *orig_env[4];
 static int saved_env_before_alias;
+static int save_restore_env_balance;

 static void save_env_before_alias(void)
 {
int i;
saved_env_before_alias = 1;
+
+   assert(save_restore_env_balance == 0);
+   save_restore_env_balance = 1;
orig_cwd = xgetcwd();
for (i = 0; i < ARRAY_SIZE(env_names); i++) {
orig_env[i] = getenv(env_names[i]);
@@ -42,6 +46,9 @@ static void save_env_before_alias(void)
 static void restore_env(int external_alias)
 {
int i;
+
+   assert(save_restore_env_balance