From 6a2a7736d867dac39f2c54833fd0a65107c7cc5b Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Fri, 8 Sep 2017 15:51:50 +0200 Subject: t1404: demonstrate two problems with reference transactions Currently, a loose reference is deleted even before locking the `packed-refs` file, let alone deleting any packed version of the reference. This leads to two problems, demonstrated by two new tests: * While a reference is being deleted, other processes might see the old, packed value of the reference for a moment before the packed version is deleted. Normally this would be hard to observe, but we can prolong the window by locking the `packed-refs` file externally before running `update-ref`, then unlocking it before `update-ref`'s attempt to acquire the lock times out. * If the `packed-refs` file is locked so long that `update-ref` fails to lock it, then the reference can be left permanently in the incorrect state described in the previous point. In a moment, both problems will be fixed. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- t/t1404-update-ref-errors.sh | 73 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 73 insertions(+) (limited to 't') diff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh index c34ece48f5..64a81345a8 100755 --- a/t/t1404-update-ref-errors.sh +++ b/t/t1404-update-ref-errors.sh @@ -404,4 +404,77 @@ test_expect_success 'broken reference blocks indirect create' ' test_cmp expected output.err ' +test_expect_failure 'no bogus intermediate values during delete' ' + prefix=refs/slow-transaction && + # Set up a reference with differing loose and packed versions: + git update-ref $prefix/foo $C && + git pack-refs --all && + git update-ref $prefix/foo $D && + git for-each-ref $prefix >unchanged && + # Now try to update the reference, but hold the `packed-refs` lock + # for a while to see what happens while the process is blocked: + : >.git/packed-refs.lock && + test_when_finished "rm -f .git/packed-refs.lock" && + { + # Note: the following command is intentionally run in the + # background. We increase the timeout so that `update-ref` + # attempts to acquire the `packed-refs` lock for longer than + # it takes for us to do the check then delete it: + git -c core.packedrefstimeout=3000 update-ref -d $prefix/foo & + } && + pid2=$! && + # Give update-ref plenty of time to get to the point where it tries + # to lock packed-refs: + sleep 1 && + # Make sure that update-ref did not complete despite the lock: + kill -0 $pid2 && + # Verify that the reference still has its old value: + sha1=$(git rev-parse --verify --quiet $prefix/foo || echo undefined) && + case "$sha1" in + $D) + # This is what we hope for; it means that nothing + # user-visible has changed yet. + : ;; + undefined) + # This is not correct; it means the deletion has happened + # already even though update-ref should not have been + # able to acquire the lock yet. + echo "$prefix/foo deleted prematurely" && + break + ;; + $C) + # This value should never be seen. Probably the loose + # reference has been deleted but the packed reference + # is still there: + echo "$prefix/foo incorrectly observed to be C" && + break + ;; + *) + # WTF? + echo "unexpected value observed for $prefix/foo: $sha1" && + break + ;; + esac >out && + rm -f .git/packed-refs.lock && + wait $pid2 && + test_must_be_empty out && + test_must_fail git rev-parse --verify --quiet $prefix/foo +' + +test_expect_failure 'delete fails cleanly if packed-refs file is locked' ' + prefix=refs/locked-packed-refs && + # Set up a reference with differing loose and packed versions: + git update-ref $prefix/foo $C && + git pack-refs --all && + git update-ref $prefix/foo $D && + git for-each-ref $prefix >unchanged && + # Now try to delete it while the `packed-refs` lock is held: + : >.git/packed-refs.lock && + test_when_finished "rm -f .git/packed-refs.lock" && + test_must_fail git update-ref -d $prefix/foo >out 2>err && + git for-each-ref $prefix >actual && + test_i18ngrep "Unable to create $Q.*packed-refs.lock$Q: File exists" err && + test_cmp unchanged actual +' + test_done -- cgit v1.2.3 From dc39e099422b1d44f6230f557f94f7945c7521a7 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Fri, 8 Sep 2017 15:51:51 +0200 Subject: files_ref_store: use a transaction to update packed refs When processing a `files_ref_store` transaction, it is sometimes necessary to delete some references from the "packed-refs" file. Do that using a reference transaction conducted against the `packed_ref_store`. This change further decouples `files_ref_store` from `packed_ref_store`. It also fixes multiple problems, including the two revealed by test cases added in the previous commit. First, the old code didn't obtain the `packed-refs` lock until `files_transaction_finish()`. This means that a failure to acquire the `packed-refs` lock (e.g., due to contention with another process) wasn't detected until it was too late (problems like this are supposed to be detected in the "prepare" phase). The new code acquires the `packed-refs` lock in `files_transaction_prepare()`, the same stage of the processing when the loose reference locks are being acquired, removing another reason why the "prepare" phase might succeed and the "finish" phase might nevertheless fail. Second, the old code deleted the loose version of a reference before deleting any packed version of the same reference. This left a moment when another process might think that the packed version of the reference is current, which is incorrect. (Even worse, the packed version of the reference can be arbitrarily old, and might even point at an object that has since been garbage-collected.) Third, if a reference deletion fails to acquire the `packed-refs` lock altogether, then the old code might leave the repository in the incorrect state (possibly corrupt) described in the previous paragraph. Now we activate the new "packed-refs" file (sans any references that are being deleted) *before* deleting the corresponding loose references. But we hold the "packed-refs" lock until after the loose references have been finalized, thus preventing a simultaneous "pack-refs" process from packing the loose version of the reference in the time gap, which would otherwise defeat our attempt to delete it. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- t/t1404-update-ref-errors.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 't') diff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh index 64a81345a8..100d50e362 100755 --- a/t/t1404-update-ref-errors.sh +++ b/t/t1404-update-ref-errors.sh @@ -404,7 +404,7 @@ test_expect_success 'broken reference blocks indirect create' ' test_cmp expected output.err ' -test_expect_failure 'no bogus intermediate values during delete' ' +test_expect_success 'no bogus intermediate values during delete' ' prefix=refs/slow-transaction && # Set up a reference with differing loose and packed versions: git update-ref $prefix/foo $C && @@ -461,7 +461,7 @@ test_expect_failure 'no bogus intermediate values during delete' ' test_must_fail git rev-parse --verify --quiet $prefix/foo ' -test_expect_failure 'delete fails cleanly if packed-refs file is locked' ' +test_expect_success 'delete fails cleanly if packed-refs file is locked' ' prefix=refs/locked-packed-refs && # Set up a reference with differing loose and packed versions: git update-ref $prefix/foo $C && -- cgit v1.2.3