Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit c6d5af5

Browse filesBrowse files
addaleaxrvagg
addaleax
authored and
rvagg
committed
src: move req_wrap_queue to base class of ReqWrap
Introduce a second base class for `ReqWrap` that does not depend on a template parameter and move the `req_wrap_queue_` field to it. This addresses undefined behaviour that occurs when casting to `ReqWrap<uv_req_t>` in the `ReqWrap` constructor. Refs: #26131 PR-URL: #26148 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
1 parent 007b2fa commit c6d5af5
Copy full SHA for c6d5af5

File tree

Expand file treeCollapse file tree

6 files changed

+39
-18
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

6 files changed

+39
-18
lines changed
Open diff view settings
Collapse file

‎src/env.cc‎

Copy file name to clipboardExpand all lines: src/env.cc
+1-1Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -420,7 +420,7 @@ void Environment::RegisterHandleCleanups() {
420420
}
421421

422422
void Environment::CleanupHandles() {
423-
for (ReqWrap<uv_req_t>* request : req_wrap_queue_)
423+
for (ReqWrapBase* request : req_wrap_queue_)
424424
request->Cancel();
425425

426426
for (HandleWrap* handle : handle_wrap_queue_)
Collapse file

‎src/env.h‎

Copy file name to clipboardExpand all lines: src/env.h
+1-2Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -877,8 +877,7 @@ class Environment {
877877
#endif
878878

879879
typedef ListHead<HandleWrap, &HandleWrap::handle_wrap_queue_> HandleWrapQueue;
880-
typedef ListHead<ReqWrap<uv_req_t>, &ReqWrap<uv_req_t>::req_wrap_queue_>
881-
ReqWrapQueue;
880+
typedef ListHead<ReqWrapBase, &ReqWrapBase::req_wrap_queue_> ReqWrapQueue;
882881

883882
inline HandleWrapQueue* handle_wrap_queue() { return &handle_wrap_queue_; }
884883
inline ReqWrapQueue* req_wrap_queue() { return &req_wrap_queue_; }
Collapse file

‎src/node_postmortem_metadata.cc‎

Copy file name to clipboardExpand all lines: src/node_postmortem_metadata.cc
+5-3Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,15 +28,14 @@
2828
V(Environment_HandleWrapQueue, head_, ListNode_HandleWrap, \
2929
Environment::HandleWrapQueue::head_) \
3030
V(ListNode_HandleWrap, next_, uintptr_t, ListNode<HandleWrap>::next_) \
31-
V(ReqWrap, req_wrap_queue_, ListNode_ReqWrapQueue, \
32-
ReqWrap<uv_req_t>::req_wrap_queue_) \
3331
V(Environment_ReqWrapQueue, head_, ListNode_ReqWrapQueue, \
3432
Environment::ReqWrapQueue::head_) \
35-
V(ListNode_ReqWrap, next_, uintptr_t, ListNode<ReqWrap<uv_req_t>>::next_)
33+
V(ListNode_ReqWrap, next_, uintptr_t, ListNode<ReqWrapBase>::next_)
3634

3735
extern "C" {
3836
int nodedbg_const_ContextEmbedderIndex__kEnvironment__int;
3937
uintptr_t nodedbg_offset_ExternalString__data__uintptr_t;
38+
uintptr_t nodedbg_offset_ReqWrap__req_wrap_queue___ListNode_ReqWrapQueue;
4039

4140
#define V(Class, Member, Type, Accessor) \
4241
NODE_EXTERN uintptr_t NODEDBG_OFFSET(Class, Member, Type);
@@ -51,6 +50,9 @@ int GenDebugSymbols() {
5150
ContextEmbedderIndex::kEnvironment;
5251

5352
nodedbg_offset_ExternalString__data__uintptr_t = NODE_OFF_EXTSTR_DATA;
53+
nodedbg_offset_ReqWrap__req_wrap_queue___ListNode_ReqWrapQueue =
54+
OffsetOf<ListNode<ReqWrapBase>, ReqWrap<uv_req_t>>(
55+
&ReqWrap<uv_req_t>::req_wrap_queue_);
5456

5557
#define V(Class, Member, Type, Accessor) \
5658
NODEDBG_OFFSET(Class, Member, Type) = OffsetOf(&Accessor);
Collapse file

‎src/node_process_methods.cc‎

Copy file name to clipboardExpand all lines: src/node_process_methods.cc
+2-1Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -252,7 +252,8 @@ static void GetActiveRequests(const FunctionCallbackInfo<Value>& args) {
252252
Environment* env = Environment::GetCurrent(args);
253253

254254
std::vector<Local<Value>> request_v;
255-
for (auto w : *env->req_wrap_queue()) {
255+
for (ReqWrapBase* req_wrap : *env->req_wrap_queue()) {
256+
AsyncWrap* w = req_wrap->GetAsyncWrap();
256257
if (w->persistent().IsEmpty())
257258
continue;
258259
request_v.push_back(w->GetOwner());
Collapse file

‎src/req_wrap-inl.h‎

Copy file name to clipboardExpand all lines: src/req_wrap-inl.h
+11-6Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -11,16 +11,16 @@
1111

1212
namespace node {
1313

14+
ReqWrapBase::ReqWrapBase(Environment* env) {
15+
env->req_wrap_queue()->PushBack(this);
16+
}
17+
1418
template <typename T>
1519
ReqWrap<T>::ReqWrap(Environment* env,
1620
v8::Local<v8::Object> object,
1721
AsyncWrap::ProviderType provider)
18-
: AsyncWrap(env, object, provider) {
19-
20-
// FIXME(bnoordhuis) The fact that a reinterpret_cast is needed is
21-
// arguably a good indicator that there should be more than one queue.
22-
env->req_wrap_queue()->PushBack(reinterpret_cast<ReqWrap<uv_req_t>*>(this));
23-
22+
: AsyncWrap(env, object, provider),
23+
ReqWrapBase(env) {
2424
Reset();
2525
}
2626

@@ -51,6 +51,11 @@ void ReqWrap<T>::Cancel() {
5151
uv_cancel(reinterpret_cast<uv_req_t*>(&req_));
5252
}
5353

54+
template <typename T>
55+
AsyncWrap* ReqWrap<T>::GetAsyncWrap() {
56+
return this;
57+
}
58+
5459
// Below is dark template magic designed to invoke libuv functions that
5560
// initialize uv_req_t instances in a unified fashion, to allow easier
5661
// tracking of active/inactive requests.
Collapse file

‎src/req_wrap.h‎

Copy file name to clipboardExpand all lines: src/req_wrap.h
+19-5Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,24 @@
1010

1111
namespace node {
1212

13+
class ReqWrapBase {
14+
public:
15+
explicit inline ReqWrapBase(Environment* env);
16+
17+
virtual ~ReqWrapBase() {}
18+
19+
virtual void Cancel() = 0;
20+
virtual AsyncWrap* GetAsyncWrap() = 0;
21+
22+
private:
23+
friend int GenDebugSymbols();
24+
friend class Environment;
25+
26+
ListNode<ReqWrapBase> req_wrap_queue_;
27+
};
28+
1329
template <typename T>
14-
class ReqWrap : public AsyncWrap {
30+
class ReqWrap : public AsyncWrap, public ReqWrapBase {
1531
public:
1632
inline ReqWrap(Environment* env,
1733
v8::Local<v8::Object> object,
@@ -23,21 +39,19 @@ class ReqWrap : public AsyncWrap {
2339
// Call this after a request has finished, if re-using this object is planned.
2440
inline void Reset();
2541
T* req() { return &req_; }
26-
inline void Cancel();
42+
inline void Cancel() final;
43+
inline AsyncWrap* GetAsyncWrap() override;
2744

2845
static ReqWrap* from_req(T* req);
2946

3047
template <typename LibuvFunction, typename... Args>
3148
inline int Dispatch(LibuvFunction fn, Args... args);
3249

3350
private:
34-
friend class Environment;
3551
friend int GenDebugSymbols();
3652
template <typename ReqT, typename U>
3753
friend struct MakeLibuvRequestCallback;
3854

39-
ListNode<ReqWrap> req_wrap_queue_;
40-
4155
typedef void (*callback_t)();
4256
callback_t original_callback_ = nullptr;
4357

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.