Skip to content
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

Add patch to make WeakMap write-barrier protected, and a write barrier for WeakMap finalizer #7669

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

maruth-stripe
Copy link
Collaborator

@maruth-stripe maruth-stripe commented Feb 8, 2024

Subset of patch in Ruby 3.3

Motivation

WeakMap defines a finalizer on every value inserted into it. When this finalizer runs, it removes the correpsonding entry from the map. However, if this finalizer does not run we may have a dangling pointer. That is, the VM may allocate the empty slot corresponding to the value to a new object. Then, on performing a WeakMap lookup , we might get some random Ruby value.

This change adds a write barrier to the finalizer defined by WeakMap, which is Ruby lambda (and hence which might get garbage collected since there's no other reference to it)

The following is a minimal repro:

// foo.c

#include "ruby.h"
#include "ruby/st.h"
#include "ruby/encoding.h"
#include "ruby/thread.h"
#include "ruby/debug.h"
#include "ruby/util.h"
#include "stdio.h"

typedef struct {
} Foo;

static void foo_mark(Foo* foo) {
}

static void foo_free(Foo* foo) {
  xfree(foo);
}

static const rb_data_type_t foo_type = {
  .wrap_struct_name = "foo",
  .function =
  {
    .dfree = foo_free,
    .dmark = foo_mark,
    .dsize = NULL,
  },
  .data = NULL,
  .flags = RUBY_TYPED_FREE_IMMEDIATELY,
};

VALUE wmp = Qnil; 
ID item_get;
ID item_set;
VALUE val = Qnil;

static VALUE foo_alloc(VALUE klass) {
  Foo* foo = ALLOC(Foo);
  rb_gc_register_address(&wmp);
  VALUE wmp_klass = rb_eval_string("ObjectSpace::WeakMap");
  wmp = rb_class_new_instance(0, NULL, wmp_klass);
  item_get = rb_intern("[]");
  item_set = rb_intern("[]=");
  val = Qnil;
  return TypedData_Wrap_Struct(klass, &foo_type, foo);
}

static VALUE foo_getit(VALUE self, VALUE x) {
  Foo* foo;
  TypedData_Get_Struct(self, Foo, &foo_type, foo);
  return rb_funcall(wmp, item_get, 1, x);
}

static VALUE foo_insert_internal(VALUE self, VALUE k) {
  Foo* foo;
  TypedData_Get_Struct(self, Foo, &foo_type, foo);
  VALUE new_string = rb_str_new_cstr("insert_internal");
  val = new_string;
  // rb_objspace;
  return rb_funcall(wmp, item_set, 2, k, new_string);
}

static VALUE foo_womp(VALUE self) {
  Foo* foo;
  TypedData_Get_Struct(self, Foo, &foo_type, foo);
  VALUE new_string = rb_str_new_cstr("WOMP");
  memcpy(val, new_string, 40);
  return INT2NUM(1);
}

void Init_foo() {
  VALUE cFoo = rb_define_class("Foo", rb_cObject);
  rb_define_alloc_func(cFoo, foo_alloc);
  rb_define_method(cFoo, "insert_internal", foo_insert_internal, 1);
  rb_define_method(cFoo, "getit", foo_getit, 1);
  rb_define_method(cFoo, "womp", foo_womp, 0);
}
require_relative 'foo'

foo = Foo.new
foo.insert_internal(2)

puts "foo.getit(2) #{foo.getit(2)}"
ObjectSpace.undefine_finalizer(foo.getit(2))
GC.start

puts "foo.getit(2) #{foo.getit(2)}"

foo.womp
puts "foo #{foo.getit(2)}"

output:

foo.getit(2) insert_internal
foo.getit(2) 
foo WOMP

To run yourself, put foo.c and test.rb in a single directory, and add the following extconf.rbfile as well

require 'mkmf'
create_makefile('foo')
  • Run ruby extconf.rb
  • Run make
  • Run ruby test.rb

Test plan

See included automated tests.

@maruth-stripe maruth-stripe requested a review from a team as a code owner February 8, 2024 18:46
@maruth-stripe maruth-stripe requested review from neilparikh and removed request for a team February 8, 2024 18:46
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.

None yet

2 participants