Skip to content

Commit fe2df3b

Browse files
trevnorrisaddaleax
authored andcommitted
async_wrap,src: add GetAsyncId() method
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: #12892 Ref: #11883 Ref: #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>
1 parent f1ed19d commit fe2df3b

25 files changed

+326
-2
lines changed

src/async-wrap.cc

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -180,6 +180,14 @@ static void SetupHooks(const FunctionCallbackInfo<Value>& args) {
180180
}
181181

182182

183+
void AsyncWrap::GetAsyncId(const FunctionCallbackInfo<Value>& args) {
184+
AsyncWrap* wrap;
185+
args.GetReturnValue().Set(-1);
186+
ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder());
187+
args.GetReturnValue().Set(wrap->get_id());
188+
}
189+
190+
183191
void AsyncWrap::Initialize(Local<Object> target,
184192
Local<Value> unused,
185193
Local<Context> context) {

src/async-wrap.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,8 @@ class AsyncWrap : public BaseObject {
8585
v8::Local<v8::Value> unused,
8686
v8::Local<v8::Context> context);
8787

88+
static void GetAsyncId(const v8::FunctionCallbackInfo<v8::Value>& args);
89+
8890
static void DestroyIdsCb(uv_idle_t* handle);
8991

9092
inline ProviderType provider_type() const;

src/cares_wrap.cc

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,7 @@ inline const char* ToErrorCodeString(int status) {
104104
class GetAddrInfoReqWrap : public ReqWrap<uv_getaddrinfo_t> {
105105
public:
106106
GetAddrInfoReqWrap(Environment* env, Local<Object> req_wrap_obj);
107+
~GetAddrInfoReqWrap();
107108

108109
size_t self_size() const override { return sizeof(*this); }
109110
};
@@ -114,10 +115,15 @@ GetAddrInfoReqWrap::GetAddrInfoReqWrap(Environment* env,
114115
Wrap(req_wrap_obj, this);
115116
}
116117

118+
GetAddrInfoReqWrap::~GetAddrInfoReqWrap() {
119+
ClearWrap(object());
120+
}
121+
117122

118123
class GetNameInfoReqWrap : public ReqWrap<uv_getnameinfo_t> {
119124
public:
120125
GetNameInfoReqWrap(Environment* env, Local<Object> req_wrap_obj);
126+
~GetNameInfoReqWrap();
121127

122128
size_t self_size() const override { return sizeof(*this); }
123129
};
@@ -128,6 +134,10 @@ GetNameInfoReqWrap::GetNameInfoReqWrap(Environment* env,
128134
Wrap(req_wrap_obj, this);
129135
}
130136

137+
GetNameInfoReqWrap::~GetNameInfoReqWrap() {
138+
ClearWrap(object());
139+
}
140+
131141

132142
int cmp_ares_tasks(const node_ares_task* a, const node_ares_task* b) {
133143
if (a->sock < b->sock)
@@ -293,6 +303,7 @@ class QueryWrap : public AsyncWrap {
293303
: AsyncWrap(env, req_wrap_obj, AsyncWrap::PROVIDER_QUERYWRAP) {
294304
if (env->in_domain())
295305
req_wrap_obj->Set(env->domain_string(), env->domain_array()->Get(0));
306+
Wrap(req_wrap_obj, this);
296307
}
297308

298309
~QueryWrap() override {
@@ -1388,10 +1399,12 @@ void Initialize(Local<Object> target,
13881399
auto is_construct_call_callback =
13891400
[](const FunctionCallbackInfo<Value>& args) {
13901401
CHECK(args.IsConstructCall());
1402+
ClearWrap(args.This());
13911403
};
13921404
Local<FunctionTemplate> aiw =
13931405
FunctionTemplate::New(env->isolate(), is_construct_call_callback);
13941406
aiw->InstanceTemplate()->SetInternalFieldCount(1);
1407+
env->SetProtoMethod(aiw, "getAsyncId", AsyncWrap::GetAsyncId);
13951408
aiw->SetClassName(
13961409
FIXED_ONE_BYTE_STRING(env->isolate(), "GetAddrInfoReqWrap"));
13971410
target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "GetAddrInfoReqWrap"),
@@ -1400,6 +1413,7 @@ void Initialize(Local<Object> target,
14001413
Local<FunctionTemplate> niw =
14011414
FunctionTemplate::New(env->isolate(), is_construct_call_callback);
14021415
niw->InstanceTemplate()->SetInternalFieldCount(1);
1416+
env->SetProtoMethod(niw, "getAsyncId", AsyncWrap::GetAsyncId);
14031417
niw->SetClassName(
14041418
FIXED_ONE_BYTE_STRING(env->isolate(), "GetNameInfoReqWrap"));
14051419
target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "GetNameInfoReqWrap"),
@@ -1408,6 +1422,7 @@ void Initialize(Local<Object> target,
14081422
Local<FunctionTemplate> qrw =
14091423
FunctionTemplate::New(env->isolate(), is_construct_call_callback);
14101424
qrw->InstanceTemplate()->SetInternalFieldCount(1);
1425+
env->SetProtoMethod(qrw, "getAsyncId", AsyncWrap::GetAsyncId);
14111426
qrw->SetClassName(
14121427
FIXED_ONE_BYTE_STRING(env->isolate(), "QueryReqWrap"));
14131428
target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "QueryReqWrap"),

src/connect_wrap.cc

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,4 +19,9 @@ ConnectWrap::ConnectWrap(Environment* env,
1919
Wrap(req_wrap_obj, this);
2020
}
2121

22+
23+
ConnectWrap::~ConnectWrap() {
24+
ClearWrap(object());
25+
}
26+
2227
} // namespace node

src/connect_wrap.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ class ConnectWrap : public ReqWrap<uv_connect_t> {
1515
ConnectWrap(Environment* env,
1616
v8::Local<v8::Object> req_wrap_obj,
1717
AsyncWrap::ProviderType provider);
18+
~ConnectWrap();
1819

1920
size_t self_size() const override { return sizeof(*this); }
2021
};

src/fs_event_wrap.cc

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,7 @@ void FSEventWrap::Initialize(Local<Object> target,
9494
t->InstanceTemplate()->SetInternalFieldCount(1);
9595
t->SetClassName(fsevent_string);
9696

97+
env->SetProtoMethod(t, "getAsyncId", AsyncWrap::GetAsyncId);
9798
env->SetProtoMethod(t, "start", Start);
9899
env->SetProtoMethod(t, "close", Close);
99100

src/js_stream.cc

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -221,6 +221,8 @@ void JSStream::Initialize(Local<Object> target,
221221
t->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "JSStream"));
222222
t->InstanceTemplate()->SetInternalFieldCount(1);
223223

224+
env->SetProtoMethod(t, "getAsyncId", AsyncWrap::GetAsyncId);
225+
224226
env->SetProtoMethod(t, "doAlloc", DoAlloc);
225227
env->SetProtoMethod(t, "doRead", DoRead);
226228
env->SetProtoMethod(t, "doAfterWrite", DoAfterWrite);

src/node_crypto.cc

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2737,6 +2737,7 @@ void Connection::Initialize(Environment* env, Local<Object> target) {
27372737
t->InstanceTemplate()->SetInternalFieldCount(1);
27382738
t->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "Connection"));
27392739

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

62596260
Local<FunctionTemplate> pb = FunctionTemplate::New(env->isolate());
62606261
pb->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "PBKDF2"));
6262+
env->SetProtoMethod(pb, "getAsyncId", AsyncWrap::GetAsyncId);
62616263
Local<ObjectTemplate> pbt = pb->InstanceTemplate();
62626264
pbt->SetInternalFieldCount(1);
62636265
env->set_pbkdf2_constructor_template(pbt);
62646266

62656267
Local<FunctionTemplate> rb = FunctionTemplate::New(env->isolate());
62666268
rb->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "RandomBytes"));
6269+
env->SetProtoMethod(rb, "getAsyncId", AsyncWrap::GetAsyncId);
62676270
Local<ObjectTemplate> rbt = rb->InstanceTemplate();
62686271
rbt->SetInternalFieldCount(1);
62696272
env->set_randombytes_constructor_template(rbt);

src/node_crypto.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -408,6 +408,7 @@ class Connection : public AsyncWrap, public SSLWrap<Connection> {
408408
bio_write_(nullptr),
409409
hello_offset_(0) {
410410
MakeWeak<Connection>(this);
411+
Wrap(wrap, this);
411412
hello_parser_.Start(SSLWrap<Connection>::OnClientHello,
412413
OnClientHelloParseEnd,
413414
this);

src/node_file.cc

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,10 @@ class FSReqWrap: public ReqWrap<uv_fs_t> {
110110
Wrap(object(), this);
111111
}
112112

113-
~FSReqWrap() { ReleaseEarly(); }
113+
~FSReqWrap() {
114+
ReleaseEarly();
115+
ClearWrap(object());
116+
}
114117

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

152155
void NewFSReqWrap(const FunctionCallbackInfo<Value>& args) {
153156
CHECK(args.IsConstructCall());
157+
ClearWrap(args.This());
154158
}
155159

156160

@@ -1474,6 +1478,7 @@ void InitFs(Local<Object> target,
14741478
Local<FunctionTemplate> fst =
14751479
FunctionTemplate::New(env->isolate(), NewFSReqWrap);
14761480
fst->InstanceTemplate()->SetInternalFieldCount(1);
1481+
env->SetProtoMethod(fst, "getAsyncId", AsyncWrap::GetAsyncId);
14771482
fst->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "FSReqWrap"));
14781483
target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "FSReqWrap"),
14791484
fst->GetFunction());

0 commit comments

Comments
 (0)