Use a block around OnStack with setjmp / longjmp for C-API exceptions

This way we ensure that the destructor for OnStack is executed, even when we encounter an exception. We use setjmp / longjmp in the C-API but that means we can't depend on RAII behavior here. By moving this into a separate block we make sure the destructor part of RAII runs before we do the longjmp.
voxik · Oct 1, 2012 · 493234c · 493234c
1 parent 0af7440
commit 493234c
Show file tree

Hide file tree

Showing 3 changed files with 69 additions and 39 deletions.
diff --git a/vm/capi/capi.cpp b/vm/capi/capi.cpp
@@ -200,12 +200,18 @@ namespace rubinius {
       // Unlock, we're leaving extension code.
       LEAVE_CAPI(env->state());
 
-      LookupData lookup(recv, recv->lookup_begin(env->state()), env->state()->globals().sym_private.get());
+      Object* ret = cNil;
-      Arguments args_o(method, recv, block, arg_count, args);
+
-      Dispatch dis(method);
+      // Run in a block so objects are properly deconstructed when we
+      // do a longjmp because of an exception.
+      {
+        LookupData lookup(recv, recv->lookup_begin(env->state()), env->state()->globals().sym_private.get());
+        Arguments args_o(method, recv, block, arg_count, args);
+        Dispatch dis(method);
 
-      Object* ret = dis.send(env->state(), env->current_call_frame(),
+        ret = dis.send(env->state(), env->current_call_frame(),
-                             lookup, args_o);
+                      lookup, args_o);
+      }
 
       // We need to get the handle for the return value before getting
       // the GEL so that ret isn't accidentally GCd while we wait.
@@ -244,12 +250,17 @@ namespace rubinius {
       Object* recv = env->get_object(receiver);
       Symbol* method = (Symbol*)method_name;
 
-      LookupData lookup(recv, recv->lookup_begin(env->state()), env->state()->globals().sym_private.get());
+      Object* ret = cNil;
-      Arguments args_o(method, recv, cNil, arg_count, args);
+      // Run in block so we properly deconstruct objects allocated
-      Dispatch dis(method);
+      // on the stack if we do a longjmp because of an exception.
-
+      {
-      Object* ret = dis.send(env->state(), env->current_call_frame(),
+        LookupData lookup(recv, recv->lookup_begin(env->state()), env->state()->globals().sym_private.get());
-                             lookup, args_o);
+        Arguments args_o(method, recv, cNil, arg_count, args);
+        Dispatch dis(method);
+
+        ret = dis.send(env->state(), env->current_call_frame(),
+                       lookup, args_o);
+      }
 
       // We need to get the handle for the return value before getting
       // the GEL so that ret isn't accidentally GCd while we wait.
@@ -283,18 +294,23 @@ namespace rubinius {
       STATE = env->state();
 
       CallFrame* call_frame = env->current_call_frame();
-      Arguments args(G(sym_call), blk, arg_count, arg_vals);
+
-
+      // Run in separate block so the arguments are destructed
-      if(BlockEnvironment* be = try_as<BlockEnvironment>(blk)) {
+      // properly before we make a potential longjmp.
-        ret = be->call(state, call_frame, args);
+      {
-      } else if(Proc* proc = try_as<Proc>(blk)) {
+        Arguments args(G(sym_call), blk, arg_count, arg_vals);
-        ret = proc->yield(state, call_frame, args);
+
-      } else if(blk->nil_p()) {
+        if(BlockEnvironment* be = try_as<BlockEnvironment>(blk)) {
-        state->raise_exception(Exception::make_lje(state, call_frame));
+          ret = be->call(state, call_frame, args);
-        ret = NULL;
+        } else if(Proc* proc = try_as<Proc>(blk)) {
-      } else {
+          ret = proc->yield(state, call_frame, args);
-        Dispatch dis(G(sym_call));
+        } else if(blk->nil_p()) {
-        ret = dis.send(state, call_frame, args);
+          state->raise_exception(Exception::make_lje(state, call_frame));
+          ret = NULL;
+        } else {
+          Dispatch dis(G(sym_call));
+          ret = dis.send(state, call_frame, args);
+        }
       }
 
       // We need to get the handle for the return value before getting
@@ -332,12 +348,17 @@ namespace rubinius {
       // Unlock, we're leaving extension code.
       LEAVE_CAPI(env->state());
 
-      LookupData lookup(recv, mod->superclass(), env->state()->globals().sym_private.get());
+      Object* ret = cNil;
-      Arguments args_o(name, recv, arg_count, args);
+      // Use a block objects on the stack are properly deconstructed when
-      Dispatch dis(name);
+      // we do a potential longjmp.
-
+      {
-      Object* ret = dis.send(env->state(), env->current_call_frame(),
+        LookupData lookup(recv, mod->superclass(), env->state()->globals().sym_private.get());
-                             lookup, args_o);
+        Arguments args_o(name, recv, arg_count, args);
+        Dispatch dis(name);
+
+        ret = dis.send(env->state(), env->current_call_frame(),
+                       lookup, args_o);
+      }
 
       // We need to get the handle for the return value before getting
       // the GEL so that ret isn't accidentally GCd while we wait.

diff --git a/vm/capi/class.cpp b/vm/capi/class.cpp
@@ -143,7 +143,6 @@ extern "C" {
   /** @note   Shares code with rb_define_module_under, change there too. --rue */
   VALUE rb_define_class_under(VALUE outer, const char* name, VALUE super) {
 
-    GCTokenImpl gct;
 
     NativeMethodEnvironment* env = NativeMethodEnvironment::get();
 
@@ -154,11 +153,17 @@ extern "C" {
     bool created = false;
 
     LEAVE_CAPI(env->state());
+    Class* opened_class = NULL;
 
-    OnStack<3> os(env->state(), module, superclass, constant);
+    // Run in a block so OnStack is properly deallocated before we
+    // might do a longjmp because of an exception.
+    {
+      GCTokenImpl gct;
+      OnStack<3> os(env->state(), module, superclass, constant);
 
-    Class* opened_class = rubinius::Helpers::open_class(env->state(), gct,
+      opened_class = rubinius::Helpers::open_class(env->state(), gct,
-        env->current_call_frame(), module, superclass, constant, &created);
+          env->current_call_frame(), module, superclass, constant, &created);
+    }
 
     // The call above could have triggered an Autoload resolve, which may
     // raise an exception, so we have to check the value returned.

diff --git a/vm/capi/module.cpp b/vm/capi/module.cpp
@@ -210,20 +210,24 @@ extern "C" {
 
   /** @note   Shares code with rb_define_class_under, change there too. --rue */
   VALUE rb_define_module_under(VALUE parent_handle, const char* name) {
-
-    GCTokenImpl gct;
-
     NativeMethodEnvironment* env = NativeMethodEnvironment::get();
 
     Module* parent = c_as<Module>(env->get_object(parent_handle));
     Symbol* constant = env->state()->symbol(name);
 
     LEAVE_CAPI(env->state());
+    Module* module = NULL;
 
-    OnStack<2> os(env->state(), parent, constant);
+    // Create a scope so we know that the OnStack variables are popped off
+    // before we possibly make a longjmp. Making a longjmp doesn't give
+    // any guarantees about destructors being run
+    {
+      GCTokenImpl gct;
+      OnStack<2> os(env->state(), parent, constant);
 
-    Module* module = rubinius::Helpers::open_module(env->state(), gct,
+      module = rubinius::Helpers::open_module(env->state(), gct,
-        env->current_call_frame(), parent, constant);
+          env->current_call_frame(), parent, constant);
+    }
 
     // The call above could have triggered an Autoload resolve, which may
     // raise an exception, so we have to check the value returned.