fix(builtin): stop nulling vacated Array slots so views cannot read invalid memory - #4190
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
There are a few concrete documentation/text inaccuracies/typos in the changed regions that should be corrected before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates Array shrinking semantics so that ArrayViews can no longer observe invalid/uninitialized memory after the underlying array is shrunk, and introduces an explicit API (Array::fill_unused) to proactively overwrite/release references retained in unused capacity.
Changes:
- Stop clearing (“nulling out”) vacated slots on non-JS backends during shrinking/removal, preventing views from reading invalid memory.
- Add
Array::fill_unusedto overwrite[length(), capacity())in-place to release retained elements on demand (documented as a no-op on JS). - Add/update documentation and non-JS regression tests covering views across shrinking operations and buffer reuse.
File summaries
| File | Description |
|---|---|
| CHANGELOG.md | Documents the behavioral change and adds an entry for Array::fill_unused. |
| builtin/pkg.generated.mbti | Exposes the new Array::fill_unused API in the generated interface. |
| builtin/arrayview.mbt | Expands ArrayView safety/behavior documentation around mutations and backend differences. |
| builtin/arraycore_nonjs.mbt | Implements the non-JS behavioral changes, adds fill_unused, and updates docs around retention/release. |
| builtin/arraycore_js.mbt | Adds fill_unused as a documented no-op and clarifies drain semantics on JS. |
| builtin/array.mbt | Updates high-level Array docs for clear and truncation-like behavior to reflect retention. |
| builtin/array_nonjs_test.mbt | Adds regression tests ensuring views never observe uninitialized memory and buffers are not replaced on shrink. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| #### Changed | ||
|
|
||
| - `Array` shrinking operations no longer null out the slots they vacate, so an `ArrayView` created before the mutation can no longer read uninitialized memory (previously a segfault on native and `null` on wasm-gc; the JavaScript backend is unchanged, and such a view still observes `undefined` past the array's current length there). Mutating an array while a view of it is alive remains a program error, but it now yields unspecified *valid* values rather than undefined behavior. The removed elements stay reachable from the buffer until later pushes reuse those slots, the buffer grows, or the array is dropped -- uniformly, `clear` included, which no longer releases them. `Array::fill_unused` overwrites the unused capacity in place to release them on demand, while `shrink_to_fit`, which already reallocated, lets them go with the old buffer. No existing signature changes |
| /// that take no fill value leave outstanding. `Array::shrink_to_fit` releases | ||
| /// them too, but by allocating an exact-size buffer and copying every survivor | ||
| /// into it; this costs one pass over the unused region and no allocation. |
| /// No removal writes to the slots it vacates, so the removed elements stay | ||
| /// reachable, and so unreleased, until a later push reuses the slot, until the | ||
| /// buffer grows, or until the buffer itself is dropped. That holds uniformly: |
Coverage Report for CI Build 6507Coverage decreased (-0.001%) to 90.974%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
…nvalid memory Taking a view from an `Array` shares the underlying buffer, and every shrinking operation cleared the slots it vacated with `%fixedarray.set_null`. A view created before such a mutation then read a null slot: SIGSEGV on native for reference element types, `null` on wasm-gc, `undefined` on JS. `Array::view` is plain `pub`, so this was undefined behaviour reachable from entirely safe code. Vacated slots are now left alone, so a view can only ever observe valid values of `T`. Mutating an array while a view of it is alive stays a program error, but what the view yields is merely unspecified rather than invalid. The cost is that a removal retains what it removes, and that applies uniformly: `clear` empties an array the same way `pop` shortens it, and neither writes to the slots it gives up. No existing signature changes. The removed elements are released once a later push reuses the slot, once the buffer grows, or once the array is dropped, so clear-and-refill self-heals -- each push releases one old occupant, and repeated fill/drain cycles on one array hold flat. Reclaiming on demand is explicit. The new `Array::release_unused(placeholder~)` overwrites every slot from `length()` to `capacity()` with the placeholder, which is exactly the region any removal leaves behind: one pass, no allocation, capacity kept. `shrink_to_fit` was already releasing those elements by letting the old buffer go, at the cost of an allocation plus a copy of every survivor; that is now documented rather than incidental. Writing into that region needs no high-water mark, because it is always either NULL or a live reference, never garbage. `%fixedarray.make_uninit` NULL-fills for reference element types -- it has to, since `moonbit_drop_object` walks a REF_ARRAY's full capacity and skips slots with `if (!obj) continue` -- and `moonbit_make_ref_array_with_blit` NULL-fills everything outside the blitted range when a buffer grows. It is the same region `Array::push` writes into on every push past the previous high-water mark. `Array::resize` shrinks like `truncate` and no longer releases on that branch. Two things differ from the code in #4135. `Array::resize_buffer` now copies only the live prefix into the new buffer instead of the whole old capacity: when vacated slots were NULL the difference was a memcpy of zeros, but with retention it decided whether `reserve_capacity` carried the retained elements into the new buffer, which would have made the "released once the buffer grows" promise false on that one path. And `Array::release_unused` goes through the same `%fixedarray.fill` intrinsic, with the same `#owned` argument, that `Array::fill` and `Array::resize` already use for this region, instead of a hand-written `unsafe_set` loop. The JavaScript backend is unchanged and still shrinks the underlying JS array, so a view reaching past the current length observes `undefined` there; the `ArrayView` documentation now says so explicitly, and `Array::release_unused` is a documented no-op on that backend. This is the `Array` half of #4135, split out so that each container can be reviewed on its own; `Deque` gets the same treatment in a separate PR, and `%fixedarray.set_null` loses its last user in core once both have landed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WjN1QF3HbJohHocMPBRcuw
|
Renamed the new method: |
68a97f5 to
d55acae
Compare
Split out of #4135 (the
Arrayhalf;Dequeis in its own PR) so that each container can be reviewed on its own. The two are independent and can land in either order;%fixedarray.set_nullloses its last user in core once both are in.Taking a view from an
Arrayshares the underlying buffer, and every shrinking operation cleared the slots it vacated with%fixedarray.set_null. A view created before such a mutation then read a null slot.Array::viewis plainpub, so this is undefined behaviour reachable from entirely safe code:Native segfaults for reference element types; wasm-gc yields
nulland JSundefined.Stringmasks it — a nullStringreports.length() == 0and compares== ""— so the value propagates silently into other containers instead of failing.Approach
Vacated slots are no longer cleared, so a view can only ever observe valid values of
T. Mutating an array while a view of it is alive stays a program error, but what the view yields is now merely unspecified rather than invalid.The cost is that a removal retains what it removes. That applies uniformly --
clearempties an array the same waypopshortens it, and neither writes to the slots it gives up -- so there is no operation-by-operation rule to learn and no existing signature changes. The elements are released when a later push reuses the slot, when the buffer grows, or when the array is dropped, which means the clear-and-refill pattern self-heals: each push releases one old occupant, so repeated fill/drain cycles on one array hold flat.Reclaiming on demand is explicit, and there are two ways to do it:
release_unused(placeholder~)— newshrink_to_fit()— already existedlength()release_unusedoverwrites every slot fromlength()tocapacity(), which is exactly the region any removal leaves behind.shrink_to_fitwas already releasing those elements by letting the old buffer go; that is now documented rather than incidental.One new method, nothing else in the public surface moves.
Array::resizeshrinks liketruncateand no longer releases on that branch. On the JavaScript backendrelease_unusedis a documented no-op: shrinking a JS array already releases the removed elements, and a view reaching past the current length observesundefinedthere as before, which theArrayViewdocumentation now states.Writing into
[length(), capacity())is safe without tracking a high-water mark: that region is always either NULL or a live reference, never garbage.%fixedarray.make_uninitNULL-fills for reference element types — it has to, sincemoonbit_drop_objectwalks a REF_ARRAY's full capacity and skips slots withif (!obj) continue— andmoonbit_make_ref_array_with_blitNULL-fills everything outside the blitted range when a buffer grows. It is the same regionArray::pushwrites into on every push past the previous high-water mark.Changes relative to #4135
An independent review pass over the split found two places where the code and the prose disagreed, both fixed here:
Array::resize_buffernow copies only the live prefix into the new buffer. It used to copy the whole old capacity, which was a memcpy of NULLs before and would have carried the retained elements into the new buffer on thereserve_capacitypath, contradicting the "released once the buffer grows" wording; every other growth path already copied onlylength()elements.Array::release_unuseduses the%fixedarray.fillintrinsic with an#ownedplaceholder, exactly asArray::fillandArray::resizedo for this same region, instead of a hand-writtenunsafe_setloop.Docs were tightened where they were imprecise: after a shifting removal (
remove,drain,retain, ...) the vacated tail holds duplicate references to shifted survivors, not "the removed elements"; the sharedclear/truncatedocs now say the JS backend releases immediately; the changelog gets an "Added" entry for the new method. The buffer-reuse test gained aremovethat actually shifts and adrainleg, which its comment already claimed.Review notes
Consequences the review surfaced that are inherent to the retain-instead-of-null design and worth weighing explicitly:
clear()until overwritten. The in-core instance islexbuf/storage.mbt, wherediscard_beforecallschunks.clear()on anArray[String]of consumed input chunks, so a streaming lexer now keeps up tocapacity()consumed chunks alive until later pushes overwrite them. Any FFI finalizers on those elements run later than before.moonbit_make_ref_array_with_blit's memcpy branch, in-place reuse) are not taken for it.release_unusedneeds a placeholder value ofT; element types without a cheap one (closures, handles) can only release viashrink_to_fit.Testing
moon teston--target native(7485),wasm-gc(7600),js(7533) andwasm(7570), all passingnot jstargets) covering: views afterclear/truncate/pop/remove/drain;release_unusedafter each of them and a run of removals settled by one call; and that no shrinking operation replaces the buffer, pinned by pushing after emptying and observing through a view taken beforehandmoon check --deny-warn --target allcleanmoon info,moon fmt—.mbtichurn is the one added method and nothing else🤖 Generated with Claude Code
https://claude.ai/code/session_01WjN1QF3HbJohHocMPBRcuw