diff --git a/c/CHANGELOG.rst b/c/CHANGELOG.rst index e75b9e2bf1..05e12d021b 100644 --- a/c/CHANGELOG.rst +++ b/c/CHANGELOG.rst @@ -10,6 +10,23 @@ In development - Update to kastore 2.1.3 to include file security fix (:user:`jeromekelleher`, :pr:`3478`). +**Bugfixes** + +- Fix off-by-one node ID bounds check in ``tsk_table_collection_link_ancestors`` + which allowed out-of-bounds memory access (:user:`jeromekelleher`, :issue:`3484`). + +- Fix off-by-one node ID bounds check in ``tsk_table_collection_ibd_within`` and + ``tsk_table_collection_ibd_between`` which allowed out-of-bounds memory access + (:user:`jeromekelleher`, :issue:`3485`). + +- ``tsk_table_collection_delete_older`` now checks table integrity before + modifying any tables, avoiding out-of-bounds memory access for invalid node + or mutation references (:user:`jeromekelleher`, :issue:`3486`). + +- Check parameters before allocating memory in + ``tsk_treeseq_genealogical_nearest_neighbours``, avoiding a very large + allocation request for invalid numbers of reference sets (:user:`jeromekelleher`). + -------------------- [1.3.1] - 2026-03-06 -------------------- diff --git a/c/tests/test_tables.c b/c/tests/test_tables.c index 7fca96563a..a3803124b3 100644 --- a/c/tests/test_tables.c +++ b/c/tests/test_tables.c @@ -1264,7 +1264,7 @@ test_node_table(void) const char *example = "An example of metadata schema with unicode 🎄🌳🌴🌲🎋"; tsk_size_t example_length = (tsk_size_t) strlen(example); const char *example2 = "A different example 🎄🌳🌴🌲🎋"; - tsk_size_t example2_length = (tsk_size_t) strlen(example); + tsk_size_t example2_length = (tsk_size_t) strlen(example2); tsk_node_table_set_metadata_schema(&table, example, example_length); CU_ASSERT_EQUAL(table.metadata_schema_length, example_length); CU_ASSERT_EQUAL(tsk_memcmp(table.metadata_schema, example, example_length), 0); @@ -1963,7 +1963,7 @@ test_edge_table_with_options(tsk_flags_t options) const char *example = "An example of metadata schema with unicode 🎄🌳🌴🌲🎋"; tsk_size_t example_length = (tsk_size_t) strlen(example); const char *example2 = "A different example 🎄🌳🌴🌲🎋"; - tsk_size_t example2_length = (tsk_size_t) strlen(example); + tsk_size_t example2_length = (tsk_size_t) strlen(example2); ret = tsk_edge_table_set_metadata_schema(&table, example, example_length); CU_ASSERT_EQUAL_FATAL(ret, 0); CU_ASSERT_EQUAL(table.metadata_schema_length, example_length); @@ -2998,7 +2998,7 @@ test_site_table(void) const char *example = "An example of metadata schema with unicode 🎄🌳🌴🌲🎋"; tsk_size_t example_length = (tsk_size_t) strlen(example); const char *example2 = "A different example 🎄🌳🌴🌲🎋"; - tsk_size_t example2_length = (tsk_size_t) strlen(example); + tsk_size_t example2_length = (tsk_size_t) strlen(example2); tsk_site_table_set_metadata_schema(&table, example, example_length); CU_ASSERT_EQUAL(table.metadata_schema_length, example_length); CU_ASSERT_EQUAL(tsk_memcmp(table.metadata_schema, example, example_length), 0); @@ -3726,7 +3726,7 @@ test_mutation_table(void) const char *example = "An example of metadata schema with unicode 🎄🌳🌴🌲🎋"; tsk_size_t example_length = (tsk_size_t) strlen(example); const char *example2 = "A different example 🎄🌳🌴🌲🎋"; - tsk_size_t example2_length = (tsk_size_t) strlen(example); + tsk_size_t example2_length = (tsk_size_t) strlen(example2); tsk_mutation_table_set_metadata_schema(&table, example, example_length); CU_ASSERT_EQUAL(table.metadata_schema_length, example_length); CU_ASSERT_EQUAL(tsk_memcmp(table.metadata_schema, example, example_length), 0); @@ -4568,7 +4568,7 @@ test_migration_table(void) const char *example = "An example of metadata schema with unicode 🎄🌳🌴🌲🎋"; tsk_size_t example_length = (tsk_size_t) strlen(example); const char *example2 = "A different example 🎄🌳🌴🌲🎋"; - tsk_size_t example2_length = (tsk_size_t) strlen(example); + tsk_size_t example2_length = (tsk_size_t) strlen(example2); tsk_migration_table_set_metadata_schema(&table, example, example_length); CU_ASSERT_EQUAL(table.metadata_schema_length, example_length); CU_ASSERT_EQUAL(tsk_memcmp(table.metadata_schema, example, example_length), 0); @@ -5365,7 +5365,7 @@ test_individual_table(void) const char *example = "An example of metadata schema with unicode 🎄🌳🌴🌲🎋"; tsk_size_t example_length = (tsk_size_t) strlen(example); const char *example2 = "A different example 🎄🌳🌴🌲🎋"; - tsk_size_t example2_length = (tsk_size_t) strlen(example); + tsk_size_t example2_length = (tsk_size_t) strlen(example2); tsk_individual_table_set_metadata_schema(&table, example, example_length); CU_ASSERT_EQUAL(table.metadata_schema_length, example_length); CU_ASSERT_EQUAL(tsk_memcmp(table.metadata_schema, example, example_length), 0); @@ -6072,7 +6072,7 @@ test_population_table(void) const char *example = "An example of metadata schema with unicode 🎄🌳🌴🌲🎋"; tsk_size_t example_length = (tsk_size_t) strlen(example); const char *example2 = "A different example 🎄🌳🌴🌲🎋"; - tsk_size_t example2_length = (tsk_size_t) strlen(example); + tsk_size_t example2_length = (tsk_size_t) strlen(example2); tsk_population_table_set_metadata_schema(&table, example, example_length); CU_ASSERT_EQUAL(table.metadata_schema_length, example_length); CU_ASSERT_EQUAL(tsk_memcmp(table.metadata_schema, example, example_length), 0); @@ -7618,6 +7618,11 @@ test_link_ancestors_input_errors(void) &tables, samples, 2, ancestors, 2, 0, &result); CU_ASSERT_EQUAL_FATAL(ret, TSK_ERR_NODE_OUT_OF_BOUNDS); + samples[0] = (tsk_id_t) tables.nodes.num_rows; + ret = tsk_table_collection_link_ancestors( + &tables, samples, 2, ancestors, 2, 0, &result); + CU_ASSERT_EQUAL_FATAL(ret, TSK_ERR_NODE_OUT_OF_BOUNDS); + /* Bad ancestor IDs */ samples[0] = 0; ancestors[0] = -1; @@ -7625,6 +7630,11 @@ test_link_ancestors_input_errors(void) &tables, samples, 2, ancestors, 2, 0, &result); CU_ASSERT_EQUAL_FATAL(ret, TSK_ERR_NODE_OUT_OF_BOUNDS); + ancestors[0] = (tsk_id_t) tables.nodes.num_rows; + ret = tsk_table_collection_link_ancestors( + &tables, samples, 2, ancestors, 2, 0, &result); + CU_ASSERT_EQUAL_FATAL(ret, TSK_ERR_NODE_OUT_OF_BOUNDS); + /* Duplicate sample IDs */ ancestors[0] = 4; samples[0] = 1; @@ -7754,6 +7764,44 @@ test_link_ancestors_samples_and_ancestors_overlap(void) tsk_treeseq_free(&ts); } +static void +test_link_ancestors_skip_internal_node(void) +{ + int ret; + tsk_treeseq_t ts; + tsk_table_collection_t tables; + tsk_edge_table_t result; + /* Node 4 is the parent of 0 and 1 but is not an ancestor, so it carries + * separate ancestry segments mapping to 0 and 1 up to node 6. */ + tsk_id_t samples[] = { 0, 1 }; + tsk_id_t ancestors[] = { 6 }; + tsk_id_t res_child[] = { 0, 1 }; + size_t i; + + tsk_treeseq_from_text(&ts, 1, single_tree_ex_nodes, single_tree_ex_edges, NULL, NULL, + NULL, NULL, NULL, 0); + ret = tsk_treeseq_copy_tables(&ts, &tables, 0); + CU_ASSERT_EQUAL_FATAL(ret, 0); + ret = tsk_edge_table_init(&result, 0); + CU_ASSERT_EQUAL_FATAL(ret, 0); + + ret = tsk_table_collection_link_ancestors( + &tables, samples, 2, ancestors, 1, 0, &result); + CU_ASSERT_EQUAL_FATAL(ret, 0); + + CU_ASSERT_EQUAL_FATAL(result.num_rows, 2); + for (i = 0; i < result.num_rows; i++) { + CU_ASSERT_EQUAL(result.parent[i], 6); + CU_ASSERT_EQUAL(result.child[i], res_child[i]); + CU_ASSERT_EQUAL(result.left[i], 0); + CU_ASSERT_EQUAL(result.right[i], 1); + } + + tsk_table_collection_free(&tables); + tsk_edge_table_free(&result); + tsk_treeseq_free(&ts); +} + static void test_link_ancestors_paper(void) { @@ -8384,6 +8432,7 @@ test_ibd_segments_errors(void) tsk_id_t samples[] = { 0, 1, 2 }; tsk_id_t duplicate_samples[] = { 0, 1, 0 }; tsk_id_t samples2[] = { -1, 1 }; + tsk_id_t samples3[] = { 0, 1, 2 }; tsk_size_t sample_set_sizes[] = { 3 }; tsk_identity_segments_t result; tsk_identity_segment_list_t *list; @@ -8404,6 +8453,17 @@ test_ibd_segments_errors(void) CU_ASSERT_EQUAL_FATAL(ret, TSK_ERR_NODE_OUT_OF_BOUNDS); tsk_identity_segments_free(&result); + samples3[0] = (tsk_id_t) tables.nodes.num_rows; + ret = tsk_table_collection_ibd_within( + &tables, &result, samples3, 3, 0.0, DBL_MAX, TSK_IBD_STORE_SEGMENTS); + CU_ASSERT_EQUAL_FATAL(ret, TSK_ERR_NODE_OUT_OF_BOUNDS); + tsk_identity_segments_free(&result); + + ret = tsk_table_collection_ibd_between(&tables, &result, 1, sample_set_sizes, + samples3, 0.0, DBL_MAX, TSK_IBD_STORE_SEGMENTS); + CU_ASSERT_EQUAL_FATAL(ret, TSK_ERR_NODE_OUT_OF_BOUNDS); + tsk_identity_segments_free(&result); + // Bad length or time ret = tsk_table_collection_ibd_within(&tables, &result, samples, 2, 0.0, -1, 0); CU_ASSERT_EQUAL_FATAL(ret, TSK_ERR_BAD_PARAM_VALUE); @@ -11886,6 +11946,60 @@ test_table_collection_delete_older(void) tsk_treeseq_free(&ts); } +static void +test_table_collection_delete_older_errors(void) +{ + int ret; + tsk_treeseq_t ts; + tsk_table_collection_t t, copy; + + const char *mutations = "0 2 1 -1\n" + "0 2 0 0\n" + "1 0 1 -1\n" + "2 5 1 -1\n"; + + tsk_treeseq_from_text(&ts, 10, paper_ex_nodes, paper_ex_edges, NULL, paper_ex_sites, + mutations, paper_ex_individuals, NULL, 0); + ret = tsk_treeseq_copy_tables(&ts, &t, 0); + CU_ASSERT_EQUAL_FATAL(ret, 0); + tsk_treeseq_free(&ts); + + /* Bad edge parent */ + t.edges.parent[0] = (tsk_id_t) t.nodes.num_rows; + ret = tsk_table_collection_copy(&t, ©, 0); + CU_ASSERT_EQUAL_FATAL(ret, 0); + ret = tsk_table_collection_delete_older(&t, 0.09, 0); + CU_ASSERT_EQUAL_FATAL(ret, TSK_ERR_NODE_OUT_OF_BOUNDS); + CU_ASSERT_TRUE(tsk_table_collection_equals(&t, ©, 0)); + t.edges.parent[0] = 4; + tsk_table_collection_free(©); + + /* Bad mutation node */ + t.mutations.node[0] = (tsk_id_t) t.nodes.num_rows; + ret = tsk_table_collection_copy(&t, ©, 0); + CU_ASSERT_EQUAL_FATAL(ret, 0); + ret = tsk_table_collection_delete_older(&t, 0.09, 0); + CU_ASSERT_EQUAL_FATAL(ret, TSK_ERR_NODE_OUT_OF_BOUNDS); + CU_ASSERT_TRUE(tsk_table_collection_equals(&t, ©, 0)); + t.mutations.node[0] = 2; + tsk_table_collection_free(©); + + /* Bad mutation parent */ + t.mutations.parent[1] = (tsk_id_t) t.mutations.num_rows; + ret = tsk_table_collection_copy(&t, ©, 0); + CU_ASSERT_EQUAL_FATAL(ret, 0); + ret = tsk_table_collection_delete_older(&t, 0.09, 0); + CU_ASSERT_EQUAL_FATAL(ret, TSK_ERR_MUTATION_OUT_OF_BOUNDS); + CU_ASSERT_TRUE(tsk_table_collection_equals(&t, ©, 0)); + t.mutations.parent[1] = 0; + tsk_table_collection_free(©); + + ret = tsk_table_collection_delete_older(&t, 0.09, 0); + CU_ASSERT_EQUAL_FATAL(ret, 0); + + tsk_table_collection_free(&t); +} + int main(int argc, char **argv) { @@ -11961,6 +12075,8 @@ main(int argc, char **argv) { "test_link_ancestors_no_edges", test_link_ancestors_no_edges }, { "test_link_ancestors_input_errors", test_link_ancestors_input_errors }, { "test_link_ancestors_single_tree", test_link_ancestors_single_tree }, + { "test_link_ancestors_skip_internal_node", + test_link_ancestors_skip_internal_node }, { "test_link_ancestors_paper", test_link_ancestors_paper }, { "test_link_ancestors_samples_and_ancestors_overlap", test_link_ancestors_samples_and_ancestors_overlap }, @@ -12030,6 +12146,8 @@ main(int argc, char **argv) { "test_table_collection_takeset_indexes", test_table_collection_takeset_indexes }, { "test_table_collection_delete_older", test_table_collection_delete_older }, + { "test_table_collection_delete_older_errors", + test_table_collection_delete_older_errors }, { NULL, NULL }, }; diff --git a/c/tests/test_trees.c b/c/tests/test_trees.c index 06c36ed45e..0a7f0c0a07 100644 --- a/c/tests/test_trees.c +++ b/c/tests/test_trees.c @@ -7873,7 +7873,7 @@ test_zero_edges(void) "1 1 1\n"; tsk_treeseq_t ts, tss; tsk_tree_t t; - tsk_id_t samples, node_map; + tsk_id_t samples, node_map[2]; const tsk_id_t z = TSK_NULL; tsk_id_t parents[] = { z, @@ -7921,9 +7921,12 @@ test_zero_edges(void) tsk_tree_print_state(&t, _devnull); tsk_tree_free(&t); - /* We give pointers ot samples and node_map here as they must be non null */ - ret = tsk_treeseq_simplify(&ts, &samples, 0, 0, &tss, &node_map); + /* We give a pointer to samples here as it must be non null; node_map must + * have space for all input nodes */ + ret = tsk_treeseq_simplify(&ts, &samples, 0, 0, &tss, node_map); CU_ASSERT_EQUAL_FATAL(ret, 0); + CU_ASSERT_EQUAL(node_map[0], TSK_NULL); + CU_ASSERT_EQUAL(node_map[1], TSK_NULL); CU_ASSERT_EQUAL(tsk_treeseq_get_num_samples(&tss), 0); CU_ASSERT_EQUAL(tsk_treeseq_get_sequence_length(&tss), 2.0); CU_ASSERT_EQUAL(tsk_treeseq_get_num_nodes(&tss), 0); diff --git a/c/tskit/tables.c b/c/tskit/tables.c index d673619141..daf8e9c4c0 100644 --- a/c/tskit/tables.c +++ b/c/tskit/tables.c @@ -7905,7 +7905,7 @@ ancestor_mapper_init_samples(ancestor_mapper_t *self, tsk_id_t *samples) /* Go through the samples to check for errors. */ for (j = 0; j < self->num_samples; j++) { - if (samples[j] < 0 || samples[j] > (tsk_id_t) self->tables->nodes.num_rows) { + if (samples[j] < 0 || samples[j] >= (tsk_id_t) self->tables->nodes.num_rows) { ret = tsk_trace_error(TSK_ERR_NODE_OUT_OF_BOUNDS); goto out; } @@ -7932,7 +7932,8 @@ ancestor_mapper_init_ancestors(ancestor_mapper_t *self, tsk_id_t *ancestors) /* Go through the samples to check for errors. */ for (j = 0; j < self->num_ancestors; j++) { - if (ancestors[j] < 0 || ancestors[j] > (tsk_id_t) self->tables->nodes.num_rows) { + if (ancestors[j] < 0 + || ancestors[j] >= (tsk_id_t) self->tables->nodes.num_rows) { ret = tsk_trace_error(TSK_ERR_NODE_OUT_OF_BOUNDS); goto out; } @@ -8632,7 +8633,7 @@ tsk_ibd_finder_init_samples_from_set( for (j = 0; j < num_samples; j++) { u = samples[j]; - if (u < 0 || u > (tsk_id_t) self->tables->nodes.num_rows) { + if (u < 0 || u >= (tsk_id_t) self->tables->nodes.num_rows) { ret = tsk_trace_error(TSK_ERR_NODE_OUT_OF_BOUNDS); goto out; } @@ -8886,7 +8887,7 @@ tsk_ibd_finder_init_between(tsk_ibd_finder_t *self, tsk_size_t num_sample_sets, for (j = 0; j < num_sample_sets; j++) { for (k = 0; k < sample_set_sizes[j]; k++) { u = sample_sets[index]; - if (u < 0 || u > (tsk_id_t) self->tables->nodes.num_rows) { + if (u < 0 || u >= (tsk_id_t) self->tables->nodes.num_rows) { ret = tsk_trace_error(TSK_ERR_NODE_OUT_OF_BOUNDS); goto out; } @@ -12629,6 +12630,10 @@ tsk_table_collection_delete_older( memset(&mutations, 0, sizeof(mutations)); memset(&migrations, 0, sizeof(migrations)); + ret = (int) tsk_table_collection_check_integrity(self, 0); + if (ret != 0) { + goto out; + } ret = tsk_edge_table_copy(&self->edges, &edges, 0); if (ret != 0) { goto out; diff --git a/c/tskit/trees.c b/c/tskit/trees.c index 1aa06e5b03..2fcf399e20 100644 --- a/c/tskit/trees.c +++ b/c/tskit/trees.c @@ -911,12 +911,10 @@ tsk_treeseq_genealogical_nearest_neighbours(const tsk_treeseq_t *self, const double sequence_length = self->tables->sequence_length; tsk_id_t tj, tk, h; double left, right, *A_row, scale, tree_length; - tsk_id_t *restrict parent = tsk_malloc(num_nodes * sizeof(*parent)); - double *restrict length = tsk_calloc(num_focal, sizeof(*length)); - uint32_t *restrict ref_count - = tsk_calloc(((tsk_size_t) K) * num_nodes, sizeof(*ref_count)); - int16_t *restrict reference_set_map - = tsk_malloc(num_nodes * sizeof(*reference_set_map)); + tsk_id_t *restrict parent = NULL; + double *restrict length = NULL; + uint32_t *restrict ref_count = NULL; + int16_t *restrict reference_set_map = NULL; uint32_t *restrict row = NULL; uint32_t *restrict child_row = NULL; uint32_t total, delta; @@ -927,6 +925,10 @@ tsk_treeseq_genealogical_nearest_neighbours(const tsk_treeseq_t *self, ret = tsk_trace_error(TSK_ERR_BAD_PARAM_VALUE); goto out; } + parent = tsk_malloc(num_nodes * sizeof(*parent)); + length = tsk_calloc(num_focal, sizeof(*length)); + ref_count = tsk_calloc(((tsk_size_t) K) * num_nodes, sizeof(*ref_count)); + reference_set_map = tsk_malloc(num_nodes * sizeof(*reference_set_map)); if (parent == NULL || ref_count == NULL || reference_set_map == NULL || length == NULL) { ret = tsk_trace_error(TSK_ERR_NO_MEMORY); diff --git a/python/CHANGELOG.rst b/python/CHANGELOG.rst index 2af4a8654b..fbce38d205 100644 --- a/python/CHANGELOG.rst +++ b/python/CHANGELOG.rst @@ -22,6 +22,9 @@ - Update to kastore C version 2.1.3 to include file security fix (:user:`jeromekelleher`, :pr:`3478`). +- ``TableCollection.delete_older`` now checks table integrity first, raising an + error for invalid node or mutation references instead of reading out of bounds + (:user:`jeromekelleher`, :issue:`3486`). -------------------- [1.0.3] - 2026-05-14 diff --git a/python/tests/test_python_c.py b/python/tests/test_python_c.py index aba3bc54cb..75d52d571b 100644 --- a/python/tests/test_python_c.py +++ b/python/tests/test_python_c.py @@ -394,11 +394,20 @@ def test_link_ancestors_bad_args(self): def test_link_ancestors(self): ts = msprime.simulate(2, random_seed=1) tc = ts.dump_tables()._ll_tables - edges = tc.link_ancestors([0, 1], [3]) + edges = tc.link_ancestors([0, 1], [2]) assert isinstance(edges, _tskit.EdgeTable) del edges assert tc.edges.num_rows == 2 + @pytest.mark.parametrize( + ["samples", "ancestors"], [([0, 1], [3]), ([0, 3], [2]), ([0, 1], [-1])] + ) + def test_link_ancestors_bad_node(self, samples, ancestors): + ts = msprime.simulate(2, random_seed=1) + tc = ts.dump_tables()._ll_tables + with pytest.raises(_tskit.LibraryError, match="TSK_ERR_NODE_OUT_OF_BOUNDS"): + tc.link_ancestors(samples, ancestors) + def test_subset_bad_args(self): ts = msprime.simulate(10, random_seed=1) tc = ts.dump_tables()._ll_tables diff --git a/python/tests/test_table_transforms.py b/python/tests/test_table_transforms.py index a3e4939fc3..7a86d7ddc0 100644 --- a/python/tests/test_table_transforms.py +++ b/python/tests/test_table_transforms.py @@ -148,6 +148,36 @@ def test_t2(self, time): tables.delete_older(time) tables.assert_equals(before, ignore_provenance=True) + @pytest.mark.parametrize("bad_node", [5, 2**30]) + def test_bad_edge_parent(self, bad_node): + tables = self.tables() + parent = tables.edges.parent + parent[0] = bad_node + tables.edges.parent = parent + before = tables.copy() + with pytest.raises(tskit.LibraryError, match="TSK_ERR_NODE_OUT_OF_BOUNDS"): + tables.delete_older(1) + tables.assert_equals(before) + + @pytest.mark.parametrize("bad_node", [5, 2**30]) + def test_bad_mutation_node(self, bad_node): + tables = self.tables() + tables.sites.add_row(0.5, "A") + tables.mutations.add_row(site=0, node=bad_node, derived_state="T") + before = tables.copy() + with pytest.raises(tskit.LibraryError, match="TSK_ERR_NODE_OUT_OF_BOUNDS"): + tables.delete_older(1) + tables.assert_equals(before) + + def test_bad_mutation_parent(self): + tables = self.tables() + tables.sites.add_row(0.5, "A") + tables.mutations.add_row(site=0, node=0, derived_state="T", parent=2**30) + before = tables.copy() + with pytest.raises(tskit.LibraryError, match="TSK_ERR_MUTATION_OUT_OF_BOUNDS"): + tables.delete_older(1) + tables.assert_equals(before) + class TestDeleteOlderSimpleTreeMutationExamples: def test_single_mutation_no_time(self):