{"thread":{"id":"49659","subject":"[PATCH] upload-pack: fix broken if/else chain in config callback","startedAt":"2018-10-24T07:27:56Z","lastAt":"2018-10-25T21:59:39Z","messageCount":3,"participants":["Jeff King","Johannes Schindelin","Josh Steadmon"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"361375","messageId":"20181024072752.GA29717@sigill.intra.peff.net","threadId":"49659","inReplyTo":null,"subject":"[PATCH] upload-pack: fix broken if/else chain in config callback","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-24T07:27:52Z","receivedAt":"2018-10-24T07:27:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The upload_pack_config() callback uses an if/else chain\nlike:\n\n if (!strcmp(var, \"a\"))\n ...\n else if (!strcmp(var, \"b\"))\n ...\n etc\n\nThis works as long as the conditions are mutually exclusive,\nbut one of them is not. 20b20a22f8 (upload-pack: provide a\nhook for running pack-objects, 2016-05-18) added:\n\n else if (current_config_scope() != CONFIG_SCOPE_REPO) {\n ... check some more options ...\n }\n\nThat was fine in that commit, because it came at the end of\nthe chain. But later, 10ac85c785 (upload-pack: add object\nfiltering for partial clone, 2017-12-08) did this:\n\n else if (current_config_scope() != CONFIG_SCOPE_REPO) {\n ... check some more options ...\n } else if (!strcmp(\"uploadpack.allowfilter\", var))\n ...\n\nWe'd always check the scope condition first, meaning we'd\n_only_ respect allowfilter when it's in the repo config. You\ncan see this with:\n\n git -c uploadpack.allowfilter=true upload-pack . | head -1\n\nwhich will not advertise the filter capability (but will\nafter this patch). We never noticed because:\n\n - our tests always set it in the repo config\n\n - in protocol v2, we use a different code path that\n actually calls repo_config_get_bool() separately, so\n that _does_ work. Real-world people experimenting with\n this may be using v2.\n\nThe more recent uploadpack.allowrefinwant option is in the\nsame boat.\n\nThere are a few possible fixes:\n\n 1. Bump the scope conditional back to the bottom of the\n chain. But that just means somebody else is likely to\n make the same mistake later.\n\n 2. Make the conditional more like the others. I.e.:\n\n else if (!current_config_scope() != CONFIG_SCOPE_REPO &&\n !strcmp(var, \"uploadpack.notallowedinrepo\"))\n\n This works, but the idea of the original structure was\n that we may grow multiple sensitive options like this.\n\n 3. Pull it out of the chain entirely. The chain mostly\n serves to avoid extra strcmp() calls after we've found\n a match. But it's not worth caring about those. In the\n worst case, when there isn't a match, we're already\n hitting every strcmp (and this happens regularly for\n stuff like \"core.bare\", etc).\n\nThis patch does (3).\n\nSigned-off-by: Jeff King \n---\nPhew. That was a lot of explanation for a small patch, but\nthis was sufficiently subtle I thought it worth it. And\nalso, I was really surprised the bug managed to exist for\nthis long without anybody noticing.\n\n upload-pack.c | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 540778d1dd..489c18e222 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -1028,14 +1028,17 @@ static int upload_pack_config(const char *var, const char *value, void *unused)\n \t\tkeepalive = git_config_int(var, value);\n \t\tif (!keepalive)\n \t\t\tkeepalive = -1;\n-\t} else if (current_config_scope() != CONFIG_SCOPE_REPO) {\n-\t\tif (!strcmp(\"uploadpack.packobjectshook\", var))\n-\t\t\treturn git_config_string(&pack_objects_hook, var, value);\n \t} else if (!strcmp(\"uploadpack.allowfilter\", var)) {\n \t\tallow_filter = git_config_bool(var, value);\n \t} else if (!strcmp(\"uploadpack.allowrefinwant\", var)) {\n \t\tallow_ref_in_want = git_config_bool(var, value);\n \t}\n+\n+\tif (current_config_scope() != CONFIG_SCOPE_REPO) {\n+\t\tif (!strcmp(\"uploadpack.packobjectshook\", var))\n+\t\t\treturn git_config_string(&pack_objects_hook, var, value);\n+\t}\n+\n \treturn parse_hide_refs_config(var, value, \"uploadpack\");\n }\n \n-- \n2.19.1.1094.gd480080bf6\n"},{"id":"361390","messageId":"nycvar.QRO.7.76.6.1810241049200.4546@tvgsbejvaqbjf.bet","threadId":"49659","inReplyTo":"20181024072752.GA29717@sigill.intra.peff.net","subject":"Re: [PATCH] upload-pack: fix broken if/else chain in config callback","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-10-24T08:50:08Z","receivedAt":"2018-10-24T08:50:22Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Wed, 24 Oct 2018, Jeff King wrote:\n\n> The upload_pack_config() callback uses an if/else chain\n> like:\n> \n> if (!strcmp(var, \"a\"))\n> ...\n> else if (!strcmp(var, \"b\"))\n> ...\n> etc\n> \n> This works as long as the conditions are mutually exclusive,\n> but one of them is not. 20b20a22f8 (upload-pack: provide a\n> hook for running pack-objects, 2016-05-18) added:\n> \n> else if (current_config_scope() != CONFIG_SCOPE_REPO) {\n> ... check some more options ...\n> }\n> \n> That was fine in that commit, because it came at the end of\n> the chain. But later, 10ac85c785 (upload-pack: add object\n> filtering for partial clone, 2017-12-08) did this:\n> \n> else if (current_config_scope() != CONFIG_SCOPE_REPO) {\n> ... check some more options ...\n> } else if (!strcmp(\"uploadpack.allowfilter\", var))\n> ...\n> \n> We'd always check the scope condition first, meaning we'd\n> _only_ respect allowfilter when it's in the repo config. You\n> can see this with:\n> \n> git -c uploadpack.allowfilter=true upload-pack . | head -1\n> \n> which will not advertise the filter capability (but will\n> after this patch). We never noticed because:\n> \n> - our tests always set it in the repo config\n> \n> - in protocol v2, we use a different code path that\n> actually calls repo_config_get_bool() separately, so\n> that _does_ work. Real-world people experimenting with\n> this may be using v2.\n> \n> The more recent uploadpack.allowrefinwant option is in the\n> same boat.\n> \n> There are a few possible fixes:\n> \n> 1. Bump the scope conditional back to the bottom of the\n> chain. But that just means somebody else is likely to\n> make the same mistake later.\n> \n> 2. Make the conditional more like the others. I.e.:\n> \n> else if (!current_config_scope() != CONFIG_SCOPE_REPO &&\n> !strcmp(var, \"uploadpack.notallowedinrepo\"))\n> \n> This works, but the idea of the original structure was\n> that we may grow multiple sensitive options like this.\n> \n> 3. Pull it out of the chain entirely. The chain mostly\n> serves to avoid extra strcmp() calls after we've found\n> a match. But it's not worth caring about those. In the\n> worst case, when there isn't a match, we're already\n> hitting every strcmp (and this happens regularly for\n> stuff like \"core.bare\", etc).\n> \n> This patch does (3).\n> \n> Signed-off-by: Jeff King \n> ---\n> Phew. That was a lot of explanation for a small patch, but\n> this was sufficiently subtle I thought it worth it. And\n> also, I was really surprised the bug managed to exist for\n> this long without anybody noticing.\n\nMaybe a lot of explanation, but definitely a good one. The explanation and\nthe patch look good to me.\n\nThanks,\nDscho\n\n> \n> upload-pack.c | 9 ++++++---\n> 1 file changed, 6 insertions(+), 3 deletions(-)\n> \n> diff --git a/upload-pack.c b/upload-pack.c\n> index 540778d1dd..489c18e222 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -1028,14 +1028,17 @@ static int upload_pack_config(const char *var, const char *value, void *unused)\n> \t\tkeepalive = git_config_int(var, value);\n> \t\tif (!keepalive)\n> \t\t\tkeepalive = -1;\n> -\t} else if (current_config_scope() != CONFIG_SCOPE_REPO) {\n> -\t\tif (!strcmp(\"uploadpack.packobjectshook\", var))\n> -\t\t\treturn git_config_string(&pack_objects_hook, var, value);\n> \t} else if (!strcmp(\"uploadpack.allowfilter\", var)) {\n> \t\tallow_filter = git_config_bool(var, value);\n> \t} else if (!strcmp(\"uploadpack.allowrefinwant\", var)) {\n> \t\tallow_ref_in_want = git_config_bool(var, value);\n> \t}\n> +\n> +\tif (current_config_scope() != CONFIG_SCOPE_REPO) {\n> +\t\tif (!strcmp(\"uploadpack.packobjectshook\", var))\n> +\t\t\treturn git_config_string(&pack_objects_hook, var, value);\n> +\t}\n> +\n> \treturn parse_hide_refs_config(var, value, \"uploadpack\");\n> }\n> \n> -- \n> 2.19.1.1094.gd480080bf6\n> \n"},{"id":"361566","messageId":"20181025215930.GJ233961@google.com","threadId":"49659","inReplyTo":"nycvar.QRO.7.76.6.1810241049200.4546@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] upload-pack: fix broken if/else chain in config callback","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-10-25T21:59:30Z","receivedAt":"2018-10-25T21:59:39Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2018.10.24 10:50, Johannes Schindelin wrote:\n> Maybe a lot of explanation, but definitely a good one. The explanation and\n> the patch look good to me.\n> \n> Thanks,\n> Dscho\n\nAgreed, as a newbie I definitely appreciate detailed explanations. Looks\ngood to me as well.\n\nReviewed-by: Josh Steadmon \n"}]}