This repository was archived by the owner on Nov 3, 2025. It is now read-only.
Rebox modified Array into mrb_value before alloc in shift#1323
Merged
Rebox modified Array into mrb_value before alloc in shift#1323
Array into mrb_value before alloc in shift#1323Conversation
If an `Integer` argument is given to `Array#shift`, multiple items are removed from the front of the `Array`'s underlying vector. These removed elements are returned from `Array#shift` as a new `Array`. `Array#shift` with an `Integer` argument is similar to an inverted `Vec::split_off` from the Rust std. The the array trampoline in `artichoke-backend` failed to repack the raw parts of the `Array` into the receiver `mrb_value` before allocating a new `Array` for the shifted elements. All allocations in the mruby heap can trigger a garbage collection. If the allocation of the new `Array` for the shifted elements triggered a GC, as it does in the `spec-runner` with `yaml` formatter as outlined in issue #1320, the mruby GC will attempt to mark all of the children in the original receiver. The pointers in the receiver's `RArray *` will be garbage, so attempting to index into the raw pointer will trigger undefined behavior, which was expressing as a segfault. This commit re-orders the code to ensure the receiver is repacked before attempting to allocate a new `Array` for the result of `shift`. The following invocation of the spec-runner succeeds as of this commit: ./target/debug/spec-runner --format yaml all-core-specs.toml Fixes #1320.
lopopolo
added
C-bug
Member
Author
Member
Author
|
The safety constraint that the code is failing to surface is that |
Member
Author
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
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.
If an
Integerargument is given toArray#shift, multiple items areremoved from the front of the
Array's underlying vector. These removedelements are returned from
Array#shiftas a newArray.Array#shiftwith an
Integerargument is similar to an invertedVec::split_offfromthe Rust std.
The the array trampoline in
artichoke-backendfailed to repack the rawparts of the
Arrayinto the receivermrb_valuebefore allocating anew
Arrayfor the shifted elements.All allocations in the mruby heap can trigger a garbage collection. If
the allocation of the new
Arrayfor the shifted elements triggered aGC, as it does in the
spec-runnerwithyamlformatter as outlined inissue #1320, the mruby GC will attempt to mark all of the children in
the original receiver. The pointers in the receiver's
RArray *will begarbage, so attempting to index into the raw pointer will trigger
undefined behavior, which was expressing as a segfault.
This commit re-orders the code to ensure the receiver is repacked before
attempting to allocate a new
Arrayfor the result ofshift.The following invocation of the spec-runner succeeds as of this commit:
Fixes #1320.
🎉 🧯 🎉 🧯 🎉 🧯 🎉 🧯 🎉 🧯 🎉 🧯 🎉 🧯 🎉 🧯 🎉 🧯 🎉 🧯 🎉 🧯 🎉 🧯 🎉 🧯 🎉 🧯 🎉 🧯 🎉 🧯 🎉 🧯 🎉