* `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 NNTP newsgroup(s).