interpret_branch_name: always respect "namelen" parameter
authorJeff King <peff@peff.net>
Wed, 15 Jan 2014 08:31:57 +0000 (03:31 -0500)
committerJunio C Hamano <gitster@pobox.com>
Wed, 15 Jan 2014 20:41:03 +0000 (12:41 -0800)
interpret_branch_name gets passed a "name" buffer to parse,
along with a "namelen" parameter representing its length. If
"namelen" is zero, we fallback to the NUL-terminated
string-length of "name".

However, it does not necessarily follow that if we have
gotten a non-zero "namelen", it is the NUL-terminated
string-length of "name". E.g., when get_sha1() is parsing
"foo:bar", we will be asked to operate only on the first
three characters.

Yet in interpret_branch_name and its helpers, we use string
functions like strchr() to operate on "name", looking past
the length we were given.  This can result in us mis-parsing
object names.  We should instead be limiting our search to
"namelen" bytes.

There are three distinct types of object names this patch
addresses:

  - The intrepret_empty_at helper uses strchr to find the
    next @-expression after our potential empty-at.  In an
    expression like "@:foo@bar", it erroneously thinks that
    the second "@" is relevant, even if we were asked only
    to look at the first character. This case is easy to
    trigger (and we test it in this patch).

  - When finding the initial @-mark for @{upstream}, we use
    strchr.  This means we might treat "foo:@{upstream}" as
    the upstream for "foo:", even though we were asked only
    to look at "foo". We cannot test this one in practice,
    because it is masked by another bug (which is fixed in
    the next patch).

  - The interpret_nth_prior_checkout helper did not receive
    the name length at all. This turns out not to be a
    problem in practice, though, because its parsing is so
    limited: it always starts from the far-left of the
    string, and will not tolerate a colon (which is
    currently the only way to get a smaller-than-strlen
    "namelen"). However, it's still worth fixing to make the
    code more obviously correct, and to future-proof us
    against callers with more exotic buffers.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
sha1_name.c
t/t1508-at-combinations.sh

index 47a71e3..afdff2f 100644 (file)
@@ -430,7 +430,7 @@ static inline int upstream_mark(const char *string, int len)
 }
 
 static int get_sha1_1(const char *name, int len, unsigned char *sha1, unsigned lookup_flags);
 }
 
 static int get_sha1_1(const char *name, int len, unsigned char *sha1, unsigned lookup_flags);
-static int interpret_nth_prior_checkout(const char *name, struct strbuf *buf);
+static int interpret_nth_prior_checkout(const char *name, int namelen, struct strbuf *buf);
 
 static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
 {
 
 static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
 {
@@ -492,7 +492,7 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
                struct strbuf buf = STRBUF_INIT;
                int detached;
 
                struct strbuf buf = STRBUF_INIT;
                int detached;
 
-               if (interpret_nth_prior_checkout(str, &buf) > 0) {
+               if (interpret_nth_prior_checkout(str, len, &buf) > 0) {
                        detached = (buf.len == 40 && !get_sha1_hex(buf.buf, sha1));
                        strbuf_release(&buf);
                        if (detached)
                        detached = (buf.len == 40 && !get_sha1_hex(buf.buf, sha1));
                        strbuf_release(&buf);
                        if (detached)
@@ -931,7 +931,8 @@ static int grab_nth_branch_switch(unsigned char *osha1, unsigned char *nsha1,
  * Parse @{-N} syntax, return the number of characters parsed
  * if successful; otherwise signal an error with negative value.
  */
  * Parse @{-N} syntax, return the number of characters parsed
  * if successful; otherwise signal an error with negative value.
  */
-static int interpret_nth_prior_checkout(const char *name, struct strbuf *buf)
+static int interpret_nth_prior_checkout(const char *name, int namelen,
+                                       struct strbuf *buf)
 {
        long nth;
        int retval;
 {
        long nth;
        int retval;
@@ -939,9 +940,11 @@ static int interpret_nth_prior_checkout(const char *name, struct strbuf *buf)
        const char *brace;
        char *num_end;
 
        const char *brace;
        char *num_end;
 
+       if (namelen < 4)
+               return -1;
        if (name[0] != '@' || name[1] != '{' || name[2] != '-')
                return -1;
        if (name[0] != '@' || name[1] != '{' || name[2] != '-')
                return -1;
-       brace = strchr(name, '}');
+       brace = memchr(name, '}', namelen);
        if (!brace)
                return -1;
        nth = strtol(name + 3, &num_end, 10);
        if (!brace)
                return -1;
        nth = strtol(name + 3, &num_end, 10);
@@ -1014,7 +1017,7 @@ static int interpret_empty_at(const char *name, int namelen, int len, struct str
                return -1;
 
        /* make sure it's a single @, or @@{.*}, not @foo */
                return -1;
 
        /* make sure it's a single @, or @@{.*}, not @foo */
-       next = strchr(name + len + 1, '@');
+       next = memchr(name + len + 1, '@', namelen - len - 1);
        if (next && next[1] != '{')
                return -1;
        if (!next)
        if (next && next[1] != '{')
                return -1;
        if (!next)
@@ -1120,7 +1123,7 @@ static int interpret_upstream_mark(const char *name, int namelen,
 int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)
 {
        char *at;
 int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)
 {
        char *at;
-       int len = interpret_nth_prior_checkout(name, buf);
+       int len = interpret_nth_prior_checkout(name, namelen, buf);
 
        if (!namelen)
                namelen = strlen(name);
 
        if (!namelen)
                namelen = strlen(name);
@@ -1134,7 +1137,7 @@ int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)
                        return reinterpret(name, namelen, len, buf);
        }
 
                        return reinterpret(name, namelen, len, buf);
        }
 
-       at = strchr(name, '@');
+       at = memchr(name, '@', namelen);
        if (!at)
                return -1;
 
        if (!at)
                return -1;
 
index ceb8449..078e119 100755 (executable)
@@ -9,8 +9,11 @@ check() {
                if test '$2' = 'commit'
                then
                        git log -1 --format=%s '$1' >actual
                if test '$2' = 'commit'
                then
                        git log -1 --format=%s '$1' >actual
-               else
+               elif test '$2' = 'ref'
+               then
                        git rev-parse --symbolic-full-name '$1' >actual
                        git rev-parse --symbolic-full-name '$1' >actual
+               else
+                       git cat-file -p '$1' >actual
                fi &&
                test_cmp expect actual
        "
                fi &&
                test_cmp expect actual
        "
@@ -82,4 +85,14 @@ check HEAD ref refs/heads/old-branch
 check "HEAD@{1}" commit new-two
 check "@{1}" commit old-one
 
 check "HEAD@{1}" commit new-two
 check "@{1}" commit old-one
 
+test_expect_success 'create path with @' '
+       echo content >normal &&
+       echo content >fun@ny &&
+       git add normal fun@ny &&
+       git commit -m "funny path"
+'
+
+check "@:normal" blob content
+check "@:fun@ny" blob content
+
 test_done
 test_done