{"thread":{"id":"6191","subject":"infinite loop with git branch -D and packed-refs","startedAt":"2007-01-02T06:44:01Z","lastAt":"2007-01-05T03:04:26Z","messageCount":6,"participants":["Nicholas Miell","Shawn O. Pearce","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"30656","messageId":"b566b20c0701012244l21f85472k83970c0c573ce105@mail.gmail.com","threadId":"6191","inReplyTo":null,"subject":"infinite loop with git branch -D and packed-refs","fromName":"Nicholas Miell","fromEmail":"nmiell@gmail.com","sentAt":"2007-01-02T06:44:01Z","receivedAt":"2007-01-02T06:44:01Z","isPatch":false,"sender":{"key":"nmiell@gmail.com","avatar":null},"body":"# this is with 1.4.4.2, spearce says master is also affected.\n# (not subscribed, please Cc:)\n\nmkdir test\ncd test\ngit init-db\ntouch blah\ngit add blah\ngit commit -m \"blah\"\ngit checkout -b A\ngit checkout -b B\ngit checkout master\ngit pack-refs --all --prune\ngit branch -D A B # --> infinite loop in lockfile.c:remove_lock_file()\n"},{"id":"30660","messageId":"20070102081709.GA28779@spearce.org","threadId":"6191","inReplyTo":"b566b20c0701012244l21f85472k83970c0c573ce105@mail.gmail.com","subject":"[PATCH] Fix infinite loop when deleting multiple packed refs.","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-01-02T08:17:09Z","receivedAt":"2007-01-02T08:17:09Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Nicholas Miell reported that `git branch -D A B` failed if both refs\nA and B were packed into .git/packed-refs. This happens because the\nsame pack_lock instance was being enqueued into the lock list twice,\ncausing the linked list to change from a singly linked list with\na NULL at the end to a circularly linked list with no termination.\nThis resulted in an infinite loop traversing the list during exit.\n\nSigned-off-by: Shawn O. Pearce \n---\n\n Nicholas Miell wrote:\n > # this is with 1.4.4.2, spearce says master is also affected.\n > # (not subscribed, please Cc:)\n > \n > mkdir test\n > cd test\n > git init-db\n > touch blah\n > git add blah\n > git commit -m \"blah\"\n > git checkout -b A\n > git checkout -b B\n > git checkout master\n > git pack-refs --all --prune\n > git branch -D A B # --> infinite loop in lockfile.c:remove_lock_file()\n\n Fixed. ;-)\n\n Junio, this applies to master, but hopefully could also apply to\n maint, as the bug also shows up there.\n\n refs.c | 9 ++++-----\n 1 files changed, 4 insertions(+), 5 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex e88ed8b..f1d1a5d 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -709,10 +709,9 @@ struct ref_lock *lock_any_ref_for_update(const char *ref, const unsigned char *o\n \treturn lock_ref_sha1_basic(ref, old_sha1, NULL);\n }\n \n-static struct lock_file packlock;\n-\n static int repack_without_ref(const char *refname)\n {\n+\tstruct lock_file *packlock;\n \tstruct ref_list *list, *packed_ref_list;\n \tint fd;\n \tint found = 0;\n@@ -726,8 +725,8 @@ static int repack_without_ref(const char *refname)\n \t}\n \tif (!found)\n \t\treturn 0;\n-\tmemset(&packlock, 0, sizeof(packlock));\n-\tfd = hold_lock_file_for_update(&packlock, git_path(\"packed-refs\"), 0);\n+\tpacklock = calloc(1, sizeof(*packlock));\n+\tfd = hold_lock_file_for_update(packlock, git_path(\"packed-refs\"), 0);\n \tif (fd < 0)\n \t\treturn error(\"cannot delete '%s' from packed refs\", refname);\n \n@@ -744,7 +743,7 @@ static int repack_without_ref(const char *refname)\n \t\t\tdie(\"too long a refname '%s'\", list->name);\n \t\twrite_or_die(fd, line, len);\n \t}\n-\treturn commit_lock_file(&packlock);\n+\treturn commit_lock_file(packlock);\n }\n \n int delete_ref(const char *refname, unsigned char *sha1)\n-- \n1.5.0.rc0.gab5a\n"},{"id":"30663","messageId":"7vtzz9x1fo.fsf@assigned-by-dhcp.cox.net","threadId":"6191","inReplyTo":"20070102081709.GA28779@spearce.org","subject":"Re: [PATCH] Fix infinite loop when deleting multiple packed refs.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-01-02T08:44:43Z","receivedAt":"2007-01-02T08:44:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" writes:\n\n> Fixed. ;-)\n>\n> Junio, this applies to master, but hopefully could also apply to\n> maint, as the bug also shows up there.\n\nI see a few instances of single static lock_file variable in our\ncode, but all of them seem to be locking the index and only\nonce, so they should be safe.\n\nThanks.\n"},{"id":"30664","messageId":"20070102084741.GA28898@spearce.org","threadId":"6191","inReplyTo":"7vtzz9x1fo.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Fix infinite loop when deleting multiple packed refs.","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-01-02T08:47:41Z","receivedAt":"2007-01-02T08:47:41Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano wrote:\n> \"Shawn O. Pearce\" writes:\n> \n> > Fixed. ;-)\n> >\n> > Junio, this applies to master, but hopefully could also apply to\n> > maint, as the bug also shows up there.\n> \n> I see a few instances of single static lock_file variable in our\n> code, but all of them seem to be locking the index and only\n> once, so they should be safe.\n> \n> Thanks.\n\nI just realized that my patch used 'calloc' and not 'xcalloc'.\nWould you mind correcting it for me? ;-)\n\n-- \nShawn.\n"},{"id":"30681","messageId":"7vy7oluthy.fsf@assigned-by-dhcp.cox.net","threadId":"6191","inReplyTo":"20070102081709.GA28779@spearce.org","subject":"Re: [PATCH] Fix infinite loop when deleting multiple packed refs.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-01-02T19:19:05Z","receivedAt":"2007-01-02T19:19:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" writes:\n\n> Nicholas Miell reported that `git branch -D A B` failed if both refs\n> A and B were packed into .git/packed-refs. This happens because the\n> same pack_lock instance was being enqueued into the lock list twice,\n\nI do not like this, actually.\n\nIt was stupid to link the same element twice to lock_file_list\nand end up in a loop, so we certainly need a fix.\n\nBut it is not like we are taking a lock on multiple files in\nthis case. It is just that we leave the linked list element on\nthe list even after commit_lock_file() successfully removes the\ncruft.\n\nWe obviously cannot remove the list element commit_lock_file();\nif we are interrupted in the middle of list manipulation, the\ncall to remove_lock_file_on_signal will happen with a broken\nlock_file_list, which would cause the cruft to remain, so not\nunlinking is the right thing to do. Instead we should be\nreusing the element already on the list.\n\nI notice that there is already a code for that in lock_file()\nfunction in lockfile.c. Notice that lk->next is checked and the\nelement is linked only when it is not already on the list? I\nthink the check is wrong for the last element on the list which\nhas NULL in next but is on the list, but if you read the check\nas \"is this element already on the list?\" it actually makes\nsense. We do not want to link it on the list again, nor we\nwould want to set up signal/atexit over and over.\n\nIn other words, I am suspecting this might be a cleaner fix.\n\n-- >8 --\n\ndiff --git a/cache.h b/cache.h\nindex a5fc232..4dbf658 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -179,6 +179,7 @@ extern int refresh_cache(unsigned int flags);\n \n struct lock_file {\n \tstruct lock_file *next;\n+\tchar on_list;\n \tchar filename[PATH_MAX];\n };\n extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);\ndiff --git a/lockfile.c b/lockfile.c\nindex 261baff..731bbf3 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -27,9 +27,12 @@ static int lock_file(struct lock_file *lk, const char *path)\n \tsprintf(lk->filename, \"%s.lock\", path);\n \tfd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);\n \tif (0 <= fd) {\n-\t\tif (!lk->next) {\n+\t\tif (!lk->on_list) {\n \t\t\tlk->next = lock_file_list;\n \t\t\tlock_file_list = lk;\n+\t\t\tlk->on_list = 1;\n+\t\t}\n+\t\tif (lock_file_list) {\n \t\t\tsignal(SIGINT, remove_lock_file_on_signal);\n \t\t\tatexit(remove_lock_file);\n \t\t}\n@@ -37,6 +40,8 @@ static int lock_file(struct lock_file *lk, const char *path)\n \t\t\treturn error(\"cannot fix permission bits on %s\",\n \t\t\t\t lk->filename);\n \t}\n+\telse\n+\t\tlk->filename[0] = 0;\n \treturn fd;\n }\n \ndiff --git a/refs.c b/refs.c\nindex 121774c..5205745 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -726,7 +726,6 @@ static int repack_without_ref(const char *refname)\n \t}\n \tif (!found)\n \t\treturn 0;\n-\tmemset(&packlock, 0, sizeof(packlock));\n \tfd = hold_lock_file_for_update(&packlock, git_path(\"packed-refs\"), 0);\n \tif (fd < 0)\n \t\treturn error(\"cannot delete '%s' from packed refs\", refname);\n"},{"id":"30855","messageId":"20070105030426.GD14042@spearce.org","threadId":"6191","inReplyTo":"7vy7oluthy.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Fix infinite loop when deleting multiple packed refs.","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-01-05T03:04:26Z","receivedAt":"2007-01-05T03:04:26Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano wrote:\n> \"Shawn O. Pearce\" writes:\n> \n> > Nicholas Miell reported that `git branch -D A B` failed if both refs\n> > A and B were packed into .git/packed-refs. This happens because the\n> > same pack_lock instance was being enqueued into the lock list twice,\n> \n> I do not like this, actually.\n[snip]\n> In other words, I am suspecting this might be a cleaner fix.\n\nAck'd. I like your patch better.\n\n-- \nShawn.\n"}]}