Skip to content

Commit

Permalink
src: move req_wrap_queue to base class of ReqWrap
Browse files Browse the repository at this point in the history
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: nodejs#26131
  • Loading branch information
addaleax committed Feb 16, 2019
1 parent e74019f commit e9abc90
Show file tree
Hide file tree
Showing 6 changed files with 39 additions and 18 deletions.
2 changes: 1 addition & 1 deletion src/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -410,7 +410,7 @@ void Environment::RegisterHandleCleanups() {
}

void Environment::CleanupHandles() {
for (ReqWrap<uv_req_t>* request : req_wrap_queue_)
for (ReqWrapBase* request : req_wrap_queue_)
request->Cancel();

for (HandleWrap* handle : handle_wrap_queue_)
Expand Down
3 changes: 1 addition & 2 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -860,8 +860,7 @@ class Environment {
#endif

typedef ListHead<HandleWrap, &HandleWrap::handle_wrap_queue_> HandleWrapQueue;
typedef ListHead<ReqWrap<uv_req_t>, &ReqWrap<uv_req_t>::req_wrap_queue_>
ReqWrapQueue;
typedef ListHead<ReqWrapBase, &ReqWrapBase::req_wrap_queue_> ReqWrapQueue;

inline HandleWrapQueue* handle_wrap_queue() { return &handle_wrap_queue_; }
inline ReqWrapQueue* req_wrap_queue() { return &req_wrap_queue_; }
Expand Down
8 changes: 5 additions & 3 deletions src/node_postmortem_metadata.cc
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,14 @@
V(Environment_HandleWrapQueue, head_, ListNode_HandleWrap, \
Environment::HandleWrapQueue::head_) \
V(ListNode_HandleWrap, next_, uintptr_t, ListNode<HandleWrap>::next_) \
V(ReqWrap, req_wrap_queue_, ListNode_ReqWrapQueue, \
ReqWrap<uv_req_t>::req_wrap_queue_) \
V(Environment_ReqWrapQueue, head_, ListNode_ReqWrapQueue, \
Environment::ReqWrapQueue::head_) \
V(ListNode_ReqWrap, next_, uintptr_t, ListNode<ReqWrap<uv_req_t>>::next_)
V(ListNode_ReqWrap, next_, uintptr_t, ListNode<ReqWrapBase>::next_)

extern "C" {
int nodedbg_const_ContextEmbedderIndex__kEnvironment__int;
uintptr_t nodedbg_offset_ExternalString__data__uintptr_t;
uintptr_t nodedbg_offset_ReqWrap__req_wrap_queue___ListNode_ReqWrapQueue;

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

nodedbg_offset_ExternalString__data__uintptr_t = NODE_OFF_EXTSTR_DATA;
nodedbg_offset_ReqWrap__req_wrap_queue___ListNode_ReqWrapQueue =
OffsetOf<ListNode<ReqWrapBase>, ReqWrap<uv_req_t>>(
&ReqWrap<uv_req_t>::req_wrap_queue_);

#define V(Class, Member, Type, Accessor) \
NODEDBG_OFFSET(Class, Member, Type) = OffsetOf(&Accessor);
Expand Down
3 changes: 2 additions & 1 deletion src/node_process_methods.cc
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,8 @@ static void GetActiveRequests(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);

std::vector<Local<Value>> request_v;
for (auto w : *env->req_wrap_queue()) {
for (ReqWrapBase* req_wrap : *env->req_wrap_queue()) {
AsyncWrap* w = req_wrap->GetAsyncWrap();
if (w->persistent().IsEmpty())
continue;
request_v.push_back(w->GetOwner());
Expand Down
17 changes: 11 additions & 6 deletions src/req_wrap-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -11,16 +11,16 @@

namespace node {

ReqWrapBase::ReqWrapBase(Environment* env) {
env->req_wrap_queue()->PushBack(this);
}

template <typename T>
ReqWrap<T>::ReqWrap(Environment* env,
v8::Local<v8::Object> object,
AsyncWrap::ProviderType provider)
: AsyncWrap(env, object, provider) {

// FIXME(bnoordhuis) The fact that a reinterpret_cast is needed is
// arguably a good indicator that there should be more than one queue.
env->req_wrap_queue()->PushBack(reinterpret_cast<ReqWrap<uv_req_t>*>(this));

: AsyncWrap(env, object, provider),
ReqWrapBase(env) {
Reset();
}

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

template <typename T>
AsyncWrap* ReqWrap<T>::GetAsyncWrap() {
return this;
}

// Below is dark template magic designed to invoke libuv functions that
// initialize uv_req_t instances in a unified fashion, to allow easier
// tracking of active/inactive requests.
Expand Down
24 changes: 19 additions & 5 deletions src/req_wrap.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,24 @@

namespace node {

class ReqWrapBase {
public:
explicit inline ReqWrapBase(Environment* env);

virtual ~ReqWrapBase() {}

virtual void Cancel() = 0;
virtual AsyncWrap* GetAsyncWrap() = 0;

private:
friend int GenDebugSymbols();
friend class Environment;

ListNode<ReqWrapBase> req_wrap_queue_;
};

template <typename T>
class ReqWrap : public AsyncWrap {
class ReqWrap : public AsyncWrap, public ReqWrapBase {
public:
inline ReqWrap(Environment* env,
v8::Local<v8::Object> object,
Expand All @@ -23,21 +39,19 @@ class ReqWrap : public AsyncWrap {
// Call this after a request has finished, if re-using this object is planned.
inline void Reset();
T* req() { return &req_; }
inline void Cancel();
inline void Cancel() final;
inline AsyncWrap* GetAsyncWrap() override;

static ReqWrap* from_req(T* req);

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

private:
friend class Environment;
friend int GenDebugSymbols();
template <typename ReqT, typename U>
friend struct MakeLibuvRequestCallback;

ListNode<ReqWrap> req_wrap_queue_;

typedef void (*callback_t)();
callback_t original_callback_ = nullptr;

Expand Down

0 comments on commit e9abc90

Please sign in to comment.