Few potential bugs in C++ code

[nsf]

Note that these are just a code review thing. I haven't verified it, but it looks like bugs.

1. As far as I know when you convert a C string to Go string via cgo's C.GoString, the latter doesn't free C string memory. As a result RunScript usage here leaks memory, it might allocate some strings for error structure which are then not freed in getError when being converted to Go strings:
   

   v8go/context.go

   Lines 58 to 59 in 00d2f88

   	rtn := C.RunScript(c.ptr, cSource, cOrigin)
   	return getValue(rtn), getError(rtn)

2. Another memory leak here:
   

   v8go/value.go

   Line 16 in 00d2f88

   return C.GoString(C.ValueToString(v.ptr))

   
   ValueToString just does a malloc at the end.

3. What you do in CopyString and similar places is potentially unsafe. The sprintf usage is unsafe:
   

   v8go/v8go.cc

   Lines 34 to 38 in 00d2f88

   	const char* CopyString(std::string str) {
   	char* data = static_cast<char*>(malloc(str.length()));
   	sprintf(data, "%s", str.c_str());
   	return data;
   	}

   
   IIRC std::string's length returns the length of the string without terminating null byte. And IIRC sprintf writes terminating null byte at the end to the buffer. Since you allocate memory that is 1 byte short of what is written, it's a potential out of bounds write.

[nsf]

4. Also I don't see corresponding "delete" calls for these "new" calls:
   

   v8go/v8go.cc

   Line 127 in 00d2f88

   m_ctx* ctx = new m_ctx;

   
   

   v8go/v8go.cc

   Line 160 in 00d2f88

   m_value* val = new m_value;

   
   It's the only C++ file here and it contains no "delete" operator calls at all. That's a memory leak as well.

[rogchap]

Thanks for the feedback @nsf, much appreciated.
To much haste in getting it working that I forgot to "clean up" 😬
Will fix.

[rogchap]

So looking a bit more into this...
From what I understand C.GoString should be fine as-is:

GoString values may not be retained by C code. https://golang.org/cmd/cgo/#hdr-Passing_pointers

It's the C.CString that you need to make sure you free up

With the CopyString with 1 byte short is kinda what I'm looking for, as I don't want the terminating NULL to be based back to Go. I tried using memcpy but then I would have the extra bytes passed back to Go; I could keep track of the str.length() and then use C.GoStringN but then I would need to wrap my strings in a struct to pass back to Go. Do you have any suggestions on a better way?

[nsf]

So looking a bit more into this...
From what I understand C.GoString should be fine as-is:

No, the documentation talks about values of _GoString_ type. Which is a garbage collected Go string value that you can pass into a C function. C.GoString is a Go function that allocates a Go string for its argument, which must be a null terminated C string. The argument is not freed by this Go function. It's not fine, it's a memory leak.

with 1 byte short

Your allocation is one byte short, but the sprintf function does write terminating null byte. See docs: http://www.cplusplus.com/reference/cstdio/sprintf/.

A terminating null character is automatically appended after the content.

Terminating NULL is not passed back to Go, how do you think C.GoString knows the length of the string argument. C string is just a pointer to some bytes. The way C expresses length in a string is by having a terminating null byte at the end.

Passing back null terminated C strings is fine, since you make a copy anyway. You just need to do it correctly. Roughly something like that:

char* CopyV8Utf8Value(const v8::String::Utf8Value& val) {
        int len = val.length();
        char *mem = (char*)malloc(len+1);
        memcpy(mem, *val, len);
        mem[len] = 0;
        return mem;
}

Note that I copy just the bytes here, even though browsing V8 source code shows that Utf8Value is actually null terminated on its own. But I'm not relying on it here. If you're 100% sure it's null terminated internally, then len+1 memcpy would do the job.

But looking at Utf8Value code:

String::Utf8Value::Utf8Value(v8::Local<v8::Value> obj)
    : str_(NULL), length_(0) {
  if (obj.IsEmpty()) return;
  i::Isolate* isolate = i::Isolate::Current();
  Isolate* v8_isolate = reinterpret_cast<Isolate*>(isolate);
  ENTER_V8(isolate);
  i::HandleScope scope(isolate);
  Local<Context> context = v8_isolate->GetCurrentContext();
  TryCatch try_catch(v8_isolate);
  Local<String> str;
  if (!obj->ToString(context).ToLocal(&str)) return;
  i::Handle<i::String> i_str = Utils::OpenHandle(*str);
  length_ = v8::Utf8Length(*i_str, isolate);
  str_ = i::NewArray<char>(length_ + 1);
  str->WriteUtf8(str_);
}

Seems like it just does a copy there as well using "WriteUtf8" String's function. Ideally you should ask String to perform a write to your buffer instead of using Utf8Value as it would just be an extra copy.

[rogchap]

Thanks again @nsf for the detailed help.
I've incorporated your suggestions into #9; Would you mind taking a look?

[nsf]

Looks good.