Skip to content

Fix two silent memory-safety bugs in recursivecopy!/copyat_or_push! - #667

Draft
ChrisRackauckas-Claude wants to merge 5 commits into
SciML:masterfrom
ChrisRackauckas-Claude:fix-recursivecopy-bounds
Draft

ChrisRackauckas-Claude wants to merge 5 commits into
SciML:masterfrom
ChrisRackauckas-Claude:fix-recursivecopy-bounds

Conversation

@ChrisRackauckas-Claude

@ChrisRackauckas-Claude ChrisRackauckas-Claude commented Oct 8, 2026 •

Copy link
Copy Markdown
Member

What changed and why

Two utils.jl helpers do their own bounds-adjacent logic and both get it wrong, causing silent memory corruption / crashes instead of a catchable error:

  • recursivecopy!(b::AbstractArray{<:StaticArray}, a::AbstractArray{<:StaticArray}) (utils.jl:89) iterates eachindex(a) instead of eachindex(b, a), so when b is shorter than a it writes b[i] out of bounds for the trailing indices instead of throwing.
  • copyat_or_push!(a, i, x) (utils.jl:370) only checked length(a) >= i before entering an @inbounds block. For i below a's own first index, that condition can be trivially true, so the function takes the in-place-update branch and indexes under @inbounds with no check at all.

Fix: recursivecopy! now loops over eachindex(b, a), which throws DimensionMismatch on a length mismatch (the same idiom recursivecopy! already uses a few methods down for the AbstractArray/AbstractVectorOfArray case). copyat_or_push! now rejects i < firstindex(a) with a BoundsError before the @inbounds block, leaving the append-on-overflow behavior (i > length(a)) untouched.

Update (round 2, addressing Grok 4.7's review): round 1 shipped the copyat_or_push! guard as i >= 1, with the PR text claiming the literal 1 "avoids a behavior change" and that firstindex "would introduce one." That claim was backwards. copyat_or_push! is exported and its docstring promises to copy into a[i] "when that element exists" — for any AbstractVector whose firstindex isn't 1 (e.g. an OffsetArray with indices 0:1), index 0 exists and master correctly updates it; the i >= 1 guard threw BoundsError there instead, which is the behavior change the PR claimed to avoid. Fixed by using i >= firstindex(a), which is false only when i is below the array's own first valid index — correct for both a plain 1-based Vector (unchanged from round 1) and a 0-based array (now matches master).

Before/after

Both bugs are severe enough that "before" isn't a wrong number, it's undefined behavior — a silent out-of-bounds write that may corrupt memory or crash depending on heap layout (observed as a reliable segfault for recursivecopy! and for copyat_or_push!(vals, -1, ...); copyat_or_push!(vals, 0, ...) specifically did not reliably reproduce as a crash in round-2's independent testing, though it is still an unchecked out-of-bounds write under @inbounds):

julia> b = [MVector{2,Float64}(0,0)]; a = [MVector{2,Float64}(1,1), MVector{2,Float64}(2,2)];
julia> recursivecopy!(b, a)
# segfaults on Julia 1.12.7 (immediately): arg_tuple / jl_method_error, signal 11
# on Julia 1.10.12: returns silently with b[2] written out of bounds (no crash this run, but UB)

julia> vals = [[1, 2], [3, 4]];
julia> copyat_or_push!(vals, -1, [9, 9])
# segfaults on both 1.10.12 and 1.12.7 — backtrace shows the crash inside
# copyat_or_push! at utils.jl:370/376 (length() called on corrupted memory)

After (this branch):

julia> recursivecopy!(b, a)
ERROR: DimensionMismatch: all inputs to eachindex must have the same indices, got Base.OneTo(1) and Base.OneTo(2)

julia> copyat_or_push!(vals, -1, [9, 9])
ERROR: BoundsError: attempt to access 2-element Vector{Vector{Int64}} at index [-1]

Normal usage (matching lengths, in-bounds i) is unaffected — verified with the existing recursivecopy!/copyat_or_push! call sites in the test suite plus the new tests below, including the round-2 0-based-array case:

z = ZeroVec([[1, 2], [3, 4]])   # a minimal 0-based AbstractVector test type
copyat_or_push!(z, 0, [9, 9])
z[0] == [9, 9]   # updates correctly, matching master — round-1 threw BoundsError here

Failing-before / passing-after

Ran test/Core/utils_test.jl directly (includes the regression tests) against unmodified src/utils.jl:

=== UNFIXED on Julia 1.12.7 ===
[...]
Allocations: 12802030 (Pool: 12801846; Big: 184); GC: 8
# process segfaults (signal 11) partway through the test file

Round-2 regression — against the round-1 i >= 1 code (commit 143d6c5), kept the Vector tests and added only the ZeroVec case:

copyat_or_push! rejects indices below firstindex: Error During Test
  Got exception outside of a @test
  BoundsError: attempt to access 2-element ZeroVec{Vector{Int64}} with indices 0:1 at index [0]
Test Summary:                                    | Pass  Error  Total  Time
copyat_or_push! rejects indices below firstindex |    5      1      6  1.7s

After this fix (commit c7b71d3): test/Core/utils_test.jl runs to completion with no failures, on both Julia 1.10.12 and 1.12.7 — all 8/8 in the copyat_or_push! testset, plus everything else in the file.

Full GROUP=Core run (both Julia versions): exit 0, all testsets green, including Utils Tests 120/120. GROUP=QA on 1.12.7: 20/20 pass. GROUP=QA on 1.10.12 fails on a pre-existing Aqua method-ambiguity report (similar(::AbstractVectorOfArray, ::Type{T}, dims::Tuple{...}) vs Base.similar, in vector_of_array.jl, untouched by this PR) — confirmed by reproducing the identical failure on unmodified master with the same Julia 1.10.12/Manifest, so it is not caused by this change.

What was not verified

  • Downstream, GPU, NoPre and AD test groups were not run (out of scope for a utils.jl bounds-check fix; nothing in those groups calls these two functions with bounds-adjacent arguments, and OrdinaryDiffEq's own copyat_or_push! call sites pass saveiter or the literals 1/2, never a non-1-based array).
  • The pre-existing GROUP=QA ambiguity failure on Julia 1.10 was reproduced but not investigated/fixed — it predates this PR and is unrelated to utils.jl.
  • Round 2's independent review flagged that recursivecopy!'s eachindex(b, a) fix is itself a (correct, intentional) behavior change for a longer destination — master's recursivecopy! silently copies only the overlapping prefix and leaves the destination's tail untouched for a longer b, matching the nested-array method's existing use of the same idiom, while this PR's eachindex(b, a) now throws DimensionMismatch for a longer destination too (previously only a shorter destination triggered the bug this PR targets). The reviewer rated this a non-blocking match with the documented, axis-matching contract (recursivecopyto! is the function for intentionally different shapes) and said no code change is required; flagging it here for visibility since the original PR text undersold it as having "no change to any code path that was previously well-defined."
  • Grok 4.7's review also noted length(a) >= i (the pre-existing upper-bound check, unchanged by either round of this PR) is not equivalent to i <= lastindex(a) for a non-1-based array with a gap (e.g. indices 5:7, i=2), which is a pre-existing, separate issue outside the scope of this fix.

Anything a reviewer should push back on

  • firstindex(a) relies on axes/Base's generic firstindex fallback for whatever AbstractVector is passed; this is correct for every case tested (Vector, the new ZeroVec, and — per the round-2 reviewer's manual check — OffsetArray), but I have not exhaustively audited every AbstractVector subtype in the ecosystem for a non-standard firstindex.

Please ignore until reviewed by @ChrisRackauckas.

Risk assessment

  • Risk: low
  • Blast radius: src/utils.jl only, two internal/exported helper functions (recursivecopy!'s StaticArray-eltype method, copyat_or_push!). Both functions already throw for the cases this PR targets would otherwise crash; the round-2 fix specifically restores the exact same in-bounds behavior master has for non-1-based arrays, which round 1 had incorrectly broken. No public API signature change.
  • Evidence: failing-before (crash/UB) / passing-after (clean DimensionMismatch/BoundsError) shown above; round-1-vs-round-2 regression test shown failing/passing; GROUP=Core full pass on Julia 1.10 and 1.12; GROUP=QA pass on 1.12, pre-existing unrelated failure reproduced on 1.10 master.
  • Independent review: Grok 4.7 reviewed round 1 and returned CHANGES (confirmed-by-running OffsetArray regression, plus non-blocking notes on the recursivecopy! longer-destination behavior and the pre-existing length(a) >= i vs lastindex gap). The blocking finding is addressed in round 2 (commit c7b71d3); round-2 independent review still pending.
  • Merge: needs human review — a memory-safety fix that already had one round of review findings; wants a second look before merge rather than auto-merging.

🤖 Generated with Claude Code

https://claude.ai/code/session_01LPHREnnonfLg1VcE1EJovv

Review findings addressed

This round answers two reviews: the PR comment (Devin, fusion-claude-opus-5-5-high-sidekick-swe-2-medium) on c7b71d3 and a second review of dd83070 that returned CHANGES.

  1. copyat_or_push! used length(a) >= i to choose between updating and appending (blocking in both reviews). Fixed in 5e5c59c. The test is now i <= lastindex(a), evaluated outside @inbounds; @inbounds covers only the read and write of a[i], which both bounds checks have already validated. On a 0-based vector, i == length(a) now appends; before, it updated the element one past the end. On a vector with axes 2:3, index 3 is now updated in place; before, it was appended. A plain 1-based Vector is unaffected, since lastindex == length there. The docstring now says an index of a is updated, i > lastindex(a) appends, and i < firstindex(a) throws BoundsError.
  2. Regression tests for item 1. The ZeroVec test type became OffsetVec(data, first). It takes any first index and supports push!. The copyat_or_push! testset now also covers appending at i == length on a 0-based vector, updating index 3 of a vector with axes 2:3, a perform_copy = false update, appending past lastindex, and BoundsError below firstindex.
  3. Test for a longer recursivecopy! destination. Added. A longer destination of MVectors throws DimensionMismatch and is left unchanged. So does a destination whose axes differ from the source's (OffsetVec with axes 0:1). The second case fails on master with a BoundsError from writing past the end, and the earlier review saw a segfault with an OffsetArray. Superseded by item 1 of the next section.
  4. Test comment described earlier code (Devin, non-blocking 3). Removed. That testset was rewritten and has no history comments.
  5. Longer-destination recursivecopy! is a behavior change (Devin, non-blocking 1). The code is unchanged; the test from item 3 pins the behavior. See ## Breaking change below. Superseded by item 1 of the next section.
  6. Corrections to the PR body above (Devin, non-blocking 4). Text above this section is left in place; these lines take precedence over it:
    • At dd83070 the Utils Tests safetestset ran 123 tests, not 120. With this round it runs 130 (results below).
    • The 5:7, i = 2 example in "What was not verified" is wrong: that index is below firstindex, and dd83070 already rejects it. The real gap was i between lastindex(a) + 1 and length(a) on 0-based vectors, plus in-bounds i > length(a) on vectors whose indices start above 1. Item 1 fixes both.
    • The opening paragraph's "leaving the append-on-overflow behavior (i > length(a)) untouched" no longer holds. Appending now happens for i > lastindex(a).

Verification for this round

Fail before, pass after, with Julia 1.13.0. Every case ran in its own @testset against src/utils.jl from master, from dd83070, and from this commit:

== master
Test Summary:                              | Fail  Error  Total
cases                                      |    4      1      5
  0-based: i == length appends             |           1      1   # BoundsError: ... index [3]
  axes 2:3: i == 3 updates in place        |    1             1   # Evaluated: [[1, 2], [3, 4], [8, 8]] == [[1, 2], [8, 8]]
  recursivecopy! longer destination throws |    2             2   # no throw; prefix copied
  recursivecopy! offset destination throws |    1             1   # Expected: DimensionMismatch, Thrown: BoundsError
== dd83070 (PR head before this round)
cases                                      |    3     1      1      5   (Pass/Fail/Error)
  0-based: i == length appends             |                 1      1   # BoundsError: attempt to access 2-element Vector{Vector{Int64}} at index [3]
  axes 2:3: i == 3 updates in place        |          1             1   # Evaluated: [[1, 2], [3, 4], [8, 8]] == [[1, 2], [8, 8]]
== this commit
cases         |    5      5

On dd83070, the committed test/Core/utils_test.jl stops with Error During Test at copyat_or_push!(z, 2, [5, 6]) (Utils Tests | 125 pass, 1 error). With this commit it passes: Utils Tests | 130 130.

GROUP=Core through Pkg.test() on this commit:

Julia 1.13.0:  exit 0, "Testing RecursiveArrayTools tests passed"
  Utils Tests 130/130, NamedArrayPartition 41/41, Partitions 129/129, Partitions and StaticArrays 6/6,
  VecOfArr Indexing 180 pass + 2 broken (marked broken on master too), VecOfArr Interface 154/154, Public API 49/49,
  Table traits 446/446, StaticArrays 39/39, Linear Algebra 54/54, SciMLStructures 53/53, Measurement 2/2
Julia 1.10.12: exit 0, "Testing RecursiveArrayTools tests passed"
  Utils Tests 130/130, VecOfArr Interface 150/150, all other testsets as above

GROUP=QA on Julia 1.13.0: QA | 20 20, exit 0. Docs: makedocs with the settings in docs/make.jl (doctest, linkcheck, checkdocs = :exports) on Julia 1.13.0, exit 0. deploydocs was not run.

Runic (Runic --check on both changed files) and typos exit 0.

Not verified in this round

  • GROUP=QA on Julia 1.10. The PR body above reports an Aqua ambiguity failure there that also fails on master.
  • The Downstream, NoPre, AD, SymbolicIndexingInterface and GPU groups, and Julia pre. None of them calls copyat_or_push!. The only recursivecopy! call among them is in test/Downstream/TrackerExt.jl, with equal lengths and TrackedArray elements, so the change does not reach it.
  • A real OffsetArrays.OffsetVector. OffsetArrays is not a test dependency, so the tests use a local OffsetVec type with the same axes behavior. The earlier review ran the OffsetArray cases by hand.

Review findings addressed (Codex Sol re-review of 5e5c59c)

The independent re-review (codex-cli 0.163.0, gpt-6.1-sol) of 5e5c59c returned CHANGES, rated medium, with one finding. It accepted the copyat_or_push! changes and every earlier finding as resolved.

  1. recursivecopy! threw DimensionMismatch for a longer destination of StaticArrays, where master copies into its leading elements. That changes defined behavior of an exported function. Fixed in 9a4aa24, using the non-breaking option the reviewer suggested. The method loops over eachindex(a), as master does, and first calls checkbounds(b, eachindex(a)) outside @inbounds. If b lacks any index of a (a shorter destination, or different axes), it throws BoundsError before writing anything. A longer destination gets master's prefix copy. The testset asserts:
    • BoundsError, with nothing written, for a shorter destination, an offset-axes destination and a smaller 2-D destination;
    • the prefix copy [[1, 1], [2, 2], [0, 0]] for a longer destination, with elements copied rather than aliased;
    • a full copy for equal lengths.

Corrections to the text above. Each line here takes precedence over the text it corrects:

  • The opening paragraph and the "After" block say recursivecopy! loops over eachindex(b, a) and throws DimensionMismatch. It now loops over eachindex(a) after checkbounds, and a shorter destination throws BoundsError: attempt to access 1-element Vector{MVector{2, Float64}} at index [1:2] and leaves the destination unchanged.
  • Items 3 and 5 of the previous "Review findings addressed" section, and the two recursivecopy! rows of its verification table, describe 5e5c59c. Item 1 above supersedes them.
  • The ## Breaking change section below has been rewritten for this head.

Verification for this round

Fail before and pass after, Julia 1.13.0, the committed test/Core/utils_test.jl wrapped in a @testset:

== 5e5c59c (PR head before this round)
recursivecopy! ... checks destination bounds: Test Failed  Expected: BoundsError  Thrown: DimensionMismatch   (x3: shorter, offset, 2-D)
recursivecopy! ... checks destination bounds: Error During Test
  DimensionMismatch: all inputs to eachindex must have the same indices, got Base.OneTo(3) and Base.OneTo(2)   (longer destination)
Utils Tests                                                        |  126     3      1    130
  recursivecopy! of StaticArray elements checks destination bounds |    2     3      1      6
== master src/utils.jl, run with --check-bounds=yes so the out-of-bounds writes throw instead of corrupting memory
recursivecopy! ... Test Failed  Evaluated: all(iszero, MVector{2, Float64}[[1.0, 1.0]])               (shorter: first element written before the throw)
recursivecopy! ... Test Failed  Evaluated: all(iszero, MVector{2, Float64}[[0.0, 0.0], [1.0, 1.0]])   (offset: written before the throw)
Utils Tests                                                        |  126     2      1    129
  recursivecopy! of StaticArray elements checks destination bounds |    7     2             9   (the longer-destination prefix copy passes on master)
  copyat_or_push! uses the indices of the destination              |    7            1      8
== this commit
Utils Tests   |  133    133

Also checked by hand on 1.13.0: an index-by-index (IndexCartesian) source (view(M, 1:2, 2:3)) copies into the matching block of a larger 3×3 destination and leaves the rest untouched. It throws BoundsError for a 1×3 destination. A source indexed by position copies into an IndexCartesian view destination.

GROUP=Core through Pkg.test() on this commit:

  • Julia 1.13.0: exit 0, Testing RecursiveArrayTools tests passed, Utils Tests | 133 133. Every other Core testset passes; the 2 broken in VecOfArr Indexing are also broken on master.
  • Julia 1.10.12: exit 0, Testing RecursiveArrayTools tests passed, Utils Tests | 133 133, all other Core testsets pass.

GROUP=QA on Julia 1.13.0: QA | 20 20, exit 0. Runic --check and typos (on the files and on the diff) exit 0.

Not verified in this round

  • The docs build was not rerun. This round changes no docstring, and the previous round's docs build covered the copyat_or_push! docstring.
  • GROUP=QA on Julia 1.10, the Downstream, NoPre, AD, SymbolicIndexingInterface and GPU groups, and Julia pre, for the same reasons as the previous round.
  • The Codex Sol re-review covers 5e5c59c. This head has not been independently reviewed yet.

Breaking change

Required bump: patch (4.5.4 → 4.5.5). No signature changes. Every behavior change in this PR is either an input that used to read or write out of bounds and now throws, or a result that now matches the documented contract. This PR does not edit Project.toml.

  • recursivecopy!(b, a) with StaticArray elements: an input that stayed in bounds on master gives the same result, including the prefix copy into a longer b. Inputs where master wrote outside b (a shorter b, or b missing an index of a) now throw BoundsError before writing. On master these were undefined behavior: segfaults and heap corruption were observed.
  • copyat_or_push!(a, i, x) on a 1-based vector: same result as master for every i ≥ 1. i ≤ 0 was an unchecked access under @inbounds and now throws BoundsError.
  • copyat_or_push!(a, i, x) on a vector that doesn't start at index 1:
    • i < firstindex(a) now throws BoundsError; it was out of bounds.
    • lastindex(a) < i ≤ length(a) now appends; it was an out-of-bounds write.
    • firstindex(a) ≤ i ≤ lastindex(a) with i > length(a) now updates a[i]. Master appended, which contradicts the docstring's "copy x into a[i] when that element exists". This is a fix to documented behavior, and the Codex Sol re-review explicitly accepted it.

🤖 Posted by an AI agent — harness: Claude Code 2.1.301 · model: claude-opus-5-5
Conversation: local session ID 5ab5a912-8ddf-4104-80aa-8ed04790f627 (headless, no URL)

Risk assessment (independent review, codex-cli gpt-6.1-sol, reviewed head 9a4aa24)

  • Risk: low

  • Blast radius: Two exported utilities: recursivecopy! for arrays of StaticArray elements and copyat_or_push! for AbstractVectors. The bounds checks prevent invalid accesses; copying into a longer destination retains its existing prefix-copy behavior. Offset-vector update/append behavior now follows the documented index contract. No signature removal, new dependency, license change, version bump requirement beyond a patch, or test silencing/weakening. The diff is limited to one implementation file and its Core tests.

  • Evidence: Reviewed head 9a4aa248484f55f060e5dd8861c4333cce0ffa59, saved at the start and rechecked unchanged. Read the whole diff, commit trailers, body, prior review, fix report, and existing review comment. There is no linked issue (closingIssuesReferences is empty). Authorship: initial commits identify Claude Code 2.1.294 / claude-sonnet-5; fix commits identify Claude Code 2.1.301 / claude-opus-5-5. Reviewed normally as the requested non-Claude reviewer.

    I ran JULIA_NUM_THREADS=4 JULIA_NUM_PRECOMPILE_TASKS=4 GROUP=Core julia +release --project=repo -e 'using Pkg; Pkg.test()' with a workspace-local writable depot layered over the shared read-only depot, on Julia 1.13.2: exit 0, Testing RecursiveArrayTools tests passed; Utils Tests 133/133. VecOfArr Indexing reports 180 pass and 2 existing broken tests; this PR does not alter them.

    I extracted both added testsets into tmp/regressions.jl and ran JULIA_NUM_THREADS=4 julia +release --project=tmp/probe-env tmp/regressions.jl: exit 0, recursivecopy bounds 9/9, copyat_or_push indices 12/12. For a safe before comparison I loaded default-branch src/utils.jl from 76bec79f7a09ce9546709376a0d7eb95deaf90f9 into RecursiveArrayTools and ran the same tests wrapped in an outer testset with --check-bounds=yes: exit 1, 14 pass / 2 fail / 1 error. The shorter and offset destinations had been modified before throwing; zero-based index 2 errored instead of appending. The longer-destination prefix-copy tests pass on both versions. Logs: core.log, before.log, after.log.

    CI was checked once at the start and once at the end: all 22 checks pending, no failed PR jobs to inspect. Current CI: https://github.com/SciML/RecursiveArrayTools.jl/actions/runs/38919684169 ; downstream: https://github.com/SciML/RecursiveArrayTools.jl/actions/runs/38919683663 ; formatting: https://github.com/SciML/RecursiveArrayTools.jl/actions/runs/38919683566 . I also read the latest completed default-branch downstream failure log: SciMLSensitivity Core4 errors with AssertionError: mi2 !== nothing in threaded ensembles: https://github.com/SciML/RecursiveArrayTools.jl/actions/runs/38819764786 . Default-branch CI itself passed: https://github.com/SciML/RecursiveArrayTools.jl/actions/runs/38819765422 . There is currently no PR failure to compare against that baseline. I did not locally run Julia 1.10, QA, docs, GPU, or downstream groups; the worker's results for those are not my own verification.

  • Independent review: codex-cli 0.162.1 gpt-6.1-sol rated it low, high confidence: focused preflight bounds checks and index-based branching fix reproduced failures while preserving valid prefix-copy behavior.

  • Merge: auto-merge candidate — no blocking correctness, test-silencing, dependency, or SemVer issue found; CI remains pending and should be checked before merging.


🤖 Posted by an AI agent — harness: codex-cli · model: gpt-6.1-sol (independent reviewer; appended by Claude Code 2.1.301 · claude-opus-5-5)
Conversation: https://claude.ai/code/session_01VZjwf7Ux9YtTzwHGHMqnsq

recursivecopy!(b::AbstractArray{<:StaticArray}, a) looped over
eachindex(a), so a shorter dest silently wrote out of bounds instead of
throwing. copyat_or_push! only checked length(a) >= i, so i=0 (or
negative) fell into the @inbounds update branch and corrupted memory
instead of throwing a BoundsError.

recursivecopy! now uses eachindex(b, a) (throws DimensionMismatch on a
length mismatch, matching Base's own idiom). copyat_or_push! now
rejects i < 1 with a BoundsError before the @inbounds block.

Before the fix, both cases segfault the Julia process on 1.10 and 1.12
(see PR description for the exact crash traces). After the fix they
throw DimensionMismatch / BoundsError and the existing test suite
(Core group, 120/120 Utils Tests) still passes unchanged on both
versions.

Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Agent-Harness: Claude Code 2.1.294
Agent-Model: claude-sonnet-5
Agent-Session: https://claude.ai/code/session_01LPHREnnonfLg1VcE1EJovv
Round-2 review (Grok 4.7, pr-667/round2): the i >= 1 guard turned a
previously correct call into a throw for any AbstractVector whose
firstindex isn't 1. copyat_or_push! is exported and its docstring
promises to copy into a[i] "when that element exists" -- on an
OffsetArray with indices 0:1, index 0 exists and master updates it;
the i >= 1 guard threw BoundsError there instead.

Use i >= firstindex(a), which is false only when i is below the
array's own first valid index, matching master's behavior for every
case the original fix needed to cover (the i=0/i=-1 segfault on a
plain, 1-based Vector) while leaving a 0-based array's own index 0
untouched.

Added a minimal 0-based AbstractVector test type (ZeroVec, avoiding
an OffsetArrays test dependency that would conflict with SciML#672's
Project.toml changes) and a regression test that copyat_or_push!(z, 0,
...) still updates index 0 while copyat_or_push!(z, -1, ...) still
throws. Fails against the i >= 1 code (BoundsError at a correct,
in-bounds index 0) and passes after this fix, on both Julia 1.10 and
1.12.

Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Agent-Harness: Claude Code 2.1.294
Agent-Model: claude-sonnet-5
Agent-Session: https://claude.ai/code/session_01LPHREnnonfLg1VcE1EJovv
@ChrisRackauckas

Copy link
Copy Markdown
Member

🤖 Automated comment from an AI agent running as @ChrisRackauckas — not written or reviewed by Chris.

Independent review (Devin CLI 3000.11.3, model fusion-claude-opus-5-5-high-sidekick-swe-2-medium): CHANGES, risk low.

Full review

VERDICT: CHANGES
RISK: low

Reviewed head c7b71d3 against origin/master 02c477c. Diff: src/utils.jl (+2/-1), test/Core/utils_test.jl (+45). There is no linked issue (closingIssuesReferences is empty), so there is no tracking issue to check against. The two fixes that are there work: both segfault cases now throw. One blocking gap remains in the same copyat_or_push! branch.

Blocking findings

  1. src/utils.jl:371: copyat_or_push! can still write out of bounds under @inbounds past the end of a non-1-based array. The PR declares this out of scope, and the example it gives for it is wrong. (confirmed by running)
    The PR adds a lower-bound guard, i >= firstindex(a), but keeps @inbounds if length(a) >= i as the upper-bound test. Round 2 made non-1-based arrays a supported case and added the ZeroVec test for them. For any such array with firstindex(a) < 1, every i in lastindex(a)+1 : length(a) passes both checks and reaches a[i] = x unchecked. Probe scratch/probe2.jl, run on Julia 1.12.7 with the PR env:

    • v = OffsetArray(view(backing, 1:2), 0:1); copyat_or_push!(v, 2, [9,9], false) returns normally. v is unchanged, nothing was appended, and backing[3] was overwritten with [9,9]: a silent write past the end of v, the same class of bug the PR title says it fixes. Master gives the same result.
    • v = OffsetArray(view(backing, 1:3), -5:-3); copyat_or_push!(v, 0, 99.0) silently writes backing[6] = 99.0 on both master and the PR.
    • A related wrong result from the same condition (not a memory-safety issue): v = OffsetArray(3 elems, 5:7); copyat_or_push!(v, 6, x) appends a 4th element instead of updating the existing v[6]. That contradicts the docstring ("copy x into a[i] when that element exists"). Master and the PR behave the same.

    The PR body (last "What was not verified" bullet) defers this using indices 5:7, i=2. That case is below firstindex and is caught by this PR: cp_offset5_2 throws BoundsError on the PR branch and only silently does nothing on master. The deferral is based on an example that doesn't show the real gap.

    Required change: replace @inbounds if length(a) >= i with @inbounds if i <= lastindex(a). For a 1-based Vector, lastindex == length, so nothing changes there. I applied exactly this one-line change in a scratch clone (scratch/fixprobe) and reran:

    • cp_offset0_2 / cp_offsetneg_0 now throw MethodError: no method matching resize!(::SubArray…). That is a safe error from push! on a non-resizable view, with no memory write.
    • Plain OffsetArray(-5:-3) with i = 1:3 appends correctly.
    • cp_offset5_6 now updates v[6] in place.
    • All the Vector cases are unchanged (cp_vec_0/cp_vec_m1 throw BoundsError; cp_vec_ok gives [[5,5,5],[9,9],[7,7]]).
    • test/Core/utils_test.jl passes 123/123.

    Add a regression test that fails before and passes after. Without an OffsetArrays dependency, one option: give ZeroVec a Base.push!(z::ZeroVec, v) = (push!(z.data, v); z) and assert that copyat_or_push!(z, 2, [5, 6]) appends, i.e. length(z) == 3 && z[2] == [5, 6]. With the current code, ZeroVec's non-propagating setindex! throws BoundsError at z.data[3] instead of appending, so the test fails before the fix.

Non-blocking findings

  1. src/utils.jl:89: deliberate behavior change for a longer destination. (confirmed by running) On master, recursivecopy!(b, a) with b = [MVector×3] and a = [MVector×2] returns normally and copies the prefix ([[1,1],[2,2],[0,0]]). On the PR it throws DimensionMismatch. This matches the nested-array method at utils.jl:117, which already uses eachindex(b, a), and the docstring points to recursivecopyto! for mismatched shapes. But the Number-eltype method (copyto!) still accepts a longer destination, so the three eltype families still disagree. It is a behavior change to an exported function, and the PR body says so. Whether that is a patch or a minor release is a maintainer call. Downstream CI (OrdinaryDiffEq Core/Interface/Downstream/Downstream2, SciMLBase, LabelledArrays, SciMLSensitivity Core2–6) passed on this head, so I found no evidence that anything downstream depends on the prefix-copy behavior.
  2. Other recursivecopy! cases checked. (confirmed by running) Same-length arrays with different shapes (2×3 vs 3×2 of SVector) and a Cartesian view destination behave the same on master and the PR. With an OffsetArray(0:1) destination and a 1-based source, master segfaulted (exit 139); the PR throws DimensionMismatch. That is an extra bug the PR fixes but doesn't mention.
  3. test/Core/utils_test.jl:190-192: comment refers to history. "matching master: the bounds check is against firstindex(a), not the literal 1" describes the round-1 code, not the current code. CLAUDE.md's "describe what exists, never what it replaced" rule applies. Reword it as a statement of the contract, e.g. "index 0 is in bounds for a 0-based vector and is updated in place", or delete it.
  4. PR body inaccuracies (confirmed by running):
    • It reports "Utils Tests 120/120". I get 123/123 on the PR versus 112/112 on master; the 11 new assertions account for the difference.
    • The 5:7, i=2 example is wrong (see Blocking 1).
    • The body does not open with the UNREVIEWED attribution banner that CLAUDE.md requires as the first line; it has only a trailing "Please ignore until reviewed" line and a session link.
  5. Fail-before is a crash, not a test failure. (confirmed by running) With master's src, the new recursivecopy! testset aborts the process (exit 134, heap corruption reported during type inference). Run alone, the copyat_or_push! testset records Test Failed at copyat_or_push!(vals, 0, …) (master returns normally with no visible change), then segfaults on i = -1 (exit 139). This is good enough evidence that the bug exists, though the "before" run never produces a clean test summary.
  6. CI. The one red check, SciMLSensitivity Core1, is a Mooncake MethodError: no constructors have been defined for Any at concrete_solve_derivatives.jl:451. The same job also fails on sibling PRs 668, 669 and 672, so it is not caused by this PR. All other checks are green, including QA (Julia 1), Core on 1/lts/pre/x86, and Runic.
  7. Sibling PRs. Among the open PRs, only the stale PR 492 touches src/utils.jl. PR 672 (OffsetArray handling in ArrayPartition) is the closest related work. It edits Project.toml but not utils.jl, so I expect no conflict. PR 678 targets the 1.10 QA ambiguity the PR body mentions, which is consistent with that failure existing before this PR.

What I ran

  • gh pr view 667, gh pr view 672, and the file lists of all open PRs; gh pr checks 667; the failed-job log for SciMLSensitivity Core1 (run 37831757516, job 113498608733).
  • Scratch environments: env_master (clone of 02c477c), env_pr (this checkout) on Julia 1.12.7, and env_pr110 (this checkout) on Julia 1.10. Each was Pkg.develop'd with the repo and lib/RecursiveArrayToolsShorthandConstructors, plus the [extras] test dependencies and OffsetArrays.
  • scratch/probe.jl, 17 cases, each in its own subprocess on master and on the PR. Results:
    • Master segfaulted: rc_shorter, rc_offset_dest, cp_vec_m1, cp_vec_nocopy_0.
    • Master returned normally after an out-of-bounds write: cp_vec_0, cp_svec_0, cp_vec_typemin, cp_offset5_2.
    • PR throws DimensionMismatch/BoundsError: all eight of those.
    • PR newly throws DimensionMismatch: rc_longer (master did a prefix copy).
    • Same result on master and PR: rc_equal_sv, rc_shape_mismatch_samelen, rc_cartesian_view, rc_offset_both, cp_vec_ok, cp_offset0_0, cp_offset5_6. One more case, cp_offset0_m1, returned normally on master and throws BoundsError on the PR.
  • scratch/probe2.jl (the out-of-bounds write past the end, Blocking 1) on master, on the PR, and with the one-line lastindex fix in scratch/fixprobe.
  • @safetestset of test/Core/utils_test.jl and partitions_and_static_arrays.jl:
    • PR on 1.12.7: 123/123 and 6/6.
    • PR on 1.10: 123/123 and 6/6.
    • Master on 1.12.7: 112/112 and 6/6.
  • copy_static_array_test.jl on the PR, 1.12.7 and 1.10: 39/39 each.
  • The new testsets extracted and run against master's src (scratch/newtests.jl, scratch/newtests_cp.jl): crash / Test Failed + segfault, as described in Non-blocking 5.
  • julia +1.12 --project=@runic -m Runic --check --diff src/utils.jl test/Core/utils_test.jl: exit 0. git diff origin/master...HEAD | typos -: exit 0.
  • All Julia processes I started ran in the foreground and have exited.

What I did not verify

  • GROUP=QA locally. I relied on the green CI QA (Julia 1) job instead. I did not reproduce the claimed 1.10 QA ambiguity failure on master.
  • The full GROUP=Core, Downstream, GPU, AD and NoPre groups locally (host limits). I relied on CI for those, all green apart from the unrelated SciMLSensitivity Core1 failure.
  • The PR body's claim that rc_shorter "returns silently" on Julia 1.10.12 instead of segfaulting. I ran the before-fix probes on 1.12.7 only.
  • A full audit of the 1008 recursivecopy!( call sites in ~/.julia/packages for code that depends on the prefix copy into a longer destination (Non-blocking 1). I only spot-checked OrdinaryDiffEqCore (uprev/u/u0 copies, which are the same length).
  • copyat_or_push! with i more than one past lastindex (it appends at lastindex+1, not at i). That behavior predates this PR and is untouched by it and by the proposed fix.

🤖 Posted by an AI agent — harness: Devin CLI 3000.11.3 (review), Claude Code 2.1.285 (posting) · model: fusion-claude-opus-5-5-high-sidekick-swe-2-medium
Conversation: local Claude session 3c311569-dda6-4687-bc9f-65febb212c33 (fleet master)

ChrisRackauckas and others added 3 commits October 16, 2026 07:40
copyat_or_push! decided between updating a[i] and appending with
`length(a) >= i`, which is only equivalent to `i <= lastindex(a)` for
1-based vectors. For a 0-based vector, `i == length(a)` took the update
branch and wrote one past the end under @inbounds; for a vector with
axes 2:3, the in-bounds index 3 was appended instead of updated. The
comparison now uses lastindex and sits outside @inbounds, and the
docstring states the BoundsError for i < firstindex(a).

The test helper vector type now takes an arbitrary first index, and the
recursivecopy! StaticArray test also pins DimensionMismatch for a longer
destination and for a destination with different axes.

Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Agent-Harness: Claude Code 2.1.301
Agent-Model: claude-opus-5-5
Agent-Session: local session ID 5ab5a912-8ddf-4104-80aa-8ed04790f627 (headless claude -p, no URL)
Looping over eachindex(b, a) made recursivecopy! of StaticArray
elements throw DimensionMismatch for a longer destination, where master
copies into the leading elements. That changes defined behavior of an
exported function. Loop over eachindex(a) as master does and call
checkbounds(b, eachindex(a)) before the @inbounds loop: a destination
missing any source index throws BoundsError before anything is written,
and a longer destination keeps the prefix copy.

Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Agent-Harness: Claude Code 2.1.301
Agent-Model: claude-opus-5-5
Agent-Session: local session ID 5ab5a912-8ddf-4104-80aa-8ed04790f627 (headless claude -p, no URL)

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants