From d55acae77d8a5d21d2ca8a858aa2299fe0d3409f Mon Sep 17 00:00:00 2001 From: Hongbo Zhang Date: Wed, 2 Sep 2026 17:43:05 +0800 Subject: [PATCH] fix(builtin): stop nulling vacated Array slots so views cannot read invalid 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) Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01WjN1QF3HbJohHocMPBRcuw --- CHANGELOG.md | 2 + builtin/array.mbt | 15 +++ builtin/array_nonjs_test.mbt | 206 +++++++++++++++++++++++++++++++++++ builtin/arraycore_js.mbt | 26 +++++ builtin/arraycore_nonjs.mbt | 88 ++++++++++++--- builtin/arrayview.mbt | 19 ++++ builtin/pkg.generated.mbti | 1 + 7 files changed, 344 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 64d8420c08..c3d221791d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,9 +13,11 @@ changelog should follow [Keep a Changelog](https://keepachangelog.com/en/1.1.0/) - Core commit: `bd827dc85` - Added `Debug` trait with `derive(Debug)` support, including `ignore=[..]` configuration for non-debuggable nested types - Added new `moonbitlang/async` APIs including `@process.spawn`, advisory file locking, `@fs.tmpdir`, `@async.all`, and `@async.any` +- Added `Array::release_unused(placeholder~)`, which overwrites the unused capacity of an array in place with a placeholder, releasing the elements that earlier removals left there #### 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::release_unused(placeholder~)` overwrites the unused capacity in place with a placeholder to release them on demand, while `shrink_to_fit`, which already reallocated, lets them go with the old buffer. No existing signature changes - **BREAKING**: `@json.parse` now rejects unpaired `\uXXXX` surrogate escapes in strings, raising `ParseError::InvalidChar` at the backslash that opens the offending escape; an escaped leading surrogate must be followed immediately by an escaped trailing surrogate, and the pair decodes to the character it denotes. Previously such escapes were decoded unchecked and produced a `String` that was not well-formed Unicode. `@json.valid` reports the same documents as invalid. JSON emitted by `JSON.stringify` in JavaScript can contain these escapes, so input that JavaScript and Python accept may now be rejected — as it is by Rust's serde_json - `@json.inspect` has been migrated to `json_inspect` - `String::sub` and `StringView::sub` now panic on invalid indices instead of raising `CreatingViewError`. The `CreatingViewError` type has been removed. diff --git a/builtin/array.mbt b/builtin/array.mbt index 21567c294a..27c26a202b 100644 --- a/builtin/array.mbt +++ b/builtin/array.mbt @@ -560,6 +560,13 @@ pub fn[T] Array::any(self : Array[T], f : (T) -> Bool raise?) -> Bool raise? { /// /// This method has no effect on the allocated capacity of the array, only setting the length to 0. /// +/// Emptying an array is a removal like any other: the buffer keeps referring to +/// the elements that were in it, and they are released once later pushes reuse +/// those slots, the buffer grows, or the array is dropped. Call +/// `Array::release_unused` to overwrite them at once, or `Array::shrink_to_fit` +/// to hand the buffer back entirely. On the JavaScript backend the removed +/// elements are released right away and `Array::release_unused` is a no-op. +/// /// # Example /// ```mbt check /// test { @@ -1862,6 +1869,14 @@ pub fn[A] Array::unsafe_pop_back(self : Array[A]) -> Unit { /// - If `len` is negative, the function does nothing. /// - If `len` exceeds current length, the array remains unchanged. /// +/// Elements beyond `len` are removed from the array, but the backing buffer +/// keeps referring to them: they are released once those slots are reused by +/// later pushes, once the buffer grows, or once the array is dropped. Call +/// `Array::release_unused` to overwrite them at once, or `Array::shrink_to_fit` +/// to move the survivors into an exact-size buffer. On the JavaScript backend +/// the removed elements are released right away and `Array::release_unused` is a +/// no-op. +/// /// Example: /// /// ```mbt check diff --git a/builtin/array_nonjs_test.mbt b/builtin/array_nonjs_test.mbt index 065c455053..04199df3a1 100644 --- a/builtin/array_nonjs_test.mbt +++ b/builtin/array_nonjs_test.mbt @@ -69,3 +69,209 @@ test "shrink_to_fit" { v.shrink_to_fit() inspect(v.capacity(), content="3") } + +///| +/// Removing elements must never leave a slot a view could read as +/// uninitialized memory, so nothing is written to the slots a removal vacates +/// and a view taken beforehand still sees the original elements. +test "clear leaves the emptied slots as they were" { + let arr = ["a", "b", "c"] + let view = arr[0:3] + arr.clear() + inspect(arr.length(), content="0") + debug_inspect( + view, + content=( + #| + ), + ) +} + +///| +test "release_unused after clear overwrites the whole buffer" { + let arr = ["a", "b", "c"] + let view = arr[0:3] + arr.clear() + arr.release_unused(placeholder="-") + inspect(arr.length(), content="0") + debug_inspect( + view, + content=( + #| + ), + ) +} + +///| +test "truncate keeps removed elements reachable until release_unused" { + let kept = ["a", "b", "c", "d"] + let kept_view = kept[0:4] + kept.truncate(2) + debug_inspect( + kept, + content=( + #|["a", "b"] + ), + ) + debug_inspect( + kept_view, + content=( + #| + ), + ) + let filled = ["a", "b", "c", "d"] + let filled_view = filled[0:4] + filled.truncate(2) + filled.release_unused(placeholder="-") + debug_inspect( + filled, + content=( + #|["a", "b"] + ), + ) + debug_inspect( + filled_view, + content=( + #| + ), + ) +} + +///| +/// No shrinking operation replaces the buffer, so emptying an array never +/// costs an allocation -- whether it happens through `clear`, `truncate`, +/// `remove` or `drain`. A later push makes that observable: it lands in the +/// reused buffer, which a view taken beforehand still points at. +test "no shrinking operation replaces the buffer" { + let cleared = ["a", "b", "c"] + let cleared_view = cleared[0:3] + cleared.clear() + cleared.push("x") + debug_inspect( + cleared_view, + content=( + #| + ), + ) + let truncated = ["a", "b", "c"] + let truncated_view = truncated[0:3] + truncated.truncate(0) + truncated.push("x") + debug_inspect( + truncated_view, + content=( + #| + ), + ) + // `remove(0)` shifts "b" down, so the buffer reads [b, b] before the push. + let removed = ["a", "b"] + let removed_view = removed[0:2] + let _ = removed.remove(0) + removed.push("x") + debug_inspect( + removed_view, + content=( + #| + ), + ) + // `drain(0, 2)` shifts "c" down, so the buffer reads [c, b, c] before the push. + let drained = ["a", "b", "c"] + let drained_view = drained[0:3] + let _ = drained.drain(0, 2) + drained.push("x") + debug_inspect( + drained_view, + content=( + #| + ), + ) +} + +///| +test "release_unused overwrites the tail a drain vacated" { + let arr = ["a", "b", "c", "d"] + let view = arr[0:4] + let drained = arr.drain(1, 3) + arr.release_unused(placeholder="-") + debug_inspect( + drained, + content=( + #|["b", "c"] + ), + ) + debug_inspect( + arr, + content=( + #|["a", "d"] + ), + ) + debug_inspect( + view, + content=( + #| + ), + ) +} + +///| +/// `pop`, `remove` and `retain` offer no fill value of their own, so the +/// elements they remove stay in the buffer until something overwrites them. +/// `release_unused` is what releases them without reallocating, and a view taken +/// beforehand shows exactly which slots it reached. +test "release_unused releases what pop and remove leave behind" { + let popped = ["a", "b", "c"] + let popped_view = popped[0:3] + let _ = popped.pop() + debug_inspect( + popped_view, + content=( + #| + ), + ) + popped.release_unused(placeholder="-") + debug_inspect( + popped_view, + content=( + #| + ), + ) + let removed = ["a", "b", "c"] + let removed_view = removed[0:3] + let _ = removed.remove(0) + removed.release_unused(placeholder="-") + debug_inspect( + removed, + content=( + #|["b", "c"] + ), + ) + debug_inspect( + removed_view, + content=( + #| + ), + ) +} + +///| +/// `release_unused` covers the whole unused region, not just the slots the most +/// recent operation vacated, so one call settles a run of removals. +test "release_unused spans every outstanding removal" { + let arr = ["a", "b", "c", "d"] + let view = arr[0:4] + let _ = arr.pop() + arr.truncate(1) + arr.release_unused(placeholder="-") + debug_inspect( + arr, + content=( + #|["a"] + ), + ) + debug_inspect( + view, + content=( + #| + ), + ) +} diff --git a/builtin/arraycore_js.mbt b/builtin/arraycore_js.mbt index eb1a9547d2..5164a78762 100644 --- a/builtin/arraycore_js.mbt +++ b/builtin/arraycore_js.mbt @@ -252,6 +252,29 @@ pub fn[T] Array::shrink_to_fit(self : Array[T]) -> Unit { ignore(self) } +///| +/// Overwrites the array's unused capacity with `placeholder`, releasing +/// whatever those slots held. +/// +/// **NOTE**: This method does nothing on the js platform -- shrinking a +/// JavaScript array releases the removed elements outright, so there is no +/// unused capacity holding on to them. +/// +/// Example: +/// +/// ```mbt check +/// test { +/// let arr = ["a", "b", "c"] +/// let _ = arr.pop() +/// arr.release_unused(placeholder="") +/// debug_inspect(arr, content="[\"a\", \"b\"]") +/// } +/// ``` +pub fn[T] Array::release_unused(self : Array[T], placeholder~ : T) -> Unit { + ignore(self) + ignore(placeholder) +} + ///| /// Adds an element to the end of the array. /// @@ -430,6 +453,9 @@ pub fn[T] Array::remove(self : Array[T], index : Int) -> T { /// @test.assert_eq(v, [3, 5]) /// } /// ``` +/// +/// On this backend the underlying JavaScript array is spliced directly, which +/// already releases the drained elements. pub fn[T] Array::drain(self : Array[T], begin : Int, end : Int) -> Array[T] { guard begin >= 0 && end <= self.length() && begin <= end else { abort( diff --git a/builtin/arraycore_nonjs.mbt b/builtin/arraycore_nonjs.mbt index 1aba9aa7a9..aa844ffb7d 100644 --- a/builtin/arraycore_nonjs.mbt +++ b/builtin/arraycore_nonjs.mbt @@ -12,9 +12,6 @@ // See the License for the specific language governing permissions and // limitations under the License. -///| -fn[T] UninitializedArray::set_null(self : UninitializedArray[T], index : Int) = "%fixedarray.set_null" - ///| /// An `Array` is a collection of values that supports random access and can /// grow in size. @@ -172,13 +169,15 @@ pub fn[T] Array::length(self : Array[T]) -> Int { /// - This function does not raise errors, but it panics if `new_len` is greater /// than the current length of the array. /// -/// TODO: this can be optimized by using the intrinsic to null out the range +/// # Retention +/// +/// The slots beyond `new_len` are left as they are rather than nulled out, so +/// that an `ArrayView` created before the call can never observe uninitialized +/// memory. The removed elements stay reachable from the buffer until those +/// slots are reused by later pushes, until the buffer grows, or until the +/// array is dropped; `Array::release_unused` overwrites them on demand. fn[T] Array::unsafe_truncate_to_length(self : Array[T], new_len : Int) -> Unit { - let len = self.length() - guard! new_len <= len - for i in new_len.. Unit { let old_buf = self.buf - let old_cap = old_buf.0.length() - let copy_len = if old_cap < new_capacity { old_cap } else { new_capacity } + // Only the live prefix is worth carrying over. The slots beyond `len` hold + // either NULL or the elements earlier removals left behind, and leaving them + // with the old buffer is what releases the latter when that buffer dies. + let len = self.len + let copy_len = if len < new_capacity { len } else { new_capacity } let new_buf = UninitializedArray::make_and_blit( old_buf, allocate_len=new_capacity, @@ -339,6 +341,12 @@ pub fn[T] Array::reserve_capacity(self : Array[T], capacity : Int) -> Unit { /// @test.assert_eq(v.capacity(), 3) /// } /// ``` +/// +/// The survivors are copied into the new buffer and the old one is released +/// with them, so this also releases whatever earlier removals left in the +/// unused capacity. It pays an allocation plus a copy of every survivor to do +/// so; `Array::release_unused` releases the same elements in one pass over the +/// unused region and no allocation, at the cost of leaving the capacity alone. pub fn[T] Array::shrink_to_fit(self : Array[T]) -> Unit { if self.capacity() <= self.length() { return @@ -346,6 +354,43 @@ pub fn[T] Array::shrink_to_fit(self : Array[T]) -> Unit { self.resize_buffer(self.length()) } +///| +/// Overwrites the array's unused capacity -- every slot from `length()` up to +/// `capacity()` -- with `placeholder`, releasing whatever those slots held. +/// +/// Shrinking an array never clears the slots it vacates, so whatever they held +/// -- a removed element, or a duplicate reference to a survivor that was +/// shifted over it -- stays reachable from the buffer and unreleased until +/// later pushes reuse those slots, the buffer grows, or the array is dropped. +/// This releases them on demand without reallocating, which is what +/// `Array::pop`, `Array::remove`, `Array::retain` and the other operations +/// that take no placeholder 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. +/// +/// An `ArrayView` created before the call observes `placeholder` in that region +/// afterwards, in place of whatever it held. +/// +/// This only matters for element types holding references -- for types such as +/// `Int` there is nothing to release and the call merely costs a pass over the +/// buffer. +/// +/// Example: +/// +/// ```mbt check +/// test { +/// let arr = ["a", "b", "c"] +/// let _ = arr.pop() +/// arr.release_unused(placeholder="") +/// debug_inspect(arr, content="[\"a\", \"b\"]") +/// } +/// ``` +#owned(placeholder) +pub fn[T] Array::release_unused(self : Array[T], placeholder~ : T) -> Unit { + let len = self.len + self.buf.unchecked_fill(len, placeholder, self.capacity() - len) +} + ///| /// Adds an element to the end of the array. /// @@ -531,6 +576,11 @@ pub fn[A] ArrayView::blit_to( ///| /// Removes the last element from an array and returns it, or `None` if it is empty. /// +/// The vacated slot goes on referring to the returned element, so an +/// `ArrayView` created beforehand keeps observing that element, and it is +/// released only once a later push reuses the slot, the buffer grows, or the +/// buffer is dropped. Call `Array::release_unused` to release it at once. +/// /// # Example /// ```mbt check /// test { @@ -546,7 +596,8 @@ pub fn[T] Array::pop(self : Array[T]) -> T? { } else { let index = len - 1 let v = self.unsafe_get(index) - self.buf.set_null(index) + // The slot keeps referring to `v` until it is reused or the buffer dies; + // see `unsafe_truncate_to_length` for why it is not nulled out. self.len = index Some(v) } @@ -561,6 +612,10 @@ pub fn[T] Array::pop(self : Array[T]) -> T? { /// /// Returns the last element of the array before removal. /// +/// As with `Array::pop`, the vacated slot goes on referring to the returned +/// element until a later push reuses it, the buffer grows, the buffer is +/// dropped, or `Array::release_unused` overwrites it. +/// /// Example: /// /// ```mbt check @@ -579,7 +634,6 @@ pub fn[T] Array::unsafe_pop(self : Array[T]) -> T { guard! len != 0 let index = len - 1 let v = self.unsafe_get(index) - self.buf.set_null(index) self.len = index v } @@ -632,6 +686,14 @@ pub fn[T] Array::remove(self : Array[T], index : Int) -> T { /// @test.assert_eq(v, [3, 5]) /// } /// ``` +/// +/// The `end - begin` slots vacated at the end of the array are not cleared: +/// each keeps whatever it held before the survivors were shifted down, so a +/// drained element or a duplicate reference to a survivor stays reachable +/// there until the slot is reused, the buffer grows, or the array is dropped. +/// Call `Array::release_unused` to overwrite them at once, or +/// `Array::shrink_to_fit` to move the survivors into an exact-size buffer. +/// pub fn[T] Array::drain(self : Array[T], begin : Int, end : Int) -> Array[T] { guard! begin >= 0 && end <= self.length() && begin <= end let num = end - begin diff --git a/builtin/arrayview.mbt b/builtin/arrayview.mbt index 49d938aa72..197098b11c 100644 --- a/builtin/arrayview.mbt +++ b/builtin/arrayview.mbt @@ -20,6 +20,25 @@ /// over a view keeps using those bounds even if the underlying array is later /// structurally modified. /// +/// Mutating an array while a view of it is alive is a program error. Because a +/// view keeps its original bounds and does not track the array, after such a +/// mutation it may observe elements that have since been removed, a value +/// handed to `Array::release_unused`, or the contents of a buffer the array has +/// stopped using. What it yields is always a valid value of `T` -- never +/// uninitialized memory -- but is otherwise unspecified. +/// +/// 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: +/// `clear` empties an array the same way `pop` shortens it. Two operations +/// release those elements on demand -- `Array::release_unused` overwrites the +/// unused capacity in place, and `Array::shrink_to_fit` moves the survivors +/// into an exact-size buffer and lets the old one go. +/// +/// On the JavaScript backend this guarantee does not yet hold: operations such +/// as `Array::pop` shrink the underlying JavaScript array, so a view reaching +/// past the array's current length observes `undefined`. +/// /// # Example /// /// ```mbt check diff --git a/builtin/pkg.generated.mbti b/builtin/pkg.generated.mbti index dba03664b8..ff222a7225 100644 --- a/builtin/pkg.generated.mbti +++ b/builtin/pkg.generated.mbti @@ -134,6 +134,7 @@ pub fn[T] Array::new(capacity? : Int) -> Self[T] pub fn[T] Array::pop(Self[T]) -> T? pub fn[T] Array::push(Self[T], T) -> Unit pub fn[T] Array::push_iter(Self[T], Iter[T]) -> Unit +pub fn[T] Array::release_unused(Self[T], placeholder~ : T) -> Unit pub fn[T] Array::remove(Self[T], Int) -> T pub fn[T] Array::repeat(Self[T], Int) -> Self[T] pub fn[T] Array::reserve_capacity(Self[T], Int) -> Unit