From ca5c1c0e776e92206c2ab3b7b4d5e3beeed47653 Mon Sep 17 00:00:00 2001 From: R script <1695515+ms609@users.noreply.github.com> Date: Wed, 5 Aug 2026 04:06:43 +0100 Subject: [PATCH 1/4] fix(hsj): skip the childless case before pointing into co.kids fitch_label_char()'s uppass loop formed `&co.kids[co.kidOff[node]]` before testing `nk`, the node's child count. CanonOrder stores children CSR-style, so `kidOff[n]` for a childless node is whatever `kids.size()` happened to be when the DFS popped it -- and for the LAST node popped that is the final size, every other node having already contributed its children by then. `co` arrives as a const reference, so this is `std::vector::operator[](size()) const`: a dereference of one past the end. The downpass and the tie-break accumulation loops above both already `continue` on `nk == 0`; this loop did not. Reproduced against `-D_GLIBCXX_ASSERTIONS` (flag in PKG_CPPFLAGS, since ~/.R/Makevars.win zeroes PKG_CXXFLAGS; 34 hits in the build log). Pre-fix, four test files abort on entry to their first HSJ block with `Assertion '__n < this->size()' failed`; post-fix all four run clean: test-tree_length.R abort -> 77 passed test-ts-xform.R abort -> 138 passed test-ts-hsj.R abort -> 147 passed test-ts-resample-hierarchy.R abort -> 74 passed The reporter's second reproducer (test-ts-xform.R) is the same defect, not a second one: it aborts inside the third test, which is the file's first `inapplicable = "hsj"` search -- hence exactly five assertions first, from the two preceding pure-xform tests. `kid` is never dereferenced when `nk == 0`, so no value was read through the bad reference and no score moves: 900 HSJ and x-transformation lengths over random matrices (4-16 tips, alpha 0/0.5/1), the issue's own data, and three seeded end-to-end searches are bit-identical either side of the fix, Inf entries included. Fixes #51 Co-Authored-By: Claude Opus 5 --- NEWS.md | 9 +++++++++ src/ts_hsj.cpp | 7 +++++++ 2 files changed, 16 insertions(+) diff --git a/NEWS.md b/NEWS.md index e104fe8c3..ac2b16deb 100644 --- a/NEWS.md +++ b/NEWS.md @@ -97,6 +97,15 @@ 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 the + last node its traversal visited 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 a hardened or instrumented build aborted + on it, which is what made the `AddressSanitizer` workflow unusable. + - `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/src/ts_hsj.cpp b/src/ts_hsj.cpp index 9c68a193f..d2f0b786e 100644 --- a/src/ts_hsj.cpp +++ b/src/ts_hsj.cpp @@ -299,6 +299,13 @@ 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 `kidOff` for the last + // node DFS popped equals co.kids.size() (every other node has already + // contributed its children by then), so forming `&co.kids[kidOff[node]]` + // for such a node dereferences one past the end -- the OOB read + // -D_GLIBCXX_ASSERTIONS aborts on. The two loops above already skip on + // nk == 0; this one did not (#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. From 5928f7f317421abe3adf7cd2262aaaf4bb7ed2e9 Mon Sep 17 00:00:00 2001 From: R script <1695515+ms609@users.noreply.github.com> Date: Wed, 5 Aug 2026 04:20:54 +0100 Subject: [PATCH 2/4] docs: record the local assertions-build route for HSJ/XFORM bounds bugs Issue agent-issues/TreeSearch#51 asked for this: the memory note it was filed against points at the ASan workflow as the route to a container-OOB, and that workflow was red on trunk for the very defect the previous commit fixes. A local -D_GLIBCXX_ASSERTIONS build reproduces the same class in seconds on Windows and can be aimed at one test file, so it belongs in the subsystem's own memory file, next to the flat-vector layouts that make this the recurring failure mode here. Also records the two ways to misread its output: the abort names the container type, never the call site; and `lib.loc` must be an absolute Windows path, or test_file()'s chdir breaks the lazy-load DB and fakes several regressions. Qualifies the in-source issue reference per AGENTS.md, since src/ fast-forwards to the public upstream, where a bare `#51` resolves to an unrelated issue. Co-Authored-By: Claude Opus 5 --- .AGENTS/memory/feature-inapplicable.md | 38 ++++++++++++++++++++++++++ src/ts_hsj.cpp | 2 +- 2 files changed, 39 insertions(+), 1 deletion(-) 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/src/ts_hsj.cpp b/src/ts_hsj.cpp index d2f0b786e..1f4d38d0c 100644 --- a/src/ts_hsj.cpp +++ b/src/ts_hsj.cpp @@ -304,7 +304,7 @@ static int fitch_label_char( // contributed its children by then), so forming `&co.kids[kidOff[node]]` // for such a node dereferences one past the end -- the OOB read // -D_GLIBCXX_ASSERTIONS aborts on. The two loops above already skip on - // nk == 0; this one did not (#51). + // 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 From 38ded4dd82a4b06b4d21d3b4f8c6854ad7f3e21f Mon Sep 17 00:00:00 2001 From: R script <1695515+ms609@users.noreply.github.com> Date: Wed, 5 Aug 2026 04:26:56 +0100 Subject: [PATCH 3/4] review: correct the mechanism claim; land the CanonOrder evidence script Independent review of ca5c1c0e7 disputed "the last node DFS popped". It is not one node but a run of them: every childless node reached after the final push_back carries the end offset. Measured on an R mirror of build_canon_order() over 900 random trees, 2-24 tips: trees with NO kidOff==size node: 0 trees with >1 such node : 843 kidOff/kidNum CSR consistency : OK The first line matters most -- the pre-fix code formed a reference to co.kids.end() on EVERY HSJ scoring call, not on some unlucky shape. The third rules out the alternative reading that the guard papers over a corrupt CSR: kids[off + 1 .. off + num] is exactly each node's canonical children, so kidOff/kidNum are sound and only the missing nk == 0 test was wrong. Comment and NEWS reworded accordingly; the script lands under dev/red-team/reviews/ as the standing evidence. The same review found a SEPARATE unguarded bound -- tip_labels' row count is validated at neither Rcpp bridge, giving an identical `_Tp = int` const-operator[] abort from a hand-crafted TreeSearch::: call. Confirmed against a build already carrying this fix, so it is not the same defect, and it is unreachable from the public API. Filed as agent-issues/TreeSearch#58 rather than widened into this branch. Co-Authored-By: Claude Opus 5 --- NEWS.md | 5 +- .../repro-02-canon-order-invariant.R | 75 +++++++++++++++++++ src/ts_hsj.cpp | 10 ++- 3 files changed, 84 insertions(+), 6 deletions(-) create mode 100644 dev/red-team/reviews/feature-hsj-oob-read/repro-02-canon-order-invariant.R diff --git a/NEWS.md b/NEWS.md index ac2b16deb..6aee4916b 100644 --- a/NEWS.md +++ b/NEWS.md @@ -99,8 +99,9 @@ - `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 the - last node its traversal visited that pointer addressed one past the end. No + 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 a hardened or instrumented build aborted 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 1f4d38d0c..f26b652a6 100644 --- a/src/ts_hsj.cpp +++ b/src/ts_hsj.cpp @@ -299,10 +299,12 @@ 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 `kidOff` for the last - // node DFS popped equals co.kids.size() (every other node has already - // contributed its children by then), so forming `&co.kids[kidOff[node]]` - // for such a node dereferences one past the end -- the OOB read + // 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; From c054e3fbfa4cb9c058010f22954009cffc5f99ce Mon Sep 17 00:00:00 2001 From: R script <1695515+ms609@users.noreply.github.com> Date: Wed, 5 Aug 2026 04:35:16 +0100 Subject: [PATCH 4/4] docs(news): name the mechanism that aborted, not the workflow that stopped Review read "which is what made the AddressSanitizer workflow unusable" as crediting ASan's own instrumentation, and objected -- correctly -- that ASan watches accesses, not address arithmetic, and that co.kids.reserve(n_node) leaves the offending address inside the live allocation anyway. The claim was about the right defect but named the wrong instrument. What aborted the gcc-ASAN job is visible in agent-issues/TreeSearch#51's own evidence: `stl_vector.h:1282 ... Assertion '__n < this->size()' failed`, a libstdc++ precondition check, not a sanitizer report. The r-hub gcc-asan container compiles with the hardened library; the flag is not in ASan.yml, which is why grepping .github/workflows/ for it finds nothing and misleads. Reworded to say what actually fires and why that workflow could not get past this package. Also distinct from the EARLIER ASan unusability that PRs #262/#275 fixed -- that was the runner configuration; this is a defect the now-working workflow immediately hit. Co-Authored-By: Claude Opus 5 --- NEWS.md | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/NEWS.md b/NEWS.md index 6aee4916b..ceb1a03b5 100644 --- a/NEWS.md +++ b/NEWS.md @@ -104,8 +104,11 @@ 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 a hardened or instrumented build aborted - on it, which is what made the `AddressSanitizer` workflow unusable. + 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