Re: [PATCH] rebase: exec leaks GIT_DIR to environment
On October 30, 2017 5:46:36 AM PDT, Johannes Schindelin 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 >wrote: >> >> Jacob Keller 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.
Re: [PATCH] rebase: exec leaks GIT_DIR to environment
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 wrote: > >> Jacob Keller 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
Re: [PATCH] rebase: exec leaks GIT_DIR to environment
On 30/10/17 06:26, Jacob Keller wrote: > On Sun, Oct 29, 2017 at 8:36 PM, Junio C Hamano wrote: >> Jacob Keller 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
Re: [PATCH] rebase: exec leaks GIT_DIR to environment
On Sun, Oct 29, 2017 at 8:36 PM, Junio C Hamano wrote: > Jacob Keller 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
Re: [PATCH] rebase: exec leaks GIT_DIR to environment
Jacob Keller 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.
Re: [PATCH] rebase: exec leaks GIT_DIR to environment
On Sun, Oct 29, 2017 at 7:26 PM, Junio C Hamano wrote: > Phillip Wood 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
Re: [PATCH] rebase: exec leaks GIT_DIR to environment
On Sun, Oct 29, 2017 at 11:34 AM, Phillip Wood 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
Re: [PATCH] rebase: exec leaks GIT_DIR to environment
Phillip Wood 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
Re: [PATCH] rebase: exec leaks GIT_DIR to environment
On 28/10/17 17:00, Johannes Schindelin wrote: > > Hi Jake, > > On Fri, 27 Oct 2017, Jacob Keller wrote: > >> From: Jacob Keller >> >> 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 >
Re: [PATCH] rebase: exec leaks GIT_DIR to environment
On Sat, Oct 28, 2017 at 9:00 AM, Johannes Schindelin wrote: > Hi Jake, > > On Fri, 27 Oct 2017, Jacob Keller wrote: > >> From: Jacob Keller >> >> 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" > > > 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
Re: [PATCH] rebase: exec leaks GIT_DIR to environment
Hi Jake, On Fri, 27 Oct 2017, Jacob Keller wrote: > From: Jacob Keller > > 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
[PATCH] rebase: exec leaks GIT_DIR to environment
From: Jacob Keller 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 --- 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