Re: [PATCH v3 04/10] daemon: recognize hidden request arguments

[Brandon Williams <bmwill@google.com>]

On 10/10, Jonathan Tan wrote:
> 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."

I can add that comment.

> 
> >   */
> > -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.

I disagree, you shouldn't be precluded from using protocol v2 if you
don't include a host argument.

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

Being lenient towards empty strings is fine, I don't see any reason why
we should disallow them.  Also, this code already
requires that there be the second NUL byte because if there isn't then
the code which parses the host argument would fail out.

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

Why not? I see no issue with allowing them.  In fact if we error out we
could be painting ourselves into a corner much like how we did with the
host parsing logic.

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

-- 
Brandon Williams