From 493234c987b04d122ca28670f07f613f348c87c3 Mon Sep 17 00:00:00 2001 From: Dirkjan Bussink Date: Mon, 1 Oct 2012 09:16:29 +0200 Subject: [PATCH] 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. --- vm/capi/capi.cpp | 79 +++++++++++++++++++++++++++++----------------- vm/capi/class.cpp | 13 +++++--- vm/capi/module.cpp | 16 ++++++---- 3 files changed, 69 insertions(+), 39 deletions(-) diff --git a/vm/capi/capi.cpp b/vm/capi/capi.cpp index 72b2fe1878..5b4cdb3318 100644 --- 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()); - Arguments args_o(method, recv, block, arg_count, args); - Dispatch dis(method); + Object* ret = cNil; + + // 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(), - lookup, args_o); + 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. @@ -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()); - Arguments args_o(method, recv, cNil, arg_count, args); - Dispatch dis(method); - - Object* ret = dis.send(env->state(), env->current_call_frame(), - lookup, args_o); + Object* ret = cNil; + // Run in block so we properly deconstruct objects allocated + // on the stack if 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, 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); - - if(BlockEnvironment* be = try_as(blk)) { - ret = be->call(state, call_frame, args); - } else if(Proc* proc = try_as(blk)) { - ret = proc->yield(state, call_frame, args); - } else if(blk->nil_p()) { - state->raise_exception(Exception::make_lje(state, call_frame)); - ret = NULL; - } else { - Dispatch dis(G(sym_call)); - ret = dis.send(state, call_frame, args); + + // Run in separate block so the arguments are destructed + // properly before we make a potential longjmp. + { + Arguments args(G(sym_call), blk, arg_count, arg_vals); + + if(BlockEnvironment* be = try_as(blk)) { + ret = be->call(state, call_frame, args); + } else if(Proc* proc = try_as(blk)) { + ret = proc->yield(state, call_frame, args); + } else if(blk->nil_p()) { + 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()); - Arguments args_o(name, recv, arg_count, args); - Dispatch dis(name); - - Object* ret = dis.send(env->state(), env->current_call_frame(), - lookup, args_o); + Object* ret = cNil; + // Use a block objects on the stack are properly deconstructed when + // we do a potential longjmp. + { + LookupData lookup(recv, mod->superclass(), env->state()->globals().sym_private.get()); + 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 index 57c0491a0f..24fca5d3f6 100644 --- 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, - env->current_call_frame(), module, superclass, constant, &created); + opened_class = rubinius::Helpers::open_class(env->state(), gct, + 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 index 7e665ddf85..5cea3d5002 100644 --- 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(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, - env->current_call_frame(), parent, constant); + module = rubinius::Helpers::open_module(env->state(), gct, + 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.