Re: Crash with filter-branch and --tag-name-filter

[Jurko Gospodnetić]

  Hi all.

Running some filter-branch commands to test some history editing scenarios I ran into a crash. Stripped down, it can be reproduced as follows:

mkdir XXX
cd XXX
git init
echo haha > file.txt
git add file.txt
git commit -m "initial commit"
git tag -m "your garden variety annotated tag" taggy
git filter-branch --tag-name-filter cat

This correctly recognizes there is nothing to change, but still goes on to the tag renaming stage which sometimes crashes at once, sometimes hangs and sometimes crashes only after stalling for a while. When this crashes it takes its whole cmd process and its respective console window down with it.

  Just ran into this again and did a little more debugging.

  The issue does not seem to occur when running from the git shell. It does occur though when running it from the native Windows cmd shell.

  Simplifying the crash scenario script further brought it down to this:

#!/bin/sh
fifi () {
    /bin/sh -c "echo ...lala..."
}
echo huhu | fifi >/dev/null 2>&1

  If you run this script from git's bash prompt - everything works fine, i.e. it produces no output and the shell process does not not crash. However, if you save it as 'script.sh' & run it from the cmd shell like this:

"C:\Program Files (x86)\Git\bin\sh.exe" ./script.sh

  the calling cmd.exe will crash.

  The same does not occur when using the Cygwin shell.

  The issue seems to be related to output redirection & calling part of the redirected command via a shell function, which is the use-case found in the git-filter-branch script.

  Anyone more knowledgeable with this code base have an idea what could be going on here?

  Best regards,

    Jurko Gospodnetić

[Jurko Gospodnetić]

  Hi all.

Running some filter-branch commands to test some history editing scenarios I ran into a crash. Stripped down, it can be reproduced as follows:

mkdir XXX
cd XXX
git init
echo haha > file.txt
git add file.txt
git commit -m "initial commit"
git tag -m "your garden variety annotated tag" taggy
git filter-branch --tag-name-filter cat

This correctly recognizes there is nothing to change, but still goes on to the tag renaming stage which sometimes crashes at once, sometimes hangs and sometimes crashes only after stalling for a while. When this crashes it takes its whole cmd process and its respective console window down with it.

 
  Also, to reproduce this, the git filter-branch command needs to be run from the WIndows cmd shell. Running it from an interactive msysgit bash shell seems to avoid the problem.

  No one is taking the bait on this one so far... :-), but just in case some developer/maintainer does show interest - the problem can be worked around like this:

--- git-filter-branch.sh:74f23e7    2013-06-07 08:48:37.000000000 +0200
+++ git-filter-branch.sh: Working Tree    2013-06-07 08:48:32.000000000 +0200
@@ -460,5 +460,5 @@
                 die "Could not create new tag object for $ref"
             if git cat-file tag "$ref" | \
-               sane_grep '^-----BEGIN PGP SIGNATURE-----' >/dev/null 2>&1
+               sane_grep -q '^-----BEGIN PGP SIGNATURE-----'
             then
                 warn "gpg signature stripped from tag object $sha1t"

  This avoids the problematic 'external process call via bash function + pipe + stdout and stderr redirect' operation that seems to trigger the crash in the calling process (most likely an access violation internally), by using the grep executable's '-q' option. This option is available with both cygwin grep & the one distributed alongside msysgit.

  The code most likely is not directly portable to different unix distributions where their grep executables might not accept the '-q' option (e.g. seen such grep on HP-UX).

  I guess a better fix would be to go into the msys code-base and fix whatever is causing the underlying operation to fail, but this does fix the filter-branch crash on Windows.

  Alternatively, the script could be changed to invoke the grep operation using x=$(grep ...) and then ignore the output collected in the $x variable instead of using stdout/stderr redirection. That would then both avoid the problem and work in environments with an older grep implementation. I chose the first implementation for now because the code is cleaner and I guess the whole fix belongs in msysgit anyway.

  Any chance of getting something like this in msysgit?

  Best regards,
    Jurko Gospodnetić

[Erik Faye-Lund]

On Fri, Jun 7, 2013 at 9:23 AM, Jurko Gospodnetić
<jurko.go...@gmail.com> wrote:
> the problem can be worked around like this:
>
>> --- git-filter-branch.sh:74f23e7 2013-06-07 08:48:37.000000000 +0200
>> +++ git-filter-branch.sh: Working Tree 2013-06-07 08:48:32.000000000
>> +0200
>> @@ -460,5 +460,5 @@
>> die "Could not create new tag object for $ref"
>> if git cat-file tag "$ref" | \
>> - sane_grep '^-----BEGIN PGP SIGNATURE-----' >/dev/null 2>&1
>> + sane_grep -q '^-----BEGIN PGP SIGNATURE-----'
>> then
>> warn "gpg signature stripped from tag object $sha1t"
>
>
> This avoids the problematic 'external process call via bash function +
> pipe + stdout and stderr redirect' operation that seems to trigger the crash
> in the calling process (most likely an access violation internally), by
> using the grep executable's '-q' option. This option is available with both
> cygwin grep & the one distributed alongside msysgit.
>
> The code most likely is not directly portable to different unix
> distributions where their grep executables might not accept the '-q' option
> (e.g. seen such grep on HP-UX).
>

Hm. POSIX defines -q for grep:
http://pubs.opengroup.org/onlinepubs/009695399/utilities/grep.html

But, it seems to do more than just quieting, it also changes the
return value in the case of an error. Perhaps this detail is related
to your crash?

> Any chance of getting something like this in msysgit?

Not likely until we find out *why* this happens.

[Jurko Gospodnetić]

  Hi.

  Thanks for replying! I was beginning to think my messages were getting lost somehow. :-)

Hm. POSIX defines -q for grep:
http://pubs.opengroup.org/onlinepubs/009695399/utilities/grep.html

But, it seems to do more than just quieting, it also changes the
return value in the case of an error. Perhaps this detail is related
to your crash?

 
  Actually, the return value stays the same. Both with or without this option grep should return 0 when it finds something or !0 if it does not, and this behavior is good enough for the filter-branch script.

>   Any chance of getting something like this in msysgit?

Not likely until we find out *why* this happens.

  Would adding some sort of a test reproducing this behaviour be enough for the fix to be applied? That way the end-user problem gets fixed ASAP.

  Real underlying reason behind the crash seems to be somewhere deep in msys/cmd internals. Could someone with access to a newer msys version try out the shell script from my other post (quoted below)?

#!/bin/sh
fifi () {
    /bin/sh -c "echo ...lala..."
}
echo huhu | fifi >/dev/null 2>&1

  If you run this script from git's bash prompt - everything works fine, i.e. it produces no output and the shell process does not not crash. However, if you save it as 'script.sh' & run it from the cmd shell like this:
"C:\Program Files (x86)\Git\bin\sh.exe" ./script.sh

  The script above basically does just the problematic part of the filter-branch script and does so without using any external executable other than the shell.

  Or if someone can send me a built newer sh.exe, I can try it out on my side.

  I'm afraid to dig in, find out how to build msys and debug the issue myself as I do not know how deep that rabbit-hole goes and I hate having to stop working on something with lots of effort already invested and no visible results. If we have a fix for the original problem, then working on the underlying issue can be done at a slower pace and with much less pressure (for me at least :-)).

  Best regards,
    Jurko Gospodnetić

[Erik Faye-Lund]

On Fri, Jun 7, 2013 at 10:18 AM, Jurko Gospodnetić
<jurko.go...@gmail.com> wrote:
> Hi.
>
> Thanks for replying! I was beginning to think my messages were getting
> lost somehow. :-)

No, they weren't lost. We don't have a strong "support team", this is
more of a "you have to do the heavy lifting yourself (or convince
someone to do it for you)" kind of project. And as the good user you
seem to be, you have shown willingness to do some debugging yourself.
Great :)

>> Hm. POSIX defines -q for grep:
>> http://pubs.opengroup.org/onlinepubs/009695399/utilities/grep.html
>>
>> But, it seems to do more than just quieting, it also changes the
>> return value in the case of an error. Perhaps this detail is related
>> to your crash?
>
>
> Actually, the return value stays the same. Both with or without this
> option grep should return 0 when it finds something or !0 if it does not,
> and this behavior is good enough for the filter-branch script.

Perhaps. The standard says "If the -q option is specified, the exit
status shall be zero if an input line is selected, even if an error
was detected." Is what you're saying that you inspected the
surrounding code well enough to conclude that "an error was detected"
is irrelevant in this case? Because that condition *does* change from
non-true to true in this case.

>> > Any chance of getting something like this in msysgit?
>>
>> Not likely until we find out *why* this happens.
>
>
> Would adding some sort of a test reproducing this behaviour be enough for
> the fix to be applied?

That would increase the chance of finding out why it happens, so yes.

> That way the end-user problem gets fixed ASAP.

I don't believe papering over problems without understanding them is
the way forward, that'll just leave us with an unmaintainable mess. So
let's focus on the "finding out why"-bit.

Especially since there's a bunch of other places where we do the exact
same thing, and they would probably need inspection too.

> Real underlying reason behind the crash seems to be somewhere deep in
> msys/cmd internals. Could someone with access to a newer msys version try
> out the shell script from my other post (quoted below)?
>
>
>>> #!/bin/sh
>>> fifi () {
>>> /bin/sh -c "echo ...lala..."
>>> }
>>> echo huhu | fifi >/dev/null 2>&1
>>
>>
>> If you run this script from git's bash prompt - everything works fine,
>> i.e. it produces no output and the shell process does not not crash.
>> However, if you save it as 'script.sh' & run it from the cmd shell like
>> this:
>> "C:\Program Files (x86)\Git\bin\sh.exe" ./script.sh
>

Thanks for the reproduction-case.

Running it like that *does* take down the shell here indeed. But we
cannot simply assume that piping all output to /dev/null is broken;
we've been doing this in a dozen different places for years without
problem. Something else is most likely going on.

My first hunch was "well, we don't set up the environment", so I tried
running cmd from sh, and issuing the command there. That succeeded.

But my next attempt was to run it from git-cmd.exe (which AFAIK is
supposed to set up the environment correctly), and the issue still
persists.

So I'm thinking that it's still a configuration issue of some sorts,
there are quite some steps involved in setting up our bash-shell. But
I'm not feeling motivated enough to dig through it all to try to
figure out what the relevant difference is :/

Another data-point: Starting sh interactively, and then running the
script does not cause a crash, it seems.

[Jurko Gospodnetić]

  Hi.

>   Actually, the return value stays the same. Both with or without this
> option grep should return 0 when it finds something or !0 if it does not,
> and this behavior is good enough for the filter-branch script.

Perhaps. The standard says "If the -q option is specified, the exit
status shall be zero if an input line is selected, even if an error
was detected." Is what you're saying that you inspected the
surrounding code well enough to conclude that "an error was detected"
is irrelevant in this case? Because that condition *does* change from
non-true to true in this case.

  Oh, did not see that statement, sorry. My fault. It's located separate from the main -q option description. :-(

  However, the only ways that original and the new code could produce different results would be:
  1. new code returned 0, old code returned !0 - that would mean that grep reported a line containing the GPG signature header that did not actually contain a GPG signature header, and that would indicate a serious grep bug or a catastrophic external error such as reading from the disk succeeding but reporting invalid data.
  2. new code returned !0, old code returned 0, error - that would mean that grep encountered an external error that prevented it from continuing the search for the line. In that case both the original and the new implementation report the a !0 result so this scenario is not possible.
  3. new code returned !0, old code returned 0, no error - that would mean that grep has a serious internal bug and failed to find the desired line even though it exists.

  So I would wager that the old and the new code is equivalent, ignoring possible effects of 'serious internal grep bugs or catastrophic environment failures' - cases in both of which all bets are off anyway.

>   Would adding some sort of a test reproducing this behaviour be enough for
> the fix to be applied?

That would increase the chance of finding out why it happens, so yes.

  Ok, we have the minimalistic shell script reproducing the crash. Is there some testing framework already set up in this project that can handle tests like 'run this process and see if it crashes'? I guess I'll have to set up the development environment and see how to run the existing tests after all... :-(

 

I don't believe papering over problems without understanding them is
the way forward, that'll just leave us with an unmaintainable mess. So
let's focus on the "finding out why"-bit.

Especially since there's a bunch of other places where we do the exact
same thing, and they would probably need inspection too.

  I checked the sources using:

grep -inr -B 1 -F ">/dev/null 2>&1" *

  and I have not been able to find other instances where git scripts redirect all output to /dev/null, and the command is piped so I do not think this bug (as tested by the script) rears its ugly head elsewhere in the current code-base.

Another data-point: Starting sh interactively, and then running the
script does not cause a crash, it seems.

  Yup. And here are some more variants:

  1. Running the script from cmd by calling 'sh.exe ./script.sh' - crashes
  2. Running the script from cmd by calling 'sh.exe -c ./script.sh' - crashes
  3. Running the script from an interactive sh.exe by calling ./script.sh - works
  4. Running the script from an interactive sh.exe by calling '/bin/sh ./script.sh' - works
  5. Running the script from an interactive sh.exe by calling '/bin/sh -c ./script.sh' - works
  6. Running the script from cmd run inside an interactive sh.exe by calling 'sh.exe ./script.sh' - works
  7. Running the script from cmd run inside an interactive sh.exe by calling 'sh.exe -c ./script.sh' - works

  So it seems that having an interactive sh.exe anywhere up in the parent process chain seems to hide the problem. Totally weird...

  Bah... off to find out how to compile msys after all.... *grumble*...

  Best regards,
    Jurko Gospodnetić

[Erik Faye-Lund]

On Fri, Jun 7, 2013 at 2:14 PM, Jurko Gospodnetić
<jurko.go...@gmail.com> wrote:
>
> However, the only ways that original and the new code could produce
> different results would be:
> 1. new code returned 0, old code returned !0 - that would mean that grep
> reported a line containing the GPG signature header that did not actually
> contain a GPG signature header, and that would indicate a serious grep bug
> or a catastrophic external error such as reading from the disk succeeding
> but reporting invalid data.
> 2. new code returned !0, old code returned 0, error - that would mean that
> grep encountered an external error that prevented it from continuing the
> search for the line. In that case both the original and the new
> implementation report the a !0 result so this scenario is not possible.
> 3. new code returned !0, old code returned 0, no error - that would mean
> that grep has a serious internal bug and failed to find the desired line
> even though it exists.
>
> So I would wager that the old and the new code is equivalent, ignoring
> possible effects of 'serious internal grep bugs or catastrophic environment
> failures' - cases in both of which all bets are off anyway.

Thanks for the detailed analysis. Yes, you are probably right.

>> > Would adding some sort of a test reproducing this behaviour be enough
>> > for
>> > the fix to be applied?
>>
>> That would increase the chance of finding out why it happens, so yes.
>
>
> Ok, we have the minimalistic shell script reproducing the crash. Is there
> some testing framework already set up in this project that can handle tests
> like 'run this process and see if it crashes'? I guess I'll have to set up
> the development environment and see how to run the existing tests after
> all... :-(
>

We do have the test-suite in the development environment, but that
only runs from bash. See /share/msysGit/run-tests.sh

https://code.google.com/p/msysgit/downloads/list?q=net+installer

It's dead-easy to install, and contains everything you need to build
Git (and our MSys binaries). And it doesn't pollute your system beyond
a single directory.

>> I don't believe papering over problems without understanding them is
>> the way forward, that'll just leave us with an unmaintainable mess. So
>> let's focus on the "finding out why"-bit.
>>
>> Especially since there's a bunch of other places where we do the exact
>> same thing, and they would probably need inspection too.
>
>
> I checked the sources using:
>>
>> grep -inr -B 1 -F ">/dev/null 2>&1" *
>
> and I have not been able to find other instances where git scripts
> redirect all output to /dev/null, and the command is piped so I do not think
> this bug (as tested by the script) rears its ugly head elsewhere in the
> current code-base.

OK. Again, thanks for doing the analysis :)

So this code-path might be unique in doing this. But I'm still quite
surprised that we haven't seen this problem before. The code in
question is from 2009, and I don't think our bash is all that new
either.

> 1. Running the script from cmd by calling 'sh.exe ./script.sh' - crashes
> 2. Running the script from cmd by calling 'sh.exe -c ./script.sh' -
> crashes
> 3. Running the script from an interactive sh.exe by calling ./script.sh -
> works
> 4. Running the script from an interactive sh.exe by calling '/bin/sh
> ./script.sh' - works
> 5. Running the script from an interactive sh.exe by calling '/bin/sh -c
> ./script.sh' - works
> 6. Running the script from cmd run inside an interactive sh.exe by calling
> 'sh.exe ./script.sh' - works
> 7. Running the script from cmd run inside an interactive sh.exe by calling
> 'sh.exe -c ./script.sh' - works
>
> So it seems that having an interactive sh.exe anywhere up in the parent
> process chain seems to hide the problem. Totally weird...

Yeah, this is quite puzzling. Perhaps it's somehow related to some
inherited process attribute?

> Bah... off to find out how to compile msys after all.... *grumble*...

Again, the msysGit development environment can build msys. There's a
separate 'msys'-branch in the root repo for that.

[Jurko Gospodnetić]

  Hi.

>   Bah... off to find out how to compile msys after all.... *grumble*...

Again, the msysGit development environment can build msys. There's a
separate 'msys'-branch in the root repo for that.

  Ok, I must be missing something here. :-(

  I can build git easily using the network installer and later on using /bin/make from the /git folder. However, I assumed that when you said 'build msys' you meant build all the msys executables including sh.exe. Or I might be not understanding something here correctly. :-(

  Researching the original problem with the minimal reproducible use-case script seems to boil down to either 'guessing what could be wrong in the process environment' or building sh.exe in debug mode and taking a look at where it crashes using a debugger.

  The git://github.com/msysgit/msysgit repository's master & msys branches both contain a prebuilt sh.exe executable and I found no way to build it from that source.

  Anyone have any hints on how that executable is supposed to be built?
Possibly from some generic msys source base and not msysgit specific at all?

  Best regards,
    Jurko Gospodnetić
 

[Jurko Gospodnetić]

  Hi.

>   Bah... off to find out how to compile msys after all.... *grumble*...

  Ok, found some more time for this so I installed a non-git related msys environment by using the installer from 'https://sourceforge.net/projects/mingw/files/Installer/mingw-get-inst'. That one comes with /bin/sh version:

$ /bin/sh --version
GNU bash, version 3.1.17(1)-release (i686-pc-msys)
Copyright (C) 2005 Free Software Foundation, Inc.

  while the one coming with msysgit reports its version as:

$ /bin/sh --version
GNU bash, version 3.1.0(1)-release (i686-pc-msys)
Copyright (C) 2005 Free Software Foundation, Inc.

  However, both see to suffer from the same bug and in both cases our little shell script causes both the newer shell and its parent cmd.exe process to die. :-(

  Now I'm off to see how to report a bug for that project, but in the mean time, I really think the 'grep -q' patch nicely cuts this long problem into smaller ones and would be happy to see it applied (with an appropriate comment) to the msysgit project...

  Best regards,
    Jurko Gospodnetić

[Jurko Gospodnetić]

  Hi.

  Now I'm off to see how to report a bug for that project, but in the mean time, I really think the 'grep -q' patch nicely cuts this long problem into smaller ones and would be happy to see it applied (with an appropriate comment) to the msysgit project...

  Bug reported to the msys/mingw project - https://sourceforge.net/p/mingw/bugs/1988

  Best regards,
    Jurko Gospodnetić

[Peter Harris]

On Sun, Jun 9, 2013 at 5:17 AM, Jurko Gospodnetić wrote:
>
> Bug reported to the msys/mingw project -
> https://sourceforge.net/p/mingw/bugs/1988

That sounds like the hotfix in
https://support.microsoft.com/kb/2458000 . You might want to give that
a try, if you haven't already.

Peter Harris

[Jurko Gospodnetić]

  Hi.

  Thanks for the tip! :-) That indeed seems to resolve the problem. With that hotfix applied the problem/crash no longer occurs with any of the /bin/sh shell versions affected earlier.

  So now we have a fix/workaround requiring manually installing a Windows hot-fix. What annoys me me greatly is the fact that the hot-fix is over 2 and a half years old, and M$ still has not been included it in any of the officially included Windows Updates. :-( I guess not enough facebook users are affected...

  I'd still like to see the original suggested 'grep -q' patch included in msysgit (or git) as this problem seems real difficult to diagnose if it occurs again for someone, e.g. someone running one of my git scripts on some other computer... :-) I can prepare a valid patch with an appropriate embedded code comment against the base git repository if that helps. I'd just need some helpful hints regarding which branch the patch should be based on, whether it is better to prepare a patch and send in to the git mailing list or fork the underlying repo on github and send a pull request instead, and such.

  Thanks again!

  Best regards,
    Jurko Gospodnetić