Skip to content

Commit 46220ca

Browse files
gitstergitster
committed
remote.c: Fix overtight refspec validation
We tightened the refspec validation code in an earlier commit ef00d15 (Tighten refspec processing, 2008-03-17) per my suggestion, but the suggestion was misguided to begin with and it broke this usage: $ git push origin HEAD~12:master The syntax of push refspecs and fetch refspecs are similar in that they are both colon separated LHS and RHS (possibly prefixed with a + to force), but the similarity ends there. For example, LHS in a push refspec can be anything that evaluates to a valid object name at runtime (except when colon and RHS is missing, or it is a glob), while it must be a valid-looking refname in a fetch refspec. To validate them correctly, the caller needs to be able to say which kind of refspecs they are. It is unreasonable to keep a single interface that cannot tell which kind it is dealing with, and ask it to behave sensibly. This commit separates the parsing of the two into different functions, and clarifies the code to implement the parsing proper (i.e. splitting into two parts, making sure both sides are wildcard or neither side is). This happens to also allow pushing a commit named with the esoteric "look for that string" syntax: $ git push ../test.git ':/remote.c: Fix overtight refspec:master' Signed-off-by: Junio C Hamano <gitster@pobox.com>
1 parent 9b33fa0 commit 46220ca

File tree

5 files changed

+188
-49
lines changed

5 files changed

+188
-49
lines changed

‎builtin-fetch.c‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -652,5 +652,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
652652

653653
signal(SIGINT, unlock_pack_on_signal);
654654
atexit(unlock_pack);
655-
return do_fetch(transport, parse_ref_spec(ref_nr, refs), ref_nr);
655+
return do_fetch(transport,
656+
parse_fetch_refspec(ref_nr, refs), ref_nr);
656657
}

‎builtin-send-pack.c‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -537,7 +537,7 @@ static void verify_remote_names(int nr_heads, const char **heads)
537537
int i;
538538

539539
for (i = 0; i < nr_heads; i++) {
540-
const char *remote = strchr(heads[i], ':');
540+
const char *remote = strrchr(heads[i], ':');
541541

542542
remote = remote ? (remote + 1) : heads[i];
543543
switch (check_ref_format(remote)) {

‎remote.c‎

Lines changed: 111 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -393,58 +393,123 @@ static void read_config(void)
393393
alias_all_urls();
394394
}
395395

396-
struct refspec *parse_ref_spec(int nr_refspec, const char **refspec)
396+
static struct refspec *parse_refspec_internal(int nr_refspec, const char **refspec, int fetch)
397397
{
398398
int i;
399399
int st;
400400
struct refspec *rs = xcalloc(sizeof(*rs), nr_refspec);
401+
401402
for (i = 0; i < nr_refspec; i++) {
402-
const char *sp, *ep, *gp;
403-
sp = refspec[i];
404-
if (*sp == '+') {
403+
size_t llen, rlen;
404+
int is_glob;
405+
const char *lhs, *rhs;
406+
407+
llen = rlen = is_glob = 0;
408+
409+
lhs = refspec[i];
410+
if (*lhs == '+') {
405411
rs[i].force = 1;
406-
sp++;
407-
}
408-
gp = strstr(sp, "/*");
409-
ep = strchr(sp, ':');
410-
if (gp && ep && gp > ep)
411-
gp = NULL;
412-
if (ep) {
413-
if (ep[1]) {
414-
const char *glob = strstr(ep + 1, "/*");
415-
if (glob && glob[2])
416-
glob = NULL;
417-
if (!glob)
418-
gp = NULL;
419-
if (gp)
420-
rs[i].dst = xstrndup(ep + 1,
421-
glob - ep - 1);
422-
else
423-
rs[i].dst = xstrdup(ep + 1);
424-
}
425-
} else {
426-
ep = sp + strlen(sp);
412+
lhs++;
427413
}
428-
if (gp && gp + 2 != ep)
429-
gp = NULL;
430-
if (gp) {
431-
rs[i].pattern = 1;
432-
ep = gp;
414+
415+
rhs = strrchr(lhs, ':');
416+
if (rhs) {
417+
rhs++;
418+
rlen = strlen(rhs);
419+
is_glob = (2 <= rlen && !strcmp(rhs + rlen - 2, "/*"));
420+
rs[i].dst = xstrndup(rhs, rlen - is_glob * 2);
433421
}
434-
rs[i].src = xstrndup(sp, ep - sp);
435422

436-
if (*rs[i].src) {
437-
st = check_ref_format(rs[i].src);
438-
if (st && st != CHECK_REF_FORMAT_ONELEVEL)
439-
die("Invalid refspec '%s'", refspec[i]);
423+
llen = (rhs ? (rhs - lhs - 1) : strlen(lhs));
424+
if (is_glob != (2 <= llen && !memcmp(lhs + llen - 2, "/*", 2)))
425+
goto invalid;
426+
427+
if (is_glob) {
428+
llen -= 2;
429+
rlen -= 2;
440430
}
441-
if (rs[i].dst && *rs[i].dst) {
442-
st = check_ref_format(rs[i].dst);
443-
if (st && st != CHECK_REF_FORMAT_ONELEVEL)
444-
die("Invalid refspec '%s'", refspec[i]);
431+
rs[i].pattern = is_glob;
432+
rs[i].src = xstrndup(lhs, llen);
433+
434+
if (fetch) {
435+
/*
436+
* LHS
437+
* - empty is allowed; it means HEAD.
438+
* - otherwise it must be a valid looking ref.
439+
*/
440+
if (!*rs[i].src)
441+
; /* empty is ok */
442+
else {
443+
st = check_ref_format(rs[i].src);
444+
if (st && st != CHECK_REF_FORMAT_ONELEVEL)
445+
goto invalid;
446+
}
447+
/*
448+
* RHS
449+
* - missing is allowed.
450+
* - empty is ok; it means not to store.
451+
* - otherwise it must be a valid looking ref.
452+
*/
453+
if (!rs[i].dst) {
454+
; /* ok */
455+
} else if (!*rs[i].dst) {
456+
; /* ok */
457+
} else {
458+
st = check_ref_format(rs[i].dst);
459+
if (st && st != CHECK_REF_FORMAT_ONELEVEL)
460+
goto invalid;
461+
}
462+
} else {
463+
/*
464+
* LHS
465+
* - empty is allowed; it means delete.
466+
* - when wildcarded, it must be a valid looking ref.
467+
* - otherwise, it must be an extended SHA-1, but
468+
* there is no existing way to validate this.
469+
*/
470+
if (!*rs[i].src)
471+
; /* empty is ok */
472+
else if (is_glob) {
473+
st = check_ref_format(rs[i].src);
474+
if (st && st != CHECK_REF_FORMAT_ONELEVEL)
475+
goto invalid;
476+
}
477+
else
478+
; /* anything goes, for now */
479+
/*
480+
* RHS
481+
* - missing is allowed, but LHS then must be a
482+
* valid looking ref.
483+
* - empty is not allowed.
484+
* - otherwise it must be a valid looking ref.
485+
*/
486+
if (!rs[i].dst) {
487+
st = check_ref_format(rs[i].src);
488+
if (st && st != CHECK_REF_FORMAT_ONELEVEL)
489+
goto invalid;
490+
} else if (!*rs[i].dst) {
491+
goto invalid;
492+
} else {
493+
st = check_ref_format(rs[i].dst);
494+
if (st && st != CHECK_REF_FORMAT_ONELEVEL)
495+
goto invalid;
496+
}
445497
}
446498
}
447499
return rs;
500+
501+
invalid:
502+
die("Invalid refspec '%s'", refspec[i]);
503+
}
504+
505+
struct refspec *parse_fetch_refspec(int nr_refspec, const char **refspec)
506+
{
507+
return parse_refspec_internal(nr_refspec, refspec, 1);
508+
}
509+
510+
struct refspec *parse_push_refspec(int nr_refspec, const char **refspec)
511+
{
512+
return parse_refspec_internal(nr_refspec, refspec, 0);
448513
}
449514

450515
static int valid_remote_nick(const char *name)
@@ -475,8 +540,8 @@ struct remote *remote_get(const char *name)
475540
add_url_alias(ret, name);
476541
if (!ret->url)
477542
return NULL;
478-
ret->fetch = parse_ref_spec(ret->fetch_refspec_nr, ret->fetch_refspec);
479-
ret->push = parse_ref_spec(ret->push_refspec_nr, ret->push_refspec);
543+
ret->fetch = parse_fetch_refspec(ret->fetch_refspec_nr, ret->fetch_refspec);
544+
ret->push = parse_push_refspec(ret->push_refspec_nr, ret->push_refspec);
480545
return ret;
481546
}
482547

@@ -489,11 +554,11 @@ int for_each_remote(each_remote_fn fn, void *priv)
489554
if (!r)
490555
continue;
491556
if (!r->fetch)
492-
r->fetch = parse_ref_spec(r->fetch_refspec_nr,
493-
r->fetch_refspec);
557+
r->fetch = parse_fetch_refspec(r->fetch_refspec_nr,
558+
r->fetch_refspec);
494559
if (!r->push)
495-
r->push = parse_ref_spec(r->push_refspec_nr,
496-
r->push_refspec);
560+
r->push = parse_push_refspec(r->push_refspec_nr,
561+
r->push_refspec);
497562
result = fn(r, priv);
498563
}
499564
return result;
@@ -824,7 +889,7 @@ int match_refs(struct ref *src, struct ref *dst, struct ref ***dst_tail,
824889
int nr_refspec, const char **refspec, int flags)
825890
{
826891
struct refspec *rs =
827-
parse_ref_spec(nr_refspec, (const char **) refspec);
892+
parse_push_refspec(nr_refspec, (const char **) refspec);
828893
int send_all = flags & MATCH_REFS_ALL;
829894
int send_mirror = flags & MATCH_REFS_MIRROR;
830895

‎remote.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,8 @@ void free_refs(struct ref *ref);
6767
*/
6868
void ref_remove_duplicates(struct ref *ref_map);
6969

70-
struct refspec *parse_ref_spec(int nr_refspec, const char **refspec);
70+
struct refspec *parse_fetch_refspec(int nr_refspec, const char **refspec);
71+
struct refspec *parse_push_refspec(int nr_refspec, const char **refspec);
7172

7273
int match_refs(struct ref *src, struct ref *dst, struct ref ***dst_tail,
7374
int nr_refspec, const char **refspec, int all);

‎t/t5511-refspec.sh‎

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,72 @@
1+
#!/bin/sh
2+
3+
test_description='refspec parsing'
4+
5+
. ./test-lib.sh
6+
7+
test_refspec () {
8+
9+
kind=$1 refspec=$2 expect=$3
10+
git config remote.frotz.url "." &&
11+
git config --remove-section remote.frotz &&
12+
git config remote.frotz.url "." &&
13+
git config "remote.frotz.$kind" "$refspec" &&
14+
if test "$expect" != invalid
15+
then
16+
title="$kind $refspec"
17+
test='git ls-remote frotz'
18+
else
19+
title="$kind $refspec (invalid)"
20+
test='test_must_fail git ls-remote frotz'
21+
fi
22+
test_expect_success "$title" "$test"
23+
}
24+
25+
test_refspec push '' invalid
26+
test_refspec push ':' invalid
27+
28+
test_refspec fetch ''
29+
test_refspec fetch ':'
30+
31+
test_refspec push 'refs/heads/*:refs/remotes/frotz/*'
32+
test_refspec push 'refs/heads/*:refs/remotes/frotz' invalid
33+
test_refspec push 'refs/heads:refs/remotes/frotz/*' invalid
34+
test_refspec push 'refs/heads/master:refs/remotes/frotz/xyzzy'
35+
36+
37+
# These have invalid LHS, but we do not have a formal "valid sha-1
38+
# expression syntax checker" so they are not checked with the current
39+
# code. They will be caught downstream anyway, but we may want to
40+
# have tighter check later...
41+
42+
: test_refspec push 'refs/heads/master::refs/remotes/frotz/xyzzy' invalid
43+
: test_refspec push 'refs/heads/maste :refs/remotes/frotz/xyzzy' invalid
44+
45+
test_refspec fetch 'refs/heads/*:refs/remotes/frotz/*'
46+
test_refspec fetch 'refs/heads/*:refs/remotes/frotz' invalid
47+
test_refspec fetch 'refs/heads:refs/remotes/frotz/*' invalid
48+
test_refspec fetch 'refs/heads/master:refs/remotes/frotz/xyzzy'
49+
test_refspec fetch 'refs/heads/master::refs/remotes/frotz/xyzzy' invalid
50+
test_refspec fetch 'refs/heads/maste :refs/remotes/frotz/xyzzy' invalid
51+
52+
test_refspec push 'master~1:refs/remotes/frotz/backup'
53+
test_refspec fetch 'master~1:refs/remotes/frotz/backup' invalid
54+
test_refspec push 'HEAD~4:refs/remotes/frotz/new'
55+
test_refspec fetch 'HEAD~4:refs/remotes/frotz/new' invalid
56+
57+
test_refspec push 'HEAD'
58+
test_refspec fetch 'HEAD'
59+
test_refspec push 'refs/heads/ nitfol' invalid
60+
test_refspec fetch 'refs/heads/ nitfol' invalid
61+
62+
test_refspec push 'HEAD:' invalid
63+
test_refspec fetch 'HEAD:'
64+
test_refspec push 'refs/heads/ nitfol:' invalid
65+
test_refspec fetch 'refs/heads/ nitfol:' invalid
66+
67+
test_refspec push ':refs/remotes/frotz/deleteme'
68+
test_refspec fetch ':refs/remotes/frotz/HEAD-to-me'
69+
test_refspec push ':refs/remotes/frotz/delete me' invalid
70+
test_refspec fetch ':refs/remotes/frotz/HEAD to me' invalid
71+
72+
test_done

0 commit comments

Comments
 (0)