fix: OOB read in HSJ token_states/CanonOrder lookup

.AGENTS/memory/feature-inapplicable.md

@@ -94,3 +94,41 @@ secondaries supported (state count = ∏k_i + 1). Nested hierarchies deferred.
 Integration complete: `ScoringMode::XFORM` in `score_tree()` dispatches
 Fitch(non-hierarchy) + Sankoff(recoded). `MaximizeParsimony()` accepts
 `inapplicable = "xform"`. End-to-end search verified.
+
+---
+
+## Memory-safety checking: reach for `-D_GLIBCXX_ASSERTIONS` before ASan
+
+The HSJ/XFORM kernels index a lot of flat `std::vector` scratch (`tip_labels`
+row-major over `n_orig_chars`, `sec_states` over `m * n_node`, CanonOrder's
+CSR `kids`/`kidOff`/`kidNum`), so container-bounds bugs are this subsystem's
+recurring failure mode. libstdc++ hardened mode catches them locally on
+Windows in seconds, where ASan needs a Linux container round-trip:
+
+```bash
+# The flag MUST go in PKG_CPPFLAGS: ~/.R/Makevars.win zeroes PKG_CXXFLAGS.
+TMPBUILD=$(mktemp -d)
+(cd "$TMPBUILD" && R CMD build --no-build-vignettes --no-manual --no-resave-data <src>)
+PKG_CPPFLAGS="-D_GLIBCXX_ASSERTIONS" \
+  R CMD INSTALL --library=.agent-<id> --preclean "$TMPBUILD"/TreeSearch_*.tar.gz
+# Confirm the flag took: grep -c _GLIBCXX_ASSERTIONS <build log>  -->  expect ~34
+NOT_CRAN=true Rscript -e "library(TreeSearch, lib.loc='<ABSOLUTE Windows path>');
+  testthat::test_file('tests/testthat/test-ts-hsj.R', reporter='summary')"
+```
+
+Three things to know when reading the result:
+
+- A failure aborts the process printing `stl_vector.h:<n>: ... Assertion
+  '__n < this->size()' failed`, naming the **container type only** — not the
+  call site. `_Tp = int` plus a `const_reference` return narrows it to a read
+  through a const `std::vector<int>`. Bisect by guarding candidate sites.
+- `lib.loc` must be an absolute *Windows* path. A relative one makes
+  `test_file()` (which chdirs to `tests/testthat/`) fail to find the lazy-load
+  DB, which looks like several real regressions.
+- It only instruments `operator[]` on libstdc++ containers, so raw-pointer
+  arithmetic off `.data()` still needs ASan.
+
+Always run the same file against a pristine-trunk build too, and treat only a
+*difference* as signal: `test-CharacterHierarchy.R` reports 5 errors under
+`library()` + `test_file()` either way, because it calls internals unqualified
+and only `R CMD check`'s namespace environment can see them.

NEWS.md

@@ -97,6 +97,19 @@
   ordinary Fitch data and now collapse like any other.  A replicate that
   retains any hierarchy block is unaffected.
 
+- `inapplicable = "hsj"` scoring no longer forms a reference one element past
+  the end of an internal vector.  The secondary-labelling uppass computed a
+  pointer to a node's children before testing whether it had any, and for a
+  childless node reached after the traversal had emitted its last child that
+  pointer addressed one past the end.  No
+  value was ever read through it and no score changed -- 900 of 900 HSJ and
+  x-transformation lengths are bit-identical either side of the fix -- but the
+  access is undefined behaviour, and any build whose standard library checks
+  its own preconditions aborted on it.  That includes the container behind the
+  `gcc-ASAN` workflow, which is why that workflow could not get past this
+  package: it stopped on the library assertion rather than on anything the
+  sanitizer itself had found.
+
 - `MaximizeParsimony(effort = )` replaces `strategy = `, which is removed (it
   was never released).  `effort` is a **relative** offset, not an absolute
   level: `0` (the default) accepts the amount of search the dataset's size and

dev/red-team/reviews/feature-hsj-oob-read/repro-02-canon-order-invariant.R

@@ -0,0 +1,75 @@
+# Mirror of ts_hsj.cpp build_canon_order() (src/ts_hsj.cpp:58-110) in R.
+# Evidence for agent-issues/TreeSearch#51.  Establishes three things about
+# the CSR children arrays, over 900 random trees of 2-24 tips:
+#
+#  1. EVERY tree has at least one node with kidOff[node] == length(kids), so
+#     the `if (nk == 0) continue` guard in fitch_label_char()'s uppass is
+#     always load-bearing -- the pre-fix code formed a reference to
+#     co.kids.end() on every single HSJ scoring call.
+#  2. Usually SEVERAL nodes do (843 of 900), not just the last one popped:
+#     any childless node reached after the final push_back carries the end
+#     offset.  The last popped node is always among them.
+#  3. kidOff/kidNum are otherwise CONSISTENT -- kids[off + 1 .. off + num]
+#     is exactly node n's canonical children for every node with children.
+#     So the guard is a bounds fix, not a patch over a corrupt CSR.
+#
+# Every such node has kidNum == 0, which is why skipping them changes no
+# score: the loop body the guard bypasses is zero-trip anyway.
+#
+# Pure R; needs no TreeSearch build.  Run:  Rscript <this file>
+suppressMessages(library("ape"))
+set.seed(1)
+
+canon <- function(edge, nTip) {
+  nNode <- max(edge)                       # 1-based node count
+  adj <- vector("list", nNode)
+  for (i in seq_len(nrow(edge))) {
+    adj[[edge[i, 1]]] <- c(adj[[edge[i, 1]]], edge[i, 2])
+    adj[[edge[i, 2]]] <- c(adj[[edge[i, 2]]], edge[i, 1])
+  }
+  # C++ indices are 0-based with tips first; ape's are already tips-first,
+  # so sorting ascending on ape's numbering matches sorting on 0-based.
+  adj <- lapply(adj, sort)
+  kidOff <- integer(nNode); kidNum <- integer(nNode)
+  kids <- integer(0); pre <- integer(0)
+  seen <- logical(nNode); stack <- 1L; seen[1] <- TRUE   # start at tip 0
+  while (length(stack)) {
+    n <- stack[length(stack)]; stack <- stack[-length(stack)]
+    pre <- c(pre, n)
+    kidOff[n] <- length(kids)                            # 0-based offset
+    for (nb in adj[[n]]) {
+      if (seen[nb]) next
+      seen[nb] <- TRUE
+      kids <- c(kids, nb); kidNum[n] <- kidNum[n] + 1L
+      stack <- c(stack, nb)
+    }
+  }
+  list(pre = pre, kids = kids, kidOff = kidOff, kidNum = kidNum,
+       nNode = nNode, nVisited = length(pre))
+}
+
+bad <- 0L; multi <- 0L; unreached <- 0L
+for (nTip in 2:24) for (rep in 1:40) {
+  tr <- if (nTip == 2) structure(list(edge = matrix(c(3L,1L,3L,2L), 2, 2,
+                                                    byrow = TRUE),
+                                      tip.label = c("a","b"), Nnode = 1L),
+                                 class = "phylo") else rtree(nTip)
+  co <- canon(tr$edge, nTip)
+  if (co$nVisited != co$nNode) unreached <- unreached + 1L
+  oob <- which(co$kidOff == length(co$kids))
+  if (length(oob) == 0) bad <- bad + 1L
+  if (length(oob) > 1) multi <- multi + 1L
+  stopifnot(all(co$kidNum[oob] == 0L))                  # OOB node is childless
+  stopifnot(co$pre[length(co$pre)] %in% oob)             # last popped is one
+  # kidOff/kidNum consistency: children of n are exactly kids[off+1 .. off+num]
+  for (n in seq_len(co$nNode)) if (co$kidNum[n] > 0) {
+    got <- co$kids[co$kidOff[n] + seq_len(co$kidNum[n])]
+    par <- tr$edge[tr$edge[, 2] == n, 1]
+    nbs <- sort(setdiff(c(tr$edge[tr$edge[,1]==n,2], par), integer(0)))
+    stopifnot(setequal(got, setdiff(nbs, co$pre[seq_len(which(co$pre==n))])))
+  }
+}
+cat(sprintf("trees with NO kidOff==size node: %d\n", bad))
+cat(sprintf("trees with >1 such node        : %d\n", multi))
+cat(sprintf("trees with unreached nodes     : %d\n", unreached))
+cat("kidOff/kidNum CSR consistency: OK\n")

src/ts_hsj.cpp

@@ -299,6 +299,15 @@ static int fitch_label_char(
   for (int i = static_cast<int>(co.post.size()) - 1; i >= 0; --i) {
     int node = co.post[i];
     int nk = co.kidNum[node];
+    // A canonical leaf has no children to resolve, and forming
+    // `&co.kids[co.kidOff[node]]` for one can dereference co.kids.end():
+    // kidOff is written as the CURRENT kids.size() when the DFS pops a node,
+    // so every childless node popped after the final push_back carries the
+    // end offset -- always the last node popped, and usually several more
+    // (>1 in 843 of 900 random 2-24 tip trees).  That is the OOB read
+    // -D_GLIBCXX_ASSERTIONS aborts on.  The two loops above already skip on
+    // nk == 0; this one did not (agent-issues/TreeSearch#51).
+    if (nk == 0) continue;
     const int* kid = &co.kids[co.kidOff[node]];
     // Resolve each child: prefer parent's (already-resolved) state if it lies
     // in the child's set (DELTRAN-style); otherwise pick order-invariantly.