Skip to content

Commit

Permalink
async_wrap,src: add GetAsyncId() method
Browse files Browse the repository at this point in the history
Allow handles to retrieve their own uid's by adding a new method on the
FunctionTemplates. Implementation of these into all other classes will
come in a future commit.

Add the method AsyncWrap::GetAsyncId() to all inheriting class objects
so the uid of the handle can be retrieved from JS.

In all applicable locations, run ClearWrap() on the object holding the
pointer so that it never points to invalid memory and make sure Wrap()
is always run so the class pointer is correctly attached to the object
and can be retrieved so GetAsyncId() can be run.

In many places a class instance was not removing its own pointer from
object() in the destructor. This left an invalid pointer in the JS
object that could cause the application to segfault under certain
conditions.

Remove ClearWrap() from ReqWrap for continuity. The ReqWrap constructor
was not the one to call Wrap(), so it shouldn't be the one to call
ClearWrap().

Wrap() has been added to all constructors that inherit from AsyncWrap.
Normally it's the child most class. Except in the case of HandleWrap.
Which must be the constructor that runs Wrap() because the class pointer
is retrieved for certain calls and because other child classes have
multiple inheritance to pointer to the HandleWrap needs to be stored.

ClearWrap() has been placed in all FunctionTemplate constructors so that
no random values are returned when running getAsyncId(). ClearWrap() has
also been placed in all class destructors, except in those that use
MakeWeak() because the destructor will run during GC. Making the
object() inaccessible.

It could be simplified to where AsyncWrap sets the internal pointer,
then if an inheriting class needs one of it's own it could set it again.
But the inverse would need to be true also, where AsyncWrap then also
runs ClearWeak. Unforunately because some of the handles are cleaned up
during GC that's impossible. Also in the case of ReqWrap it runs Reset()
in the destructor, making the object() inaccessible. Meaning,
ClearWrap() must be run by the class that runs Wrap(). There's currently
no generalized way of taking care of this across all instances of
AsyncWrap.

I'd prefer that there be checks in there for these things, but haven't
found a way to place them that wouldn't be just as unreliable.

Add test that checks all resources that can run getAsyncId(). Would like
a way to enforce that any new classes that can also run getAsyncId() are
tested, but don't have one.

PR-URL: nodejs#12892
Ref: nodejs#11883
Ref: nodejs#8531
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
  • Loading branch information
trevnorris authored and Olivier Martin committed May 19, 2017
1 parent 3e183dc commit 8130199
Show file tree
Hide file tree
Showing 25 changed files with 326 additions and 2 deletions.
8 changes: 8 additions & 0 deletions src/async-wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,14 @@ static void SetupHooks(const FunctionCallbackInfo<Value>& args) {
}


void AsyncWrap::GetAsyncId(const FunctionCallbackInfo<Value>& args) {
AsyncWrap* wrap;
args.GetReturnValue().Set(-1);
ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder());
args.GetReturnValue().Set(wrap->get_id());
}


void AsyncWrap::Initialize(Local<Object> target,
Local<Value> unused,
Local<Context> context) {
Expand Down
2 changes: 2 additions & 0 deletions src/async-wrap.h
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,8 @@ class AsyncWrap : public BaseObject {
v8::Local<v8::Value> unused,
v8::Local<v8::Context> context);

static void GetAsyncId(const v8::FunctionCallbackInfo<v8::Value>& args);

static void DestroyIdsCb(uv_idle_t* handle);

inline ProviderType provider_type() const;
Expand Down
15 changes: 15 additions & 0 deletions src/cares_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ inline const char* ToErrorCodeString(int status) {
class GetAddrInfoReqWrap : public ReqWrap<uv_getaddrinfo_t> {
public:
GetAddrInfoReqWrap(Environment* env, Local<Object> req_wrap_obj);
~GetAddrInfoReqWrap();

size_t self_size() const override { return sizeof(*this); }
};
Expand All @@ -114,10 +115,15 @@ GetAddrInfoReqWrap::GetAddrInfoReqWrap(Environment* env,
Wrap(req_wrap_obj, this);
}

GetAddrInfoReqWrap::~GetAddrInfoReqWrap() {
ClearWrap(object());
}


class GetNameInfoReqWrap : public ReqWrap<uv_getnameinfo_t> {
public:
GetNameInfoReqWrap(Environment* env, Local<Object> req_wrap_obj);
~GetNameInfoReqWrap();

size_t self_size() const override { return sizeof(*this); }
};
Expand All @@ -128,6 +134,10 @@ GetNameInfoReqWrap::GetNameInfoReqWrap(Environment* env,
Wrap(req_wrap_obj, this);
}

GetNameInfoReqWrap::~GetNameInfoReqWrap() {
ClearWrap(object());
}


int cmp_ares_tasks(const node_ares_task* a, const node_ares_task* b) {
if (a->sock < b->sock)
Expand Down Expand Up @@ -293,6 +303,7 @@ class QueryWrap : public AsyncWrap {
: AsyncWrap(env, req_wrap_obj, AsyncWrap::PROVIDER_QUERYWRAP) {
if (env->in_domain())
req_wrap_obj->Set(env->domain_string(), env->domain_array()->Get(0));
Wrap(req_wrap_obj, this);
}

~QueryWrap() override {
Expand Down Expand Up @@ -1388,10 +1399,12 @@ void Initialize(Local<Object> target,
auto is_construct_call_callback =
[](const FunctionCallbackInfo<Value>& args) {
CHECK(args.IsConstructCall());
ClearWrap(args.This());
};
Local<FunctionTemplate> aiw =
FunctionTemplate::New(env->isolate(), is_construct_call_callback);
aiw->InstanceTemplate()->SetInternalFieldCount(1);
env->SetProtoMethod(aiw, "getAsyncId", AsyncWrap::GetAsyncId);
aiw->SetClassName(
FIXED_ONE_BYTE_STRING(env->isolate(), "GetAddrInfoReqWrap"));
target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "GetAddrInfoReqWrap"),
Expand All @@ -1400,6 +1413,7 @@ void Initialize(Local<Object> target,
Local<FunctionTemplate> niw =
FunctionTemplate::New(env->isolate(), is_construct_call_callback);
niw->InstanceTemplate()->SetInternalFieldCount(1);
env->SetProtoMethod(niw, "getAsyncId", AsyncWrap::GetAsyncId);
niw->SetClassName(
FIXED_ONE_BYTE_STRING(env->isolate(), "GetNameInfoReqWrap"));
target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "GetNameInfoReqWrap"),
Expand All @@ -1408,6 +1422,7 @@ void Initialize(Local<Object> target,
Local<FunctionTemplate> qrw =
FunctionTemplate::New(env->isolate(), is_construct_call_callback);
qrw->InstanceTemplate()->SetInternalFieldCount(1);
env->SetProtoMethod(qrw, "getAsyncId", AsyncWrap::GetAsyncId);
qrw->SetClassName(
FIXED_ONE_BYTE_STRING(env->isolate(), "QueryReqWrap"));
target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "QueryReqWrap"),
Expand Down
5 changes: 5 additions & 0 deletions src/connect_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,9 @@ ConnectWrap::ConnectWrap(Environment* env,
Wrap(req_wrap_obj, this);
}


ConnectWrap::~ConnectWrap() {
ClearWrap(object());
}

} // namespace node
1 change: 1 addition & 0 deletions src/connect_wrap.h
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ class ConnectWrap : public ReqWrap<uv_connect_t> {
ConnectWrap(Environment* env,
v8::Local<v8::Object> req_wrap_obj,
AsyncWrap::ProviderType provider);
~ConnectWrap();

size_t self_size() const override { return sizeof(*this); }
};
Expand Down
1 change: 1 addition & 0 deletions src/fs_event_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ void FSEventWrap::Initialize(Local<Object> target,
t->InstanceTemplate()->SetInternalFieldCount(1);
t->SetClassName(fsevent_string);

env->SetProtoMethod(t, "getAsyncId", AsyncWrap::GetAsyncId);
env->SetProtoMethod(t, "start", Start);
env->SetProtoMethod(t, "close", Close);

Expand Down
2 changes: 2 additions & 0 deletions src/js_stream.cc
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,8 @@ void JSStream::Initialize(Local<Object> target,
t->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "JSStream"));
t->InstanceTemplate()->SetInternalFieldCount(1);

env->SetProtoMethod(t, "getAsyncId", AsyncWrap::GetAsyncId);

env->SetProtoMethod(t, "doAlloc", DoAlloc);
env->SetProtoMethod(t, "doRead", DoRead);
env->SetProtoMethod(t, "doAfterWrite", DoAfterWrite);
Expand Down
3 changes: 3 additions & 0 deletions src/node_crypto.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2737,6 +2737,7 @@ void Connection::Initialize(Environment* env, Local<Object> target) {
t->InstanceTemplate()->SetInternalFieldCount(1);
t->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "Connection"));

env->SetProtoMethod(t, "getAsyncId", AsyncWrap::GetAsyncId);
env->SetProtoMethod(t, "encIn", Connection::EncIn);
env->SetProtoMethod(t, "clearOut", Connection::ClearOut);
env->SetProtoMethod(t, "clearIn", Connection::ClearIn);
Expand Down Expand Up @@ -6258,12 +6259,14 @@ void InitCrypto(Local<Object> target,

Local<FunctionTemplate> pb = FunctionTemplate::New(env->isolate());
pb->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "PBKDF2"));
env->SetProtoMethod(pb, "getAsyncId", AsyncWrap::GetAsyncId);
Local<ObjectTemplate> pbt = pb->InstanceTemplate();
pbt->SetInternalFieldCount(1);
env->set_pbkdf2_constructor_template(pbt);

Local<FunctionTemplate> rb = FunctionTemplate::New(env->isolate());
rb->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "RandomBytes"));
env->SetProtoMethod(rb, "getAsyncId", AsyncWrap::GetAsyncId);
Local<ObjectTemplate> rbt = rb->InstanceTemplate();
rbt->SetInternalFieldCount(1);
env->set_randombytes_constructor_template(rbt);
Expand Down
1 change: 1 addition & 0 deletions src/node_crypto.h
Original file line number Diff line number Diff line change
Expand Up @@ -408,6 +408,7 @@ class Connection : public AsyncWrap, public SSLWrap<Connection> {
bio_write_(nullptr),
hello_offset_(0) {
MakeWeak<Connection>(this);
Wrap(wrap, this);
hello_parser_.Start(SSLWrap<Connection>::OnClientHello,
OnClientHelloParseEnd,
this);
Expand Down
7 changes: 6 additions & 1 deletion src/node_file.cc
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,10 @@ class FSReqWrap: public ReqWrap<uv_fs_t> {
Wrap(object(), this);
}

~FSReqWrap() { ReleaseEarly(); }
~FSReqWrap() {
ReleaseEarly();
ClearWrap(object());
}

void* operator new(size_t size) = delete;
void* operator new(size_t size, char* storage) { return storage; }
Expand Down Expand Up @@ -151,6 +154,7 @@ void FSReqWrap::Dispose() {

void NewFSReqWrap(const FunctionCallbackInfo<Value>& args) {
CHECK(args.IsConstructCall());
ClearWrap(args.This());
}


Expand Down Expand Up @@ -1474,6 +1478,7 @@ void InitFs(Local<Object> target,
Local<FunctionTemplate> fst =
FunctionTemplate::New(env->isolate(), NewFSReqWrap);
fst->InstanceTemplate()->SetInternalFieldCount(1);
env->SetProtoMethod(fst, "getAsyncId", AsyncWrap::GetAsyncId);
fst->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "FSReqWrap"));
target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "FSReqWrap"),
fst->GetFunction());
Expand Down
1 change: 1 addition & 0 deletions src/node_http_parser.cc
Original file line number Diff line number Diff line change
Expand Up @@ -785,6 +785,7 @@ void InitHttpParser(Local<Object> target,
#undef V
target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "methods"), methods);

env->SetProtoMethod(t, "getAsyncId", AsyncWrap::GetAsyncId);
env->SetProtoMethod(t, "close", Parser::Close);
env->SetProtoMethod(t, "execute", Parser::Execute);
env->SetProtoMethod(t, "finish", Parser::Finish);
Expand Down
2 changes: 2 additions & 0 deletions src/node_stat_watcher.cc
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ void StatWatcher::Initialize(Environment* env, Local<Object> target) {
t->InstanceTemplate()->SetInternalFieldCount(1);
t->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "StatWatcher"));

env->SetProtoMethod(t, "getAsyncId", AsyncWrap::GetAsyncId);
env->SetProtoMethod(t, "start", StatWatcher::Start);
env->SetProtoMethod(t, "stop", StatWatcher::Stop);

Expand All @@ -66,6 +67,7 @@ StatWatcher::StatWatcher(Environment* env, Local<Object> wrap)
: AsyncWrap(env, wrap, AsyncWrap::PROVIDER_STATWATCHER),
watcher_(new uv_fs_poll_t) {
MakeWeak<StatWatcher>(this);
Wrap(wrap, this);
uv_fs_poll_init(env->event_loop(), watcher_);
watcher_->data = static_cast<void*>(this);
}
Expand Down
2 changes: 2 additions & 0 deletions src/node_zlib.cc
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ class ZCtx : public AsyncWrap {
refs_(0),
gzip_id_bytes_read_(0) {
MakeWeak<ZCtx>(this);
Wrap(wrap, this);
}


Expand Down Expand Up @@ -678,6 +679,7 @@ void InitZlib(Local<Object> target,

z->InstanceTemplate()->SetInternalFieldCount(1);

env->SetProtoMethod(z, "getAsyncId", AsyncWrap::GetAsyncId);
env->SetProtoMethod(z, "write", ZCtx::Write<true>);
env->SetProtoMethod(z, "writeSync", ZCtx::Write<false>);
env->SetProtoMethod(z, "init", ZCtx::Init);
Expand Down
4 changes: 4 additions & 0 deletions src/pipe_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,8 @@ void PipeWrap::Initialize(Local<Object> target,
t->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "Pipe"));
t->InstanceTemplate()->SetInternalFieldCount(1);

env->SetProtoMethod(t, "getAsyncId", AsyncWrap::GetAsyncId);

env->SetProtoMethod(t, "close", HandleWrap::Close);
env->SetProtoMethod(t, "unref", HandleWrap::Unref);
env->SetProtoMethod(t, "ref", HandleWrap::Ref);
Expand All @@ -95,9 +97,11 @@ void PipeWrap::Initialize(Local<Object> target,
// Create FunctionTemplate for PipeConnectWrap.
auto constructor = [](const FunctionCallbackInfo<Value>& args) {
CHECK(args.IsConstructCall());
ClearWrap(args.This());
};
auto cwt = FunctionTemplate::New(env->isolate(), constructor);
cwt->InstanceTemplate()->SetInternalFieldCount(1);
env->SetProtoMethod(cwt, "getAsyncId", AsyncWrap::GetAsyncId);
cwt->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "PipeConnectWrap"));
target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "PipeConnectWrap"),
cwt->GetFunction());
Expand Down
2 changes: 2 additions & 0 deletions src/process_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ class ProcessWrap : public HandleWrap {
constructor->InstanceTemplate()->SetInternalFieldCount(1);
constructor->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "Process"));

env->SetProtoMethod(constructor, "getAsyncId", AsyncWrap::GetAsyncId);

env->SetProtoMethod(constructor, "close", HandleWrap::Close);

env->SetProtoMethod(constructor, "spawn", Spawn);
Expand Down
1 change: 0 additions & 1 deletion src/req-wrap-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ template <typename T>
ReqWrap<T>::~ReqWrap() {
CHECK_EQ(req_.data, this); // Assert that someone has called Dispatched().
CHECK_EQ(false, persistent().IsEmpty());
ClearWrap(object());
persistent().Reset();
}

Expand Down
1 change: 1 addition & 0 deletions src/signal_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ class SignalWrap : public HandleWrap {
constructor->InstanceTemplate()->SetInternalFieldCount(1);
constructor->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "Signal"));

env->SetProtoMethod(constructor, "getAsyncId", AsyncWrap::GetAsyncId);
env->SetProtoMethod(constructor, "close", HandleWrap::Close);
env->SetProtoMethod(constructor, "ref", HandleWrap::Ref);
env->SetProtoMethod(constructor, "unref", HandleWrap::Unref);
Expand Down
8 changes: 8 additions & 0 deletions src/stream_base.h
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,10 @@ class ShutdownWrap : public ReqWrap<uv_shutdown_t>,
Wrap(req_wrap_obj, this);
}

~ShutdownWrap() {
ClearWrap(object());
}

static ShutdownWrap* from_req(uv_shutdown_t* req) {
return ContainerOf(&ShutdownWrap::req_, req);
}
Expand Down Expand Up @@ -98,6 +102,10 @@ class WriteWrap: public ReqWrap<uv_write_t>,
Wrap(obj, this);
}

~WriteWrap() {
ClearWrap(object());
}

void* operator new(size_t size) = delete;
void* operator new(size_t size, char* storage) { return storage; }

Expand Down
3 changes: 3 additions & 0 deletions src/stream_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -62,18 +62,21 @@ void StreamWrap::Initialize(Local<Object> target,
auto is_construct_call_callback =
[](const FunctionCallbackInfo<Value>& args) {
CHECK(args.IsConstructCall());
ClearWrap(args.This());
};
Local<FunctionTemplate> sw =
FunctionTemplate::New(env->isolate(), is_construct_call_callback);
sw->InstanceTemplate()->SetInternalFieldCount(1);
sw->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "ShutdownWrap"));
env->SetProtoMethod(sw, "getAsyncId", AsyncWrap::GetAsyncId);
target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "ShutdownWrap"),
sw->GetFunction());

Local<FunctionTemplate> ww =
FunctionTemplate::New(env->isolate(), is_construct_call_callback);
ww->InstanceTemplate()->SetInternalFieldCount(1);
ww->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "WriteWrap"));
env->SetProtoMethod(ww, "getAsyncId", AsyncWrap::GetAsyncId);
target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "WriteWrap"),
ww->GetFunction());
env->set_write_wrap_constructor_function(ww->GetFunction());
Expand Down
3 changes: 3 additions & 0 deletions src/tcp_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ void TCPWrap::Initialize(Local<Object> target,
"onconnection"),
Null(env->isolate()));

env->SetProtoMethod(t, "getAsyncId", AsyncWrap::GetAsyncId);

env->SetProtoMethod(t, "close", HandleWrap::Close);

Expand Down Expand Up @@ -116,9 +117,11 @@ void TCPWrap::Initialize(Local<Object> target,
// Create FunctionTemplate for TCPConnectWrap.
auto constructor = [](const FunctionCallbackInfo<Value>& args) {
CHECK(args.IsConstructCall());
ClearWrap(args.This());
};
auto cwt = FunctionTemplate::New(env->isolate(), constructor);
cwt->InstanceTemplate()->SetInternalFieldCount(1);
env->SetProtoMethod(cwt, "getAsyncId", AsyncWrap::GetAsyncId);
cwt->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "TCPConnectWrap"));
target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "TCPConnectWrap"),
cwt->GetFunction());
Expand Down
2 changes: 2 additions & 0 deletions src/timer_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,8 @@ class TimerWrap : public HandleWrap {

env->SetTemplateMethod(constructor, "now", Now);

env->SetProtoMethod(constructor, "getAsyncId", AsyncWrap::GetAsyncId);

env->SetProtoMethod(constructor, "close", HandleWrap::Close);
env->SetProtoMethod(constructor, "ref", HandleWrap::Ref);
env->SetProtoMethod(constructor, "unref", HandleWrap::Unref);
Expand Down
1 change: 1 addition & 0 deletions src/tls_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -939,6 +939,7 @@ void TLSWrap::Initialize(Local<Object> target,
t->InstanceTemplate()->SetInternalFieldCount(1);
t->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "TLSWrap"));

env->SetProtoMethod(t, "getAsyncId", AsyncWrap::GetAsyncId);
env->SetProtoMethod(t, "receive", Receive);
env->SetProtoMethod(t, "start", Start);
env->SetProtoMethod(t, "setVerifyMode", SetVerifyMode);
Expand Down
2 changes: 2 additions & 0 deletions src/tty_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@ void TTYWrap::Initialize(Local<Object> target,
t->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "TTY"));
t->InstanceTemplate()->SetInternalFieldCount(1);

env->SetProtoMethod(t, "getAsyncId", AsyncWrap::GetAsyncId);

env->SetProtoMethod(t, "close", HandleWrap::Close);
env->SetProtoMethod(t, "unref", HandleWrap::Unref);
env->SetProtoMethod(t, "ref", HandleWrap::Ref);
Expand Down
Loading

0 comments on commit 8130199

Please sign in to comment.