{"thread":{"id":"46158","subject":"[PATCH 0/1] Fix for mh/packed-ref-store-prep","startedAt":"2017-06-12T08:06:30Z","lastAt":"2017-06-12T18:21:49Z","messageCount":3,"participants":["Michael Haggerty","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"321950","messageId":"cover.1497253685.git.mhagger@alum.mit.edu","threadId":"46158","inReplyTo":null,"subject":"[PATCH 0/1] Fix for mh/packed-ref-store-prep","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-06-12T08:06:12Z","receivedAt":"2017-06-12T08:06:30Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"When preparing a new patch series, I noticed a bug that crept into\nmh/packed-ref-store-prep while I was shuffling patches around. It\nintroduces a race condition if another process rewrites the\n`packed-refs` file just before we lock it. See the commit message for\nmore information.\n\nThis patch applies on top of mh/packed-ref-store-prep and merges to\nmaster without problems.\n\nMichael\n\nMichael Haggerty (1):\n  lock_packed_refs(): fix cache validity check\n\n refs/files-backend.c | 32 +++++++++++++++++++++++---------\n 1 file changed, 23 insertions(+), 9 deletions(-)\n\n-- \n2.11.0\n\n"},{"id":"321951","messageId":"18f059555ce0e4f46fba783973947f6a26ac4b65.1497253685.git.mhagger@alum.mit.edu","threadId":"46158","inReplyTo":"cover.1497253685.git.mhagger@alum.mit.edu","subject":"[PATCH 1/1] lock_packed_refs(): fix cache validity check","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-06-12T08:06:13Z","receivedAt":"2017-06-12T08:06:36Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Commit 28ed9830b1 (get_packed_ref_cache(): assume \"packed-refs\" won't\nchange while locked, 2017-05-22) assumes that the \"packed-refs\" file\ncannot change while we hold the lock. That assumption is\njustified *if* the lock has been held the whole time since the\n\"packed-refs\" file was last read.\n\nBut in `lock_packed_refs()`, we ourselves lock the \"packed-refs\" file\nand then call `get_packed_ref_cache()` to ensure that the cache agrees\nwith the file. The intent is to guard against the possibility that\nanother process changed the \"packed-refs\" file the moment before we\nlocked it.\n\nThis check was defeated because `get_packed_ref_cache()` saw that the\nfile was locked, and therefore didn't do the `stat_validity_check()`\nthat we want.\n\nThe mistake was compounded with a misleading comment in\n`lock_packed_refs()` claiming that it was doing the right thing. That\ncomment came from an earlier draft of the mh/packed-ref-store-prep\npatch series when the commits were in a different order.\n\nSo instead:\n\n* Extract a function `validate_packed_ref_cache()` that does the\n  validity check independent of whether the lock is held.\n\n* Change `get_packed_ref_cache()` to call the new function, but only\n  if the lock *isn't* held.\n\n* Change `lock_packed_refs()` to call the new function in any case\n  before calling `get_packed_ref_cache()`.\n\n* Fix the comment in `lock_packed_refs()`.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\nIf anybody is curious, patch 28ed9830b1 originally appeared before\n00d174489e (files_ref_store: put the packed files lock directly in\nthis struct, 2017-05-22), when `refs->packed_refs_lock` could be left\nNULL until after `get_packed_ref_cache()` was called. In that version\nof the patch series, the breakage occurred when the the lock was moved\nand the check in `get_packed_ref_cache()` was changed from\n`refs->packed_refs_lock` to\n`is_lock_file_locked(&refs->packed_refs_lock)`. So either way, it was\nthe combination of the two patches that caused the regression.\n\n refs/files-backend.c | 32 +++++++++++++++++++++++---------\n 1 file changed, 23 insertions(+), 9 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex d8b3f73147..b040bb3b0a 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -369,6 +369,18 @@ static void files_ref_path(struct files_ref_store *refs,\n \t}\n }\n \n+/*\n+ * Check that the packed refs cache (if any) still reflects the\n+ * contents of the file. If not, clear the cache.\n+ */\n+static void validate_packed_ref_cache(struct files_ref_store *refs)\n+{\n+\tif (refs->packed &&\n+\t    !stat_validity_check(&refs->packed->validity,\n+\t\t\t\t files_packed_refs_path(refs)))\n+\t\tclear_packed_ref_cache(refs);\n+}\n+\n /*\n  * Get the packed_ref_cache for the specified files_ref_store,\n  * creating and populating it if it hasn't been read before or if the\n@@ -381,10 +393,8 @@ static struct packed_ref_cache *get_packed_ref_cache(struct files_ref_store *ref\n {\n \tconst char *packed_refs_file = files_packed_refs_path(refs);\n \n-\tif (refs->packed &&\n-\t    !is_lock_file_locked(&refs->packed_refs_lock) &&\n-\t    !stat_validity_check(&refs->packed->validity, packed_refs_file))\n-\t\tclear_packed_ref_cache(refs);\n+\tif (!is_lock_file_locked(&refs->packed_refs_lock))\n+\t\tvalidate_packed_ref_cache(refs);\n \n \tif (!refs->packed)\n \t\trefs->packed = read_packed_refs(packed_refs_file);\n@@ -1311,13 +1321,17 @@ static int lock_packed_refs(struct files_ref_store *refs, int flags)\n \t\t\t    &refs->packed_refs_lock, files_packed_refs_path(refs),\n \t\t\t    flags, timeout_value) < 0)\n \t\treturn -1;\n+\n \t/*\n-\t * Get the current packed-refs while holding the lock. It is\n-\t * important that we call `get_packed_ref_cache()` before\n-\t * setting `packed_ref_cache->lock`, because otherwise the\n-\t * former will see that the file is locked and assume that the\n-\t * cache can't be stale.\n+\t * Now that we hold the `packed-refs` lock, make sure that our\n+\t * cache matches the current version of the file. Normally\n+\t * `get_packed_ref_cache()` does that for us, but that\n+\t * function assumes that when the file is locked, any existing\n+\t * cache is still valid. We've just locked the file, but it\n+\t * might have changed the moment *before* we locked it.\n \t */\n+\tvalidate_packed_ref_cache(refs);\n+\n \tpacked_ref_cache = get_packed_ref_cache(refs);\n \t/* Increment the reference count to prevent it from being freed: */\n \tacquire_packed_ref_cache(packed_ref_cache);\n-- \n2.11.0\n\n"},{"id":"321979","messageId":"20170612182142.6giu6ovjwx2fjkzp@sigill.intra.peff.net","threadId":"46158","inReplyTo":"18f059555ce0e4f46fba783973947f6a26ac4b65.1497253685.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 1/1] lock_packed_refs(): fix cache validity check","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-12T18:21:42Z","receivedAt":"2017-06-12T18:21:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 12, 2017 at 10:06:13AM +0200, Michael Haggerty wrote:\n\n> So instead:\n> \n> * Extract a function `validate_packed_ref_cache()` that does the\n>   validity check independent of whether the lock is held.\n> \n> * Change `get_packed_ref_cache()` to call the new function, but only\n>   if the lock *isn't* held.\n> \n> * Change `lock_packed_refs()` to call the new function in any case\n>   before calling `get_packed_ref_cache()`.\n> \n> * Fix the comment in `lock_packed_refs()`.\n\nYour explanation makes perfect sense, and I agree this should fix the\nproblem. Sorry to have missed it in review.\n\n-Peff\n"}]}