{"thread":{"id":"63167","subject":"`git fetch origin --prune --atomic` core dumped","startedAt":"2025-03-20T08:36:33Z","lastAt":"2025-03-24T15:25:46Z","messageCount":6,"participants":["李葵","Justin Tobler","Patrick Steinhardt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"514721","messageId":"e8789a03-41ea-42e2-9f2d-5124b849277a.likui@oschina.cn","threadId":"63167","inReplyTo":"37599b30-dee2-4a36-8129-04fe5f6b633e.likui@oschina.cn","subject":"`git fetch origin --prune --atomic` core dumped","fromName":"李葵","fromEmail":"likui@oschina.cn","sentAt":"2025-03-20T08:36:26Z","receivedAt":"2025-03-20T08:36:33Z","isPatch":false,"sender":{"key":"likui@oschina.cn","avatar":null},"body":"Thank you for filling out a Git bug report!\nPlease answer the following questions to help us understand your issue.\n\nWhat did you do before the bug happened? (Steps to reproduce your issue)\n\n  ```bash\n  git clone --quiet --mirror https://github.com/git/git-merge.git git-merge.git.git\n  git clone --quiet --mirror git-merge.git.git git-merge.git\n\n  git --git-dir=git-merge.git.git branch test main\n\n  touch git-merge.git/refs/heads/test.lock\n  git --git-dir=git-merge.git fetch origin --prune --atomic \"+refs/*:refs/*\"\n  ```\n\nWhat did you expect to happen? (Expected behavior)\n\n  Without core dumped.\n\nWhat happened instead? (Actual behavior)\n\n  ```bash\n  $ git clone --quiet --mirror https://github.com/git/git-merge.git git-merge.git.git\n\n  $ git clone --quiet --mirror git-merge.git.git git-merge.git\n\n  $ git --git-dir=git-merge.git.git branch test main\n  branch 'test' set up to track 'origin/main'.\n\n  $ touch git-merge.git/refs/heads/test.lock\n\n  $ git --git-dir=git-merge.git fetch origin --prune --atomic \"+refs/*:refs/*\"\n  From /tmp/git/git-merge.git\n   * [new branch]      test       -> test\n  error: cannot lock ref 'refs/heads/test': Unable to create '/tmp/git/git-merge.git/refs/heads/test.lock': File exists.\n  \n  Another git process seems to be running in this repository, e.g.\n  an editor opened by 'git commit'. Please make sure all processes\n  are terminated then try again. If it still fails, a git process\n  may have crashed in this repository earlier:\n  remove the file manually to continue.\n  BUG: refs.c:2435: abort called on a closed reference transaction\n  [1]    1298748 IOT instruction (core dumped)  git --git-dir=git-merge.git fetch origin --prune --atomic \"+refs/*:refs/*\"\n\n  $ ls\n  core.1298748  git-merge.git  git-merge.git.git\n  ```\n\nWhat's different between what you expected and what actually happened?\n\n  Without core dumped.\n\nAnything else you want to add:\n\nPlease review the rest of the bug report below.\nYou can delete any lines you don't wish to share.\n\n\n[System Info]\ngit version:\ngit version 2.49.0.rc1.120.g683c54c999\ncpu: x86_64\nbuilt from commit: 683c54c999c301c2cd6f715c411407c413b1d84e\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nlibcurl: 7.81.0\nOpenSSL: OpenSSL 3.0.2 15 Mar 2022\nzlib: 1.2.11\nuname: Linux 6.8.0-52-generic #53~22.04.1-Ubuntu SMP PREEMPT_DYNAMIC Wed Jan 15 19:18:46 UTC 2 x86_64\ncompiler info: gnuc: 11.4\nlibc info: glibc: 2.35\n$SHELL (typically, interactive shell): /usr/bin/zsh\n\n\n[Enabled Hooks]\nnot run from a git repository - no hooks to show\n\n\n"},{"id":"514779","messageId":"g4baz2kt25ysb6wcesoqxhvw2ooxkmqio3dj6b44h6gt5l6z3r@rocsjlys5nqs","threadId":"63167","inReplyTo":"e8789a03-41ea-42e2-9f2d-5124b849277a.likui@oschina.cn","subject":"Re: `git fetch origin --prune --atomic` core dumped","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2025-03-20T16:10:19Z","receivedAt":"2025-03-20T16:13:55Z","isPatch":false,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 25/03/20 04:36PM, 李葵 wrote:\n> Thank you for filling out a Git bug report!\n> Please answer the following questions to help us understand your issue.\n> \n> What did you do before the bug happened? (Steps to reproduce your issue)\n> \n>   ```bash\n>   git clone --quiet --mirror https://github.com/git/git-merge.git git-merge.git.git\n>   git clone --quiet --mirror git-merge.git.git git-merge.git\n> \n>   git --git-dir=git-merge.git.git branch test main\n> \n>   touch git-merge.git/refs/heads/test.lock\n>   git --git-dir=git-merge.git fetch origin --prune --atomic \"+refs/*:refs/*\"\n>   ```\n> \n> What did you expect to happen? (Expected behavior)\n> \n>   Without core dumped.\n> \n> What happened instead? (Actual behavior)\n> \n>   ```bash\n>   $ git clone --quiet --mirror https://github.com/git/git-merge.git git-merge.git.git\n> \n>   $ git clone --quiet --mirror git-merge.git.git git-merge.git\n> \n>   $ git --git-dir=git-merge.git.git branch test main\n>   branch 'test' set up to track 'origin/main'.\n> \n>   $ touch git-merge.git/refs/heads/test.lock\n> \n>   $ git --git-dir=git-merge.git fetch origin --prune --atomic \"+refs/*:refs/*\"\n>   From /tmp/git/git-merge.git\n>    * [new branch]      test       -> test\n>   error: cannot lock ref 'refs/heads/test': Unable to create '/tmp/git/git-merge.git/refs/heads/test.lock': File exists.\n>   \n>   Another git process seems to be running in this repository, e.g.\n>   an editor opened by 'git commit'. Please make sure all processes\n>   are terminated then try again. If it still fails, a git process\n>   may have crashed in this repository earlier:\n>   remove the file manually to continue.\n>   BUG: refs.c:2435: abort called on a closed reference transaction\n>   [1]    1298748 IOT instruction (core dumped)  git --git-dir=git-merge.git fetch origin --prune --atomic \"+refs/*:refs/*\"\n> \n>   $ ls\n>   core.1298748  git-merge.git  git-merge.git.git\n>   ```\n> \n> What's different between what you expected and what actually happened?\n> \n>   Without core dumped.\n> \n> Anything else you want to add:\n> \n> Please review the rest of the bug report below.\n> You can delete any lines you don't wish to share.\n> \n> \n> [System Info]\n> git version:\n> git version 2.49.0.rc1.120.g683c54c999\n> cpu: x86_64\n> built from commit: 683c54c999c301c2cd6f715c411407c413b1d84e\n> sizeof-long: 8\n> sizeof-size_t: 8\n> shell-path: /bin/sh\n> libcurl: 7.81.0\n> OpenSSL: OpenSSL 3.0.2 15 Mar 2022\n> zlib: 1.2.11\n> 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\n> compiler info: gnuc: 11.4\n> libc info: glibc: 2.35\n> $SHELL (typically, interactive shell): /usr/bin/zsh\n> \n> \n> [Enabled Hooks]\n> not run from a git repository - no hooks to show\n\nI was able to reproduce the reported bug with the following:\n\n    git init foo &&\n    git -C foo commit --allow-empty -m init &&\n    git clone --mirror foo bar.git &&\n    git -C bar.git branch test master &&\n    touch bar.git/refs/heads/test.lock &&\n    git -C bar.git fetch origin --prune --atomic \"+refs/*:refs/*\"\n\nThis bisects to c92abe71df (builtin/fetch: fix leaking transaction with\n`--atomic`, 2024-08-22). cc'ing the author.\n\n-Justin\n"},{"id":"514815","messageId":"20250321004437.505461-1-jltobler@gmail.com","threadId":"63167","inReplyTo":"g4baz2kt25ysb6wcesoqxhvw2ooxkmqio3dj6b44h6gt5l6z3r@rocsjlys5nqs","subject":"[PATCH] builtin/fetch: avoid aborting closed reference transaction","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2025-03-21T00:44:37Z","receivedAt":"2025-03-21T00:57:47Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"As part of the reference transaction commit phase, the transaction is\nset to a closed state regardless of whether it was successful of not.\nAttempting to abort a closed transaction via `ref_transaction_abort()`\nresults in a `BUG()`.\n\nIn c92abe71df (builtin/fetch: fix leaking transaction with `--atomic`,\n2024-08-22), logic to free a transaction after the commit phase is moved\nto the centralized exit path. In cases where the transaction commit\nfailed, this results in a closed transaction being aborted and signaling\na bug.\n\nFree the transaction and set it to NULL when the commit fails. This\nallows the exit path to correctly handle the error without attempting to\nabort the transaction.\n\nSigned-off-by: Justin Tobler <jltobler@gmail.com>\n---\n\nGreetings,\n\nIt has been reported[1] that executing git-fetch(1) using transactions\ncan result in an unexpected bug message appearing in some scenarios\nwhere it previously did not. This issue can be reproduced with the\nfollowing commands:\n\n        git init foo &&\n        git -C foo commit --allow-empty -m 1 &&\n        git clone --mirror foo bar &&\n        git -C foo commit --allow-empty -m 2 &&\n        touch bar/refs/heads/master.lock &&\n        git -C bar fetch --atomic origin master\n\nIn this example, the reference updates for the fetch are done in a\ntransaction. Since one of the references is already locked, the\ntransaction is expected to fail, but an unexpected \"BUG: refs.c:2435:\nabort called on a closed reference transaction\" message also gets\nprinted.\n\nThis issue bisects to c92abe71df (builtin/fetch: fix leaking transaction\nwith `--atomic`, 2024-08-22) which changes how transaction cleanup is\nhandled to address a memory leak. This change starts relying on using\n`ref_transaction_abort()` to free the transaction when an error occurs\nduring `ref_transaction_commit()`. This is problematic because\n`ref_transaction_commit()` always closes the transaction and\n`ref_transaction_abort()` cannot be invoked on a closed transaction.\nThis explains why we start to see the `BUG()` message.\n\nThis patch takes a simple approach to fix the issue by explicitly\nfreeing the transaction and setting it to NULL if the commit phase\nfails.\n\nI've included a test that expects the reference transaction to fail when\na reference is already locked. If the `BUG()` was still being printed\nthe process would be aborted resulting in the test failing. This test\nunfortunately relies on the files backend to create the lock file. I\nwould prefer to be reference backend agnostic, but am unsure of an easy\nway to exercise the issue.\n\n-Justin\n\n[1]: <e8789a03-41ea-42e2-9f2d-5124b849277a.likui@oschina.cn>\n\n---\n builtin/fetch.c  |  9 ++++++++-\n t/t5510-fetch.sh | 13 +++++++++++++\n 2 files changed, 21 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 95fd0018b9..f2555731f4 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1867,8 +1867,15 @@ static int do_fetch(struct transport *transport,\n \t\t\tgoto cleanup;\n \n \t\tretcode = ref_transaction_commit(transaction, &err);\n-\t\tif (retcode)\n+\t\tif (retcode) {\n+\t\t\t/*\n+\t\t\t * Explicitly handle transaction cleanup to avoid\n+\t\t\t * aborting an already closed transaction.\n+\t\t\t */\n+\t\t\tref_transaction_free(transaction);\n+\t\t\ttransaction = NULL;\n \t\t\tgoto cleanup;\n+\t\t}\n \t}\n \n \tcommit_fetch_head(&fetch_head);\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 5f350facf5..690755d2a8 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -537,6 +537,19 @@ test_expect_success 'fetch --atomic --append appends to FETCH_HEAD' '\n \ttest_cmp expected atomic/.git/FETCH_HEAD\n '\n \n+test_expect_success REFFILES 'fetch --atomic fails transaction if reference locked' '\n+\ttest_when_finished \"rm -rf upstream repo\" &&\n+\n+\tgit init upstream &&\n+\tgit -C upstream commit --allow-empty -m 1 &&\n+\tgit -C upstream switch -c foobar &&\n+\tgit clone --mirror upstream repo &&\n+\tgit -C upstream commit --allow-empty -m 2 &&\n+\ttouch repo/refs/heads/foobar.lock &&\n+\n+\ttest_must_fail git -C repo fetch --atomic origin\n+'\n+\n test_expect_success '--refmap=\"\" ignores configured refspec' '\n \tcd \"$TRASH_DIRECTORY\" &&\n \tgit clone \"$D\" remote-refs &&\n\nbase-commit: 683c54c999c301c2cd6f715c411407c413b1d84e\n-- \n2.49.0\n\n"},{"id":"514914","messageId":"Z-E2ns9VziejSLB2@pks.im","threadId":"63167","inReplyTo":"20250321004437.505461-1-jltobler@gmail.com","subject":"Re: [PATCH] builtin/fetch: avoid aborting closed reference transaction","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-03-24T10:40:30Z","receivedAt":"2025-03-24T10:40:38Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Mar 20, 2025 at 07:44:37PM -0500, Justin Tobler wrote:\n> As part of the reference transaction commit phase, the transaction is\n> set to a closed state regardless of whether it was successful of not.\n> Attempting to abort a closed transaction via `ref_transaction_abort()`\n> results in a `BUG()`.\n\nYeah, this is one of the more intricate parts of ref transactions, and\nit has been biting me several times in the past. It feels somewhat\nsimilar in spirit to how the `ref_iterator` used to automatically free\nitself once it has reached its end, which led to the same class of bugs\ndue to the interface being way too intricate.\n\nSo I wonderer whether we should refactor this interface in the same way:\ninstead of automatically freeing the transaction on commit/abort, we'd\nnever do so and require the caller to always free it themselves. This\nwould make it way easier to use because we can now unconditionally free\nthe transaction everywhere.\n\nThat wouldn't help with the fixed bug though, which is that we call\nabort after a failed commit even though the transaction was already\naborted.\n\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 95fd0018b9..f2555731f4 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -1867,8 +1867,15 @@ static int do_fetch(struct transport *transport,\n>  \t\t\tgoto cleanup;\n>  \n>  \t\tretcode = ref_transaction_commit(transaction, &err);\n> -\t\tif (retcode)\n> +\t\tif (retcode) {\n> +\t\t\t/*\n> +\t\t\t * Explicitly handle transaction cleanup to avoid\n> +\t\t\t * aborting an already closed transaction.\n> +\t\t\t */\n> +\t\t\tref_transaction_free(transaction);\n> +\t\t\ttransaction = NULL;\n>  \t\t\tgoto cleanup;\n> +\t\t}\n>  \t}\n>  \n>  \tcommit_fetch_head(&fetch_head);\n\nOkay, makes sense.\n\nThanks for the fix!\n\nPatrick\n"},{"id":"514937","messageId":"vmmr2cwiyd2iz7c3yfyzym3l2aono7lsx7wig3pwmw33gst37h@orjtfygfyow4","threadId":"63167","inReplyTo":"Z-E2ns9VziejSLB2@pks.im","subject":"Re: [PATCH] builtin/fetch: avoid aborting closed reference transaction","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2025-03-24T15:10:44Z","receivedAt":"2025-03-24T15:14:25Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 25/03/24 11:40AM, Patrick Steinhardt wrote:\n> On Thu, Mar 20, 2025 at 07:44:37PM -0500, Justin Tobler wrote:\n> > As part of the reference transaction commit phase, the transaction is\n> > set to a closed state regardless of whether it was successful of not.\n> > Attempting to abort a closed transaction via `ref_transaction_abort()`\n> > results in a `BUG()`.\n> \n> Yeah, this is one of the more intricate parts of ref transactions, and\n> it has been biting me several times in the past. It feels somewhat\n> similar in spirit to how the `ref_iterator` used to automatically free\n> itself once it has reached its end, which led to the same class of bugs\n> due to the interface being way too intricate.\n> \n> So I wonderer whether we should refactor this interface in the same way:\n> instead of automatically freeing the transaction on commit/abort, we'd\n> never do so and require the caller to always free it themselves. This\n> would make it way easier to use because we can now unconditionally free\n> the transaction everywhere.\n\nI was also considering this. The interface here feels rather awkward\nsince aborted transactions free themselves automatically while committed\nones do not. It would be easier to reason about if the caller was always\nreponsible for freeing the transaction.\n\n> That wouldn't help with the fixed bug though, which is that we call\n> abort after a failed commit even though the transaction was already\n> aborted.\n\nI wonder if it would make sense to stop closing the transaction on a\nfailed commit and require the caller to abort it. This would allow error\nhandling to unconditionally abort the transaction during cleanup.\n\nI wouldn't mind sending a followup series to refactor these interfaces\nif that is something we would be interested in.\n\n-Justin\n"},{"id":"514953","messageId":"Z-F5dJXq0uCxpqoL@pks.im","threadId":"63167","inReplyTo":"vmmr2cwiyd2iz7c3yfyzym3l2aono7lsx7wig3pwmw33gst37h@orjtfygfyow4","subject":"Re: [PATCH] builtin/fetch: avoid aborting closed reference transaction","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-03-24T15:25:40Z","receivedAt":"2025-03-24T15:25:46Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Mar 24, 2025 at 10:10:44AM -0500, Justin Tobler wrote:\n> On 25/03/24 11:40AM, Patrick Steinhardt wrote:\n> > That wouldn't help with the fixed bug though, which is that we call\n> > abort after a failed commit even though the transaction was already\n> > aborted.\n> \n> I wonder if it would make sense to stop closing the transaction on a\n> failed commit and require the caller to abort it. This would allow error\n> handling to unconditionally abort the transaction during cleanup.\n\nI think it might still feel somewhat awkward because now every caller\nwould have to both abort and free the transaction when the commit fails.\nAn alternative could be to make abortion idempotent, where aborting an\nalready aborted transaction is fine. But I'm not too sure whether that\nwould significantly improve things.\n\nI don't really have a gut feeling here what the best route to go would\nbe.\n\nPatrick\n"}]}