Few potential bugs in C++ code about v8go HOT 6 CLOSED

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.

from v8go.

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

from v8go.

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?

from v8go.

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.

from v8go.

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

from v8go.

Looks good.

from v8go.