git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCH] rebase: exec leaks GIT_DIR to environment
@ 2017-10-28  0:01 Jacob Keller
  2017-10-28 16:00 ` Johannes Schindelin
  0 siblings, 1 reply; 12+ messages in thread
From: Jacob Keller @ 2017-10-28  0:01 UTC (permalink / raw)
  To: git; +Cc: Jacob Keller

From: Jacob Keller <jacob.keller@gmail.com>

I noticed a failure with git rebase interactive mode which causes "exec"
commands to be run with GIT_DIR set. When GIT_DIR is in the environment,
then any command which results in running a git command in
a subdirectory will fail because GIT_DIR=".git".

This unfortunately breaks one of my project's Makefiles, which uses
git-describe to find the version information, but does so from within
a sub directory.

I'm in the process of running a bisect to find where this got
introduced, but I suspect it's part of the rebase--helper changes that
happened a while ago.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 t/t3404-rebase-interactive.sh | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 3704dbb2ecf6..60ab5136f702 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -108,6 +108,17 @@ test_expect_success 'rebase -i with the exec command runs from tree root' '
 	rm -fr subdir
 '
 
+test_expect_failure 'rebase -i with the exec git commands in subdirs still work' '
+	test_when_finished "rm -ff subdir" &&
+	test_when_finished "git rebase --abort" &&
+	git checkout master &&
+	mkdir subdir && (cd subdir &&
+	set_fake_editor &&
+	FAKE_LINES="1 exec_>cd_subdir_&&_git_rev-parse_--is-inside-work-tree" \
+		git rebase -i HEAD^
+	)
+'
+
 test_expect_success 'rebase -i with the exec command checks tree cleanness' '
 	git checkout master &&
 	set_fake_editor &&
-- 
2.11.1.4.gad8c7cd


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] rebase: exec leaks GIT_DIR to environment
  2017-10-28  0:01 [PATCH] rebase: exec leaks GIT_DIR to environment Jacob Keller
@ 2017-10-28 16:00 ` Johannes Schindelin
  2017-10-28 22:35   ` Jacob Keller
  2017-10-29 18:34   ` Phillip Wood
  0 siblings, 2 replies; 12+ messages in thread
From: Johannes Schindelin @ 2017-10-28 16:00 UTC (permalink / raw)
  To: Jacob Keller; +Cc: git, Jacob Keller

Hi Jake,

On Fri, 27 Oct 2017, Jacob Keller wrote:

> From: Jacob Keller <jacob.keller@gmail.com>
> 
> I noticed a failure with git rebase interactive mode which causes "exec"
> commands to be run with GIT_DIR set. When GIT_DIR is in the environment,
> then any command which results in running a git command in
> a subdirectory will fail because GIT_DIR=".git".
> 
> This unfortunately breaks one of my project's Makefiles, which uses
> git-describe to find the version information, but does so from within
> a sub directory.
> 
> I'm in the process of running a bisect to find where this got
> introduced, but I suspect it's part of the rebase--helper changes that
> happened a while ago.

A safe assumption. I do not know how the shell code managed that GIT_DIR
reset, though:

-- snip from v2.12.0's git-rebase--interactive.sh --
        x|"exec")
                read -r command rest < "$todo"
                mark_action_done
                eval_gettextln "Executing: \$rest"
                "${SHELL:-@SHELL_PATH@}" -c "$rest" # Actual execution
-- snap --

Maybe you can spot it?

The fix should be as easy as

-- snip --
diff --git a/sequencer.c b/sequencer.c
index f2c84c2fa62..018ba8d27e2 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1859,11 +1859,13 @@ static int error_failed_squash(struct commit *commit,
 static int do_exec(const char *command_line)
 {
 	const char *child_argv[] = { NULL, NULL };
+	const char *child_env[] = { "GIT_DIR", NULL };
 	int dirty, status;
 
 	fprintf(stderr, "Executing: %s\n", command_line);
 	child_argv[0] = command_line;
-	status = run_command_v_opt(child_argv, RUN_USING_SHELL);
+	status = run_command_v_opt_cd_env(child_argv, RUN_USING_SHELL, NULL,
+					  child_env);
 
 	/* force re-reading of the cache */
 	if (discard_cache() < 0 || read_cache() < 0)
-- snap --

*However*, your test still fails with this, as

- your added test tries to remove the directory with -ff instead of -rf

- it tries to run `git rebase --abort` afterwards, which fails with my fix
  because there is no rebase in progress

- instead of `cd subdir && ...`, it calls `>cd subdir && ...`, which
  causes it to abort with a "subdir: not fonud"

So I need this, too, to make it all work:

-- snip --
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 60ab5136f70..967caab222a 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -108,13 +108,13 @@ test_expect_success 'rebase -i with the exec command runs from tree root' '
 	rm -fr subdir
 '
 
-test_expect_failure 'rebase -i with the exec git commands in subdirs still work' '
-	test_when_finished "rm -ff subdir" &&
-	test_when_finished "git rebase --abort" &&
+test_expect_success 'rebase -i with the exec git commands in subdirs still work' '
+	test_when_finished "rm -rf subdir" &&
+	test_when_finished "git rebase --abort || :" &&
 	git checkout master &&
 	mkdir subdir && (cd subdir &&
 	set_fake_editor &&
-	FAKE_LINES="1 exec_>cd_subdir_&&_git_rev-parse_--is-inside-work-tree" \
+	FAKE_LINES="1 exec_cd_subdir_&&_git_rev-parse_--is-inside-work-tree" \
 		git rebase -i HEAD^
 	)
 '
-- snap --

I only had time to write these two patches, and to verify that t3404
passes now, but not that anything else passes, neither to write a proper
commit message.

Maybe you can take it from there?

Ciao,
Dscho

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] rebase: exec leaks GIT_DIR to environment
  2017-10-28 16:00 ` Johannes Schindelin
@ 2017-10-28 22:35   ` Jacob Keller
  2017-10-29 18:34   ` Phillip Wood
  1 sibling, 0 replies; 12+ messages in thread
From: Jacob Keller @ 2017-10-28 22:35 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Jacob Keller, Git mailing list

On Sat, Oct 28, 2017 at 9:00 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Jake,
>
> On Fri, 27 Oct 2017, Jacob Keller wrote:
>
>> From: Jacob Keller <jacob.keller@gmail.com>
>>
>> I noticed a failure with git rebase interactive mode which causes "exec"
>> commands to be run with GIT_DIR set. When GIT_DIR is in the environment,
>> then any command which results in running a git command in
>> a subdirectory will fail because GIT_DIR=".git".
>>
>> This unfortunately breaks one of my project's Makefiles, which uses
>> git-describe to find the version information, but does so from within
>> a sub directory.
>>
>> I'm in the process of running a bisect to find where this got
>> introduced, but I suspect it's part of the rebase--helper changes that
>> happened a while ago.
>
> A safe assumption. I do not know how the shell code managed that GIT_DIR
> reset, though:
>
> -- snip from v2.12.0's git-rebase--interactive.sh --
>         x|"exec")
>                 read -r command rest < "$todo"
>                 mark_action_done
>                 eval_gettextln "Executing: \$rest"
>                 "${SHELL:-@SHELL_PATH@}" -c "$rest" # Actual execution
> -- snap --
>

>
> *However*, your test still fails with this, as
>

Yea I'm not surprised the test failed, I was in a hurry at the end of
the workday when I spotted it, but wanted to get something on the
mailing list before I left.

> - your added test tries to remove the directory with -ff instead of -rf
>
> - it tries to run `git rebase --abort` afterwards, which fails with my fix
>   because there is no rebase in progress
>
> - instead of `cd subdir && ...`, it calls `>cd subdir && ...`, which
>   causes it to abort with a "subdir: not fonud"
>
<snip>
>
> I only had time to write these two patches, and to verify that t3404
> passes now, but not that anything else passes, neither to write a proper
> commit message.
>
> Maybe you can take it from there?
>

Yep, thanks for spotting the fix!

Thanks,
Jake

> Ciao,
> Dscho

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] rebase: exec leaks GIT_DIR to environment
  2017-10-28 16:00 ` Johannes Schindelin
  2017-10-28 22:35   ` Jacob Keller
@ 2017-10-29 18:34   ` Phillip Wood
  2017-10-30  2:26     ` Junio C Hamano
  2017-10-30  2:51     ` Jacob Keller
  1 sibling, 2 replies; 12+ messages in thread
From: Phillip Wood @ 2017-10-29 18:34 UTC (permalink / raw)
  To: Johannes Schindelin, Jacob Keller; +Cc: git, Jacob Keller

On 28/10/17 17:00, Johannes Schindelin wrote:
> 
> Hi Jake,
> 
> On Fri, 27 Oct 2017, Jacob Keller wrote:
> 
>> From: Jacob Keller <jacob.keller@gmail.com>
>>
>> I noticed a failure with git rebase interactive mode which causes "exec"
>> commands to be run with GIT_DIR set. When GIT_DIR is in the environment,
>> then any command which results in running a git command in
>> a subdirectory will fail because GIT_DIR=".git".
>>
>> This unfortunately breaks one of my project's Makefiles, which uses
>> git-describe to find the version information, but does so from within
>> a sub directory.
>>
>> I'm in the process of running a bisect to find where this got
>> introduced, but I suspect it's part of the rebase--helper changes that
>> happened a while ago.
> 
> A safe assumption. I do not know how the shell code managed that GIT_DIR
> reset, though:
> 
> -- snip from v2.12.0's git-rebase--interactive.sh --
>         x|"exec")
>                 read -r command rest < "$todo"
>                 mark_action_done
>                 eval_gettextln "Executing: \$rest"
>                 "${SHELL:-@SHELL_PATH@}" -c "$rest" # Actual execution
> -- snap --
> 
> Maybe you can spot it?
> 
> The fix should be as easy as
> 
> -- snip --
> diff --git a/sequencer.c b/sequencer.c
> index f2c84c2fa62..018ba8d27e2 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1859,11 +1859,13 @@ static int error_failed_squash(struct commit *commit,
>  static int do_exec(const char *command_line)
>  {
>  	const char *child_argv[] = { NULL, NULL };
> +	const char *child_env[] = { "GIT_DIR", NULL };
>  	int dirty, status;
>  
>  	fprintf(stderr, "Executing: %s\n", command_line);
>  	child_argv[0] = command_line;
> -	status = run_command_v_opt(child_argv, RUN_USING_SHELL);
> +	status = run_command_v_opt_cd_env(child_argv, RUN_USING_SHELL, NULL,
> +					  child_env);
>  
>  	/* force re-reading of the cache */
>  	if (discard_cache() < 0 || read_cache() < 0)
> -- snap --

Just clearing GIT_DIR does not match the behavior of the shell version
(tested by passing -p to avoid rebase--helper) as that passes GIT_DIR to
exec commands if it has been explicitly set. I think that users that set
GIT_DIR on the command line would expect it to be propagated to exec
commands.

$ git rebase -px'echo $GIT_DIR' @

                                                            Merge commit
'7c2f1abd64' into phil
Executing: echo $GIT_DIR

Successfully rebased and updated refs/heads/phil.

$ env GIT_DIR=.git git rebase -px'echo $GIT_DIR' @

                                                            Merge commit
'7c2f1abd64' into phil
Executing: echo $GIT_DIR
/home/phil/Documents/src/git/.git/worktrees/git-next
Successfully rebased and updated refs/heads/phil.

> *However*, your test still fails with this, as
> 
> - your added test tries to remove the directory with -ff instead of -rf
> 
> - it tries to run `git rebase --abort` afterwards, which fails with my fix
>   because there is no rebase in progress
> 
> - instead of `cd subdir && ...`, it calls `>cd subdir && ...`, which
>   causes it to abort with a "subdir: not fonud"
> 
> So I need this, too, to make it all work:
> 
> -- snip --
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 60ab5136f70..967caab222a 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -108,13 +108,13 @@ test_expect_success 'rebase -i with the exec command runs from tree root' '
>  	rm -fr subdir
>  '
>  
> -test_expect_failure 'rebase -i with the exec git commands in subdirs still work' '
> -	test_when_finished "rm -ff subdir" &&
> -	test_when_finished "git rebase --abort" &&
> +test_expect_success 'rebase -i with the exec git commands in subdirs still work' '
> +	test_when_finished "rm -rf subdir" &&
> +	test_when_finished "git rebase --abort || :" &&
>  	git checkout master &&
>  	mkdir subdir && (cd subdir &&
>  	set_fake_editor &&
> -	FAKE_LINES="1 exec_>cd_subdir_&&_git_rev-parse_--is-inside-work-tree" \
> +	FAKE_LINES="1 exec_cd_subdir_&&_git_rev-parse_--is-inside-work-tree" \
>  		git rebase -i HEAD^
>  	)
>  '
> -- snap --
> 
> I only had time to write these two patches, and to verify that t3404
> passes now, but not that anything else passes, neither to write a proper
> commit message.
> 
> Maybe you can take it from there?
> 
> Ciao,
> Dscho
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] rebase: exec leaks GIT_DIR to environment
  2017-10-29 18:34   ` Phillip Wood
@ 2017-10-30  2:26     ` Junio C Hamano
  2017-10-30  2:53       ` Jacob Keller
  2017-10-30  2:51     ` Jacob Keller
  1 sibling, 1 reply; 12+ messages in thread
From: Junio C Hamano @ 2017-10-30  2:26 UTC (permalink / raw)
  To: Phillip Wood; +Cc: Johannes Schindelin, Jacob Keller, git, Jacob Keller

Phillip Wood <phillip.wood@talktalk.net> writes:

> Just clearing GIT_DIR does not match the behavior of the shell version
> (tested by passing -p to avoid rebase--helper) as that passes GIT_DIR to
> exec commands if it has been explicitly set. I think that users that set
> GIT_DIR on the command line would expect it to be propagated to exec
> commands.
>
> $ git rebase -px'echo $GIT_DIR' @
>
>                                                             Merge commit
> '7c2f1abd64' into phil
> Executing: echo $GIT_DIR
>
> Successfully rebased and updated refs/heads/phil.
>
> $ env GIT_DIR=.git git rebase -px'echo $GIT_DIR' @
>
>                                                             Merge commit
> '7c2f1abd64' into phil
> Executing: echo $GIT_DIR
> /home/phil/Documents/src/git/.git/worktrees/git-next
> Successfully rebased and updated refs/heads/phil.

Hmmm, I do not mess with GIT_DIR at all in my workflow, so I am
having a bit of hard time judging if this regression is serious
enough to be a release blocker.  

I'd prefer to avoid reverting the whole js/rebase-i-final topic from
'master' this late in the game, even though I do not expect we would
see the remainder of the system gets broken due to hidden dependency
on the topic, because the changes on the topic are relatively well
isolated.


570676e011
d1114d87c7
2f0e14e649
5f3108b7b6

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] rebase: exec leaks GIT_DIR to environment
  2017-10-29 18:34   ` Phillip Wood
  2017-10-30  2:26     ` Junio C Hamano
@ 2017-10-30  2:51     ` Jacob Keller
  1 sibling, 0 replies; 12+ messages in thread
From: Jacob Keller @ 2017-10-30  2:51 UTC (permalink / raw)
  To: phillip.wood; +Cc: Johannes Schindelin, Jacob Keller, Git mailing list

On Sun, Oct 29, 2017 at 11:34 AM, Phillip Wood
<phillip.wood@talktalk.net> wrote:
>
> Just clearing GIT_DIR does not match the behavior of the shell version
> (tested by passing -p to avoid rebase--helper) as that passes GIT_DIR to
> exec commands if it has been explicitly set. I think that users that set
> GIT_DIR on the command line would expect it to be propagated to exec
> commands.
>
> $ git rebase -px'echo $GIT_DIR' @
>
>                                                             Merge commit
> '7c2f1abd64' into phil
> Executing: echo $GIT_DIR
>
> Successfully rebased and updated refs/heads/phil.
>
> $ env GIT_DIR=.git git rebase -px'echo $GIT_DIR' @
>
>                                                             Merge commit
> '7c2f1abd64' into phil
> Executing: echo $GIT_DIR
> /home/phil/Documents/src/git/.git/worktrees/git-next
> Successfully rebased and updated refs/heads/phil.
>

I'm not really sure what the exact fix is, since we do want to pass in
GIT_DIR if it was passed to us, but maybe it needs to be an absolute
path?

Thanks,
Jake

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] rebase: exec leaks GIT_DIR to environment
  2017-10-30  2:26     ` Junio C Hamano
@ 2017-10-30  2:53       ` Jacob Keller
  2017-10-30  3:36         ` Junio C Hamano
  0 siblings, 1 reply; 12+ messages in thread
From: Jacob Keller @ 2017-10-30  2:53 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Phillip Wood, Johannes Schindelin, Jacob Keller, Git mailing list

On Sun, Oct 29, 2017 at 7:26 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Phillip Wood <phillip.wood@talktalk.net> writes:
>
>> Just clearing GIT_DIR does not match the behavior of the shell version
>> (tested by passing -p to avoid rebase--helper) as that passes GIT_DIR to
>> exec commands if it has been explicitly set. I think that users that set
>> GIT_DIR on the command line would expect it to be propagated to exec
>> commands.
>>
>> $ git rebase -px'echo $GIT_DIR' @
>>
>>                                                             Merge commit
>> '7c2f1abd64' into phil
>> Executing: echo $GIT_DIR
>>
>> Successfully rebased and updated refs/heads/phil.
>>
>> $ env GIT_DIR=.git git rebase -px'echo $GIT_DIR' @
>>
>>                                                             Merge commit
>> '7c2f1abd64' into phil
>> Executing: echo $GIT_DIR
>> /home/phil/Documents/src/git/.git/worktrees/git-next
>> Successfully rebased and updated refs/heads/phil.
>
> Hmmm, I do not mess with GIT_DIR at all in my workflow, so I am
> having a bit of hard time judging if this regression is serious
> enough to be a release blocker.
>

So, I don't directly mess with GIT_DIR in my use case either, I just
happened to run a build from a sub directory which relies on git
commands continuing to work. However, because GIT_DIR was set to a
relative path, it broke this Make in the subdirectory, which resulted
in the problem occurring, but *only* during the exec command, if I ran
the command manually after reach rebase step, it worked fine (since
GIT_DIR was no longer set).

I don't know how big a deal it is, since I didn't notice it for quite some time.

Thanks,
Jake

> I'd prefer to avoid reverting the whole js/rebase-i-final topic from
> 'master' this late in the game, even though I do not expect we would
> see the remainder of the system gets broken due to hidden dependency
> on the topic, because the changes on the topic are relatively well
> isolated.
>
>
> 570676e011
> d1114d87c7
> 2f0e14e649
> 5f3108b7b6

I am pretty confident we can fix it. I think the easiest solution
would be to just make sure GIT_DIR is an absolute path when passing it
to the exec command.

Thanks,
Jake

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] rebase: exec leaks GIT_DIR to environment
  2017-10-30  2:53       ` Jacob Keller
@ 2017-10-30  3:36         ` Junio C Hamano
  2017-10-30  6:26           ` Jacob Keller
  0 siblings, 1 reply; 12+ messages in thread
From: Junio C Hamano @ 2017-10-30  3:36 UTC (permalink / raw)
  To: Jacob Keller
  Cc: Phillip Wood, Johannes Schindelin, Jacob Keller, Git mailing list

Jacob Keller <jacob.keller@gmail.com> writes:

> I am pretty confident we can fix it....

I am sure we can eventually, but 3 hours is not enough soak time.  

I am inclined to leave the fix for 2.15.1/2.16.0 instead of delaying
the release by 10 more days.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] rebase: exec leaks GIT_DIR to environment
  2017-10-30  3:36         ` Junio C Hamano
@ 2017-10-30  6:26           ` Jacob Keller
  2017-10-30 10:20             ` Phillip Wood
  0 siblings, 1 reply; 12+ messages in thread
From: Jacob Keller @ 2017-10-30  6:26 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Phillip Wood, Johannes Schindelin, Jacob Keller, Git mailing list

On Sun, Oct 29, 2017 at 8:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jacob Keller <jacob.keller@gmail.com> writes:
>
>> I am pretty confident we can fix it....
>
> I am sure we can eventually, but 3 hours is not enough soak time.
>
> I am inclined to leave the fix for 2.15.1/2.16.0 instead of delaying
> the release by 10 more days.

That's fair. I'm not even sure it was introduced since the last
release (I tried 2.12, but not 2.13 or 2.14 manually). Thus, it likely
wasn't noticed for at least a release, meaning it's less important (to
me at least) that we provide a fix immediately, since it went
unnoticed this long, likely that means few people will be impacted.

As far as I can tell, you'd only be impacted if you attempt to run a
git command from within a sub directory inside of an exec statement.
Is there some place we can list it as a known bug in case someone else
runs into it?

Regards,
Jake

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] rebase: exec leaks GIT_DIR to environment
  2017-10-30  6:26           ` Jacob Keller
@ 2017-10-30 10:20             ` Phillip Wood
  2017-10-30 12:46               ` Johannes Schindelin
  0 siblings, 1 reply; 12+ messages in thread
From: Phillip Wood @ 2017-10-30 10:20 UTC (permalink / raw)
  To: Jacob Keller, Junio C Hamano
  Cc: Johannes Schindelin, Jacob Keller, Git mailing list

On 30/10/17 06:26, Jacob Keller wrote:
> On Sun, Oct 29, 2017 at 8:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Jacob Keller <jacob.keller@gmail.com> writes:
>>
>>> I am pretty confident we can fix it....
>>
>> I am sure we can eventually, but 3 hours is not enough soak time.
>>
>> I am inclined to leave the fix for 2.15.1/2.16.0 instead of delaying
>> the release by 10 more days.
> 
> That's fair. I'm not even sure it was introduced since the last
> release (I tried 2.12, but not 2.13 or 2.14 manually). Thus, it likely
> wasn't noticed for at least a release, meaning it's less important (to
> me at least) that we provide a fix immediately, since it went
> unnoticed this long, likely that means few people will be impacted.

It is in 2.14.3, I haven't bisected but I suspect it was introduced by
311af5266b sequencer (rebase -i): implement the 'exec' command

Running
git rebase -x'perl -e '\''$,=$\="\n"; print  grep { /^GIT_/ } sort keys
%ENV'\' @
Shows that the rebase--helper version also sets GIT_PREFIX as well as
GIT_DIR, I suspect the difference is coming from differences in the
setup for builtin commands vs external commands. The shell version and
the rebase--helper version set GIT_CHERRY_PICK_HELP, GIT_EDITOR,
GIT_INTERNAL_GETTEXT_SH_SCHEME, GIT_REFLOG_ACTION

Best Wishes

Phillip



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] rebase: exec leaks GIT_DIR to environment
  2017-10-30 10:20             ` Phillip Wood
@ 2017-10-30 12:46               ` Johannes Schindelin
  2017-10-31  8:13                 ` Jacob Keller
  0 siblings, 1 reply; 12+ messages in thread
From: Johannes Schindelin @ 2017-10-30 12:46 UTC (permalink / raw)
  To: phillip.wood; +Cc: Jacob Keller, Junio C Hamano, Jacob Keller, Git mailing list

Hi Phillip,

On Mon, 30 Oct 2017, Phillip Wood wrote:

> On 30/10/17 06:26, Jacob Keller wrote:
> > On Sun, Oct 29, 2017 at 8:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
> >> Jacob Keller <jacob.keller@gmail.com> writes:
> >>
> >>> I am pretty confident we can fix it....
> >>
> >> I am sure we can eventually, but 3 hours is not enough soak time.
> >>
> >> I am inclined to leave the fix for 2.15.1/2.16.0 instead of delaying
> >> the release by 10 more days.
> > 
> > That's fair. I'm not even sure it was introduced since the last
> > release (I tried 2.12, but not 2.13 or 2.14 manually). Thus, it likely
> > wasn't noticed for at least a release, meaning it's less important (to
> > me at least) that we provide a fix immediately, since it went
> > unnoticed this long, likely that means few people will be impacted.
> 
> It is in 2.14.3, I haven't bisected but I suspect it was introduced by
> 311af5266b sequencer (rebase -i): implement the 'exec' command
> 
> Running
> git rebase -x'perl -e '\''$,=$\="\n"; print  grep { /^GIT_/ } sort keys
> %ENV'\' @
> Shows that the rebase--helper version also sets GIT_PREFIX as well as
> GIT_DIR, I suspect the difference is coming from differences in the
> setup for builtin commands vs external commands. The shell version and
> the rebase--helper version set GIT_CHERRY_PICK_HELP, GIT_EDITOR,
> GIT_INTERNAL_GETTEXT_SH_SCHEME, GIT_REFLOG_ACTION

Indeed, when you look at git_dir_init in git-sh-setup, you will see that
Unix shell scripts explicitly get their GIT_DIR turned into an absolute
path.

So my suggested patch is wrong, and it should be more along the lines of

	struct strbuf buf = STRBUF_INIT;
	const char *child_env[] = { NULL, NULL };

	strbuf_addf(&buf, "GIT_DIR=%s", absolute_path(get_git_dir()));
	child_env[0] = buf.buf;

	...

	strbuf_release(&buf);

Jake, can I still take you up on taking it from here?

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] rebase: exec leaks GIT_DIR to environment
  2017-10-30 12:46               ` Johannes Schindelin
@ 2017-10-31  8:13                 ` Jacob Keller
  0 siblings, 0 replies; 12+ messages in thread
From: Jacob Keller @ 2017-10-31  8:13 UTC (permalink / raw)
  To: Johannes Schindelin, phillip.wood
  Cc: Junio C Hamano, Jacob Keller, Git mailing list



On October 30, 2017 5:46:36 AM PDT, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
>Hi Phillip,
>
>On Mon, 30 Oct 2017, Phillip Wood wrote:
>
>> On 30/10/17 06:26, Jacob Keller wrote:
>> > On Sun, Oct 29, 2017 at 8:36 PM, Junio C Hamano <gitster@pobox.com>
>wrote:
>> >> Jacob Keller <jacob.keller@gmail.com> writes:
>> >>
>> >>> I am pretty confident we can fix it....
>> >>
>> >> I am sure we can eventually, but 3 hours is not enough soak time.
>> >>
>> >> I am inclined to leave the fix for 2.15.1/2.16.0 instead of
>delaying
>> >> the release by 10 more days.
>> > 
>> > That's fair. I'm not even sure it was introduced since the last
>> > release (I tried 2.12, but not 2.13 or 2.14 manually). Thus, it
>likely
>> > wasn't noticed for at least a release, meaning it's less important
>(to
>> > me at least) that we provide a fix immediately, since it went
>> > unnoticed this long, likely that means few people will be impacted.
>> 
>> It is in 2.14.3, I haven't bisected but I suspect it was introduced
>by
>> 311af5266b sequencer (rebase -i): implement the 'exec' command
>> 
>> Running
>> git rebase -x'perl -e '\''$,=$\="\n"; print  grep { /^GIT_/ } sort
>keys
>> %ENV'\' @
>> Shows that the rebase--helper version also sets GIT_PREFIX as well as
>> GIT_DIR, I suspect the difference is coming from differences in the
>> setup for builtin commands vs external commands. The shell version
>and
>> the rebase--helper version set GIT_CHERRY_PICK_HELP, GIT_EDITOR,
>> GIT_INTERNAL_GETTEXT_SH_SCHEME, GIT_REFLOG_ACTION
>
>Indeed, when you look at git_dir_init in git-sh-setup, you will see
>that
>Unix shell scripts explicitly get their GIT_DIR turned into an absolute
>path.
>
>So my suggested patch is wrong, and it should be more along the lines
>of
>
>	struct strbuf buf = STRBUF_INIT;
>	const char *child_env[] = { NULL, NULL };
>
>	strbuf_addf(&buf, "GIT_DIR=%s", absolute_path(get_git_dir()));
>	child_env[0] = buf.buf;
>
>	...
>
>	strbuf_release(&buf);
>
>Jake, can I still take you up on taking it from here?
>
>Ciao,
>Dscho

Yes, I will clean this up and submit something tomorrow once I get to a computer. I didn't end up having time to work on Git today.

Thanks,
Jake

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2017-10-31  8:13 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-28  0:01 [PATCH] rebase: exec leaks GIT_DIR to environment Jacob Keller
2017-10-28 16:00 ` Johannes Schindelin
2017-10-28 22:35   ` Jacob Keller
2017-10-29 18:34   ` Phillip Wood
2017-10-30  2:26     ` Junio C Hamano
2017-10-30  2:53       ` Jacob Keller
2017-10-30  3:36         ` Junio C Hamano
2017-10-30  6:26           ` Jacob Keller
2017-10-30 10:20             ` Phillip Wood
2017-10-30 12:46               ` Johannes Schindelin
2017-10-31  8:13                 ` Jacob Keller
2017-10-30  2:51     ` Jacob Keller

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).