pmed / v8pp

Bind C++ functions and classes into V8 JavaScript engine
http://pmed.github.io/v8pp/
Other
898 stars 120 forks source link

Possible bug in `get_option` #58

Closed jcmonnin closed 7 years ago

jcmonnin commented 7 years ago

When using get_option I encountered strange crashes. I think the Handle/Local returned by get_option is deleted by the HandleScope inside the get_option function.

When commenting out v8::HandleScope scope(isolate); in the function, the spooky behaviour disappears. I this case the the responsibility of the caller's HandleScope to delete the handles. Maybe a fix using EscapableHandleScope would be better?

template<typename T>
bool get_option(v8::Isolate* isolate, v8::Handle<v8::Object> options,
    char const* name, T& value)
{
    char const* dot = strchr(name, '.');
    if (dot)
    {
        std::string const subname(name, dot);
        //v8::HandleScope scope(isolate);   // THIS IS MY FIX
        v8::Local<v8::Object> suboptions;
        return get_option(isolate, options, subname.c_str(), suboptions)
            && get_option(isolate, suboptions, dot + 1, value);
    }
    v8::Local<v8::Value> val = options->Get(v8pp::to_v8(isolate, name));
    if (val.IsEmpty() || val->IsUndefined())
    {
        return false;
    }
    value = from_v8<T>(isolate, val);
    return true;
}

The problem can be reproduced with following code:

context.run_script("test = { test : function() { return 1; } }");
if (try_catch.HasCaught()) {
    throw "error";
}

v8::Local<v8::Function> fTest;
if (!v8pp::get_option(isolate, isolate->GetCurrentContext()->Global(), "test.test", fTest)) {
    throw std::runtime_error("No 'test.test'");
}
if (!fTest->IsFunction()) {
    throw std::runtime_error("It's not a function, something is wrong");
}

fTest seems to be garbage.

pmed commented 7 years ago

Hi @jcmonnin,

Thank you for reporting and test case, I've fixed this bug. Yes, that HandleScope is not required.