pm_setup_args mallocs one kw_arg buffer with references = 0. The inline_new branch then feeds that same pointer
to three callinfo constructions:
- 3789 — new_callinfo(... method_id="new" ..., kw_arg, 0) -> the opt_new ci
- 3795 — PUSH_SEND_R(... "initialize", ..., flags | VM_CALL_FCALL, kw_arg)
- 3800 — PUSH_SEND_R(... method_id="new", ..., flags, kw_arg) (fallback)
Tracing the refcount through rb_vm_ci_lookup()
1. It increments kwarg->references and allocates a fresh new_ci before the dedup st_update
2. The dedup (vm_ci_hash_cmp) compares kwarg contents, not the pointer. So if an earlier line already interned a new ci with the
identical keyword set, st_update returns that pre-existing ci (which holds a different buffer) and discards our new_ci.
3. Our kw_arg is now orphaned: references == 1, but the only holder is the discarded new_ci, which is a normal collectable imemo
4. An allocation like PUSH_INSN2 opt_new at or new_callinfo() can trigger a GC. References back to 0, kw_arg buffer freed.
5. new_callinfo() using the freed buffer, does argc += kw_arg->keyword_len (use-after-free)
The fix: Keep the buffer alive across the allocations in inline_new.
Fixes [Bug #22104] (Backport 4.0)
class.c: rb_class_duplicate_classext also dup content of cvc_tbl
[Bug #21952]
Shallow copying the table result in the same memory being shared
between multiple box, causing double free when one of the box
is garbage collected.
---
class.c: Make cvc_tbl a managed object
[Bug #21952]
Solves the double-free or use after-free concern with boxes.
Now entries can safely be used for copy-on-write.
Also is likely necessary to make it save to read cvar from
secondary ractors, as allowed since: ab32c0e690
Before this change, GC'ing any Ractor object caused you to lose all
enabled tracepoints across all ractors (even main). Now tracepoints are
ractor-local and this doesn't happen. Internal events are still global.
Fixes [Bug #19112]
to adopt strict shareable rule.
* (basically) shareable objects only refer shareable objects
* (exception) shareable objects can refere unshareable objects
but should not leak reference to unshareable objects to Ruby world
When we clone a complex imemo_fields, it calls creates the imemo_fields
using rb_imemo_fields_new_complex, which allocates and initializes a new
st_table. However, st_replace will directly replace any exisiting fields
in the st_table, causing it to leak.
For example, this script demonstrates the leak:
obj = Class.new
8.times do |i|
obj.instance_variable_set(:"@test#{i}", nil)
obj.remove_instance_variable(:"@test#{i}")
end
obj.instance_variable_set(:"@test", 1)
10.times do
100_000.times do
obj.dup
end
puts `ps -o rss= -p #{$$}`
end
Before:
26320
39296
52320
63136
75520
87008
97856
114800
120864
133504
After:
16288
20112
20416
20720
20800
20864
21184
21424
21904
21904
The imemo_fields_new function takes a capacity in the number of fields to
preallocate. rb_imemo_fields_new_complex_tbl is using it incorrectly because
it is preallocating sizeof(struct rb_fields) number of fields.
We should not assume that a complex imemo_field takes only one additional
VALUE space. This is fragile as it will break if we add additional fields
to complex imemo_field.
imemo_tmpbuf is not write-barrier protected and uses mark maybe to mark
the buffer it holds. The normal rb_imemo_new creates a write-barrier
protected object which can make the tmpbuf miss marking references.
[Bug #21547]
Followup: https://github.com/ruby/ruby/pull/14201
When adding an instance variable and the IMEMO/fields need to be
larger, we allocate a new one and clear the old one.
Since the old one may still be in other ec's cache, on a hit we must
check the IMEMO/fields isn't a stale one.
It is much more convenient than storing the klass, especially
when dealing with `object_id` as it allows to update the id2ref
table without having to dereference the owner, which may be
garbage at that point.
* Skip assertion when cc->klass is Qundef
* Invalidate CCs when cme is invalidated in marking
* Add additional assertions that CC references stay valid
Co-authored-by: Peter Zhu <peter@peterzhu.ca>
Once klass becomes Qundef, it's disconnected and won't be invalidated
when the CME is. So once that happens we must not mark or attempt to
move the cme_ field.
In multi-ractor mode, the `cc_tbl` mutations use the RCU pattern,
which allow lock-less reads.
Based on the assumption that invalidations and misses should be
increasingly rare as the process ages, locking on modification
isn't a big concern.
For now this doesn't change anything, but now that the table
is managed by GC, it opens the door to use RCU when in multi-ractor
mode, hence allow unsynchornized reads.
One of the biggest remaining contention point is `RClass.cc_table`.
The logical solution would be to turn it into a managed object, so
we can use an RCU strategy, given it's read heavy.
However, that's not currently possible because the table can't
be freed before the owning class, given the class free function
MUST go over all the CC entries to invalidate them.
However if the `CC->klass` reference is weak marked, then the
GC will take care of setting the reference to `Qundef`.
Followup: https://github.com/ruby/ruby/pull/13589
This simplify a lot of things, as we no longer need to manually
manage the memory, we can use the Read-Copy-Update pattern and
avoid numerous race conditions.
Co-Authored-By: Étienne Barrié <etienne.barrie@gmail.com>
Now that class fields have been deletated to a T_IMEMO/class_fields
when we're in multi-ractor mode, we can read and write class instance
variable in an atomic way using Read-Copy-Update (RCU).
Note when in multi-ractor mode, we always use RCU. In theory
we don't need to, instead if we ensured the field is written
before the shape is updated it would be safe.
Benchmark:
```ruby
Warning[:experimental] = false
class Foo
@foo = 1
@bar = 2
@baz = 3
@egg = 4
@spam = 5
class << self
attr_reader :foo, :bar, :baz, :egg, :spam
end
end
ractors = 8.times.map do
Ractor.new do
1_000_000.times do
Foo.bar + Foo.baz * Foo.egg - Foo.spam
end
end
end
if Ractor.method_defined?(:value)
ractors.each(&:value)
else
ractors.each(&:take)
end
```
This branch vs Ruby 3.4:
```bash
$ hyperfine -w 1 'ruby --disable-all ../test.rb' './miniruby ../test.rb'
Benchmark 1: ruby --disable-all ../test.rb
Time (mean ± σ): 3.162 s ± 0.071 s [User: 2.783 s, System: 10.809 s]
Range (min … max): 3.093 s … 3.337 s 10 runs
Benchmark 2: ./miniruby ../test.rb
Time (mean ± σ): 208.7 ms ± 4.6 ms [User: 889.7 ms, System: 6.9 ms]
Range (min … max): 202.8 ms … 222.0 ms 14 runs
Summary
./miniruby ../test.rb ran
15.15 ± 0.47 times faster than ruby --disable-all ../test.rb
```
This behave almost exactly as a T_OBJECT, the layout is entirely
compatible.
This aims to solve two problems.
First, it solves the problem of namspaced classes having
a single `shape_id`. Now each namespaced classext
has an object that can hold the namespace specific
shape.
Second, it open the door to later make class instance variable
writes atomics, hence be able to read class variables
without locking the VM.
In the future, in multi-ractor mode, we can do the write
on a copy of the `fields_obj` and then atomically swap it.
Considerations:
- Right now the `RClass` shape_id is always synchronized,
but with namespace we should likely mark classes that have
multiple namespace with a specific shape flag.
We observed T_NONE on `cc->cme_` on a --repeat-count=50 run a compaction
test on CI:
http://ci.rvm.jp/results/trunk-repeat50@ruby-sp2-noble-docker/5654900
During reference updating for imemo_callcache in
rb_imemo_mark_and_move(), if `cc->klass` is not live, but `cc->_cme` is
live and moved, we go to the vm_cc_invalidate() path which
leaves `cc->_cme` not updated and stale. In the next marking run after
compaction, CME would've become a T_NONE.
So to quote the comment above "... cc is invalidated by
`vm_cc_invalidate()` and cc->cme is not be accessed."
`struct rb_callcache *` point to an imemo object on the GC heap when
pushed into `struct rb_class_cc_entries`, but by the time vm_ccs_free()
runs, the entire GC page the imemo was on could already be deallocated.
With the right conditions, vm_ccs_free() wrote to freed memory.
rb_objspace_garbage_object_p() by itself is not enough to determine
liveness.
I conjectured this situation to be possible in
<https://github.com/ruby/ruby/pull/11995> using hints from crashes
in the wild. With c37bdfa531 ("Make
asan_poison_object poison the whole slot"), the in-tree test suite
now recreates this scenario[^1][^2][^3].
Use rb_gc_pointer_to_heap_p(). Other uses of
rb_objspace_garbage_object_p() could be making the same mistake, but
correcting them might introduce serious performance regressions, so
leave them alone for now.
[^1]: http://ci.rvm.jp/results/trunk_asan@ruby-sp1/5477412
[^2]: http://ci.rvm.jp/results/trunk_asan@ruby-sp1/5477445
[^3]: http://ci.rvm.jp/results/trunk_asan@ruby-sp1/5477448
So that it doesn't get included in the generated binaries for builds
that don't support loading shared GC modules
Co-Authored-By: Peter Zhu <peter@peterzhu.ca>
This commit splits gc.c into two files:
- gc.c now only contains code not specific to Ruby GC. This includes
code to mark objects (which the GC implementation may choose not to
use) and wrappers for internal APIs that the implementation may need
to use (e.g. locking the VM).
- gc_impl.c now contains the implementation of Ruby's GC. This includes
marking, sweeping, compaction, and statistics. Most importantly,
gc_impl.c only uses public APIs in Ruby and a limited set of functions
exposed in gc.c. This allows us to build gc_impl.c independently of
Ruby and plug Ruby's GC into itself.
"super" CC's are "orphans", meaning there is no class CC table that
points at them. Since they are orphans, we should mark the class
reference so that if the cache happens to be used, the class will still
be alive
Luke Gruber