Re: [PATCH 3/8] daemon: recognize hidden request arguments

[Brandon Williams <bmwill@google.com>]

On 09/13, Stefan Beller wrote:
> On Wed, Sep 13, 2017 at 2:54 PM, Brandon Williams <bmwill@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 <bmwill@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