@matheusbsbs / matheus-bagliano / commits / 5aa02f9868

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>

Jeff King committed Jul 31, 2019 at 00:38 UTC 5aa02f98685d78666293149087d3f69b97528cfb

3 files changed +23 -10

tree-walk.c

+20 -8

@@ -181,21 +181,32 @@ void setup_traverse_info(struct traverse_info *info, const char *base)

181 info->prev = &dummy;

182 }

183

184 -char *make_traverse_path(char *path, const struct traverse_info *info,

184 +char *make_traverse_path(char *path, size_t pathlen,

185 + const struct traverse_info *info,

186 const char *name, size_t namelen)

187 {

187 - size_t pathlen = info->pathlen;

188 + /* Always points to the end of the name we're about to add */

189 + size_t pos = st_add(info->pathlen, namelen);

190

189 - path[pathlen + namelen] = 0;

191 + if (pos >= pathlen)

192 + BUG("too small buffer passed to make_traverse_path");

193 +

194 + path[pos] = 0;

195 for (;;) {

191 - memcpy(path + pathlen, name, namelen);

192 - if (!pathlen)

196 + if (pos < namelen)

197 + BUG("traverse_info pathlen does not match strings");

198 + pos -= namelen;

199 + memcpy(path + pos, name, namelen);

200 +

201 + if (!pos)

202 break;

194 - path[--pathlen] = '/';

203 + path[--pos] = '/';

204 +

205 + if (!info)

206 + BUG("traverse_info ran out of list items");

207 name = info->name;

208 namelen = info->namelen;

209 info = info->prev;

198 - pathlen -= namelen;

210 }

211 return path;

212 }

@@ -207,7 +218,8 @@ void strbuf_make_traverse_path(struct strbuf *out,

218 size_t len = traverse_path_len(info, namelen);

219

220 strbuf_grow(out, len);

210 - make_traverse_path(out->buf + out->len, info, name, namelen);

221 + make_traverse_path(out->buf + out->len, out->alloc - out->len,

222 + info, name, namelen);

223 strbuf_setlen(out, out->len + len);

224 }

225

tree-walk.h

+1 -1

@@ -70,7 +70,7 @@ struct traverse_info {

70 };

72 int get_tree_entry(const struct object_id *, const char *, struct object_id *, unsigned short *);

73 -char *make_traverse_path(char *path, const struct traverse_info *info,

73 +char *make_traverse_path(char *path, size_t pathlen, const struct traverse_info *info,

74 const char *name, size_t namelen);

75 void strbuf_make_traverse_path(struct strbuf *out,

76 const struct traverse_info *info,

unpack-trees.c

+2 -1

@@ -968,7 +968,8 @@ static struct cache_entry *create_ce_entry(const struct traverse_info *info,

968 ce->ce_flags = create_ce_flags(stage);

969 ce->ce_namelen = len;

970 oidcpy(&ce->oid, &n->oid);

971 - make_traverse_path(ce->name, info, n->path, n->pathlen);

971 + /* len+1 because the cache_entry allocates space for NUL */

972 + make_traverse_path(ce->name, len + 1, info, n->path, n->pathlen);

973

974 return ce;

975 }