* [PATCH 1/3] rev-parse: don't accept options after dashdash
2020-11-10 21:35 ` Jeff King
@ 2020-11-10 21:37 ` Jeff King
2020-11-10 21:38 ` [PATCH 2/3] rev-parse: put all options under the "-" check Jeff King
2020-11-10 21:40 ` [PATCH 3/3] rev-parse: handle --end-of-options Jeff King
2 siblings, 0 replies; 10+ messages in thread
From: Jeff King @ 2020-11-10 21:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Demi M. Obenour, Git
Because of the order in which we check options in rev-parse, there are a
few options we accept even after a "--". This is wrong, because the
whole point of "--" is to say "everything after here is a path". Let's
move the "did we see a dashdash" check (it's called "as_is" in the code)
to the top of the parsing loop.
Note there is one subtlety here. The options are ordered so that some
are checked before we even see if we're in a repository (they continue
the loop, and if we get past a certain point, then we do the repository
setup). By moving the as_is check higher, it's also in that "before
setup" section, even though it might look at the repository via
verify_filename(). However, this works out: we'd never set as_is until
we parse "--", and we don't parse that until after doing the setup.
An alternative here to avoid the subtlety is to put the as_is check at
the top of the post-setup options. But then every pre-setup option would
have to remember to check "if (!as_is && !strcmp(...))". So while this
is a bit magical, it's harder for future code to get wrong.
Signed-off-by: Jeff King <peff@peff.net>
---
I guess it could also shove them all into an:
if (!as_is) {
if (!strcmp("--local-env-vars"))
}
conditional. I do end up having to do that later for end-of-options
anyway.
builtin/rev-parse.c | 11 ++++++-----
t/t1506-rev-parse-diagnosis.sh | 9 +++++++++
2 files changed, 15 insertions(+), 5 deletions(-)
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index ed200c8af1..293428fa0d 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -622,6 +622,12 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
for (i = 1; i < argc; i++) {
const char *arg = argv[i];
+ if (as_is) {
+ if (show_file(arg, output_prefix) && as_is < 2)
+ verify_filename(prefix, arg, 0);
+ continue;
+ }
+
if (!strcmp(arg, "--local-env-vars")) {
int i;
for (i = 0; local_repo_env[i]; i++)
@@ -655,11 +661,6 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
i++;
continue;
}
- if (as_is) {
- if (show_file(arg, output_prefix) && as_is < 2)
- verify_filename(prefix, arg, 0);
- continue;
- }
if (!strcmp(arg,"-n")) {
if (++i >= argc)
die("-n requires an argument");
diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh
index 3e657e693b..2ed5d50059 100755
--- a/t/t1506-rev-parse-diagnosis.sh
+++ b/t/t1506-rev-parse-diagnosis.sh
@@ -254,4 +254,13 @@ test_expect_success 'escaped char does not trigger wildcard rule' '
test_must_fail git rev-parse "foo\\*bar"
'
+test_expect_success 'arg after dashdash not interpreted as option' '
+ cat >expect <<-\EOF &&
+ --
+ --local-env-vars
+ EOF
+ git rev-parse -- --local-env-vars >actual &&
+ test_cmp expect actual
+'
+
test_done
--
2.29.2.640.g9e24689a4c
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/3] rev-parse: put all options under the "-" check
2020-11-10 21:35 ` Jeff King
2020-11-10 21:37 ` [PATCH 1/3] rev-parse: don't accept options after dashdash Jeff King
@ 2020-11-10 21:38 ` Jeff King
2020-11-10 21:40 ` [PATCH 3/3] rev-parse: handle --end-of-options Jeff King
2 siblings, 0 replies; 10+ messages in thread
From: Jeff King @ 2020-11-10 21:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Demi M. Obenour, Git
The option-parsing loop of rev-parse checks whether the first character
of an arg is "-". If so, then it enters a series of conditionals
checking for individual options. But some options are inexplicably
outside of that outer conditional.
This doesn't produce the wrong behavior; the conditional is actually
redundant with the individual option checks, and it's really only its
fallback "continue" that we care about. But we should at least be
consistent.
One obvious alternative is that we could get rid of the conditional
entirely. But we'll be using the extra block it provides in the next
patch.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/rev-parse.c | 47 ++++++++++++++++++++++-----------------------
1 file changed, 23 insertions(+), 24 deletions(-)
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 293428fa0d..79689286d8 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -652,30 +652,6 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
did_repo_setup = 1;
}
- if (!strcmp(arg, "--git-path")) {
- if (!argv[i + 1])
- die("--git-path requires an argument");
- strbuf_reset(&buf);
- puts(relative_path(git_path("%s", argv[i + 1]),
- prefix, &buf));
- i++;
- continue;
- }
- if (!strcmp(arg,"-n")) {
- if (++i >= argc)
- die("-n requires an argument");
- if ((filter & DO_FLAGS) && (filter & DO_REVS)) {
- show(arg);
- show(argv[i]);
- }
- continue;
- }
- if (starts_with(arg, "-n")) {
- if ((filter & DO_FLAGS) && (filter & DO_REVS))
- show(arg);
- continue;
- }
-
if (*arg == '-') {
if (!strcmp(arg, "--")) {
as_is = 2;
@@ -684,6 +660,29 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
show_file(arg, 0);
continue;
}
+ if (!strcmp(arg, "--git-path")) {
+ if (!argv[i + 1])
+ die("--git-path requires an argument");
+ strbuf_reset(&buf);
+ puts(relative_path(git_path("%s", argv[i + 1]),
+ prefix, &buf));
+ i++;
+ continue;
+ }
+ if (!strcmp(arg,"-n")) {
+ if (++i >= argc)
+ die("-n requires an argument");
+ if ((filter & DO_FLAGS) && (filter & DO_REVS)) {
+ show(arg);
+ show(argv[i]);
+ }
+ continue;
+ }
+ if (starts_with(arg, "-n")) {
+ if ((filter & DO_FLAGS) && (filter & DO_REVS))
+ show(arg);
+ continue;
+ }
if (!strcmp(arg, "--default")) {
def = argv[++i];
if (!def)
--
2.29.2.640.g9e24689a4c
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 3/3] rev-parse: handle --end-of-options
2020-11-10 21:35 ` Jeff King
2020-11-10 21:37 ` [PATCH 1/3] rev-parse: don't accept options after dashdash Jeff King
2020-11-10 21:38 ` [PATCH 2/3] rev-parse: put all options under the "-" check Jeff King
@ 2020-11-10 21:40 ` Jeff King
2020-11-10 22:23 ` Junio C Hamano
2 siblings, 1 reply; 10+ messages in thread
From: Jeff King @ 2020-11-10 21:40 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Demi M. Obenour, Git
We taught rev-list a new way to separate options from revisions in
19e8789b23 (revision: allow --end-of-options to end option parsing,
2019-08-06), but rev-parse uses its own parser. It should know about
--end-of-options not only for consistency, but because it may be
presented with similarly ambiguous cases. E.g., if a caller does:
git rev-parse "$rev" -- "$path"
to parse an untrusted input, then it will get confused if $rev contains
an option-like string like "--local-env-vars". Or even "--not-real",
which we'd keep as an option to pass along to rev-list.
Or even more importantly:
git rev-parse --verify "$rev"
can be confused by options, even though its purpose is safely parsing
untrusted input. On the plus side, it will always fail the --verify
part, as it will not have parsed a revision, so the caller will
generally "fail closed" rather than continue to use the untrusted
string. But it will still trigger whatever option was in "$rev"; this
should be mostly harmless, since rev-parse options are all read-only,
but I didn't carefully audit all paths.
This patch lets callers write:
git rev-parse --end-of-options "$rev" -- "$path"
and:
git rev-parse --verify --end-of-options "$rev"
which will both treat "$rev" always as a revision parameter. The latter
is a bit clunky. It would be nicer if we had defined "--verify" to
require that its next argument be the revision. But we have not
historically done so, and:
git rev-parse --verify -q "$rev"
does currently work. I added a test here to confirm that we didn't break
that.
A few implementation notes:
- We don't document --end-of-options explicitly in commands, but rather
in gitcli(7). So I didn't give it its own section in git-rev-parse(1).
But I did call it out specifically in the --verify section, and
include it in the examples, which should show best practices.
- We don't have to re-indent the main option-parsing block, because we
can combine our "did we see end of options" check with "does it start
with a dash". The exception is the pre-setup options, which need
their own block.
- We do however have to pull the "--" parsing out of the "does it start
with dash" block, because we want to parse it even if we've seen
--end-of-options.
- We'll leave "--end-of-options" in the output. This is probably not
technically necessary, as a careful caller will do:
git rev-parse --end-of-options $revs -- $paths
and anything in $revs will be resolved to an object id. However, it
does help a slightly less careful caller like:
git rev-parse --end-of-options $revs_or_paths
where a path "--foo" will remain in the output as long as it also
exists on disk. In that case, it's helpful to retain --end-of-options
to get passed along to rev-list, s it would otherwise see just
"--foo".
Signed-off-by: Jeff King <peff@peff.net>
---
Documentation/git-rev-parse.txt | 8 +++--
builtin/rev-parse.c | 56 +++++++++++++++++++--------------
t/t1503-rev-parse-verify.sh | 13 ++++++++
t/t1506-rev-parse-diagnosis.sh | 16 ++++++++++
4 files changed, 68 insertions(+), 25 deletions(-)
diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
index 19b12b6d43..5013daa6ef 100644
--- a/Documentation/git-rev-parse.txt
+++ b/Documentation/git-rev-parse.txt
@@ -109,6 +109,10 @@ names an existing object that is a commit-ish (i.e. a commit, or an
annotated tag that points at a commit). To make sure that `$VAR`
names an existing object of any type, `git rev-parse "$VAR^{object}"`
can be used.
++
+Note that if you are verifying a name from an untrusted source, it is
+wise to use `--end-of-options` so that the name argument is not mistaken
+for another option.
-q::
--quiet::
@@ -446,15 +450,15 @@ $ git rev-parse --verify HEAD
* Print the commit object name from the revision in the $REV shell variable:
+
------------
-$ git rev-parse --verify $REV^{commit}
+$ git rev-parse --verify --end-of-options $REV^{commit}
------------
+
This will error out if $REV is empty or not a valid revision.
* Similar to above:
+
------------
-$ git rev-parse --default master --verify $REV
+$ git rev-parse --default master --verify --end-of-options $REV
------------
+
but if $REV is empty, the commit object name from master will be printed.
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 79689286d8..69ba7326cf 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -595,6 +595,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
struct object_context unused;
struct strbuf buf = STRBUF_INIT;
const int hexsz = the_hash_algo->hexsz;
+ int seen_end_of_options = 0;
if (argc > 1 && !strcmp("--parseopt", argv[1]))
return cmd_parseopt(argc - 1, argv + 1, prefix);
@@ -628,21 +629,23 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
continue;
}
- if (!strcmp(arg, "--local-env-vars")) {
- int i;
- for (i = 0; local_repo_env[i]; i++)
- printf("%s\n", local_repo_env[i]);
- continue;
- }
- if (!strcmp(arg, "--resolve-git-dir")) {
- const char *gitdir = argv[++i];
- if (!gitdir)
- die("--resolve-git-dir requires an argument");
- gitdir = resolve_gitdir(gitdir);
- if (!gitdir)
- die("not a gitdir '%s'", argv[i]);
- puts(gitdir);
- continue;
+ if (!seen_end_of_options) {
+ if (!strcmp(arg, "--local-env-vars")) {
+ int i;
+ for (i = 0; local_repo_env[i]; i++)
+ printf("%s\n", local_repo_env[i]);
+ continue;
+ }
+ if (!strcmp(arg, "--resolve-git-dir")) {
+ const char *gitdir = argv[++i];
+ if (!gitdir)
+ die("--resolve-git-dir requires an argument");
+ gitdir = resolve_gitdir(gitdir);
+ if (!gitdir)
+ die("not a gitdir '%s'", argv[i]);
+ puts(gitdir);
+ continue;
+ }
}
/* The rest of the options require a git repository. */
@@ -652,14 +655,15 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
did_repo_setup = 1;
}
- if (*arg == '-') {
- if (!strcmp(arg, "--")) {
- as_is = 2;
- /* Pass on the "--" if we show anything but files.. */
- if (filter & (DO_FLAGS | DO_REVS))
- show_file(arg, 0);
- continue;
- }
+ if (!strcmp(arg, "--")) {
+ as_is = 2;
+ /* Pass on the "--" if we show anything but files.. */
+ if (filter & (DO_FLAGS | DO_REVS))
+ show_file(arg, 0);
+ continue;
+ }
+
+ if (!seen_end_of_options && *arg == '-') {
if (!strcmp(arg, "--git-path")) {
if (!argv[i + 1])
die("--git-path requires an argument");
@@ -937,6 +941,12 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
puts(the_hash_algo->name);
continue;
}
+ if (!strcmp(arg, "--end-of-options")) {
+ seen_end_of_options = 1;
+ if (filter & (DO_FLAGS | DO_REVS))
+ show_file(arg, 0);
+ continue;
+ }
if (show_flag(arg) && verify)
die_no_single_rev(quiet);
continue;
diff --git a/t/t1503-rev-parse-verify.sh b/t/t1503-rev-parse-verify.sh
index 492edffa9c..dc9fe3cbf1 100755
--- a/t/t1503-rev-parse-verify.sh
+++ b/t/t1503-rev-parse-verify.sh
@@ -144,4 +144,17 @@ test_expect_success SYMLINKS 'ref resolution not confused by broken symlinks' '
test_must_fail git rev-parse --verify broken
'
+test_expect_success 'options can appear after --verify' '
+ git rev-parse --verify HEAD >expect &&
+ git rev-parse --verify -q HEAD >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'verify respects --end-of-options' '
+ git update-ref refs/heads/-tricky HEAD &&
+ git rev-parse --verify HEAD >expect &&
+ git rev-parse --verify --end-of-options -tricky >actual &&
+ test_cmp expect actual
+'
+
test_done
diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh
index 2ed5d50059..e2ae15a2cf 100755
--- a/t/t1506-rev-parse-diagnosis.sh
+++ b/t/t1506-rev-parse-diagnosis.sh
@@ -263,4 +263,20 @@ test_expect_success 'arg after dashdash not interpreted as option' '
test_cmp expect actual
'
+test_expect_success 'arg after end-of-options not interpreted as option' '
+ test_must_fail git rev-parse --end-of-options --not-real -- 2>err &&
+ test_i18ngrep bad.revision.*--not-real err
+'
+
+test_expect_success 'end-of-options still allows --' '
+ cat >expect <<-EOF &&
+ --end-of-options
+ $(git rev-parse --verify HEAD)
+ --
+ path
+ EOF
+ git rev-parse --end-of-options HEAD -- path >actual &&
+ test_cmp expect actual
+'
+
test_done
--
2.29.2.640.g9e24689a4c
^ permalink raw reply related [flat|nested] 10+ messages in thread