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

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

From
Michael Haggerty <mhagger@alum.mit.edu>
Date
Jun 20, 2013, 09:09 UTC
Message-ID
<51C2C6DD.90107@alum.mit.edu>
In-Reply-To
<20130619185645.GB23647@sigill.intra.peff.net>
On 06/19/2013 08:56 PM, Jeff King wrote:
Show 19 quoted lines
> On Wed, Jun 19, 2013 at 09:51:21AM +0200, Michael Haggerty wrote:
> 
>> Re-roll of mh/ref-races.  Thanks to Peff, Junio, and Ramsay for
>> reviewing v1.
> 
> Thanks. I just read through them again. Everything looks good to me.
> 
> Patches 10 and 11 are missing my signoff, but obviously:
> 
>   Signed-off-by: Jeff King <peff@peff.net>
> 
>> The last patch is still optional--it avoids a little bit of work when
>> rewriting the packed-refs file, but relies on the stat-based freshness
>> check not giving a false negative.
> 
> I don't have a real problem with it, but given the cygwin confusions
> that Ramsay mentioned, maybe it is better to hold back on it for now? It
> sounds like the cygwin problems go the other way (false positives
> instead of false negatives).

I just realized that there is another long-standing problem in loose/packed refs handling:

Suppose there is a loose reference for which an old version is still present in the packed-refs file. If the reference is deleted:

1. delete_ref() gets a lock on the loose reference file
2. delete_ref() deletes the loose reference file while retaining the lock
3. delete_ref() calls repack_without_ref()
4. repack_without_ref() tries to acquire a lock on the packed-refs file
to delete the packed version of the reference.

If the packed-refs file is already locked by another process (and there is no reason why that cannot be, and there is only one attempt to acquire the lock), then repack_without_ref() emits an error and returns with an error code. delete_ref() passes the error along, but doesn't restore the loose ref.

The net result is that an old version of the ref (namely the one in the loose refs file) has been revived. That version might even point at an object that has already been garbage collected.

This problem is similar to race conditions that have been discussed earlier, but this problem has nothing to do with the freshness/staleness of the packed-refs file WRT to the loose reference; it only depends on the packed-refs file being locked by another process at an inopportune time.

I think this problem would also be fixed by the locking scheme that I proposed earlier [1]: to acquire and hold the packed-refs lock across the modification of *both* files, and to rewrite the packed-refs file *before* deleting the loose-refs file (because rewriting the packed-refs file without the to-be-deleted reference is a logical NOP).

Michael
[1] http://article.gmane.org/gmane.comp.version-control.git/212131
-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
Previous: Jeff KingNext: Jeff King
Message 23 of 24 in “Fix some reference-related races”
  1. 00/12 Fix some reference-related racesMichael Haggerty, Jun 19, 2013
  2. 01/12 repack_without_ref(): split list curation and entry writingMichael Haggerty, Jun 19, 2013
  3. 02/12 pack_refs(): split creation of packed refs and entry writingMichael Haggerty, Jun 19, 2013
  4. 03/12 refs: wrap the packed refs cache in a level of indirectionMichael Haggerty, Jun 19, 2013
  5. 04/12 refs: implement simple transactions for the packed-refs fileMichael Haggerty, Jun 19, 2013
  6. Junio C HamanoJun 19, 2013
  7. Michael HaggertyJun 20, 2013
  8. Jeff KingJun 20, 2013
  9. Michael HaggertyJun 20, 2013
  10. Jeff KingJun 20, 2013
  11. Junio C HamanoJun 20, 2013
  12. Michael HaggertyJun 20, 2013
  13. Junio C HamanoJun 20, 2013
  14. 05/12 refs: manage lifetime of packed refs cache via reference countingMichael Haggerty, Jun 19, 2013
  15. 06/12 do_for_each_entry(): increment the packed refs cache refcountMichael Haggerty, Jun 19, 2013
  16. 07/12 packed_ref_cache: increment refcount when lockedMichael Haggerty, Jun 19, 2013
  17. 08/12 Extract a struct stat_data from cache_entryMichael Haggerty, Jun 19, 2013
  18. 09/12 add a stat_validity structMichael Haggerty, Jun 19, 2013
  19. 10/12 get_packed_ref_cache: reload packed-refs file when it changesMichael Haggerty, Jun 19, 2013
  20. 11/12 for_each_ref: load all loose refs before packed refsMichael Haggerty, Jun 19, 2013
  21. 12/12 refs: do not invalidate the packed-refs cache unnecessarilyMichael Haggerty, Jun 19, 2013
  22. Jeff KingJun 19, 2013
  23. Michael HaggertyJun 20, 2013
  24. Jeff KingJun 20, 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.