{"thread":{"id":"56165","subject":"[PATCH] fetch-negotiator: call BUG() on API misuse, don't segfault","startedAt":"2021-07-27T00:02:33Z","lastAt":"2021-07-27T00:02:33Z","messageCount":1,"participants":["Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"431263","messageId":"patch-1.1-f1da49de63-20210727T000203Z-avarab@gmail.com","threadId":"56165","inReplyTo":null,"subject":"[PATCH] fetch-negotiator: call BUG() on API misuse, don't segfault","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-27T00:02:24Z","receivedAt":"2021-07-27T00:02:33Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"As noted in ec06283844a (fetch-pack: introduce negotiator API,\n2018-06-14) it's important that the fetch negotiator's callbacks be\ncalled in the documented order, and that some of them never be called\nagain after other \"later\" callbacks are called.\n\nBut let's assert that with a BUG(), instead of setting the relevant\ncallbacks to NULL. We'll now give a meaningful error on API misuse,\ninstead of segfaulting.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n fetch-negotiator.c    | 11 +++++++++++\n fetch-negotiator.h    | 10 ++++++++++\n negotiator/default.c  |  6 +++---\n negotiator/noop.c     |  3 +++\n negotiator/skipping.c |  6 +++---\n 5 files changed, 30 insertions(+), 6 deletions(-)\n\ndiff --git a/fetch-negotiator.c b/fetch-negotiator.c\nindex 57ed5784e1..b0888e2656 100644\n--- a/fetch-negotiator.c\n+++ b/fetch-negotiator.c\n@@ -24,3 +24,14 @@ void fetch_negotiator_init(struct repository *r,\n \t\treturn;\n \t}\n }\n+\n+void known_common_BUG(struct fetch_negotiator *negotiator,\n+\t\t      struct commit *commit)\n+{\n+\tBUG(\"known_common() called after add_tip() and/or next() was called\");\n+}\n+\n+void add_tip_BUG(struct fetch_negotiator *negotiator, struct commit *commit)\n+{\n+\tBUG(\"add_tip() called after next() called\");\n+}\ndiff --git a/fetch-negotiator.h b/fetch-negotiator.h\nindex ea78868504..b6461260f5 100644\n--- a/fetch-negotiator.h\n+++ b/fetch-negotiator.h\n@@ -28,6 +28,9 @@ struct fetch_negotiator {\n \t * Once this function is invoked, known_common() cannot be invoked any\n \t * more.\n \t *\n+\t * Set \"known_common\" to \"known_common_BUG\" in this callback\n+\t * to assert the invocation flow.\n+\t *\n \t * Indicate that this commit and all its ancestors are to be checked\n \t * for commonality with the server.\n \t */\n@@ -37,6 +40,10 @@ struct fetch_negotiator {\n \t * Once this function is invoked, known_common() and add_tip() cannot\n \t * be invoked any more.\n \t *\n+\t * Set \"add_tip\" to \"add_tip_BUG\" in this callback to assert\n+\t * the invocation flow, and \"known_common\" to\n+\t * \"known_common_BUG\" as noted for in add_tip() above.\n+\t *\n \t * Return the next commit that the client should send as a \"have\" line.\n \t */\n \tconst struct object_id *(*next)(struct fetch_negotiator *);\n@@ -56,4 +63,7 @@ struct fetch_negotiator {\n void fetch_negotiator_init(struct repository *r,\n \t\t\t   struct fetch_negotiator *negotiator);\n \n+void known_common_BUG(struct fetch_negotiator *, struct commit *);\n+void add_tip_BUG(struct fetch_negotiator *, struct commit *);\n+\n #endif\ndiff --git a/negotiator/default.c b/negotiator/default.c\nindex 434189ae5d..d6ad595ba4 100644\n--- a/negotiator/default.c\n+++ b/negotiator/default.c\n@@ -135,14 +135,14 @@ static void known_common(struct fetch_negotiator *n, struct commit *c)\n \n static void add_tip(struct fetch_negotiator *n, struct commit *c)\n {\n-\tn->known_common = NULL;\n+\tn->known_common = known_common_BUG;\n \trev_list_push(n->data, c, SEEN);\n }\n \n static const struct object_id *next(struct fetch_negotiator *n)\n {\n-\tn->known_common = NULL;\n-\tn->add_tip = NULL;\n+\tn->known_common = known_common_BUG;\n+\tn->add_tip = add_tip_BUG;\n \treturn get_rev(n->data);\n }\n \ndiff --git a/negotiator/noop.c b/negotiator/noop.c\nindex 60569b8350..3271048b27 100644\n--- a/negotiator/noop.c\n+++ b/negotiator/noop.c\n@@ -11,10 +11,13 @@ static void known_common(struct fetch_negotiator *n, struct commit *c)\n static void add_tip(struct fetch_negotiator *n, struct commit *c)\n {\n \t/* do nothing */\n+\tn->known_common = known_common_BUG;\n }\n \n static const struct object_id *next(struct fetch_negotiator *n)\n {\n+\tn->known_common = known_common_BUG;\n+\tn->add_tip = add_tip_BUG;\n \treturn NULL;\n }\n \ndiff --git a/negotiator/skipping.c b/negotiator/skipping.c\nindex 1236e79224..18448aeb80 100644\n--- a/negotiator/skipping.c\n+++ b/negotiator/skipping.c\n@@ -204,7 +204,7 @@ static void known_common(struct fetch_negotiator *n, struct commit *c)\n \n static void add_tip(struct fetch_negotiator *n, struct commit *c)\n {\n-\tn->known_common = NULL;\n+\tn->known_common = known_common_BUG;\n \tif (c->object.flags & SEEN)\n \t\treturn;\n \trev_list_push(n->data, c, 0);\n@@ -212,8 +212,8 @@ static void add_tip(struct fetch_negotiator *n, struct commit *c)\n \n static const struct object_id *next(struct fetch_negotiator *n)\n {\n-\tn->known_common = NULL;\n-\tn->add_tip = NULL;\n+\tn->known_common = known_common_BUG;\n+\tn->add_tip = add_tip_BUG;\n \treturn get_rev(n->data);\n }\n \n-- \n2.32.0.988.g1a6a4b2c5f\n\n"}]}