From 754d398ec9b32ec1e468aad8ed32a34193c16d4e Mon Sep 17 00:00:00 2001 From: Stefan Siegl Date: Sat, 1 Aug 2015 17:09:48 +0200 Subject: [PATCH 01/10] Move v8js_commonjs.cc forward declarations in .h file --- v8js_commonjs.cc | 2 +- v8js_commonjs.h | 18 ++++++++++++++++++ v8js_methods.cc | 4 +--- 3 files changed, 20 insertions(+), 4 deletions(-) create mode 100644 v8js_commonjs.h diff --git a/v8js_commonjs.cc b/v8js_commonjs.cc index b737051..a3ca53f 100644 --- a/v8js_commonjs.cc +++ b/v8js_commonjs.cc @@ -22,7 +22,7 @@ extern "C" { #include "php_v8js_macros.h" -void v8js_commonjs_split_terms(char *identifier, std::vector &terms) +static void v8js_commonjs_split_terms(char *identifier, std::vector &terms) { char *term = (char *)malloc(PATH_MAX), *ptr = term; diff --git a/v8js_commonjs.h b/v8js_commonjs.h new file mode 100644 index 0000000..a2537e2 --- /dev/null +++ b/v8js_commonjs.h @@ -0,0 +1,18 @@ +/* + +----------------------------------------------------------------------+ + | PHP Version 5 | + +----------------------------------------------------------------------+ + | Copyright (c) 1997-2015 The PHP Group | + +----------------------------------------------------------------------+ + | http://www.opensource.org/licenses/mit-license.php MIT License | + +----------------------------------------------------------------------+ + | Author: Stefan Siegl | + +----------------------------------------------------------------------+ +*/ + +#ifndef V8JS_COMMONJS_H +#define V8JS_COMMONJS_H + +void v8js_commonjs_normalise_identifier(char *base, char *identifier, char *normalised_path, char *module_name); + +#endif /* V8JS_COMMONJS_H */ diff --git a/v8js_methods.cc b/v8js_methods.cc index 9c59655..48249af 100644 --- a/v8js_methods.cc +++ b/v8js_methods.cc @@ -16,14 +16,12 @@ #endif #include "php_v8js_macros.h" +#include "v8js_commonjs.h" extern "C" { #include "zend_exceptions.h" } -/* Forward declarations */ -void v8js_commonjs_normalise_identifier(char *base, char *identifier, char *normalised_path, char *module_name); - /* global.exit - terminate execution */ V8JS_METHOD(exit) /* {{{ */ { From 3d89f0250dab5b4c77732e05582729cb7bb67119 Mon Sep 17 00:00:00 2001 From: Stefan Siegl Date: Sat, 1 Aug 2015 17:49:11 +0200 Subject: [PATCH 02/10] Provide key compare function for modules_loaded Without the compare function std::map simply compares one pointer to another. However we need to compare the actual strings. Besides we must not efree normalised_module_id on return of require method, since the pointer was added to modules_loaded (and is valid until destruction of V8Js class); instead it is now freed during V8Js object destruction. --- tests/commonjs_modules_caching.phpt | 26 ++++++++++++++++++++++++++ v8js_class.cc | 3 ++- v8js_class.h | 8 +++++++- v8js_methods.cc | 1 - 4 files changed, 35 insertions(+), 3 deletions(-) create mode 100644 tests/commonjs_modules_caching.phpt diff --git a/tests/commonjs_modules_caching.phpt b/tests/commonjs_modules_caching.phpt new file mode 100644 index 0000000..6efec77 --- /dev/null +++ b/tests/commonjs_modules_caching.phpt @@ -0,0 +1,26 @@ +--TEST-- +Test V8Js::setModuleLoader : Returned modules are cached +--SKIPIF-- + +--FILE-- +setModuleLoader(function($module) { + print("setModuleLoader called for ".$module."\n"); + return 'exports.bar = 23;'; +}); + +$v8->executeString($JS, 'module.js'); +?> +===EOF=== +--EXPECT-- +setModuleLoader called for test +setModuleLoader called for test2 +===EOF=== diff --git a/v8js_class.cc b/v8js_class.cc index 924de4d..8a43a1c 100644 --- a/v8js_class.cc +++ b/v8js_class.cc @@ -156,6 +156,7 @@ static void v8js_free_storage(void *object TSRMLS_DC) /* {{{ */ /* Clear persistent handles in module cache */ for (std::map::iterator it = c->modules_loaded.begin(); it != c->modules_loaded.end(); ++it) { + efree(it->first); it->second.Reset(); } c->modules_loaded.~map(); @@ -200,7 +201,7 @@ static zend_object_value 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->modules_loaded) std::map; new(&c->template_cache) std::map(); new(&c->accessor_list) std::vector(); diff --git a/v8js_class.h b/v8js_class.h index 4500f99..aad8e96 100644 --- a/v8js_class.h +++ b/v8js_class.h @@ -23,6 +23,12 @@ typedef v8::Persistent > v8 struct v8js_v8object; struct v8js_accessor_ctx; +struct cmp_str { + bool operator()(char const *a, char const *b) const { + return strcmp(a, b) < 0; + } +}; + /* {{{ Context container */ struct v8js_ctx { zend_object std; @@ -43,7 +49,7 @@ struct v8js_ctx { zval *module_loader; std::vector modules_stack; std::vector modules_base; - std::map modules_loaded; + std::map modules_loaded; std::map template_cache; std::map weak_objects; diff --git a/v8js_methods.cc b/v8js_methods.cc index 48249af..371a01a 100644 --- a/v8js_methods.cc +++ b/v8js_methods.cc @@ -398,7 +398,6 @@ V8JS_METHOD(require) c->modules_loaded[normalised_module_id].Reset(isolate, newobj); info.GetReturnValue().Set(newobj); - efree(normalised_module_id); } void v8js_register_methods(v8::Handle global, v8js_ctx *c) /* {{{ */ From d8e239a7566bfa241808da4c7908d4eb35614945 Mon Sep 17 00:00:00 2001 From: Stefan Siegl Date: Sat, 1 Aug 2015 17:57:57 +0200 Subject: [PATCH 03/10] test per isolate module cache seperation --- ...caching.phpt => commonjs_caching_001.phpt} | 0 tests/commonjs_caching_002.phpt | 34 +++++++++++++++++++ 2 files changed, 34 insertions(+) rename tests/{commonjs_modules_caching.phpt => commonjs_caching_001.phpt} (100%) create mode 100644 tests/commonjs_caching_002.phpt diff --git a/tests/commonjs_modules_caching.phpt b/tests/commonjs_caching_001.phpt similarity index 100% rename from tests/commonjs_modules_caching.phpt rename to tests/commonjs_caching_001.phpt diff --git a/tests/commonjs_caching_002.phpt b/tests/commonjs_caching_002.phpt new file mode 100644 index 0000000..bdc34b2 --- /dev/null +++ b/tests/commonjs_caching_002.phpt @@ -0,0 +1,34 @@ +--TEST-- +Test V8Js::setModuleLoader : module cache seperated per isolate +--SKIPIF-- + +--FILE-- +setModuleLoader(function($module) { + print("setModuleLoader called for ".$module."\n"); + return 'exports.bar = 23;'; +}); + +$v8two = new V8Js(); +$v8two->setModuleLoader(function($module) { + print("setModuleLoader called for ".$module."\n"); + return 'exports.bar = 23;'; +}); + +$v8->executeString($JS, 'module.js'); +echo "--- v8two ---\n"; +$v8two->executeString($JS, 'module.js'); +?> +===EOF=== +--EXPECT-- +setModuleLoader called for test +--- v8two --- +setModuleLoader called for test +===EOF=== From 2ce7e420a75f9732f5636c8dd42a483744710c1c Mon Sep 17 00:00:00 2001 From: Stefan Siegl Date: Sat, 1 Aug 2015 18:11:39 +0200 Subject: [PATCH 04/10] Fix use-after-free on module reuse --- v8js_methods.cc | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/v8js_methods.cc b/v8js_methods.cc index 371a01a..8631cdd 100644 --- a/v8js_methods.cc +++ b/v8js_methods.cc @@ -254,12 +254,13 @@ V8JS_METHOD(require) // If we have already loaded and cached this module then use it if (c->modules_loaded.count(normalised_module_id) > 0) { - efree(normalised_module_id); - efree(normalised_path); - v8::Persistent newobj; newobj.Reset(isolate, c->modules_loaded[normalised_module_id]); info.GetReturnValue().Set(newobj); + + efree(normalised_module_id); + efree(normalised_path); + return; } From bfc6b299893a0034c3f931ec1291c4606af85ae1 Mon Sep 17 00:00:00 2001 From: Stefan Siegl Date: Sat, 1 Aug 2015 19:07:45 +0200 Subject: [PATCH 05/10] Use emalloc/estrdup/efree in v8js_commonjs.cc + fix memory leak --- v8js_commonjs.cc | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/v8js_commonjs.cc b/v8js_commonjs.cc index a3ca53f..41155b3 100644 --- a/v8js_commonjs.cc +++ b/v8js_commonjs.cc @@ -24,7 +24,7 @@ extern "C" { static void v8js_commonjs_split_terms(char *identifier, std::vector &terms) { - char *term = (char *)malloc(PATH_MAX), *ptr = term; + char *term = (char *) emalloc(PATH_MAX), *ptr = term; // Initialise the term string *term = 0; @@ -34,7 +34,7 @@ static void v8js_commonjs_split_terms(char *identifier, std::vector &ter if (strlen(term) > 0) { // Terminate term string and add to terms vector *ptr++ = 0; - terms.push_back(strdup(term)); + terms.push_back(estrdup(term)); // Reset term string memset(term, 0, strlen(term)); @@ -50,12 +50,10 @@ static void v8js_commonjs_split_terms(char *identifier, std::vector &ter if (strlen(term) > 0) { // Terminate term string and add to terms vector *ptr++ = 0; - terms.push_back(strdup(term)); + terms.push_back(estrdup(term)); } - if (term > 0) { - free(term); - } + efree(term); } void v8js_commonjs_normalise_identifier(char *base, char *identifier, char *normalised_path, char *module_name) @@ -92,6 +90,8 @@ void v8js_commonjs_normalise_identifier(char *base, char *identifier, char *norm *module_name = 0; strcat(module_name, normalised_terms.back()); + + efree(normalised_terms.back()); normalised_terms.pop_back(); for (std::vector::iterator it = normalised_terms.begin(); it != normalised_terms.end(); it++) { From ea3ec4bd65199da6b4666113a030d745f8ff7c34 Mon Sep 17 00:00:00 2001 From: Stefan Siegl Date: Sat, 1 Aug 2015 19:30:55 +0200 Subject: [PATCH 06/10] Fix commonjs memory leaks (and increase test coverage to 100%) --- tests/commonjs_normalise_001.phpt | 23 +++++++++++++++++++++++ tests/commonjs_normalise_002.phpt | 23 +++++++++++++++++++++++ tests/commonjs_normalise_003.phpt | 25 +++++++++++++++++++++++++ tests/commonjs_normalise_004.phpt | 30 ++++++++++++++++++++++++++++++ tests/commonjs_normalise_005.phpt | 30 ++++++++++++++++++++++++++++++ v8js_commonjs.cc | 10 +++++++++- 6 files changed, 140 insertions(+), 1 deletion(-) create mode 100644 tests/commonjs_normalise_001.phpt create mode 100644 tests/commonjs_normalise_002.phpt create mode 100644 tests/commonjs_normalise_003.phpt create mode 100644 tests/commonjs_normalise_004.phpt create mode 100644 tests/commonjs_normalise_005.phpt diff --git a/tests/commonjs_normalise_001.phpt b/tests/commonjs_normalise_001.phpt new file mode 100644 index 0000000..ada367f --- /dev/null +++ b/tests/commonjs_normalise_001.phpt @@ -0,0 +1,23 @@ +--TEST-- +Test V8Js::setModuleLoader : Path normalisation #001 +--SKIPIF-- + +--FILE-- +setModuleLoader(function($module) { + print("setModuleLoader called for ".$module."\n"); + return 'exports.bar = 23;'; +}); + +$v8->executeString($JS, 'module.js'); +?> +===EOF=== +--EXPECT-- +setModuleLoader called for test +===EOF=== diff --git a/tests/commonjs_normalise_002.phpt b/tests/commonjs_normalise_002.phpt new file mode 100644 index 0000000..b93dff4 --- /dev/null +++ b/tests/commonjs_normalise_002.phpt @@ -0,0 +1,23 @@ +--TEST-- +Test V8Js::setModuleLoader : Path normalisation #002 +--SKIPIF-- + +--FILE-- +setModuleLoader(function($module) { + print("setModuleLoader called for ".$module."\n"); + return 'exports.bar = 23;'; +}); + +$v8->executeString($JS, 'module.js'); +?> +===EOF=== +--EXPECT-- +setModuleLoader called for test +===EOF=== diff --git a/tests/commonjs_normalise_003.phpt b/tests/commonjs_normalise_003.phpt new file mode 100644 index 0000000..e8a95ff --- /dev/null +++ b/tests/commonjs_normalise_003.phpt @@ -0,0 +1,25 @@ +--TEST-- +Test V8Js::setModuleLoader : Path normalisation #003 +--SKIPIF-- + +--FILE-- +setModuleLoader(function($module) { + print("setModuleLoader called for ".$module."\n"); + return 'exports.bar = 23;'; +}); + +$v8->executeString($JS, 'module.js'); +?> +===EOF=== +--EXPECT-- +setModuleLoader called for foo/test +setModuleLoader called for foo/bar/baz/test +===EOF=== diff --git a/tests/commonjs_normalise_004.phpt b/tests/commonjs_normalise_004.phpt new file mode 100644 index 0000000..f8f2b8f --- /dev/null +++ b/tests/commonjs_normalise_004.phpt @@ -0,0 +1,30 @@ +--TEST-- +Test V8Js::setModuleLoader : Path normalisation #004 +--SKIPIF-- + +--FILE-- +setModuleLoader(function($module) { + print("setModuleLoader called for ".$module."\n"); + + switch($module) { + case 'foo/test': + return 'require("./blar");'; + case 'foo/blar': + return 'exports.bar = 23;'; + } +}); + +$v8->executeString($JS, 'module.js'); +?> +===EOF=== +--EXPECT-- +setModuleLoader called for foo/test +setModuleLoader called for foo/blar +===EOF=== diff --git a/tests/commonjs_normalise_005.phpt b/tests/commonjs_normalise_005.phpt new file mode 100644 index 0000000..73be992 --- /dev/null +++ b/tests/commonjs_normalise_005.phpt @@ -0,0 +1,30 @@ +--TEST-- +Test V8Js::setModuleLoader : Path normalisation #005 +--SKIPIF-- + +--FILE-- +setModuleLoader(function($module) { + print("setModuleLoader called for ".$module."\n"); + + switch($module) { + case 'foo/test': + return 'require("../blar");'; + case 'blar': + return 'exports.bar = 23;'; + } +}); + +$v8->executeString($JS, 'module.js'); +?> +===EOF=== +--EXPECT-- +setModuleLoader called for foo/test +setModuleLoader called for blar +===EOF=== diff --git a/v8js_commonjs.cc b/v8js_commonjs.cc index 41155b3..2d67e3c 100644 --- a/v8js_commonjs.cc +++ b/v8js_commonjs.cc @@ -76,12 +76,19 @@ void v8js_commonjs_normalise_identifier(char *base, char *identifier, char *norm if (!strcmp(term, "..")) { // Ignore parent term (..) if it's the first normalised term if (normalised_terms.size() > 0) { - // Remove the parent normalized term + // Remove the parent normalized term (and free it) + efree(normalised_terms.back()); normalised_terms.pop_back(); } + + // free the ".." term + efree(term); } else if (strcmp(term, ".")) { // Add the term if it's not the current term (.) normalised_terms.push_back(term); + } else { + // Discard "." term + efree(term); } } @@ -102,5 +109,6 @@ void v8js_commonjs_normalise_identifier(char *base, char *identifier, char *norm } strcat(normalised_path, term); + efree(term); } } From f61c11f9956da1dc35abb3fed2a705392240a297 Mon Sep 17 00:00:00 2001 From: Stefan Siegl Date: Sat, 1 Aug 2015 19:41:50 +0200 Subject: [PATCH 07/10] declare base & identifier const --- v8js_commonjs.cc | 4 ++-- v8js_commonjs.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/v8js_commonjs.cc b/v8js_commonjs.cc index 2d67e3c..80a8a3b 100644 --- a/v8js_commonjs.cc +++ b/v8js_commonjs.cc @@ -22,7 +22,7 @@ extern "C" { #include "php_v8js_macros.h" -static void v8js_commonjs_split_terms(char *identifier, std::vector &terms) +static void v8js_commonjs_split_terms(const char *identifier, std::vector &terms) { char *term = (char *) emalloc(PATH_MAX), *ptr = term; @@ -56,7 +56,7 @@ static void v8js_commonjs_split_terms(char *identifier, std::vector &ter efree(term); } -void v8js_commonjs_normalise_identifier(char *base, char *identifier, char *normalised_path, char *module_name) +void v8js_commonjs_normalise_identifier(const char *base, const char *identifier, char *normalised_path, char *module_name) { std::vector id_terms, terms; v8js_commonjs_split_terms(identifier, id_terms); diff --git a/v8js_commonjs.h b/v8js_commonjs.h index a2537e2..1c05fb6 100644 --- a/v8js_commonjs.h +++ b/v8js_commonjs.h @@ -13,6 +13,6 @@ #ifndef V8JS_COMMONJS_H #define V8JS_COMMONJS_H -void v8js_commonjs_normalise_identifier(char *base, char *identifier, char *normalised_path, char *module_name); +void v8js_commonjs_normalise_identifier(const char *base, const char *identifier, char *normalised_path, char *module_name); #endif /* V8JS_COMMONJS_H */ From cedcac13189e3df290fbf8a291e9b42055b53969 Mon Sep 17 00:00:00 2001 From: Stefan Siegl Date: Sat, 1 Aug 2015 19:45:21 +0200 Subject: [PATCH 08/10] skip useless estrdup/efree cycle The garbage collector cannot free the object as it is allocated on the local stack. --- v8js_methods.cc | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/v8js_methods.cc b/v8js_methods.cc index 8631cdd..2bd9013 100644 --- a/v8js_methods.cc +++ b/v8js_methods.cc @@ -221,13 +221,11 @@ V8JS_METHOD(require) v8::String::Utf8Value module_id_v8(info[0]); - // Make sure to duplicate the module name string so it doesn't get freed by the V8 garbage collector - char *module_id = estrdup(ToCString(module_id_v8)); + const char *module_id = ToCString(module_id_v8); char *normalised_path = (char *)emalloc(PATH_MAX); char *module_name = (char *)emalloc(PATH_MAX); v8js_commonjs_normalise_identifier(c->modules_base.back(), module_id, normalised_path, module_name); - efree(module_id); char *normalised_module_id = (char *)emalloc(strlen(normalised_path)+1+strlen(module_name)+1); *normalised_module_id = 0; From 441f7b7fabe2d5edd5fc66bfeb8192767e5c7aec Mon Sep 17 00:00:00 2001 From: Stefan Siegl Date: Sat, 1 Aug 2015 19:54:23 +0200 Subject: [PATCH 09/10] v8js_commonjs_split_terms: use pointer comparison instead of strlen The strlen usage on term obviously was wrong here, since the term string is not null-terminated at that place. --- tests/commonjs_normalise_003.phpt | 2 ++ v8js_commonjs.cc | 4 ++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/commonjs_normalise_003.phpt b/tests/commonjs_normalise_003.phpt index e8a95ff..7f752d4 100644 --- a/tests/commonjs_normalise_003.phpt +++ b/tests/commonjs_normalise_003.phpt @@ -8,6 +8,7 @@ Test V8Js::setModuleLoader : Path normalisation #003 $JS = <<< EOT var foo = require("foo/test"); var foo = require("foo/bar/baz/test"); +var foo = require("foo//bar//baz//blub"); EOT; $v8 = new V8Js(); @@ -22,4 +23,5 @@ $v8->executeString($JS, 'module.js'); --EXPECT-- setModuleLoader called for foo/test setModuleLoader called for foo/bar/baz/test +setModuleLoader called for foo/bar/baz/blub ===EOF=== diff --git a/v8js_commonjs.cc b/v8js_commonjs.cc index 80a8a3b..e0e09b0 100644 --- a/v8js_commonjs.cc +++ b/v8js_commonjs.cc @@ -31,7 +31,7 @@ static void v8js_commonjs_split_terms(const char *identifier, std::vector 0) { if (*identifier == '/') { - if (strlen(term) > 0) { + if (ptr > term) { // Terminate term string and add to terms vector *ptr++ = 0; terms.push_back(estrdup(term)); @@ -47,7 +47,7 @@ static void v8js_commonjs_split_terms(const char *identifier, std::vector 0) { + if (ptr > term) { // Terminate term string and add to terms vector *ptr++ = 0; terms.push_back(estrdup(term)); From 6bbe4d932b5074b96e18523ec72f96664e879bf0 Mon Sep 17 00:00:00 2001 From: Stefan Siegl Date: Sat, 1 Aug 2015 19:58:44 +0200 Subject: [PATCH 10/10] remove useless zero-initializations (immediately overridden afterwards) --- v8js_commonjs.cc | 4 ---- 1 file changed, 4 deletions(-) diff --git a/v8js_commonjs.cc b/v8js_commonjs.cc index e0e09b0..fcd59a1 100644 --- a/v8js_commonjs.cc +++ b/v8js_commonjs.cc @@ -26,9 +26,6 @@ static void v8js_commonjs_split_terms(const char *identifier, std::vector 0) { if (*identifier == '/') { if (ptr > term) { @@ -37,7 +34,6 @@ static void v8js_commonjs_split_terms(const char *identifier, std::vector