Repository navigation
Fix two silent memory-safety bugs in recursivecopy!/copyat_or_push! - #667
ChrisRackauckas-Claude wants to merge 5 commits into
Conversation
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
Independent review (Devin CLI 3000.11.3, model fusion-claude-opus-5-5-high-sidekick-swe-2-medium): CHANGES, risk low. Full reviewVERDICT: CHANGES Reviewed head Blocking findings
Non-blocking findings
What I ran
What I did not verify
🤖 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 |
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)
What changed and why
Two
utils.jlhelpers 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) iterateseachindex(a)instead ofeachindex(b, a), so whenbis shorter thanait writesb[i]out of bounds for the trailing indices instead of throwing.copyat_or_push!(a, i, x)(utils.jl:370) only checkedlength(a) >= ibefore entering an@inboundsblock. Foribelowa's own first index, that condition can be trivially true, so the function takes the in-place-update branch and indexes under@inboundswith no check at all.Fix:
recursivecopy!now loops overeachindex(b, a), which throwsDimensionMismatchon a length mismatch (the same idiomrecursivecopy!already uses a few methods down for theAbstractArray/AbstractVectorOfArraycase).copyat_or_push!now rejectsi < firstindex(a)with aBoundsErrorbefore the@inboundsblock, 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 asi >= 1, with the PR text claiming the literal1"avoids a behavior change" and thatfirstindex"would introduce one." That claim was backwards.copyat_or_push!is exported and its docstring promises to copy intoa[i]"when that element exists" — for anyAbstractVectorwhosefirstindexisn't1(e.g. anOffsetArraywith indices0:1), index0exists and master correctly updates it; thei >= 1guard threwBoundsErrorthere instead, which is the behavior change the PR claimed to avoid. Fixed by usingi >= firstindex(a), which is false only wheniis below the array's own first valid index — correct for both a plain 1-basedVector(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 forcopyat_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):After (this branch):
Normal usage (matching lengths, in-bounds
i) is unaffected — verified with the existingrecursivecopy!/copyat_or_push!call sites in the test suite plus the new tests below, including the round-2 0-based-array case:Failing-before / passing-after
Ran
test/Core/utils_test.jldirectly (includes the regression tests) against unmodifiedsrc/utils.jl:Round-2 regression — against the round-1
i >= 1code (commit143d6c5), kept the Vector tests and added only theZeroVeccase:After this fix (commit
c7b71d3):test/Core/utils_test.jlruns to completion with no failures, on both Julia 1.10.12 and 1.12.7 — all 8/8 in thecopyat_or_push!testset, plus everything else in the file.Full
GROUP=Corerun (both Julia versions): exit 0, all testsets green, includingUtils Tests120/120.GROUP=QAon 1.12.7: 20/20 pass.GROUP=QAon 1.10.12 fails on a pre-existingAquamethod-ambiguity report (similar(::AbstractVectorOfArray, ::Type{T}, dims::Tuple{...})vsBase.similar, invector_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
utils.jlbounds-check fix; nothing in those groups calls these two functions with bounds-adjacent arguments, andOrdinaryDiffEq's owncopyat_or_push!call sites passsaveiteror the literals1/2, never a non-1-based array).GROUP=QAambiguity failure on Julia 1.10 was reproduced but not investigated/fixed — it predates this PR and is unrelated toutils.jl.recursivecopy!'seachindex(b, a)fix is itself a (correct, intentional) behavior change for a longer destination — master'srecursivecopy!silently copies only the overlapping prefix and leaves the destination's tail untouched for a longerb, matching the nested-array method's existing use of the same idiom, while this PR'seachindex(b, a)now throwsDimensionMismatchfor 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."length(a) >= i(the pre-existing upper-bound check, unchanged by either round of this PR) is not equivalent toi <= lastindex(a)for a non-1-based array with a gap (e.g. indices5: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 onaxes/Base's genericfirstindexfallback for whateverAbstractVectoris passed; this is correct for every case tested (Vector, the newZeroVec, and — per the round-2 reviewer's manual check —OffsetArray), but I have not exhaustively audited everyAbstractVectorsubtype in the ecosystem for a non-standardfirstindex.Please ignore until reviewed by @ChrisRackauckas.
Risk assessment
src/utils.jlonly, two internal/exported helper functions (recursivecopy!'sStaticArray-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.DimensionMismatch/BoundsError) shown above; round-1-vs-round-2 regression test shown failing/passing;GROUP=Corefull pass on Julia 1.10 and 1.12;GROUP=QApass on 1.12, pre-existing unrelated failure reproduced on 1.10 master.OffsetArrayregression, plus non-blocking notes on therecursivecopy!longer-destination behavior and the pre-existinglength(a) >= ivslastindexgap). The blocking finding is addressed in round 2 (commitc7b71d3); round-2 independent review still pending.🤖 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) onc7b71d3and a second review ofdd83070that returned CHANGES.copyat_or_push!usedlength(a) >= ito choose between updating and appending (blocking in both reviews). Fixed in 5e5c59c. The test is nowi <= lastindex(a), evaluated outside@inbounds;@inboundscovers only the read and write ofa[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 axes2:3, index3is now updated in place; before, it was appended. A plain 1-basedVectoris unaffected, sincelastindex == lengththere. The docstring now says an index ofais updated,i > lastindex(a)appends, andi < firstindex(a)throwsBoundsError.ZeroVectest type becameOffsetVec(data, first). It takes any first index and supportspush!. Thecopyat_or_push!testset now also covers appending ati == lengthon a 0-based vector, updating index 3 of a vector with axes2:3, aperform_copy = falseupdate, appending pastlastindex, andBoundsErrorbelowfirstindex.recursivecopy!destination. Added. A longer destination ofMVectors throwsDimensionMismatchand is left unchanged. So does a destination whose axes differ from the source's (OffsetVecwith axes0:1). The second case fails on master with aBoundsErrorfrom writing past the end, and the earlier review saw a segfault with anOffsetArray. Superseded by item 1 of the next section.recursivecopy!is a behavior change (Devin, non-blocking 1). The code is unchanged; the test from item 3 pins the behavior. See## Breaking changebelow. Superseded by item 1 of the next section.dd83070theUtils Testssafetestset ran 123 tests, not 120. With this round it runs 130 (results below).5:7, i = 2example in "What was not verified" is wrong: that index is belowfirstindex, anddd83070already rejects it. The real gap wasibetweenlastindex(a) + 1andlength(a)on 0-based vectors, plus in-boundsi > length(a)on vectors whose indices start above 1. Item 1 fixes both.i > length(a)) untouched" no longer holds. Appending now happens fori > lastindex(a).Verification for this round
Fail before, pass after, with Julia 1.13.0. Every case ran in its own
@testsetagainstsrc/utils.jlfrom master, fromdd83070, and from this commit:On
dd83070, the committedtest/Core/utils_test.jlstops withError During Testatcopyat_or_push!(z, 2, [5, 6])(Utils Tests | 125 pass, 1 error). With this commit it passes:Utils Tests | 130 130.GROUP=CorethroughPkg.test()on this commit:GROUP=QAon Julia 1.13.0:QA | 20 20, exit 0. Docs:makedocswith the settings indocs/make.jl(doctest, linkcheck,checkdocs = :exports) on Julia 1.13.0, exit 0.deploydocswas not run.Runic (
Runic --checkon both changed files) andtyposexit 0.Not verified in this round
GROUP=QAon Julia 1.10. The PR body above reports an Aqua ambiguity failure there that also fails on master.pre. None of them callscopyat_or_push!. The onlyrecursivecopy!call among them is intest/Downstream/TrackerExt.jl, with equal lengths andTrackedArrayelements, so the change does not reach it.OffsetArrays.OffsetVector. OffsetArrays is not a test dependency, so the tests use a localOffsetVectype with the same axes behavior. The earlier review ran theOffsetArraycases by hand.Review findings addressed (Codex Sol re-review of 5e5c59c)
The independent re-review (codex-cli 0.163.0,
gpt-6.1-sol) of5e5c59creturned CHANGES, rated medium, with one finding. It accepted thecopyat_or_push!changes and every earlier finding as resolved.recursivecopy!threwDimensionMismatchfor a longer destination ofStaticArrays, 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 overeachindex(a), as master does, and first callscheckbounds(b, eachindex(a))outside@inbounds. Ifblacks any index ofa(a shorter destination, or different axes), it throwsBoundsErrorbefore 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;[[1, 1], [2, 2], [0, 0]]for a longer destination, with elements copied rather than aliased;Corrections to the text above. Each line here takes precedence over the text it corrects:
recursivecopy!loops overeachindex(b, a)and throwsDimensionMismatch. It now loops overeachindex(a)aftercheckbounds, and a shorter destination throwsBoundsError: attempt to access 1-element Vector{MVector{2, Float64}} at index [1:2]and leaves the destination unchanged.recursivecopy!rows of its verification table, describe5e5c59c. Item 1 above supersedes them.## Breaking changesection 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.jlwrapped in a@testset: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 throwsBoundsErrorfor a 1×3 destination. A source indexed by position copies into anIndexCartesianview destination.GROUP=CorethroughPkg.test()on this commit:Testing RecursiveArrayTools tests passed,Utils Tests | 133 133. Every other Core testset passes; the 2 broken in VecOfArr Indexing are also broken on master.Testing RecursiveArrayTools tests passed,Utils Tests | 133 133, all other Core testsets pass.GROUP=QAon Julia 1.13.0:QA | 20 20, exit 0. Runic--checkandtypos(on the files and on the diff) exit 0.Not verified in this round
copyat_or_push!docstring.GROUP=QAon Julia 1.10, the Downstream, NoPre, AD, SymbolicIndexingInterface and GPU groups, and Juliapre, for the same reasons as the previous round.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)withStaticArrayelements: an input that stayed in bounds on master gives the same result, including the prefix copy into a longerb. Inputs where master wrote outsideb(a shorterb, orbmissing an index ofa) now throwBoundsErrorbefore 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 everyi ≥ 1.i ≤ 0was an unchecked access under@inboundsand now throwsBoundsError.copyat_or_push!(a, i, x)on a vector that doesn't start at index 1:i < firstindex(a)now throwsBoundsError; it was out of bounds.lastindex(a) < i ≤ length(a)now appends; it was an out-of-bounds write.firstindex(a) ≤ i ≤ lastindex(a)withi > length(a)now updatesa[i]. Master appended, which contradicts the docstring's "copyxintoa[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 andcopyat_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 (closingIssuesReferencesis 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.jland ranJULIA_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-branchsrc/utils.jlfrom76bec79f7a09ce9546709376a0d7eb95deaf90f9into 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 !== nothingin 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