T-328/T-323: guard Wagner Rcpp-boundary indices (tip_data values, addition_order)#261
Merged
Merged
Conversation
…y (T-328, T-323) simplify_patterns() indexed token_states[tip_data-1] with no range check, so an out-of-range tip_data value from a hand-crafted matrix passed to the unexported ts_wagner_tree()/ts_random_wagner_tree()/ts_resample_search()/ ts_parallel_resample()/ts_successive_approx()/ts_simplify_diag() entry points read out of bounds (0 -> before the array; > n_tokens -> past the end, likely a segfault). Add a shared validate_tip_data_values() check at the single Rcpp boundary and its four duplicated call sites (Rcpp::stop on violation), and give ts_simplify_diag its first weight/levels length guards while there. Bundled with the sibling T-323 gap: ts_wagner_tree's addition_order was converted 1-based -> 0-based with no length/range/duplicate check, so a short vector read past its end (reproduced segfault) and an out-of-range or duplicated value corrupted the tree via OOB heap writes. Add validate_addition_order() mirroring the existing weight/levels/min_steps guards. Only reachable via TreeSearch:::, not the public API (AdditionTree()/ MaximizeParsimony() always build valid inputs from a checked phyDat).
4f8c5bd to
87f0a16
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
simplify_patternsindexedtoken_states[tip_data - 1]with no range check, so an out-of-rangetip_datavalue passed to the unexported Wagner entry points (ts_wagner_tree,ts_random_wagner_tree,ts_resample_search,ts_parallel_resample,ts_successive_approx,ts_simplify_diag) read out of bounds (0 → before the array; > n_tokens → past the end, likely a segfault).ts_wagner_treeconvertedaddition_order1-based → 0-based with no length/range/duplicate check — a short vector read past its end (reproduced segfault), and an out-of-range or duplicated value corrupted the tree via OOB heap writes.validate_tip_data_values,validate_addition_order) insrc/ts_rcpp.cpp, called from every affected entry point.ts_simplify_diagalso gained its firstweight/levelslength guards (it had none) while touching that function.AdditionTree()/MaximizeParsimony()API, which always buildstip_data/order from a validatedphyDat— only reachable viaTreeSearch:::with a hand-crafted matrix/vector.Test plan
Rscript dev/build-fast.R— clean buildtests/testthat/test-ts-wagner.R— 57/57 pass locally (10 new tests coveringtip_datavalue 0/negative/>n_tokens andaddition_orderwrong-length/out-of-range/zero/duplicate, each asserting a cleanRcpp::stopinstead of a crash, across all 6 affected entry points)covrcheck on every new/changed line insrc/ts_rcpp.cpp— 100% coveredgcc-ASAN) — triggers automatically on this PR (src/**,tests/testthat/**.Rpaths); local ASan is blocked on this machine, so this is the authoritative confirmation that the fixed OOB reads no longer fire