tree-walk: harden make_traverse_path() length computations

The make_traverse_path() function isn't very careful about checking its
output buffer boundaries. In fact, it doesn't even _know_ the size of
the buffer it's writing to, and just assumes that the caller used
traverse_path_len() correctly. And even then we assume that our
traverse_info.pathlen components are all correct, and just blindly write
into the buffer.

Let's improve this situation a bit:

- have the caller pass in their allocated buffer length, which we'll
check against our own computations

- check for integer underflow as we do our backwards-insertion of
pathnames into the buffer

- check that we do not run out items in our list to traverse before
we've filled the expected number of bytes

None of these should be triggerable in practice (especially since our
switch to size_t everywhere in a previous commit), but it doesn't hurt
to check our assumptions.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/tree-walk.c b/tree-walk.c
index c2952f3793fd2f028b60ceb8764b8298189f054b..4f1e9d79ab9aa2a13a4dc63edbaf2b4c613105d3 100644 (file)
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -181,21 +181,32 @@ void setup_traverse_info(struct traverse_info *info, const char *base)
                info->prev = &dummy;
 }
 
-char *make_traverse_path(char *path, const struct traverse_info *info,
+char *make_traverse_path(char *path, size_t pathlen,
+                        const struct traverse_info *info,
                         const char *name, size_t namelen)
 {
-       size_t pathlen = info->pathlen;
+       /* Always points to the end of the name we're about to add */
+       size_t pos = st_add(info->pathlen, namelen);
 
-       path[pathlen + namelen] = 0;
+       if (pos >= pathlen)
+               BUG("too small buffer passed to make_traverse_path");
+
+       path[pos] = 0;
        for (;;) {
-               memcpy(path + pathlen, name, namelen);
-               if (!pathlen)
+               if (pos < namelen)
+                       BUG("traverse_info pathlen does not match strings");
+               pos -= namelen;
+               memcpy(path + pos, name, namelen);
+
+               if (!pos)
                        break;
-               path[--pathlen] = '/';
+               path[--pos] = '/';
+
+               if (!info)
+                       BUG("traverse_info ran out of list items");
                name = info->name;
                namelen = info->namelen;
                info = info->prev;
-               pathlen -= namelen;
        }
        return path;
 }
@@ -207,7 +218,8 @@ void strbuf_make_traverse_path(struct strbuf *out,
        size_t len = traverse_path_len(info, namelen);
 
        strbuf_grow(out, len);
-       make_traverse_path(out->buf + out->len, info, name, namelen);
+       make_traverse_path(out->buf + out->len, out->alloc - out->len,
+                          info, name, namelen);
        strbuf_setlen(out, out->len + len);
 }
 

diff --git a/tree-walk.h b/tree-walk.h
index 994c14a49964b3b3a27a73baeea892b4e168e98b..a3ad54e6ce7def4711268c9f972556efd53a6ff1 100644 (file)
--- a/tree-walk.h
+++ b/tree-walk.h
@@ -70,7 +70,7 @@ struct traverse_info {
 };
 
 int get_tree_entry(const struct object_id *, const char *, struct object_id *, unsigned short *);
-char *make_traverse_path(char *path, const struct traverse_info *info,
+char *make_traverse_path(char *path, size_t pathlen, const struct traverse_info *info,
                         const char *name, size_t namelen);
 void strbuf_make_traverse_path(struct strbuf *out,
                               const struct traverse_info *info,

diff --git a/unpack-trees.c b/unpack-trees.c
index c3059c2440cec5808057ba99f2eb202ce3d5b27a..65c4677578ffafc56259a91bd13797e063d87a49 100644 (file)
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -968,7 +968,8 @@ static struct cache_entry *create_ce_entry(const struct traverse_info *info,
        ce->ce_flags = create_ce_flags(stage);
        ce->ce_namelen = len;
        oidcpy(&ce->oid, &n->oid);
-       make_traverse_path(ce->name, info, n->path, n->pathlen);
+       /* len+1 because the cache_entry allocates space for NUL */
+       make_traverse_path(ce->name, len + 1, info, n->path, n->pathlen);
 
        return ce;
 }