git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Ramsay Jones <ramsay@ramsay1.demon.co.uk>
Date
Jun 15, 2013, 20:13 UTC
Message-ID
<51BCCAD2.9070106@ramsay1.demon.co.uk>
In-Reply-To
<1370987312-6761-1-git-send-email-mhagger@alum.mit.edu>
Michael Haggerty wrote:
> *This patch series must be built on top of mh/reflife.*
> 
[...]
Show 5 quoted lines
> 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

Previous: Jeff KingNext: Michael Haggerty
Message 19 of 22 in “Fix some reference-related races”
  1. 00/12 Fix some reference-related racesMichael Haggerty, Jun 11, 2013
  2. 01/12 repack_without_ref(): split list curation and entry writingMichael Haggerty, Jun 11, 2013
  3. Jeff KingJun 12, 2013
  4. Michael HaggertyJun 12, 2013
  5. 02/12 pack_refs(): split creation of packed refs and entry writingMichael Haggerty, Jun 11, 2013
  6. 03/12 refs: wrap the packed refs cache in a level of indirectionMichael Haggerty, Jun 11, 2013
  7. 04/12 refs: implement simple transactions for the packed-refs fileMichael Haggerty, Jun 11, 2013
  8. Jeff KingJun 12, 2013
  9. 05/12 refs: manage lifetime of packed refs cache via reference countingMichael Haggerty, Jun 11, 2013
  10. 06/12 do_for_each_entry(): increment the packed refs cache refcountMichael Haggerty, Jun 11, 2013
  11. 07/12 packed_ref_cache: increment refcount when lockedMichael Haggerty, Jun 11, 2013
  12. 08/12 Extract a struct stat_data from cache_entryMichael Haggerty, Jun 11, 2013
  13. 09/12 add a stat_validity structMichael Haggerty, Jun 11, 2013
  14. 10/12 get_packed_ref_cache: reload packed-refs file when it changesMichael Haggerty, Jun 11, 2013
  15. 11/12 for_each_ref: load all loose refs before packed refsMichael Haggerty, Jun 11, 2013
  16. 12/12 refs: do not invalidate the packed-refs cache unnecessarilyMichael Haggerty, Jun 11, 2013
  17. Jeff KingJun 12, 2013
  18. Jeff KingJun 12, 2013
  19. Ramsay JonesJun 15, 2013
  20. Michael HaggertyJun 16, 2013
  21. Ramsay JonesJun 18, 2013
  22. Michael HaggertyJun 19, 2013

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.