git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Shawn O. Pearce" <spearce@spearce.org>
Cc: git@vger.kernel.org
Subject: Re: [PATCH] daemon: Skip unknown "extra arg" information
Date: Thu, 04 Jun 2009 17:50:07 -0700	[thread overview]
Message-ID: <7vbpp3tsg0.fsf@alter.siamese.dyndns.org> (raw)
In-Reply-To: <20090604220824.GT3355@spearce.org> (Shawn O. Pearce's message of "Thu\, 4 Jun 2009 15\:08\:24 -0700")

"Shawn O. Pearce" <spearce@spearce.org> writes:

> If we don't recognize an extra arg supplied hidden behind the
> command, we should skip it and look at the next extra arg, in
> case we recognize the next one.
>
> For example, we currently don't recognize the "user=" extra arg,
> but we should still be able to start this connection anyway:

I do not necessarily agree 100% with that argument.

When there is a room for "protocol negotiation", it is clear that somebody
asking for what we said we do not support is bogus, and we should reject.

Unfortunately, there is no room in the protocol for git-daemon to announce
its capabilities, so the initial exchange can only be "unilaterally ask
and hope for the best" from the client's point of view.

However, in such a protocol exchange, there should be a way for a client
to say "if you do not understand this request, it is _NOT_ OK to continue,
pretending as if I never asked for it; instead please disconnect to avoid
doing any further harm".

We do something similar in the index extension.  In that area, obviously
there is no way for the writer who writes the index file to negotiate with
the reader who will later read from the index to know what the reader can
understand.  Extension sections are marked with section names, whose case
allows the reader to tell if it is Ok to simply ignore the section, or
understanding of the section contents is required for the proper operation
by the reader.  Perhaps we should be doing something similar.

>  perl -e '
>    $s="git-upload-pack /.git\0user=me\0host=localhost\0";
>    printf "%4.4x%s",4+length $s,$s
>  ' | ./git-daemon --inetd --base-path=`pwd` --export-all
>
> Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
> ---
>
>  This should go in maint.

In its current form, I do not think so.  Rejecting unknown is safer and
maint is about safety, not about feature.  Setting a precedent to accept
anything unknown will make it harder to later say "the server must
understand extension tokens that begin with uppercase".  It always is
easier to start stricter and then later loosen the rules in a controlled
way.

>  daemon.c |    9 ++++-----
>  1 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/daemon.c b/daemon.c
> index daa4c8e..a9a4f02 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -411,14 +411,13 @@ static char *xstrdup_tolower(const char *str)
>  static void parse_extra_args(char *extra_args, int buflen)
>  {
>  	char *val;
> -	int vallen;
>  	char *end = extra_args + buflen;
>  
>  	while (extra_args < end && *extra_args) {
> +		int arglen = strlen(extra_args);
>  		saw_extended_args = 1;
>  		if (strncasecmp("host=", extra_args, 5) == 0) {
>  			val = extra_args + 5;
> -			vallen = strlen(val) + 1;
>  			if (*val) {
>  				/* Split <host>:<port> at colon. */
>  				char *host = val;
> @@ -432,10 +431,10 @@ static void parse_extra_args(char *extra_args, int buflen)
>  				free(hostname);
>  				hostname = xstrdup_tolower(host);
>  			}
> -
> -			/* On to the next one */
> -			extra_args = val + vallen;
>  		}
> +
> +		/* On to the next one */
> +		extra_args += arglen + 1;
>  	}
>  
>  	/*
> -- 
> 1.6.3.1.333.g3ebba7

  reply	other threads:[~2009-06-05  0:50 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-06-04 22:08 Shawn O. Pearce
2009-06-05  0:50 ` Junio C Hamano [this message]
2009-06-05  1:33   ` Shawn O. Pearce
2009-06-05  8:41     ` Jakub Narebski
2009-06-05 13:16     ` Sergey Vlasov
2009-06-07 18:56     ` Johannes Sixt
2009-06-07 20:29       ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7vbpp3tsg0.fsf@alter.siamese.dyndns.org \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=spearce@spearce.org \
    --subject='Re: [PATCH] daemon: Skip unknown "extra arg" information' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).