Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug]: Native node module crashes due to ObjectWrap::Unwrap<> returning a different pointer than what was passed to ObjectWrap::Wrap<> #40340

Closed
3 tasks done
rkeithhill-keysight opened this issue Oct 25, 2023 · 2 comments

Comments

@rkeithhill-keysight
Copy link

rkeithhill-keysight commented Oct 25, 2023

Preflight Checklist

Electron Version

22.3.27

What operating system are you using?

Windows

Operating System Version

Windows 11 22H2 (build 22621.2283)

What arch are you using?

x64

Last Known Working Electron version

21.4.4

Expected Behavior

We pass back a ptr to the object that implements our native node module's functionality to JavaScript using Nan::ObjectWrap::Wrap(). When we invoke a method in our native node module, that ptr is passed back in via the Nan::FunctionCallbackInfo<v8::Value> const& info parameter, which we get by using ObjectWrap::Unwrap<NodeAdapter>(info.Holder());.

Up through Electron 21.4.4., that Unwrap call has always returned the same pointer that was passed into the Wrap() call in our void NodeAdapter::New(const Nan::FunctionCallbackInfo<v8::Value>& info) method. This is important because there are instance fields on this object that get initialized at construct time that we expect to use later when the native node module's methods are invoked.

Actual Behavior

Starting with Electron 22.3.27, the ptr we get back from Unwrap is not the same as what was passed to Wrap() in the void NodeAdapter::New(const Nan::FunctionCallbackInfo<v8::Value>& info) method. This causes our renderer process to crash as soon as a native node module method is called and it tries to dereference the bad ptr it unwrapped.:

// New method
void NodeAdapter::New(const Nan::FunctionCallbackInfo<v8::Value>& info)
{
    // Adapter address returned here (during debug run) is: 0x000001d854075940
    auto* adapter = new NodeAdapter();

    adapter->Wrap(info.This());
    info.GetReturnValue().Set(info.This());
}

// log method
void NodeAdapter::log(Nan::FunctionCallbackInfo<v8::Value> const& info)
{
    // Problem first observed here.
    // Wrong address returned for this object (during same debug run): 0x00036b6000000000
    auto const adapter = ObjectWrap::Unwrap<NodeAdapter>(info.Holder());

    ...

    // CRASH - the first ptr deref crashes because we didn't get back the correct/original adapter address
    adapter->m_guiLogger->log(level, message);
}

Testcase Gist URL

No response

Additional Information

I looked through changes between node v16.16 (Electron 21) and v16.17 (Electron 22) and I don't see anything that could be related. I see that Electron 21 introduced the V8 memory cage feature as discussed in this issue on crashing native node modules: #35801. But the thing is, our native node module works on Electron 21 (unless the issue is intermittent). Also, we don't use ArrayBuffer explicitly but we do use the new keyword to create an instance of our NodeAdapter class (see below).

There is also this issue on crashing renderer process starting at Electron 22 that may be related: #36999.

Our BrowserWindow (renderer) runs with nodeIntegration: true and contextIsolation: false. This native node module started on Electron 4 and has been working just fine until we tried to upgrade to Electron 22. Here's a redacted/abbreviated snapshot of our native node module code:

// NativeExtension.cpp:
// -----------------------------------------------------------------------
NAN_MODULE_INIT(InitAll)
{
    NodeAdapter::Init(target);
}

NAN_MODULE_WORKER_ENABLED(NativeExtension, InitAll);


// NodeAdapter.h
// -----------------------------------------------------------------------
#include <nan.h>
#include "GuiLogger.h"

struct NodeAdapter : public Nan::ObjectWrap
{
    static void Init(v8::Local<v8::Object> exports);

private:
    static void New(Nan::FunctionCallbackInfo<v8::Value> const& info);
    static void log(Nan::FunctionCallbackInfo<v8::Value> const& info);

    static Nan::Persistent<v8::Function> s_constructor;

    explicit NodeAdapter();

public:
    std::shared_ptr<GuiLogger> m_guiLogger;
}


// NodeAdapter.cpp:
// -----------------------------------------------------------------------
#include "NodeAdapter.h"

Nan::Persistent<v8::Function> NodeAdapter::s_constructor;
static constexpr auto ObjectName = "NodeAdapterWrapper";

// Init method
void NodeAdapter::Init(v8::Local<v8::Object> exports)
{
    v8::Local<v8::FunctionTemplate> tpl = Nan::New<v8::FunctionTemplate>(New);
    tpl->SetClassName(Nan::New(ObjectName).ToLocalChecked());
    tpl->InstanceTemplate()->SetInternalFieldCount(1);

    Nan::SetPrototypeMethod(tpl, "log", log);

    s_constructor.Reset(Nan::GetFunction(tpl).ToLocalChecked());
    Nan::Set(exports, Nan::New(ObjectName).ToLocalChecked(), Nan::GetFunction(tpl).ToLocalChecked());
}


// New method
void NodeAdapter::New(const Nan::FunctionCallbackInfo<v8::Value>& info)
{
    // Adapter address returned here (during debug run) is: 0x000001d854075940
    auto* adapter = new NodeAdapter();

    adapter->Wrap(info.This());
    info.GetReturnValue().Set(info.This());
}


// ctor
NodeAdapter::NodeAdapter()
{
    const auto guiLogger = new GuiLogger();
    m_guiLogger = guiLogger->getLogger();
}

// log method
void NodeAdapter::log(Nan::FunctionCallbackInfo<v8::Value> const& info)
{
    // Problem first observed here.
    // Wrong address returned for this object (during same debug run): 0x00036b6000000000
    auto const adapter = ObjectWrap::Unwrap<NodeAdapter>(info.Holder());

    ...

    // CRASH - the first ptr deref crashes because we didn't get back the correct/original adapter address
    adapter->m_guiLogger->log(level, message);
}


// Compiled with cmake-js & VS 2022 (MDd) with the following defines:
// -----------------------------------------------------------------------
//    -DV8_31BIT_SMIS_ON_64BIT_ARCH 
//    -DV8_COMPRESS_POINTERS
//    -DV8_REVERSE_JSARGS
@rkeithhill-keysight
Copy link
Author

rkeithhill-keysight commented Oct 26, 2023

BTW we still see this bug on Electron 25.9.2.

@jkleinsc
Copy link
Contributor

Thanks for reaching out and logging this issue! Because we treat our issues list as the team's backlog, we close issues that are questions since they don't represent a task needing to be completed. For most questions about Electron there are a lot of options.

Check out the Electron community. There are also a bunch of helpful people in this Discord that should be willing to point you in the right direction. For your question, I'd recommend the Discord - we have many active help channels and mentors, as well as fellow devs, who can help you out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants