{"thread":{"id":"51921","subject":"[PATCH] promisor-remote: skip move_to_tail when n=1","startedAt":"2019-09-25T21:37:26Z","lastAt":"2019-10-01T05:12:37Z","messageCount":10,"participants":["Emily Shaffer","Jeff King","Christian Couder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"382947","messageId":"20190925213718.231231-1-emilyshaffer@google.com","threadId":"51921","inReplyTo":null,"subject":"[PATCH] promisor-remote: skip move_to_tail when n=1","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-09-25T21:37:18Z","receivedAt":"2019-09-25T21:37:26Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"Previously, when promisor_remote_move_to_tail() is called for a\npromisor_remote which is currently the *only* element in promisors, a\ncycle is created in the promisors linked list. This cycle leads to a\ndouble free later on in promisor_remote_clear(): promisors is set to\npromisors->next (a no-op, as promisors->next == promisors); the previous\nvalue of promisors is free()'d; then the new value of promisors (which\nis equal to the previous value of promisors) is also free()'d. This\ndouble-free error was unrecoverable for the user without removing the\nfilter or re-cloning the repo and hoping to miss this edge case.\n\nNow, when promisor_remote_move_to_tail() would be a no-op, just do a\nno-op. In cases of promisor_remote_move_to_tail() where n>1, it works\ncorrectly.\n\nSigned-off-by: Emily Shaffer <emilyshaffer@google.com>\n---\nThis change showed up for us in a user bugreport; I'm actually fairly\nunfamiliar with the codebase here but given the drastic nature of the\nfailure, I wanted to get a fix up quickly. I'm still working on how to\nreproduce this exact case in the test suite (and actually would\nappreciate any pointers). Specifically, it looks like we only really\nbreak if we have a single promisor_remote in the linked list, call\nmove_to_tail() on it at least once, and then call clear() on it without\nadding another promisor_remote first.\n\n promisor-remote.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex 9bc296cdde..dccd697c2d 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -89,6 +89,9 @@ static struct promisor_remote *promisor_remote_lookup(const char *remote_name,\n static void promisor_remote_move_to_tail(struct promisor_remote *r,\n \t\t\t\t\t struct promisor_remote *previous)\n {\n+\tif (promisors == r && promisors->next == NULL)\n+\t\treturn;\n+\n \tif (previous)\n \t\tprevious->next = r->next;\n \telse\n-- \n2.23.0.351.gc4317032e6-goog\n\n"},{"id":"382957","messageId":"20190926075535.GC20653@sigill.intra.peff.net","threadId":"51921","inReplyTo":"20190925213718.231231-1-emilyshaffer@google.com","subject":"Re: [PATCH] promisor-remote: skip move_to_tail when n=1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-26T07:55:35Z","receivedAt":"2019-09-26T07:55:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 25, 2019 at 02:37:18PM -0700, Emily Shaffer wrote:\n\n> Previously, when promisor_remote_move_to_tail() is called for a\n> promisor_remote which is currently the *only* element in promisors, a\n> cycle is created in the promisors linked list. This cycle leads to a\n> double free later on in promisor_remote_clear(): promisors is set to\n> promisors->next (a no-op, as promisors->next == promisors); the previous\n> value of promisors is free()'d; then the new value of promisors (which\n> is equal to the previous value of promisors) is also free()'d. This\n> double-free error was unrecoverable for the user without removing the\n> filter or re-cloning the repo and hoping to miss this edge case.\n\nNice clear description.\n\nIs the problem only when the remote is the only element of the list, or\nwould it also happen when it's at the end?\n\nI think the problematic lines are:\n\n  r->next = NULL;\n  *promisors_tail = r;\n\nIf our \"r\" is already the tail, then promisors_tail points to &r->next,\nand we create a cycle in the linked list.\n\nI think if you swap those two lines, the problem goes away. That said,\nit's subtle enough that I think covering the noop case at the start of\nthe function is much clearer.\n\nUsing that strategy, I think this:\n\n> +\tif (promisors == r && promisors->next == NULL)\n> +\t\treturn;\n\nshould probably just see if we're already at the end, which also covers\nthe single-element case. Like:\n\n  if (!r->next)\n\treturn; /* we're already at the end */\n\nor possibly:\n\n  if (promisors_tail == &r->next)\n\treturn; /* we're already at the end */\n\nI also can't help but think this would all be a lot simpler using the\nimplementation in list.h. Then we don't have to pass this weird\n\"previous\" pointer around (because it's a doubly-linked list). And\nfunctions like this one could go away in favor of list_move(). But\nthat's obviously a bigger change.\n\n> This change showed up for us in a user bugreport; I'm actually fairly\n> unfamiliar with the codebase here but given the drastic nature of the\n> failure, I wanted to get a fix up quickly. I'm still working on how to\n> reproduce this exact case in the test suite (and actually would\n> appreciate any pointers). Specifically, it looks like we only really\n> break if we have a single promisor_remote in the linked list, call\n> move_to_tail() on it at least once, and then call clear() on it without\n> adding another promisor_remote first.\n\nThe only call is in promisor_remote_init(), where we try to move\nwhatever promisor we get from looking up repository_format_partial_clone.\nThat name in turn comes from the extensions.partialclone config.\n\nThe init function also creates elements in the list from any remotes\nmarked with remote.XYZ.promisor in the config.\n\nSo this is enough to create the cycle in the linked list:\n\n  git init\n  git remote add foo /wherever\n  git config remote.foo.promisor true\n  git config extensions.partialclone foo\n  git fetch foo\n\nbut it doesn't trigger the double-free because we don't ever free\nanything. We only call the clear() function during reinit(). And that\nonly happens in partial_clone_register(). So if we take the commands\nabove, and then try to actually do a partial clone with it (causing it\nto re-register), I get (building with ASan, but vanilla glibc seems to\ndetect the double-free too):\n\n  $ git fetch --filter=blob:none foo\n  ==30356==ERROR: AddressSanitizer: heap-use-after-free on address 0x603000001f90 at pc 0x559adb9a0919 bp 0x7ffeb7a52b30 sp 0x7ffeb7a52b28\n  READ of size 8 at 0x603000001f90 thread T0\n    #0 0x559adb9a0918 in promisor_remote_clear /home/peff/compile/git/promisor-remote.c:173\n    #1 0x559adb9a0918 in promisor_remote_reinit /home/peff/compile/git/promisor-remote.c:183\n    #2 0x559adb5856c0 in fetch_one_setup_partial builtin/fetch.c:1570\n    #3 0x559adb5856c0 in cmd_fetch builtin/fetch.c:1736\n    [...etc...]\n\nAnother way to manifest the error:\n\n  git remote add bar /does-not-matter\n  git fetch --filter=blob:none bar\n\nNow we'll try to register \"bar\". But since it's _not_ already in the\nlinked list, our search will go on forever, since the list loops back on\nitself.\n\n-Peff\n"},{"id":"383007","messageId":"20190926175308.GA223193@google.com","threadId":"51921","inReplyTo":"20190926075535.GC20653@sigill.intra.peff.net","subject":"Re: [PATCH] promisor-remote: skip move_to_tail when n=1","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-09-26T17:53:08Z","receivedAt":"2019-09-26T17:53:16Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Thu, Sep 26, 2019 at 03:55:35AM -0400, Jeff King wrote:\n> On Wed, Sep 25, 2019 at 02:37:18PM -0700, Emily Shaffer wrote:\n> \n> > Previously, when promisor_remote_move_to_tail() is called for a\n> > promisor_remote which is currently the *only* element in promisors, a\n> > cycle is created in the promisors linked list. This cycle leads to a\n> > double free later on in promisor_remote_clear(): promisors is set to\n> > promisors->next (a no-op, as promisors->next == promisors); the previous\n> > value of promisors is free()'d; then the new value of promisors (which\n> > is equal to the previous value of promisors) is also free()'d. This\n> > double-free error was unrecoverable for the user without removing the\n> > filter or re-cloning the repo and hoping to miss this edge case.\n> \n> Nice clear description.\n> \n> Is the problem only when the remote is the only element of the list, or\n> would it also happen when it's at the end?\n\nIt totally does. Nice catch, I wasn't seeing past my specific example.\n:)\n\n> \n> I think the problematic lines are:\n> \n>   r->next = NULL;\n>   *promisors_tail = r;\n> \n> If our \"r\" is already the tail, then promisors_tail points to &r->next,\n> and we create a cycle in the linked list.\n> \n> I think if you swap those two lines, the problem goes away. That said,\n> it's subtle enough that I think covering the noop case at the start of\n> the function is much clearer.\n> \n> Using that strategy, I think this:\n> \n> > +\tif (promisors == r && promisors->next == NULL)\n> > +\t\treturn;\n> \n> should probably just see if we're already at the end, which also covers\n> the single-element case. Like:\n> \n>   if (!r->next)\n> \treturn; /* we're already at the end */\n\nHmm, I guess I wasn't familiar enough on the lifetime of a\npromisor_remote - I suppose I was expecting\npromisor_remote_move_to_tail() could be used for a first-time insert,\ntoo, although it looks like promisor_remote_new() actually does the\ninsert for us every time.\n\n> \n> or possibly:\n> \n>   if (promisors_tail == &r->next)\n> \treturn; /* we're already at the end */\n\nWith the above concern I initially feel a little more comfortable with\nthis, although now that I'm thinking through the case when 'r' isn't\nalready in the list, I think it would replace the entire list by taking\nthe 'else' branch, having a nulled r->next, and therefore replacing the\nhead pointer 'promisors' with itself.\n\nSo, considering that in the former approach incorrect usage (I calloc my\nown promisor_remote and call move_to_tail() on it) is a no-op, and in\nthe latter approach the same incorrect use case is disastrous (we leak\nany promisor_remotes already in the list), I'll stick with the former.\n\nThanks for the suggestions and for pointing out my edge case was wider\nthan I thought!\n\n> \n> I also can't help but think this would all be a lot simpler using the\n> implementation in list.h. Then we don't have to pass this weird\n> \"previous\" pointer around (because it's a doubly-linked list). And\n> functions like this one could go away in favor of list_move(). But\n> that's obviously a bigger change.\n\nAgreed. I joked to my team that this was the first time I've needed to\nunderstand linked list manipulation outside of an interview setting,\never ;)\n\n> \n> > This change showed up for us in a user bugreport; I'm actually fairly\n> > unfamiliar with the codebase here but given the drastic nature of the\n> > failure, I wanted to get a fix up quickly. I'm still working on how to\n> > reproduce this exact case in the test suite (and actually would\n> > appreciate any pointers). Specifically, it looks like we only really\n> > break if we have a single promisor_remote in the linked list, call\n> > move_to_tail() on it at least once, and then call clear() on it without\n> > adding another promisor_remote first.\n> \n> The only call is in promisor_remote_init(), where we try to move\n> whatever promisor we get from looking up repository_format_partial_clone.\n> That name in turn comes from the extensions.partialclone config.\n> \n> The init function also creates elements in the list from any remotes\n> marked with remote.XYZ.promisor in the config.\n> \n> So this is enough to create the cycle in the linked list:\n> \n>   git init\n>   git remote add foo /wherever\n>   git config remote.foo.promisor true\n>   git config extensions.partialclone foo\n>   git fetch foo\n> \n> but it doesn't trigger the double-free because we don't ever free\n> anything. We only call the clear() function during reinit(). And that\n> only happens in partial_clone_register(). So if we take the commands\n> above, and then try to actually do a partial clone with it (causing it\n> to re-register), I get (building with ASan, but vanilla glibc seems to\n> detect the double-free too):\n> \n>   $ git fetch --filter=blob:none foo\n>   ==30356==ERROR: AddressSanitizer: heap-use-after-free on address 0x603000001f90 at pc 0x559adb9a0919 bp 0x7ffeb7a52b30 sp 0x7ffeb7a52b28\n>   READ of size 8 at 0x603000001f90 thread T0\n>     #0 0x559adb9a0918 in promisor_remote_clear /home/peff/compile/git/promisor-remote.c:173\n>     #1 0x559adb9a0918 in promisor_remote_reinit /home/peff/compile/git/promisor-remote.c:183\n>     #2 0x559adb5856c0 in fetch_one_setup_partial builtin/fetch.c:1570\n>     #3 0x559adb5856c0 in cmd_fetch builtin/fetch.c:1736\n>     [...etc...]\n> \n> Another way to manifest the error:\n> \n>   git remote add bar /does-not-matter\n>   git fetch --filter=blob:none bar\n> \n> Now we'll try to register \"bar\". But since it's _not_ already in the\n> linked list, our search will go on forever, since the list loops back on\n> itself.\n\nI really appreciate your coming up with these cases. I'll test-ify them\nand send another patch, plus the modified fix as discussed as above.\n\n - Emily\n"},{"id":"383009","messageId":"20190926180604.GA5162@sigill.intra.peff.net","threadId":"51921","inReplyTo":"20190926175308.GA223193@google.com","subject":"Re: [PATCH] promisor-remote: skip move_to_tail when n=1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-26T18:06:04Z","receivedAt":"2019-09-26T18:06:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 26, 2019 at 10:53:08AM -0700, Emily Shaffer wrote:\n\n> > should probably just see if we're already at the end, which also covers\n> > the single-element case. Like:\n> > \n> >   if (!r->next)\n> > \treturn; /* we're already at the end */\n> \n> Hmm, I guess I wasn't familiar enough on the lifetime of a\n> promisor_remote - I suppose I was expecting\n> promisor_remote_move_to_tail() could be used for a first-time insert,\n> too, although it looks like promisor_remote_new() actually does the\n> insert for us every time.\n\nRight, having to call move_to_tail at all is itself a special case, for\nwhen partialclone points to a remote which doesn't have its\n\"remote.*.promisor\" config set. I haven't followed the details of this\npromisor stuff enough to know what it means, but presumably that's what\nthe non-multi-promisor case looks like.\n\n> > or possibly:\n> > \n> >   if (promisors_tail == &r->next)\n> > \treturn; /* we're already at the end */\n> \n> With the above concern I initially feel a little more comfortable with\n> this, although now that I'm thinking through the case when 'r' isn't\n> already in the list, I think it would replace the entire list by taking\n> the 'else' branch, having a nulled r->next, and therefore replacing the\n> head pointer 'promisors' with itself.\n\nYeah, I agree things get weird there.\n\n> > I also can't help but think this would all be a lot simpler using the\n> > implementation in list.h. Then we don't have to pass this weird\n> > \"previous\" pointer around (because it's a doubly-linked list). And\n> > functions like this one could go away in favor of list_move(). But\n> > that's obviously a bigger change.\n> \n> Agreed. I joked to my team that this was the first time I've needed to\n> understand linked list manipulation outside of an interview setting,\n> ever ;)\n\nI was curious how this would look, so I sketched it out. One of the\nannoying things about list.h is that there's a little extra boilerplate\nwhen iterating, since you have to cast back to the original type with\nlist_entry() (in the kernel they use typeof() to avoid this, but it's\nnot portable enough for us).\n\nThe result _is_ shorter by lines. I don't know if it's worth the churn.\n\n---\n promisor-remote.c | 75 ++++++++++++++++++++++++-------------------------------\n promisor-remote.h |  4 ++-\n 2 files changed, 35 insertions(+), 44 deletions(-)\n\ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex 9bc296cdde..a158ac44e0 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -50,8 +50,7 @@ static int fetch_objects(const char *remote_name,\n \treturn fetch_refs(remote_name, ref);\n }\n \n-static struct promisor_remote *promisors;\n-static struct promisor_remote **promisors_tail = &promisors;\n+static LIST_HEAD(promisors);\n \n static struct promisor_remote *promisor_remote_new(const char *remote_name)\n {\n@@ -64,40 +63,24 @@ static struct promisor_remote *promisor_remote_new(const char *remote_name)\n \t}\n \n \tFLEX_ALLOC_STR(r, name, remote_name);\n-\n-\t*promisors_tail = r;\n-\tpromisors_tail = &r->next;\n-\n+\tlist_add_tail(&r->list, &promisors);\n \treturn r;\n }\n \n-static struct promisor_remote *promisor_remote_lookup(const char *remote_name,\n-\t\t\t\t\t\t      struct promisor_remote **previous)\n+static struct promisor_remote *promisor_remote_lookup(const char *remote_name)\n {\n-\tstruct promisor_remote *r, *p;\n+\tstruct list_head *pos;\n \n-\tfor (p = NULL, r = promisors; r; p = r, r = r->next)\n-\t\tif (!strcmp(r->name, remote_name)) {\n-\t\t\tif (previous)\n-\t\t\t\t*previous = p;\n+\tlist_for_each(pos, &promisors) {\n+\t\tstruct promisor_remote *r =\n+\t\t\tlist_entry(pos, struct promisor_remote, list);\n+\t\tif (!strcmp(r->name, remote_name))\n \t\t\treturn r;\n-\t\t}\n+\t}\n \n \treturn NULL;\n }\n \n-static void promisor_remote_move_to_tail(struct promisor_remote *r,\n-\t\t\t\t\t struct promisor_remote *previous)\n-{\n-\tif (previous)\n-\t\tprevious->next = r->next;\n-\telse\n-\t\tpromisors = r->next ? r->next : r;\n-\tr->next = NULL;\n-\t*promisors_tail = r;\n-\tpromisors_tail = &r->next;\n-}\n-\n static int promisor_remote_config(const char *var, const char *value, void *data)\n {\n \tconst char *name;\n@@ -119,7 +102,7 @@ static int promisor_remote_config(const char *var, const char *value, void *data\n \n \t\tremote_name = xmemdupz(name, namelen);\n \n-\t\tif (!promisor_remote_lookup(remote_name, NULL))\n+\t\tif (!promisor_remote_lookup(remote_name))\n \t\t\tpromisor_remote_new(remote_name);\n \n \t\tfree(remote_name);\n@@ -129,7 +112,7 @@ static int promisor_remote_config(const char *var, const char *value, void *data\n \t\tstruct promisor_remote *r;\n \t\tchar *remote_name = xmemdupz(name, namelen);\n \n-\t\tr = promisor_remote_lookup(remote_name, NULL);\n+\t\tr = promisor_remote_lookup(remote_name);\n \t\tif (!r)\n \t\t\tr = promisor_remote_new(remote_name);\n \n@@ -155,26 +138,27 @@ static void promisor_remote_init(void)\n \tgit_config(promisor_remote_config, NULL);\n \n \tif (repository_format_partial_clone) {\n-\t\tstruct promisor_remote *o, *previous;\n+\t\tstruct promisor_remote *o;\n \n-\t\to = promisor_remote_lookup(repository_format_partial_clone,\n-\t\t\t\t\t   &previous);\n-\t\tif (o)\n-\t\t\tpromisor_remote_move_to_tail(o, previous);\n-\t\telse\n+\t\to = promisor_remote_lookup(repository_format_partial_clone);\n+\t\tif (o) {\n+\t\t\tlist_del(&o->list);\n+\t\t\tlist_add_tail(&o->list, &promisors);\n+\t\t} else\n \t\t\tpromisor_remote_new(repository_format_partial_clone);\n \t}\n }\n \n static void promisor_remote_clear(void)\n {\n-\twhile (promisors) {\n-\t\tstruct promisor_remote *r = promisors;\n-\t\tpromisors = promisors->next;\n+\tstruct list_head *pos, *tmp;\n+\n+\tlist_for_each_safe(pos, tmp, &promisors) {\n+\t\tstruct promisor_remote *r =\n+\t\t\tlist_entry(pos, struct promisor_remote, list);\n+\t\tlist_del(pos);\n \t\tfree(r);\n \t}\n-\n-\tpromisors_tail = &promisors;\n }\n \n void promisor_remote_reinit(void)\n@@ -189,9 +173,11 @@ struct promisor_remote *promisor_remote_find(const char *remote_name)\n \tpromisor_remote_init();\n \n \tif (!remote_name)\n-\t\treturn promisors;\n+\t\treturn list_empty(&promisors) ?\n+\t\t       NULL :\n+\t\t       list_first_entry(&promisors, struct promisor_remote, list);\n \n-\treturn promisor_remote_lookup(remote_name, NULL);\n+\treturn promisor_remote_lookup(remote_name);\n }\n \n int has_promisor_remote(void)\n@@ -235,15 +221,18 @@ int promisor_remote_get_direct(struct repository *repo,\n \t\t\t       const struct object_id *oids,\n \t\t\t       int oid_nr)\n {\n-\tstruct promisor_remote *r;\n+\tstruct list_head *pos;\n \tstruct object_id *remaining_oids = (struct object_id *)oids;\n \tint remaining_nr = oid_nr;\n \tint to_free = 0;\n \tint res = -1;\n \n \tpromisor_remote_init();\n \n-\tfor (r = promisors; r; r = r->next) {\n+\tlist_for_each(pos, &promisors) {\n+\t\tstruct promisor_remote *r =\n+\t\t\tlist_entry(pos, struct promisor_remote, list);\n+\n \t\tif (fetch_objects(r->name, remaining_oids, remaining_nr) < 0) {\n \t\t\tif (remaining_nr == 1)\n \t\t\t\tcontinue;\ndiff --git a/promisor-remote.h b/promisor-remote.h\nindex 8200dfc940..e3ddc329ba 100644\n--- a/promisor-remote.h\n+++ b/promisor-remote.h\n@@ -1,6 +1,8 @@\n #ifndef PROMISOR_REMOTE_H\n #define PROMISOR_REMOTE_H\n \n+#include \"list.h\"\n+\n struct object_id;\n \n /*\n@@ -10,7 +12,7 @@ struct object_id;\n  * from extensions.partialclone or core.partialclonefilter.\n  */\n struct promisor_remote {\n-\tstruct promisor_remote *next;\n+\tstruct list_head list;\n \tconst char *partial_clone_filter;\n \tconst char name[FLEX_ARRAY];\n };\n"},{"id":"383036","messageId":"20190926213156.88185-1-emilyshaffer@google.com","threadId":"51921","inReplyTo":"20190925213718.231231-1-emilyshaffer@google.com","subject":"[PATCH v2] promisor-remote: skip move_to_tail when no-op","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-09-26T21:31:56Z","receivedAt":"2019-09-26T21:32:16Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"Previously, when promisor_remote_move_to_tail() is called for a\npromisor_remote which is currently the final element in promisors, a\ncycle is created in the promisors linked list. This cycle leads to a\ndouble free later on in promisor_remote_clear() when the final element\nof the promisors list is removed: promisors is set to promisors->next (a\nno-op, as promisors->next == promisors); the previous value of promisors\nis free()'d; then the new value of promisors (which is equal to the\nprevious value of promisors) is also free()'d. This double-free error\nwas unrecoverable for the user without removing the filter or re-cloning\nthe repo and hoping to miss this edge case.\n\nNow, when promisor_remote_move_to_tail() would be a no-op, just do a\nno-op. In cases of promisor_remote_move_to_tail() where r is not already\nat the tail of the list, it works as before.\n\nSigned-off-by: Emily Shaffer <emilyshaffer@google.com>\nHelped-by: Jeff King <peff@peff.net>\n---\nThanks Peff for pointing out this bug occurs any time the tail is being\nmoved to tail in the promisors list. Modified patch to check just for\n\"already at tail\" condition.\n\nAdded a test case based on Peff's reproduction steps (thanks!) and\nconfirmed it failed before the patch, exiting gracelessly with a\nSIGABRT, and now fails gracefully (and passes test_must_fail condition).\n\n - Emily\n\n\n promisor-remote.c        |  3 +++\n t/t0410-partial-clone.sh | 12 ++++++++++++\n 2 files changed, 15 insertions(+)\n\ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex 9bc296cdde..9bd5b79d59 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -89,6 +89,9 @@ static struct promisor_remote *promisor_remote_lookup(const char *remote_name,\n static void promisor_remote_move_to_tail(struct promisor_remote *r,\n \t\t\t\t\t struct promisor_remote *previous)\n {\n+\tif (r->next == NULL)\n+\t\treturn;\n+\n \tif (previous)\n \t\tprevious->next = r->next;\n \telse\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex d4b7e535ea..4db4d488c8 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -429,6 +429,18 @@ test_expect_success 'rev-list dies for missing objects on cmd line' '\n \tdone\n '\n \n+test_expect_success 'single promisor remote can be re-initialized gracefully' '\n+\t# ensure one promisor is in the promisors list\n+\trm -rf repo &&\n+\ttest_create_repo repo &&\n+\tgit -C repo remote add foo /wherever &&\n+\tgit -C repo config remote.foo.promisor true &&\n+\tgit -C repo config extensions.partialclone foo &&\n+\n+\t# reinitialize the promisors list; this must fail gracefully\n+\ttest_must_fail git -C repo fetch --filter=blob:none foo 2>fetch_err\n+'\n+\n test_expect_success 'gc repacks promisor objects separately from non-promisor objects' '\n \trm -rf repo &&\n \ttest_create_repo repo &&\n-- \n2.23.0.444.g18eeb5a265-goog\n\n"},{"id":"383046","messageId":"20190927003223.GA22473@sigill.intra.peff.net","threadId":"51921","inReplyTo":"20190926213156.88185-1-emilyshaffer@google.com","subject":"Re: [PATCH v2] promisor-remote: skip move_to_tail when no-op","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-27T00:32:24Z","receivedAt":"2019-09-27T00:32:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 26, 2019 at 02:31:56PM -0700, Emily Shaffer wrote:\n\n> ---\n> Thanks Peff for pointing out this bug occurs any time the tail is being\n> moved to tail in the promisors list. Modified patch to check just for\n> \"already at tail\" condition.\n> \n> Added a test case based on Peff's reproduction steps (thanks!) and\n> confirmed it failed before the patch, exiting gracelessly with a\n> SIGABRT, and now fails gracefully (and passes test_must_fail condition).\n\nThanks, this is looking good, though I have a few nits.\n\n> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>\n> Helped-by: Jeff King <peff@peff.net>\n\nThe trailers are supposed to be chronological. It probably doesn't\nmatter a lot here, but it's more important for something like a signoff\nchain. I suppose you could also argue that you signed off and then I\nhelped but I think you signed off again after. :)\n\n> diff --git a/promisor-remote.c b/promisor-remote.c\n> index 9bc296cdde..9bd5b79d59 100644\n> --- a/promisor-remote.c\n> +++ b/promisor-remote.c\n> @@ -89,6 +89,9 @@ static struct promisor_remote *promisor_remote_lookup(const char *remote_name,\n>  static void promisor_remote_move_to_tail(struct promisor_remote *r,\n>  \t\t\t\t\t struct promisor_remote *previous)\n>  {\n> +\tif (r->next == NULL)\n> +\t\treturn;\n> +\n>  \tif (previous)\n>  \t\tprevious->next = r->next;\n>  \telse\n\nYeah, I think of all the discussed options, this one is pretty easy to\nunderstand.\n\n> --- a/t/t0410-partial-clone.sh\n> +++ b/t/t0410-partial-clone.sh\n> @@ -429,6 +429,18 @@ test_expect_success 'rev-list dies for missing objects on cmd line' '\n>  \tdone\n>  '\n>  \n> +test_expect_success 'single promisor remote can be re-initialized gracefully' '\n> +\t# ensure one promisor is in the promisors list\n> +\trm -rf repo &&\n> +\ttest_create_repo repo &&\n> +\tgit -C repo remote add foo /wherever &&\n> +\tgit -C repo config remote.foo.promisor true &&\n> +\tgit -C repo config extensions.partialclone foo &&\n> +\n> +\t# reinitialize the promisors list; this must fail gracefully\n> +\ttest_must_fail git -C repo fetch --filter=blob:none foo 2>fetch_err\n> +'\n\nCould we make this a little more robust by using a real repo instead of\n\"/wherever\", and confirming that the command actually succeeds?\n\nAs a side note, if we're not going to check the content of fetch_err, I\nthink we're better off not redirecting it (it goes to /dev/null by\ndefault, or the user or log if running with \"-v\" or \"--verbose-log\").\n\n-Peff\n"},{"id":"383163","messageId":"20190930202818.2172-1-emilyshaffer@google.com","threadId":"51921","inReplyTo":"20190926213156.88185-1-emilyshaffer@google.com","subject":"[PATCH v3] promisor-remote: skip move_to_tail when no-op","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-09-30T20:28:18Z","receivedAt":"2019-09-30T20:37:10Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"Previously, when promisor_remote_move_to_tail() is called for a\npromisor_remote which is currently the final element in promisors, a\ncycle is created in the promisors linked list. This cycle leads to a\ndouble free later on in promisor_remote_clear() when the final element\nof the promisors list is removed: promisors is set to promisors->next (a\nno-op, as promisors->next == promisors); the previous value of promisors\nis free()'d; then the new value of promisors (which is equal to the\nprevious value of promisors) is also free()'d. This double-free error\nwas unrecoverable for the user without removing the filter or re-cloning\nthe repo and hoping to miss this edge case.\n\nNow, when promisor_remote_move_to_tail() would be a no-op, just do a\nno-op. In cases of promisor_remote_move_to_tail() where r is not already\nat the tail of the list, it works as before.\n\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Emily Shaffer <emilyshaffer@google.com>\n---\nFixed up some nits from Peff in v2. Thanks especially for the catch on\ncapturing the output of the fetch - I had been grepping it before I\nrealized that test_must_fail accounted for unexpected signal exits, and\nforgot to remove the redirect.\n\n - Emily\n\n promisor-remote.c        |  3 +++\n t/t0410-partial-clone.sh | 13 +++++++++++++\n 2 files changed, 16 insertions(+)\n\ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex 9bc296cdde..9bd5b79d59 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -89,6 +89,9 @@ static struct promisor_remote *promisor_remote_lookup(const char *remote_name,\n static void promisor_remote_move_to_tail(struct promisor_remote *r,\n \t\t\t\t\t struct promisor_remote *previous)\n {\n+\tif (r->next == NULL)\n+\t\treturn;\n+\n \tif (previous)\n \t\tprevious->next = r->next;\n \telse\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex d4b7e535ea..12274705c2 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -429,6 +429,19 @@ test_expect_success 'rev-list dies for missing objects on cmd line' '\n \tdone\n '\n \n+test_expect_success 'single promisor remote can be re-initialized gracefully' '\n+\t# ensure one promisor is in the promisors list\n+\trm -rf repo &&\n+\ttest_create_repo repo &&\n+\ttest_create_repo other &&\n+\tgit -C repo remote add foo \"file://$(pwd)/other\" &&\n+\tgit -C repo config remote.foo.promisor true &&\n+\tgit -C repo config extensions.partialclone foo &&\n+\n+\t# reinitialize the promisors list; this must fail gracefully\n+\tgit -C repo fetch --filter=blob:none foo\n+'\n+\n test_expect_success 'gc repacks promisor objects separately from non-promisor objects' '\n \trm -rf repo &&\n \ttest_create_repo repo &&\n-- \n2.23.0.444.g18eeb5a265-goog\n\n"},{"id":"383180","messageId":"20190930212758.GA18708@sigill.intra.peff.net","threadId":"51921","inReplyTo":"20190930202818.2172-1-emilyshaffer@google.com","subject":"Re: [PATCH v3] promisor-remote: skip move_to_tail when no-op","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-30T21:27:58Z","receivedAt":"2019-09-30T21:28:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 30, 2019 at 01:28:18PM -0700, Emily Shaffer wrote:\n\n> Previously, when promisor_remote_move_to_tail() is called for a\n> promisor_remote which is currently the final element in promisors, a\n> cycle is created in the promisors linked list. This cycle leads to a\n> double free later on in promisor_remote_clear() when the final element\n> of the promisors list is removed: promisors is set to promisors->next (a\n> no-op, as promisors->next == promisors); the previous value of promisors\n> is free()'d; then the new value of promisors (which is equal to the\n> previous value of promisors) is also free()'d. This double-free error\n> was unrecoverable for the user without removing the filter or re-cloning\n> the repo and hoping to miss this edge case.\n> \n> Now, when promisor_remote_move_to_tail() would be a no-op, just do a\n> no-op. In cases of promisor_remote_move_to_tail() where r is not already\n> at the tail of the list, it works as before.\n> \n> Helped-by: Jeff King <peff@peff.net>\n> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>\n> ---\n> Fixed up some nits from Peff in v2. Thanks especially for the catch on\n> capturing the output of the fetch - I had been grepping it before I\n> realized that test_must_fail accounted for unexpected signal exits, and\n> forgot to remove the redirect.\n\nThanks, this looks pretty good, except one little thing:\n\n> +test_expect_success 'single promisor remote can be re-initialized gracefully' '\n> +\t# ensure one promisor is in the promisors list\n> +\trm -rf repo &&\n> +\ttest_create_repo repo &&\n> +\ttest_create_repo other &&\n> +\tgit -C repo remote add foo \"file://$(pwd)/other\" &&\n> +\tgit -C repo config remote.foo.promisor true &&\n> +\tgit -C repo config extensions.partialclone foo &&\n> +\n> +\t# reinitialize the promisors list; this must fail gracefully\n> +\tgit -C repo fetch --filter=blob:none foo\n> +'\n\nWe expect this to succeed now, so \"this must fail gracefully\" no longer\napplies, right?\n\n-Peff\n"},{"id":"383183","messageId":"20190930220355.108394-1-emilyshaffer@google.com","threadId":"51921","inReplyTo":"20190930202818.2172-1-emilyshaffer@google.com","subject":"[PATCH v4] promisor-remote: skip move_to_tail when no-op","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-09-30T22:03:55Z","receivedAt":"2019-09-30T22:04:02Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"Previously, when promisor_remote_move_to_tail() is called for a\npromisor_remote which is currently the final element in promisors, a\ncycle is created in the promisors linked list. This cycle leads to a\ndouble free later on in promisor_remote_clear() when the final element\nof the promisors list is removed: promisors is set to promisors->next (a\nno-op, as promisors->next == promisors); the previous value of promisors\nis free()'d; then the new value of promisors (which is equal to the\nprevious value of promisors) is also free()'d. This double-free error\nwas unrecoverable for the user without removing the filter or re-cloning\nthe repo and hoping to miss this edge case.\n\nNow, when promisor_remote_move_to_tail() would be a no-op, just do a\nno-op. In cases of promisor_remote_move_to_tail() where r is not already\nat the tail of the list, it works as before.\n\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Emily Shaffer <emilyshaffer@google.com>\n---\nSince v3 missed a small comment change.\n promisor-remote.c        |  3 +++\n t/t0410-partial-clone.sh | 13 +++++++++++++\n 2 files changed, 16 insertions(+)\n\ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex 9bc296cdde..9bd5b79d59 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -89,6 +89,9 @@ static struct promisor_remote *promisor_remote_lookup(const char *remote_name,\n static void promisor_remote_move_to_tail(struct promisor_remote *r,\n \t\t\t\t\t struct promisor_remote *previous)\n {\n+\tif (r->next == NULL)\n+\t\treturn;\n+\n \tif (previous)\n \t\tprevious->next = r->next;\n \telse\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex d4b7e535ea..5c676c71a5 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -429,6 +429,19 @@ test_expect_success 'rev-list dies for missing objects on cmd line' '\n \tdone\n '\n \n+test_expect_success 'single promisor remote can be re-initialized gracefully' '\n+\t# ensure one promisor is in the promisors list\n+\trm -rf repo &&\n+\ttest_create_repo repo &&\n+\ttest_create_repo other &&\n+\tgit -C repo remote add foo \"file://$(pwd)/other\" &&\n+\tgit -C repo config remote.foo.promisor true &&\n+\tgit -C repo config extensions.partialclone foo &&\n+\n+\t# reinitialize the promisors list\n+\tgit -C repo fetch --filter=blob:none foo\n+'\n+\n test_expect_success 'gc repacks promisor objects separately from non-promisor objects' '\n \trm -rf repo &&\n \ttest_create_repo repo &&\n-- \n2.23.0.444.g18eeb5a265-goog\n\n"},{"id":"383190","messageId":"CAP8UFD3vdf-WHeoBaiJw5S_uUPegY_UENLKOKoMV6QVssv=QDQ@mail.gmail.com","threadId":"51921","inReplyTo":"20190930220355.108394-1-emilyshaffer@google.com","subject":"Re: [PATCH v4] promisor-remote: skip move_to_tail when no-op","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-10-01T05:12:23Z","receivedAt":"2019-10-01T05:12:37Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, Oct 1, 2019 at 12:03 AM Emily Shaffer <emilyshaffer@google.com> wrote:\n>\n> Previously, when promisor_remote_move_to_tail() is called for a\n> promisor_remote which is currently the final element in promisors, a\n> cycle is created in the promisors linked list. This cycle leads to a\n> double free later on in promisor_remote_clear() when the final element\n> of the promisors list is removed: promisors is set to promisors->next (a\n> no-op, as promisors->next == promisors); the previous value of promisors\n> is free()'d; then the new value of promisors (which is equal to the\n> previous value of promisors) is also free()'d. This double-free error\n> was unrecoverable for the user without removing the filter or re-cloning\n> the repo and hoping to miss this edge case.\n>\n> Now, when promisor_remote_move_to_tail() would be a no-op, just do a\n> no-op. In cases of promisor_remote_move_to_tail() where r is not already\n> at the tail of the list, it works as before.\n\nYeah, thank you Emily and Peff for finding and fixing this! The fix\nand the test look good to me.\n"}]}