Skip to content

Commit 1b01cdb

Browse files
gitstergitster
committed
Merge branch 'jk/tree-walk-overflow'
Codepaths to walk tree objects have been audited for integer overflows and hardened. * jk/tree-walk-overflow: tree-walk: harden make_traverse_path() length computations tree-walk: add a strbuf wrapper for make_traverse_path() tree-walk: accept a raw length for traverse_path_len() tree-walk: use size_t consistently tree-walk: drop oid from traverse_info setup_traverse_info(): stop copying oid
2 parents 8aa76ab + 5aa02f9 commit 1b01cdb

File tree

6 files changed

+103
-68
lines changed

6 files changed

+103
-68
lines changed

‎Documentation/technical/api-tree-walking.txt‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -62,9 +62,7 @@ Initializing
6262
`setup_traverse_info`::
6363

6464
Initialize a `traverse_info` given the pathname of the tree to start
65-
traversing from. The `base` argument is assumed to be the `path`
66-
member of the `name_entry` being recursed into unless the tree is a
67-
top-level tree in which case the empty string ("") is used.
65+
traversing from.
6866

6967
Walking
7068
-------
@@ -140,6 +138,10 @@ same in the next callback invocation.
140138
This utilizes the memory structure of a tree entry to avoid the
141139
overhead of using a generic strlen().
142140

141+
`strbuf_make_traverse_path`::
142+
143+
Convenience wrapper to `make_traverse_path` into a strbuf.
144+
143145
Authors
144146
-------
145147

‎builtin/merge-tree.c‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -180,8 +180,9 @@ static struct merge_list *create_entry(unsigned stage, unsigned mode, const stru
180180

181181
static char *traverse_path(const struct traverse_info *info, const struct name_entry *n)
182182
{
183-
char *path = xmallocz(traverse_path_len(info, n) + the_hash_algo->rawsz);
184-
return make_traverse_path(path, info, n);
183+
struct strbuf buf = STRBUF_INIT;
184+
strbuf_make_traverse_path(&buf, info, n->path, n->pathlen);
185+
return strbuf_detach(&buf, NULL);
185186
}
186187

187188
static void resolve(const struct traverse_info *info, struct name_entry *ours, struct name_entry *result)

‎cache-tree.c‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -713,7 +713,7 @@ static struct cache_tree *find_cache_tree_from_traversal(struct cache_tree *root
713713
if (!info->prev)
714714
return root;
715715
our_parent = find_cache_tree_from_traversal(root, info->prev);
716-
return cache_tree_find(our_parent, info->name.path);
716+
return cache_tree_find(our_parent, info->name);
717717
}
718718

719719
int cache_tree_matches_traversal(struct cache_tree *root,

‎tree-walk.c‎

Lines changed: 42 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -170,40 +170,61 @@ int tree_entry_gently(struct tree_desc *desc, struct name_entry *entry)
170170

171171
void setup_traverse_info(struct traverse_info *info, const char *base)
172172
{
173-
int pathlen = strlen(base);
173+
size_t pathlen = strlen(base);
174174
static struct traverse_info dummy;
175175

176176
memset(info, 0, sizeof(*info));
177177
if (pathlen && base[pathlen-1] == '/')
178178
pathlen--;
179179
info->pathlen = pathlen ? pathlen + 1 : 0;
180-
info->name.path = base;
181-
info->name.pathlen = pathlen;
182-
if (pathlen) {
183-
hashcpy(info->name.oid.hash, (const unsigned char *)base + pathlen + 1);
180+
info->name = base;
181+
info->namelen = pathlen;
182+
if (pathlen)
184183
info->prev = &dummy;
185-
}
186184
}
187185

188-
char *make_traverse_path(char *path, const struct traverse_info *info, const struct name_entry *n)
186+
char *make_traverse_path(char *path, size_t pathlen,
187+
const struct traverse_info *info,
188+
const char *name, size_t namelen)
189189
{
190-
int len = tree_entry_len(n);
191-
int pathlen = info->pathlen;
190+
/* Always points to the end of the name we're about to add */
191+
size_t pos = st_add(info->pathlen, namelen);
192+
193+
if (pos >= pathlen)
194+
BUG("too small buffer passed to make_traverse_path");
192195

193-
path[pathlen + len] = 0;
196+
path[pos] = 0;
194197
for (;;) {
195-
memcpy(path + pathlen, n->path, len);
196-
if (!pathlen)
198+
if (pos < namelen)
199+
BUG("traverse_info pathlen does not match strings");
200+
pos -= namelen;
201+
memcpy(path + pos, name, namelen);
202+
203+
if (!pos)
197204
break;
198-
path[--pathlen] = '/';
199-
n = &info->name;
200-
len = tree_entry_len(n);
205+
path[--pos] = '/';
206+
207+
if (!info)
208+
BUG("traverse_info ran out of list items");
209+
name = info->name;
210+
namelen = info->namelen;
201211
info = info->prev;
202-
pathlen -= len;
203212
}
204213
return path;
205214
}
206215

216+
void strbuf_make_traverse_path(struct strbuf *out,
217+
const struct traverse_info *info,
218+
const char *name, size_t namelen)
219+
{
220+
size_t len = traverse_path_len(info, namelen);
221+
222+
strbuf_grow(out, len);
223+
make_traverse_path(out->buf + out->len, out->alloc - out->len,
224+
info, name, namelen);
225+
strbuf_setlen(out, out->len + len);
226+
}
227+
207228
struct tree_desc_skip {
208229
struct tree_desc_skip *prev;
209230
const void *ptr;
@@ -400,13 +421,12 @@ int traverse_trees(struct index_state *istate,
400421
tx[i].d = t[i];
401422

402423
if (info->prev) {
403-
strbuf_grow(&base, info->pathlen);
404-
make_traverse_path(base.buf, info->prev, &info->name);
405-
base.buf[info->pathlen-1] = '/';
406-
strbuf_setlen(&base, info->pathlen);
407-
traverse_path = xstrndup(base.buf, info->pathlen);
424+
strbuf_make_traverse_path(&base, info->prev,
425+
info->name, info->namelen);
426+
strbuf_addch(&base, '/');
427+
traverse_path = xstrndup(base.buf, base.len);
408428
} else {
409-
traverse_path = xstrndup(info->name.path, info->pathlen);
429+
traverse_path = xstrndup(info->name, info->pathlen);
410430
}
411431
info->traverse_path = traverse_path;
412432
for (;;) {

‎tree-walk.h‎

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -58,8 +58,11 @@ enum get_oid_result get_tree_entry_follow_symlinks(struct repository *r, struct
5858
struct traverse_info {
5959
const char *traverse_path;
6060
struct traverse_info *prev;
61-
struct name_entry name;
62-
int pathlen;
61+
const char *name;
62+
size_t namelen;
63+
unsigned mode;
64+
65+
size_t pathlen;
6366
struct pathspec *pathspec;
6467

6568
unsigned long df_conflicts;
@@ -69,12 +72,17 @@ struct traverse_info {
6972
};
7073

7174
int get_tree_entry(struct repository *, const struct object_id *, const char *, struct object_id *, unsigned short *);
72-
char *make_traverse_path(char *path, const struct traverse_info *info, const struct name_entry *n);
75+
char *make_traverse_path(char *path, size_t pathlen, const struct traverse_info *info,
76+
const char *name, size_t namelen);
77+
void strbuf_make_traverse_path(struct strbuf *out,
78+
const struct traverse_info *info,
79+
const char *name, size_t namelen);
7380
void setup_traverse_info(struct traverse_info *info, const char *base);
7481

75-
static inline int traverse_path_len(const struct traverse_info *info, const struct name_entry *n)
82+
static inline size_t traverse_path_len(const struct traverse_info *info,
83+
size_t namelen)
7684
{
77-
return info->pathlen + tree_entry_len(n);
85+
return st_add(info->pathlen, namelen);
7886
}
7987

8088
/* in general, positive means "kind of interesting" */

‎unpack-trees.c‎

Lines changed: 39 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -632,7 +632,7 @@ static int unpack_index_entry(struct cache_entry *ce,
632632
return ret;
633633
}
634634

635-
static int find_cache_pos(struct traverse_info *, const struct name_entry *);
635+
static int find_cache_pos(struct traverse_info *, const char *p, size_t len);
636636

637637
static void restore_cache_bottom(struct traverse_info *info, int bottom)
638638
{
@@ -651,7 +651,7 @@ static int switch_cache_bottom(struct traverse_info *info)
651651
if (o->diff_index_cached)
652652
return 0;
653653
ret = o->cache_bottom;
654-
pos = find_cache_pos(info->prev, &info->name);
654+
pos = find_cache_pos(info->prev, info->name, info->namelen);
655655

656656
if (pos < -1)
657657
o->cache_bottom = -2 - pos;
@@ -686,21 +686,19 @@ static int index_pos_by_traverse_info(struct name_entry *names,
686686
struct traverse_info *info)
687687
{
688688
struct unpack_trees_options *o = info->data;
689-
int len = traverse_path_len(info, names);
690-
char *name = xmalloc(len + 1 /* slash */ + 1 /* NUL */);
689+
struct strbuf name = STRBUF_INIT;
691690
int pos;
692691

693-
make_traverse_path(name, info, names);
694-
name[len++] = '/';
695-
name[len] = '\0';
696-
pos = index_name_pos(o->src_index, name, len);
692+
strbuf_make_traverse_path(&name, info, names->path, names->pathlen);
693+
strbuf_addch(&name, '/');
694+
pos = index_name_pos(o->src_index, name.buf, name.len);
697695
if (pos >= 0)
698696
BUG("This is a directory and should not exist in index");
699697
pos = -pos - 1;
700-
if (!starts_with(o->src_index->cache[pos]->name, name) ||
701-
(pos > 0 && starts_with(o->src_index->cache[pos-1]->name, name)))
698+
if (!starts_with(o->src_index->cache[pos]->name, name.buf) ||
699+
(pos > 0 && starts_with(o->src_index->cache[pos-1]->name, name.buf)))
702700
BUG("pos must point at the first entry in this directory");
703-
free(name);
701+
strbuf_release(&name);
704702
return pos;
705703
}
706704

@@ -811,8 +809,10 @@ static int traverse_trees_recursive(int n, unsigned long dirmask,
811809
newinfo = *info;
812810
newinfo.prev = info;
813811
newinfo.pathspec = info->pathspec;
814-
newinfo.name = *p;
815-
newinfo.pathlen += tree_entry_len(p) + 1;
812+
newinfo.name = p->path;
813+
newinfo.namelen = p->pathlen;
814+
newinfo.mode = p->mode;
815+
newinfo.pathlen = st_add3(newinfo.pathlen, tree_entry_len(p), 1);
816816
newinfo.df_conflicts |= df_conflicts;
817817

818818
/*
@@ -863,14 +863,18 @@ static int traverse_trees_recursive(int n, unsigned long dirmask,
863863
* itself - the caller needs to do the final check for the cache
864864
* entry having more data at the end!
865865
*/
866-
static int do_compare_entry_piecewise(const struct cache_entry *ce, const struct traverse_info *info, const struct name_entry *n)
866+
static int do_compare_entry_piecewise(const struct cache_entry *ce,
867+
const struct traverse_info *info,
868+
const char *name, size_t namelen,
869+
unsigned mode)
867870
{
868-
int len, pathlen, ce_len;
871+
int pathlen, ce_len;
869872
const char *ce_name;
870873

871874
if (info->prev) {
872875
int cmp = do_compare_entry_piecewise(ce, info->prev,
873-
&info->name);
876+
info->name, info->namelen,
877+
info->mode);
874878
if (cmp)
875879
return cmp;
876880
}
@@ -884,15 +888,15 @@ static int do_compare_entry_piecewise(const struct cache_entry *ce, const struct
884888
ce_len -= pathlen;
885889
ce_name = ce->name + pathlen;
886890

887-
len = tree_entry_len(n);
888-
return df_name_compare(ce_name, ce_len, S_IFREG, n->path, len, n->mode);
891+
return df_name_compare(ce_name, ce_len, S_IFREG, name, namelen, mode);
889892
}
890893

891894
static int do_compare_entry(const struct cache_entry *ce,
892895
const struct traverse_info *info,
893-
const struct name_entry *n)
896+
const char *name, size_t namelen,
897+
unsigned mode)
894898
{
895-
int len, pathlen, ce_len;
899+
int pathlen, ce_len;
896900
const char *ce_name;
897901
int cmp;
898902

@@ -902,7 +906,7 @@ static int do_compare_entry(const struct cache_entry *ce,
902906
* it is quicker to use the precomputed version.
903907
*/
904908
if (!info->traverse_path)
905-
return do_compare_entry_piecewise(ce, info, n);
909+
return do_compare_entry_piecewise(ce, info, name, namelen, mode);
906910

907911
cmp = strncmp(ce->name, info->traverse_path, info->pathlen);
908912
if (cmp)
@@ -917,29 +921,29 @@ static int do_compare_entry(const struct cache_entry *ce,
917921
ce_len -= pathlen;
918922
ce_name = ce->name + pathlen;
919923

920-
len = tree_entry_len(n);
921-
return df_name_compare(ce_name, ce_len, S_IFREG, n->path, len, n->mode);
924+
return df_name_compare(ce_name, ce_len, S_IFREG, name, namelen, mode);
922925
}
923926

924927
static int compare_entry(const struct cache_entry *ce, const struct traverse_info *info, const struct name_entry *n)
925928
{
926-
int cmp = do_compare_entry(ce, info, n);
929+
int cmp = do_compare_entry(ce, info, n->path, n->pathlen, n->mode);
927930
if (cmp)
928931
return cmp;
929932

930933
/*
931934
* Even if the beginning compared identically, the ce should
932935
* compare as bigger than a directory leading up to it!
933936
*/
934-
return ce_namelen(ce) > traverse_path_len(info, n);
937+
return ce_namelen(ce) > traverse_path_len(info, tree_entry_len(n));
935938
}
936939

937940
static int ce_in_traverse_path(const struct cache_entry *ce,
938941
const struct traverse_info *info)
939942
{
940943
if (!info->prev)
941944
return 1;
942-
if (do_compare_entry(ce, info->prev, &info->name))
945+
if (do_compare_entry(ce, info->prev,
946+
info->name, info->namelen, info->mode))
943947
return 0;
944948
/*
945949
* If ce (blob) is the same name as the path (which is a tree
@@ -954,7 +958,7 @@ static struct cache_entry *create_ce_entry(const struct traverse_info *info,
954958
struct index_state *istate,
955959
int is_transient)
956960
{
957-
int len = traverse_path_len(info, n);
961+
size_t len = traverse_path_len(info, tree_entry_len(n));
958962
struct cache_entry *ce =
959963
is_transient ?
960964
make_empty_transient_cache_entry(len) :
@@ -964,7 +968,8 @@ static struct cache_entry *create_ce_entry(const struct traverse_info *info,
964968
ce->ce_flags = create_ce_flags(stage);
965969
ce->ce_namelen = len;
966970
oidcpy(&ce->oid, &n->oid);
967-
make_traverse_path(ce->name, info, n);
971+
/* len+1 because the cache_entry allocates space for NUL */
972+
make_traverse_path(ce->name, len + 1, info, n->path, n->pathlen);
968973

969974
return ce;
970975
}
@@ -1057,13 +1062,12 @@ static int unpack_failed(struct unpack_trees_options *o, const char *message)
10571062
* the directory.
10581063
*/
10591064
static int find_cache_pos(struct traverse_info *info,
1060-
const struct name_entry *p)
1065+
const char *p, size_t p_len)
10611066
{
10621067
int pos;
10631068
struct unpack_trees_options *o = info->data;
10641069
struct index_state *index = o->src_index;
10651070
int pfxlen = info->pathlen;
1066-
int p_len = tree_entry_len(p);
10671071

10681072
for (pos = o->cache_bottom; pos < index->cache_nr; pos++) {
10691073
const struct cache_entry *ce = index->cache[pos];
@@ -1099,7 +1103,7 @@ static int find_cache_pos(struct traverse_info *info,
10991103
ce_len = ce_slash - ce_name;
11001104
else
11011105
ce_len = ce_namelen(ce) - pfxlen;
1102-
cmp = name_compare(p->path, p_len, ce_name, ce_len);
1106+
cmp = name_compare(p, p_len, ce_name, ce_len);
11031107
/*
11041108
* Exact match; if we have a directory we need to
11051109
* delay returning it.
@@ -1114,7 +1118,7 @@ static int find_cache_pos(struct traverse_info *info,
11141118
* E.g. ce_name == "t-i", and p->path == "t"; we may
11151119
* have "t/a" in the index.
11161120
*/
1117-
if (p_len < ce_len && !memcmp(ce_name, p->path, p_len) &&
1121+
if (p_len < ce_len && !memcmp(ce_name, p, p_len) &&
11181122
ce_name[p_len] < '/')
11191123
continue; /* keep looking */
11201124
break;
@@ -1125,7 +1129,7 @@ static int find_cache_pos(struct traverse_info *info,
11251129
static struct cache_entry *find_cache_entry(struct traverse_info *info,
11261130
const struct name_entry *p)
11271131
{
1128-
int pos = find_cache_pos(info, p);
1132+
int pos = find_cache_pos(info, p->path, p->pathlen);
11291133
struct unpack_trees_options *o = info->data;
11301134

11311135
if (0 <= pos)
@@ -1138,10 +1142,10 @@ static void debug_path(struct traverse_info *info)
11381142
{
11391143
if (info->prev) {
11401144
debug_path(info->prev);
1141-
if (*info->prev->name.path)
1145+
if (*info->prev->name)
11421146
putchar('/');
11431147
}
1144-
printf("%s", info->name.path);
1148+
printf("%s", info->name);
11451149
}
11461150

11471151
static void debug_name_entry(int i, struct name_entry *n)

0 commit comments

Comments
 (0)