From a9fbe2ea101d18d6da488426d4f86b27225fca56 Mon Sep 17 00:00:00 2001 From: Bogdan Padalko Date: Sat, 13 Aug 2016 19:30:13 +0300 Subject: [PATCH 1/2] Fix templates recursion --- README.md | 29 ++----- src/php_v8_function_template.cc | 4 + src/php_v8_function_template.h | 3 + src/php_v8_object_template.cc | 4 + src/php_v8_object_template.h | 20 ++--- src/php_v8_template.cc | 23 +++++- src/php_v8_template.h | 61 +++++++++++++++ .../003-V8ObjectTemplate_recursive_chain.phpt | 39 ++++++++++ ...03-V8ObjectTemplate_recursive_global.phpt} | 14 +++- ... 003-V8ObjectTemplate_recursive_self.phpt} | 16 ++-- .../003-V8ObjectTemplate_recursive_tree.phpt | 77 +++++++++++++++++++ 11 files changed, 246 insertions(+), 44 deletions(-) create mode 100644 tests/003-V8ObjectTemplate_recursive_chain.phpt rename tests/{V8ObjectTemplate_recursive_global.phpt => 003-V8ObjectTemplate_recursive_global.phpt} (62%) rename tests/{V8ObjectTemplate_recursive.phpt => 003-V8ObjectTemplate_recursive_self.phpt} (62%) create mode 100644 tests/003-V8ObjectTemplate_recursive_tree.phpt diff --git a/README.md b/README.md index 46821fb..2088d3e 100644 --- a/README.md +++ b/README.md @@ -26,11 +26,11 @@ provides an accurate native V8 C++ API implementation available from PHP. - multiple isolates and contexts at the same time; - it works; -With this extension almost all what native V8 C++ API provides can be used. It provides a way to pass php scalars, +With this extension almost all that native V8 C++ API provides can be used. It provides a way to pass php scalars, objects and function to V8 runtime and specify interaction with passed values (objects and functions only, as scalars become js scalars too). While specific functionality will be done in PHP userland rather then in C/C++ this extension, -it let get into V8 hacking faster, reduces time costs and let have more maintainable solution. And it doesn't make any -assumptions for you so you are the boss, it does exactly what you ask for. +it lets you get into V8 hacking faster, reduces time costs and gives you a more maintainable solution. And it doesn't +make any assumptions for you, so you stay in control, it does exactly what you ask it to do. With php-v8 you can even implement nodejs in PHP. Not sure whether anyone should/will do this anyway, but it's doable. @@ -55,8 +55,8 @@ $result = $script->Run($context); echo $result->ToString($context)->Value(), PHP_EOL; ``` -which will output `Hello, World!`. See how it shorter and readable from that C++ version? And it also doesn't limit you -from V8 API utilizing to implement more amazing stuff. +which will output `Hello, World!`. See how it's shorter and readable from that C++ version? And it also doesn't limit +you from V8 API utilizing to implement more amazing stuff. ## Installation @@ -103,25 +103,6 @@ $ sudo make install - To track memory usage you may want to use `smem`, `pmem` and even `lsof` to see what shared object are loaded and `free` to display free and used memory in the system. - -## Edge cases: - -### Templates recursion: - -When you set property on any `Template` (`ObjectTemplate` or `FunctionTemplate`) it shouldn't lead to recursion during -template instantiation while it leads to segfault and for now there are no reasonable way to avoid this on extension -level (probably, some wrapper around `ObjectTemplate` and `FunctionTemplate` will solve this. - -Known issues demo: - -```php -$isolate = new V8\Isolate(); - -$template = new V8\ObjectTemplate($isolate); - -$template->Set('self', $template); // leads to segfault -``` - ## License Copyright (c) 2015-2016 Bogdan Padalko <pinepain@gmail.com> diff --git a/src/php_v8_function_template.cc b/src/php_v8_function_template.cc index ac5bd0f..02895bd 100644 --- a/src/php_v8_function_template.cc +++ b/src/php_v8_function_template.cc @@ -104,6 +104,8 @@ static void php_v8_function_template_free(zend_object *object) { } } + delete php_v8_function_template->node; + if (php_v8_function_template->gc_data) { efree(php_v8_function_template->gc_data); } @@ -122,6 +124,8 @@ static zend_object * php_v8_function_template_ctor(zend_class_entry *ce) { php_v8_function_template->persistent = new v8::Persistent(); php_v8_function_template->callbacks = new php_v8_callbacks_t(); + php_v8_function_template->node = new phpv8::TemplateNode(); + php_v8_function_template->std.handlers = &php_v8_function_template_object_handlers; return &php_v8_function_template->std; diff --git a/src/php_v8_function_template.h b/src/php_v8_function_template.h index 35f4cd3..cea5543 100644 --- a/src/php_v8_function_template.h +++ b/src/php_v8_function_template.h @@ -17,6 +17,7 @@ typedef struct _php_v8_function_template_t php_v8_function_template_t; +#include "php_v8_template.h" #include "php_v8_exceptions.h" #include "php_v8_isolate.h" #include @@ -61,6 +62,8 @@ struct _php_v8_function_template_t { zval *gc_data; int gc_data_count; + phpv8::TemplateNode *node; + zend_object std; }; diff --git a/src/php_v8_object_template.cc b/src/php_v8_object_template.cc index 721390c..d6a64ab 100644 --- a/src/php_v8_object_template.cc +++ b/src/php_v8_object_template.cc @@ -98,6 +98,8 @@ static void php_v8_object_template_free(zend_object *object) { } } + delete php_v8_object_template->node; + if (php_v8_object_template->gc_data) { efree(php_v8_object_template->gc_data); } @@ -116,6 +118,8 @@ static zend_object * php_v8_object_template_ctor(zend_class_entry *ce) { php_v8_object_template->persistent = new v8::Persistent(); php_v8_object_template->callbacks = new php_v8_callbacks_t(); + php_v8_object_template->node = new phpv8::TemplateNode(); + php_v8_object_template->std.handlers = &php_v8_object_template_object_handlers; return &php_v8_object_template->std; diff --git a/src/php_v8_object_template.h b/src/php_v8_object_template.h index b3a1ec7..57986ff 100644 --- a/src/php_v8_object_template.h +++ b/src/php_v8_object_template.h @@ -17,6 +17,7 @@ typedef struct _php_v8_object_template_t php_v8_object_template_t; +#include "php_v8_template.h" #include "php_v8_exceptions.h" #include "php_v8_template.h" #include "php_v8_isolate.h" @@ -53,20 +54,21 @@ extern php_v8_object_template_t * php_v8_object_template_fetch_object(zend_objec PHP_V8_COPY_POINTER_TO_ISOLATE((to_php_v8_val), (from_php_v8_val)); - struct _php_v8_object_template_t { - php_v8_isolate_t *php_v8_isolate; + php_v8_isolate_t *php_v8_isolate; + + uint32_t isolate_handle; - uint32_t isolate_handle; + bool is_weak; + v8::Persistent *persistent; + php_v8_callbacks_t *callbacks; - bool is_weak; - v8::Persistent *persistent; - php_v8_callbacks_t *callbacks; + zval *gc_data; + int gc_data_count; - zval *gc_data; - int gc_data_count; + phpv8::TemplateNode *node; - zend_object std; + zend_object std; }; diff --git a/src/php_v8_template.cc b/src/php_v8_template.cc index ba7a2e1..d2b6c21 100644 --- a/src/php_v8_template.cc +++ b/src/php_v8_template.cc @@ -102,6 +102,21 @@ void php_v8_function_template_SetNativeDataProperty(INTERNAL_FUNCTION_PARAMETERS php_v8_template_SetNativeDataProperty(isolate, local_template, php_v8_template, INTERNAL_FUNCTION_PARAM_PASSTHRU); } +template +static inline bool php_v8_template_node_set(M *parent, N *child) { + if (parent->node->isSelf(child->node)) { + PHP_V8_THROW_EXCEPTION("Can't set: recursion detected"); + return false; + } + + if (parent->node->isParent(child->node)) { + PHP_V8_THROW_EXCEPTION("Can't set: recursion detected"); + return false; + } + + parent->node->addChild(child->node); + return true; +} template void php_v8_template_Set(v8::Isolate *isolate, v8::Local local_template, N* php_v8_template, INTERNAL_FUNCTION_PARAMETERS) { @@ -135,7 +150,9 @@ void php_v8_template_Set(v8::Isolate *isolate, v8::Local local_template, N* p PHP_V8_DATA_ISOLATES_CHECK(php_v8_template, php_v8_object_template_to_set); - local_template->Set(local_name, php_v8_object_template_get_local(isolate, php_v8_object_template_to_set), static_cast(attributes)); + if (php_v8_template_node_set(php_v8_template, php_v8_object_template_to_set)) { + local_template->Set(local_name, php_v8_object_template_get_local(isolate, php_v8_object_template_to_set), static_cast(attributes)); + } } else if (instanceof_function(Z_OBJCE_P(php_v8_value_zv), php_v8_function_template_class_entry)) { // set function template @@ -143,7 +160,9 @@ void php_v8_template_Set(v8::Isolate *isolate, v8::Local local_template, N* p PHP_V8_DATA_ISOLATES_CHECK(php_v8_template, php_v8_function_template_to_set); - local_template->Set(local_name, php_v8_function_template_get_local(isolate, php_v8_function_template_to_set), static_cast(attributes)); + if (php_v8_template_node_set(php_v8_template, php_v8_function_template_to_set)) { + local_template->Set(local_name, php_v8_function_template_get_local(isolate, php_v8_function_template_to_set), static_cast(attributes)); + } } else { // should never get here PHP_V8_THROW_EXCEPTION("Unknown type to set"); diff --git a/src/php_v8_template.h b/src/php_v8_template.h index 1bd7756..3834e1f 100644 --- a/src/php_v8_template.h +++ b/src/php_v8_template.h @@ -15,6 +15,10 @@ #ifndef PHP_V8_TEMPLATE_H #define PHP_V8_TEMPLATE_H +namespace phpv8 { + class TemplateNode; +} + extern "C" { #include "php.h" @@ -23,6 +27,8 @@ extern "C" { #endif } +#include + extern zend_class_entry* php_v8_template_ce; extern void php_v8_object_template_Set(INTERNAL_FUNCTION_PARAMETERS); @@ -37,6 +43,61 @@ extern void php_v8_function_template_SetNativeDataProperty(INTERNAL_FUNCTION_PAR #define PHP_V8_TEMPLATE_STORE_ISOLATE(to_zval, from_isolate_zv) zend_update_property(php_v8_template_ce, (to_zval), ZEND_STRL("isolate"), (from_isolate_zv)); #define PHP_V8_TEMPLATE_READ_ISOLATE(from_zval) zend_read_property(php_v8_template_ce, (from_zval), ZEND_STRL("isolate"), 0, &rv) +namespace phpv8 { + class TemplateNode { + public: + std::set children; + std::set parents; + + bool isSelf(TemplateNode *node) { + return this == node; + } + + bool isParent(TemplateNode *node) { + if (parents.find(node) != parents.end()) { + return true; + } + + for (TemplateNode *parent : parents) { + if (parent->isParent(node)) { + return true; + } + } + + return false; + } + + //bool isChild(TemplateNode *node) { + // if (children.find(node) != children.end()) { + // return true; + // } + // + // for (TemplateNode *child : children) { + // if (child->isChild(node)) { + // return true; + // } + // } + // + // return false; + //} + + void addChild(TemplateNode *node) { + children.insert(node); + node->parents.insert(this); + } + + ~TemplateNode() { + for (TemplateNode *parent : parents) { + parent->children.erase(this); + } + + for (TemplateNode *child : children) { + child->parents.erase(this); + } + } + }; +} + PHP_MINIT_FUNCTION(php_v8_template); diff --git a/tests/003-V8ObjectTemplate_recursive_chain.phpt b/tests/003-V8ObjectTemplate_recursive_chain.phpt new file mode 100644 index 0000000..0b35405 --- /dev/null +++ b/tests/003-V8ObjectTemplate_recursive_chain.phpt @@ -0,0 +1,39 @@ +--TEST-- +V8\ObjectTemplate - recursive 2 +--SKIPIF-- + +--FILE-- +Set(new \V8\StringValue($isolate, 'that2'), $template2); +$template2->Set(new \V8\StringValue($isolate, 'that3'), $template3); + +try { + $template3->Set(new \V8\StringValue($isolate, 'that1'), $template2); +} catch (GenericException $e) { + $helper->exception_export($e); +} + + +$context = new \V8\Context($isolate); +$context->GlobalObject()->Set($context, new \V8\StringValue($isolate, 'test'), $template1->NewInstance($context)); + +?> +--EXPECT-- +V8\Exceptions\GenericException: Can't set: recursion detected diff --git a/tests/V8ObjectTemplate_recursive_global.phpt b/tests/003-V8ObjectTemplate_recursive_global.phpt similarity index 62% rename from tests/V8ObjectTemplate_recursive_global.phpt rename to tests/003-V8ObjectTemplate_recursive_global.phpt index f881a3f..f1b89e9 100644 --- a/tests/V8ObjectTemplate_recursive_global.phpt +++ b/tests/003-V8ObjectTemplate_recursive_global.phpt @@ -2,9 +2,11 @@ V8\ObjectTemplate --SKIPIF-- - --FILE-- Set(new \V8\StringValue($isolate, 'self'), $template); + +try { + $template->Set(new \V8\StringValue($isolate, 'self'), $template); +} catch (GenericException $e) { + $helper->exception_export($e); +} $context = new \V8\Context($isolate, [], $template); ?> ---XFAIL-- -Recursive templates are known to segfault --EXPECT-- +V8\Exceptions\GenericException: Can't set: recursion detected diff --git a/tests/V8ObjectTemplate_recursive.phpt b/tests/003-V8ObjectTemplate_recursive_self.phpt similarity index 62% rename from tests/V8ObjectTemplate_recursive.phpt rename to tests/003-V8ObjectTemplate_recursive_self.phpt index aaeaedf..c7f7587 100644 --- a/tests/V8ObjectTemplate_recursive.phpt +++ b/tests/003-V8ObjectTemplate_recursive_self.phpt @@ -1,10 +1,12 @@ --TEST-- -V8\ObjectTemplate +V8\ObjectTemplate::Set() - recursive self --SKIPIF-- - --FILE-- Set(new \V8\StringValue($isolate, 'self'), $template); + +try { + $template->Set(new \V8\StringValue($isolate, 'self'), $template); +} catch (GenericException $e) { + $helper->exception_export($e); +} $context = new \V8\Context($isolate); $context->GlobalObject()->Set($context, new \V8\StringValue($isolate, 'test'), $template->NewInstance($context)); ?> ---XFAIL-- -Recursive templates are known to segfault --EXPECT-- +V8\Exceptions\GenericException: Can't set: recursion detected diff --git a/tests/003-V8ObjectTemplate_recursive_tree.phpt b/tests/003-V8ObjectTemplate_recursive_tree.phpt new file mode 100644 index 0000000..273bb3d --- /dev/null +++ b/tests/003-V8ObjectTemplate_recursive_tree.phpt @@ -0,0 +1,77 @@ +--TEST-- +V8\ObjectTemplate::Set() - recursive tree +--SKIPIF-- + +--FILE-- +Set($s2, $t2); + +$t2->Set($s3, $t3); +$t2->Set($s4, $t4); + +$t1->Set($s5, $t5); + +$t5->Set($s6, $t6); +$t5->Set($s7, $t7); + +$t7->Set($s8, $t8); +$t7->Set($s9, $t9); + +try { + $t9->Set($s1, $t1); +} catch (\V8\Exceptions\GenericException $e) { + $helper->exception_export($e); +} + + +try { + $t7->Set($s1, $t1); +} catch (\V8\Exceptions\GenericException $e) { + $helper->exception_export($e); +} + +$t4->Set($s6, $t6); + +try { + $t6->Set($s4, $t4); +} catch (\V8\Exceptions\GenericException $e) { + $helper->exception_export($e); +} + +$context = new \V8\Context($isolate); +$context->GlobalObject()->Set($context, new \V8\StringValue($isolate, 'test'), $t1->NewInstance($context)); + +?> +--EXPECT-- +V8\Exceptions\GenericException: Can't set: recursion detected +V8\Exceptions\GenericException: Can't set: recursion detected +V8\Exceptions\GenericException: Can't set: recursion detected From 090ca9c0809a1e8f3886190606672aa5d4d98ea1 Mon Sep 17 00:00:00 2001 From: Bogdan Padalko Date: Sat, 13 Aug 2016 22:59:13 +0300 Subject: [PATCH 2/2] Increase timeout upper limit to pass tests under valgrind --- tests/.testsuite.php | 5 +++++ tests/V8Isolate_limit_time.phpt | 9 +++------ tests/V8Isolate_limit_time_nested.phpt | 9 +++------ tests/V8Isolate_limit_time_set_during_execution.phpt | 9 +++------ 4 files changed, 14 insertions(+), 18 deletions(-) diff --git a/tests/.testsuite.php b/tests/.testsuite.php index 496c865..e25466e 100644 --- a/tests/.testsuite.php +++ b/tests/.testsuite.php @@ -267,6 +267,11 @@ public function dump_object_methods($object, array $arguments = [], FilterInterf echo $rc->getName(), $info, $access, $m->getName(), '():', $final_res, PHP_EOL; } } + + public function need_more_time() { + // NOTE: this check is a bit fragile but should fits our need + return isset($_ENV['TRAVIS']) && isset($_ENV['TEST_PHP_ARGS']) && $_ENV['TEST_PHP_ARGS'] == '-m'; + } } interface FilterInterface diff --git a/tests/V8Isolate_limit_time.phpt b/tests/V8Isolate_limit_time.phpt index d056d15..0cdcc71 100644 --- a/tests/V8Isolate_limit_time.phpt +++ b/tests/V8Isolate_limit_time.phpt @@ -25,18 +25,15 @@ $file_name = 'test.js'; $script = new V8\Script($context, new \V8\StringValue($isolate, $source), new \V8\ScriptOrigin($file_name)); -// NOTE: this check is a bit fragile but should fits our need -$needs_more_time = isset($_ENV['TRAVIS']) && isset($_ENV['TEST_PHP_ARGS']) && $_ENV['TEST_PHP_ARGS'] == '-m'; - -if ($needs_more_time) { +if ($helper->need_more_time()) { // On travis when valgrind active it takes more time to complete all operations so we just increase initial limits $time_limit = 5.0; $low_range = 4.5; - $high_range = 7.5; + $high_range = 10.0; } else { $time_limit = 1.5; $low_range = 1.45; - $high_range = 1.6; + $high_range = 1.65; } $helper->assert('Time limit accessor report no hit', false === $isolate->IsTimeLimitHit()); diff --git a/tests/V8Isolate_limit_time_nested.phpt b/tests/V8Isolate_limit_time_nested.phpt index 4eaea3d..32683a7 100644 --- a/tests/V8Isolate_limit_time_nested.phpt +++ b/tests/V8Isolate_limit_time_nested.phpt @@ -57,18 +57,15 @@ $file_name1 = 'test.js'; $script1 = new V8\Script($context1, new \V8\StringValue($isolate1, $source1), new \V8\ScriptOrigin($file_name1)); -// NOTE: this check is a bit fragile but should fits our need -$needs_more_time = isset($_ENV['TRAVIS']) && isset($_ENV['TEST_PHP_ARGS']) && $_ENV['TEST_PHP_ARGS'] == '-m'; - -if ($needs_more_time) { +if ($helper->need_more_time()) { // On travis when valgrind active it takes more time to complete all operations so we just increase initial limits $time_limit = 5.0; $low_range = 4.5; - $high_range = 7.5; + $high_range = 10.0; } else { $time_limit = 1.5; $low_range = 1.45; - $high_range = 1.6; + $high_range = 1.65; } $isolate1->SetTimeLimit($time_limit); diff --git a/tests/V8Isolate_limit_time_set_during_execution.phpt b/tests/V8Isolate_limit_time_set_during_execution.phpt index e1ccd6b..00abfb5 100644 --- a/tests/V8Isolate_limit_time_set_during_execution.phpt +++ b/tests/V8Isolate_limit_time_set_during_execution.phpt @@ -17,18 +17,15 @@ $global_template1 = new V8\ObjectTemplate($isolate1); $context1 = new V8\Context($isolate1, $extensions1, $global_template1); -// NOTE: this check is a bit fragile but should fits our need -$needs_more_time = isset($_ENV['TRAVIS']) && isset($_ENV['TEST_PHP_ARGS']) && $_ENV['TEST_PHP_ARGS'] == '-m'; - -if ($needs_more_time) { +if ($helper->need_more_time()) { // On travis when valgrind active it takes more time to complete all operations so we just increase initial limits $time_limit = 5.0; $low_range = 4.5; - $high_range = 7.5; + $high_range = 10.0; } else { $time_limit = 1.5; $low_range = 1.45; - $high_range = 1.6; + $high_range = 1.65; } $func = new V8\FunctionObject($context1, function (\V8\FunctionCallbackInfo $info) use (&$helper, $time_limit) {