On Tue,  3 Oct 2017 13:15:01 -0700
Brandon Williams <bmw...@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]

> +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.

> +             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.

> +
> +     if (git_protocol.len > 0)
> +             argv_array_pushf(env, GIT_PROTOCOL_ENVIRONMENT "=%s",
> +                              git_protocol.buf);
> +     strbuf_release(&git_protocol);
>  }

Reply via email to