fix(builtin, deque): stop nulling vacated slots so views cannot read invalid memory - #4135
Open
Yu-zh wants to merge 1 commit into
Open
fix(builtin, deque): stop nulling vacated slots so views cannot read invalid memory#4135Yu-zh wants to merge 1 commit into
Yu-zh wants to merge 1 commit into
Conversation
Collaborator
Coverage Report for CI Build 6354Coverage decreased (-0.02%) to 90.908%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
…invalid memory Taking a view from an `Array` or a `Deque` 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` and `Deque::as_views` are both 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 a container 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 container is dropped, so clear-and-refill self-heals -- each push releases one old occupant, and repeated fill/drain cycles on one container hold flat. Reclaiming on demand is explicit. `Array::fill_unused` and `Deque::fill_unused` overwrite every slot from `length()` to `capacity()`, or its wrapped equivalent for a deque, 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. `Deque::clear` and `Deque::truncate` collapse to length assignments, and `Deque::truncate(0)` no longer routes through `Deque::clear`. 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. `%fixedarray.set_null` has no users left in core. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KYfC4NDSQ4ZdCMapJmioL6
Yu-zh
force-pushed
the
Yu-zh/no-set-null-fill-value
branch
from
August 26, 2026 03:16
fa77bec to
f76ee90
Compare
Yu-zh
marked this pull request as ready for review
August 26, 2026 03:28
This was referenced Sep 2, 2026
Contributor
|
Split into two independent PRs so each container can be reviewed on its own, both rebased onto current
Each PR body has a "Changes relative to #4135" section listing what differs from this branch (for |
bobzhang
added a commit
that referenced
this pull request
Sep 3, 2026
…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
bobzhang
added a commit
that referenced
this pull request
Sep 3, 2026
…emory `Deque::as_views` hands out `ArrayView`s that share the deque's 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. `Deque::as_views` 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 `A`. Mutating a deque 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 a deque the same way `pop_back` 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 deque is dropped, so clear-and-refill self-heals -- each push releases one old occupant, and repeated fill/drain cycles on one deque hold flat. Reclaiming on demand is explicit. The new `Deque::release_unused(placeholder~)` overwrites every slot not currently holding an element with the placeholder -- the complement of the occupied run, which may wrap around the end of the buffer -- and that 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: `UninitializedArray::make` (`%fixedarray.make_uninit`) NULL-fills for reference element types, growing blits into a fresh NULL-filled buffer, and the pushes already store into exactly such slots. `Deque::clear` and `Deque::truncate` collapse to length assignments, and `Deque::truncate(0)` no longer routes through `Deque::clear`. `Deque::drain` keeps its blits and loses only the nulling passes; the branch that existed solely to null an emptied back run is folded away. This is the `Deque` half of #4135, split out so that each container can be reviewed on its own; `Array` 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
bobzhang
added a commit
that referenced
this pull request
Sep 3, 2026
…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
bobzhang
added a commit
that referenced
this pull request
Sep 3, 2026
…emory `Deque::as_views` hands out `ArrayView`s that share the deque's 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. `Deque::as_views` 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 `A`. Mutating a deque 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 a deque the same way `pop_back` 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 deque is dropped, so clear-and-refill self-heals -- each push releases one old occupant, and repeated fill/drain cycles on one deque hold flat. Reclaiming on demand is explicit. The new `Deque::release_unused(placeholder~)` overwrites every slot not currently holding an element with the placeholder -- the complement of the occupied run, which may wrap around the end of the buffer -- and that 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: `UninitializedArray::make` (`%fixedarray.make_uninit`) NULL-fills for reference element types, growing blits into a fresh NULL-filled buffer, and the pushes already store into exactly such slots. `Deque::clear` and `Deque::truncate` collapse to length assignments, and `Deque::truncate(0)` no longer routes through `Deque::clear`. `Deque::drain` keeps its blits and loses only the nulling passes; the branch that existed solely to null an emptied back run is folded away. This is the `Deque` half of #4135, split out so that each container can be reviewed on its own; `Array` 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
bobzhang
added a commit
that referenced
this pull request
Sep 3, 2026
…emory `Deque::as_views` hands out `ArrayView`s that share the deque's 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. `Deque::as_views` 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 `A`. Mutating a deque 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 a deque the same way `pop_back` 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 deque is dropped, so clear-and-refill self-heals -- each push releases one old occupant, and repeated fill/drain cycles on one deque hold flat. Reclaiming on demand is explicit. The new `Deque::release_unused(placeholder~)` overwrites every slot not currently holding an element with the placeholder -- the complement of the occupied run, which may wrap around the end of the buffer -- and that 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: `UninitializedArray::make` (`%fixedarray.make_uninit`) NULL-fills for reference element types, growing blits into a fresh NULL-filled buffer, and the pushes already store into exactly such slots. `Deque::clear` and `Deque::truncate` collapse to length assignments, and `Deque::truncate(0)` no longer routes through `Deque::clear`. `Deque::drain` keeps its blits and loses only the nulling passes; the branch that existed solely to null an emptied back run is folded away. This is the `Deque` half of #4135, split out so that each container can be reviewed on its own; `Array` 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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Taking a view from an
Arrayor aDequeshares 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::viewandDeque::as_viewsare both 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 a container 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 container is dropped, which means the clear-and-refill pattern self-heals: each push releases one old occupant, so repeated fill/drain cycles on one container hold flat.Reclaiming on demand is explicit, and there are two ways to do it:
fill_unused(value)— newshrink_to_fit()— already existedlength()fill_unusedoverwrites every slot fromlength()tocapacity()— or its wrapped equivalent for aDeque— 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.Two new methods, nothing else in the public surface moves.
Array::resizeshrinks liketruncateand no longer releases on that branch.Deque::clearandDeque::truncateboth collapse to length assignments, andDeque::truncate(0)no longer routes throughDeque::clear.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.%fixedarray.set_nullnow has no users in core.Testing
moon teston--target native(7479),wasm(7561),wasm-gc(7561) andjs(7495), all passingpop/clear/truncate/drain;fill_unusedafter each of them, including the wrapped case forDequeand 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 allclean apart from a pre-existingencoding/utf8/decode_js.mbt:21FFI deprecationmoon info,moon fmt—.mbtichurn is the two added methods and nothing else