`git fetch origin --prune --atomic` core dumped

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

On 25/03/20 04:36PM, 李葵 wrote:

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

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(-)

On Thu, Mar 20, 2025 at 07:44:37PM -0500, Justin Tobler wrote:

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.

Okay, makes sense.

Thanks for the fix!

Patrick

On 25/03/24 11:40AM, Patrick Steinhardt wrote:

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.

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

On Mon, Mar 24, 2025 at 10:10:44AM -0500, Justin Tobler wrote:

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