Skip to content

HSJ tip_labels row count is unvalidated at both Rcpp bridges #58

Description

@ms609

Found while fixing #51, by an independent review of that fix. Distinct defect, same assertion signature — which is the reason to file it rather than leave it: a future gcc-ASAN red carrying _Tp = int + const_reference will otherwise be misattributed to the site #51 closed.

The gap

score_hierarchy_block() and fitch_label_char() index

tip_labels[t * n_orig_chars + <char>]

for t over [0, tree.n_tip)src/ts_hsj.cpp:166, :176, :397, :425.

Both bridges validate tip_labels' values and column count, and neither validates its row count:

  • validate_hsj_tip_labels(), src/ts_rcpp.cpp:70-82 — values in [0, n_tokens) only.
  • the T-398 block in unpack_hsj(), src/ts_rcpp.cpp:1864-1890 — every block's primary/secondary index against tl.ncol().

ts_hsj_score() then takes n_tips from tip_labels_r.nrow() (src/ts_rcpp.cpp:3253) while tree is built from edge, so the two are never reconciled. A tip_labels with fewer rows than the tree has tips reads past the flattened vector.

This is exactly the threat model validate_hsj_tip_labels' own comment declares it covers (src/ts_rcpp.cpp:64-69): a direct TreeSearch::: call with a hand-crafted argument, plus hsjConfig via R/ts-driven-compat.R:198.

Reproduce

Needs the -D_GLIBCXX_ASSERTIONS build described in .AGENTS/memory/feature-inapplicable.md (flag in PKG_CPPFLAGS; ~/.R/Makevars.win zeroes PKG_CXXFLAGS). Verified against a build that already carries the #51 fix, so this is not that bug:

tip_labels dim: 6 3 ; tree tips: 6
full tip_labels score: 4
--- calling with a 4-row tip_labels and a 6-tip tree ---
bits/stl_vector.h:1149: std::vector<_Tp, _Alloc>::const_reference
std::vector<_Tp, _Alloc>::operator[](size_type) const [with _Tp = int; ...]:
Assertion '__n < this->size()' failed.

Script:

# Probe: does ts_hsj_score() guard tip_labels ROW count against the tree's
# tip count?  unpack_hsj()/validate_hsj_tip_labels() (src/ts_rcpp.cpp:70-82,
# :1864-1890) validate VALUES and COLUMNS but never ROWS, while
# score_hierarchy_block() (src/ts_hsj.cpp:495 ff.) loops t in [0, tree.n_tip)
# over `const std::vector<int>& tip_labels`.
lib <- commandArgs(TRUE)[1]
library(TreeSearch, lib.loc = lib)
suppressMessages(library(TreeTools, quietly = TRUE))
mat <- matrix(c(
  "0", "-", "-",
  "1", "0", "0",
  "1", "0", "1",
  "1", "1", "0",
  "1", "1", "1",
  "0", "-", "-"
), nrow = 6, byrow = TRUE, dimnames = list(paste0("t", 1:6), NULL))
ds <- phangorn::phyDat(mat, type = "USER", levels = c("-", "0", "1"),
                       ambiguity = "?")
tree <- Renumber(RenumberTips(ape::read.tree(
  text = "(((t1,t2),(t3,t4)),(t5,t6));"), names(ds)))
h <- CharacterHierarchy("1" = 2:3)
at <- attributes(ds)
tipData <- matrix(unlist(ds, use.names = FALSE), nrow = length(ds),
                  byrow = TRUE)
tl <- TreeSearch:::.BuildTipLabels(ds)
cat("tip_labels dim:", dim(tl), "; tree tips:", length(tree$tip.label), "\n")
ok <- TreeSearch:::ts_hsj_score(
  tree$edge, at$contrast, tipData,
  as.integer(TreeSearch:::.NonHierarchyWeights(ds, h)), at$levels,
  TreeSearch:::.HierarchyToBlocks(h), 1,
  tl, TreeSearch:::.HSJAbsentState(ds))
cat("full tip_labels score:", ok, "\n")
# Now truncate to 4 rows while the tree still has 6 tips.
cat("--- calling with a 4-row tip_labels and a 6-tip tree ---\n"); flush(stdout())
bad <- TreeSearch:::ts_hsj_score(
  tree$edge, at$contrast, tipData,
  as.integer(TreeSearch:::.NonHierarchyWeights(ds, h)), at$levels,
  TreeSearch:::.HierarchyToBlocks(h), 1,
  tl[1:4, , drop = FALSE], TreeSearch:::.HSJAbsentState(ds))
cat("NO ABORT; score =", bad, "\n")

Not reachable from the public API

Every public path builds tip_labels from the same dataset the tree's tips come from — R/tree_length.R:184, R/MaximizeParsimony.R:1537, and R/Resample.R:89 (which reuses the full matrix unchanged across replicates). So this is a guard-completeness issue at an internal boundary, not a user-facing crash. That is why it was left out of #51's fix rather than folded into it.

Suggested fix

Mirror the existing column check at both bridges: reject tl.nrow() that does not match the tip count the tree will have. Cheap, and it makes the two tip_labels bounds symmetric.

Metadata

Metadata

Assignees

No one assigned

    Labels

    area:10Red-team focus area 10red-teamFiled by the /red-team rotationsev:medP2: wrong on edge input / search quality

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions