diff --git a/.AGENTS/memory/feature-inapplicable.md b/.AGENTS/memory/feature-inapplicable.md index 7c776b65c..511ba168b 100644 --- a/.AGENTS/memory/feature-inapplicable.md +++ b/.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 ) +PKG_CPPFLAGS="-D_GLIBCXX_ASSERTIONS" \ + R CMD INSTALL --library=.agent- --preclean "$TMPBUILD"/TreeSearch_*.tar.gz +# Confirm the flag took: grep -c _GLIBCXX_ASSERTIONS --> expect ~34 +NOT_CRAN=true Rscript -e "library(TreeSearch, lib.loc=''); + 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:: ... 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`. 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. diff --git a/NEWS.md b/NEWS.md index e104fe8c3..ceb1a03b5 100644 --- a/NEWS.md +++ b/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 diff --git a/dev/red-team/reviews/feature-hsj-oob-read/repro-02-canon-order-invariant.R b/dev/red-team/reviews/feature-hsj-oob-read/repro-02-canon-order-invariant.R new file mode 100644 index 000000000..632ca2b23 --- /dev/null +++ b/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 +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") diff --git a/src/ts_hsj.cpp b/src/ts_hsj.cpp index 9c68a193f..f26b652a6 100644 --- a/src/ts_hsj.cpp +++ b/src/ts_hsj.cpp @@ -299,6 +299,15 @@ static int fitch_label_char( for (int i = static_cast(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.