As part of the reference transaction commit phase, the transaction is
set to a closed state regardless of whether it was successful of not.
Attempting to abort a closed transaction via `ref_transaction_abort()`
results in a `BUG()`.
In c92abe71df (builtin/fetch: fix leaking transaction with `--atomic`,
2024-08-22), logic to free a transaction after the commit phase is moved
to the centralized exit path. In cases where the transaction commit
failed, this results in a closed transaction being aborted and signaling
a bug.
Free the transaction and set it to NULL when the commit fails. This
allows the exit path to correctly handle the error without attempting to
abort the transaction.
Signed-off-by: Justin Tobler <jltobler@xxxxxxxxx>
---
Greetings,
It has been reported[1] that executing git-fetch(1) using transactions
can result in an unexpected bug message appearing in some scenarios
where it previously did not. This issue can be reproduced with the
following commands:
git init foo &&
git -C foo commit --allow-empty -m 1 &&
git clone --mirror foo bar &&
git -C foo commit --allow-empty -m 2 &&
touch bar/refs/heads/master.lock &&
git -C bar fetch --atomic origin master
In this example, the reference updates for the fetch are done in a
transaction. Since one of the references is already locked, the
transaction is expected to fail, but an unexpected "BUG: refs.c:2435:
abort called on a closed reference transaction" message also gets
printed.
This issue bisects to c92abe71df (builtin/fetch: fix leaking transaction
with `--atomic`, 2024-08-22) which changes how transaction cleanup is
handled to address a memory leak. This change starts relying on using
`ref_transaction_abort()` to free the transaction when an error occurs
during `ref_transaction_commit()`. This is problematic because
`ref_transaction_commit()` always closes the transaction and
`ref_transaction_abort()` cannot be invoked on a closed transaction.
This explains why we start to see the `BUG()` message.
This patch takes a simple approach to fix the issue by explicitly
freeing the transaction and setting it to NULL if the commit phase
fails.
I've included a test that expects the reference transaction to fail when
a reference is already locked. If the `BUG()` was still being printed
the process would be aborted resulting in the test failing. This test
unfortunately relies on the files backend to create the lock file. I
would prefer to be reference backend agnostic, but am unsure of an easy
way to exercise the issue.
-Justin
[1]: <e8789a03-41ea-42e2-9f2d-5124b849277a.likui@xxxxxxxxxx>
---
builtin/fetch.c | 9 ++++++++-
t/t5510-fetch.sh | 13 +++++++++++++
2 files changed, 21 insertions(+), 1 deletion(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 95fd0018b9..f2555731f4 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1867,8 +1867,15 @@ static int do_fetch(struct transport *transport,
goto cleanup;
retcode = ref_transaction_commit(transaction, &err);
- if (retcode)
+ if (retcode) {
+ /*
+ * Explicitly handle transaction cleanup to avoid
+ * aborting an already closed transaction.
+ */
+ ref_transaction_free(transaction);
+ transaction = NULL;
goto cleanup;
+ }
}
commit_fetch_head(&fetch_head);
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 5f350facf5..690755d2a8 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -537,6 +537,19 @@ test_expect_success 'fetch --atomic --append appends to FETCH_HEAD' '
test_cmp expected atomic/.git/FETCH_HEAD
'
+test_expect_success REFFILES 'fetch --atomic fails transaction if reference locked' '
+ test_when_finished "rm -rf upstream repo" &&
+
+ git init upstream &&
+ git -C upstream commit --allow-empty -m 1 &&
+ git -C upstream switch -c foobar &&
+ git clone --mirror upstream repo &&
+ git -C upstream commit --allow-empty -m 2 &&
+ touch repo/refs/heads/foobar.lock &&
+
+ test_must_fail git -C repo fetch --atomic origin
+'
+
test_expect_success '--refmap="" ignores configured refspec' '
cd "$TRASH_DIRECTORY" &&
git clone "$D" remote-refs &&
base-commit: 683c54c999c301c2cd6f715c411407c413b1d84e
--
2.49.0
