* `git fetch origin --prune --atomic` core dumped
[not found] <37599b30-dee2-4a36-8129-04fe5f6b633e.likui@oschina.cn>
@ 2025-03-20 8:36 ` 李葵
2025-03-20 16:10 ` Justin Tobler
0 siblings, 1 reply; 6+ messages in thread
From: 李葵 @ 2025-03-20 8:36 UTC (permalink / raw)
To: git
Thank you for filling out a Git bug report!
Please answer the following questions to help us understand your issue.
What did you do before the bug happened? (Steps to reproduce your issue)
```bash
git clone --quiet --mirror https://github.com/git/git-merge.git git-merge.git.git
git clone --quiet --mirror git-merge.git.git git-merge.git
git --git-dir=git-merge.git.git branch test main
touch git-merge.git/refs/heads/test.lock
git --git-dir=git-merge.git fetch origin --prune --atomic "+refs/*:refs/*"
```
What did you expect to happen? (Expected behavior)
Without core dumped.
What happened instead? (Actual behavior)
```bash
$ git clone --quiet --mirror https://github.com/git/git-merge.git git-merge.git.git
$ git clone --quiet --mirror git-merge.git.git git-merge.git
$ git --git-dir=git-merge.git.git branch test main
branch 'test' set up to track 'origin/main'.
$ touch git-merge.git/refs/heads/test.lock
$ git --git-dir=git-merge.git fetch origin --prune --atomic "+refs/*:refs/*"
From /tmp/git/git-merge.git
* [new branch] test -> test
error: cannot lock ref 'refs/heads/test': Unable to create '/tmp/git/git-merge.git/refs/heads/test.lock': File exists.
Another git process seems to be running in this repository, e.g.
an editor opened by 'git commit'. Please make sure all processes
are terminated then try again. If it still fails, a git process
may have crashed in this repository earlier:
remove the file manually to continue.
BUG: refs.c:2435: abort called on a closed reference transaction
[1] 1298748 IOT instruction (core dumped) git --git-dir=git-merge.git fetch origin --prune --atomic "+refs/*:refs/*"
$ ls
core.1298748 git-merge.git git-merge.git.git
```
What's different between what you expected and what actually happened?
Without core dumped.
Anything else you want to add:
Please review the rest of the bug report below.
You can delete any lines you don't wish to share.
[System Info]
git version:
git version 2.49.0.rc1.120.g683c54c999
cpu: x86_64
built from commit: 683c54c999c301c2cd6f715c411407c413b1d84e
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
libcurl: 7.81.0
OpenSSL: OpenSSL 3.0.2 15 Mar 2022
zlib: 1.2.11
uname: Linux 6.8.0-52-generic #53~22.04.1-Ubuntu SMP PREEMPT_DYNAMIC Wed Jan 15 19:18:46 UTC 2 x86_64
compiler info: gnuc: 11.4
libc info: glibc: 2.35
$SHELL (typically, interactive shell): /usr/bin/zsh
[Enabled Hooks]
not run from a git repository - no hooks to show
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: `git fetch origin --prune --atomic` core dumped
2025-03-20 8:36 ` `git fetch origin --prune --atomic` core dumped 李葵
@ 2025-03-20 16:10 ` Justin Tobler
2025-03-21 0:44 ` [PATCH] builtin/fetch: avoid aborting closed reference transaction Justin Tobler
0 siblings, 1 reply; 6+ messages in thread
From: Justin Tobler @ 2025-03-20 16:10 UTC (permalink / raw)
To: 李葵; +Cc: git, ps
On 25/03/20 04:36PM, 李葵 wrote:
> Thank you for filling out a Git bug report!
> Please answer the following questions to help us understand your issue.
>
> What did you do before the bug happened? (Steps to reproduce your issue)
>
> ```bash
> git clone --quiet --mirror https://github.com/git/git-merge.git git-merge.git.git
> git clone --quiet --mirror git-merge.git.git git-merge.git
>
> git --git-dir=git-merge.git.git branch test main
>
> touch git-merge.git/refs/heads/test.lock
> git --git-dir=git-merge.git fetch origin --prune --atomic "+refs/*:refs/*"
> ```
>
> What did you expect to happen? (Expected behavior)
>
> Without core dumped.
>
> What happened instead? (Actual behavior)
>
> ```bash
> $ git clone --quiet --mirror https://github.com/git/git-merge.git git-merge.git.git
>
> $ git clone --quiet --mirror git-merge.git.git git-merge.git
>
> $ git --git-dir=git-merge.git.git branch test main
> branch 'test' set up to track 'origin/main'.
>
> $ touch git-merge.git/refs/heads/test.lock
>
> $ git --git-dir=git-merge.git fetch origin --prune --atomic "+refs/*:refs/*"
> From /tmp/git/git-merge.git
> * [new branch] test -> test
> error: cannot lock ref 'refs/heads/test': Unable to create '/tmp/git/git-merge.git/refs/heads/test.lock': File exists.
>
> Another git process seems to be running in this repository, e.g.
> an editor opened by 'git commit'. Please make sure all processes
> are terminated then try again. If it still fails, a git process
> may have crashed in this repository earlier:
> remove the file manually to continue.
> BUG: refs.c:2435: abort called on a closed reference transaction
> [1] 1298748 IOT instruction (core dumped) git --git-dir=git-merge.git fetch origin --prune --atomic "+refs/*:refs/*"
>
> $ ls
> core.1298748 git-merge.git git-merge.git.git
> ```
>
> What's different between what you expected and what actually happened?
>
> Without core dumped.
>
> Anything else you want to add:
>
> Please review the rest of the bug report below.
> You can delete any lines you don't wish to share.
>
>
> [System Info]
> git version:
> git version 2.49.0.rc1.120.g683c54c999
> cpu: x86_64
> built from commit: 683c54c999c301c2cd6f715c411407c413b1d84e
> sizeof-long: 8
> sizeof-size_t: 8
> shell-path: /bin/sh
> libcurl: 7.81.0
> OpenSSL: OpenSSL 3.0.2 15 Mar 2022
> zlib: 1.2.11
> uname: Linux 6.8.0-52-generic #53~22.04.1-Ubuntu SMP PREEMPT_DYNAMIC Wed Jan 15 19:18:46 UTC 2 x86_64
> compiler info: gnuc: 11.4
> libc info: glibc: 2.35
> $SHELL (typically, interactive shell): /usr/bin/zsh
>
>
> [Enabled Hooks]
> not run from a git repository - no hooks to show
I was able to reproduce the reported bug with the following:
git init foo &&
git -C foo commit --allow-empty -m init &&
git clone --mirror foo bar.git &&
git -C bar.git branch test master &&
touch bar.git/refs/heads/test.lock &&
git -C bar.git fetch origin --prune --atomic "+refs/*:refs/*"
This bisects to c92abe71df (builtin/fetch: fix leaking transaction with
`--atomic`, 2024-08-22). cc'ing the author.
-Justin
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] builtin/fetch: avoid aborting closed reference transaction
2025-03-20 16:10 ` Justin Tobler
@ 2025-03-21 0:44 ` Justin Tobler
2025-03-24 10:40 ` Patrick Steinhardt
0 siblings, 1 reply; 6+ messages in thread
From: Justin Tobler @ 2025-03-21 0:44 UTC (permalink / raw)
To: git; +Cc: ps, likui, Justin Tobler
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@gmail.com>
---
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@oschina.cn>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] builtin/fetch: avoid aborting closed reference transaction
2025-03-21 0:44 ` [PATCH] builtin/fetch: avoid aborting closed reference transaction Justin Tobler
@ 2025-03-24 10:40 ` Patrick Steinhardt
2025-03-24 15:10 ` Justin Tobler
0 siblings, 1 reply; 6+ messages in thread
From: Patrick Steinhardt @ 2025-03-24 10:40 UTC (permalink / raw)
To: Justin Tobler; +Cc: git, likui
On Thu, Mar 20, 2025 at 07:44:37PM -0500, Justin Tobler wrote:
> 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()`.
Yeah, this is one of the more intricate parts of ref transactions, and
it has been biting me several times in the past. It feels somewhat
similar in spirit to how the `ref_iterator` used to automatically free
itself once it has reached its end, which led to the same class of bugs
due to the interface being way too intricate.
So I wonderer whether we should refactor this interface in the same way:
instead of automatically freeing the transaction on commit/abort, we'd
never do so and require the caller to always free it themselves. This
would make it way easier to use because we can now unconditionally free
the transaction everywhere.
That wouldn't help with the fixed bug though, which is that we call
abort after a failed commit even though the transaction was already
aborted.
> 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);
Okay, makes sense.
Thanks for the fix!
Patrick
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] builtin/fetch: avoid aborting closed reference transaction
2025-03-24 10:40 ` Patrick Steinhardt
@ 2025-03-24 15:10 ` Justin Tobler
2025-03-24 15:25 ` Patrick Steinhardt
0 siblings, 1 reply; 6+ messages in thread
From: Justin Tobler @ 2025-03-24 15:10 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, likui
On 25/03/24 11:40AM, Patrick Steinhardt wrote:
> On Thu, Mar 20, 2025 at 07:44:37PM -0500, Justin Tobler wrote:
> > 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()`.
>
> Yeah, this is one of the more intricate parts of ref transactions, and
> it has been biting me several times in the past. It feels somewhat
> similar in spirit to how the `ref_iterator` used to automatically free
> itself once it has reached its end, which led to the same class of bugs
> due to the interface being way too intricate.
>
> So I wonderer whether we should refactor this interface in the same way:
> instead of automatically freeing the transaction on commit/abort, we'd
> never do so and require the caller to always free it themselves. This
> would make it way easier to use because we can now unconditionally free
> the transaction everywhere.
I was also considering this. The interface here feels rather awkward
since aborted transactions free themselves automatically while committed
ones do not. It would be easier to reason about if the caller was always
reponsible for freeing the transaction.
> That wouldn't help with the fixed bug though, which is that we call
> abort after a failed commit even though the transaction was already
> aborted.
I wonder if it would make sense to stop closing the transaction on a
failed commit and require the caller to abort it. This would allow error
handling to unconditionally abort the transaction during cleanup.
I wouldn't mind sending a followup series to refactor these interfaces
if that is something we would be interested in.
-Justin
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] builtin/fetch: avoid aborting closed reference transaction
2025-03-24 15:10 ` Justin Tobler
@ 2025-03-24 15:25 ` Patrick Steinhardt
0 siblings, 0 replies; 6+ messages in thread
From: Patrick Steinhardt @ 2025-03-24 15:25 UTC (permalink / raw)
To: Justin Tobler; +Cc: git, likui
On Mon, Mar 24, 2025 at 10:10:44AM -0500, Justin Tobler wrote:
> On 25/03/24 11:40AM, Patrick Steinhardt wrote:
> > That wouldn't help with the fixed bug though, which is that we call
> > abort after a failed commit even though the transaction was already
> > aborted.
>
> I wonder if it would make sense to stop closing the transaction on a
> failed commit and require the caller to abort it. This would allow error
> handling to unconditionally abort the transaction during cleanup.
I think it might still feel somewhat awkward because now every caller
would have to both abort and free the transaction when the commit fails.
An alternative could be to make abortion idempotent, where aborting an
already aborted transaction is fine. But I'm not too sure whether that
would significantly improve things.
I don't really have a gut feeling here what the best route to go would
be.
Patrick
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-03-24 15:25 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <37599b30-dee2-4a36-8129-04fe5f6b633e.likui@oschina.cn>
2025-03-20 8:36 ` `git fetch origin --prune --atomic` core dumped 李葵
2025-03-20 16:10 ` Justin Tobler
2025-03-21 0:44 ` [PATCH] builtin/fetch: avoid aborting closed reference transaction Justin Tobler
2025-03-24 10:40 ` Patrick Steinhardt
2025-03-24 15:10 ` Justin Tobler
2025-03-24 15:25 ` Patrick Steinhardt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).