Re: [PATCH 00/12] Fix some reference-related races

[Ramsay Jones <ramsay@ramsay1.demon.co.uk>]

Michael Haggerty wrote:
> *This patch series must be built on top of mh/reflife.*
> 

[...]

> The other problem was that the for_each_ref() functions will die if
> the ref cache that they are iterating over is freed out from under
> them.  This problem is solved by using reference counts to avoid
> freeing the old packed ref cache (even if it is no longer valid) until
> all users are done with it.

Yes, I found exactly this happened to me on cygwin, earlier this week,
with the previous version of this code. After seeing this mail, I had
decided not to describe the failure on the old version, but wait and
test this version instead.

This version is a great improvement, but it still has some failures on
cygwin. So, it may be worth (briefly) describing the old failure anyway!
Note that several tests failed, but I will only mention t3211-peel-ref.sh
tests #7-8.

  $ pwd
  /home/ramsay/git/t/trash directory.t3211-peel-ref
  $

  $ ../../bin-wrappers/git show-ref -d
  d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/heads/master
  eb0e854c2cd2511c7571b1a5e3c8b8146193fb30 refs/outside/foo
  d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/outside/foo^{}
        5 [main] git 3540 _cygtls::handle_exceptions: Error while dumping state (p
  robably corrupted stack)
  Segmentation fault (core dumped)
  $

The stack-trace for the faulting code looks something like:

  cmd_show_ref()
    for_each_ref(show_ref, NULL)
      do_for_each_ref(&ref_cache, "", show_ref, 0, 0, NULL)
        do_for_each_entry(&ref_cache, "", do_one_ref, &data)
          do_for_each_entry_in_dirs(packed_dir, loose_dir, do_one_ref, &data)
            *dfeeid() recursive calls*
              do_one_ref(entry, &data)
                show_ref("refs/outside/foo", sha1, NULL) [2nd match]
                  peel_ref("refs/outside/foo", sha1)
                    peel_entry(entry, 0)
                      peel_object(name, sha1)
                        deref_tag_noverify(o)
                          parse_object(sha1 <eb0e854c2...>)
                            lookup_replace_object(sha1)
                              do_lookup_replace_object(sha1)
                                prepare_replace_object()

  [un-indent here!]
      for_each_replace_ref(register_replace_ref, NULL)
        do_for_each_ref(&ref_cache, "refs/replace", fn, 13, 0, NULL)
          do_for_each_entry(&ref_cache, "refs/replace", fn, &data)
            get_packed_refs(&ref_cache)
              clear_packed_ref_cache(&ref_cache) *free_ref_entries etc*
  ** return to show_ref() [2nd match] above **
  ** return to recursive dfeeid() call in original iteration
  ** dir1->entries has been free()-ed and reused => segmentation fault
  [dir1->entries == 0x64633263 => dc2c => part of sha1 for refs/outside/foo]

So, the nested "replace-reference-iteration" causes the ref_cache to be
freed out from under the initial show-ref iteration, so this works:

  $ GIT_NO_REPLACE_OBJECTS=1 ../../bin-wrappers/git show-ref -d
  d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/heads/master
  eb0e854c2cd2511c7571b1a5e3c8b8146193fb30 refs/outside/foo
  d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/outside/foo^{}
  d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/tags/base
  eb0e854c2cd2511c7571b1a5e3c8b8146193fb30 refs/tags/foo
  d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/tags/foo^{}
  $

You may be wondering why clear_packed_ref_cache() is called? Well, that
is because stat_validity_check() *incorrectly* indicates that the
packed-refs file has changed. Why does it do that? Well, this is one
more example of the problems caused by the cygwin schizophrenic stat()
functions. :( [ARGHHHHHHHHH]

At this point, I tried running 'git show-ref' with core.checkstat set
on the command line; but that didn't work! I had to fix show-ref and
re-build git, and then, this works:

  $ ../../bin-wrappers/git -c core.checkstat=minimal -c core.trustctime=f
  alse show-ref -d
  d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/heads/master
  eb0e854c2cd2511c7571b1a5e3c8b8146193fb30 refs/outside/foo
  d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/outside/foo^{}
  d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/tags/base
  eb0e854c2cd2511c7571b1a5e3c8b8146193fb30 refs/tags/foo
  d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/tags/foo^{}
  $

Now, turning to the new code, t3211-peel-ref.sh test #7 now works, but
test #8 still fails...

  $ ./t3211-peel-ref.sh -i -v

  ...

  ok 7 - refs are peeled outside of refs/tags (old packed)

  expecting success:
          git pack-refs --all &&
          cp .git/packed-refs fully-peeled &&
          git branch yadda &&
          git pack-refs --all &&
          git branch -d yadda &&
          test_cmp fully-peeled .git/packed-refs

  fatal: internal error: packed-ref cache cleared while locked
  not ok 8 - peeled refs survive deletion of packed ref
  #
  #               git pack-refs --all &&
  #               cp .git/packed-refs fully-peeled &&
  #               git branch yadda &&
  #               git pack-refs --all &&
  #               git branch -d yadda &&
  #               test_cmp fully-peeled .git/packed-refs
  #
  $ cd trash\ directory.t3211-peel-ref/
  $ ../../bin-wrappers/git pack-refs --all
  fatal: internal error: packed-ref cache cleared while locked
  $ ls
  actual  base.t  expect
  $ ls .git
  COMMIT_EDITMSG  branches/  description      index  logs/     packed-refs
  HEAD            config     hooks-disabled/  info/  objects/  refs/
  $ ls -l .git/packed-refs
  -rw-r--r-- 1 ramsay None 296 Jun 14 20:34 .git/packed-refs
  $ cat .git/packed-refs
  # pack-refs with: peeled
  d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/heads/master
  eb0e854c2cd2511c7571b1a5e3c8b8146193fb30 refs/outside/foo
  d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/tags/base
  eb0e854c2cd2511c7571b1a5e3c8b8146193fb30 refs/tags/foo
  ^d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e
  $

Now, I have a test-stat program which prints the difference between
the two stat implementations used in cygwin git, thus:

  $ ../../test-stat .git/packed-refs
  stat for '.git/packed-refs':
  *dev:   -1395862925, 0
  *ino:   166044, 0
  *mode:  100644 -rw-, 100600 -rw-
   nlink: 1, 1
  *uid:   1005, 0
  *gid:   513, 0
  *rdev:  -1395862925, 0
   size:  296, 296
   atime: 1371238550, 1371238550 Fri Jun 14 20:35:50 2013
   mtime: 1371238469, 1371238469 Fri Jun 14 20:34:29 2013
   ctime: 1371238469, 1371238469 Fri Jun 14 20:34:29 2013
  $ ../../bin-wrappers/git -c core.checkstat=minimal pack-refs --all
  fatal: internal error: packed-ref cache cleared while locked
  $

Hmmm, that should have worked! Wait, fix 'git pack-refs' to support
setting config variables on the command line, rebuild and:

  $ ../../bin-wrappers/git -c core.checkstat=minimal pack-refs --all
  $ cat .git/packed-refs
  # pack-refs with: peeled fully-peeled
  d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/heads/master
  eb0e854c2cd2511c7571b1a5e3c8b8146193fb30 refs/outside/foo
  ^d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e
  d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/tags/base
  eb0e854c2cd2511c7571b1a5e3c8b8146193fb30 refs/tags/foo
  ^d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e
  $

I haven't checked the remaining test failures to see if they are
caused by this code (I don't think so, but ...), but this failure
is clearly a cygwin specific issue.

HTH

ATB,
Ramsay Jones