Re: [PATCH v3 04/10] daemon: recognize hidden request arguments
- From
- Jonathan Tan <jonathantanmy@google.com>
- Date
- Oct 10, 2017, 18:24 UTC
- Message-ID
- <20171010112416.107a7325cef61cd2425d6204@google.com>
- In-Reply-To
- <20171003201507.3589-5-bmwill@google.com>
On Tue, 3 Oct 2017 13:15:01 -0700 Brandon Williams <bmwill@google.com> wrote:
> /* > * Read the host as supplied by the client connection.
The return value is probably worth documenting. Something like "Returns a pointer to the character *after* the NUL byte terminating the host argument, or extra_args if there is no host argument."
> */ > -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)
[snip]
Show 8 quoted lines
> +static void parse_extra_args(struct hostinfo *hi, struct argv_array *env,
> + char *extra_args, int buflen)
> +{
> + const char *end = extra_args + buflen;
> + struct strbuf git_protocol = STRBUF_INIT;
> +
> + /* First look for the host argument */
> + extra_args = parse_host_arg(hi, extra_args, buflen);This works, but is a bit loose in what it accepts. I think it's better to be tighter - in particular, if there is no host argument, we shouldn't be looking for extra args.
> +
> + /* Look for additional arguments places after a second NUL byte */
> + for (; extra_args < end; extra_args += strlen(extra_args) + 1) {Assuming that the host argument exists, this for loop should start at extra_args + 1 to skip the second NUL byte. This currently works because this code is lenient towards empty strings.
Show 17 quoted lines
> + 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);
> + }
> + }But I think we shouldn't be lenient towards empty strings.
Show 6 quoted lines
> + > + if (git_protocol.len > 0) > + argv_array_pushf(env, GIT_PROTOCOL_ENVIRONMENT "=%s", > + git_protocol.buf); > + strbuf_release(&git_protocol); > }