git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 0/3] --end-of-options marker
Date: Tue, 6 Aug 2019 13:33:50 -0400
Message-ID: <20190806173349.GA4961@sigill.intra.peff.net> (raw)
In-Reply-To: <xmqqa7cml0s9.fsf@gitster-ct.c.googlers.com>

On Tue, Aug 06, 2019 at 09:24:38AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > It's hard for scripted uses of rev-list, etc, to avoid option injection
> > from untrusted arguments, because revision arguments must come before
> > any "--" separator. I.e.:
> >
> >   git rev-list "$revision" -- "$path"
> >
> > might mistake "$revision" for an option (with rev-list, that would make
> > it an error, but something like git-log would default to HEAD).
> 
> Just to make sure I understand what I just read, let me paraphrase.
> We would want to accept
> 
> 	git rev-list --max-parents=4 \
> 		--end-of-options \
> 		--count -- docs/
> 
> so that '--count' would go thru the usual "as we have -- later, it
> must be a rev and we do not even disambiguate.  What does get_sha1()
> say it is?" and "docs/" would be taken as a pathspec.

Yes, that's how I'd expect that to be parsed.

> "git rev-list --max-parents=4 --count -- docs/" would have treated
> "--count" as an option and would error out due to lack of any
> starting revision.

Right. Though some options may have an impact even before rev-list would
complain. For instance --output=foo (which is actually a diff option,
but revision.c handles both) opens and truncates "foo" before rev-list
realizes it doesn't have enough options.

> On the other hand, "git log --count -- docs/" would take "--count"
> as an option, but does not complain about lack of any revs.  It just
> starts digging from HEAD and ends up ignoring the "--count" branch

Correct.

> (or is this feature meant to support tags?  As far as I recall, we
> do not allow branch names that begin with a dash).

We do forbid them via "git branch", but they are not forbidden by
check-ref-format. So:

  git update-ref refs/heads/-foo $oid

works fine, and receive-pack is happy to accept it as a push.  I think
it might be reasonable to forbid that, but we'd have to accept potential
fallouts.

That said, for the purposes of option injection, it actually doesn't
matter whether the ref exists or not (or if it's even an allowed name).
The problem is that the caller is feeding what it _thinks_ will be
interpreted as a rev name, but it isn't. So yes, most of the time
treating "--count" as a rev is nonsense and will fail. But the important
thing is not so much that we do the right thing when you have a branch
(or tag) with a funny name, but that we _don't_ do something dangerous
or unexpected.

-Peff

  parent reply	other threads:[~2019-08-06 17:33 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-06 14:38 Jeff King
2019-08-06 14:39 ` [PATCH 1/3] revision: allow --end-of-options to end option parsing Jeff King
2019-08-06 14:40 ` [PATCH 2/3] parse-options: allow --end-of-options as a synonym for "--" Jeff King
2019-08-06 14:40 ` [PATCH 3/3] gitcli: document --end-of-options Jeff King
2019-08-06 16:24 ` [PATCH 0/3] --end-of-options marker Junio C Hamano
2019-08-06 16:36   ` Randall S. Becker
2019-08-06 17:38     ` Jeff King
2019-08-06 17:58       ` Randall S. Becker
2019-08-06 18:14       ` SZEDER Gábor
2019-08-08 10:03         ` Jeff King
2019-08-06 17:33   ` Jeff King [this message]
2019-08-06 22:58 ` brian m. carlson
2019-08-06 23:43   ` Jeff King
2019-08-07  4:17     ` brian m. carlson
2019-08-07 16:54       ` Taylor Blau
2019-08-08 10:28       ` Jeff King

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=20190806173349.GA4961@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    /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

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for the project(s) associated with this inbox:

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

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git