Re: [PATCH 3/8] daemon: recognize hidden request arguments
From: Jonathan Tan <hidden>
Date: 2017-09-21 00:24:55
On Wed, 13 Sep 2017 14:54:43 -0700 Brandon Williams [off-list ref] wrote:
A normal request to git-daemon is structured as "command path/to/repo\0host=..\0" and due to a bug in an old version of git-daemon 73bb33a94 (daemon: Strictly parse the "extra arg" part of the command, 2009-06-04) we aren't able to place any extra args (separated by NULs) besides the host. In order to get around this limitation teach git-daemon to recognize additional request arguments hidden behind a second NUL byte. Requests can then be structured like: "command path/to/repo\0host=..\0\0version=1\0key=value\0". git-daemon can then parse out the extra arguments and set 'GIT_PROTOCOL' accordingly.
A test in this patch (if possible) would be nice, but it is probably clearer to test this when one of the commands (e.g. upload-pack) is done. Could a test be added to the next patch to verify (using GIT_TRACE_PACKET, maybe) that the expected strings are sent? Then mention in this commit message that this will be tested in the next patch too.
quoted hunk ↗ jump to hunk
@@ -574,7 +583,7 @@ static void canonicalize_client(struct strbuf *out, const char *in) /* * Read the host as supplied by the client connection. */ -static void parse_host_arg(struct hostinfo *hi, char *extra_args, int buflen) +static char *parse_host_arg(struct hostinfo *hi, char *extra_args, int buflen) { char *val; int vallen;@@ -602,6 +611,39 @@ static void parse_host_arg(struct hostinfo *hi, char *extra_args, int buflen) if (extra_args < end && *extra_args) die("Invalid request"); } + + return extra_args; +} + +static void parse_extra_args(struct argv_array *env, const char *extra_args, + int buflen) +{ + const char *end = extra_args + buflen; + struct strbuf git_protocol = STRBUF_INIT; + + for (; extra_args < end; extra_args += strlen(extra_args) + 1) { + const char *arg = extra_args; + + /* + * Parse the extra arguments, adding most to 'git_protocol' + * which will be used to set the 'GIT_PROTOCOL' envvar in the + * service that will be run. + * + * If there ends up being a particular arg in the future that + * git-daemon needs to parse specificly (like the 'host' arg) + * then it can be parsed here and not added to 'git_protocol'. + */ + if (*arg) { + if (git_protocol.len > 0) + strbuf_addch(&git_protocol, ':'); + strbuf_addstr(&git_protocol, arg); + } + } + + if (git_protocol.len > 0) + argv_array_pushf(env, GIT_PROTOCOL_ENVIRONMENT "=%s", + git_protocol.buf); + strbuf_release(&git_protocol); }
I would rewrite this with parse_extra_args() calling parse_host_arg() instead (right now, you have 2 functions with 2 different meanings of "extra_args"). If you want to keep this arrangement, though, add a documentation comment about the meaning of the return value of parse_host_arg().