Re: [PATCH 0/3] --end-of-options marker

From: Jeff King <peff@peff.net>
Date: Aug 6, 2019, 17:33 UTC
Message-ID: <20190806173349.GA4961@sigill.intra.peff.net>
In-Reply-To: <xmqqa7cml0s9.fsf@gitster-ct.c.googlers.com>

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

Show 21 quoted lines

> 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