From eda74908cc3ffe2ca20efe8e8549dc8e52a32a37 Mon Sep 17 00:00:00 2001 From: Stefan Siegl Date: Fri, 19 Sep 2014 22:36:22 +0000 Subject: [PATCH] Fix module caching, closes #107 Use v8::Persistent handle to keep module instances around. Objects cannot be shared between isolates anyhow, hence moved modules_loaded map from global V8JSG structure to php_v8js_ctx. Besides fixes a use-after-free on normalised_module_id. --- php_v8js_macros.h | 3 +-- tests/commonjs_multiassign.phpt | 26 ++++++++++++++++++++++++++ v8js.cc | 13 +++++++++++-- v8js_methods.cc | 21 ++++++++++++++------- 4 files changed, 52 insertions(+), 11 deletions(-) create mode 100644 tests/commonjs_multiassign.phpt diff --git a/php_v8js_macros.h b/php_v8js_macros.h index 3628932..e593f62 100644 --- a/php_v8js_macros.h +++ b/php_v8js_macros.h @@ -200,6 +200,7 @@ struct php_v8js_ctx { zval *module_loader; std::vector modules_stack; std::vector modules_base; + std::map modules_loaded; std::map template_cache; std::map weak_objects; @@ -262,8 +263,6 @@ ZEND_BEGIN_MODULE_GLOBALS(v8js) std::mutex timer_mutex; bool timer_stop; - std::map > modules_loaded; - // fatal error unwinding bool fatal_error_abort; int error_num; diff --git a/tests/commonjs_multiassign.phpt b/tests/commonjs_multiassign.phpt new file mode 100644 index 0000000..54024ae --- /dev/null +++ b/tests/commonjs_multiassign.phpt @@ -0,0 +1,26 @@ +--TEST-- +Test V8Js::setModuleLoader : Assign result multiple times +--SKIPIF-- + +--FILE-- +setModuleLoader(function($module) { + return 'exports.bar = 23;'; +}); + +$v8->executeString($JS, 'module.js'); +?> +===EOF=== +--EXPECT-- +int(23) +int(23) +===EOF=== diff --git a/v8js.cc b/v8js.cc index 1a93d17..7313713 100644 --- a/v8js.cc +++ b/v8js.cc @@ -718,6 +718,13 @@ static void php_v8js_free_storage(void *object TSRMLS_DC) /* {{{ */ } c->php_v8js_objects.~list(); + /* Clear persistent handles in module cache */ + for (std::map::iterator it = c->modules_loaded.begin(); + it != c->modules_loaded.end(); ++it) { + it->second.Reset(); + } + c->modules_loaded.~map(); + c->isolate->Dispose(); if(c->tz != NULL) { @@ -726,6 +733,7 @@ static void php_v8js_free_storage(void *object TSRMLS_DC) /* {{{ */ c->modules_stack.~vector(); c->modules_base.~vector(); + efree(object); } /* }}} */ @@ -753,6 +761,8 @@ static zend_object_value php_v8js_new(zend_class_entry *ce TSRMLS_DC) /* {{{ */ new(&c->modules_stack) std::vector(); new(&c->modules_base) std::vector(); + new(&c->modules_loaded) std::map; + new(&c->template_cache) std::map(); new(&c->accessor_list) std::vector(); @@ -1957,11 +1967,11 @@ static PHP_GINIT_FUNCTION(v8js) v8js_globals->extensions = NULL; v8js_globals->v8_initialized = 0; v8js_globals->v8_flags = NULL; + v8js_globals->timer_thread = NULL; v8js_globals->timer_stop = false; new(&v8js_globals->timer_mutex) std::mutex; new(&v8js_globals->timer_stack) std::stack; - new(&v8js_globals->modules_loaded) std::map>; v8js_globals->fatal_error_abort = 0; v8js_globals->error_num = 0; @@ -1990,7 +2000,6 @@ static PHP_GSHUTDOWN_FUNCTION(v8js) #ifdef ZTS v8js_globals->timer_stack.~stack(); v8js_globals->timer_mutex.~mutex(); - v8js_globals->modules_loaded.~map(); #endif } /* }}} */ diff --git a/v8js_methods.cc b/v8js_methods.cc index 1800ff4..61932a3 100644 --- a/v8js_methods.cc +++ b/v8js_methods.cc @@ -245,11 +245,13 @@ V8JS_METHOD(require) } // If we have already loaded and cached this module then use it - if (V8JSG(modules_loaded).count(normalised_module_id) > 0) { + if (c->modules_loaded.count(normalised_module_id) > 0) { efree(normalised_module_id); efree(normalised_path); - info.GetReturnValue().Set(V8JSG(modules_loaded)[normalised_module_id]); + v8::Persistent newobj; + newobj.Reset(isolate, c->modules_loaded[normalised_module_id]); + info.GetReturnValue().Set(newobj); return; } @@ -324,7 +326,7 @@ V8JS_METHOD(require) v8::Locker locker(isolate); v8::Isolate::Scope isolate_scope(isolate); - v8::EscapableHandleScope handle_scope(isolate); + v8::HandleScope handle_scope(isolate); // Enter the module context v8::Context::Scope scope(context); @@ -357,12 +359,12 @@ V8JS_METHOD(require) c->modules_stack.pop_back(); c->modules_base.pop_back(); - efree(normalised_module_id); efree(normalised_path); // Script possibly terminated, return immediately if (!try_catch.CanContinue()) { info.GetReturnValue().Set(isolate->ThrowException(V8JS_SYM("Module script compile failed"))); + efree(normalised_module_id); return; } @@ -370,20 +372,25 @@ V8JS_METHOD(require) if (try_catch.HasCaught()) { // Rethrow the exception back to JS info.GetReturnValue().Set(try_catch.ReThrow()); + efree(normalised_module_id); return; } + v8::Handle newobj; + // Cache the module so it doesn't need to be compiled and run again // Ensure compatibility with CommonJS implementations such as NodeJS by playing nicely with module.exports and exports if (module->Has(V8JS_SYM("exports")) && !module->Get(V8JS_SYM("exports"))->IsUndefined()) { // If module.exports has been set then we cache this arbitrary value... - V8JSG(modules_loaded)[normalised_module_id] = handle_scope.Escape(module->Get(V8JS_SYM("exports"))->ToObject()); + newobj = module->Get(V8JS_SYM("exports"))->ToObject(); } else { // ...otherwise we cache the exports object itself - V8JSG(modules_loaded)[normalised_module_id] = handle_scope.Escape(exports); + newobj = exports; } - info.GetReturnValue().Set(V8JSG(modules_loaded)[normalised_module_id]); + c->modules_loaded[normalised_module_id].Reset(isolate, newobj); + info.GetReturnValue().Set(newobj); + efree(normalised_module_id); } void php_v8js_register_methods(v8::Handle global, php_v8js_ctx *c) /* {{{ */