Skip to content

Update reference for RCLASS_INCLUDER during compaction #5880

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 3, 2022

Conversation

XrXr
Copy link
Member

@XrXr XrXr commented May 3, 2022

We didn't update the includer field during compaction so it could become
a dangling pointer after compaction. It's only recently that we started
to dereference the field, and we were only comparing the pointer before
then, so the omission only recently started to cause crashes.

By instrumenting object.c:833 with rp(includer);, you can see the
includer field become T_NONE with the following script:

mod = Module.new do
  protected def foo = 1
end

klass = Class.new do
  include Module.new
  def run
    foo
  end
end

klass.include(mod)

GC.verify_compaction_references(double_heap: true, toward: :empty)

klass.new.run

I found a crash in a private application that this patch fixes, but
wasn't able to develop a small reproducer. Hence the above demo that
requires instrumentation.

We didn't update the includer field during compaction so it could become
a dangling pointer after compaction. It's only recently that we started
to dereference the field, and we were only comparing the pointer before
then, so the omission only recently started to cause crashes.

By instrumenting object.c:833 with `rp(includer);`, you can see the
includer field become `T_NONE` with the following script:

```ruby
mod = Module.new do
  protected def foo = 1
end

klass = Class.new do
  include Module.new
  def run
    foo
  end
end

klass.include(mod)

GC.verify_compaction_references(double_heap: true, toward: :empty)

klass.new.run
```

I found a crash in a private application that this patch fixes, but
wasn't able to develop a small reproducer. Hence the above demo that
requires instrumentation.
@XrXr XrXr merged commit 379f5a6 into ruby:master May 3, 2022
@XrXr XrXr deleted the update-includer branch May 3, 2022 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant