Merge pull request #1 from pinepain/fix_templates_recursion

Fix templates recursion

Author: pinepain

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>

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<v8::FunctionTemplate>();
     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;

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 <v8.h>
@@ -61,6 +62,8 @@ struct _php_v8_function_template_t {
     zval *gc_data;
     int gc_data_count;
 
+    phpv8::TemplateNode *node;
+
     zend_object std;
 };
 

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<v8::ObjectTemplate>();
     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;

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<v8::ObjectTemplate> *persistent;
+    php_v8_callbacks_t *callbacks;
 
-  bool is_weak;
-  v8::Persistent<v8::ObjectTemplate> *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;
 };
 
 

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<typename M, typename N>
+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<class T, typename N>
 void php_v8_template_Set(v8::Isolate *isolate, v8::Local<T> local_template, N* php_v8_template, INTERNAL_FUNCTION_PARAMETERS) {
@@ -135,15 +150,19 @@ void php_v8_template_Set(v8::Isolate *isolate, v8::Local<T> 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<v8::PropertyAttribute>(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<v8::PropertyAttribute>(attributes));
+        }
 
     } else if (instanceof_function(Z_OBJCE_P(php_v8_value_zv), php_v8_function_template_class_entry)) {
         // set function template
         PHP_V8_FETCH_FUNCTION_TEMPLATE_WITH_CHECK(php_v8_value_zv, php_v8_function_template_to_set);
 
         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<v8::PropertyAttribute>(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<v8::PropertyAttribute>(attributes));
+        }
     } else {
         // should never get here
         PHP_V8_THROW_EXCEPTION("Unknown type to set");

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 <set>
+
 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<TemplateNode *> children;
+        std::set<TemplateNode *> 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);
 

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

tests/003-V8ObjectTemplate_recursive_chain.phpt

@@ -0,0 +1,39 @@
+--TEST--
+V8\ObjectTemplate - recursive 2
+--SKIPIF--
+<?php if (!extension_loaded("v8")) print "skip"; ?>
+--FILE--
+<?php
+
+use V8\Exceptions\GenericException;
+
+/** @var \Phpv8Testsuite $helper */
+$helper = require '.testsuite.php';
+
+require '.v8-helpers.php';
+$v8_helper = new PhpV8Helpers($helper);
+
+// Tests:
+
+$isolate = new \V8\Isolate();
+
+$template1 = new \V8\ObjectTemplate($isolate);
+$template2 = new \V8\ObjectTemplate($isolate);
+$template3 = new \V8\ObjectTemplate($isolate);
+
+$template1->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

tests/V8ObjectTemplate_recursive_global.phpt renamed to tests/003-V8ObjectTemplate_recursive_global.phpt

@@ -2,9 +2,11 @@
 V8\ObjectTemplate
 --SKIPIF--
 <?php if (!extension_loaded("v8")) print "skip"; ?>
-<?php print "skip this test is known to fail and it hangs on travis"; ?>
 --FILE--
 <?php
+
+use V8\Exceptions\GenericException;
+
 /** @var \Phpv8Testsuite $helper */
 $helper = require '.testsuite.php';
 
@@ -16,12 +18,16 @@ $v8_helper = new PhpV8Helpers($helper);
 $isolate = new \V8\Isolate();
 
 $template = new \V8\ObjectTemplate($isolate);
-$template->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

tests/V8ObjectTemplate_recursive.phpt renamed to tests/003-V8ObjectTemplate_recursive_self.phpt

@@ -1,10 +1,12 @@
 --TEST--
-V8\ObjectTemplate
+V8\ObjectTemplate::Set() - recursive self
 --SKIPIF--
 <?php if (!extension_loaded("v8")) print "skip"; ?>
-<?php print "skip this test is known to fail and it hangs on travis"; ?>
 --FILE--
 <?php
+
+use V8\Exceptions\GenericException;
+
 /** @var \Phpv8Testsuite $helper */
 $helper = require '.testsuite.php';
 
@@ -16,12 +18,16 @@ $v8_helper = new PhpV8Helpers($helper);
 $isolate = new \V8\Isolate();
 
 $template = new \V8\ObjectTemplate($isolate);
-$template->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