Permalink
Browse files

Fix race condition when concurrently reading and modifying header

This fixes a potential number of race conditions. The most problematic
one is the following. If we are in the middle of inflating a header and
concurrently reading another flag there's a potential problem. If we
check the header status first and it's not inflated but then read the
flags after it's inflated, the flags contain bogus information.

We fix this by making sure we always first copy the header, then check
inflation status and return this copy if it is not inflated. This means
that the code can see slightly outdated information, but it can already
handle that properly because this type of race could happen in other
places too.

We also change every modification of changing the flags to be atomic.
This is necessary to prevent certain information from being lost. If we
for example freeze an object while concurrently inflating the header, it
could be that we set the frozen bit in the header after we already
copied the flags into the inflated header. In that case setting the
frozen status would be lost.

This is not a huge problem, but the side effect is more problematic.
Since we reuse the header space as the pointer to the inflated header,
modifying a bit there would mean the inflated header would point at a
totally different inflated header. This would mean the object would
suddenly get a very different status.

These changes are related to issue #1940. In this issue the behavior
exposed itself as follows. The code resulted in a concurrent scenario
where an object header was inflated while concurrently retrieving an
instance variable. Because of this, the returned obj_type was the value
of the bits inside the inflated header pointer:

$2 = {
  <rubinius::ObjectHeader> = {
    header = {
      f = {
        inflated = 1,
        meaning = 0,
        obj_type = 217,
        zone = rubinius::UnspecifiedZone,
        age = 2,
        Forwarded = 1,
        Remember = 1,
        Marked = 1,
        InImmix = 0,
        Pinned = 0,
        Frozen = 0,
        Tainted = 1,
        Untrusted = 0,
        LockContended = 0,
        unused = 0,
        aux_word = 1
      },
      flags64 = 4312680137,
      all_flags = 0x1010e46c9
    },
    klass_ = 0x104a3ff90,
    ivars_ = 0x1a,
    __body__ = {0x0}
  }, <No data fields>}

The value here is obj_type 217, which is actually an invalid value. What
happens because of this is that the code in Object#get_ivar would see
that obj_type. This resulted in it not recognizing it as a packed object
and this not running the proper code to retrieve the instance variable.

This resulted in a nil being returned as the instance variable instead
of the actual instance variable and then resulted in a NoMethodError.
  • Loading branch information...
1 parent 87a71e6 commit c188bd649d25070e576a682ec89b819869c78ada @dbussink dbussink committed Oct 5, 2012
Showing with 271 additions and 54 deletions.
  1. +171 −23 vm/oop.cpp
  2. +100 −31 vm/oop.hpp
View
@@ -182,59 +182,215 @@ namespace rubinius {
}
unsigned int ObjectHeader::inc_age() {
- return flags().age++;
+ for(;;) {
+ if(inflated_header_p()) {
+ return inflated_header()->inc_age();
+ } else {
+ HeaderWord copy = header;
+ unsigned int new_age = copy.f.age++;
+
+ if(header.atomic_set(header, copy)) return new_age;
+ }
+ }
}
void ObjectHeader::set_age(unsigned int age) {
- flags().age = age;
+ for(;;) {
+ if(inflated_header_p()) {
+ inflated_header()->set_age(age);
+ return;
+ } else {
+ HeaderWord copy = header;
+ copy.f.age = age;
+
+ if(header.atomic_set(header, copy)) return;
+ }
+ }
}
void ObjectHeader::mark(unsigned int which) {
- flags().Marked = which;
+ for(;;) {
+ if(inflated_header_p()) {
+ inflated_header()->mark(which);
+ return;
+ } else {
+ HeaderWord copy = header;
+ copy.f.Marked = which;
+
+ if(header.atomic_set(header, copy)) return;
+ }
+ }
+ }
+
+ void ObjectHeader::clear_mark() {
+ for(;;) {
+ if(inflated_header_p()) {
+ inflated_header()->clear_mark();
+ return;
+ } else {
+ HeaderWord copy = header;
+ copy.f.Marked = 0;
+
+ if(header.atomic_set(header, copy)) return;
+ }
+ }
}
bool ObjectHeader::pin() {
// Can't pin young objects!
if(young_object_p()) return false;
- flags().Pinned = 1;
+ for(;;) {
+ if(inflated_header_p()) {
+ return inflated_header()->pin();
+ } else {
+ HeaderWord copy = header;
+ copy.f.Pinned = 1;
+
+ if(header.atomic_set(header, copy)) return true;
+ }
+ }
return true;
}
void ObjectHeader::unpin() {
- flags().Pinned = 0;
+ for(;;) {
+ if(inflated_header_p()) {
+ inflated_header()->unpin();
+ return;
+ } else {
+ HeaderWord copy = header;
+ copy.f.Pinned = 0;
+
+ if(header.atomic_set(header, copy)) return;
+ }
+ }
}
void ObjectHeader::set_in_immix() {
- flags().InImmix = 1;
+ for(;;) {
+ if(inflated_header_p()) {
+ inflated_header()->set_in_immix();
+ return;
+ } else {
+ HeaderWord copy = header;
+ copy.f.InImmix = 1;
+
+ if(header.atomic_set(header, copy)) return;
+ }
+ }
+ }
+
+ void ObjectHeader::set_zone(gc_zone zone) {
+ for(;;) {
+ if(inflated_header_p()) {
+ inflated_header()->set_zone(zone);
+ return;
+ } else {
+ HeaderWord copy = header;
+ copy.f.zone = zone;
+
+ if(header.atomic_set(header, copy)) return;
+ }
+ }
}
void ObjectHeader::set_lock_contended() {
- flags().LockContended = 1;
+ for(;;) {
+ if(inflated_header_p()) {
+ inflated_header()->set_lock_contended();
+ return;
+ } else {
+ HeaderWord copy = header;
+ copy.f.LockContended = 1;
+
+ if(header.atomic_set(header, copy)) return;
+ }
+ }
}
void ObjectHeader::clear_lock_contended() {
- flags().LockContended = 0;
+ for(;;) {
+ if(inflated_header_p()) {
+ inflated_header()->clear_lock_contended();
+ return;
+ } else {
+ HeaderWord copy = header;
+ copy.f.LockContended = 0;
+
+ if(header.atomic_set(header, copy)) return;
+ }
+ }
}
void ObjectHeader::set_remember() {
- flags().Remember = 1;
+ for(;;) {
+ if(inflated_header_p()) {
+ inflated_header()->set_remember();
+ return;
+ } else {
+ HeaderWord copy = header;
+ copy.f.Remember = 1;
+
+ if(header.atomic_set(header, copy)) return;
+ }
+ }
}
void ObjectHeader::clear_remember() {
- flags().Remember = 0;
+ for(;;) {
+ if(inflated_header_p()) {
+ inflated_header()->clear_remember();
+ return;
+ } else {
+ HeaderWord copy = header;
+ copy.f.Remember = 0;
+
+ if(header.atomic_set(header, copy)) return;
+ }
+ }
}
void ObjectHeader::set_frozen(int val) {
- flags().Frozen = val;
+ for(;;) {
+ if(inflated_header_p()) {
+ inflated_header()->set_frozen(val);
+ return;
+ } else {
+ HeaderWord copy = header;
+ copy.f.Frozen = val;
+
+ if(header.atomic_set(header, copy)) return;
+ }
+ }
}
void ObjectHeader::set_tainted(int val) {
- flags().Tainted = val;
+ for(;;) {
+ if(inflated_header_p()) {
+ inflated_header()->set_tainted(val);
+ return;
+ } else {
+ HeaderWord copy = header;
+ copy.f.Tainted = val;
+
+ if(header.atomic_set(header, copy)) return;
+ }
+ }
}
void ObjectHeader::set_untrusted(int val) {
- flags().Untrusted = val;
+ for(;;) {
+ if(inflated_header_p()) {
+ inflated_header()->set_untrusted(val);
+ return;
+ } else {
+ HeaderWord copy = header;
+ copy.f.Untrusted = val;
+
+ if(header.atomic_set(header, copy)) return;
+ }
+ }
}
void ObjectHeader::clear_handle(STATE) {
@@ -678,8 +834,6 @@ namespace rubinius {
set_age(new_age);
klass_ = other->klass_;
ivars_ = other->ivars_;
-
- clear_forwarded();
}
void ObjectHeader::initialize_full_state(VM* state, Object* other, unsigned int age) {
@@ -698,12 +852,6 @@ namespace rubinius {
header.f.aux_word = hdr.f.aux_word;
}
- //if(other->object_id() > 0) {
- // set_object_id(state, state->om, other->object_id());
- //}
-
- clear_forwarded();
-
if(other->is_tainted_p()) set_tainted();
copy_body(state, other);
@@ -713,8 +861,8 @@ namespace rubinius {
// This method is only used by the GC to move an object, so must retain
// the settings flags.
- flags().Frozen = other->flags().Frozen;
- flags().Tainted = other->flags().Tainted;
+ set_frozen(other->is_frozen_p());
+ set_tainted(other->is_tainted_p());
}
void ObjectHeader::copy_body(VM* state, Object* other) {
Oops, something went wrong.

0 comments on commit c188bd6

Please sign in to comment.