From b18239f42f3f3cdd2e0bfeb3381a542ec6842fb2 Mon Sep 17 00:00:00 2001 From: asystemoffields Date: Sun, 18 Oct 2026 19:05:48 -0400 Subject: [PATCH 1/2] Reject sample and ancestor IDs equal to num_nodes in link_ancestors ancestor_mapper_init_samples and ancestor_mapper_init_ancestors checked IDs with `> num_rows` instead of `>= num_rows`, so an ID equal to the number of nodes passed validation and was then used to index is_sample, is_ancestor and the ancestor map arrays, all allocated with exactly num_nodes entries. A standalone call with such an ID segfaults. Use `>=`, add the one-past-the-end case to the C and Python error tests, and fix test_link_ancestors in test_python_c.py, which asked for ancestor 3 of a tree sequence with nodes 0-2 and only passed because of the bug. Fixes #3484 Co-Authored-By: Claude Fable 5.1 --- c/CHANGELOG.rst | 5 +++++ c/tests/test_tables.c | 13 +++++++++++++ c/tskit/tables.c | 5 +++-- python/CHANGELOG.rst | 3 +++ python/tests/test_python_c.py | 7 ++++++- 5 files changed, 30 insertions(+), 3 deletions(-) diff --git a/c/CHANGELOG.rst b/c/CHANGELOG.rst index e75b9e2bf1..27e5f431bf 100644 --- a/c/CHANGELOG.rst +++ b/c/CHANGELOG.rst @@ -10,6 +10,11 @@ In development - Update to kastore 2.1.3 to include file security fix (:user:`jeromekelleher`, :pr:`3478`). +- Fix ``tsk_table_collection_link_ancestors`` accepting a sample or ancestor ID + equal to the number of nodes, which indexed node-sized arrays one past their + end. Such IDs now return ``TSK_ERR_NODE_OUT_OF_BOUNDS``. + (:user:`asystemoffields`, :issue:`3484`, :pr:`TBD`) + -------------------- [1.3.1] - 2026-03-06 -------------------- diff --git a/c/tests/test_tables.c b/c/tests/test_tables.c index 7fca96563a..013c1d3eb1 100644 --- a/c/tests/test_tables.c +++ b/c/tests/test_tables.c @@ -7625,6 +7625,19 @@ test_link_ancestors_input_errors(void) &tables, samples, 2, ancestors, 2, 0, &result); CU_ASSERT_EQUAL_FATAL(ret, TSK_ERR_NODE_OUT_OF_BOUNDS); + /* Sample and ancestor IDs one past the last node */ + ancestors[0] = 4; + 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); + + samples[0] = 0; + 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; diff --git a/c/tskit/tables.c b/c/tskit/tables.c index d673619141..c6db8e17a0 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; } diff --git a/python/CHANGELOG.rst b/python/CHANGELOG.rst index 2af4a8654b..3ebaaec0fa 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`). +- ``link_ancestors`` now raises ``LibraryError`` (node out of bounds) for a sample or + ancestor ID equal to the number of nodes, instead of reading and writing past the + end of its node arrays. (:user:`asystemoffields`, :issue:`3484`, :pr:`TBD`) -------------------- [1.0.3] - 2026-05-14 diff --git a/python/tests/test_python_c.py b/python/tests/test_python_c.py index aba3bc54cb..c594c7a38f 100644 --- a/python/tests/test_python_c.py +++ b/python/tests/test_python_c.py @@ -390,11 +390,16 @@ def test_link_ancestors_bad_args(self): tc.link_ancestors(samples=[0, 1], ancestors=[11, -1]) with pytest.raises(_tskit.LibraryError): tc.link_ancestors(samples=[0, -1], ancestors=[11]) + # One past the last node ID is out of bounds too + with pytest.raises(_tskit.LibraryError, match="out of bounds"): + tc.link_ancestors(samples=[0, tc.nodes.num_rows], ancestors=[11]) + with pytest.raises(_tskit.LibraryError, match="out of bounds"): + tc.link_ancestors(samples=[0, 1], ancestors=[tc.nodes.num_rows]) 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 From 4a5f92f1c8dd3bf5c3acbc33b87b89eb79c89521 Mon Sep 17 00:00:00 2001 From: asystemoffields Date: Sun, 18 Oct 2026 19:12:27 -0400 Subject: [PATCH 2/2] Fill in the PR number in the changelogs Co-Authored-By: Claude Fable 5.1 --- c/CHANGELOG.rst | 2 +- python/CHANGELOG.rst | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/c/CHANGELOG.rst b/c/CHANGELOG.rst index 27e5f431bf..70ead6e4ea 100644 --- a/c/CHANGELOG.rst +++ b/c/CHANGELOG.rst @@ -13,7 +13,7 @@ In development - Fix ``tsk_table_collection_link_ancestors`` accepting a sample or ancestor ID equal to the number of nodes, which indexed node-sized arrays one past their end. Such IDs now return ``TSK_ERR_NODE_OUT_OF_BOUNDS``. - (:user:`asystemoffields`, :issue:`3484`, :pr:`TBD`) + (:user:`asystemoffields`, :issue:`3484`, :pr:`3492`) -------------------- [1.3.1] - 2026-03-06 diff --git a/python/CHANGELOG.rst b/python/CHANGELOG.rst index 3ebaaec0fa..887360c8c5 100644 --- a/python/CHANGELOG.rst +++ b/python/CHANGELOG.rst @@ -24,7 +24,7 @@ :pr:`3478`). - ``link_ancestors`` now raises ``LibraryError`` (node out of bounds) for a sample or ancestor ID equal to the number of nodes, instead of reading and writing past the - end of its node arrays. (:user:`asystemoffields`, :issue:`3484`, :pr:`TBD`) + end of its node arrays. (:user:`asystemoffields`, :issue:`3484`, :pr:`3492`) -------------------- [1.0.3] - 2026-05-14