[PATCH 15/19] refs.c: fix handling of badly named refs

[Jonathan Nieder <jrnieder@gmail.com>]

From: Ronnie Sahlberg <sahlberg@google.com>
Date: Wed, 3 Sep 2014 11:45:43 -0700

We currently do not handle badly named refs well:

  $ cp .git/refs/heads/master .git/refs/heads/master.....@\*@\\.
  $ git branch
    fatal: Reference has invalid format: 'refs/heads/master.....@*@\.'
  $ git branch -D master.....@\*@\\.
    error: branch 'master.....@*@\.' not found.

Currently we can not really recover from a badly named ref with less than
manually deleting the .git/refs/heads/<refname> file. This will also help
if change the naming rules in the future. For example if we decide to remove
`, " and such from the set of valid characters.

The purpose of this change is to allow git branch --list to show these refs
and to allow git branch -d/-D and update-ref -d to delete them.
Other functions will continue to not handle these refs but can be changed in
later patches.

Introduce two new internal flags: RESOLVE_REF_ALLOW_BAD_NAME and REF_BADNAMEOK.

In resolving functions, refuse to resolve badly named refs unless the new
RESOLVE_REF_ALLOW_BAD_NAME flag is passed. For these cases, if the badly named
ref exists then flag it as REF_ISBROKEN and resolve it to nullsha1.

In locking functions, refuse to act on badly named refs unless the new
REF_BADNAMEOK flag is passed. This is currently used only from branch.c and
update-ref.c when deleting a ref.

Change the internal functions that read loose and packed refs to allow
badly named refs but flag them as REF_ISBROKEN just like unresolvable refs
are.

During ref iteration only include these refs during for_each_rawref but
none of the other iterators. I.e. just like unresolvable refs, flag
inclusion of these refs based on DO_FOR_EACH_INCLUDE_BROKEN during
iteration.

In the transaction API, always refuse to create or update badly named refs
and refuse to delete them unless REF_BADNAMEOK flag is passed.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 builtin/branch.c            | 13 ++++++----
 builtin/update-ref.c        |  3 ++-
 cache.h                     |  8 +++++-
 refs.c                      | 63 ++++++++++++++++++++++++++++++++++++++-------
 refs.h                      |  2 ++
 t/t1402-check-ref-format.sh | 48 ++++++++++++++++++++++++++++++++++
 6 files changed, 121 insertions(+), 16 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 7925660..5d5bc56 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -234,10 +234,13 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 		free(name);
 
 		name = mkpathdup(fmt, bname.buf);
-		target = resolve_ref_unsafe(name, sha1,
-					    &flags, RESOLVE_REF_NODEREF);
+		target = resolve_ref_unsafe(name, sha1, &flags,
+					    RESOLVE_REF_READING
+					    | RESOLVE_REF_NODEREF
+					    | RESOLVE_REF_ALLOW_BAD_NAME);
 		if (!target ||
-		    (!(flags & REF_ISSYMREF) && is_null_sha1(sha1))) {
+		    (!(flags & (REF_ISSYMREF|REF_ISBROKEN)) &&
+		     is_null_sha1(sha1))) {
 			error(remote_branch
 			      ? _("remote branch '%s' not found.")
 			      : _("branch '%s' not found."), bname.buf);
@@ -245,14 +248,14 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 			continue;
 		}
 
-		if (!(flags & REF_ISSYMREF) &&
+		if (!(flags & (REF_ISSYMREF|REF_ISBROKEN)) &&
 		    check_branch_commit(bname.buf, name, sha1, head_rev, kinds,
 					force)) {
 			ret = 1;
 			continue;
 		}
 
-		if (delete_ref(name, sha1, REF_NODEREF)) {
+		if (delete_ref(name, sha1, REF_NODEREF|REF_BADNAMEOK)) {
 			error(remote_branch
 			      ? _("Error deleting remote branch '%s'")
 			      : _("Error deleting branch '%s'"),
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 6c9be05..e379fdd 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -419,7 +419,8 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
 	if (no_deref)
 		flags = REF_NODEREF;
 	if (delete)
-		return delete_ref(refname, oldval ? oldsha1 : NULL, flags);
+		return delete_ref(refname, oldval ? oldsha1 : NULL,
+				  flags|REF_BADNAMEOK);
 	else
 		return update_ref(msg, refname, sha1, oldval ? oldsha1 : NULL,
 				  flags, UPDATE_REFS_DIE_ON_ERR);
diff --git a/cache.h b/cache.h
index 03ade12..03a6144 100644
--- a/cache.h
+++ b/cache.h
@@ -979,10 +979,16 @@ extern int read_ref(const char *refname, unsigned char *sha1);
  * NULL.  If more than MAXDEPTH recursive symbolic lookups are needed,
  * give up and return NULL.
  *
- * errno is set to something meaningful on error.
+ * RESOLVE_REF_ALLOW_BAD_NAME disables most of the ref name checking except
+ * for names that are absolute paths or contain ".." components. For both
+ * these cases the function will return NULL and set errno to EINVAL.
+ * If the name is bad then the function will set the REF_ISBROKEN flag and
+ * return the name, if the ref exists, or NULL, if it does not.
+ * When this flag is set, any badly named refs will resolve to nullsha1.
  */
 #define RESOLVE_REF_READING 0x01
 #define RESOLVE_REF_NODEREF 0x02
+#define RESOLVE_REF_ALLOW_BAD_NAME 0x04
 extern const char *resolve_ref_unsafe(const char *ref, unsigned char *sha1, int *flags, int resolve_flags);
 extern char *resolve_refdup(const char *ref, unsigned char *sha1, int *flags, int resolve_flags);
 
diff --git a/refs.c b/refs.c
index f11df33..6faf0c8 100644
--- a/refs.c
+++ b/refs.c
@@ -1060,7 +1060,13 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)
 
 		refname = parse_ref_line(refline, sha1);
 		if (refname) {
-			last = create_ref_entry(refname, sha1, REF_ISPACKED, 1);
+			int flag = REF_ISPACKED;
+
+			if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL|REFNAME_DOT_COMPONENT)) {
+				flag |= REF_ISBROKEN;
+				hashclr(sha1);
+			}
+			last = create_ref_entry(refname, sha1, flag, 0);
 			if (peeled == PEELED_FULLY ||
 			    (peeled == PEELED_TAGS && starts_with(refname, "refs/tags/")))
 				last->flag |= REF_KNOWS_PEELED;
@@ -1197,8 +1203,14 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir)
 				hashclr(sha1);
 				flag |= REF_ISBROKEN;
 			}
+			if (check_refname_format(refname.buf,
+						 REFNAME_ALLOW_ONELEVEL|
+						 REFNAME_DOT_COMPONENT)) {
+				hashclr(sha1);
+				flag |= REF_ISBROKEN;
+			}
 			add_entry_to_dir(dir,
-					 create_ref_entry(refname.buf, sha1, flag, 1));
+					 create_ref_entry(refname.buf, sha1, flag, 0));
 		}
 		strbuf_setlen(&refname, dirnamelen);
 	}
@@ -1344,6 +1356,18 @@ static const char *handle_missing_loose_ref(const char *refname,
 	}
 }
 
+static int escapes_cwd(const char *path) {
+	char *buf;
+	int result;
+
+	if (is_absolute_path(path))
+		return 1;
+	buf = xmalloc(strlen(path) + 1);
+	result = normalize_path_copy(buf, path);
+	free(buf);
+	return result;
+}
+
 /* This function needs to return a meaningful errno on failure */
 const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int *flags, int resolve_flags)
 {
@@ -1351,13 +1375,21 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int *fl
 	ssize_t len;
 	char buffer[256];
 	static char refname_buffer[256];
+	int bad_name = 0;
 
 	if (flags)
 		*flags = 0;
 
 	if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
-		errno = EINVAL;
-		return NULL;
+		if (!(resolve_flags & RESOLVE_REF_ALLOW_BAD_NAME) ||
+		    escapes_cwd(refname)) {
+			errno = EINVAL;
+			return NULL;
+		}
+		hashclr(sha1);
+		if (flags)
+			*flags |= REF_ISBROKEN;
+		bad_name = 1;
 	}
 	for (;;) {
 		char path[PATH_MAX];
@@ -1461,6 +1493,8 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int *fl
 				errno = EINVAL;
 				return NULL;
 			}
+			if (bad_name)
+				hashclr(sha1);
 			return refname;
 		}
 		if (flags)
@@ -2108,14 +2142,12 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 	int missing = 0;
 	int attempts_remaining = 3;
 
-	if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
-		errno = EINVAL;
-		return NULL;
-	}
-
 	lock = xcalloc(1, sizeof(struct ref_lock));
 	lock->lock_fd = -1;
 
+	if (flags & REF_BADNAMEOK)
+		resolve_flags |= RESOLVE_REF_ALLOW_BAD_NAME;
+
 	if (mustexist)
 		resolve_flags |= RESOLVE_REF_READING;
 	if (flags & REF_NODEREF)
@@ -3472,6 +3504,13 @@ int ref_transaction_update(struct ref_transaction *transaction,
 	if (have_old && !old_sha1)
 		die("BUG: have_old is true but old_sha1 is NULL");
 
+	if (!is_null_sha1(new_sha1) &&
+	    check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
+		strbuf_addf(err, "refusing to update ref with bad name %s",
+			    refname);
+		return -1;
+	}
+
 	update = add_update(transaction, refname);
 	hashcpy(update->new_sha1, new_sha1);
 	update->flags = flags;
@@ -3497,6 +3536,12 @@ int ref_transaction_create(struct ref_transaction *transaction,
 	if (!new_sha1 || is_null_sha1(new_sha1))
 		die("BUG: create ref with null new_sha1");
 
+	if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
+		strbuf_addf(err, "refusing to create ref with bad name %s",
+			    refname);
+		return -1;
+	}
+
 	update = add_update(transaction, refname);
 
 	hashcpy(update->new_sha1, new_sha1);
diff --git a/refs.h b/refs.h
index fafa493..a96e174 100644
--- a/refs.h
+++ b/refs.h
@@ -175,10 +175,12 @@ extern int peel_ref(const char *refname, unsigned char *sha1);
  * ref_transaction_create(), etc.
  * REF_NODEREF: act on the ref directly, instead of dereferencing
  *              symbolic references.
+ * REF_BADNAMEOK: allow locking a ref that has a bad name.
  *
  * Flags >= 0x100 are reserved for internal use.
  */
 #define REF_NODEREF	0x01
+#define REF_BADNAMEOK	0x02
 /*
  * This function sets errno to something meaningful on failure.
  */
diff --git a/t/t1402-check-ref-format.sh b/t/t1402-check-ref-format.sh
index 1a5a5f3..058fa37 100755
--- a/t/t1402-check-ref-format.sh
+++ b/t/t1402-check-ref-format.sh
@@ -196,4 +196,52 @@ invalid_ref_normalized 'heads///foo.lock'
 invalid_ref_normalized 'foo.lock/bar'
 invalid_ref_normalized 'foo.lock///bar'
 
+test_expect_success 'git branch shows badly named ref' '
+	cp .git/refs/heads/master .git/refs/heads/broken...ref &&
+	test_when_finished "rm -f .git/refs/heads/broken...ref" &&
+	git branch >output &&
+	grep -e "broken...ref" output
+'
+
+test_expect_success 'git branch -d can delete badly named ref' '
+	cp .git/refs/heads/master .git/refs/heads/broken...ref &&
+	test_when_finished "rm -f .git/refs/heads/broken...ref" &&
+	git branch -d broken...ref &&
+	git branch >output &&
+	! grep -e "broken...ref" output
+'
+
+test_expect_success 'git branch -D can delete badly named ref' '
+	cp .git/refs/heads/master .git/refs/heads/broken...ref &&
+	test_when_finished "rm -f .git/refs/heads/broken...ref" &&
+	git branch -D broken...ref &&
+	git branch >output &&
+	! grep -e "broken...ref" output
+'
+
+test_expect_success 'git update-ref -d can delete badly named ref' '
+	cp .git/refs/heads/master .git/refs/heads/broken...ref &&
+	test_when_finished "rm -f .git/refs/heads/broken...ref" &&
+	git update-ref -d refs/heads/broken...ref &&
+	git branch >output &&
+	! grep -e "broken...ref" output
+'
+
+test_expect_success 'git branch can not create a badly named ref' '
+	test_must_fail git branch broken...ref
+'
+
+test_expect_success 'git branch can not rename to a bad ref name' '
+	git branch goodref &&
+	test_must_fail git branch -m goodref broken...ref
+'
+
+test_expect_failure 'git branch can rename from a bad ref name' '
+	cp .git/refs/heads/master .git/refs/heads/broken...ref &&
+	test_when_finished "rm -f .git/refs/heads/broken...ref" &&
+	git branch -m broken...ref renamed &&
+	test_must_fail git rev-parse broken...ref &&
+	test_cmp_rev master renamed
+'
+
 test_done
-- 
2.1.0.rc2.206.gedb03e5