Re: input validation in receive-pack
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:44:02
Subsystem:
the rest · Maintainer:
Linus Torvalds
Possibly related (same subject, not in this thread)
- 2016-06-15 · input validation in receive-pack · Martin Koegler <hidden>
- 2016-06-15 · Re: input validation in receive-pack · Junio C Hamano <hidden>
- 2016-06-15 · Re: input validation in receive-pack · Daniel Barkalow <hidden>
- 2016-06-15 · Re: input validation in receive-pack · Martin Koegler <hidden>
- 2016-06-15 · Re: input validation in receive-pack · Junio C Hamano <hidden>
Daniel Barkalow [off-list ref] writes:
It might be wise to make "default" the return NULL case, and list the two okay cases explicitly, so it doesn't need to be changed if check_ref_format() someday gets additional "okay for some purposes" values.
Sounds sensible.
Aside from that, it looks good, except that builtin-send-pack.c and fast-import.c should probably use the symbolic constants, too. (All other callers only check whether the value is true or not).
Also sensible. -- >8 -- Update callers of check_ref_format() This updates send-pack and fast-import to use symbolic constants for checking the return values from check_ref_format(), and also futureproof the logic in lock_any_ref_for_update() to explicitly name the case that is usually considered an error but is Ok for this particular use. Signed-off-by: Junio C Hamano <redacted> --- builtin-send-pack.c | 10 ++++++---- fast-import.c | 5 +++-- refs.c | 6 +++--- 3 files changed, 12 insertions(+), 9 deletions(-)
diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 25ae1fe..8afb1d0 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c@@ -541,10 +541,12 @@ static void verify_remote_names(int nr_heads, const char **heads) remote = remote ? (remote + 1) : heads[i]; switch (check_ref_format(remote)) { case 0: /* ok */ - case -2: /* ok but a single level -- that is fine for - * a match pattern. - */ - case -3: /* ok but ends with a pattern-match character */ + case CHECK_REF_FORMAT_ONELEVEL: + /* ok but a single level -- that is fine for + * a match pattern. + */ + case CHECK_REF_FORMAT_WILDCARD: + /* ok but ends with a pattern-match character */ continue; } die("remote part of refspec is not a valid name in %s",
diff --git a/fast-import.c b/fast-import.c
index 4646c05..74597c9 100644
--- a/fast-import.c
+++ b/fast-import.c@@ -642,8 +642,9 @@ static struct branch *new_branch(const char *name) if (b) die("Invalid attempt to create duplicate branch: %s", name); switch (check_ref_format(name)) { - case 0: break; /* its valid */ - case -2: break; /* valid, but too few '/', allow anyway */ + case 0: break; /* its valid */ + case CHECK_REF_FORMAT_ONELEVEL: + break; /* valid, but too few '/', allow anyway */ default: die("Branch name doesn't conform to GIT standards: %s", name); }
diff --git a/refs.c b/refs.c
index 7484a46..58f6d17 100644
--- a/refs.c
+++ b/refs.c@@ -822,10 +822,10 @@ 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) { switch (check_ref_format(ref)) { - case CHECK_REF_FORMAT_ERROR: - case CHECK_REF_FORMAT_WILDCARD: - return NULL; default: + return NULL; + case 0: + case CHECK_REF_FORMAT_ONELEVEL: return lock_ref_sha1_basic(ref, old_sha1, flags, NULL); } }