@matheusbsbs / matheus-bagliano / commits / 7db118303a

unpack_trees: fix breakage when o->src_index != o->dst_index

Currently, all callers of unpack_trees() set o->src_index == o->dst_index. The code in unpack_trees() does not correctly handle them being different. There are two separate issues: First, there is the possibility of memory corruption. Since unpack_trees() creates a temporary index in o->result and then discards o->dst_index and overwrites it with o->result, in the special case that o->src_index == o->dst_index, it is safe to just reuse o->src_index's split_index for o->result. However, when src and dst are different, reusing o->src_index's split_index for o->result will cause the split_index to be shared. If either index then has entries replaced or removed, it will result in the other index referring to free()'d memory. Second, we can drop the index extensions. Previously, we were moving index extensions from o->dst_index to o->result. Since o->src_index is the one that will have the necessary extensions (o->dst_index is likely to be a new index temporary index created to store the results), we should be moving the index extensions from there. Signed-off-by: Elijah Newren <newren@gmail.com> Acked-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Elijah Newren committed Apr 23, 2018 at 23:50 UTC 7db118303aa04930bea66e855e8659e042f3fa5d

1 file changed +15 -4

unpack-trees.c

+15 -4

@@ -1284,9 +1284,20 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options

1284 o->result.timestamp.sec = o->src_index->timestamp.sec;

1285 o->result.timestamp.nsec = o->src_index->timestamp.nsec;

1286 o->result.version = o->src_index->version;

1287 - o->result.split_index = o->src_index->split_index;

1288 - if (o->result.split_index)

1287 + if (!o->src_index->split_index) {

1288 + o->result.split_index = NULL;

1289 + } else if (o->src_index == o->dst_index) {

1290 + /*

1291 + * o->dst_index (and thus o->src_index) will be discarded

1292 + * and overwritten with o->result at the end of this function,

1293 + * so just use src_index's split_index to avoid having to

1294 + * create a new one.

1295 + */

1296 + o->result.split_index = o->src_index->split_index;

1297 o->result.split_index->refcount++;

1298 + } else {

1299 + o->result.split_index = init_split_index(&o->result);

1300 + }

1301 hashcpy(o->result.sha1, o->src_index->sha1);

1302 o->merge_size = len;

1303 mark_all_ce_unused(o->src_index);

@@ -1401,7 +1412,6 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options

1412 }

1413 }

1414

1404 - o->src_index = NULL;

1415 ret = check_updates(o) ? (-2) : 0;

1416 if (o->dst_index) {

1417 if (!ret) {

@@ -1412,12 +1422,13 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options

1422 WRITE_TREE_SILENT |

1423 WRITE_TREE_REPAIR);

1424 }

1415 - move_index_extensions(&o->result, o->dst_index);

1425 + move_index_extensions(&o->result, o->src_index);

1426 discard_index(o->dst_index);

1427 *o->dst_index = o->result;

1428 } else {

1429 discard_index(&o->result);

1430 }

1431 + o->src_index = NULL;

1432

1433 done:

1434 clear_exclude_list(&el);