On 09/13, Stefan Beller wrote:
> On Wed, Sep 13, 2017 at 2:54 PM, Brandon Williams <bmw...@google.com> 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.
> >
> > Signed-off-by: Brandon Williams <bmw...@google.com>
> > ---
> > daemon.c | 71
> > +++++++++++++++++++++++++++++++++++++++++++++++++++++++---------
> > 1 file changed, 61 insertions(+), 10 deletions(-)
> >
> > diff --git a/daemon.c b/daemon.c
> > index 30747075f..250dbf82c 100644
> > --- a/daemon.c
> > +++ b/daemon.c
> > @@ -282,7 +282,7 @@ static const char *path_ok(const char *directory,
> > struct hostinfo *hi)
> > return NULL; /* Fallthrough. Deny by default */
> > }
> >
> > -typedef int (*daemon_service_fn)(void);
> > +typedef int (*daemon_service_fn)(const struct argv_array *env);
> > struct daemon_service {
> > const char *name;
> > const char *config_name;
> > @@ -363,7 +363,7 @@ static int run_access_hook(struct daemon_service
> > *service, const char *dir,
> > }
> >
> > static int run_service(const char *dir, struct daemon_service *service,
> > - struct hostinfo *hi)
> > + struct hostinfo *hi, const struct argv_array *env)
> > {
> > const char *path;
> > int enabled = service->enabled;
> > @@ -422,7 +422,7 @@ static int run_service(const char *dir, struct
> > daemon_service *service,
> > */
> > signal(SIGTERM, SIG_IGN);
> >
> > - return service->fn();
> > + return service->fn(env);
> > }
> >
> > static void copy_to_log(int fd)
> > @@ -462,25 +462,34 @@ static int run_service_command(struct child_process
> > *cld)
> > return finish_command(cld);
> > }
> >
> > -static int upload_pack(void)
> > +static int upload_pack(const struct argv_array *env)
> > {
> > struct child_process cld = CHILD_PROCESS_INIT;
> > argv_array_pushl(&cld.args, "upload-pack", "--strict", NULL);
> > argv_array_pushf(&cld.args, "--timeout=%u", timeout);
> > +
> > + argv_array_pushv(&cld.env_array, env->argv);
> > +
> > return run_service_command(&cld);
> > }
> >
> > -static int upload_archive(void)
> > +static int upload_archive(const struct argv_array *env)
> > {
> > struct child_process cld = CHILD_PROCESS_INIT;
> > argv_array_push(&cld.args, "upload-archive");
> > +
> > + argv_array_pushv(&cld.env_array, env->argv);
> > +
> > return run_service_command(&cld);
> > }
> >
> > -static int receive_pack(void)
> > +static int receive_pack(const struct argv_array *env)
> > {
> > struct child_process cld = CHILD_PROCESS_INIT;
> > argv_array_push(&cld.args, "receive-pack");
> > +
> > + argv_array_pushv(&cld.env_array, env->argv);
> > +
> > return run_service_command(&cld);
> > }
> >
> > @@ -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 wonder if this could be written as
>
> begin = extra_args;
> p = extra_args;
> end = extra_args + buflen;
>
> while (p < end) {
> if (!*p)
> *p = ':';
> p++;
> }
> argv_array_pushf(env, GIT_PROTOCOL_ENVIRONMENT "=%s", begin);
>
> to ease the load on the server side, as then we do not
> have to copy the partial strings into strbufs and then
> count the length individually? (maybe performance is no big deal here?)
I'm sure something like that could work, and I don't know how
performance sensitive this bit is. That and depending on if we need the
unmodified string for anything at a later point maybe its best to not
modify it in place? I don't know :)
>
>
> >
> > /*
> > @@ -695,6 +737,7 @@ static int execute(void)
> > int pktlen, len, i;
> > char *addr = getenv("REMOTE_ADDR"), *port = getenv("REMOTE_PORT");
> > struct hostinfo hi;
> > + struct argv_array env = ARGV_ARRAY_INIT;
> >
> > hostinfo_init(&hi);
> >
> > @@ -716,8 +759,14 @@ static int execute(void)
> > pktlen--;
> > }
> >
> > - if (len != pktlen)
> > - parse_host_arg(&hi, line + len + 1, pktlen - len - 1);
> > + if (len != pktlen) {
> > + const char *extra_args;
> > + /* retrieve host */
> > + extra_args = parse_host_arg(&hi, line + len + 1, pktlen -
> > len - 1);
> > +
> > + /* parse additional args hidden behind a second NUL byte */
> > + parse_extra_args(&env, extra_args + 1, pktlen - (extra_args
> > - line) - 1);
> > + }
> >
> > for (i = 0; i < ARRAY_SIZE(daemon_service); i++) {
> > struct daemon_service *s = &(daemon_service[i]);
> > @@ -730,13 +779,15 @@ static int execute(void)
> > * Note: The directory here is probably context
> > sensitive,
> > * and might depend on the actual service being
> > performed.
> > */
> > - int rc = run_service(arg, s, &hi);
> > + int rc = run_service(arg, s, &hi, &env);
> > hostinfo_clear(&hi);
> > + argv_array_clear(&env);
> > return rc;
> > }
> > }
> >
> > hostinfo_clear(&hi);
> > + argv_array_clear(&env);
> > logerror("Protocol error: '%s'", line);
> > return -1;
> > }
> > --
> > 2.14.1.690.gbb1197296e-goog
> >
--
Brandon Williams
