Fix crashes from template finalizer (rogchap#182)

* Fix crash from template finalizer releasing V8 data, just free the wrapper v8::Persistent::Reset() shouldn't be used without synchronization from the finalizer goroutine to avoid race conditions. Instead, we can just free the wrapper and leave the V8 data on the isolate heap to be freed when the isolate is disposed. * Keep alive the template while its C++ pointer is still being used Otherwise, the garbage collector can collect the template and the finalizer can free the C++ m_template struct before it is finished being used.
YaoApp · Sep 27, 2021 · fd090e9 · fd090e9
1 parent 7e0f8ff
commit fd090e9
Show file tree

Hide file tree

Showing 7 changed files with 17 additions and 5 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -24,6 +24,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 - Add some missing error propagation
+- Fix crash from template finalizer releasing V8 data, let it be disposed with the isolate
+- Fix crash by keeping alive the template while its C++ pointer is still being used
 
 ## [v0.6.0] - 2021-05-11
 

diff --git a/context.go b/context.go
@@ -8,6 +8,7 @@ package v8go
 // #include "v8go.h"
 import "C"
 import (
+	"runtime"
 	"sync"
 	"unsafe"
 )
@@ -72,6 +73,7 @@ func NewContext(opt ...ContextOption) *Context {
 		ptr: C.NewContext(opts.iso.ptr, opts.gTmpl.ptr, C.int(ref)),
 		iso: opts.iso,
 	}
+	runtime.KeepAlive(opts.gTmpl)
 	return ctx
 }
 

diff --git a/function_template.go b/function_template.go
@@ -66,6 +66,7 @@ func NewFunctionTemplate(iso *Isolate, callback FunctionCallback) *FunctionTempl
 // GetFunction returns an instance of this function template bound to the given context.
 func (tmpl *FunctionTemplate) GetFunction(ctx *Context) *Function {
 	rtn := C.FunctionTemplateGetFunction(tmpl.ptr, ctx.ptr)
+	runtime.KeepAlive(tmpl)
 	val, err := valueResult(ctx, rtn)
 	if err != nil {
 		panic(err) // TODO: Consider returning the error

diff --git a/object_template.go b/object_template.go
@@ -56,6 +56,7 @@ func (o *ObjectTemplate) NewInstance(ctx *Context) (*Object, error) {
 	}
 
 	rtn := C.ObjectTemplateNewInstance(o.ptr, ctx.ptr)
+	runtime.KeepAlive(o)
 	return objectResult(ctx, rtn)
 }
 

diff --git a/template.go b/template.go
@@ -42,8 +42,10 @@ func (t *template) Set(name string, val interface{}, attributes ...PropertyAttri
 		C.TemplateSetValue(t.ptr, cname, newVal.ptr, C.int(attrs))
 	case *ObjectTemplate:
 		C.TemplateSetTemplate(t.ptr, cname, v.ptr, C.int(attrs))
+		runtime.KeepAlive(v)
 	case *FunctionTemplate:
 		C.TemplateSetTemplate(t.ptr, cname, v.ptr, C.int(attrs))
+		runtime.KeepAlive(v)
 	case *Value:
 		if v.IsObject() || v.IsExternal() {
 			return errors.New("v8go: unsupported property: value type must be a primitive or use a template")
@@ -52,12 +54,15 @@ func (t *template) Set(name string, val interface{}, attributes ...PropertyAttri
 	default:
 		return fmt.Errorf("v8go: unsupported property type `%T`, must be one of string, int32, uint32, int64, uint64, float64, *big.Int, *v8go.Value, *v8go.ObjectTemplate or *v8go.FunctionTemplate", v)
 	}
+	runtime.KeepAlive(t)
 
 	return nil
 }
 
 func (t *template) finalizer() {
-	C.TemplateFree(t.ptr)
+	// Using v8::PersistentBase::Reset() wouldn't be thread-safe to do from
+	// this finalizer goroutine so just free the wrapper and let the template
+	// itself get cleaned up when the isolate is disposed.
+	C.TemplateFreeWrapper(t.ptr)
 	t.ptr = nil
-	runtime.SetFinalizer(t, nil)
 }
diff --git a/v8go.cc b/v8go.cc
@@ -213,8 +213,9 @@ IsolateHStatistics IsolationGetHeapStatistics(IsolatePtr iso) {
   HandleScope handle_scope(iso);     \
   Local<Template> tmpl = tmpl_ptr->ptr.Get(iso);
 
-void TemplateFree(TemplatePtr ptr) {
-  delete ptr;
+void TemplateFreeWrapper(TemplatePtr tmpl) {
+  tmpl->ptr.Empty();  // Just does `val_ = 0;` without calling V8::DisposeGlobal
+  delete tmpl;
 }
 
 void TemplateSetValue(TemplatePtr ptr,

diff --git a/v8go.h b/v8go.h
@@ -85,7 +85,7 @@ extern RtnValue JSONParse(ContextPtr ctx_ptr, const char* str);
 const char* JSONStringify(ContextPtr ctx_ptr, ValuePtr val_ptr);
 extern ValuePtr ContextGlobal(ContextPtr ctx_ptr);
 
-extern void TemplateFree(TemplatePtr ptr);
+extern void TemplateFreeWrapper(TemplatePtr ptr);
 extern void TemplateSetValue(TemplatePtr ptr,
                              const char* name,
                              ValuePtr val_ptr,
-Original file line number
+Diff line change
@@ Expand Up @@
     	}
     	rtn := C.ObjectTemplateNewInstance(o.ptr, ctx.ptr)
+    	runtime.KeepAlive(o)
     	return objectResult(ctx, rtn)
     }
@@ Expand Down @@