{"thread":{"id":"59405","subject":"[PATCH] git_connect(): fix corner cases in downgrading v2 to v0","startedAt":"2023-03-17T19:08:58Z","lastAt":"2023-03-17T22:23:09Z","messageCount":3,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"473660","messageId":"ZBS6wwWXcEBwGBm/@coredump.intra.peff.net","threadId":"59405","inReplyTo":null,"subject":"[PATCH] git_connect(): fix corner cases in downgrading v2 to v0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-17T19:08:51Z","receivedAt":"2023-03-17T19:08:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There's code in git_connect() that checks whether we are doing a push\nwith protocol_v2, and if so, drops us to protocol_v0 (since we only know\nhow to do v0 for fetches). But it misses some corner cases:\n\n  1. it checks the \"prog\" variable, which is actually the path to\n     receive-pack on the remote side. By default this is just\n     \"git-receive-pack\", but it could be an arbitrary string (like\n     \"/path/to/git receive-pack\", etc). We'd accidentally stay in v2\n     mode in this case.\n\n  2. besides \"receive-pack\" and \"upload-pack\", there's one other value\n     we'd expect: \"upload-archive\" for handling \"git archive --remote\".\n     Like receive-pack, this doesn't understand v2, and should use the\n     v0 protocol.\n\nIn practice, neither of these causes bugs in the real world so far. We\ndo send a \"we understand v2\" probe to the server, but since no server\nimplements v2 for anything but upload-pack, it's simply ignored. But\nthis would eventually become a problem if we do implement v2 for those\nendpoints, as older clients would falsely claim to understand it,\nleading to a server response they can't parse.\n\nWe can fix (1) by passing in both the program path and the \"name\" of the\noperation. I treat the name as a string here, because that's the pattern\nset in transport_connect(), which is one of our callers (we were simply\nthrowing away the \"name\" value there before).\n\nWe can fix (2) by allowing only known-v2 protocols (\"upload-pack\"),\nrather than blocking unknown ones (\"receive-pack\" and \"upload-archive\").\nThat will mean whoever eventually implements v2 push will have to adjust\nthis list, but that's reasonable. We'll do the safe, conservative thing\n(sticking to v0) by default, and anybody working on v2 will quickly\nrealize this spot needs to be updated.\n\nThe new tests cover the receive-pack and upload-archive cases above, and\nre-confirm that we allow v2 with an arbitrary \"--upload-pack\" path (that\nalready worked before this patch, of course, but it would be an easy\nthing to break if we flipped the allow/block logic without also handling\n\"name\" separately).\n\nHere are a few miscellaneous implementation notes, since I had to do a\nlittle head-scratching to understand who calls what:\n\n  - transport_connect() is called only for git-upload-archive. For\n    non-http git remotes, that resolves to the virtual connect_git()\n    function (which then calls git_connect(); confused yet?). So\n    plumbing through \"name\" in connect_git() covers that.\n\n  - for regular fetches and pushes, callers use higher-level functions\n    like transport_fetch_refs(). For non-http git remotes, that means\n    calling git_connect() under the hood via connect_setup(). And that\n    uses the \"for_push\" flag to decide which name to use.\n\n  - likewise, plumbing like fetch-pack and send-pack may call\n    git_connect() directly; they each know which name to use.\n\n  - for remote helpers (including http), we already have separate\n    parameters for \"name\" and \"exec\" (another name for \"prog\"). In\n    process_connect_service(), we feed the \"name\" to the helper via\n    \"connect\" or \"stateless-connect\" directives.\n\n    There's also a \"servpath\" option, which can be used to tell the\n    helper about the \"exec\" path. But no helpers we implement support\n    it! For http it would be useless anyway (no reasonable server\n    implementation will allow you to send a shell command to run the\n    server). In theory it would be useful for more obscure helpers like\n    remote-ext, but even there it is not implemented.\n\n    It's tempting to get rid of it simply to reduce confusion, but we\n    have publicly documented it since it was added in fa8c097cc9\n    (Support remote helpers implementing smart transports, 2009-12-09),\n    so it's possible some helper in the wild is using it.\n\n  - So for v2, helpers (again, including http) are mainly used via\n    stateless-connect, driven by the main program. But they do still\n    need to decide whether to do a v2 probe. And so there's similar\n    logic in remote-curl.c's discover_refs() that looks for\n    \"git-receive-pack\". But it's not buggy in the same way. Since it\n    doesn't support servpath, it is always dealing with a \"service\"\n    string like \"git-receive-pack\". And since it doesn't support\n    straight \"connect\", it can't be used for \"upload-archive\".\n\n    So we could leave that spot alone. But I've updated it here to match\n    the logic we're changing in connect_git(). That seems like the least\n    confusing thing for somebody who has to touch both of these spots\n    later (say, to add v2 push support). I didn't add a new test to make\n    sure this doesn't break anything; we already have several tests (in\n    t5551 and elsewhere) that make sure we are using v2 over http.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nTo some degree, this is closing the barn door after the horse is out. If\nwe were to add v2 push support in the next year or so, I'd probably\nsuggest using \"v3\" as the probe to differentiate requests from older,\nbuggy clients. But IMHO this is still worth fixing, if only because from\na cleanliness perspective. And if we wait long enough to do v2 push,\nthen maybe it would be safe to discount older, buggy clients. ;)\n\nAlso, just a side note: I found this bug because of the unused \"name\"\nparameter in connect_git(). I was tempted to mark it UNUSED and move on,\nbut digging revealed this rather curious situation.\n\n builtin/fetch-pack.c   |  4 ++--\n builtin/send-pack.c    |  2 +-\n connect.c              |  8 +++++---\n connect.h              |  2 +-\n remote-curl.c          |  7 ++++---\n t/t5702-protocol-v2.sh | 27 +++++++++++++++++++++++++++\n transport.c            | 10 +++++++---\n 7 files changed, 47 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex afe679368de..a71dbc9988d 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -211,8 +211,8 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \t\tint flags = args.verbose ? CONNECT_VERBOSE : 0;\n \t\tif (args.diag_url)\n \t\t\tflags |= CONNECT_DIAG_URL;\n-\t\tconn = git_connect(fd, dest, args.uploadpack,\n-\t\t\t\t   flags);\n+\t\tconn = git_connect(fd, dest, \"git-upload-pack\",\n+\t\t\t\t   args.uploadpack, flags);\n \t\tif (!conn)\n \t\t\treturn args.diag_url ? 0 : 1;\n \t}\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex 4c5d125fa0a..2452e3cceb6 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -275,7 +275,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n \t\tfd[0] = 0;\n \t\tfd[1] = 1;\n \t} else {\n-\t\tconn = git_connect(fd, dest, receivepack,\n+\t\tconn = git_connect(fd, dest, \"git-receive-pack\", receivepack,\n \t\t\targs.verbose ? CONNECT_VERBOSE : 0);\n \t}\n \ndiff --git a/connect.c b/connect.c\nindex 63e59641c0d..fc977256572 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -1406,6 +1406,7 @@ static void fill_ssh_args(struct child_process *conn, const char *ssh_host,\n  * the connection failed).\n  */\n struct child_process *git_connect(int fd[2], const char *url,\n+\t\t\t\t  const char *name,\n \t\t\t\t  const char *prog, int flags)\n {\n \tchar *hostandport, *path;\n@@ -1415,10 +1416,11 @@ struct child_process *git_connect(int fd[2], const char *url,\n \n \t/*\n \t * NEEDSWORK: If we are trying to use protocol v2 and we are planning\n-\t * to perform a push, then fallback to v0 since the client doesn't know\n-\t * how to push yet using v2.\n+\t * to perform any operation that doesn't involve upload-pack (i.e., a\n+\t * fetch, ls-remote, etc), then fallback to v0 since we don't know how\n+\t * to do anything else (like push or remote archive) via v2.\n \t */\n-\tif (version == protocol_v2 && !strcmp(\"git-receive-pack\", prog))\n+\tif (version == protocol_v2 && strcmp(\"git-upload-pack\", name))\n \t\tversion = protocol_v0;\n \n \t/* Without this we cannot rely on waitpid() to tell\ndiff --git a/connect.h b/connect.h\nindex b26f7de7841..f41a0b4c1fb 100644\n--- a/connect.h\n+++ b/connect.h\n@@ -7,7 +7,7 @@\n #define CONNECT_DIAG_URL      (1u << 1)\n #define CONNECT_IPV4          (1u << 2)\n #define CONNECT_IPV6          (1u << 3)\n-struct child_process *git_connect(int fd[2], const char *url, const char *prog, int flags);\n+struct child_process *git_connect(int fd[2], const char *url, const char *name, const char *prog, int flags);\n int finish_connect(struct child_process *conn);\n int git_connection_is_socket(struct child_process *conn);\n int server_supports(const char *feature);\ndiff --git a/remote-curl.c b/remote-curl.c\nindex a76b6405eb2..812e940f10a 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -472,10 +472,11 @@ static struct discovery *discover_refs(const char *service, int for_push)\n \n \t/*\n \t * NEEDSWORK: If we are trying to use protocol v2 and we are planning\n-\t * to perform a push, then fallback to v0 since the client doesn't know\n-\t * how to push yet using v2.\n+\t * to perform any operation that doesn't involve upload-pack (i.e., a\n+\t * fetch, ls-remote, etc), then fallback to v0 since we don't know how\n+\t * to do anything else (like push or remote archive) via v2.\n \t */\n-\tif (version == protocol_v2 && !strcmp(\"git-receive-pack\", service))\n+\tif (version == protocol_v2 && strcmp(\"git-upload-pack\", service))\n \t\tversion = protocol_v0;\n \n \t/* Add the extra Git-Protocol header */\ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex e4db7513f42..71aabe30b7d 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -728,6 +728,33 @@ test_expect_success 'file:// --negotiate-only with protocol v0' '\n \ttest_i18ngrep \"negotiate-only requires protocol v2\" err\n '\n \n+test_expect_success 'push with custom path does not request v2' '\n+\trm -f env.trace &&\n+\tgit -C client push \\\n+\t\t--receive-pack=\"env >../env.trace; git-receive-pack\" \\\n+\t\torigin HEAD:refs/heads/custom-push-test &&\n+\ttest_path_is_file env.trace &&\n+\t! grep ^GIT_PROTOCOL env.trace\n+'\n+\n+test_expect_success 'fetch with custom path does request v2' '\n+\trm -f env.trace &&\n+\tgit -C client fetch \\\n+\t\t--upload-pack=\"env >../env.trace; git-upload-pack\" \\\n+\t\torigin HEAD &&\n+\tgrep ^GIT_PROTOCOL=version=2 env.trace\n+'\n+\n+test_expect_success 'archive with custom path does not request v2' '\n+\trm -f env.trace &&\n+\tgit -C client archive \\\n+\t\t--exec=\"env >../env.trace; git-upload-archive\" \\\n+\t\t--remote=origin \\\n+\t\tHEAD >/dev/null &&\n+\ttest_path_is_file env.trace &&\n+\t! grep ^GIT_PROTOCOL env.trace\n+'\n+\n # Test protocol v2 with 'http://' transport\n #\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\ndiff --git a/transport.c b/transport.c\nindex 77a61a9d7bb..81950bf755f 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -276,8 +276,12 @@ static int connect_setup(struct transport *transport, int for_push)\n \t}\n \n \tdata->conn = git_connect(data->fd, transport->url,\n-\t\t\t\t for_push ? data->options.receivepack :\n-\t\t\t\t data->options.uploadpack,\n+\t\t\t\t for_push ?\n+\t\t\t\t\t\"git-receive-pack\" :\n+\t\t\t\t\t\"git-upload-pack\",\n+\t\t\t\t for_push ?\n+\t\t\t\t\tdata->options.receivepack :\n+\t\t\t\t\tdata->options.uploadpack,\n \t\t\t\t flags);\n \n \treturn 0;\n@@ -911,7 +915,7 @@ static int connect_git(struct transport *transport, const char *name,\n {\n \tstruct git_transport_data *data = transport->data;\n \tdata->conn = git_connect(data->fd, transport->url,\n-\t\t\t\t executable, 0);\n+\t\t\t\t name, executable, 0);\n \tfd[0] = data->fd[0];\n \tfd[1] = data->fd[1];\n \treturn 0;\n-- \n2.40.0.581.g8d688c70eca\n"},{"id":"473674","messageId":"xmqqr0tnf2i2.fsf@gitster.g","threadId":"59405","inReplyTo":"ZBS6wwWXcEBwGBm/@coredump.intra.peff.net","subject":"Re: [PATCH] git_connect(): fix corner cases in downgrading v2 to v0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-17T22:15:49Z","receivedAt":"2023-03-17T22:15:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> There's code in git_connect() that checks whether we are doing a push\n> with protocol_v2, and if so, drops us to protocol_v0 (since we only know\n> how to do v0 for fetches).\n\nI had to read this three times to make sense out of it, and then\nafter reading the NEEDSWORK comment at the beginning of\ngit_connect(), started to suspect that there is a bit of typo there\nthat made it confusing to me.  Either \"(since we only know how to do\nv2 for fetches)\", or \"(since we only know how to push via v0)\",\nperhaps.\n\nOther than that, superb reasoning and a very clear solution.  Will\napply.  Thanks.\n"},{"id":"473675","messageId":"ZBToEJVQ6W6vTxK5@coredump.intra.peff.net","threadId":"59405","inReplyTo":"xmqqr0tnf2i2.fsf@gitster.g","subject":"Re: [PATCH] git_connect(): fix corner cases in downgrading v2 to v0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-17T22:22:08Z","receivedAt":"2023-03-17T22:23:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 17, 2023 at 03:15:49PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > There's code in git_connect() that checks whether we are doing a push\n> > with protocol_v2, and if so, drops us to protocol_v0 (since we only know\n> > how to do v0 for fetches).\n> \n> I had to read this three times to make sense out of it, and then\n> after reading the NEEDSWORK comment at the beginning of\n> git_connect(), started to suspect that there is a bit of typo there\n> that made it confusing to me.  Either \"(since we only know how to do\n> v2 for fetches)\", or \"(since we only know how to push via v0)\",\n> perhaps.\n\nNo matter how many times I proofread... :)\n\nYes, it should be \"we only know how to do v2 for fetches\".\n\n> Other than that, superb reasoning and a very clear solution.  Will\n> apply.  Thanks.\n\nThanks. Looks like you already fixed up s/v0/v2/ in what you queued.\n\n-Peff\n"}]}