Re: input validation in receive-pack

[Junio C Hamano <gitster@pobox.com>]

Daniel Barkalow <barkalow@iabervon.org> writes:

> On Tue, 1 Jan 2008, Junio C Hamano wrote:
>
>> mkoegler@auto.tuwien.ac.at (Martin Koegler) writes:
>> 
>> > In the update code path, the check is done in refs.c:
>> > | struct ref_lock *lock_any_ref_for_update(const char *ref, const unsigned char *old_sha1, int flags)
>> > | {
>> > |         if (check_ref_format(ref) == -1)
>> > |                 return NULL;
>> > |         return lock_ref_sha1_basic(ref, old_sha1, flags, NULL);
>> > | }
>> >
>> > check_ref_format may also return -2 (less than two name levels) and -3
>> > (* at the end), which are ignored. Is it really intended, that
>> > receive-pack can create such refs.
>> 
>> Misconversion in 8558fd9ece4c8250a037a6d5482a8040d600ef47 that
>> changed check_ref_format() without looking at what its callers
>> are checking, I think.
>
> When I got to it, it was already accepting -2. It clearly shouldn't accept 
> -3 (and I don't know why I missed it; I was probably misinterpreting the 
> original logic there.

You are right.

builtin-commit.c uses it to lock "HEAD", and update_ref() calls
it to update any ref so we cannot reject -2.  However, I do not
think allowing wildcard is useful for any caller.

-- >8 --
lock_any_ref_for_update(): reject wildcard return from check_ref_format

Recent check_ref_format() returns -3 as well as -1 (general
error) and -2 (less than two levels).  The caller was explicitly
checking for -1, to allow "HEAD" but still disallow bogus refs.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 refs.c |   27 ++++++++++++++++++---------
 refs.h |    5 ++++-
 2 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/refs.c b/refs.c
index 759924d..7484a46 100644
--- a/refs.c
+++ b/refs.c
@@ -613,32 +613,37 @@ int check_ref_format(const char *ref)
 		while ((ch = *cp++) == '/')
 			; /* tolerate duplicated slashes */
 		if (!ch)
-			return -1; /* should not end with slashes */
+			/* should not end with slashes */
+			return CHECK_REF_FORMAT_ERROR;
 
 		/* we are at the beginning of the path component */
 		if (ch == '.')
-			return -1;
+			return CHECK_REF_FORMAT_ERROR;
 		bad_type = bad_ref_char(ch);
 		if (bad_type) {
-			return (bad_type == 2 && !*cp) ? -3 : -1;
+			return (bad_type == 2 && !*cp)
+				? CHECK_REF_FORMAT_WILDCARD
+				: CHECK_REF_FORMAT_ERROR;
 		}
 
 		/* scan the rest of the path component */
 		while ((ch = *cp++) != 0) {
 			bad_type = bad_ref_char(ch);
 			if (bad_type) {
-				return (bad_type == 2 && !*cp) ? -3 : -1;
+				return (bad_type == 2 && !*cp)
+					? CHECK_REF_FORMAT_WILDCARD
+					: CHECK_REF_FORMAT_ERROR;
 			}
 			if (ch == '/')
 				break;
 			if (ch == '.' && *cp == '.')
-				return -1;
+				return CHECK_REF_FORMAT_ERROR;
 		}
 		level++;
 		if (!ch) {
 			if (level < 2)
-				return -2; /* at least of form "heads/blah" */
-			return 0;
+				return CHECK_REF_FORMAT_ONELEVEL;
+			return CHECK_REF_FORMAT_OK;
 		}
 	}
 }
@@ -816,9 +821,13 @@ struct ref_lock *lock_ref_sha1(const char *ref, const unsigned char *old_sha1)
 
 struct ref_lock *lock_any_ref_for_update(const char *ref, const unsigned char *old_sha1, int flags)
 {
-	if (check_ref_format(ref) == -1)
+	switch (check_ref_format(ref)) {
+	case CHECK_REF_FORMAT_ERROR:
+	case CHECK_REF_FORMAT_WILDCARD:
 		return NULL;
-	return lock_ref_sha1_basic(ref, old_sha1, flags, NULL);
+	default:
+		return lock_ref_sha1_basic(ref, old_sha1, flags, NULL);
+	}
 }
 
 static struct lock_file packlock;
diff --git a/refs.h b/refs.h
index 9dc8aa0..9cd16f8 100644
--- a/refs.h
+++ b/refs.h
@@ -52,7 +52,10 @@ int for_each_reflog_ent(const char *ref, each_reflog_ent_fn fn, void *cb_data);
  */
 extern int for_each_reflog(each_ref_fn, void *);
 
-/** Returns 0 if target has the right format for a ref. **/
+#define CHECK_REF_FORMAT_OK 0
+#define CHECK_REF_FORMAT_ERROR (-1)
+#define CHECK_REF_FORMAT_ONELEVEL (-2)
+#define CHECK_REF_FORMAT_WILDCARD (-3)
 extern int check_ref_format(const char *target);
 
 /** rename ref, return 0 on success **/