Re: [PATCH v2 4/9] daemon: recognize hidden request arguments

[Brandon Williams]

On 09/27, Brandon Williams wrote:
> On 09/27, Junio C Hamano wrote:
> > Brandon Williams  writes:
> >
> > > 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.
> >
> > It's a bit unclear if that commit _introduced_ a bug, or just
> > noticed an old bug and documented it in its log message.  How does
> > that commit impact the versons of Git that the updated code is
> > capable of interracting with?
>
> You're right, after reading it again it isn't clear.  I'll change this
> to indicate that the commit is a fix to a bug and that the fix doesn't
> allow us to place any additional args.

Ok how about this wording for the commit msg:

  daemon: recognize hidden request arguments

  A normal request to git-daemon is structured as "command
  path/to/repo\0host=..\0" and due to a bug introduced in
  49ba83fb6 (Add virtualization support to git-daemon, 2006-09-19)
  we aren't able to place any extra arguments (separated by NULs)
  besides the host otherwise the parsing of those arguments would
  enter an infinite loop.  This bug was fixed in 73bb33a94
  (daemon: Strictly parse the "extra arg" part of the command,
  2009-06-04) but a check was put in place to disallow extra
  arguments so that new clients wouldn't trigger this bug in older
  servers.

  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.

  By placing these extra arguments behind a second NUL byte we can
  skirt around both the infinite loop bug in 49ba83fb6 (Add
  virtualization support to git-daemon, 2006-09-19) as well as the
  explicit disallowing of extra arguments introduced in 73bb33a94
  (daemon: Strictly parse the "extra arg" part of the command,
  2009-06-04) because both of these versions of git-daemon check
  for a single NUL byte after the host argument before terminating
  the argument parsing.

This way I'm citing when the bug was actually introduced as well as
describing why the 'fix' didn't completely resolve the issue.  I also
explain a little bit about how this change should work even with very
old servers which still have the bug.  (I tried to get the introp test
to work on a version of git that old but am having some difficulty even
getting the old version to launch git-daemon without hanging for some
unknown reason)

--
Brandon Williams

Re: [PATCH v2 4/9] daemon: recognize hidden request arguments

[Brandon Williams]

On 09/27, Junio C Hamano wrote:
> Brandon Williams  writes:
> 
> > 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.
> 
> It's a bit unclear if that commit _introduced_ a bug, or just
> noticed an old bug and documented it in its log message.  How does
> that commit impact the versons of Git that the updated code is
> capable of interracting with?

You're right, after reading it again it isn't clear.  I'll change this
to indicate that the commit is a fix to a bug and that the fix doesn't
allow us to place any additional args.

> 
> > +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);
> > +
> > +   /* Look for additional arguments places after a second NUL byte */
> > +   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(_protocol, ':');
> > +   strbuf_addstr(_protocol, arg);
> > +   }
> > +   }
> > +
> > +   if (git_protocol.len > 0)
> > +   argv_array_pushf(env, GIT_PROTOCOL_ENVIRONMENT "=%s",
> > +git_protocol.buf);
> > +   strbuf_release(_protocol);
> >  }

-- 
Brandon Williams

Re: [PATCH v2 4/9] daemon: recognize hidden request arguments

[Junio C Hamano]

Brandon Williams  writes:

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

It's a bit unclear if that commit _introduced_ a bug, or just
noticed an old bug and documented it in its log message.  How does
that commit impact the versons of Git that the updated code is
capable of interracting with?

> +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);
> +
> + /* Look for additional arguments places after a second NUL byte */
> + 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(_protocol, ':');
> + strbuf_addstr(_protocol, arg);
> + }
> + }
> +
> + if (git_protocol.len > 0)
> + argv_array_pushf(env, GIT_PROTOCOL_ENVIRONMENT "=%s",
> +  git_protocol.buf);
> + strbuf_release(_protocol);
>  }

[PATCH v2 4/9] daemon: recognize hidden request arguments

[Brandon Williams]

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 
---
 daemon.c | 68 +++-
 1 file changed, 59 insertions(+), 9 deletions(-)

diff --git a/daemon.c b/daemon.c
index 30747075f..36cc794c9 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(, "upload-pack", "--strict", NULL);
argv_array_pushf(, "--timeout=%u", timeout);
+
+   argv_array_pushv(_array, env->argv);
+
return run_service_command();
 }
 
-static int upload_archive(void)
+static int upload_archive(const struct argv_array *env)
 {
struct child_process cld = CHILD_PROCESS_INIT;
argv_array_push(, "upload-archive");
+
+   argv_array_pushv(_array, env->argv);
+
return run_service_command();
 }
 
-static int receive_pack(void)
+static int receive_pack(const struct argv_array *env)
 {
struct child_process cld = CHILD_PROCESS_INIT;
argv_array_push(, "receive-pack");
+
+   argv_array_pushv(_array, env->argv);
+
return run_service_command();
 }
 
@@ -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,43 @@ 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 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);
+
+   /* Look for additional arguments places after a second NUL byte */
+   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(_protocol, ':');
+   strbuf_addstr(_protocol, arg);
+   }
+   }
+
+   if (git_protocol.len > 0)
+   argv_array_pushf(env, GIT_PROTOCOL_ENVIRONMENT "=%s",
+git_protocol.buf);
+   strbuf_release(_protocol);
 }
 
 /*
@@ -695,6 +741,7 @@ static int execute(void)