Merge pull request #70 from cscott/travis-fix-3

More memory leak and safety fixes; fix travis configuration.

.travis.yml

@@ -1,5 +1,11 @@
-language: cpp
-compiler: gcc
+language: php
+php:
+  - 5.5
+  - 5.4
+  - 5.3
+env:
+  - V8VER=3.22.22
+  - V8VER=3.21.12
 before_install: make -f Makefile.travis before_install
 install: make -f Makefile.travis install
 script: make -f Makefile.travis test

Makefile.travis

@@ -1,6 +1,6 @@
 # Configure and build scripts for travis CI system
-V8VER=3.22.10
-CPPVER=4.6
+V8VER ?= 3.22.10
+CPPVER ?= 4.6
 
 export CXX=g++-$(CPPVER)
 export LINK=g++-$(CPPVER)

php_v8js_macros.h

@@ -142,8 +142,18 @@ v8::Handle<v8::Value> zval_to_v8js(zval *, v8::Isolate * TSRMLS_DC);
 /* Convert V8 value into zval */
 int v8js_to_zval(v8::Handle<v8::Value>, zval *, int, v8::Isolate * TSRMLS_DC);
 
+struct php_v8js_accessor_ctx
+{
+    char *variable_name_string;
+    uint variable_name_string_len;
+    v8::Isolate *isolate;
+};
+
+void php_v8js_accessor_ctx_dtor(php_v8js_accessor_ctx * TSRMLS_DC);
+
 /* Register accessors into passed object */
-void php_v8js_register_accessors(v8::Local<v8::ObjectTemplate>, zval *, v8::Isolate * TSRMLS_DC);
+void php_v8js_register_accessors(std::vector<php_v8js_accessor_ctx*> *accessor_list, v8::Local<v8::FunctionTemplate>, zval *, v8::Isolate * TSRMLS_DC);
+
 
 /* {{{ Context container */
 struct php_v8js_ctx {
@@ -161,6 +171,7 @@ struct php_v8js_ctx {
   std::vector<char *> modules_stack;
   std::vector<char *> modules_base;
   std::map<const char *,v8js_tmpl_t> template_cache;
+  std::vector<php_v8js_accessor_ctx *> accessor_list;
 #ifdef ZTS
   void ***zts_ctx;
 #endif
@@ -180,6 +191,7 @@ struct php_v8js_timer_ctx
   long memory_limit;
   std::chrono::time_point<std::chrono::high_resolution_clock> time_point;
   php_v8js_ctx *v8js_ctx;
+  bool killed;
 };
 
 /* {{{ Object container */
@@ -188,6 +200,7 @@ struct php_v8js_object {
 	v8::Persistent<v8::Value> v8obj;
 	int flags;
 	v8::Isolate *isolate;
+	HashTable *properties;
 };
 /* }}} */
 

v8js.cc

@@ -305,8 +305,16 @@ static HashTable *php_v8js_v8_get_properties(zval *object TSRMLS_DC) /* {{{ */
 	php_v8js_object *obj = (php_v8js_object *) zend_object_store_get_object(object TSRMLS_CC);
 	HashTable *retval;
 
-	ALLOC_HASHTABLE(retval);
-	zend_hash_init(retval, 0, NULL, ZVAL_PTR_DTOR, 0);
+	if (obj->properties == NULL) {
+		if (GC_G(gc_active)) {
+			/* the garbage collector is running, don't create more zvals */
+			return NULL;
+		}
+		ALLOC_HASHTABLE(obj->properties);
+		zend_hash_init(obj->properties, 0, NULL, ZVAL_PTR_DTOR, 0);
+	} else {
+		zend_hash_clean(obj->properties);
+	}
 
 	v8::Isolate *isolate = obj->isolate;
 	v8::Locker locker(isolate);
@@ -316,8 +324,8 @@ static HashTable *php_v8js_v8_get_properties(zval *object TSRMLS_DC) /* {{{ */
 	v8::Context::Scope temp_scope(temp_context);
 	v8::Local<v8::Value> v8obj = v8::Local<v8::Value>::New(isolate, obj->v8obj);
 
-	if (php_v8js_v8_get_properties_hash(v8obj, retval, obj->flags, isolate TSRMLS_CC) == SUCCESS) {
-		return retval;
+	if (php_v8js_v8_get_properties_hash(v8obj, obj->properties, obj->flags, isolate TSRMLS_CC) == SUCCESS) {
+		return obj->properties;
 	}
 
 	return NULL;
@@ -326,7 +334,7 @@ static HashTable *php_v8js_v8_get_properties(zval *object TSRMLS_DC) /* {{{ */
 
 static HashTable *php_v8js_v8_get_debug_info(zval *object, int *is_temp TSRMLS_DC) /* {{{ */
 {
-	*is_temp = 1;
+	*is_temp = 0;
 	return php_v8js_v8_get_properties(object TSRMLS_CC);
 }
 /* }}} */
@@ -460,6 +468,12 @@ static void php_v8js_v8_free_storage(void *object, zend_object_handle handle TSR
 {
 	php_v8js_object *c = (php_v8js_object *) object;
 
+	if (c->properties) {
+		zend_hash_destroy(c->properties);
+		FREE_HASHTABLE(c->properties);
+		c->properties = NULL;
+	}
+
 	zend_object_std_dtor(&c->std TSRMLS_CC);
 
 	c->v8obj.Reset();
@@ -495,6 +509,7 @@ void php_v8js_create_v8(zval *res, v8::Handle<v8::Value> value, int flags, v8::I
 	c->v8obj.Reset(isolate, value);
 	c->flags = flags;
 	c->isolate = isolate;
+	c->properties = NULL;
 }
 /* }}} */
 
@@ -512,6 +527,10 @@ static void php_v8js_free_storage(void *object TSRMLS_DC) /* {{{ */
 		zval_ptr_dtor(&c->pending_exception);
 	}
 
+	if (c->module_loader) {
+		zval_ptr_dtor(&c->module_loader);
+	}
+
 	/* Delete PHP global object from JavaScript */
 	if (!c->context.IsEmpty()) {
 		v8::Locker locker(c->isolate);
@@ -535,6 +554,13 @@ static void php_v8js_free_storage(void *object TSRMLS_DC) /* {{{ */
 	}
 	c->template_cache.~map();
 
+	/* Clear contexts */
+	for (std::vector<php_v8js_accessor_ctx*>::iterator it = c->accessor_list.begin();
+		 it != c->accessor_list.end(); ++it) {
+		php_v8js_accessor_ctx_dtor(*it TSRMLS_CC);
+	}
+	c->accessor_list.~vector();
+
 	/* Clear global object, dispose context */
 	if (!c->context.IsEmpty()) {
 		c->context.Reset();
@@ -581,6 +607,7 @@ static zend_object_value php_v8js_new(zend_class_entry *ce TSRMLS_DC) /* {{{ */
 	new(&c->modules_stack) std::vector<char*>();
 	new(&c->modules_base) std::vector<char*>();
 	new(&c->template_cache) std::map<const char *,v8js_tmpl_t>();
+	new(&c->accessor_list) std::vector<php_v8js_accessor_ctx *>();
 
 	retval.handle = zend_objects_store_put(c, NULL, (zend_objects_free_object_storage_t) php_v8js_free_storage, NULL TSRMLS_CC);
 	retval.handlers = &v8js_object_handlers;
@@ -799,7 +826,7 @@ static PHP_METHOD(V8Js, __construct)
 
 	/* Register Get accessor for passed variables */
 	if (vars_arr && zend_hash_num_elements(Z_ARRVAL_P(vars_arr)) > 0) {
-		php_v8js_register_accessors(php_obj_t->InstanceTemplate(), vars_arr, isolate TSRMLS_CC);
+		php_v8js_register_accessors(&c->accessor_list, php_obj_t, vars_arr, isolate TSRMLS_CC);
 	}
 
 	/* Set name for the PHP JS object */
@@ -870,6 +897,7 @@ static void php_v8js_timer_push(long time_limit, long memory_limit, php_v8js_ctx
 	timer_ctx->memory_limit = memory_limit;
 	timer_ctx->time_point = from + duration;
 	timer_ctx->v8js_ctx = c;
+	timer_ctx->killed = false;
 	V8JSG(timer_stack).push(timer_ctx);
 
 	V8JSG(timer_mutex).unlock();
@@ -880,12 +908,11 @@ static void php_v8js_timer_pop(TSRMLS_D)
 	V8JSG(timer_mutex).lock();
 
 	if (V8JSG(timer_stack).size()) {
-		// Free the timer context memory
 		php_v8js_timer_ctx *timer_ctx = V8JSG(timer_stack).top();
-		efree(timer_ctx);
-
 		// Remove the timer context from the stack
 		V8JSG(timer_stack).pop();
+		// Free the timer context memory
+		efree(timer_ctx);
 	}
 
 	V8JSG(timer_mutex).unlock();
@@ -895,9 +922,7 @@ static void php_v8js_terminate_execution(php_v8js_ctx *c TSRMLS_DC)
 {
 	// Forcefully terminate the current thread of V8 execution in the isolate
 	v8::V8::TerminateExecution(c->isolate);
-
-	// Remove this timer from the stack
-	php_v8js_timer_pop(TSRMLS_C);
+	// This timer will be removed from stack by the parent thread.
 }
 
 static void php_v8js_timer_thread(TSRMLS_D)
@@ -914,7 +939,9 @@ static void php_v8js_timer_thread(TSRMLS_D)
 			// Get memory usage statistics for the isolate
 			c->isolate->GetHeapStatistics(&hs);
 
-			if (timer_ctx->time_limit > 0 && now > timer_ctx->time_point) {
+			if (timer_ctx->time_limit > 0 && now > timer_ctx->time_point &&
+				!timer_ctx->killed) {
+				timer_ctx->killed = true;
 				php_v8js_terminate_execution(c TSRMLS_CC);
 
 				V8JSG(timer_mutex).lock();
@@ -922,7 +949,9 @@ static void php_v8js_timer_thread(TSRMLS_D)
 				V8JSG(timer_mutex).unlock();
 			}
 
-			if (timer_ctx->memory_limit > 0 && hs.used_heap_size() > timer_ctx->memory_limit) {
+			if (timer_ctx->memory_limit > 0 && hs.used_heap_size() > timer_ctx->memory_limit &&
+				!timer_ctx->killed) {
+				timer_ctx->killed = true;
 				php_v8js_terminate_execution(c TSRMLS_CC);
 
 				V8JSG(timer_mutex).lock();
@@ -1001,7 +1030,7 @@ static PHP_METHOD(V8Js, executeString)
 	v8::Local<v8::Value> result = script->Run();
 	c->in_execution--;
 
-	if (time_limit > 0) {
+	if (time_limit > 0 || memory_limit > 0) {
 		php_v8js_timer_pop(TSRMLS_C);
 	}
 
@@ -1670,6 +1699,16 @@ static PHP_MINFO_FUNCTION(v8js)
  */
 static PHP_GINIT_FUNCTION(v8js)
 {
+	/*
+	  If ZTS is disabled, the v8js_globals instance is declared right
+	  in the BSS and hence automatically initialized by C++ compiler.
+	  Most of the variables are just zeroed.
+
+	  If ZTS is enabled however, v8js_globals just points to a freshly
+	  allocated, uninitialized piece of memory, hence we need to
+	  initialize all fields on our own.  Likewise on shutdown we have to
+	  run the destructors manually.
+	*/
 #ifdef ZTS
 	v8js_globals->extensions = NULL;
 	v8js_globals->v8_initialized = 0;

v8js_convert.cc

@@ -29,6 +29,8 @@ extern "C" {
 #include <v8.h>
 #include <stdexcept>
 
+static void php_v8js_weak_object_callback(const v8::WeakCallbackData<v8::Object, zval> &data);
+
 /* Callback for PHP methods and functions */
 static void php_v8js_call_php_func(zval *value, zend_class_entry *ce, zend_function *method_ptr, v8::Isolate *isolate, const v8::FunctionCallbackInfo<v8::Value>& info TSRMLS_DC) /* {{{ */
 {
@@ -178,6 +180,7 @@ static void php_v8js_construct_callback(const v8::FunctionCallbackInfo<v8::Value
 	// @todo assert constructor call
 	v8::Handle<v8::Object> newobj = info.This();
 	v8::Local<v8::External> php_object;
+	zval *value;
 
 	if (!info.IsConstructCall()) {
 		return;
@@ -190,6 +193,10 @@ static void php_v8js_construct_callback(const v8::FunctionCallbackInfo<v8::Value
 	if (info[0]->IsExternal()) {
 		// Object created by v8js in php_v8js_hash_to_jsobj, PHP object passed as v8::External.
 		php_object = v8::Local<v8::External>::Cast(info[0]);
+		value = reinterpret_cast<zval *>(php_object->Value());
+		// Increase the reference count of this value because we're storing it internally for use later
+		// See https://github.com/preillyme/v8js/issues/6
+		Z_ADDREF_P(value);
 	} else {
 		// Object created from JavaScript context.  Need to create PHP object first.
 		V8JS_TSRMLS_FETCH();
@@ -202,7 +209,6 @@ static void php_v8js_construct_callback(const v8::FunctionCallbackInfo<v8::Value
 			return;
 		}
 
-		zval *value;
 		MAKE_STD_ZVAL(value);
 		object_init_ex(value, ce);
 
@@ -215,6 +221,16 @@ static void php_v8js_construct_callback(const v8::FunctionCallbackInfo<v8::Value
 
 	newobj->SetAlignedPointerInInternalField(0, ext_tmpl->Value());
 	newobj->SetHiddenValue(V8JS_SYM(PHPJS_OBJECT_KEY), php_object);
+
+	// Since we got to decrease the reference count again, in case v8 garbage collector
+	// decides to dispose the JS object, we add a weak persistent handle and register
+	// a callback function that removes the reference.
+	v8::Persistent<v8::Object> persist_newobj(isolate, newobj);
+	persist_newobj.SetWeak(value, php_v8js_weak_object_callback);
+
+	// Just tell v8 that we're allocating some external memory
+	// (for the moment we just always tell 1k instead of trying to find out actual values)
+	v8::V8::AdjustAmountOfExternalAllocatedMemory(1024);
 }
 /* }}} */
 
@@ -499,8 +515,7 @@ static inline v8::Local<v8::Value> php_v8js_named_property_callback(v8::Local<v8
 	v8::Local<v8::Function> cb;
 
 	V8JS_TSRMLS_FETCH();
-	zend_class_entry *scope = NULL; /* XXX? */
-	zend_class_entry *ce;
+	zend_class_entry *scope, *ce;
 	zend_function *method_ptr = NULL;
 	zval *php_value;
 
@@ -508,7 +523,7 @@ static inline v8::Local<v8::Value> php_v8js_named_property_callback(v8::Local<v8
 	v8::Local<v8::FunctionTemplate> tmpl =
 		v8::Local<v8::FunctionTemplate>::New
 		(isolate, *reinterpret_cast<v8js_tmpl_t *>(self->GetAlignedPointerFromInternalField(0)));
-	ce = Z_OBJCE_P(object);
+	ce = scope = Z_OBJCE_P(object);
 
 	/* First, check the (case-insensitive) method table */
 	php_strtolower(lower, name_len);
@@ -583,8 +598,15 @@ static inline v8::Local<v8::Value> php_v8js_named_property_callback(v8::Local<v8
 				// wrap it
 				ret_value = zval_to_v8js(php_value, isolate TSRMLS_CC);
 			}
-			/* php_value is the value in the property table; we don't own a
-			 * reference to it (and so don't have to deref) */
+			/* php_value is the value in the property table; *usually* we
+			 * don't own a reference to it (and so don't have to deref).
+			 * But sometimes the value is the result of a __get() call and
+			 * the refcnt of the returned value is 0.  In that case, free
+			 * it. */
+			if (php_value != EG(uninitialized_zval_ptr)) {
+				zval_add_ref(&php_value);
+				zval_ptr_dtor(&php_value);
+			}
 		} else if (callback_type == V8JS_PROP_SETTER) {
 			MAKE_STD_ZVAL(php_value);
 			if (v8js_to_zval(set_value, php_value, 0, isolate TSRMLS_CC) == SUCCESS) {
@@ -734,20 +756,6 @@ static v8::Handle<v8::Value> php_v8js_hash_to_jsobj(zval *value, v8::Isolate *is
 		v8::Handle<v8::Value> external = v8::External::New(value);
 		newobj = new_tpl->GetFunction()->NewInstance(1, &external);
 
-		// Increase the reference count of this value because we're storing it internally for use later
-		// See https://github.com/preillyme/v8js/issues/6
-		Z_ADDREF_P(value);
-
-		// Since we got to decrease the reference count again, in case v8 garbage collector
-		// decides to dispose the JS object, we add a weak persistent handle and register
-		// a callback function that removes the reference.
-		v8::Persistent<v8::Object> persist_newobj(isolate, newobj);
-		persist_newobj.SetWeak(value, php_v8js_weak_object_callback);
-
-		// Just tell v8 that we're allocating some external memory
-		// (for the moment we just always tell 1k instead of trying to find out actual values)
-		v8::V8::AdjustAmountOfExternalAllocatedMemory(1024);
-
 		if (ce == zend_ce_closure) {
 			// free uncached function template when object is freed
 			v8::Persistent<v8::Object> persist_newobj2(isolate, newobj);

v8js_methods.cc

@@ -234,6 +234,7 @@ V8JS_METHOD(require)
 	for (std::vector<char *>::iterator it = c->modules_stack.begin(); it != c->modules_stack.end(); ++it)
     {
     	if (!strcmp(*it, normalised_module_id)) {
+    		efree(normalised_module_id);
     		efree(normalised_path);
 
 		info.GetReturnValue().Set(v8::ThrowException(V8JS_SYM("Module cyclic dependency")));
@@ -243,6 +244,7 @@ V8JS_METHOD(require)
 
     // If we have already loaded and cached this module then use it
 	if (V8JSG(modules_loaded).count(normalised_module_id) > 0) {
+		efree(normalised_module_id);
 		efree(normalised_path);
 
 		info.GetReturnValue().Set(V8JSG(modules_loaded)[normalised_module_id]);
@@ -251,22 +253,26 @@ V8JS_METHOD(require)
 
 	// Callback to PHP to load the module code
 
-	zval module_code;
+	zval *module_code;
 	zval *normalised_path_zend;
 
 	MAKE_STD_ZVAL(normalised_path_zend);
 	ZVAL_STRING(normalised_path_zend, normalised_module_id, 1);
-	zval* params[] = { normalised_path_zend };
 
-	if (FAILURE == call_user_function(EG(function_table), NULL, c->module_loader, &module_code, 1, params TSRMLS_CC)) {
+	zval **params[1] = {&normalised_path_zend};
+	if (FAILURE == call_user_function_ex(EG(function_table), NULL, c->module_loader, &module_code, 1, params, 0, NULL TSRMLS_CC)) {
+		zval_ptr_dtor(&normalised_path_zend);
+		efree(normalised_module_id);
 		efree(normalised_path);
 
 		info.GetReturnValue().Set(v8::ThrowException(V8JS_SYM("Module loader callback failed")));
 		return;
 	}
+	zval_ptr_dtor(&normalised_path_zend);
 
 	// Check if an exception was thrown
 	if (EG(exception)) {
+		efree(normalised_module_id);
 		efree(normalised_path);
 
 		// Clear the PHP exception and throw it in V8 instead
@@ -276,12 +282,14 @@ V8JS_METHOD(require)
 	}
 
 	// Convert the return value to string
-	if (Z_TYPE(module_code) != IS_STRING) {
-    	convert_to_string(&module_code);
+	if (Z_TYPE_P(module_code) != IS_STRING) {
+		convert_to_string(module_code);
 	}
 
 	// Check that some code has been returned
-	if (!strlen(Z_STRVAL(module_code))) {
+	if (Z_STRLEN_P(module_code)==0) {
+		zval_ptr_dtor(&module_code);
+		efree(normalised_module_id);
 		efree(normalised_path);
 
 		info.GetReturnValue().Set(v8::ThrowException(V8JS_SYM("Module loader callback did not return code")));
@@ -321,13 +329,15 @@ V8JS_METHOD(require)
 	// Set script identifier
 	v8::Local<v8::String> sname = V8JS_SYM("require");
 
-	v8::Local<v8::String> source = V8JS_STR(Z_STRVAL(module_code));
+	v8::Local<v8::String> source = V8JS_STRL(Z_STRVAL_P(module_code), Z_STRLEN_P(module_code));
+	zval_ptr_dtor(&module_code);
 
 	// Create and compile script
 	v8::Local<v8::Script> script = v8::Script::New(source, sname);
 
 	// The script will be empty if there are compile errors
 	if (script.IsEmpty()) {
+		efree(normalised_module_id);
 		efree(normalised_path);
 		info.GetReturnValue().Set(v8::ThrowException(V8JS_SYM("Module script compile failed")));
 		return;
@@ -345,6 +355,7 @@ 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

v8js_variables.cc

@@ -25,13 +25,6 @@ extern "C" {
 #include <v8.h>
 #include <string>
 
-struct php_v8js_accessor_ctx
-{
-    char *variable_name_string;
-    uint variable_name_string_len;
-    v8::Isolate *isolate;
-};
-
 static void php_v8js_fetch_php_variable(v8::Local<v8::String> name, const v8::PropertyCallbackInfo<v8::Value>& info) /* {{{ */
 {
     v8::Handle<v8::External> data = v8::Handle<v8::External>::Cast(info.Data());
@@ -50,12 +43,20 @@ static void php_v8js_fetch_php_variable(v8::Local<v8::String> name, const v8::Pr
 }
 /* }}} */
 
-void php_v8js_register_accessors(v8::Local<v8::ObjectTemplate> php_obj, zval *array, v8::Isolate *isolate TSRMLS_DC) /* {{{ */
+void php_v8js_accessor_ctx_dtor(php_v8js_accessor_ctx *ctx TSRMLS_DC) /* {{{ */
+{
+	efree(ctx->variable_name_string);
+	efree(ctx);
+}
+/* }}} */
+
+void php_v8js_register_accessors(std::vector<php_v8js_accessor_ctx*> *accessor_list, v8::Local<v8::FunctionTemplate> php_obj_t, zval *array, v8::Isolate *isolate TSRMLS_DC) /* {{{ */
 {
 	char *property_name;
 	uint property_name_len;
 	ulong index;
 	zval **item;
+	v8::Local<v8::ObjectTemplate> php_obj = php_obj_t->InstanceTemplate();
 
 	for (zend_hash_internal_pointer_reset(Z_ARRVAL_P(array));
 		zend_hash_get_current_data(Z_ARRVAL_P(array), (void **) &item) != FAILURE;
@@ -85,7 +86,10 @@ void php_v8js_register_accessors(v8::Local<v8::ObjectTemplate> php_obj, zval *ar
         ctx->isolate = isolate;
 
 		/* Set the variable fetch callback for given symbol on named property */
-		php_obj->SetAccessor(V8JS_STRL(property_name, property_name_len - 1), php_v8js_fetch_php_variable, NULL, v8::External::New(ctx), v8::PROHIBITS_OVERWRITING, v8::ReadOnly);
+		php_obj->SetAccessor(V8JS_STRL(property_name, property_name_len - 1), php_v8js_fetch_php_variable, NULL, v8::External::New(ctx), v8::PROHIBITS_OVERWRITING, v8::ReadOnly, v8::AccessorSignature::New(php_obj_t));
+
+		/* record the context so we can free it later */
+		accessor_list->push_back(ctx);
 	}
 }
 /* }}} */